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.

Sign up for GitHub

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

Jump to bottom

grpc: better RFC 3986 compliant target parsing #4817

Merged
merged 24 commits into from Oct 14, 2021
Merged

grpc: better RFC 3986 compliant target parsing #4817

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
fb12d07
grpc: increase compliance with RFC 3986 while parsing the dial target
easwars Sep 24, 2021
cee1b44
fix resolver in load_balancing example
easwars Sep 27, 2021
d19cd9b
fix resolver in name_resolving example
easwars Sep 27, 2021
0d0c1a5
fix typo
easwars Sep 27, 2021
c98608a
make unix target canonical in test
easwars Sep 27, 2021
fb4b9c0
add `Unparsed` field to `resolver.Target`
easwars Sep 29, 2021
2750421
make getServerName a method on cc instead of ac
easwars Sep 29, 2021
c522792
some comments
easwars Sep 29, 2021
4f56e2e
use passthrough whenever custom dialer is used
easwars Sep 29, 2021
ae490d0
resolver builder
easwars Sep 29, 2021
99fdc43
strip leading "/" from endpoint for backward compatibility
easwars Sep 30, 2021
f578ae5
moved these tests to clientconn_parsed_target_test.go
easwars Sep 30, 2021
1fa26dc
revert unwanted import change
easwars Sep 30, 2021
7936989
Merge branch 'master' into url_parse_attempt_3
easwars Sep 30, 2021
499f603
update comment for `ParsedURL`
easwars Sep 30, 2021
c5c4383
add tests for `scheme:absolute-path`
easwars Sep 30, 2021
5cff707
return error if ParsedURL is nil in unix resolver
easwars Sep 30, 2021
c36957f
store the parsed url by value in `resolver.Target`
easwars Oct 1, 2021
8546581
review comments
easwars Oct 5, 2021
78fcb25
review comments
easwars Oct 11, 2021
4fb05c3
edit todo for optional resolver interface
easwars Oct 11, 2021
c6057e5
ClientHandshake comments
easwars Oct 11, 2021
f230c53
deprecate old fields of resolver.Target and rename `ParsedURL` to `URL`
easwars Oct 11, 2021
5f96946
validate parsed URL in unit tests
easwars Oct 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
185 changes: 144 additions & 41 deletions clientconn.go
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ import (
"errors"
"fmt"
"math"
"net/url"
"reflect"
"strings"
"sync"
Expand All @@ -37,7 +38,6 @@ import (
"google.golang.org/grpc/internal/backoff"
"google.golang.org/grpc/internal/channelz"
"google.golang.org/grpc/internal/grpcsync"
"google.golang.org/grpc/internal/grpcutil"
iresolver "google.golang.org/grpc/internal/resolver"
"google.golang.org/grpc/internal/transport"
"google.golang.org/grpc/keepalive"
Expand Down Expand Up @@ -248,38 +248,12 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
}

// Determine the resolver to use.
cc.parsedTarget = grpcutil.ParseTarget(cc.target, cc.dopts.copts.Dialer != nil)
channelz.Infof(logger, cc.channelzID, "parsed scheme: %q", cc.parsedTarget.Scheme)
resolverBuilder := cc.getResolver(cc.parsedTarget.Scheme)
if resolverBuilder == nil {
// If resolver builder is still nil, the parsed target's scheme is
// not registered. Fallback to default resolver and set Endpoint to
// the original target.
channelz.Infof(logger, cc.channelzID, "scheme %q not registered, fallback to default scheme", cc.parsedTarget.Scheme)
cc.parsedTarget = resolver.Target{
Scheme: resolver.GetDefaultScheme(),
Endpoint: target,
}
resolverBuilder = cc.getResolver(cc.parsedTarget.Scheme)
if resolverBuilder == nil {
return nil, fmt.Errorf("could not get resolver for default scheme: %q", cc.parsedTarget.Scheme)
}
}

creds := cc.dopts.copts.TransportCredentials
if creds != nil && creds.Info().ServerName != "" {
cc.authority = creds.Info().ServerName
} else if cc.dopts.insecure && cc.dopts.authority != "" {
cc.authority = cc.dopts.authority
} else if strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:") {
cc.authority = "localhost"
} else if strings.HasPrefix(cc.parsedTarget.Endpoint, ":") {
cc.authority = "localhost" + cc.parsedTarget.Endpoint
} else {
// Use endpoint from "scheme://authority/endpoint" as the default
// authority for ClientConn.
cc.authority = cc.parsedTarget.Endpoint
resolverBuilder, err := cc.parseTargetAndFindResolver()
if err != nil {
return nil, err
}
cc.determineAuthority()
channelz.Infof(logger, cc.channelzID, "Channel authority set to %q", cc.authority)

if cc.dopts.scChan != nil && !scSet {
// Blocking wait for the initial service config.
Expand Down Expand Up @@ -902,10 +876,7 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool {
// ac.state is Ready, try to find the connected address.
var curAddrFound bool
for _, a := range addrs {
// a.ServerName takes precedent over ClientConn authority, if present.
if a.ServerName == "" {
a.ServerName = ac.cc.authority
}
a.ServerName = ac.cc.getServerName(a)
if reflect.DeepEqual(ac.curAddr, a) {
curAddrFound = true
break
Expand All @@ -919,6 +890,26 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool {
return curAddrFound
}

// getServerName determines the serverName to be used in the connection
// handshake. The default value for the serverName is the authority on the
// ClientConn, which either comes from the user's dial target or through an
// authority override specified using the WithAuthority dial option. Name
// resolvers can specify a per-address override for the serverName through the
// resolver.Address.ServerName field which is used only if the WithAuthority
// dial option was not used. The rationale is that per-address authority
// overrides specified by the name resolver can represent a security risk, while
// an override specified by the user is more dependable since they probably know
// what they are doing.
func (cc *ClientConn) getServerName(addr resolver.Address) string {
if cc.dopts.authority != "" {
return cc.dopts.authority
}
if addr.ServerName != "" {
return addr.ServerName
}
return cc.authority
}

func getMethodConfig(sc *ServiceConfig, method string) MethodConfig {
if sc == nil {
return MethodConfig{}
Expand Down Expand Up @@ -1275,11 +1266,7 @@ func (ac *addrConn) createTransport(addr resolver.Address, copts transport.Conne
prefaceReceived := grpcsync.NewEvent()
connClosed := grpcsync.NewEvent()

// addr.ServerName takes precedent over ClientConn authority, if present.
if addr.ServerName == "" {
addr.ServerName = ac.cc.authority
}

addr.ServerName = ac.cc.getServerName(addr)
hctx, hcancel := context.WithCancel(ac.ctx)
hcStarted := false // protected by ac.mu

Expand Down Expand Up @@ -1621,3 +1608,119 @@ func (cc *ClientConn) connectionError() error {
defer cc.lceMu.Unlock()
return cc.lastConnectionError
}

func (cc *ClientConn) parseTargetAndFindResolver() (resolver.Builder, error) {
channelz.Infof(logger, cc.channelzID, "original dial target is: %q", cc.target)

var rb resolver.Builder
parsedTarget, err := parseTarget(cc.target)
if err != nil {
channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", cc.target, err)
} else {
channelz.Infof(logger, cc.channelzID, "parsed dial target is: %+v", parsedTarget)
rb = cc.getResolver(parsedTarget.Scheme)
if rb != nil {
cc.parsedTarget = parsedTarget
return rb, nil
}
}

// We are here because the user's dial target did not contain a scheme or
// specified an unregistered scheme. We should fallback to the default
// scheme, except when a custom dialer is specified in which case, we should
// always use passthrough scheme.
defScheme := resolver.GetDefaultScheme()
if cc.dopts.copts.Dialer != nil {
defScheme = "passthrough"
}
channelz.Infof(logger, cc.channelzID, "fallback to scheme %q", defScheme)
canonicalTarget := defScheme + ":///" + cc.target

parsedTarget, err = parseTarget(canonicalTarget)
if err != nil {
channelz.Infof(logger, cc.channelzID, "dial target %q parse failed: %v", canonicalTarget, err)
return nil, err
}
channelz.Infof(logger, cc.channelzID, "parsed dial target is: %+v", parsedTarget)
rb = cc.getResolver(parsedTarget.Scheme)
if rb == nil {
return nil, fmt.Errorf("could not get resolver for default scheme: %q", parsedTarget.Scheme)
}
cc.parsedTarget = parsedTarget
return rb, nil
}

// parseTarget uses RFC 3986 semantics to parse the given target into a
// resolver.Target struct containing scheme, authority and endpoint. Query
// params are stripped from the endpoint.
func parseTarget(target string) (resolver.Target, error) {
u, err := url.Parse(target)
if err != nil {
return resolver.Target{}, err
}
// For targets of the form "[scheme]://[authority]/endpoint, the endpoint
// value returned from url.Parse() contains a leading "/". Although this is
// in accordance with RFC 3986, we do not want to break existing resolver
// implementations which expect the endpoint without the leading "/". So, we
// end up stripping the leading "/" here. But this will result in an
// incorrect parsing for something like "unix:///path/to/socket". Since we
// own the "unix" resolver, we can workaround in the unix resolver by using
// the `ParsedURL` field instead of the `Endpoint` field.
endpoint := u.Path
if endpoint == "" {
endpoint = u.Opaque
}
endpoint = strings.TrimPrefix(endpoint, "/")
return resolver.Target{
Scheme: u.Scheme,
Authority: u.Host,
Endpoint: endpoint,
ParsedURL: *u,
}, nil
}

// Determine channel authority. The order of precedence is as follows:
// - user specified authority override using `WithAuthority` dial option
// - creds' notion of server name for the authentication handshake
// - endpoint from dial target of the form "scheme://[authority]/endpoint"
func (cc *ClientConn) determineAuthority() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be cleaner if you passed dopts as a parameter, returned the authority, and didn't make this a method. I'd rather see cc.authority := determineAuthority(cc.dopts) than cc.determineAuthority() since in the former case I can reason about what might and might not be happening, whereas in the latter case I need to inspect cc.determineAuthority to determine what all it might be doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

// Historically, we had two options for users to specify the serverName or
// authority for a channel. One was through the transport credentials
// (either in its constructor, or through the OverrideServerName() method).
// The other option (for cases where WithInsecure() dial option was used)
// was to use the WithAuthority() dial option.
//
// A few things have changed since:
// - `insecure` package with an implementation of the `TransportCredentials`
// interface for the insecure case
// - WithAuthority() dial option support for secure credentials
authorityFromCreds := ""
if creds := cc.dopts.copts.TransportCredentials; creds != nil && creds.Info().ServerName != "" {
authorityFromCreds = creds.Info().ServerName
}
authorityFromDialOption := ""
if cc.dopts.authority != "" {
authorityFromDialOption = cc.dopts.authority
}
Copy link
Member

Choose a reason for hiding this comment

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

Simplify: authorityFromDialOption := cc.dopts.authority

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

if (authorityFromCreds != "" && authorityFromDialOption != "") && authorityFromCreds != authorityFromDialOption {
channelz.Warningf(logger, cc.channelzID, "ClientConn's authority from transport creds %q and dial option %q don't match. Will use the former.", authorityFromCreds, authorityFromDialOption)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should return an error here. The Creds+WithAuthority combination wasn't even allowed in the past so now that we're allowing it I'd rather fail if they are both specified and not consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


// TODO: Define an optional interface on the resolver builder to return the
// channel authority given the user's dial target. Once we have this, we can
// get rid of the special cases for unix and `:port`.
switch {
case authorityFromDialOption != "":
cc.authority = authorityFromDialOption
case authorityFromCreds != "":
cc.authority = authorityFromCreds
case strings.HasPrefix(cc.target, "unix:") || strings.HasPrefix(cc.target, "unix-abstract:"):
Copy link
Member

Choose a reason for hiding this comment

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

// TODO: remove when the unix resolver returns addresses with ServerName set and we make that affect the authority used for RPCs on that connection or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

cc.authority = "localhost"
case strings.HasPrefix(cc.parsedTarget.Endpoint, ":"):
cc.authority = "localhost" + cc.parsedTarget.Endpoint
default:
// Use endpoint from "scheme://authority/endpoint" as the default
// authority for ClientConn.
cc.authority = cc.parsedTarget.Endpoint
}
}
7 changes: 3 additions & 4 deletions clientconn_authority_test.go
View file
Open in desktop
Expand Up @@ -59,11 +59,10 @@ func (s) TestClientConnAuthority(t *testing.T) {
wantAuthority: "authority-override",
},
{
name: "override-via-creds-and-WithAuthority",
target: "Non-Existent.Server:8080",
// WithAuthority override works only for insecure creds.
name: "override-via-creds-and-WithAuthority",
target: "Non-Existent.Server:8080",
opts: []DialOption{WithTransportCredentials(creds), WithAuthority("authority-override")},
wantAuthority: serverNameOverride,
wantAuthority: "authority-override",
},
{
name: "unix relative",
Expand Down
49 changes: 33 additions & 16 deletions clientconn_parsed_target_test.go
View file
Open in desktop
Expand Up @@ -25,6 +25,9 @@ import (
"testing"
"time"

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"

"google.golang.org/grpc/resolver"
)

Expand All @@ -45,7 +48,7 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
{target: "a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a/b"}},
{target: "a//b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a//b"}},
{target: "google.com", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com"}},
{target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/?a=b"}},
{target: "google.com/?a=b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "google.com/"}},
{target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}},

// An unregistered scheme is specified.
Expand All @@ -60,20 +63,22 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
// A registered scheme is specified.
{target: "dns:///google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "google.com"}},
{target: "dns://a.server.com/google.com", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com"}},
{target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/?a=b"}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}},
{target: "dns://a.server.com/google.com/?a=b", wantParsed: resolver.Target{Scheme: "dns", Authority: "a.server.com", Endpoint: "google.com/"}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"}},
Copy link
Member

Choose a reason for hiding this comment

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

Let's validate the URL (at least URL.Path) here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since I marked the older fields as deprecated, I added the URL to every case.

{target: "unix-abstract:a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}},
{target: "unix-abstract:a b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a b"}},
{target: "unix-abstract:a:b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a:b"}},
{target: "unix-abstract:a-b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a-b"}},
{target: "unix-abstract:/ a///://::!@#$%^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/ a///://::!@#$%^&*()b"}},
{target: "unix-abstract:/ a///://::!@#$%25^&*()b", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: " a///://::!@"}},
{target: "unix-abstract:passthrough:abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "passthrough:abc"}},
{target: "unix-abstract:unix:///abc", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "unix:///abc"}},
{target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/a/b/c"}},
{target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "/"}},
{target: "unix-abstract://authority", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "//authority"}},
{target: "unix://domain", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix://domain"}},
{target: "unix-abstract:///a/b/c", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: "a/b/c"}},
{target: "unix-abstract:///", wantParsed: resolver.Target{Scheme: "unix-abstract", Authority: "", Endpoint: ""}},
Copy link
Member

Choose a reason for hiding this comment

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

Why did you delete the other two test cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not support unix*://<relative-path> as per our naming guide. Earlier these used to parse differently. But now, they parse with resolver.Target.Authority set. This means that the unix* resolver will reject this because it does not support an authority. These cases are now moved to the TestParsedTarget_Failure_WithoutCustomDialer test.

{target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}},

// Cases for `scheme:absolute-path`.
{target: "dns:/a/b/c", wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "a/b/c"}},
{target: "unregistered:/a/b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unregistered:/a/b/c"}},
}

for _, test := range tests {
Expand All @@ -84,8 +89,8 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
}
defer cc.Close()

if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed {
t.Errorf("cc.parsedTarget = %+v, want %+v", gotParsed, test.wantParsed)
if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) {
t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed)
}
})
}
Expand All @@ -94,7 +99,9 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
func (s) TestParsedTarget_Failure_WithoutCustomDialer(t *testing.T) {
targets := []string{
"unix://a/b/c",
"unix://authority",
"unix-abstract://authority/a/b/c",
"unix-abstract://authority",
}

for _, target := range targets {
Expand All @@ -118,17 +125,17 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
// different behaviors with a custom dialer.
{
target: "unix:a/b/c",
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:a/b/c"},
wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"},
wantDialerAddress: "unix:a/b/c",
},
{
target: "unix:/a/b/c",
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "unix:/a/b/c"},
wantDialerAddress: "unix:/a/b/c",
wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"},
wantDialerAddress: "unix:///a/b/c",
},
{
target: "unix:///a/b/c",
wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"},
wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "a/b/c"},
wantDialerAddress: "unix:///a/b/c",
},
{
Expand All @@ -151,6 +158,16 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"},
wantDialerAddress: "://authority/127.0.0.1:50051",
},
{
target: "/unix/socket/address",
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"},
wantDialerAddress: "/unix/socket/address",
},
{
target: "passthrough://a.server.com/google.com",
wantParsed: resolver.Target{Scheme: "passthrough", Authority: "a.server.com", Endpoint: "google.com"},
wantDialerAddress: "google.com",
},
}

