credentials: Apply defaults to TLS configs provided through GetConfigForClient#7754
Conversation
arjan-bal
force-pushed
the
defaults-in-get-config-for-client
branch
from
b3de47d to
87016c7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7754 +/- ##
==========================================
+ Coverage 79.70% 79.81% +0.11%
==========================================
Files 365 365
Lines 36383 36394 +11
==========================================
+ Hits 29000 29049 +49
+ Misses 6183 6154 -29
+ Partials 1200 1191 -9
|
arjan-bal
added
the
Type: Behavior Change
There was a problem hiding this comment.
Thanks for the PR!
I think this is good, NewTLS should (generally) support the features of the tls.Config where we can and it makes sense. WDYT Matt?
I haven't done a pass on the test file yet, I'll go through that when we are sure we want to add this handling in general.
| func NewTLS(c *tls.Config) TransportCredentials { | ||
| tc := &tlsCreds{credinternal.CloneTLSConfig(c)} | ||
| tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos) | ||
| cfg := applyDefaults(c) |
There was a problem hiding this comment.
Don't shorten names - cfg -> config
There was a problem hiding this comment.
Changed as suggested.
| return applyDefaults(cfgForClient), nil | ||
| } | ||
| } | ||
| tc := &tlsCreds{config: cfg} |
There was a problem hiding this comment.
removed the temporary variable an inlined the return.
|
Also, regarding the release notes, let's specify that this is specifically for credentials: TLS credentials created via |
Updated, thanks for the suggestion. |
gtcooke94
left a comment
gtcooke94
left a comment
There was a problem hiding this comment.
Just that one test structure question, otherwise LGTM
arjan-bal
added
Area: Auth
5 participants
Fixes: #7709
TLS server configs provided at TLS handshake time through
GetConfigForClientwere not modified by grpc-go to ensure secure defaults and a valid ALPN configuration. This PR applies the same handling applied to static TLS configs to the configs obtained byGetConfigForClient.RELEASE NOTES:
NewTLSthat usetls.Config.GetConfigForClientwill now have necessary gRPC-Go defaults, namely CipherSuites, supported TLS versions and ALPN configured automatically. This handling was already present for configs not using the GetConfigForClient function.