Skip to content

Commit

Permalink
grpc: increase compliance with RFC 3986 while parsing the dial target
Browse files Browse the repository at this point in the history
  • Loading branch information
@easwars
easwars committed Sep 24, 2021
1 parent 11437f6 commit fb12d07
Show file tree
Hide file tree
Showing 14 changed files with 209 additions and 185 deletions.
3 changes: 2 additions & 1 deletion balancer/grpclb/grpclb.go
View file
Expand Up @@ -26,6 +26,7 @@ import (
"context"
"errors"
"fmt"
"strings"
"sync"
"time"

Expand Down Expand Up @@ -135,7 +136,7 @@ func (b *lbBuilder) Build(cc balancer.ClientConn, opt balancer.BuildOptions) bal

lb := &lbBalancer{
cc: newLBCacheClientConn(cc),
target: opt.Target.Endpoint,
target: strings.TrimPrefix(opt.Target.Endpoint, "/"),
opt: opt,
fallbackTimeout: b.fallbackTimeout,
doneCh: make(chan struct{}),
Expand Down
8 changes: 6 additions & 2 deletions balancer/rls/internal/config.go
View file
Expand Up @@ -22,15 +22,16 @@ import (
"bytes"
"encoding/json"
"fmt"
"net/url"
"time"

"github.com/golang/protobuf/jsonpb"
"github.com/golang/protobuf/ptypes"
durationpb "github.com/golang/protobuf/ptypes/duration"

"google.golang.org/grpc/balancer"
"google.golang.org/grpc/balancer/rls/internal/keys"
rlspb "google.golang.org/grpc/balancer/rls/internal/proto/grpc_lookup_v1"
"google.golang.org/grpc/internal/grpcutil"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/serviceconfig"
)
Expand Down Expand Up @@ -201,7 +202,10 @@ func (*rlsBB) ParseConfig(c json.RawMessage) (serviceconfig.LoadBalancingConfig,
if lookupService == "" {
return nil, fmt.Errorf("rls: empty lookup_service in service config {%+v}", string(c))
}
parsedTarget := grpcutil.ParseTarget(lookupService, false)
parsedTarget, err := url.Parse(lookupService)
if err != nil {
return nil, fmt.Errorf("rls: invalid target URI in lookup_service {%s}", lookupService)
}
if parsedTarget.Scheme == "" {
parsedTarget.Scheme = resolver.GetDefaultScheme()
}
Expand Down
140 changes: 102 additions & 38 deletions clientconn.go
View file
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,37 +248,30 @@ 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)
}
resolverBuilder, err := cc.parseTargetAndFindResolver()
if err != nil {
return nil, err
}

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:") {
// Determine the authority for the channel. For now, we will use the parsed
// endpoint from the user's dial target (with a leading "/" stripped), as
// the default value. We do have some special handling for targets with unix
// scheme and empty hostname (":port").
// TODO: Define an optional interface on the name resolver to return the
// authority given the user's dial target.
cc.authority = strings.TrimPrefix(cc.parsedTarget.Endpoint, "/")
if strings.HasPrefix(cc.authority, ":") {
cc.authority = "localhost" + cc.authority
}
if cc.parsedTarget.Scheme == "unix" || cc.parsedTarget.Scheme == "unx-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
}
// The only authority override supported at this point in time is the one
// using the WithAuthority dial option, and this trumps over any per-address
// overrides specified by the name resolver in the
// resolver.Address.ServerName field.
if cc.dopts.authority != "" {
cc.authority = cc.dopts.authority
}

if cc.dopts.scChan != nil && !scSet {
Expand Down Expand Up @@ -902,10 +895,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.getServerName(a)
if reflect.DeepEqual(ac.curAddr, a) {
curAddrFound = true
break
Expand All @@ -919,6 +909,25 @@ 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 (ac *addrConn) getServerName(addr resolver.Address) string {
if ac.cc.dopts.authority == "" {
if addr.ServerName != "" {
return addr.ServerName
}
}
return ac.cc.authority
}

func getMethodConfig(sc *ServiceConfig, method string) MethodConfig {
if sc == nil {
return MethodConfig{}
Expand Down Expand Up @@ -1275,11 +1284,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.getServerName(addr)
hctx, hcancel := context.WithCancel(ac.ctx)
hcStarted := false // protected by ac.mu

Expand Down Expand Up @@ -1621,3 +1626,62 @@ 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)
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 no scheme was specified in the user's dial target and
// a custom dialer was specified. In the latter case, we should always use
// passthrough scheme.
defScheme := resolver.GetDefaultScheme()
if parsedTarget.Scheme == "" && 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
}
path := u.Path
if path == "" {
path = u.Opaque
}
return resolver.Target{
Scheme: u.Scheme,
Authority: u.Host,
Endpoint: path,
}, nil
}
17 changes: 9 additions & 8 deletions clientconn_authority_test.go
View file
Expand Up @@ -47,10 +47,12 @@ func (s) TestClientConnAuthority(t *testing.T) {
wantAuthority: "Non-Existent.Server:8080",
},
{
name: "override-via-creds",
target: "Non-Existent.Server:8080",
opts: []DialOption{WithTransportCredentials(creds)},
wantAuthority: serverNameOverride,
name: "override-via-creds",
target: "Non-Existent.Server:8080",
opts: []DialOption{WithTransportCredentials(creds)},
// Overriding authority from the transport credentials is not
// supported anymore.
wantAuthority: "Non-Existent.Server:8080",
},
{
name: "override-via-WithAuthority",
Expand All @@ -59,11 +61,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
66 changes: 33 additions & 33 deletions clientconn_parsed_target_test.go
View file
Expand Up @@ -35,45 +35,43 @@ func (s) TestParsedTarget_Success_WithoutCustomDialer(t *testing.T) {
wantParsed resolver.Target
}{
// No scheme is specified.
{target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ""}},
{target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://"}},
{target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///"}},
{target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/"}},
{target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///a"}},
{target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://a/b"}},
{target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}},
{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: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/unix/socket/address"}},
{target: "", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/"}},
{target: "://", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://"}},
{target: ":///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///"}},
{target: "://a/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/"}},
{target: ":///a", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///a"}},
{target: "://a/b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://a/b"}},
{target: "/", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//"}},
{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/"}},
{target: "/unix/socket/address", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "//unix/socket/address"}},

// An unregistered scheme is specified.
{target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a:///"}},
{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: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b/c"}},
{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: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "a://b"}},
{target: "a:///", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a:///"}},
{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: "a://b/c", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b/c"}},
{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: "a://b", wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/a://b"}},

// 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: "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/"}},
{target: "unix:///a/b/c", wantParsed: resolver.Target{Scheme: "unix", Authority: "", Endpoint: "/a/b/c"}},
{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: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "unix:///a/b/c"}},
{target: "passthrough:///unix:///a/b/c", wantParsed: resolver.Target{Scheme: "passthrough", Authority: "", Endpoint: "/unix:///a/b/c"}},
}

for _, test := range tests {
Expand All @@ -94,7 +92,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,13 +118,13 @@ 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",
Expand All @@ -133,22 +133,22 @@ func (s) TestParsedTarget_WithCustomDialer(t *testing.T) {
},
{
target: "dns:///127.0.0.1:50051",
wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "127.0.0.1:50051"},
wantParsed: resolver.Target{Scheme: "dns", Authority: "", Endpoint: "/127.0.0.1:50051"},
wantDialerAddress: "127.0.0.1:50051",
},
{
target: ":///127.0.0.1:50051",
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: ":///127.0.0.1:50051"},
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/:///127.0.0.1:50051"},
wantDialerAddress: ":///127.0.0.1:50051",
},
{
target: "dns://authority/127.0.0.1:50051",
wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "127.0.0.1:50051"},
wantParsed: resolver.Target{Scheme: "dns", Authority: "authority", Endpoint: "/127.0.0.1:50051"},
wantDialerAddress: "127.0.0.1:50051",
},
{
target: "://authority/127.0.0.1:50051",
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "://authority/127.0.0.1:50051"},
wantParsed: resolver.Target{Scheme: defScheme, Authority: "", Endpoint: "/://authority/127.0.0.1:50051"},
wantDialerAddress: "://authority/127.0.0.1:50051",
},
}
Expand Down

0 comments on commit fb12d07

Please sign in to comment.