Skip to content
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

mTLS authentication w/ tests #4

Merged
merged 11 commits into from
Oct 29, 2020
Merged

mTLS authentication w/ tests #4

merged 11 commits into from
Oct 29, 2020

Conversation

andrejtokarcik
Copy link
Owner

No description provided.

Copy link
Collaborator

@bernardjkim bernardjkim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Would be nice to see more documentation and comments for exported functions and structs.

)

func init() {
flag.IntVar(&grpcPort, "grpc-port", 50051, "Port to expose the gRPC server on")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Define default port number constant since it is used by both server and client.

tc := validTestCase()
tc.clientCredsFiles.Cert = "self-signed.crt"
tc.clientCredsFiles.Key = "self-signed.key"
tc.expectedErr = errors.New("context deadline exceeded")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to improve the error handling for this case? Receiving a 'context deadline exceeded' here might not be too helpful in figuring out what went wrong.

Copy link
Owner Author

@andrejtokarcik andrejtokarcik Oct 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only option I see is to somehow inspect the logs emitted by the grpc framework. In the returned error object itself, the more precise message seems to be just a "connection error" or "connection closed".

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does grpc.WithBlock() (or some other dial option) help catch the TLS error early?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. I already use grpc.WithReturnConnectionError() which implies grpc.WithBlock() and is even more helpful in this regard.

cmd/client/main.go Show resolved Hide resolved
cmd/client/main.go Outdated Show resolved Hide resolved
cmd/client/main.go Show resolved Hide resolved
server/authn.go Outdated Show resolved Hide resolved
test/data/x509/Makefile.local Outdated Show resolved Hide resolved
client/client.go Outdated Show resolved Hide resolved
cmd/client/main.go Outdated Show resolved Hide resolved
cmd/client/main.go Outdated Show resolved Hide resolved
tc := validTestCase()
tc.clientCredsFiles.Cert = "self-signed.crt"
tc.clientCredsFiles.Key = "self-signed.key"
tc.expectedErr = errors.New("context deadline exceeded")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does grpc.WithBlock() (or some other dial option) help catch the TLS error early?

test/bufconn.go Outdated Show resolved Hide resolved
test/bufconn.go Outdated Show resolved Hide resolved
test/data/path.go Outdated Show resolved Hide resolved
@andrejtokarcik andrejtokarcik merged commit 5526e13 into master Oct 29, 2020
@andrejtokarcik andrejtokarcik deleted the feat/authn branch October 29, 2020 00:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants