tls: enable multiple SSL certificate support. #5317

htuch · 2018-12-16T05:29:03Z

This PR wraps up #1319. The patch enables multiple TLS certificate
ingest for downstream TLS contexts, adds related unit and integration
tests, docs and release notes.

Risk Level: Low
Testing: Additional unit and integration tests. To avoid combinatorial
explosion, we validate mixed TLS v1.2/1.3 behavior in
ssl_integration_test only, and have more targeted certificate
selection tests in ssl_socket_Test.
Docs Changes: Added to architectural overview of TLS support.

Fixes #1319.

Signed-off-by: Harvey Tuch htuch@google.com

This PR wraps up envoyproxy#1319. The patch enables multiple TLS certificate ingest for upstream TLS contexts, adds related unit and integration tests, docs and release notes. Risk Level: Low Testing: Additional unit and integration tests. To avoid combinatorial explosion, we validate mixed TLS v1.2/1.3 behavior in ssl_integration_test only, and have more targeted certificate selection tests in ssl_socket_Test. Docs Changes: Added to architectural overview of TLS support. Fixes envoyproxy#1319. Signed-off-by: Harvey Tuch <htuch@google.com>

mattklein123

Quick drive by. Very cool to see this implemented!

mattklein123 · 2018-12-16T05:59:46Z

api/envoy/api/v2/auth/cert.proto

- // Multiple TLS certificates can be associated with the same context.
- // E.g. to allow both RSA and ECDSA certificates, two TLS certificates can be configured.
+ // :ref:`Multiple TLS certificates <arch_overview_ssl_multi_cert>` can be associated with the same
+ // context.B E.g. to allow both RSA and ECDSA certificates, two TLS certificates can be


mattklein123 · 2018-12-16T06:00:06Z

docs/root/intro/arch_overview/ssl.rst

+Multiple certificates
+---------------------
+
+:ref:`UpstreamTlsContexts <envoy_api_msg_auth.UpstreamTlsContext>` support multiple TLS


I think it's the reverse? We support multiple contexts for downstream but not for upstream?

PiotrSikora · 2018-12-16T06:04:00Z

docs/root/intro/arch_overview/ssl.rst

+---------------------
+
+:ref:`UpstreamTlsContexts <envoy_api_msg_auth.UpstreamTlsContext>` support multiple TLS
+certificates. These may be a mix of RSA and P-256 ECDSA certificates. The following rules apply:


I believe that I asked about this before, but could we enforce that at most single RSA and single P-256 ECDSA are configured? We know that anything more is never going to be used at runtime, so it's most likely misconfiguration. This should only require a few easy checks in ContextImpl() and ServerContextConfigImpl().

Yes, OK, let's go with this, it simplifies other logic later on.

PiotrSikora · 2018-12-16T06:04:37Z

docs/root/intro/arch_overview/ssl.rst

+certificates. These may be a mix of RSA and P-256 ECDSA certificates. The following rules apply:
+
+* Only the first certificate of a particular type (RSA or ECDSA) is considered.
+* Non-P256 server ECDSA certificates are rejected.


This isn't specific to multiple certificates.

(We should have it documented somewhere, but probably not here.)

I will rename this section to something like "Certificate selection" and we can keep it here.

PiotrSikora · 2018-12-16T06:05:32Z

docs/root/intro/arch_overview/ssl.rst

+
+* Only the first certificate of a particular type (RSA or ECDSA) is considered.
+* Non-P256 server ECDSA certificates are rejected.
+* The client must indicate P-256 support to be considered ECDSA capable.


This line is redundant with the more descriptive line below.

PiotrSikora · 2018-12-16T06:11:24Z

api/envoy/api/v2/auth/cert.proto

- // Multiple TLS certificates can be associated with the same context.
- // E.g. to allow both RSA and ECDSA certificates, two TLS certificates can be configured.
+ // :ref:`Multiple TLS certificates <arch_overview_ssl_multi_cert>` can be associated with the same
+ // context.B E.g. to allow both RSA and ECDSA certificates, two TLS certificates can be


s/B E.g./, e.g./, but I'd probably rephrase the whole thing as:

// :ref:`Multiple TLS certificates <arch_overview_ssl_multi_cert> can be associated with the same // context to allow both RSA and ECDSA certificates.

PiotrSikora · 2018-12-16T06:12:18Z

docs/root/intro/version_history.rst

