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

arjan-bal · 2024-10-17T11:41:02Z

TLS server configs provided at TLS handshake time through GetConfigForClient were 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 by GetConfigForClient.

RELEASE NOTES:

credentials: TLS credentials created via NewTLS that use tls.Config.GetConfigForClient will 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.

codecov · 2024-10-17T11:48:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.81%. Comparing base (c538c31) to head (0922abc).
Report is 28 commits behind head on master.

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

Files with missing lines	Coverage Δ
credentials/tls.go	`86.57% <100.00%> (+1.06%)`	⬆️

... and 17 files with indirect coverage changes

gtcooke94

Thanks for the PR!

cc: @matthewstevenson88

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.

gtcooke94 · 2024-10-17T14:15:38Z

credentials/tls.go

@@ -200,25 +200,41 @@ var tls12ForbiddenCipherSuites = map[uint16]struct{}{

 // 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)


Don't shorten names - cfg -> config

Changed as suggested.

gtcooke94 · 2024-10-17T14:18:01Z

credentials/tls.go

+			return applyDefaults(cfgForClient), nil
+		}
+	}
+	tc := &tlsCreds{config: cfg}


nit tc -> creds

removed the temporary variable an inlined the return.

credentials/tls.go

gtcooke94 · 2024-10-17T14:30:50Z

Also, regarding the release notes, let's specify that this is specifically for NewTLS as well, the advancedtls way of configuring would not be affected by this. Suggested wording:

credentials: TLS credentials created via NewTLS that use tls.Config.GetConfigForClient will 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.

arjan-bal · 2024-10-17T17:18:37Z

Also, regarding the release notes, let's specify that this is specifically for NewTLS as well, the advancedtls way of configuring would not be affected by this. Suggested wording:

credentials: TLS credentials created via NewTLS that use tls.Config.GetConfigForClient will 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.

Updated, thanks for the suggestion.

easwars

LGTM.

credentials/tls_ext_test.go

gtcooke94

Just that one test structure question, otherwise LGTM

dfawley

Thanks for the fix!

…fig-for-client

…ForClient (grpc#7754)

…ForClient (#7754) (#7813)

…ForClient (grpc#7754)

arjan-bal added the Type: Feature New features or improvements in behavior label Oct 17, 2024

arjan-bal added this to the 1.69 Release milestone Oct 17, 2024

arjan-bal requested review from easwars, gtcooke94 and dfawley October 17, 2024 11:41

arjan-bal assigned easwars, gtcooke94 and dfawley Oct 17, 2024

Apply secure defaults to TLS configs provided through GetConfigForClient

87016c7

arjan-bal force-pushed the defaults-in-get-config-for-client branch from b3de47d to 87016c7 Compare October 17, 2024 11:43

arjan-bal added the Type: Behavior Change Behavior changes not categorized as bugs label Oct 17, 2024

gtcooke94 reviewed Oct 17, 2024

View reviewed changes

Add cases which return nil from GetConfigForClient

67dfb32

better variable names

042ec4b

easwars approved these changes Oct 17, 2024

View reviewed changes

credentials/tls_ext_test.go Outdated Show resolved Hide resolved

credentials/tls_ext_test.go Outdated Show resolved Hide resolved

credentials/tls_ext_test.go Outdated Show resolved Hide resolved

easwars assigned arjan-bal and unassigned easwars Oct 17, 2024

Use a getter for tls config in tests

0b1146d

arjan-bal assigned matthewstevenson88 and unassigned arjan-bal Oct 18, 2024

gtcooke94 reviewed Oct 18, 2024

View reviewed changes

credentials/tls_ext_test.go Outdated Show resolved Hide resolved

gtcooke94 approved these changes Oct 18, 2024

View reviewed changes

dfawley approved these changes Oct 18, 2024

View reviewed changes

dfawley assigned arjan-bal and unassigned dfawley and gtcooke94 Oct 18, 2024

Inline TLS configs in test cases

5da078b

arjan-bal unassigned matthewstevenson88 Oct 21, 2024

Merge remote-tracking branch 'source/master' into defaults-in-get-con…

0922abc

…fig-for-client

arjan-bal merged commit 80937a9 into grpc:master Oct 22, 2024
15 checks passed

arjan-bal deleted the defaults-in-get-config-for-client branch October 22, 2024 17:28

DmitriyMV mentioned this pull request Oct 22, 2024

Please reconsider removal of GRPC_ENFORCE_ALPN_ENABLED in version 1.68.0 and beyond. #7769

Closed

arjan-bal added Area: Auth Includes regular credentials API and implementation. Also includes advancedtls, authz, rbac etc. and removed Area: Auth Includes regular credentials API and implementation. Also includes advancedtls, authz, rbac etc. labels Nov 6, 2024

purnesh42H pushed a commit to purnesh42H/grpc-go that referenced this pull request Nov 6, 2024

credentials: Apply defaults to TLS configs provided through GetConfig…

bcfee8a

…ForClient (grpc#7754)

purnesh42H mentioned this pull request Nov 6, 2024

Cherry-pick #7754 into v1.68.x #7813

Merged

purnesh42H added a commit that referenced this pull request Nov 6, 2024

credentials: Apply defaults to TLS configs provided through GetConfig…

5363dca

…ForClient (#7754) (#7813)

misvivek pushed a commit to misvivek/grpc-go that referenced this pull request Nov 7, 2024

credentials: Apply defaults to TLS configs provided through GetConfig…

f3dcf12

…ForClient (grpc#7754)

tbg mentioned this pull request Nov 27, 2024

DEPS: upgrade grpc to v1.68.0 cockroachdb/cockroach#136278

Draft

16 tasks

dfawley modified the milestones: 1.69 Release, 1.68 Release Dec 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arjan-bal commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

gtcooke94 left a comment •

edited

Loading

gtcooke94 Oct 17, 2024

arjan-bal Oct 17, 2024

gtcooke94 Oct 17, 2024

arjan-bal Oct 17, 2024

gtcooke94 commented Oct 17, 2024

arjan-bal commented Oct 17, 2024

easwars left a comment

gtcooke94 left a comment

dfawley left a comment

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

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

Conversation

arjan-bal commented Oct 17, 2024 • edited Loading

codecov bot commented Oct 17, 2024 • edited Loading

Codecov Report

gtcooke94 left a comment • edited Loading

Choose a reason for hiding this comment

gtcooke94 Oct 17, 2024

Choose a reason for hiding this comment

arjan-bal Oct 17, 2024

Choose a reason for hiding this comment

gtcooke94 Oct 17, 2024

Choose a reason for hiding this comment

arjan-bal Oct 17, 2024

Choose a reason for hiding this comment

gtcooke94 commented Oct 17, 2024

arjan-bal commented Oct 17, 2024

easwars left a comment

Choose a reason for hiding this comment

gtcooke94 left a comment

Choose a reason for hiding this comment

dfawley left a comment

Choose a reason for hiding this comment

arjan-bal commented Oct 17, 2024 •

edited

Loading

codecov bot commented Oct 17, 2024 •

edited

Loading

gtcooke94 left a comment •

edited

Loading