Support multiple server TlsCertificates per DownstreamTlsContext

[htuch]

In the v2 API, we allow multiple TlsCertificates to be specified per DownstreamTlsContext (https://github.com/lyft/envoy-api/blob/master/api/tls_context.proto#L107). In the current Envoy implementation, each TLS context only supports a single certificate/key (https://github.com/lyft/envoy/blob/master/source/common/ssl/context_impl.cc#L77). This issue will track the design and implementation of multiple cert support. @PiotrSikora

[mattklein123]

This is a dup of #95 right? We should implement SNI in the "v1" world also, as it is trivial to do so. @PiotrSikora if you are going to work on this can we maybe do the v1 work first since it will be used in v2 as well?

[htuch]

I was interested in understanding what it means to have multiple certs here. Piotr pointed out there are a number of different options, e.g. are we doing this with SNI, different cert types. If it's just SNI then it's a dupe.

[mattklein123]

Yeah that's true, this is beyond SNI, like having both EC and RSA certs potentially I guess? Still, I would like to see us implement SNI ASAP since I'm getting tired of getting asked about it. :)

[PiotrSikora]

This (repeated TlsCertificates in v2 API) is for having different types of certificates for the same context (e.g. RSA and ECDSA), which is different from full SNI support (#95). Let's keep this simple and move SNI discussion back to #95.

[stale]

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

[ggreenway]

This is still needed. I'll implement eventually if nobody else has first. I specifically need EC and RSA certs (with the same SANs) on the same context.

[PiotrSikora]

@ggreenway I discussed this on and off with @davidben. The reason why I didn't implement this in Envoy via "userland" certificate selection yet is that the plan is for BoringSSL to eventually re-add the support for multiple certificates of different types in the same context (exactly for the RSA & ECDSA use case).

[ggreenway]

Any idea what the timeline is for that change to BoringSSL?

[ggreenway]

@PiotrSikora is the "userland" approach to create a different SSL_CTX for each cert type, and set the CTX in one of the callbacks based on supported cipher types in the ClientHello?

[davidben]

Sorry about the delay on that feature. I'm kind of running around all over the place right now due to an upcoming move, and a couple of tasks popped up in front of it that took longer than expected. I do intend to finally chew on it this week, though if it's not done by IETF and my move, it might get delayed.

[PiotrSikora]

@ggreenway yes, pretty much. The "userland" approach isn't a complex change in Envoy, but the support for RSA+ECDSA isn't something frequently requested by the community, and since the proper support for it is going to be added to BoringSSL sooner rather than later, I'd rather not add it now, unless required for a production deployment.

Do you have any deadline for when you need this feature to be available?

[ggreenway]

I don't have a hard date right now, but I'd guess I'll probably need it within 3 months. I'll hold off as long as possible in hopes that BoringSSL adds the functionality we need. But if I need to do it in userland, and then we rip it out later, that's fine. I'll comment here when I have firmer dates, or when I'm going to start working on it.

[htuch]

I'm going to take a shot at this one, since we need this sooner than later.

[htuch]

FTR, I've had internal discussions with @PiotrSikora and @davidben. The PoR is to add in explicit client handshake and certificate type detection to Envoy for the initial implementation, as BoringSSL won't add this for some time. When this feature arrives in BoringSSL, we will replace our implementation with that.

[htuch]

Quick design question @mattklein123 @ggreenway @lizan @PiotrSikora. Do we want to do the client handshake capability detection in the TLS inspector or directly in the TLS transport socket?

A reason for TLS inspector include that we're doing ClientHello peaking already for SNI/ALPN. A reason against is that mixed certificate type doesn't necessarily imply SNI use, so we will have additional scenarios for requiring the inspector extension.

I'm thinking we'll be doing it in the inspector, since this is conceptually coherent with what's done for other handshake attributes, but just wanted to check your thoughts.

[htuch]

Oh right, we inject the TLS inspector automatically anyway, so the above is moot, I see.

[mattklein123]

In general doing it as part of the inspector sounds good to me, and then we can use that data to drive later decisions around which cert to use.

[htuch]

A couple more things to bounce off folks on this issue; the resolution order for certificates. What I'm thinking is:

1. We keep track of the first ECDSA and first RSA certs in a certificate chain.
2. For server contexts, we will use the client handshake ECDSA indicators from the group and cipher suite extensions to determine which to use.

Some design choices we can make:

1. We could skip any certificates that don't validate and only select the first matching one in (1) above. Or, we could maintain existing semantics and reject any invalid cert/key pairs as a config rejection. Which to do?
2. I'm wondering how best to handle client contexts. Do we just pick the first certificate and call it a day?

There's a tradeoff here between keeping it simple (i.e. the cert list has a cardinality <= 2) vs. being more flexible. Either way, we'll need to document this well, as it's probably going to be confusing to reason about. Ideally we maybe should just have a singleton ECDSA and a singleton RSA option in the config and let the earlier non-Envoy config pipeline do this resolution.

[mattklein123]

Or, we could maintain existing semantics and reject any invalid cert/key pairs as a config rejection.

Big +1. IMO much less surprising. I would do this until if/when someone complains which I would find very surprising and we can potentially push back on.

I'm wondering how best to handle client contexts. Do we just pick the first certificate and call it a day?

Then what about just continuing to force 1 (with config error for > 1) for client until someone actually needs it and can come up with a sensible way of configuring it?

[lizan]

IIUC per https://github.com/envoyproxy/envoy/blob/master/source/server/listener_manager_impl.cc#L243
TLS inspector is only injected when you have certain filter_chain_match. So if a listener is configured for pre-knowledge TLS, then it is valid that no TLS inspector is injected.

Also do we want to select filter chain based on ClientHello certificate type? If no perhaps adding in TLS transport is better and it is easier to replace with BoringSSL implementation? (Assuming BoringSSL implementation will be having multiple certificates in same SSL_CTX)

[htuch]

After some discussion with @davidben and @PiotrSikora, I think it's clear that we should have the ECDSA detection happen in the TLS socket, rather than TLS inspector. The rationale here is that BoringSSL will eventually subsume this capability, so we should try and localize it rather than plumb deprecated ECDSA details (signature algorithms, cipher suites) across the listener socket objects.