@@ -78,6 +78,7 @@ Version history
 * tls: added support for CRLs in :ref:`trusted_ca <envoy_api_field_auth.CertificateValidationContext.trusted_ca>`.
 * tls: added support for :ref:`password encrypted private keys <envoy_api_field_auth.TlsCertificate.password>`.
 * tls: added ssl.versions.<version> to :ref:`listener metrics <config_listener_stats>` to track TLS versions in use.
+* tls: added support for :ref:`multiple server TLS certificates <arch_overview_ssl_multi_cert>`.


Nit: move it above ssl.versions.

I think ssl.versions should be moved above the other to enforce alpha order.

PiotrSikora · 2018-12-16T06:12:48Z

source/common/ssl/context_config_impl.cc

 if ((config.common_tls_context().tls_certificates().size() +
 config.common_tls_context().tls_certificate_sds_secret_configs().size()) == 0) {
 throw EnvoyException("No TLS certificates found for server context");
- } else if ((config.common_tls_context().tls_certificates().size() +
- config.common_tls_context().tls_certificate_sds_secret_configs().size()) > 1) {


As far as I recall, we still only support single certificate served over SDS, don't we?

I think you may have multiple sds config for single cluster/listener? https://github.com/envoyproxy/envoy/blob/master/api/envoy/api/v2/auth/cert.proto#L239

But it doesn't work with multiple certificates, because callback clears all other certificates on update, see:

envoy/source/common/ssl/context_config_impl.cc

Line 186 in 8475fcf

// This breaks multiple certificate support, but today SDS is only single cert.

PiotrSikora · 2018-12-16T06:13:53Z

test/common/ssl/context_impl_test.cc

+ ASSERT_EQ(3, tls_certs.size());
+ EXPECT_THAT(tls_certs[0].get().privateKeyPath(), EndsWith("selfsigned_key.pem"));
+ EXPECT_THAT(tls_certs[1].get().privateKeyPath(), EndsWith("selfsigned_key_ecdsa_p256.pem"));
+ EXPECT_THAT(tls_certs[2].get().privateKeyPath(), EndsWith("selfsigned_key.pem"));


Erm, we really shouldn't be able to load the same certificate twice.

PiotrSikora · 2018-12-16T06:21:47Z

docs/root/intro/arch_overview/ssl.rst

+Multiple certificates
+---------------------
+
+:ref:`UpstreamTlsContexts <envoy_api_msg_auth.UpstreamTlsContext>` support multiple TLS


Downstream contexts support multiple TLS certificates, not upstream.

PiotrSikora · 2018-12-16T06:22:05Z

docs/root/intro/arch_overview/ssl.rst

+* The client must indicate P-256 support to be considered ECDSA capable.
+* If the client supports P-256 ECDSA, a P-256 ECDSA certificate will be selected if present in the
+ :ref:`UpstreamTlsContext <envoy_api_msg_auth.UpstreamTlsContext>`.
+* If the client only supports RSA certificate, an RSA certificate will be selected if present in the


s/certificate/certificates/, s/an/a/(?)

repokitteh-read-only · 2018-12-16T06:25:44Z

🙀 Error while processing event:

invalid command
error: invalid command line string

🐱

Caused by: a #5317 (comment) was created by @PiotrSikora.

see: more, trace.

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora · 2018-12-16T12:01:41Z

source/common/ssl/context_config_impl.cc

- throw EnvoyException("A single TLS certificate is required for server contexts");
+ } else if (!config.common_tls_context().tls_certificates().empty() &&
+ !config.common_tls_context().tls_certificate_sds_secret_configs().empty()) {
+ throw EnvoyException("Static and dynamic TLS certificates may not be mixed in server contexts");


Nit: this isn't really "static" and "dynamic", since LDS/CDS and SDS can be both "static" and "dynamic".

Maybe just say that (i.e. LDS/CDS and SDS TLS certificates may not be mixed in server contexts) or is it too nitty-gritty detail for operators?

I think "Non-SDS and SDS TLS certificates may not be mixed.." is clearer here, as LDS/CDS doesn't refer to bootstrap static config.

secrets (i.e. "static" SDS) can be also in the bootstrap static config.

But yeah, I agree that it sounds cleaner.

PiotrSikora · 2018-12-16T12:23:07Z

source/common/ssl/context_impl.cc

@@ -259,6 +267,12 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeS
 "ECDSA certificates are supported",
 ctx.cert_chain_file_path_));
 }
