xds/clusterresolver: set ServerName to an address for LogicalDNS #5793
Conversation
…r matches server name used in TLS handshake when the latter is overridden by the name resolver
| @@ -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})) | |||
There was a problem hiding this comment.
This PR just adds this change.
Other changes are inherited from #5748 as I described in the description of this PR.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...?
There was a problem hiding this comment.
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.
because this change needs #5748 for setting
:authorityheader.What
Motivation
With the current
xds/clusterresolverimplementation, when we useLogicalDNS, the:authorityheader of a request gets to the xds target name (e.g. if we usexds:///xds-targetas a URI, the:authorityheader will bexds-target).This causes a problem if we use some proxies that use
:authorityheader meaningfully.One such proxy is Google Cloud Run, a managed container platform. Google Cloud Run creates a dedicated domain for each deployment like
my-service-xxx.run.app. Even though we use this domain for a request URI, Google Cloud Run does not route the traffic as expected if the:authorityheader of the request does not match this dedicated domain. Therefore, when we distribute the target URI for Google Cloud Run deployment via xds (CDS) with LogicalDNS to a gRPC client, the gRPC client can not reach out to the desired deployment.I know my use case (not for Service Mesh) might be a edge case for the gRPC xds feature, but I'd like it use proper
:authorityheader as I'm using Google Cloud Run.