A29: xDS-Based Security for gRPC Clients and Servers #184

sanjaypujare · 2020-06-03T23:32:52Z

Draft PR for xDS transport security

markdroth

I think we should have had more internal consensus on this document before we published it for external review.

Please let me know if you have any questions about any of this. Thanks!

markdroth · 2020-06-05T22:39:57Z

A29-xds-tls-security.md

@@ -0,0 +1,340 @@
+A29: xDS-Based Security for gRPC Clients and Servers


Why don't we do this in two separate gRFCs, one for the client side and another for the server side? I think the server-side security design will need to wait for a separate gRFC written by @ejona86 for server-side xDS integration, but the client-side design can proceed without waiting for that.

markdroth · 2020-06-08T19:23:36Z

A29-xds-tls-security.md

+
+We leverage the xDS API's TLS configuration capabilities to achieve this and extend
+gRPC's channel and server credentials concept to enable users to take advantage of
+the xDS control plane. The proposal is based on version 3 of the xDS API but


I'll be putting together a gRFC for xDS v3 support early next quarter, and we can reference it here. In the interim, it might be a good idea to put a TODO here, so that we don't forget to add a link.

markdroth · 2020-06-08T19:27:38Z

A29-xds-tls-security.md

+auto channel = grpc::CreateChannel("xds:///myserver:8080", channel_creds);
+```
+
+`XdsCredentials` is a type of (or subclass of) a gRPC channel credentials. The user


We should explicitly state that the client will not use the certificate information provided by the xDS server unless XdsCredentials is used. If they use any other type of credentials, the certificate information provided by the xDS server will be ignored, even though the rest of the xDS data will continue to be used.

markdroth · 2020-06-08T19:34:57Z

A29-xds-tls-security.md

+
+```
+// build a server to use xDS 
+ServerBuilder builder;


This will almost certainly not be how the server-side xDS API looks. We're still working on the design details on this, which is why I think that the server-side changes should be moved to a separate gRFC.

markdroth · 2020-06-08T19:37:07Z

A29-xds-tls-security.md

+
+#### Fallback Credentials
+
+Both `XdsCredentials` and `XdsServerCredentials` require a mandatory value for something


This parameter is mandatory, but the examples above don't include it. This is likely to cause confusion.

I suggest moving this information up into the section where XdsCredentials is introduced.

markdroth · 2020-06-08T20:11:39Z

A29-xds-tls-security.md

+[Certificate Provider](#certificate-provider) for each of the two `certificate_provider`
+values received. If these two values turn out to be the same, the same Certificate
+Provider will be used in both the cases. The [Distributor](#distributor)s
+associated with the Certificate Providers are passed via channel args to all the descendants


The discussion of "channel args" seems specific to C-core. Can we phrase this in a more implementation-agnostic way?

markdroth · 2020-06-08T20:14:22Z

A29-xds-tls-security.md

+of the CDS policy which enables a subchannel created from any of these descendant policies
+to receive Certificate updates and provide them to the associated security connector.
+
+![gRPC xDS Security Client Flow Diagram](A29_graphics/grpc_xds_security_client_flow.png)


There's actually another layer of indirection here that is not captured in either the description or the diagram. The distributor passed down into the subchannels is not the same one that provides data from the CertificateProvider to the CDS policy. This layer of indirection is important for two reasons:

If we are using a different CertificateProvider for the root cert than for the identity cert, the CDS policy needs to merge the data from two different distributors before passing it down to the subchannels.

We don't want to have to recreate all subchannels whenever we get a CDS update that changes the CertificateProvider.

markdroth · 2020-06-08T20:15:22Z

A29-xds-tls-security.md

+
+![gRPC xDS Security Client Flow Diagram](A29_graphics/grpc_xds_security_client_flow.png)
+
+#### gRPC Server (Listener) Side Changes


As mentioned above, let's remove server-side discussion from this document for now.

markdroth · 2020-06-08T20:29:02Z

A29-xds-tls-security.md

+SDS configuration from the xDS control plane. For gRPC to seamlessly interoperate with
+Envoy, it needs to be able to consume the same SDS config. It is possible to support
+SDS within this plugin architecture where an SDS client can be implemented as a
+Certificate Provider plugin which talks to the SDS server (a Node Agent) over SDS and


s/a Node Agent/typically a local daemon, like Istio's Node Agent/

markdroth · 2020-06-08T20:34:22Z

A29-xds-tls-security.md

+gets them from a Certification Authority). Current Envoy deployments receive
+SDS configuration from the xDS control plane. For gRPC to seamlessly interoperate with
+Envoy, it needs to be able to consume the same SDS config. It is possible to support
+SDS within this plugin architecture where an SDS client can be implemented as a


Let's not be proscriptive here. We haven't yet agreed on whether we would implement this as a CertificateProvider plugin or as a separate code path. And in the context of this document, it doesn't really matter which of those routes we choose. I think the important thing to say here is that we do intend to eventually add SDS support as well.

howardjohn · 2021-01-14T18:54:27Z

A29-xds-tls-security.md

+A prerequisite is to use xDS on the client side as described
+[here](https://github.com/grpc/proposal/blob/master/A27-xds-global-load-balancing.md#grpc-client-architecture).
+The gRPC developer also needs to use a new type of (client-side) credentials called
+`XdsCredentials` to use the feature on the client side. An example of this in C++ is:


This seems to drift from how Envoy behaves in unexpected ways. This is all from the perspective of a user, and not someone familiar with gRPC internals, but this seems un-intuitive to me.

Today, when writing an XDS control plane, the control plane is the source of truth and declares all behavior for the client. Aside from mismatched versions (sending new fields to old Envoys), if I send the same config to two Envoys I would expect them to both start using that configuration.

However, by requiring XdsCredentials, I believe that is no longer the case. The control plane may send the gRPC client some config to use mTLS, and its dropped on the floor. I think at the very least this could use some indication (perhaps client_features?) to the server that its going to ignore the TLS settings. As a control plane developer, its already confusing enough which fields gRPC supports - if its selectively supporting them it sounds like a nightmare to develop around, and certainly a major challenge to just drop an existing XDS server in place (rather than making a custom gRPC XDS server).

One example of where this could blow up - Istio's XDS server automatically determines whether to tell clients/servers to use mTLS based on its global view of the world. If well tell all connected clients to use mTLS, but half of them decide to ignore it, we are going to have problems.

I also feel like this has a dangerous precedent. Why is TLS the only setting that can override the XDS response? Will we soon need add a grpc::XdsRetryOptions, a grpc::XdsLoadBalancingPolicy, etc that overrides the XDS response and applies some local rules instead? If so, we may suffer the same issues mentioned above. If not, it feels inconsistent to me.

Finally, by adding this requirement, there are artificial barriers to adoption, as a user cannot just drop in the xds:/// scheme, but rather needs more code changes. This is discussed extensively in grpc/grpc-go#4123 though, so it may make sense to continue the discussion there instead of splitting the conversation.

Unfortunately, we can't do that with breaking the security guarantees of our existing API. The gRPC API requires the application to explicitly indicate the type of credentials to use, and they have an expectation that we will use the type of credentials that they told us to use. We cannot simply decide to ignore what the user asked us to do and instead use what xDS tells us to do, because (e.g.) if they told us to use TLS and xDS told us to use plaintext, we would wind up sending their data unencrypted, which would be completely surprising to them and could cause a significant security hole.

The only safe way for us to do this without causing security problems is for the user to explicitly tell us that it's okay to trust xDS to tell us what credentials to use. If they don't do that, we can't just assume it's okay to do so. I understand that this does mean that applications need to make an additional change to use the xDS mTLS config, but we can't avoid it in this case.

I do not expect this same thing to come up for other features (e.g., retry options or LB policy). It's a fairly security-specific constraint.

@markdroth understood, thanks for the explanation. I will say if you flipped xds <-> grpc, it would all still make sense. Ultimately, we have two different sources of configuration for the same field, which is pretty tricky.

I can imagine a use case where I just want XDS for load balancing, and now I get a big security hole since the XDS server didn't send TLS configs. However, I can also see the inverse where someone wants essentially an Envoy replacement and is happy with the xDS being the complete source of truth. Have we explored exposing such information in the bootstrap configuration as well? One benefit of bootstrap vs code is the latter can be more easily changed I would imagine, and likely invisible to the user.

I assume that the inverse case you're talking about is where a user was originally sending gRPC traffic through Envoy, so their client and server are both configured to use InsecureCreds (i.e., plaintext), and now they want to convert to proxyless.

In that situation, the first thing they would do is change the server side to use the new xDS-enabled server APIs that we're working on. As part of that change, they would need to have their server listen on an external IP instead of localhost, and they would need to specify creds at the same time, so it should be fairly obvious that InsecureCreds is not the right thing anymore (although we should make sure to clearly document this). And if they get the server side right, then if they change the client to use an "xds:" URI without using XdsCreds, then they'll immediately see the problem, because the client will be unable to connect to the server.

I don't think the bootstrap file solves this problem. The fundamental issue here is that the user has told us what creds to use via the gRPC API, and I think we would severely violate their expectations if we do something different than what they told us due to some external source of data, regardless of whether that external source is the bootstrap file or the xDS server.

ejona86 · 2021-04-26T19:05:56Z

A29-xds-tls-security.md

+or [combined_validation_context.validation_context_certificate_provider](https://github.com/envoyproxy/envoy/blob/35702fed462f63a0a237cfbfdf26184272207c11/api/envoy/extensions/transport_sockets/tls/v3/tls.proto#L145). For mTLS, it will also include local identity certificate
+information in [tls_certificate_certificate_provider](https://github.com/envoyproxy/envoy/blob/35702fed462f63a0a237cfbfdf26184272207c11/api/envoy/extensions/transport_sockets/tls/v3/tls.proto#L169).
+
+If a channel is using `XdsCredentials`, whenever its CDS balancer receives a


I may be missing it, but I don't see any mention of how clients will do server authorization/hostname verification.

This PR needs more work and your point will be addressed as part of that rework

sanjaypujare · 2021-06-04T23:11:02Z

I am going to close this PR and open a brand new one after incorporating comments and other changes in the proposal. Let me know if that's going to be a problem (e.g. links to this PR might break etc)

sanjaypujare · 2021-06-08T14:47:58Z

Closing this in favor of another one.

A29: xDS-Based Security for gRPC Clients and Servers

48f23e5

sanjaypujare mentioned this pull request Jun 4, 2020

docs: add CertificateProvider documentation envoyproxy/envoy#11379

Closed

sanjaypujare marked this pull request as ready for review June 8, 2020 18:12

added a link to grpc.io thread

f1c99a2

sanjaypujare marked this pull request as draft June 8, 2020 19:08

markdroth requested changes Jun 8, 2020

View reviewed changes

srini100 mentioned this pull request Jan 14, 2021

xds: allow usage without specific import of xds package grpc/grpc-go#4123

Closed

howardjohn reviewed Jan 14, 2021

View reviewed changes

ejona86 reviewed Apr 26, 2021

View reviewed changes

sanjaypujare closed this Jun 8, 2021

howardjohn mentioned this pull request Nov 2, 2021

Expose canonical-name and canonical-revision to grpc-agent container istio/istio#35834

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A29: xDS-Based Security for gRPC Clients and Servers #184

A29: xDS-Based Security for gRPC Clients and Servers #184

sanjaypujare commented Jun 3, 2020

markdroth left a comment

markdroth Jun 5, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

markdroth Jun 8, 2020

howardjohn Jan 14, 2021

markdroth Jan 14, 2021

howardjohn Jan 14, 2021

markdroth Jan 14, 2021

ejona86 Apr 26, 2021

sanjaypujare Apr 27, 2021

sanjaypujare commented Jun 4, 2021

sanjaypujare commented Jun 8, 2021

		@@ -0,0 +1,340 @@
		A29: xDS-Based Security for gRPC Clients and Servers


		#### Fallback Credentials

		Both `XdsCredentials` and `XdsServerCredentials` require a mandatory value for something


		![gRPC xDS Security Client Flow Diagram](A29_graphics/grpc_xds_security_client_flow.png)

		#### gRPC Server (Listener) Side Changes

A29: xDS-Based Security for gRPC Clients and Servers #184

A29: xDS-Based Security for gRPC Clients and Servers #184

Conversation

sanjaypujare commented Jun 3, 2020

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanjaypujare commented Jun 4, 2021

sanjaypujare commented Jun 8, 2021