+ } else if (have_rsa_cert) {


There are other certificate types (though, they are not very popular), but this code path assumes that !ECDSA == RSA, so it would potentially reject those.

PiotrSikora · 2018-12-16T12:27:40Z

test/common/ssl/ssl_socket_test.cc

+)EOF";
+
+ EXPECT_THROW_WITH_REGEX(testUtil(client_ctx_yaml, server_ctx_yaml, "", "", "", "", "", "", "",
+ "ssl.no_certificate", true, GetParam()),


The stat and connected status seem bogus here, since this will throw before reaching that stage.

Maybe consider moving those "bad configuration" tests to test/common/ssl/context_impl_test.cc, where we usually test them?

PiotrSikora · 2018-12-16T12:27:51Z

test/common/ssl/ssl_socket_test.cc

+)EOF";
+
+ EXPECT_THROW_WITH_REGEX(testUtil(client_ctx_yaml, server_ctx_yaml, "", "", "", "", "", "", "",
+ "ssl.no_certificate", true, GetParam()),


PiotrSikora · 2018-12-16T12:31:08Z

test/common/ssl/ssl_socket_test.cc

+ filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_key.pem"
+)EOF";
+
+ testUtil(client_ctx_yaml, server_ctx_yaml, "", "", "", "", "", "", "", "ssl.no_certificate", true,


Could you test a more "happy" stat, e.g. ssl.handshake?

PiotrSikora · 2018-12-16T12:32:53Z

test/common/ssl/test_data/certs.sh

- echo "constexpr char TEST_$(echo $1 | tr a-z A-Z)_CERT_SERIAL[] = \"$(openssl x509 -in $1_cert.pem -noout -serial | cut -d"=" -f2 | awk '{print tolower($0)}')\";" >> $1_cert_info.h
- echo "constexpr char TEST_$(echo $1 | tr a-z A-Z)_CERT_NOT_BEFORE[] = \"$(openssl x509 -in $1_cert.pem -noout -startdate | cut -d"=" -f2)\";" >> $1_cert_info.h
- echo "constexpr char TEST_$(echo $1 | tr a-z A-Z)_CERT_NOT_AFTER[] = \"$(openssl x509 -in $1_cert.pem -noout -enddate | cut -d"=" -f2)\";" >> $1_cert_info.h
+ generate_info_header $1
 }

 # $1=<certificate name>


Nit: add $2=[certificate file name](?).

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora

LGTM, other than the 2 typos and too many test certs, but feel free to ignore it if you disagree with my comment.

PiotrSikora · 2018-12-17T11:01:00Z

test/config/integration/certs/certs.sh

@@ -41,6 +41,11 @@ rm -f server_ecdsacert.cfg
 # Generate cert for the client.
 generate_rsa_key client ca
 generate_x509_cert client ca
+# Generate ECDSA cert for the celitn.


s/celint/client/

PiotrSikora · 2018-12-17T11:24:15Z

source/common/ssl/context_impl.cc