for _, test := range tests {
Expand All @@ -175,8 +192,8 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
case <-time.After(time.Second):
t.Fatal("timeout when waiting for custom dialer to be invoked")
}
if gotParsed := cc.parsedTarget; gotParsed != test.wantParsed {
t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, gotParsed, test.wantParsed)
if !cmp.Equal(cc.parsedTarget, test.wantParsed, cmpopts.IgnoreFields(resolver.Target{}, "ParsedURL")) {
t.Errorf("cc.parsedTarget for dial target %q = %+v, want %+v", test.target, cc.parsedTarget, test.wantParsed)
}
})
}
Expand Down
17 changes: 14 additions & 3 deletions credentials/credentials.go
View file
Open in desktop
Expand Up @@ -140,6 +140,13 @@ type TransportCredentials interface {
// Additionally, ClientHandshakeInfo data will be available via the context
// passed to this call.
//
// The second argument to this method is the ClientConn's authority, usually
// derived from the user's dial target. Implementations should use this as
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, ClientConn's authority? But what if the ServerName on the address overrides it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reworded it a little.

// the server name during the authentication handshake. Implementations may
// choose to ignore this value and use a value acquired through other means,
// in which case they must make sure that the value is acquired through
// secure means and that a possible attacker cannot tamper with the value.
Copy link
Member

Choose a reason for hiding this comment

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

Holy cow, we should never, ever recommend (or even mention) ignoring this field. Implementations MUST honor this value, period. Otherwise the :authority header won't match and who knows what problems that will cause. Just change the previous sentence to Implementations must use this as the server name during the authentication handshake. and leave it at that. If someone really, really knows what they're doing and they want to abuse this, then that's fine and we won't support them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

//
// If the returned net.Conn is closed, it MUST close the net.Conn provided.
ClientHandshake(context.Context, string, net.Conn) (net.Conn, AuthInfo, error)
// ServerHandshake does the authentication handshake for servers. It returns
Expand All @@ -153,9 +160,13 @@ type TransportCredentials interface {
Info() ProtocolInfo
// Clone makes a copy of this TransportCredentials.
Clone() TransportCredentials
// OverrideServerName overrides the server name used to verify the hostname on the returned certificates from the server.
// gRPC internals also use it to override the virtual hosting name if it is set.
// It must be called before dialing. Currently, this is only used by grpclb.
// 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.
OverrideServerName(string) error
}

Expand Down