From 36f199376fc1c54970f18baec8de54336b4bf21b Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Wed, 5 Dec 2018 08:48:57 -0500 Subject: [PATCH 1/4] [WIP] tls: reject non-P256 TLS certificates. In support of #5200. There are no performance or security advantages today of non-P256 ECDSA certs. Risk Level: Low (unlikely anyone is using these). Testing: Pending; will add when we have agreement this is where we want to go. Signed-off-by: Harvey Tuch --- source/common/ssl/context_impl.cc | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 3d8bd3bd7d03..0c23cce18639 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -229,6 +229,14 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeS bssl::UniquePtr public_key(X509_get_pubkey(ctx.cert_chain_.get())); ctx.is_ecdsa_ = EVP_PKEY_id(public_key.get()) == EVP_PKEY_EC; + if (ctx.is_ecdsa_) { + // We only support P-256 ECDSA today. + EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get()); + const int nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ecdsa_public_key)); + if (nid != NID_X9_62_prime256v1) { + throw EnvoyException(fmt::format("ECDSA key is not P256 (NID = {})", nid)); + } + } // Load private key. bio.reset(BIO_new_mem_buf(const_cast(tls_certificate.privateKey().data()), From d22b4d96bfc19754393c7fce101cc9695caf84cc Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Dec 2018 10:30:38 -0500 Subject: [PATCH 2/4] Fix exception message, some additional guards. Signed-off-by: Harvey Tuch --- source/common/ssl/context_impl.cc | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 0c23cce18639..815b3686b749 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -232,9 +232,13 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeS if (ctx.is_ecdsa_) { // We only support P-256 ECDSA today. EC_KEY* ecdsa_public_key = EVP_PKEY_get0_EC_KEY(public_key.get()); - const int nid = EC_GROUP_get_curve_name(EC_KEY_get0_group(ecdsa_public_key)); - if (nid != NID_X9_62_prime256v1) { - throw EnvoyException(fmt::format("ECDSA key is not P256 (NID = {})", nid)); + // Since we checked the key type above, this should be valid. + ASSERT(ecdsa_public_key != nullptr); + const EC_GROUP* ecdsa_group = EC_KEY_get0_group(ecdsa_public_key); + if (ecdsa_group == nullptr || EC_GROUP_get_curve_name(ecdsa_group) != NID_X9_62_prime256v1) { + throw EnvoyException(fmt::format("Failed to load certificate from chain {}, only P-256 " + "certificates are supported", + ctx.cert_chain_file_path_)); } } From fed0cd0cead3328eab13435a07bc8837198726f1 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Dec 2018 12:16:50 -0500 Subject: [PATCH 3/4] Tests for happy/sad paths. Signed-off-by: Harvey Tuch --- test/common/ssl/context_impl_test.cc | 41 +++++++++++++++++++ test/common/ssl/test_data/certs.sh | 8 ++++ .../test_data/selfsigned_cert_ecdsa_p256.pem | 16 ++++++++ .../test_data/selfsigned_cert_ecdsa_p384.pem | 17 ++++++++ .../test_data/selfsigned_key_ecdsa_p256.pem | 8 ++++ .../test_data/selfsigned_key_ecdsa_p384.pem | 9 ++++ 6 files changed, 99 insertions(+) create mode 100644 test/common/ssl/test_data/selfsigned_cert_ecdsa_p256.pem create mode 100644 test/common/ssl/test_data/selfsigned_cert_ecdsa_p384.pem create mode 100644 test/common/ssl/test_data/selfsigned_key_ecdsa_p256.pem create mode 100644 test/common/ssl/test_data/selfsigned_key_ecdsa_p384.pem diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index d39f83ef0298..70555cd43147 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -519,6 +519,47 @@ TEST(ClientContextConfigImplTest, InvalidCertificateSpki) { EnvoyException, "Invalid base64-encoded SHA-256 .*"); } +// Validate that P256 ECDSA certs load. +TEST(ClientContextConfigImplTest, P256EcdsaCert) { + envoy::api::v2::auth::UpstreamTlsContext tls_context; + NiceMock factory_context; + const std::string tls_certificate_yaml = R"EOF( + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert_ecdsa_p256.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key_ecdsa_p256.pem" + )EOF"; + MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), + *tls_context.mutable_common_tls_context()->add_tls_certificates()); + ClientContextConfigImpl client_context_config(tls_context, factory_context); + Event::SimulatedTimeSystem time_system; + ContextManagerImpl manager(time_system); + Stats::IsolatedStoreImpl store; + manager.createSslClientContext(store, client_context_config); +} + +// Validate that non-P256 ECDSA certs are rejected. +TEST(ClientContextConfigImplTest, NonP256EcdsaCert) { + envoy::api::v2::auth::UpstreamTlsContext tls_context; + NiceMock factory_context; + const std::string tls_certificate_yaml = R"EOF( + certificate_chain: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_cert_ecdsa_p384.pem" + private_key: + filename: "{{ test_rundir }}/test/common/ssl/test_data/selfsigned_key_ecdsa_p384.pem" + )EOF"; + MessageUtil::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml), + *tls_context.mutable_common_tls_context()->add_tls_certificates()); + ClientContextConfigImpl client_context_config(tls_context, factory_context); + Event::SimulatedTimeSystem time_system; + ContextManagerImpl manager(time_system); + Stats::IsolatedStoreImpl store; + EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), + EnvoyException, + "Failed to load certificate from chain .*selfsigned_cert_ecdsa_p384.pem, " + "only P-256 certificates are supported"); +} + // Multiple TLS certificates are not yet supported. // TODO(PiotrSikora): Support multiple TLS certificates. TEST(ClientContextConfigImplTest, MultipleTlsCertificates) { diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index c611c1689b30..a92630de4f4b 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -13,6 +13,8 @@ set -e # openssl genrsa -out san_multiple_dns_key.pem 1024 # openssl genrsa -out san_uri_key.pem 1024 # openssl genrsa -out selfsigned_key.pem 1024 +# openssl ecparam -name secp256r1 -genkey -out selfsigned_key_ecdsa_p256.pem +# openssl ecparam -name secp384r1 -genkey -out selfsigned_key_ecdsa_p384.pem # openssl genrsa -out expired_key.pem 1024 # openssl genrsa -out expired_san_uri_key.pem 1024 @@ -65,6 +67,12 @@ openssl x509 -req -days 730 -in san_uri_cert.csr -sha256 -CA ca_cert.pem -CAkey # Generate selfsigned_cert.pem. openssl req -new -x509 -days 730 -key selfsigned_key.pem -out selfsigned_cert.pem -config selfsigned_cert.cfg -batch -sha256 +# Generate selfsigned_cert_ecdsa_p256.pem. +openssl req -new -x509 -days 730 -key selfsigned_key_ecdsa_p256.pem -out selfsigned_cert_ecdsa_p256.pem -config selfsigned_cert.cfg -batch -sha256 + +# Generate selfsigned_cert_ecdsa_p384.pem. +openssl req -new -x509 -days 730 -key selfsigned_key_ecdsa_p384.pem -out selfsigned_cert_ecdsa_p384.pem -config selfsigned_cert.cfg -batch -sha256 + # Generate expired_cert.pem as a self-signed, expired cert (will fail on Mac OS 10.13+ because of negative days value). openssl req -new -key expired_key.pem -out expired_cert.csr -config selfsigned_cert.cfg -batch -sha256 openssl x509 -req -days -365 -in expired_cert.csr -signkey expired_key.pem -out expired_cert.pem diff --git a/test/common/ssl/test_data/selfsigned_cert_ecdsa_p256.pem b/test/common/ssl/test_data/selfsigned_cert_ecdsa_p256.pem new file mode 100644 index 000000000000..67159993ad84 --- /dev/null +++ b/test/common/ssl/test_data/selfsigned_cert_ecdsa_p256.pem @@ -0,0 +1,16 @@ +-----BEGIN CERTIFICATE----- +MIICijCCAi+gAwIBAgIJALJe5fMxxImWMAoGCCqGSM49BAMCMHoxCzAJBgNVBAYT +AlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2Nv +MQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2luZWVyaW5nMRQwEgYD +VQQDDAtUZXN0IFNlcnZlcjAeFw0xODEyMDYxNzI2MzNaFw0yMDEyMDUxNzI2MzNa +MHoxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1T +YW4gRnJhbmNpc2NvMQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2lu +ZWVyaW5nMRQwEgYDVQQDDAtUZXN0IFNlcnZlcjBZMBMGByqGSM49AgEGCCqGSM49 +AwEHA0IABK64d3LUKGgoZW6+SmRmomON7VKNpnLrzxnab+YDSVaIa6Ra5wKfwpPl +lY13E/pvImznLi7CGeqDnAOlR1FfoLKjgZ0wgZowDAYDVR0TAQH/BAIwADALBgNV +HQ8EBAMCBeAwHQYDVR0lBBYwFAYIKwYBBQUHAwIGCCsGAQUFBwMBMB4GA1UdEQQX +MBWCE3NlcnZlcjEuZXhhbXBsZS5jb20wHQYDVR0OBBYEFL4/swsRcOIQC1vhhteF +lW+xYkx5MB8GA1UdIwQYMBaAFL4/swsRcOIQC1vhhteFlW+xYkx5MAoGCCqGSM49 +BAMCA0kAMEYCIQDZDEX2FYhfrc7lW63rqG+0ZBUaf/rOj6cJ+1sLgCy31AIhAMQW +9QbcVxRgisBD/aE45Zow7epY6ZYLpSj9FKnHe3Uh +-----END CERTIFICATE----- diff --git a/test/common/ssl/test_data/selfsigned_cert_ecdsa_p384.pem b/test/common/ssl/test_data/selfsigned_cert_ecdsa_p384.pem new file mode 100644 index 000000000000..cac150de4951 --- /dev/null +++ b/test/common/ssl/test_data/selfsigned_cert_ecdsa_p384.pem @@ -0,0 +1,17 @@ +-----BEGIN CERTIFICATE----- +MIICxjCCAkygAwIBAgIJAMZErjw+7ImlMAoGCCqGSM49BAMCMHoxCzAJBgNVBAYT +AlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2Nv +MQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2luZWVyaW5nMRQwEgYD +VQQDDAtUZXN0IFNlcnZlcjAeFw0xODEyMDYxNzI2MzNaFw0yMDEyMDUxNzI2MzNa +MHoxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1T +YW4gRnJhbmNpc2NvMQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2lu +ZWVyaW5nMRQwEgYDVQQDDAtUZXN0IFNlcnZlcjB2MBAGByqGSM49AgEGBSuBBAAi +A2IABGzQqH4QvGjbJGZdyjs3mvyxvgTpi9X5OczwyMQvYYx9v3G/d9AdiPM5IYgG +WhpgXCp8w5Y2aSs7zVhPrgvHzBENngD7/GApYYiddORZ49c8VU9pNjI4ldUHkCzw +MZ11caOBnTCBmjAMBgNVHRMBAf8EAjAAMAsGA1UdDwQEAwIF4DAdBgNVHSUEFjAU +BggrBgEFBQcDAgYIKwYBBQUHAwEwHgYDVR0RBBcwFYITc2VydmVyMS5leGFtcGxl +LmNvbTAdBgNVHQ4EFgQU4voPUUR2VO230OxpFg4NwiwdHK0wHwYDVR0jBBgwFoAU +4voPUUR2VO230OxpFg4NwiwdHK0wCgYIKoZIzj0EAwIDaAAwZQIxAJho8eSlS/2X +b3xtYfkoqjJQDr1CPvaAoPj63fo3gMwvmF7I+xNKTTlGCpHpMjjfVQIwUB1BkDhe +dvcOktVfBvnH5TQ4tzieSiuPawAU0QkcmgKIyd2LKJ1kimU2FdFVO40f +-----END CERTIFICATE----- diff --git a/test/common/ssl/test_data/selfsigned_key_ecdsa_p256.pem b/test/common/ssl/test_data/selfsigned_key_ecdsa_p256.pem new file mode 100644 index 000000000000..1be4f08d7f65 --- /dev/null +++ b/test/common/ssl/test_data/selfsigned_key_ecdsa_p256.pem @@ -0,0 +1,8 @@ +-----BEGIN EC PARAMETERS----- +BggqhkjOPQMBBw== +-----END EC PARAMETERS----- +-----BEGIN EC PRIVATE KEY----- +MHcCAQEEIF1Qz3pVyjoD6qgCa/tg9gpF3vNSfgK88UpNJllXIbT/oAoGCCqGSM49 +AwEHoUQDQgAErrh3ctQoaChlbr5KZGaiY43tUo2mcuvPGdpv5gNJVohrpFrnAp/C +k+WVjXcT+m8ibOcuLsIZ6oOcA6VHUV+gsg== +-----END EC PRIVATE KEY----- diff --git a/test/common/ssl/test_data/selfsigned_key_ecdsa_p384.pem b/test/common/ssl/test_data/selfsigned_key_ecdsa_p384.pem new file mode 100644 index 000000000000..775534115f6a --- /dev/null +++ b/test/common/ssl/test_data/selfsigned_key_ecdsa_p384.pem @@ -0,0 +1,9 @@ +-----BEGIN EC PARAMETERS----- +BgUrgQQAIg== +-----END EC PARAMETERS----- +-----BEGIN EC PRIVATE KEY----- +MIGkAgEBBDA4MtE/FxALE2H3TnzCVd61Uz+sNTqkCmn53nsIk13JUlyEgLeSfTIN +N7tT7XmgnTagBwYFK4EEACKhZANiAARs0Kh+ELxo2yRmXco7N5r8sb4E6YvV+TnM +8MjEL2GMfb9xv3fQHYjzOSGIBloaYFwqfMOWNmkrO81YT64Lx8wRDZ4A+/xgKWGI +nXTkWePXPFVPaTYyOJXVB5As8DGddXE= +-----END EC PRIVATE KEY----- From 0b14302f09417197a58727ab43660e59d1b68f26 Mon Sep 17 00:00:00 2001 From: Harvey Tuch Date: Thu, 6 Dec 2018 23:41:12 -0500 Subject: [PATCH 4/4] Nit. Signed-off-by: Harvey Tuch --- source/common/ssl/context_impl.cc | 2 +- test/common/ssl/context_impl_test.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 815b3686b749..e7500ee9075c 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -237,7 +237,7 @@ ContextImpl::ContextImpl(Stats::Scope& scope, const ContextConfig& config, TimeS const EC_GROUP* ecdsa_group = EC_KEY_get0_group(ecdsa_public_key); if (ecdsa_group == nullptr || EC_GROUP_get_curve_name(ecdsa_group) != NID_X9_62_prime256v1) { throw EnvoyException(fmt::format("Failed to load certificate from chain {}, only P-256 " - "certificates are supported", + "ECDSA certificates are supported", ctx.cert_chain_file_path_)); } } diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 70555cd43147..3e044ee99935 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -557,7 +557,7 @@ TEST(ClientContextConfigImplTest, NonP256EcdsaCert) { EXPECT_THROW_WITH_REGEX(manager.createSslClientContext(store, client_context_config), EnvoyException, "Failed to load certificate from chain .*selfsigned_cert_ecdsa_p384.pem, " - "only P-256 certificates are supported"); + "only P-256 ECDSA certificates are supported"); } // Multiple TLS certificates are not yet supported.