credentials: fix behavior of grpc.WithAuthority and credential handshake precedence (#8488)

Author: dfawley

clientconn.go

@@ -1824,7 +1824,7 @@ func (cc *ClientConn) initAuthority() error {
 	} else if auth, ok := cc.resolverBuilder.(resolver.AuthorityOverrider); ok {
 		cc.authority = auth.OverrideAuthority(cc.parsedTarget)
 	} else if strings.HasPrefix(endpoint, ":") {
-		cc.authority = "localhost" + endpoint
+		cc.authority = "localhost" + encodeAuthority(endpoint)
 	} else {
 		cc.authority = encodeAuthority(endpoint)
 	}

credentials/alts/alts_test.go

@@ -153,9 +153,6 @@ func (s) TestInfo(t *testing.T) {
 	// use NewServerCreds and not NewClientCreds.
 	c := NewServerCreds(DefaultServerOptions())
 	info := c.Info()
-	if got, want := info.ProtocolVersion, ""; got != want {
-		t.Errorf("info.ProtocolVersion=%v, want %v", got, want)
-	}
 	if got, want := info.SecurityProtocol, "alts"; got != want {
 		t.Errorf("info.SecurityProtocol=%v, want %v", got, want)
 	}

credentials/credentials.go

@@ -96,10 +96,11 @@ func (c CommonAuthInfo) GetCommonAuthInfo() CommonAuthInfo {
 	return c
 }
 
-// ProtocolInfo provides information regarding the gRPC wire protocol version,
-// security protocol, security protocol version in use, server name, etc.
+// ProtocolInfo provides static information regarding transport credentials.
 type ProtocolInfo struct {
 	// ProtocolVersion is the gRPC wire protocol version.
+	//
+	// Deprecated: this is unused by gRPC.
 	ProtocolVersion string
 	// SecurityProtocol is the security protocol in use.
 	SecurityProtocol string
@@ -109,7 +110,16 @@ type ProtocolInfo struct {
 	//
 	// Deprecated: please use Peer.AuthInfo.
 	SecurityVersion string
-	// ServerName is the user-configured server name.
+	// ServerName is the user-configured server name.  If set, this overrides
+	// the default :authority header used for all RPCs on the channel using the
+	// containing credentials, unless grpc.WithAuthority is set on the channel,
+	// in which case that setting will take precedence.
+	//
+	// This must be a valid `:authority` header according to
+	// [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986#section-3.2).
+	//
+	// Deprecated: Users should use grpc.WithAuthority to override the authority
+	// on a channel instead of configuring the credentials.
 	ServerName string
 }
 
@@ -173,12 +183,17 @@ type TransportCredentials interface {
 	// Clone makes a copy of this TransportCredentials.
 	Clone() TransportCredentials
 	// OverrideServerName specifies the value used for the following:
+	//
 	// - verifying the hostname on the returned certificates
 	// - as SNI in the client's handshake to support virtual hosting
 	// - as the value for `:authority` header at stream creation time
 	//
-	// Deprecated: use grpc.WithAuthority instead. Will be supported
-	// throughout 1.x.
+	// The provided string should be a valid `:authority` header according to
+	// [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986#section-3.2).
+	//
+	// Deprecated: this method is unused by gRPC.  Users should use
+	// grpc.WithAuthority to override the authority on a channel instead of
+	// configuring the credentials.
 	OverrideServerName(string) error
 }
 

credentials/tls.go

@@ -110,14 +110,14 @@ func (c tlsCreds) Info() ProtocolInfo {
 func (c *tlsCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (_ net.Conn, _ AuthInfo, err error) {
 	// use local cfg to avoid clobbering ServerName if using multiple endpoints
 	cfg := credinternal.CloneTLSConfig(c.config)
-	if cfg.ServerName == "" {
-		serverName, _, err := net.SplitHostPort(authority)
-		if err != nil {
-			// If the authority had no host port or if the authority cannot be parsed, use it as-is.
-			serverName = authority
-		}
-		cfg.ServerName = serverName
+
+	serverName, _, err := net.SplitHostPort(authority)
+	if err != nil {
+		// If the authority had no host port or if the authority cannot be parsed, use it as-is.
+		serverName = authority
 	}
+	cfg.ServerName = serverName
+
 	conn := tls.Client(rawConn, cfg)
 	errChannel := make(chan error, 1)
 	go func() {
@@ -259,9 +259,11 @@ func applyDefaults(c *tls.Config) *tls.Config {
 // certificates to establish the identity of the client need to be included in
 // the credentials (eg: for mTLS), use NewTLS instead, where a complete
 // tls.Config can be specified.
-// serverNameOverride is for testing only. If set to a non empty string,
-// it will override the virtual host name of authority (e.g. :authority header
-// field) in requests.
+//
+// serverNameOverride is for testing only. If set to a non empty string, it will
+// override the virtual host name of authority (e.g. :authority header field) in
+// requests.  Users should use grpc.WithAuthority passed to grpc.NewClient to
+// override the authority of the client instead.
 func NewClientTLSFromCert(cp *x509.CertPool, serverNameOverride string) TransportCredentials {
 	return NewTLS(&tls.Config{ServerName: serverNameOverride, RootCAs: cp})
 }
@@ -271,9 +273,11 @@ func NewClientTLSFromCert(cp *x509.CertPool, serverNameOverride string) Transpor
 // certificates to establish the identity of the client need to be included in
 // the credentials (eg: for mTLS), use NewTLS instead, where a complete
 // tls.Config can be specified.
-// serverNameOverride is for testing only. If set to a non empty string,
-// it will override the virtual host name of authority (e.g. :authority header
-// field) in requests.
+//
+// serverNameOverride is for testing only. If set to a non empty string, it will
+// override the virtual host name of authority (e.g. :authority header field) in
+// requests.  Users should use grpc.WithAuthority passed to grpc.NewClient to
+// override the authority of the client instead.
 func NewClientTLSFromFile(certFile, serverNameOverride string) (TransportCredentials, error) {
 	b, err := os.ReadFile(certFile)
 	if err != nil {

credentials/xds/xds_client_test.go

@@ -47,7 +47,7 @@ const (
 	defaultTestTimeout      = 1 * time.Second
 	defaultTestShortTimeout = 10 * time.Millisecond
 	defaultTestCertSAN      = "abc.test.example.com"
-	authority               = "authority"
+	authority               = "x.test.example.com"
 )
 
 type s struct {
@@ -61,7 +61,7 @@ func Test(t *testing.T) {
 // Helper function to create a real TLS client credentials which is used as
 // fallback credentials from multiple tests.
 func makeFallbackClientCreds(t *testing.T) credentials.TransportCredentials {
-	creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "x.test.example.com")
+	creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "")
 	if err != nil {
 		t.Fatal(err)
 	}

dialoptions.go

@@ -608,6 +608,8 @@ func WithChainStreamInterceptor(interceptors ...StreamClientInterceptor) DialOpt
 
 // WithAuthority returns a DialOption that specifies the value to be used as the
 // :authority pseudo-header and as the server name in authentication handshake.
+// This overrides all other ways of setting authority on the channel, but can be
+// overridden per-call by using grpc.CallAuthority.
 func WithAuthority(a string) DialOption {
 	return newFuncDialOption(func(o *dialOptions) {
 		o.authority = a

experimental/credentials/tls.go

@@ -56,14 +56,14 @@ func (c tlsCreds) Info() credentials.ProtocolInfo {
 func (c *tlsCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (_ net.Conn, _ credentials.AuthInfo, err error) {
 	// use local cfg to avoid clobbering ServerName if using multiple endpoints
 	cfg := cloneTLSConfig(c.config)
-	if cfg.ServerName == "" {
-		serverName, _, err := net.SplitHostPort(authority)
-		if err != nil {
-			// If the authority had no host port or if the authority cannot be parsed, use it as-is.
-			serverName = authority
-		}
-		cfg.ServerName = serverName
+
+	serverName, _, err := net.SplitHostPort(authority)
+	if err != nil {
+		// If the authority had no host port or if the authority cannot be parsed, use it as-is.
+		serverName = authority
 	}
+	cfg.ServerName = serverName
+
 	conn := tls.Client(rawConn, cfg)
 	errChannel := make(chan error, 1)
 	go func() {

resolver/resolver.go

@@ -332,6 +332,11 @@ type AuthorityOverrider interface {
 	// OverrideAuthority returns the authority to use for a ClientConn with the
 	// given target. The implementation must generate it without blocking,
 	// typically in line, and must keep it unchanged.
+	//
+	// The returned string must be a valid ":authority" header value, i.e. be
+	// encoded according to
+	// [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986#section-3.2) as
+	// necessary.
 	OverrideAuthority(Target) string
 }
 

scripts/vet.sh

@@ -179,6 +179,7 @@ NewSubConn is deprecated:
 OverrideServerName is deprecated:
 RemoveSubConn is deprecated:
 SecurityVersion is deprecated:
+.ServerName is deprecated:
 stats.PickerUpdated is deprecated:
 Target is deprecated: Use the Target field in the BuildOptions instead.
 UpdateAddresses is deprecated:

security/advancedtls/advancedtls.go

@@ -430,11 +430,10 @@ func (c advancedTLSCreds) Info() credentials.ProtocolInfo {
 func (c *advancedTLSCreds) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
 	// Use local cfg to avoid clobbering ServerName if using multiple endpoints.
 	cfg := credinternal.CloneTLSConfig(c.config)
-	// We return the full authority name to users if ServerName is empty without
-	// stripping the trailing port.
-	if cfg.ServerName == "" {
-		cfg.ServerName = authority
-	}
+	// We return the full authority name to users without stripping the trailing
+	// port.
+	cfg.ServerName = authority
+
 	peerVerifiedChains := CertificateChains{}
 	cfg.VerifyPeerCertificate = buildVerifyFunc(c, cfg.ServerName, rawConn, &peerVerifiedChains)
 	conn := tls.Client(rawConn, cfg)