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

credentials: Apply defaults to TLS configs provided through GetConfigForClient #7754

Merged
merged 6 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 23 additions & 7 deletions credentials/tls.go
Original file line number Diff line number Diff line change
Expand Up @@ -200,25 +200,41 @@

// NewTLS uses c to construct a TransportCredentials based on TLS.
func NewTLS(c *tls.Config) TransportCredentials {
tc := &tlsCreds{credinternal.CloneTLSConfig(c)}
tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos)
cfg := applyDefaults(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't shorten names - cfg -> config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as suggested.

if cfg.GetConfigForClient != nil {
oldFn := cfg.GetConfigForClient
cfg.GetConfigForClient = func(hello *tls.ClientHelloInfo) (*tls.Config, error) {
cfgForClient, err := oldFn(hello)
if err != nil || cfgForClient == nil {
return cfgForClient, err

Check warning on line 209 in credentials/tls.go

View check run for this annotation

Codecov / codecov/patch

credentials/tls.go#L209

Added line #L209 was not covered by tests
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
}
return applyDefaults(cfgForClient), nil
}
}
tc := &tlsCreds{config: cfg}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit tc -> creds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the temporary variable an inlined the return.

return tc
}

func applyDefaults(c *tls.Config) *tls.Config {
config := credinternal.CloneTLSConfig(c)
config.NextProtos = credinternal.AppendH2ToNextProtos(config.NextProtos)
// If the user did not configure a MinVersion and did not configure a
// MaxVersion < 1.2, use MinVersion=1.2, which is required by
// https://datatracker.ietf.org/doc/html/rfc7540#section-9.2
if tc.config.MinVersion == 0 && (tc.config.MaxVersion == 0 || tc.config.MaxVersion >= tls.VersionTLS12) {
tc.config.MinVersion = tls.VersionTLS12
if config.MinVersion == 0 && (config.MaxVersion == 0 || config.MaxVersion >= tls.VersionTLS12) {
config.MinVersion = tls.VersionTLS12
}
// If the user did not configure CipherSuites, use all "secure" cipher
// suites reported by the TLS package, but remove some explicitly forbidden
// by https://datatracker.ietf.org/doc/html/rfc7540#appendix-A
if tc.config.CipherSuites == nil {
if config.CipherSuites == nil {
for _, cs := range tls.CipherSuites() {
if _, ok := tls12ForbiddenCipherSuites[cs.ID]; !ok {
tc.config.CipherSuites = append(tc.config.CipherSuites, cs.ID)
config.CipherSuites = append(config.CipherSuites, cs.ID)
}
}
}
return tc
return config
}

// NewClientTLSFromCert constructs TLS credentials from the provided root
Expand Down
Loading