TLS: Improve story with intermediate and/or multiple CAs

[PiotrSikora]

(forked from #615)

Currently, Envoy assumes that:

• there is single CA in ca_cert_file (at least when it comes to client certificates, displaying CA information and calculating expiration dates),
• there are no intermediates in cert_chain_file (at least for the purpose of calculating expiration dates).

@mattklein123 How useful is the expiration date tracking in practice? Are you using this in production?

@timperrett Could you verify that this fixes the issues you described in #615?

diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc
index 1ca93c4b7..aa94ba32f 100644
--- a/source/common/ssl/context_impl.cc
+++ b/source/common/ssl/context_impl.cc
@@ -45,9 +45,12 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, Contex
           fmt::format("Failed to load verify locations file {}", config.caCertFile()));
     }
 
-    // This will send an acceptable CA list to browsers which will prevent pop ups.
-    rc = SSL_CTX_add_client_CA(ctx_.get(), ca_cert_.get());
-    RELEASE_ASSERT(1 == rc);
+    bssl::UniquePtr<STACK_OF(X509_NAME)> list(SSL_load_client_CA_file(config.caCertFile().c_str()));
+    if (nullptr == list) {
+      throw EnvoyException(
+          fmt::format("Failed to load verify locations file {}", config.caCertFile()));
+    }
+    SSL_CTX_set_client_CA_list(ctx_.get(), list.release());
 
     verify_mode = SSL_VERIFY_PEER;
   }

[mattklein123]

How useful is the expiration date tracking in practice? Are you using this in production?

Extremely useful. We have dashboards and alarms based on this information. Definitely can't lose it.

[timperrett]

@mattklein123 huh, this should be documented for sure - i had no idea Envoy reported that information! That would indeed be useful when using externally rooted certificates

[timperrett]

@PiotrSikora absolutely - i'll do it mid-morning and report back. Thanks for tracking this down!

[mattklein123]

@timperret the admin endpoint is documented (though it could be better):
https://lyft.github.io/envoy/docs/operations/admin.html#get--certs

But I don't think the server level stats are now that I search for it:
https://github.com/lyft/envoy/blob/master/source/server/server.h#L37

Will make a note to document unless someone else gets to it first.

[timperrett]

@PiotrSikora excellent - this does indeed seem to resolve the issue. I tried the scenarios previously enumerated in #615, and it works!

HUZZAH!

Nice one @PiotrSikora 🍻 🏅

[timperrett]

@PiotrSikora one bit of feedback on this change: in certain cases, I need to use a browser (mainly for tooling cases that are secured with mTLS). Safari prompts me for a client certificate, but Chrome and Firefox do not... they seem to get stuck in a loop of some kind. When we use the same certs with nginx this doesn't happen, so im wondering if there's some subtle bug here?

[timperrett]

@PiotrSikora ok some more information on this: best i can tell right now, it appears that its actually verifying against the list of CAs (as per my previous note at the end of last week), but what its not doing - and i believe this is incorrect - is its still only presenting the head of the list as an acceptable client CA, which is why the browsers don't prompt for a choice.

[PiotrSikora]

@timperrett it definitely sends a list of CAs with the patch. You can see that list when connecting with openssl s_client, under Acceptable client certificate CA names. It's 1 item with the master branch, and multiple items with the patch.

I also verified that the CertificateRequest message is sent to Chrome with both: tcpdump and chrome://net-internals.

Please note that Chrome won't ask for the client certificate (via the popup window) if it doesn't have any certificate that matches client certificate CA from the list sent by the server... But you say that it works with NGINX, so it should have matching certificate.

Are you using exactly the same files with both: NGINX & Envoy? NGINX is also using SSL_CTX_set_client_CA_list(), so there shouldn't be a difference.

You didn't revert the patch, by any chance, did you?

[timperrett]

@PiotrSikora todays lesson is that Tim should not be programming past midnight, as no productive work can happen. Had been upgrading Envoy and testing out various patches and did indeed use the wrong build when i was testing this on another project, making last nights lack of sleep entirely fruitless.

Very sorry for the noise - i've re-tested this morning with the correct build and yep, you're right: its sending down the full list of acceptable CAs. 🍻

[mattklein123]

@PiotrSikora this got auto closed by the other PR. Since you said "partially fixes" I'm going to reopen this. Let me know if it should be closed.

[PiotrSikora]

The negotiation with multiple CAs is fixed, the part that's still broken is /certs and calculating expiration dates. Should we track it here (and keep this open) or split into another issue? Up to you, I don't have a preference.

[mattklein123]

Tracking here is fine with me.

[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.

[kramerul]

We are also interested in supporting this scenario.

But in our case I think it looks a little bit different.
We would like to have an intermediate CA ( subca.crt signed by a root CA ca.crt), which is not known to envoy, but envoy is aware of the root CA ca.crt. It should be possible to open the connection to envoy with a client certificate subca-client.crt, which is signed by this intermediate CA subca.crt.

Openssl supports such a scenario:

openssl s_server -accept 9001 -www  -CAfile ca.crt \
    -cert server.crt -key server.key  -verify 3

openssl s_client   -connect  <server>:9001   \
   -key subca-client.key -cert subca-client.crt  \
    -CAfile subca.crt   \
    -servername <server>

[dsymonds]

I am also interested in improvements here.

My use case has not been directly mentioned before: it is to support TLS cert rotation when using plain certs as a CA file. That is, I have Envoy doing TLS termination for clients that provide self-signed certificates. Envoy is configured to load those same certs as the downstream tls_context.common_tls_context.validation_context.trusted_ca. This works fine, except when trying to rotate certs gracefully with the usual three-step shuffle of loading both certs into the trusted CA pool, switching clients to using the new certs, then unloading the old cert from the trusted CA pool. Envoy only checks client-presented certs against the first entry in the CA pool, when it seems like it could easily check them all.

[tazmanian10]

Hello,

I have been trying to set multiple trusted CAs through Secret Discovery Service, however it seems that only the first one in the file is taken into account. Is there any other way i am missing out in order to set a list of trusted CAs, or is the feature not supported at all through SDS?

[PiotrSikora]

Multiple trusted CAs should work in the same file / SDS resource. How are you testing this, exactly? In any case, please open a separate issue, since this issue is tracking expiration date when using intermediate certificates.

[jhanbo]

Hello, we are facing the same issue, just only the first certificate in the file is picked up and the rest is ignored

[jhanbo]

Hello, we are facing the same issue, just only the first certificate in the file is picked up and the rest is ignored

Hello, just to confirm, it works pretty fine. Our issue was that we used Multiple CAs which have the same issuer.