-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
credentials/tls: reject connections with ALPN disabled #7184
Conversation
98033c3
to
514de04
Compare
514de04
to
6fb5651
Compare
6fb5651
to
a49fb8f
Compare
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.
LGTM overall! Thanks for picking this up
credentials/tls_ext_test.go
Outdated
|
||
matchPat, err := regexp.Compile(tc.wantErrMatchPattern) | ||
if err != nil { | ||
t.Fatalf("Error message match pattern %q is invalid due to error: %v", tc.wantErrMatchPattern, 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.
it should be changed to got
before want
? Similar for line 328
https://google.github.io/styleguide/go/decisions#got-before-want
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.
The regex pattern is expected to always be valid, the want is implicitly nil
. There is not want
in this message. If this line executes, it means that the test case is invalid.
credentials/tls_ext_test.go
Outdated
t.Fatalf("Error starting server: %v", err) | ||
} | ||
|
||
errCh := make(chan error) |
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.
This one needs a buffer of size 1 too.
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.
Changed.
@dfawley : For second set of eyes. |
Fixes #434
Since HTTP/2 mandates the use of ALPN during the TLS handshake to establish connections, gRPC clients will reject connections to servers that don't have ALPN enabled. gRPC servers will also reject TLS connections from clients with ALPN disabled.
As this change can break existing clients, this behaviour is guarded by an environment variable flag. The error message returned is similar to the message returned by the C++ implementation.
RELEASE NOTES: