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

xds/clusterresolver: set ServerName to an address for LogicalDNS #5793

Closed
wants to merge 9 commits into from
Closed

xds/clusterresolver: set ServerName to an address for LogicalDNS #5793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
40ec788
transport: new stream with actual server name
holdno Oct 28, 2022
2b12d75
renamed and add test for: transport: ensure value of :authority heade…
holdno Nov 4, 2022
95a427b
fix: copy callHdr to resolve race
holdno Nov 4, 2022
4cbfed1
remove and modify valueless comments
holdno Nov 11, 2022
6f29d4e
move resolver authority test to e2e test
holdno Nov 11, 2022
acdbf62
restore transport/transport_test.go
holdno Nov 11, 2022
56e5899
modify test notes
holdno Nov 11, 2022
3d99b17
fix: undefined resolver
holdno Nov 14, 2022
8429bb7
xds/clusterresolver: set a ServerName to an address for LogicalDNS
110y Nov 14, 2022
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
28 changes: 22 additions & 6 deletions internal/transport/http2_client.go
View file
Open in desktop
Expand Up @@ -59,11 +59,15 @@ var clientConnectionCounter uint64

// http2Client implements the ClientTransport interface with HTTP2.
type http2Client struct {
lastRead int64 // Keep this field 64-bit aligned. Accessed atomically.
ctx context.Context
cancel context.CancelFunc
ctxDone <-chan struct{} // Cache the ctx.Done() chan.
userAgent string
lastRead int64 // Keep this field 64-bit aligned. Accessed atomically.
ctx context.Context
cancel context.CancelFunc
ctxDone <-chan struct{} // Cache the ctx.Done() chan.
userAgent string
// address contains the resolver returned address for this transport.
// If the `ServerName` field is set, it takes precedence over `CallHdr.Host`
// passed to `NewStream`, when determining the :authority header.
address resolver.Address
md metadata.MD
conn net.Conn // underlying communication channel
loopy *loopyWriter
Expand Down Expand Up @@ -314,6 +318,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
cancel: cancel,
userAgent: opts.UserAgent,
registeredCompressors: grpcutil.RegisteredCompressors(),
address: addr,
conn: conn,
remoteAddr: conn.RemoteAddr(),
localAddr: conn.LocalAddr(),
Expand Down Expand Up @@ -702,7 +707,18 @@ func (e NewStreamError) Error() string {
// streams. All non-nil errors returned will be *NewStreamError.
func (t *http2Client) NewStream(ctx context.Context, callHdr *CallHdr) (*Stream, error) {
ctx = peer.NewContext(ctx, t.getPeer())
headerFields, err := t.createHeaderFields(ctx, callHdr)

dupCallHdr := *callHdr
// ServerName field of the resolver returned address takes precedence over
// Host field of CallHdr to determine the :authority header. This is because,
// the ServerName field takes precedence for server authentication during
// TLS handshake, and the :authority header should match the value used
// for server authentication.
if t.address.ServerName != "" {
dupCallHdr.Host = t.address.ServerName
}

headerFields, err := t.createHeaderFields(ctx, &dupCallHdr)
if err != nil {
return nil, &NewStreamError{Err: err, AllowTransparentRetry: false}
}
Expand Down
34 changes: 34 additions & 0 deletions test/authority_test.go
View file
Open in desktop
Expand Up @@ -36,6 +36,8 @@ import (
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/internal/stubserver"
"google.golang.org/grpc/metadata"
"google.golang.org/grpc/resolver"
"google.golang.org/grpc/resolver/manual"
"google.golang.org/grpc/status"
testpb "google.golang.org/grpc/test/grpc_testing"
)
Expand Down Expand Up @@ -205,3 +207,35 @@ func (s) TestColonPortAuthority(t *testing.T) {
t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err)
}
}

// TestAuthorityReplacedWithResolverAddress This test makes sure that the http2 client
// replaces the authority to the resolver address server name when it is set.
func (s) TestAuthorityReplacedWithResolverAddress(t *testing.T) {
const expectedAuthority = "test.server.name"

ss := &stubserver.StubServer{
EmptyCallF: func(ctx context.Context, _ *testpb.Empty) (*testpb.Empty, error) {
return authorityChecker(ctx, expectedAuthority)
},
Network: "tcp",
}
if err := ss.Start(nil); err != nil {
t.Fatalf("Error starting endpoint server: %v", err)
}
defer ss.Stop()

r := manual.NewBuilderWithScheme("whatever")
r.InitialState(resolver.State{Addresses: []resolver.Address{{Addr: ss.Address, ServerName: expectedAuthority}}})
cc, err := grpc.Dial(r.Scheme()+":///whatever", grpc.WithInsecure(), grpc.WithResolvers(r))
if err != nil {
t.Fatalf("grpc.Dial(%q) = %v", ss.Address, err)
}
defer cc.Close()

ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
_, err = testpb.NewTestServiceClient(cc).EmptyCall(ctx, &testpb.Empty{})
if err != nil {
t.Errorf("us.client.EmptyCall(_, _) = _, %v; want _, nil", err)
}
}
2 changes: 1 addition & 1 deletion xds/internal/balancer/clusterresolver/configbuilder.go
View file
Open in desktop
Expand Up @@ -198,7 +198,7 @@ func buildClusterImplConfigForDNS(g *nameGenerator, addrStrs []string, mechanism
retAddrs := make([]resolver.Address, 0, len(addrStrs))
pName := fmt.Sprintf("priority-%v", g.prefix)
for _, addrStr := range addrStrs {
retAddrs = append(retAddrs, hierarchy.Set(resolver.Address{Addr: addrStr}, []string{pName}))
retAddrs = append(retAddrs, hierarchy.Set(resolver.Address{Addr: addrStr, ServerName: mechanism.DNSHostname}, []string{pName}))
Copy link
Author

Choose a reason for hiding this comment

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

This PR just adds this change.
Other changes are inherited from #5748 as I described in the description of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our current behavior (which is to set the :authority header to the authority on the channel and not the DNS name received as part of the Cluster resource) was an explicit choice. It is a fairly serious security risk to set it to the DNS name received from the Cluster resource.

We are going to be investigating this further at some point to support certain serverless use cases. But it's not yet clear to us, how we will solve the security problem here. We'll definitely need more design work before we can safely do something like this. And when we have a design for this, we will publish a gRFC for the same, and the change will happen across all supported gRPC languages.

Unfortunately, we cannot accept this PR at this point in time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please feel free to engage with the gRPC team on our mailing list if you want to continue this discussion and work towards a secure solution for this problem. Thank you.

Copy link
Author

Choose a reason for hiding this comment

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

@easwars

Thank you for your review!

It is a fairly serious security risk to set it to the DNS name received from the Cluster resource.

Could you please let me know the risk regarding this change briefly...?

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, the authority for the connection is used to verify the peer -- e.g., for TLS, the client checks that the server's certificate matches that authority. Because of that, we essentially view it as a requirement that the authority be explicitly specified by the client application (either via the target URI used to create the channel or via an explicit channel option indicating the authority to use) rather than coming from some external control plane (e.g., being specified by the resolver or by an LB policy), because otherwise we would basically give a compromised control plane the ability to tell us to talk to some arbitrary endpoint without us actually verifing that the endpoint is authenticated as the intended peer.

}
return pName, &clusterimpl.LBConfig{
Cluster: mechanism.Cluster,
Expand Down