-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added New Sections Related To Memory Management, Communication Security and Process Management #91
base: master
Are you sure you want to change the base?
Conversation
pypalkar23
commented
Dec 11, 2022
- In the Memory Management section, added a new subsection related to memory leakage scenarios.
- In Communication Security, a new subsection related to gRPC(Google Remote Procedure Call) has been added. To provide greater clarity on the topic code snippet has been added to a subdirectory under the Communication Security subdirectory.
- An entirely new section of Process Management has been added to the book. The section deals with scenarios that may result in zombie/dangling Goroutines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pypalkar23, thanks for bringing up this topics!
That's my first time reviewing PRs in this repo, I hope it's fine with the maintainers.
Left few comments to the gRPC related sections.
import ( | ||
"context" | ||
"crypto/tls" | ||
"crypto/x509" | ||
"flag" | ||
"fmt" | ||
"io/ioutil" | ||
"log" | ||
pb "pentest/grpc/samplebuff" | ||
"time" | ||
|
||
"google.golang.org/grpc" | ||
"google.golang.org/grpc/credentials" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fmt
import ( | |
"context" | |
"crypto/tls" | |
"crypto/x509" | |
"flag" | |
"fmt" | |
"io/ioutil" | |
"log" | |
pb "pentest/grpc/samplebuff" | |
"time" | |
"google.golang.org/grpc" | |
"google.golang.org/grpc/credentials" | |
) | |
import ( | |
"context" | |
"crypto/tls" | |
"crypto/x509" | |
"flag" | |
"fmt" | |
"io/ioutil" | |
"log" | |
"time" | |
pb "pentest/grpc/samplebuff" | |
"google.golang.org/grpc" | |
"google.golang.org/grpc/credentials" | |
) |
defer cancel() | ||
r, err := c.Greet(ctx, &pb.SendMsg{Name: *name}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fmt
defer cancel() | |
r, err := c.Greet(ctx, &pb.SendMsg{Name: *name}) | |
defer cancel() | |
r, err := c.Greet(ctx, &pb.SendMsg{Name: *name}) |
|
||
func main() { | ||
flag.Parse() | ||
b, _ := ioutil.ReadFile("../cert/ca.cert") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ioutil
package is deprecated since go1.16. In this case we should use os.ReadFile
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: let's promote best practice of not skipping errors
b, _ := ioutil.ReadFile("../cert/ca.cert") | |
b, err := ioutil.ReadFile("../cert/ca.cert") | |
if err != nil { | |
log.Fatalf("Could read ca.cert: %v", err) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anatolym Shouldn't the error message be "Couldn't read ca.cert: %v", instead?
//Configuring the certificates | ||
creds, err := credentials.NewServerTLSFromFile("../cert/service.pem", "../cert/service.key") | ||
|
||
if err != nil { | ||
log.Fatalf("TLS setup failed: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fmt
//Configuring the certificates | |
creds, err := credentials.NewServerTLSFromFile("../cert/service.pem", "../cert/service.key") | |
if err != nil { | |
log.Fatalf("TLS setup failed: %v", err) | |
} | |
// Configuring the certificates. | |
creds, err := credentials.NewServerTLSFromFile("../cert/service.pem", "../cert/service.key") | |
if err != nil { | |
log.Fatalf("TLS setup failed: %v", err) | |
} |
@@ -0,0 +1,18 @@ | |||
syntax = "proto3"; | |||
|
|||
option go_package = "github.com/pypalkar23/go-rpc-cis5209/sample"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps, this name should be changed to something else like pentest/grpc/samplebuff
?
// server is used to implement sample.GreeterServer. | ||
type server struct { | ||
pb.UnimplementedSampleServiceServer | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would suggest not embedding gRPC service interface to a structure like this. The issue is that this implicitly says the server
struct implements the interface, and the program compiles with no issues. Now imaging we remove server.Greet
method. The program should still compile, but now it panics in runtime when Greet
is called. (it will panic because of nil receiver)
Instead, we can enforce interface implementation check on the compile time like this.
// server is used to implement sample.GreeterServer. | |
type server struct { | |
pb.UnimplementedSampleServiceServer | |
} | |
// Verify interface implementation on compile. | |
var _ pb.UnimplementedSampleServiceServer = (*server)(nil) | |
// server implements sample.GreeterServer interface. | |
type server struct {} |
Note, the interface implementation check will be implicitly performed by compiler by the pb.RegisterSampleServiceServer(s, &server{})
call below. This is ok but explicit check with var _ ...
is more obvious for a reader.
|
||
### Secure gRPC Server: | ||
The complete code can be found [here][2]. | ||
```go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: reminder, we should update the examples if the above comments are accepted
|
||
Let's look at how we can implement secure gRPC calls using TLS Encryption on both client and server side. | ||
|
||
### Secure gRPC Server: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: fmt here and in other MD files, in general titles and subtitles shouldn't end with punctuation marks (incl :
)
### Secure gRPC Server: | |
### Secure gRPC Server |
@pypalkar23 and @anatolym thank you both for the great contributions and sorry for the late reply. @pypalkar23 can you please consider @anatolym suggestions? @anatolym did you have the chance to review the other suggestions changes/additions? Cheers, |