- ctx.is_ecdsa_ = EVP_PKEY_id(public_key.get()) == EVP_PKEY_EC;
+ const int pkey_id = EVP_PKEY_id(public_key.get());
+ if (!cert_pkey_ids.insert(pkey_id).second) {
+ throw EnvoyException(fmt::format("Failed to load certificate from chain {}, at most one "


s/certificate from chain/certificate chain from/

PiotrSikora · 2018-12-17T11:27:12Z

test/common/ssl/context_impl_test.cc

+ private_key:
+ filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem"
+ - certificate_chain:
+ filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned2_cert.pem"


Could you re-use existing certs (selfsigned & selfsigned_rsa_1024* and selfsigned_ecdsa_p256 and selfsigned_ecdsa_p384) instead of adding another set? We have a bit too many test certs already, and we could get rid of the OUTPUT_PREFIX hack in certs.sh, then.

*either copy it from #5318 or wait until it's merged (later today?)

I think if we did that, it might work, because we check for duplicate certs of a given type before validating the cert, but this is just an implementation detail which could potentially change so let's stick with this for now.

Long term, I think the right direction to go here is to have some custom Go/Python script for programatically generating the certs (since we can't rely on the openssl CLI in any Bazel automation), so we don't need to check in so many certs.

Long term, I think the right direction to go here is to have some custom Go/Python script for programatically generating the certs (since we can't rely on the openssl CLI in any Bazel automation), so we don't need to check in so many certs.

We used to do openssl in genrule and that broke once in OSX, but I think we may enable that again by using bssl binary?

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora

Thanks!

This PR wraps up envoyproxy#1319. The patch enables multiple TLS certificate ingest for downstream TLS contexts, adds related unit and integration tests, docs and release notes. Risk Level: Low Testing: Additional unit and integration tests. To avoid combinatorial explosion, we validate mixed TLS v1.2/1.3 behavior in ssl_integration_test only, and have more targeted certificate selection tests in ssl_socket_Test. Docs Changes: Added to architectural overview of TLS support. Fixes envoyproxy#1319. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

htuch requested a review from PiotrSikora December 16, 2018 05:29

htuch assigned PiotrSikora Dec 16, 2018

mattklein123 reviewed Dec 16, 2018

View reviewed changes

PiotrSikora suggested changes Dec 16, 2018

View reviewed changes

lizan self-assigned this Dec 16, 2018

htuch added 7 commits December 16, 2018 01:46

Fix docs.

dd7e4f4

Signed-off-by: Harvey Tuch <htuch@google.com>

Reject multiple SDS configs.

2efbe34

Signed-off-by: Harvey Tuch <htuch@google.com>

wip

d735985

Signed-off-by: Harvey Tuch <htuch@google.com>

wip

bd375bf

Signed-off-by: Harvey Tuch <htuch@google.com>

At most one ECDSA/RSA cert per type.

c006166

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into multi-cert

70c8a28

Cert generation automation.

bc63fe7

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora suggested changes Dec 16, 2018

View reviewed changes

htuch added 5 commits December 16, 2018 19:07

Merge remote-tracking branch 'upstream/master' into multi-cert

4d1cbba

Move multi-RSA/ECDSA cert tests to context_impl_text.

e6582d0

Signed-off-by: Harvey Tuch <htuch@google.com>

Potato potatoe.

8957777

Signed-off-by: Harvey Tuch <htuch@google.com>

Cert types other than RSA/ECDSA.

89b7d1c

Signed-off-by: Harvey Tuch <htuch@google.com>

Nits.

2cb1935

Signed-off-by: Harvey Tuch <htuch@google.com>

PiotrSikora suggested changes Dec 17, 2018

View reviewed changes

htuch added 3 commits December 17, 2018 15:22

Merge remote-tracking branch 'upstream/master' into multi-cert

24d59fd

Typos.

242f046

Signed-off-by: Harvey Tuch <htuch@google.com>

fix_format

c658615

Signed-off-by: Harvey Tuch <htuch@google.com>

lizan approved these changes Dec 17, 2018

View reviewed changes

PiotrSikora approved these changes Dec 17, 2018

View reviewed changes

htuch merged commit fdb0880 into envoyproxy:master Dec 17, 2018

htuch deleted the multi-cert branch December 17, 2018 22:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: enable multiple SSL certificate support. #5317

tls: enable multiple SSL certificate support. #5317

htuch commented Dec 16, 2018 •

edited

Loading

mattklein123 left a comment

mattklein123 Dec 16, 2018

mattklein123 Dec 16, 2018

PiotrSikora Dec 16, 2018

htuch Dec 16, 2018

PiotrSikora Dec 16, 2018

htuch Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

htuch Dec 16, 2018

PiotrSikora Dec 16, 2018

lizan Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

repokitteh-read-only bot commented Dec 16, 2018

PiotrSikora Dec 16, 2018

htuch Dec 17, 2018

PiotrSikora Dec 17, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora Dec 16, 2018

PiotrSikora left a comment

PiotrSikora Dec 17, 2018

PiotrSikora Dec 17, 2018

PiotrSikora Dec 17, 2018

htuch Dec 17, 2018

lizan Dec 17, 2018

PiotrSikora left a comment

tls: enable multiple SSL certificate support. #5317

tls: enable multiple SSL certificate support. #5317

Conversation

htuch commented Dec 16, 2018 • edited Loading

mattklein123 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

repokitteh-read-only bot commented Dec 16, 2018

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

PiotrSikora 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

PiotrSikora left a comment

Choose a reason for hiding this comment

htuch commented Dec 16, 2018 •

edited

Loading