tls: enable multiple SSL certificate support. #5317
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -308,13 +308,12 @@ ServerContextConfigImpl::ServerContextConfigImpl( | |
|
|
||
| return ret; | ||
| }()) { | ||
| 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"); | ||
|
There was a problem hiding this comment. As far as I recall, we still only support single certificate served over SDS, don't we? There was a problem hiding this comment. 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 There was a problem hiding this comment. But it doesn't work with multiple certificates, because callback clears all other certificates on update, see: |
||
| } else if (!config.common_tls_context().tls_certificates().empty() && | ||
| !config.common_tls_context().tls_certificate_sds_secret_configs().empty()) { | ||
| throw EnvoyException("SDS and non-SDS TLS certificates may not be mixed in server contexts"); | ||
| } | ||
| } | ||
|
|
||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,8 @@ | ||
| #include <string> | ||
| #include <vector> | ||
|
|
||
| #include "envoy/api/v2/auth/cert.pb.validate.h" | ||
|
|
||
| #include "common/json/json_loader.h" | ||
| #include "common/secret/sds_api.h" | ||
| #include "common/ssl/context_config_impl.h" | ||
|
|
@@ -22,6 +24,7 @@ | |
| #include "openssl/x509v3.h" | ||
|
|
||
| using Envoy::Protobuf::util::MessageDifferencer; | ||
| using testing::EndsWith; | ||
| using testing::NiceMock; | ||
| using testing::ReturnRef; | ||
|
|
||
|
|
@@ -295,6 +298,52 @@ TEST_F(SslContextImplTest, TestNoCert) { | |
| EXPECT_TRUE(context->getCertChainInformation().empty()); | ||
| } | ||
|
|
||
| // Multiple RSA certificates are rejected. | ||
| TEST_F(SslContextImplTest, AtMostOneRsaCert) { | ||
| envoy::api::v2::auth::DownstreamTlsContext tls_context; | ||
| NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context; | ||
| const std::string tls_context_yaml = R"EOF( | ||
| common_tls_context: | ||
| tls_certificates: | ||
| - certificate_chain: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" | ||
| 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" | ||
|
There was a problem hiding this comment. Could you re-use existing certs ( *either copy it from #5318 or wait until it's merged (later today?) There was a problem hiding this comment. 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. There was a problem hiding this comment.
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? |
||
| private_key: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" | ||
| )EOF"; | ||
| MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); | ||
| ServerContextConfigImpl server_context_config(tls_context, factory_context); | ||
| EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), | ||
| EnvoyException, | ||
| "at most one certificate of a given type may be specified"); | ||
| } | ||
|
|
||
| // Multiple ECDSA certificates are rejected. | ||
| TEST_F(SslContextImplTest, AtMostOneEcdsaCert) { | ||
| envoy::api::v2::auth::DownstreamTlsContext tls_context; | ||
| NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context; | ||
| const std::string tls_context_yaml = R"EOF( | ||
| common_tls_context: | ||
| tls_certificates: | ||
| - certificate_chain: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_cert.pem" | ||
| private_key: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_key.pem" | ||
| - certificate_chain: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned2_ecdsa_p256_cert.pem" | ||
| private_key: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_key.pem" | ||
| )EOF"; | ||
| MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_context_yaml), tls_context); | ||
| ServerContextConfigImpl server_context_config(tls_context, factory_context); | ||
| EXPECT_THROW_WITH_REGEX(manager_.createSslServerContext(store_, server_context_config, {}), | ||
| EnvoyException, | ||
| "at most one certificate of a given type may be specified"); | ||
| } | ||
|
|
||
| class SslServerContextImplTicketTest : public SslContextImplTest { | ||
| public: | ||
| void loadConfig(ServerContextConfigImpl& cfg) { | ||
|
|
@@ -575,7 +624,7 @@ TEST(ClientContextConfigImplTest, RSA1024Cert) { | |
| Stats::IsolatedStoreImpl store; | ||
| EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), | ||
| EnvoyException, | ||
| "Failed to load certificate chain from .*selfsigned_rsa_1024_cert.pem, " | ||
| "only RSA certificates with 2048-bit or larger keys are supported"); | ||
| } | ||
|
|
||
|
|
@@ -616,7 +665,7 @@ TEST(ClientContextConfigImplTest, NonP256EcdsaCert) { | |
| Stats::IsolatedStoreImpl store; | ||
| EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), | ||
| EnvoyException, | ||
| "Failed to load certificate chain from .*selfsigned_ecdsa_p384_cert.pem, " | ||
| "only P-256 ECDSA certificates are supported"); | ||
| } | ||
|
|
||
|
|
@@ -942,28 +991,34 @@ TEST(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) { | |
| "Unknown static certificate validation context: missing"); | ||
| } | ||
|
|
||
| // Multiple TLS certificates are supported. | ||
| TEST(ServerContextConfigImplTest, MultipleTlsCertificates) { | ||
| envoy::api::v2::auth::DownstreamTlsContext tls_context; | ||
| NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context; | ||
| EXPECT_THROW_WITH_MESSAGE( | ||
| ServerContextConfigImpl client_context_config(tls_context, factory_context), EnvoyException, | ||
| "No TLS certificates found for server context"); | ||
| const std::string rsa_tls_certificate_yaml = R"EOF( | ||
| certificate_chain: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert.pem" | ||
| private_key: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key.pem" | ||
| )EOF"; | ||
| const std::string ecdsa_tls_certificate_yaml = R"EOF( | ||
| certificate_chain: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_cert.pem" | ||
| private_key: | ||
| filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_ecdsa_p256_key.pem" | ||
| )EOF"; | ||
| MessageUtil::loadFromYaml(TestEnvironment::substitute(rsa_tls_certificate_yaml), | ||
| *tls_context.mutable_common_tls_context()->add_tls_certificates()); | ||
| MessageUtil::loadFromYaml(TestEnvironment::substitute(ecdsa_tls_certificate_yaml), | ||
| *tls_context.mutable_common_tls_context()->add_tls_certificates()); | ||
| ServerContextConfigImpl server_context_config(tls_context, factory_context); | ||
| auto tls_certs = server_context_config.tlsCertificates(); | ||
| ASSERT_EQ(2, tls_certs.size()); | ||
| EXPECT_THAT(tls_certs[0].get().privateKeyPath(), EndsWith("selfsigned_key.pem")); | ||
| EXPECT_THAT(tls_certs[1].get().privateKeyPath(), EndsWith("selfsigned_ecdsa_p256_key.pem")); | ||
| } | ||
|
|
||
| TEST(ServerContextConfigImplTest, TlsCertificatesAndSdsConfig) { | ||
|
|
@@ -983,7 +1038,16 @@ TEST(ServerContextConfigImplTest, TlsCertificatesAndSdsConfig) { | |
| tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); | ||
| EXPECT_THROW_WITH_MESSAGE( | ||
| ServerContextConfigImpl server_context_config(tls_context, factory_context), EnvoyException, | ||
| "SDS and non-SDS TLS certificates may not be mixed in server contexts"); | ||
| } | ||
|
|
||
| TEST(ServerContextConfigImplTest, MultiSdsConfig) { | ||
| envoy::api::v2::auth::DownstreamTlsContext tls_context; | ||
| tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); | ||
| tls_context.mutable_common_tls_context()->add_tls_certificate_sds_secret_configs(); | ||
| EXPECT_THROW_WITH_REGEX( | ||
| MessageUtil::validate<envoy::api::v2::auth::DownstreamTlsContext>(tls_context), | ||
| EnvoyException, "Proto constraint validation failed"); | ||
| } | ||
|
|
||
| TEST(ServerContextConfigImplTest, SecretNotReady) { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,23 +1,23 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIID0jCCArqgAwIBAgIJAIcAyF1+jvpEMA0GCSqGSIb3DQEBCwUAMHYxCzAJBgNV | ||
| BAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNp | ||
| c2NvMQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2luZWVyaW5nMRAw | ||
| DgYDVQQDDAdUZXN0IENBMB4XDTE4MTIxNzIwMTcyNVoXDTIwMTIxNjIwMTcyNVow | ||
| djELMAkGA1UEBhMCVVMxEzARBgNVBAgMCkNhbGlmb3JuaWExFjAUBgNVBAcMDVNh | ||
| biBGcmFuY2lzY28xDTALBgNVBAoMBEx5ZnQxGTAXBgNVBAsMEEx5ZnQgRW5naW5l | ||
| ZXJpbmcxEDAOBgNVBAMMB1Rlc3QgQ0EwggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAw | ||
| ggEKAoIBAQDMuACQq9YTqCCpej9zgiUOurHJl0AhYp4uXP5nCF/Sxtf6olu97jDs | ||
| T6XZzsow4LZRYsiVdLNklNdnGdFiqndl4jKJtnLQ+XdVMJcTP/kVVflx9NH1kuWt | ||
| HqOBuIktv5FYJA3cDc0t8B/myZOl8K7nlJGoToudnV7PmZnfz1NMb+QaIYIXQyio | ||
| 5rppHq4aC8TTL3VOZL93wzjBXDT2OtOBWec8yCnFm3JfU37BfAM2mbTkgpol7qvA | ||
| UsjAef3C6gbIvL79CNA1G6sRDR6pjvQ8XhNr/hXj4IeUrMnwMaRub5MXo2/naTRA | ||
| zMSenxKNJ0FL9y7fBmkySiEur6fZLbHJAgMBAAGjYzBhMA8GA1UdEwEB/wQFMAMB | ||
| Af8wDgYDVR0PAQH/BAQDAgEGMB0GA1UdDgQWBBTTfgjkmQUAL7gTfjZPv+FRy5KS | ||
| TzAfBgNVHSMEGDAWgBTTfgjkmQUAL7gTfjZPv+FRy5KSTzANBgkqhkiG9w0BAQsF | ||
| AAOCAQEAbtJILJvq+KfayHHdHkPrC4XWScuUwlSDPjy614oXzJfjkV34T6YsInpf | ||
| xqiIkm3WuQYkoNAitlU/JGt4lZdaUL6JKSaAq2WFueYpVLPQkVN5M0U62rGP8sJ9 | ||
| q5B2W5ZW/X1/cPimdpRzU0TGW5DDdSP8KXq4ND4SBaQfttw/xTnSCHJ1sL+Xy8KP | ||
| 6jAPuCcH/6jWx6qYTddI8E7OtHWeOFQ1iNomTBRC8T2F+vnHnu4NHM5aYDA5kxr+ | ||
| lgNS2qZJKUApKqi7s4/d/wc1tFBt2/kZYNfoTvpNtYuEMdPwZ1Hq8BhuAG7ETk2e | ||
| Y5SaIOpV5Zsta9j3+D6lfSwnJ+f6nQ== | ||
| -----END CERTIFICATE----- |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()andServerContextConfigImpl().There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, OK, let's go with this, it simplifies other logic later on.