credentials: fix behavior of grpc.WithAuthority and credential handshake precedence

[dfawley]

Before this change, credentials would ignore the channel's understanding of the server name if the user configured them to do so. This is a mistake. The credentials should use the channel's setting. The channel already reads the credentials' preference and includes that in its logic.

A summary of how this worked:

• User creates credentials, optionally configuring a server name override at configuration.
• Optionally, user calls creds.OverrideServerName, which can/should be implemented such that it affects the server name override. It does this by setting the ProtocolInfo.ServerName field that is returned by its Info method.
• gRPC has a list of precedence for determining the authority of the channel: grpc.WithAuthority; credentials; name resolver; channel default based on target string.
• gRPC passes the determined authority to creds.ClientHandshake. The creds should honor the value passed in.

I also tidied up the documentation a bit and added a deprecation notice to ProtocolInfo.ServerName.

@gtcooke94 since this impacts advancedtls.

cc @ejona86 FYI

RELEASE NOTES:

• credentials: properly apply grpc.WithAuthority as the highest-priority option for setting authority, above the setting in the credentials themselves. Now that this WithAuthority is available, the credentials should not be used to override the authority. Also, properly percent-encode the port (if needed, which is very unlikely) when the target string omits the hostname and only specifies a port (grpc.NewClient(":<port-number-or-name>")).

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.43%. Comparing base (85240a5) to head (ef25aa4).
⚠️ Report is 3 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8488      +/-   ##
==========================================
- Coverage   82.48%   82.43%   -0.05%     
==========================================
  Files         413      413              
  Lines       40518    40534      +16     
==========================================
- Hits        33422    33415       -7     
- Misses       5738     5759      +21     
- Partials     1358     1360       +2     

Files with missing lines	Coverage Δ
clientconn.go	90.52% <100.00%> (-0.28%)	⬇️
credentials/credentials.go	88.57% <ø> (ø)
credentials/tls.go	90.56% <100.00%> (+0.05%)	⬆️
dialoptions.go	87.40% <ø> (ø)
experimental/credentials/tls.go	79.20% <100.00%> (+3.39%)	⬆️
resolver/resolver.go	100.00% <ø> (ø)

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[easwars]

My first reading of this comment made me think that this is a value that the user can set. But that is not the case. This is a value returned by gRPC to the user. Would it be possible to reword this a little so that it is clear that this only conveys the creds' view of things.

[dfawley]

I think your initial understanding was correct.

This is returned by the credentials implementation and read by gRPC. The user controls how this is set by configuring the credentials directly.

[easwars]

Is this a breaking change? Should this be release noted?

[dfawley]

It's a behavior change. Unclear if it's breaking; I would call it a bug fix. I'll mention it in the notes.

[dfawley]

Added:

Also, properly percent-encode the port (if needed, which is very unlikely) when the target string omits the hostname and only specifies a port (grpc.NewClient(":<port-number-or-name>")).

[easwars]

Should we document on the NewTLS function that config.ServerName is ignored?

[dfawley]

It's not ignored, actually. It's used by the channel in the logic mentioned in the first PR comment, as long as the user didn't manually override the channel's authority.

grpc-go/clientconn.go

Lines 1810 to 1813 in 9fa3267

	authorityFromCreds := ""
	if creds := dopts.copts.TransportCredentials; creds != nil && creds.Info().ServerName != "" {
	authorityFromCreds = creds.Info().ServerName
	}

grpc-go/credentials/tls.go

Line 106 in 9fa3267

ServerName: c.config.ServerName,

[gtcooke94]

This LGTM, I do worry about breakages from small behavior changes, but this is a bug so if a breakage does happen it's probably good in the end to discover it?

cc: @matthewstevenson88 for awareness

[dfawley]

but this is a bug so if a breakage does happen it's probably good in the end to discover it?

It's definitely a bug in that the credentials override is currently winning against the channel's override for handshaking, but what is used for RPCs is something else. The intent was for grpc.WithAuthority (fairly recently added) to be the highest priority for the channel, with the CallOption taking even high precedence. This change enacts that intent.

Thanks!