From d4f4c643b517b710202bddab6f631fb6bc5f9dbe Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:05:06 -0500 Subject: [PATCH 01/25] Add support for 'crl' field in CertificateValidationContext and v1 configuration Signed-off-by: Andrew Dunham --- include/envoy/ssl/BUILD | 1 + include/envoy/ssl/context_config.h | 7 +++++++ source/common/config/tls_context_json.cc | 6 ++++++ source/common/json/config_schemas.cc | 3 ++- source/common/ssl/context_config_impl.h | 2 ++ 5 files changed, 18 insertions(+), 1 deletion(-) diff --git a/include/envoy/ssl/BUILD b/include/envoy/ssl/BUILD index f6b94ea22bd9a..d99c7ce7f1b62 100644 --- a/include/envoy/ssl/BUILD +++ b/include/envoy/ssl/BUILD @@ -20,6 +20,7 @@ envoy_cc_library( envoy_cc_library( name = "context_config_interface", + external_deps = ["ssl"], hdrs = ["context_config.h"], ) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 83df4dc70360e..6bc56f3b04572 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -6,6 +6,8 @@ #include "envoy/common/pure.h" +#include "openssl/ssl.h" + namespace Envoy { namespace Ssl { @@ -71,6 +73,11 @@ class ContextConfig { */ virtual const std::string& privateKeyPath() const PURE; + /** + * @return The CRL to check if a cert is revoked. + */ + virtual const std::vector>& certificateRevocationLists() const PURE; + /** * @return The subject alt names to be verified, if enabled. Otherwise, "" */ diff --git a/source/common/config/tls_context_json.cc b/source/common/config/tls_context_json.cc index 0ec4e2e1c6b99..309d878a2b0e8 100644 --- a/source/common/config/tls_context_json.cc +++ b/source/common/config/tls_context_json.cc @@ -61,6 +61,12 @@ void TlsContextJson::translateCommonTlsContext( common_tls_context.mutable_tls_params()->add_cipher_suites(std::string{cipher_suite}); } + // TODO: this doesn't currently support inline data + if (json_tls_context.hasObject("crl_file")) { + validation_context->mutable_crl()->set_filename( + json_tls_context.getString("crl_file", "")); + } + const std::string ecdh_curves_str{json_tls_context.getString("ecdh_curves", "")}; for (auto ecdh_curve : StringUtil::splitToken(ecdh_curves_str, ":")) { common_tls_context.mutable_tls_params()->add_ecdh_curves(std::string{ecdh_curve}); diff --git a/source/common/json/config_schemas.cc b/source/common/json/config_schemas.cc index f887d8e121d6d..d7c9f1c44ff83 100644 --- a/source/common/json/config_schemas.cc +++ b/source/common/json/config_schemas.cc @@ -156,7 +156,8 @@ const std::string Json::Schema::LISTENER_SCHEMA(R"EOF( } }, "cipher_suites" : {"type" : "string", "minLength" : 1}, - "ecdh_curves" : {"type" : "string", "minLength" : 1} + "ecdh_curves" : {"type" : "string", "minLength" : 1}, + "crl_file" : {"type" : "string"} }, "required": ["cert_chain_file", "private_key_file"], "additionalProperties": false diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 83212fa722e33..2c6783bf1b189 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -33,6 +33,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& privateKeyPath() const override { return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; } + const std::vector>& certificateRevocationLists() const override { return certificate_revocation_lists_; } const std::vector& verifySubjectAltNameList() const override { return verify_subject_alt_name_list_; }; @@ -64,6 +65,7 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string cert_chain_path_; const std::string private_key_; const std::string private_key_path_; + const std::vector> certificate_revocation_lists_; const std::vector verify_subject_alt_name_list_; const std::string verify_certificate_hash_; const unsigned min_protocol_version_; From 26a9abccb06eb140493072267788689f4fc59c01 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:05:31 -0500 Subject: [PATCH 02/25] Load CRLs from context Signed-off-by: Andrew Dunham --- source/common/ssl/context_config_impl.cc | 59 ++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 9414002913228..546330292f595 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -34,6 +34,64 @@ const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256"; +static std::vector> loadCRL(const envoy::api::v2::CertificateValidationContext& vctx) { + std::vector> crls; + + if (!vctx.has_crl()) { + return crls; + } + + // We want to load multiple CRLs from the same file, so we need to use + // OpenSSL's STACK abstraction. We read from the input file or inline data, + // and load into the stack of X509_INFOs. + bssl::UniquePtr xis(nullptr); + + auto crl_conf = vctx.crl(); + if (crl_conf.filename().length()) { + std::unique_ptr fp( + fopen(crl_conf.filename().c_str(), "r"), + &fclose + ); + + if (!fp.get()) { + throw EnvoyException(fmt::format("Failed to open CRL file '{}'", crl_conf.filename().c_str())); + } + + xis.reset(PEM_X509_INFO_read(fp.get(), nullptr, nullptr, nullptr)); + } else { + std::unique_ptr bio( + BIO_new_mem_buf(crl_conf.inline_().c_str(), crl_conf.inline_().length()), + BIO_free_all + ); + + if (!bio.get()) { + throw EnvoyException(fmt::format("Failed to load inline CRL")); + } + + xis.reset(PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); + } + + if (!xis) { + throw EnvoyException(fmt::format("Failed to parse CRL")); + } + + // Iterate through every item in the stack, adding all found CRLs to our + // return array. + for (unsigned int i = 0; i < sk_X509_INFO_num(xis.get()); i++) { + auto xi = sk_X509_INFO_value(xis.get(), i); + if (xi->crl != nullptr) { + crls.emplace_back(xi->crl); + + // We need to set the pointer to nullptr here, since otherwise the + // pointer's destructor (`sk_X509_INFO_pop_free`) will free the + // underlying CRL, that we've just copied to the vector. + xi->crl = nullptr; + } + } + + return crls; +} + ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& config) : alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), alt_alpn_protocols_(config.deprecated_v1().alt_alpn_protocols()), @@ -55,6 +113,7 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con private_key_path_(config.tls_certificates().empty() ? "" : getDataSourcePath(config.tls_certificates()[0].private_key())), + certificate_revocation_lists_(loadCRL(config.validation_context())), verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), config.validation_context().verify_subject_alt_name().end()), verify_certificate_hash_(config.validation_context().verify_certificate_hash().empty() From 40e99853365f2a61b737f43f9064662e6757246e Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:05:53 -0500 Subject: [PATCH 03/25] Add CRLs to X509_STORE if any are present Signed-off-by: Andrew Dunham --- source/common/ssl/context_impl.cc | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index ea020a25eb85b..8594d91d16490 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -152,6 +152,16 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, } } + if (config.certificateRevocationLists().size() > 0) { + X509_STORE *cs = SSL_CTX_get_cert_store(ctx_.get()); + + for (const bssl::UniquePtr& crl : config.certificateRevocationLists()) { + X509_STORE_add_crl(cs, crl.get()); + } + + X509_STORE_set_flags(cs, X509_V_FLAG_CRL_CHECK); + } + // use the server's cipher list preferences SSL_CTX_set_options(ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); From 3599a9d289873d310dfd7f144a3913e1368456e5 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:07:00 -0500 Subject: [PATCH 04/25] Add test data for CRLs Signed-off-by: Andrew Dunham --- test/common/ssl/test_data/certs.sh | 15 ++++++++ test/common/ssl/test_data/crl.cfg | 18 ++++++++++ test/common/ssl/test_data/crl/crl_number | 1 + test/common/ssl/test_data/crl/crl_number.old | 1 + test/common/ssl/test_data/crl/index.txt | 1 + test/common/ssl/test_data/crl/index.txt.attr | 1 + .../ssl/test_data/crl/index.txt.attr.old | 0 test/common/ssl/test_data/crl/index.txt.old | 0 test/common/ssl/test_data/revoked_cert.cfg | 36 +++++++++++++++++++ test/common/ssl/test_data/revoked_cert.crl | 10 ++++++ test/common/ssl/test_data/revoked_cert.pem | 19 ++++++++++ test/common/ssl/test_data/revoked_key.pem | 15 ++++++++ 12 files changed, 117 insertions(+) create mode 100644 test/common/ssl/test_data/crl.cfg create mode 100644 test/common/ssl/test_data/crl/crl_number create mode 100644 test/common/ssl/test_data/crl/crl_number.old create mode 100644 test/common/ssl/test_data/crl/index.txt create mode 100644 test/common/ssl/test_data/crl/index.txt.attr create mode 100644 test/common/ssl/test_data/crl/index.txt.attr.old create mode 100644 test/common/ssl/test_data/crl/index.txt.old create mode 100644 test/common/ssl/test_data/revoked_cert.cfg create mode 100644 test/common/ssl/test_data/revoked_cert.crl create mode 100644 test/common/ssl/test_data/revoked_cert.pem create mode 100644 test/common/ssl/test_data/revoked_key.pem diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index a442f31a5500a..8126a3d822f76 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -14,6 +14,7 @@ set -e # openssl genrsa -out san_uri_key.pem 1024 # openssl genrsa -out selfsigned_key.pem 1024 # openssl genrsa -out expired_key.pem 1024 +# openssl genrsa -out revoked_key.pem 1024 # Generate ca_cert.pem. openssl req -new -key ca_key.pem -out ca_cert.csr -config ca_cert.cfg -batch -sha256 @@ -68,6 +69,19 @@ openssl req -new -x509 -days 730 -key selfsigned_key.pem -out selfsigned_cert.pe 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 +# Generate revoked_cert.pem +openssl req -new -key revoked_key.pem -out revoked_cert.csr -config revoked_cert.cfg -batch -sha256 +openssl x509 -req -days 730 -in revoked_cert.csr -sha256 -CA ca_cert.pem -CAkey ca_key.pem -CAcreateserial -out revoked_cert.pem -extensions v3_ca -extfile revoked_cert.cfg + +# Initialize information for CRL process +mkdir crl +touch crl/index.txt crl/index.txt.attr +echo 00 > crl/crl_number + +# Revoke the certificate and generate a CRL +openssl ca -revoke revoked_cert.pem -keyfile ca_key.pem -cert ca_cert.pem -config crl.cfg +openssl ca -gencrl -keyfile ca_key.pem -cert ca_cert.pem -out revoked_cert.crl -config crl.cfg + # Write session ticket key files openssl rand 80 > ticket_key_a openssl rand 80 > ticket_key_b @@ -75,3 +89,4 @@ openssl rand 79 > ticket_key_wrong_len rm *csr rm *srl +rm -r crl diff --git a/test/common/ssl/test_data/crl.cfg b/test/common/ssl/test_data/crl.cfg new file mode 100644 index 0000000000000..ee3ac47cc5ae5 --- /dev/null +++ b/test/common/ssl/test_data/crl.cfg @@ -0,0 +1,18 @@ +[ default ] +dir = ./crl + +[ ca ] +default_ca = CA_default + +[ CA_default ] +database = $dir/index.txt +crlnumber = $dir/crl_number + +default_days = 3650 +default_crl_days = 3650 +default_md = sha256 +preserve = no +unique_subject = no + +[ crl_ext ] +authorityKeyIdentifier = keyid:always,issuer:always diff --git a/test/common/ssl/test_data/crl/crl_number b/test/common/ssl/test_data/crl/crl_number new file mode 100644 index 0000000000000..8a0f05e166aa6 --- /dev/null +++ b/test/common/ssl/test_data/crl/crl_number @@ -0,0 +1 @@ +01 diff --git a/test/common/ssl/test_data/crl/crl_number.old b/test/common/ssl/test_data/crl/crl_number.old new file mode 100644 index 0000000000000..4daddb72ffc04 --- /dev/null +++ b/test/common/ssl/test_data/crl/crl_number.old @@ -0,0 +1 @@ +00 diff --git a/test/common/ssl/test_data/crl/index.txt b/test/common/ssl/test_data/crl/index.txt new file mode 100644 index 0000000000000..2aad5a8e499f7 --- /dev/null +++ b/test/common/ssl/test_data/crl/index.txt @@ -0,0 +1 @@ +R 191220171237Z 171220171254Z D9CBF429EE201F1F unknown /C=US/ST=California/L=San Francisco/O=Lyft/OU=Lyft Engineering/CN=Revoked Certificate diff --git a/test/common/ssl/test_data/crl/index.txt.attr b/test/common/ssl/test_data/crl/index.txt.attr new file mode 100644 index 0000000000000..3a7e39e6ee60a --- /dev/null +++ b/test/common/ssl/test_data/crl/index.txt.attr @@ -0,0 +1 @@ +unique_subject = no diff --git a/test/common/ssl/test_data/crl/index.txt.attr.old b/test/common/ssl/test_data/crl/index.txt.attr.old new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/test/common/ssl/test_data/crl/index.txt.old b/test/common/ssl/test_data/crl/index.txt.old new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/test/common/ssl/test_data/revoked_cert.cfg b/test/common/ssl/test_data/revoked_cert.cfg new file mode 100644 index 0000000000000..0a8344e35bffb --- /dev/null +++ b/test/common/ssl/test_data/revoked_cert.cfg @@ -0,0 +1,36 @@ +[req] +distinguished_name = req_distinguished_name +req_extensions = v3_req + +[req_distinguished_name] +countryName = US +countryName_default = US +stateOrProvinceName = California +stateOrProvinceName_default = California +localityName = San Francisco +localityName_default = San Francisco +organizationName = Lyft +organizationName_default = Lyft +organizationalUnitName = Lyft Engineering +organizationalUnitName_default = Lyft Engineering +commonName = Revoked Certificate +commonName_default = Revoked Certificate +commonName_max = 64 + +[v3_req] +basicConstraints = CA:FALSE +keyUsage = nonRepudiation, digitalSignature, keyEncipherment +extendedKeyUsage = clientAuth, serverAuth +subjectAltName = @alt_names +subjectKeyIdentifier = hash + +[v3_ca] +basicConstraints = critical, CA:FALSE +keyUsage = nonRepudiation, digitalSignature, keyEncipherment +extendedKeyUsage = clientAuth, serverAuth +subjectAltName = @alt_names +subjectKeyIdentifier = hash +authorityKeyIdentifier = keyid:always + +[alt_names] +DNS.1 = server1.example.com diff --git a/test/common/ssl/test_data/revoked_cert.crl b/test/common/ssl/test_data/revoked_cert.crl new file mode 100644 index 0000000000000..722a277f4d602 --- /dev/null +++ b/test/common/ssl/test_data/revoked_cert.crl @@ -0,0 +1,10 @@ +-----BEGIN X509 CRL----- +MIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE +CBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME +THlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD +QRcNMTcxMjIwMTcxNDA4WhcNMjcxMjE4MTcxNDA4WjAcMBoCCQDZy/Qp7iAfHxcN +MTcxMjIwMTcxMjU0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA +OTn5Fgb44xtFd9QGtbTElZ3iwdlcOxRHjgQMd+ydzEEZRMzMgb4/NmEsgXAsxbrx +tKmpgll8TblscitkglvGk8s4obi/OtgxNIvn+7pOBTjmrgJkcktBUDEWRbLZjsZx +yH+5teBZ0tH0tVy914QeGitZFV8awK1hlJwlAz9g/jo= +-----END X509 CRL----- diff --git a/test/common/ssl/test_data/revoked_cert.pem b/test/common/ssl/test_data/revoked_cert.pem new file mode 100644 index 0000000000000..4a3dedb4b4aaa --- /dev/null +++ b/test/common/ssl/test_data/revoked_cert.pem @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDFTCCAn6gAwIBAgIJANnL9CnuIB8fMA0GCSqGSIb3DQEBCwUAMHYxCzAJBgNV +BAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJhbmNp +c2NvMQ0wCwYDVQQKEwRMeWZ0MRkwFwYDVQQLExBMeWZ0IEVuZ2luZWVyaW5nMRAw +DgYDVQQDEwdUZXN0IENBMB4XDTE3MTIyMDE3MTIzN1oXDTE5MTIyMDE3MTIzN1ow +gYIxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1T +YW4gRnJhbmNpc2NvMQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2lu +ZWVyaW5nMRwwGgYDVQQDDBNSZXZva2VkIENlcnRpZmljYXRlMIGfMA0GCSqGSIb3 +DQEBAQUAA4GNADCBiQKBgQDTz3LeIa8b7vz0y0ha9yty0oAzySpVeGBOG82bNa9U +M2dDtfg5/oaTBafKhFNLenMy9KSQHBErXQ5jbWHGQEqceVHOE1qsVptstXUkQqfZ +OkBJvTJZuBxmHD+ly83+vhimHfyp4FZBQ4rQ6INlqRpx4x8aE+oxMnM16petjD6w +5wIDAQABo4GdMIGaMAwGA1UdEwEB/wQCMAAwCwYDVR0PBAQDAgXgMB0GA1UdJQQW +MBQGCCsGAQUFBwMCBggrBgEFBQcDATAeBgNVHREEFzAVghNzZXJ2ZXIxLmV4YW1w +bGUuY29tMB0GA1UdDgQWBBQxiVjNEJfrpCbaKcjpPqsKxRIrOTAfBgNVHSMEGDAW +gBQ7eKRRTxaEkxxIKHoMrSuWQcp9eTANBgkqhkiG9w0BAQsFAAOBgQCR6MiZA1Tk +R39ggdS4c4yYnllDv8IkpBV4OP8+P8uwXJhu4zwZ1j6d6dBcvpYBc4E9XmiEdF67 ++j50/0QOj5aMj/OFFSqUO1CogQmDasepVboLBSyR34QXamgug94hjHeO7lW9oAcr +r+Aq/0AjkspEmTQfioy6IxM4wTnwhFRzGA== +-----END CERTIFICATE----- diff --git a/test/common/ssl/test_data/revoked_key.pem b/test/common/ssl/test_data/revoked_key.pem new file mode 100644 index 0000000000000..7f9b0a3b5c7c7 --- /dev/null +++ b/test/common/ssl/test_data/revoked_key.pem @@ -0,0 +1,15 @@ +-----BEGIN RSA PRIVATE KEY----- +MIICXgIBAAKBgQDTz3LeIa8b7vz0y0ha9yty0oAzySpVeGBOG82bNa9UM2dDtfg5 +/oaTBafKhFNLenMy9KSQHBErXQ5jbWHGQEqceVHOE1qsVptstXUkQqfZOkBJvTJZ +uBxmHD+ly83+vhimHfyp4FZBQ4rQ6INlqRpx4x8aE+oxMnM16petjD6w5wIDAQAB +AoGBAKRczp5hNSlQAytStAsi0qx/fMyyxg8dIl56ZMqUlkGYwgFhLAaU5IkiUlps +5NYlZ0+bWDgcD5a+13OAZecZ7MqmVE3Ud3gV/tkHZdH90zCPx58IiwsIinV5BqP0 +XrSJWgSieCx+Zg8BDE+ZZpfCuOsAVE7VweOQIdiD+U6nwecBAkEA6QldHBPlNKFZ +EHVqL8pkYYub5b2e0l+ZEf6NshB/0m1ryeCJn3LHp/7jUd/9wlDqZtLMlWo1pdPx +K/+9MHtLEQJBAOiuoIWGlXOcVO6UPNqpWrx5pfzv+vMpBJiMvMrBmUwqFxwHbqCP +AB2k5H+xdFany0EvMPjg6D/hJd4yXQDKDHcCQQDagQcm7pi5spgaUJ3SVcmtlQQG +dLfYtf6G2tHtpn7TxfmNftZMBYmjweFPweDkNI60/u8JIl9PL90wzkiMju6hAkBR +LDKFwni610vt2zsLkU89NzcH8XRbhfC7g0WNelKPdpOPTKx0SM7iiJbKUU7juC+5 +Msxj1ppPRq1eQbWeQ95rAkEAgkcunIs1zLMO/trrB3+ILuUcZP5HDlPFQzDQ2ofX +NKA3FbkD1gR47Xz9iRxkclQyZ9mFTg6BQzw3+v8iFRzO1A== +-----END RSA PRIVATE KEY----- From c1b339b5a52f2a8c1a0bbc4cf5c84eb1572b3b09 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:07:18 -0500 Subject: [PATCH 05/25] Add test that verifies our configuration parses Signed-off-by: Andrew Dunham --- test/common/ssl/context_impl_test.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index b842a5a09ccc9..ad31eb6913785 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -284,5 +284,17 @@ TEST_F(SslServerContextImplTicketTest, TicketKeyInlineStringFailTooSmall) { EXPECT_THROW(loadConfigV2(cfg), EnvoyException); } +TEST_F(SslServerContextImplTicketTest, CRLSuccess) { + std::string json = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" + } + )EOF"; + + EXPECT_NO_THROW(loadConfigJson(json)); +} + } // namespace Ssl } // namespace Envoy From 99abed1f2c1673ac755dd4c32a2f1d5f29397558 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:07:46 -0500 Subject: [PATCH 06/25] Add test that verifies that a revoked certificate + CRL works Signed-off-by: Andrew Dunham --- test/common/ssl/ssl_socket_test.cc | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 5a87edd4230b3..904a4f08f1d03 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -1842,6 +1842,35 @@ TEST_P(SslSocketTest, SniProtocolVersions) { "ssl.handshake", 2, GetParam()); } +TEST_P(SslSocketTest, RevokedCertificate) { + std::string server_ctx_json = R"EOF( + { + "cert_chain_file": "{{ test_tmpdir }}/unittestcert.pem", + "private_key_file": "{{ test_tmpdir }}/unittestkey.pem", + "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem", + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" + } + )EOF"; + + // This should fail, since the certificate has been revoked. + std::string revoked_client_ctx_json = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" + } + )EOF"; + testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "ssl.fail_verify_error", false, GetParam()); + + // This should succeed, since the cert isn't revoked. + std::string successful_client_ctx_json = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" + } + )EOF"; + testUtil(successful_client_ctx_json, server_ctx_json, "", "", "ssl.handshake", true, GetParam()); +} + class SslReadBufferLimitTest : public SslCertsTest, public testing::WithParamInterface { public: From 6d15f1f436c005dc0f12c85c870eddc9917edbc9 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 19:27:07 -0500 Subject: [PATCH 07/25] Formatting fixes Signed-off-by: Andrew Dunham --- include/envoy/ssl/BUILD | 2 +- source/common/config/tls_context_json.cc | 3 +-- source/common/ssl/context_config_impl.cc | 17 +++++++---------- source/common/ssl/context_config_impl.h | 4 +++- source/common/ssl/context_impl.cc | 2 +- test/common/ssl/ssl_socket_test.cc | 3 ++- 6 files changed, 15 insertions(+), 16 deletions(-) diff --git a/include/envoy/ssl/BUILD b/include/envoy/ssl/BUILD index d99c7ce7f1b62..98722dc5caad2 100644 --- a/include/envoy/ssl/BUILD +++ b/include/envoy/ssl/BUILD @@ -20,8 +20,8 @@ envoy_cc_library( envoy_cc_library( name = "context_config_interface", - external_deps = ["ssl"], hdrs = ["context_config.h"], + external_deps = ["ssl"], ) envoy_cc_library( diff --git a/source/common/config/tls_context_json.cc b/source/common/config/tls_context_json.cc index 309d878a2b0e8..4c7de4fcaff88 100644 --- a/source/common/config/tls_context_json.cc +++ b/source/common/config/tls_context_json.cc @@ -63,8 +63,7 @@ void TlsContextJson::translateCommonTlsContext( // TODO: this doesn't currently support inline data if (json_tls_context.hasObject("crl_file")) { - validation_context->mutable_crl()->set_filename( - json_tls_context.getString("crl_file", "")); + validation_context->mutable_crl()->set_filename(json_tls_context.getString("crl_file", "")); } const std::string ecdh_curves_str{json_tls_context.getString("ecdh_curves", "")}; diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 546330292f595..08ee8a34d4421 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -34,7 +34,8 @@ const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256"; -static std::vector> loadCRL(const envoy::api::v2::CertificateValidationContext& vctx) { +static std::vector> +loadCRL(const envoy::api::v2::CertificateValidationContext& vctx) { std::vector> crls; if (!vctx.has_crl()) { @@ -42,27 +43,23 @@ static std::vector> loadCRL(const envoy::api::v2::Cert } // We want to load multiple CRLs from the same file, so we need to use - // OpenSSL's STACK abstraction. We read from the input file or inline data, + // OpenSSL's STACK abstraction. We read from the input file or inline data, // and load into the stack of X509_INFOs. bssl::UniquePtr xis(nullptr); auto crl_conf = vctx.crl(); if (crl_conf.filename().length()) { - std::unique_ptr fp( - fopen(crl_conf.filename().c_str(), "r"), - &fclose - ); + std::unique_ptr fp(fopen(crl_conf.filename().c_str(), "r"), &fclose); if (!fp.get()) { - throw EnvoyException(fmt::format("Failed to open CRL file '{}'", crl_conf.filename().c_str())); + throw EnvoyException( + fmt::format("Failed to open CRL file '{}'", crl_conf.filename().c_str())); } xis.reset(PEM_X509_INFO_read(fp.get(), nullptr, nullptr, nullptr)); } else { std::unique_ptr bio( - BIO_new_mem_buf(crl_conf.inline_().c_str(), crl_conf.inline_().length()), - BIO_free_all - ); + BIO_new_mem_buf(crl_conf.inline_().c_str(), crl_conf.inline_().length()), BIO_free_all); if (!bio.get()) { throw EnvoyException(fmt::format("Failed to load inline CRL")); diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 2c6783bf1b189..63d97ca9bc6e9 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -33,7 +33,9 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& privateKeyPath() const override { return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; } - const std::vector>& certificateRevocationLists() const override { return certificate_revocation_lists_; } + const std::vector>& certificateRevocationLists() const override { + return certificate_revocation_lists_; + } const std::vector& verifySubjectAltNameList() const override { return verify_subject_alt_name_list_; }; diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 8594d91d16490..b198f13ce400a 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -153,7 +153,7 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, } if (config.certificateRevocationLists().size() > 0) { - X509_STORE *cs = SSL_CTX_get_cert_store(ctx_.get()); + X509_STORE* cs = SSL_CTX_get_cert_store(ctx_.get()); for (const bssl::UniquePtr& crl : config.certificateRevocationLists()) { X509_STORE_add_crl(cs, crl.get()); diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 904a4f08f1d03..dd830807f77e4 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -1859,7 +1859,8 @@ TEST_P(SslSocketTest, RevokedCertificate) { "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } )EOF"; - testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "ssl.fail_verify_error", false, GetParam()); + testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "ssl.fail_verify_error", false, + GetParam()); // This should succeed, since the cert isn't revoked. std::string successful_client_ctx_json = R"EOF( From 691483942174bd0c836087ec3855320dc1acf4ad Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 21 Dec 2017 21:36:52 -0500 Subject: [PATCH 08/25] Review feedback: simplify iterating over X509_INFO stack, use real reference method Signed-off-by: Andrew Dunham --- source/common/ssl/context_config_impl.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 08ee8a34d4421..058156c49836a 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -74,15 +74,13 @@ loadCRL(const envoy::api::v2::CertificateValidationContext& vctx) { // Iterate through every item in the stack, adding all found CRLs to our // return array. - for (unsigned int i = 0; i < sk_X509_INFO_num(xis.get()); i++) { - auto xi = sk_X509_INFO_value(xis.get(), i); + for (const auto& xi : xis.get()) { if (xi->crl != nullptr) { crls.emplace_back(xi->crl); - // We need to set the pointer to nullptr here, since otherwise the - // pointer's destructor (`sk_X509_INFO_pop_free`) will free the - // underlying CRL, that we've just copied to the vector. - xi->crl = nullptr; + // We need to hold an additional reference to the CRL, or our stack's + // destructor will free it. + X509_CRL_up_ref(xi->crl); } } From f05137dffc89f2efa8a95fdef51b6e54998d7037 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 2 Jan 2018 13:50:23 -0800 Subject: [PATCH 09/25] Add a test for loading an invalid CRL Signed-off-by: Andrew Dunham --- test/common/ssl/context_impl_test.cc | 12 ++++++++++++ test/common/ssl/test_data/not_a_crl.crl | 3 +++ 2 files changed, 15 insertions(+) create mode 100644 test/common/ssl/test_data/not_a_crl.crl diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index ad31eb6913785..fcf7c256d449f 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -296,5 +296,17 @@ TEST_F(SslServerContextImplTicketTest, CRLSuccess) { EXPECT_NO_THROW(loadConfigJson(json)); } +TEST_F(SslServerContextImplTicketTest, CRLInvalid) { + std::string json = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/not_a_crl.crl" + } + )EOF"; + + EXPECT_THROW(loadConfigJson(json), EnvoyException); +} + } // namespace Ssl } // namespace Envoy diff --git a/test/common/ssl/test_data/not_a_crl.crl b/test/common/ssl/test_data/not_a_crl.crl new file mode 100644 index 0000000000000..e2c83b8d3c1cd --- /dev/null +++ b/test/common/ssl/test_data/not_a_crl.crl @@ -0,0 +1,3 @@ +-----BEGIN X509 CRL----- +TOTALLY_NOT_A_CRL_HERE +-----END X509 CRL----- From 518de3c4a160eb4af1eba8e63e475d9aeb1a0421 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Mon, 22 Jan 2018 20:07:30 -0800 Subject: [PATCH 10/25] Review feedback: store raw CRL bytes in ContextConfig Signed-off-by: Andrew Dunham --- include/envoy/ssl/BUILD | 1 - include/envoy/ssl/context_config.h | 10 +++-- source/common/ssl/context_config_impl.cc | 56 +----------------------- source/common/ssl/context_config_impl.h | 12 +++-- source/common/ssl/context_impl.cc | 24 ++++++++-- 5 files changed, 39 insertions(+), 64 deletions(-) diff --git a/include/envoy/ssl/BUILD b/include/envoy/ssl/BUILD index 98722dc5caad2..f6b94ea22bd9a 100644 --- a/include/envoy/ssl/BUILD +++ b/include/envoy/ssl/BUILD @@ -21,7 +21,6 @@ envoy_cc_library( envoy_cc_library( name = "context_config_interface", hdrs = ["context_config.h"], - external_deps = ["ssl"], ) envoy_cc_library( diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index 6bc56f3b04572..a4f21419e020f 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -6,8 +6,6 @@ #include "envoy/common/pure.h" -#include "openssl/ssl.h" - namespace Envoy { namespace Ssl { @@ -76,7 +74,13 @@ class ContextConfig { /** * @return The CRL to check if a cert is revoked. */ - virtual const std::vector>& certificateRevocationLists() const PURE; + virtual const std::string& certificateRevocationList() const PURE; + + /** + * @return Path of the certificate revocation list, or "" if the CRL + * was inlined. + */ + virtual const std::string& certificateRevocationListPath() const PURE; /** * @return The subject alt names to be verified, if enabled. Otherwise, "" diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 058156c49836a..80c777a5e8c6c 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -34,59 +34,6 @@ const std::string ContextConfigImpl::DEFAULT_CIPHER_SUITES = const std::string ContextConfigImpl::DEFAULT_ECDH_CURVES = "X25519:P-256"; -static std::vector> -loadCRL(const envoy::api::v2::CertificateValidationContext& vctx) { - std::vector> crls; - - if (!vctx.has_crl()) { - return crls; - } - - // We want to load multiple CRLs from the same file, so we need to use - // OpenSSL's STACK abstraction. We read from the input file or inline data, - // and load into the stack of X509_INFOs. - bssl::UniquePtr xis(nullptr); - - auto crl_conf = vctx.crl(); - if (crl_conf.filename().length()) { - std::unique_ptr fp(fopen(crl_conf.filename().c_str(), "r"), &fclose); - - if (!fp.get()) { - throw EnvoyException( - fmt::format("Failed to open CRL file '{}'", crl_conf.filename().c_str())); - } - - xis.reset(PEM_X509_INFO_read(fp.get(), nullptr, nullptr, nullptr)); - } else { - std::unique_ptr bio( - BIO_new_mem_buf(crl_conf.inline_().c_str(), crl_conf.inline_().length()), BIO_free_all); - - if (!bio.get()) { - throw EnvoyException(fmt::format("Failed to load inline CRL")); - } - - xis.reset(PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); - } - - if (!xis) { - throw EnvoyException(fmt::format("Failed to parse CRL")); - } - - // Iterate through every item in the stack, adding all found CRLs to our - // return array. - for (const auto& xi : xis.get()) { - if (xi->crl != nullptr) { - crls.emplace_back(xi->crl); - - // We need to hold an additional reference to the CRL, or our stack's - // destructor will free it. - X509_CRL_up_ref(xi->crl); - } - } - - return crls; -} - ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& config) : alpn_protocols_(RepeatedPtrUtil::join(config.alpn_protocols(), ",")), alt_alpn_protocols_(config.deprecated_v1().alt_alpn_protocols()), @@ -108,7 +55,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con private_key_path_(config.tls_certificates().empty() ? "" : getDataSourcePath(config.tls_certificates()[0].private_key())), - certificate_revocation_lists_(loadCRL(config.validation_context())), + certificate_revocation_list_(readDataSource(config.validation_context().crl(), true)), + certificate_revocation_list_path_(getDataSourcePath(config.validation_context().crl())), verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), config.validation_context().verify_subject_alt_name().end()), verify_certificate_hash_(config.validation_context().verify_certificate_hash().empty() diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 63d97ca9bc6e9..7c0f28190a806 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -33,8 +33,13 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& privateKeyPath() const override { return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; } - const std::vector>& certificateRevocationLists() const override { - return certificate_revocation_lists_; + const std::string& certificateRevocationList() const override { + return certificate_revocation_list_; + } + const std::string& certificateRevocationListPath() const override { + return (certificate_revocation_list_path_.empty() && !certificate_revocation_list_.empty()) + ? INLINE_STRING + : certificate_revocation_list_path_; } const std::vector& verifySubjectAltNameList() const override { return verify_subject_alt_name_list_; @@ -67,7 +72,8 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string cert_chain_path_; const std::string private_key_; const std::string private_key_path_; - const std::vector> certificate_revocation_lists_; + const std::string certificate_revocation_list_; + const std::string certificate_revocation_list_path_; const std::vector verify_subject_alt_name_list_; const std::string verify_certificate_hash_; const unsigned min_protocol_version_; diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index b198f13ce400a..f7b3d7e9e4150 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -152,11 +152,29 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, } } - if (config.certificateRevocationLists().size() > 0) { + if (!config.certificateRevocationList().empty()) { + bssl::UniquePtr bio( + BIO_new_mem_buf(const_cast(config.certificateRevocationList().data()), + config.certificateRevocationList().size())); + RELEASE_ASSERT(bio != nullptr); + + // Based on BoringSSL's X509_load_cert_crl_file(). + bssl::UniquePtr list( + PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); + if (list == nullptr) { + throw EnvoyException( + fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); + } + X509_STORE* cs = SSL_CTX_get_cert_store(ctx_.get()); + for (const X509_INFO* item : list.get()) { + if (item->crl) { + X509_STORE_add_crl(cs, item->crl); - for (const bssl::UniquePtr& crl : config.certificateRevocationLists()) { - X509_STORE_add_crl(cs, crl.get()); + // We need to hold an additional reference to the CRL, or our stack's + // destructor will free it. + X509_CRL_up_ref(item->crl); + } } X509_STORE_set_flags(cs, X509_V_FLAG_CRL_CHECK); From a6805973b342968dd153dc7704983e49f0019ea8 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Mon, 22 Jan 2018 20:33:10 -0800 Subject: [PATCH 11/25] Fix tests and remove unnecessary X509_CRL_up_ref Signed-off-by: Andrew Dunham --- source/common/ssl/context_impl.cc | 4 ---- test/common/ssl/ssl_socket_test.cc | 7 ++++--- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index f7b3d7e9e4150..94a8a7d8546be 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -170,10 +170,6 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, for (const X509_INFO* item : list.get()) { if (item->crl) { X509_STORE_add_crl(cs, item->crl); - - // We need to hold an additional reference to the CRL, or our stack's - // destructor will free it. - X509_CRL_up_ref(item->crl); } } diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index dd830807f77e4..99c122acf18d0 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -1859,8 +1859,8 @@ TEST_P(SslSocketTest, RevokedCertificate) { "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } )EOF"; - testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "ssl.fail_verify_error", false, - GetParam()); + testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "", "", "", "ssl.fail_verify_error", + false, GetParam()); // This should succeed, since the cert isn't revoked. std::string successful_client_ctx_json = R"EOF( @@ -1869,7 +1869,8 @@ TEST_P(SslSocketTest, RevokedCertificate) { "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } )EOF"; - testUtil(successful_client_ctx_json, server_ctx_json, "", "", "ssl.handshake", true, GetParam()); + testUtil(successful_client_ctx_json, server_ctx_json, "", "", "", "", "", "ssl.handshake", true, + GetParam()); } class SslReadBufferLimitTest : public SslCertsTest, From 0db0cacd20298d278d7196886d8fd83e5c5f9551 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Tue, 23 Jan 2018 11:07:56 -0800 Subject: [PATCH 12/25] Review feedback: remove TODO Signed-off-by: Andrew Dunham --- source/common/config/tls_context_json.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/config/tls_context_json.cc b/source/common/config/tls_context_json.cc index 4c7de4fcaff88..96f2d34d1bb3d 100644 --- a/source/common/config/tls_context_json.cc +++ b/source/common/config/tls_context_json.cc @@ -61,7 +61,6 @@ void TlsContextJson::translateCommonTlsContext( common_tls_context.mutable_tls_params()->add_cipher_suites(std::string{cipher_suite}); } - // TODO: this doesn't currently support inline data if (json_tls_context.hasObject("crl_file")) { validation_context->mutable_crl()->set_filename(json_tls_context.getString("crl_file", "")); } From 8eb8991348640000b278d44d5a690a6fc9c2509a Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 13:26:17 -0800 Subject: [PATCH 13/25] Review feedback: naming and moving things around a bit Signed-off-by: Andrew Dunham --- source/common/ssl/context_config_impl.cc | 4 ++-- source/common/ssl/context_config_impl.h | 4 ++-- source/common/ssl/context_impl.cc | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 80c777a5e8c6c..bd21eb9a8076f 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -43,6 +43,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con RepeatedPtrUtil::join(config.tls_params().ecdh_curves(), ":"), DEFAULT_ECDH_CURVES)), ca_cert_(readDataSource(config.validation_context().trusted_ca(), true)), ca_cert_path_(getDataSourcePath(config.validation_context().trusted_ca())), + certificate_revocation_list_(readDataSource(config.validation_context().crl(), true)), + certificate_revocation_list_path_(getDataSourcePath(config.validation_context().crl())), cert_chain_(config.tls_certificates().empty() ? "" : readDataSource(config.tls_certificates()[0].certificate_chain(), true)), @@ -55,8 +57,6 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con private_key_path_(config.tls_certificates().empty() ? "" : getDataSourcePath(config.tls_certificates()[0].private_key())), - certificate_revocation_list_(readDataSource(config.validation_context().crl(), true)), - certificate_revocation_list_path_(getDataSourcePath(config.validation_context().crl())), verify_subject_alt_name_list_(config.validation_context().verify_subject_alt_name().begin(), config.validation_context().verify_subject_alt_name().end()), verify_certificate_hash_(config.validation_context().verify_certificate_hash().empty() diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 7c0f28190a806..2fab32339c3dc 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -68,12 +68,12 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string ecdh_curves_; const std::string ca_cert_; const std::string ca_cert_path_; + const std::string certificate_revocation_list_; + const std::string certificate_revocation_list_path_; const std::string cert_chain_; const std::string cert_chain_path_; const std::string private_key_; const std::string private_key_path_; - const std::string certificate_revocation_list_; - const std::string certificate_revocation_list_path_; const std::vector verify_subject_alt_name_list_; const std::string verify_certificate_hash_; const unsigned min_protocol_version_; diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 94a8a7d8546be..8edda69889a8c 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -166,14 +166,14 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); } - X509_STORE* cs = SSL_CTX_get_cert_store(ctx_.get()); + X509_STORE* store_ctx = SSL_CTX_get_cert_store(ctx_.get()); for (const X509_INFO* item : list.get()) { if (item->crl) { - X509_STORE_add_crl(cs, item->crl); + X509_STORE_add_crl(store_ctx, item->crl); } } - X509_STORE_set_flags(cs, X509_V_FLAG_CRL_CHECK); + X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK); } // use the server's cipher list preferences From 0a9dfba2a42f4eb877251f740a0e1b3c4eb43039 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 14:39:55 -0800 Subject: [PATCH 14/25] Review feedback: add tests for v2 config, including with inlined CRLs Signed-off-by: Andrew Dunham --- test/server/listener_manager_impl_test.cc | 61 +++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 7009f4902d3b3..04f742998eb4b 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1295,5 +1295,66 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, OriginalDstTestFilter) { EXPECT_EQ("127.0.0.2:2345", socket.localAddress()->asString()); } +TEST_F(ListenerManagerImplWithRealFiltersTest, CRLFilename) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + address: + socket_address: { address: 127.0.0.1, port_value: 1234 } + filter_chains: + - tls_context: + common_tls_context: + tls_certificates: + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + validation_context: + crl: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" } + )EOF", + Network::Address::IpVersion::v4); + + EXPECT_CALL(server_.random_, uuid()); + EXPECT_CALL(listener_factory_, createListenSocket(_, true)); + manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true); + EXPECT_EQ(1U, manager_->listeners().size()); +} + +TEST_F(ListenerManagerImplWithRealFiltersTest, CRLInline) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + address: + socket_address: { address: 127.0.0.1, port_value: 1234 } + filter_chains: + - tls_context: + common_tls_context: + tls_certificates: + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + validation_context: + crl: { inline_string: "-----BEGIN X509 CRL-----\nMIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE\nCBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME\nTHlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD\nQRcNMTcxMjIwMTcxNDA4WhcNMjcxMjE4MTcxNDA4WjAcMBoCCQDZy/Qp7iAfHxcN\nMTcxMjIwMTcxMjU0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA\nOTn5Fgb44xtFd9QGtbTElZ3iwdlcOxRHjgQMd+ydzEEZRMzMgb4/NmEsgXAsxbrx\ntKmpgll8TblscitkglvGk8s4obi/OtgxNIvn+7pOBTjmrgJkcktBUDEWRbLZjsZx\nyH+5teBZ0tH0tVy914QeGitZFV8awK1hlJwlAz9g/jo=\n-----END X509 CRL-----" } + )EOF", + Network::Address::IpVersion::v4); + + EXPECT_CALL(server_.random_, uuid()); + EXPECT_CALL(listener_factory_, createListenSocket(_, true)); + manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true); + EXPECT_EQ(1U, manager_->listeners().size()); +} + +TEST_F(ListenerManagerImplWithRealFiltersTest, InvalidCRLInline) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + address: + socket_address: { address: 127.0.0.1, port_value: 1234 } + filter_chains: + - tls_context: + common_tls_context: + tls_certificates: + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + validation_context: + crl: { inline_string: "-----BEGIN X509 CRL-----\nTOTALLY_NOT_A_CRL_HERE\n-----END X509 CRL-----\n" } + )EOF", + Network::Address::IpVersion::v4); + + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true), + EnvoyException, "Failed to load CRL from "); +} + } // namespace Server } // namespace Envoy From 71063a81ccecf89a5f9385f2042c62028afdaa50 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 14:39:05 -0800 Subject: [PATCH 15/25] Review feedback: be more specific in test assertion Signed-off-by: Andrew Dunham --- test/common/ssl/context_impl_test.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index fcf7c256d449f..917605365b43d 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -9,6 +9,7 @@ #include "test/common/ssl/ssl_certs_test.h" #include "test/mocks/runtime/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/utility.h" #include "gtest/gtest.h" @@ -305,7 +306,8 @@ TEST_F(SslServerContextImplTicketTest, CRLInvalid) { } )EOF"; - EXPECT_THROW(loadConfigJson(json), EnvoyException); + EXPECT_THROW_WITH_REGEX(loadConfigJson(json), EnvoyException, + "Failed to load CRL from .*/not_a_crl.crl$"); } } // namespace Ssl From 47150bc02d4ec99c3b216ceece35f0293e108f35 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 14:54:52 -0800 Subject: [PATCH 16/25] Review feedback: don't generate a new cert for revocation Signed-off-by: Andrew Dunham --- test/common/ssl/context_impl_test.cc | 2 +- test/common/ssl/ssl_socket_test.cc | 10 +++++----- test/common/ssl/test_data/ca_cert.crl | 10 ++++++++++ test/common/ssl/test_data/certs.sh | 9 ++------- test/common/ssl/test_data/crl/crl_number | 1 - test/common/ssl/test_data/crl/crl_number.old | 1 - test/common/ssl/test_data/crl/index.txt | 1 - test/common/ssl/test_data/crl/index.txt.attr | 1 - .../ssl/test_data/crl/index.txt.attr.old | 0 test/common/ssl/test_data/crl/index.txt.old | 0 test/common/ssl/test_data/revoked_cert.crl | 10 ---------- test/common/ssl/test_data/revoked_cert.pem | 19 ------------------- test/common/ssl/test_data/revoked_key.pem | 15 --------------- test/server/listener_manager_impl_test.cc | 14 +++++++------- 14 files changed, 25 insertions(+), 68 deletions(-) create mode 100644 test/common/ssl/test_data/ca_cert.crl delete mode 100644 test/common/ssl/test_data/crl/crl_number delete mode 100644 test/common/ssl/test_data/crl/crl_number.old delete mode 100644 test/common/ssl/test_data/crl/index.txt delete mode 100644 test/common/ssl/test_data/crl/index.txt.attr delete mode 100644 test/common/ssl/test_data/crl/index.txt.attr.old delete mode 100644 test/common/ssl/test_data/crl/index.txt.old delete mode 100644 test/common/ssl/test_data/revoked_cert.crl delete mode 100644 test/common/ssl/test_data/revoked_cert.pem delete mode 100644 test/common/ssl/test_data/revoked_key.pem diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 917605365b43d..a96f1a938bfb0 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -290,7 +290,7 @@ TEST_F(SslServerContextImplTicketTest, CRLSuccess) { { "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", - "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } )EOF"; diff --git a/test/common/ssl/ssl_socket_test.cc b/test/common/ssl/ssl_socket_test.cc index 99c122acf18d0..f1bde2f0fe41e 100644 --- a/test/common/ssl/ssl_socket_test.cc +++ b/test/common/ssl/ssl_socket_test.cc @@ -1848,15 +1848,15 @@ TEST_P(SslSocketTest, RevokedCertificate) { "cert_chain_file": "{{ test_tmpdir }}/unittestcert.pem", "private_key_file": "{{ test_tmpdir }}/unittestkey.pem", "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem", - "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } )EOF"; // This should fail, since the certificate has been revoked. std::string revoked_client_ctx_json = R"EOF( { - "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem", - "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } )EOF"; testUtil(revoked_client_ctx_json, server_ctx_json, "", "", "", "", "", "ssl.fail_verify_error", @@ -1865,8 +1865,8 @@ TEST_P(SslSocketTest, RevokedCertificate) { // This should succeed, since the cert isn't revoked. std::string successful_client_ctx_json = R"EOF( { - "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", - "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert2.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key2.pem" } )EOF"; testUtil(successful_client_ctx_json, server_ctx_json, "", "", "", "", "", "ssl.handshake", true, diff --git a/test/common/ssl/test_data/ca_cert.crl b/test/common/ssl/test_data/ca_cert.crl new file mode 100644 index 0000000000000..322daf1c122ec --- /dev/null +++ b/test/common/ssl/test_data/ca_cert.crl @@ -0,0 +1,10 @@ +-----BEGIN X509 CRL----- +MIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE +CBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME +THlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD +QRcNMTgwMTI0MjI1MzUxWhcNMjgwMTIyMjI1MzUxWjAcMBoCCQDzgo6yT9d50BcN +MTgwMTI0MjI1MzQ0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA +C5J09IOxdXzNEhkxgBu5uptb/l5NCZ3ajf1dYUkQsd0v7JBIx/LOz5XtluXJlet7 +OCCFs4a9UmCFGJoGgSAKTJX/FckprBUTnqRBfKxqGuJ/mM0ff+dMuu75yapvBrIT +ys5oVHYIdL8rk0SyIvmx20/hhu5g5AGL35Wku2YVCR4= +-----END X509 CRL----- diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index 8126a3d822f76..1b8b0f001cb32 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -14,7 +14,6 @@ set -e # openssl genrsa -out san_uri_key.pem 1024 # openssl genrsa -out selfsigned_key.pem 1024 # openssl genrsa -out expired_key.pem 1024 -# openssl genrsa -out revoked_key.pem 1024 # Generate ca_cert.pem. openssl req -new -key ca_key.pem -out ca_cert.csr -config ca_cert.cfg -batch -sha256 @@ -69,18 +68,14 @@ openssl req -new -x509 -days 730 -key selfsigned_key.pem -out selfsigned_cert.pe 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 -# Generate revoked_cert.pem -openssl req -new -key revoked_key.pem -out revoked_cert.csr -config revoked_cert.cfg -batch -sha256 -openssl x509 -req -days 730 -in revoked_cert.csr -sha256 -CA ca_cert.pem -CAkey ca_key.pem -CAcreateserial -out revoked_cert.pem -extensions v3_ca -extfile revoked_cert.cfg - # Initialize information for CRL process mkdir crl touch crl/index.txt crl/index.txt.attr echo 00 > crl/crl_number # Revoke the certificate and generate a CRL -openssl ca -revoke revoked_cert.pem -keyfile ca_key.pem -cert ca_cert.pem -config crl.cfg -openssl ca -gencrl -keyfile ca_key.pem -cert ca_cert.pem -out revoked_cert.crl -config crl.cfg +openssl ca -revoke san_dns_cert.pem -keyfile ca_key.pem -cert ca_cert.pem -config crl.cfg +openssl ca -gencrl -keyfile ca_key.pem -cert ca_cert.pem -out ca_cert.crl -config crl.cfg # Write session ticket key files openssl rand 80 > ticket_key_a diff --git a/test/common/ssl/test_data/crl/crl_number b/test/common/ssl/test_data/crl/crl_number deleted file mode 100644 index 8a0f05e166aa6..0000000000000 --- a/test/common/ssl/test_data/crl/crl_number +++ /dev/null @@ -1 +0,0 @@ -01 diff --git a/test/common/ssl/test_data/crl/crl_number.old b/test/common/ssl/test_data/crl/crl_number.old deleted file mode 100644 index 4daddb72ffc04..0000000000000 --- a/test/common/ssl/test_data/crl/crl_number.old +++ /dev/null @@ -1 +0,0 @@ -00 diff --git a/test/common/ssl/test_data/crl/index.txt b/test/common/ssl/test_data/crl/index.txt deleted file mode 100644 index 2aad5a8e499f7..0000000000000 --- a/test/common/ssl/test_data/crl/index.txt +++ /dev/null @@ -1 +0,0 @@ -R 191220171237Z 171220171254Z D9CBF429EE201F1F unknown /C=US/ST=California/L=San Francisco/O=Lyft/OU=Lyft Engineering/CN=Revoked Certificate diff --git a/test/common/ssl/test_data/crl/index.txt.attr b/test/common/ssl/test_data/crl/index.txt.attr deleted file mode 100644 index 3a7e39e6ee60a..0000000000000 --- a/test/common/ssl/test_data/crl/index.txt.attr +++ /dev/null @@ -1 +0,0 @@ -unique_subject = no diff --git a/test/common/ssl/test_data/crl/index.txt.attr.old b/test/common/ssl/test_data/crl/index.txt.attr.old deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/test/common/ssl/test_data/crl/index.txt.old b/test/common/ssl/test_data/crl/index.txt.old deleted file mode 100644 index e69de29bb2d1d..0000000000000 diff --git a/test/common/ssl/test_data/revoked_cert.crl b/test/common/ssl/test_data/revoked_cert.crl deleted file mode 100644 index 722a277f4d602..0000000000000 --- a/test/common/ssl/test_data/revoked_cert.crl +++ /dev/null @@ -1,10 +0,0 @@ ------BEGIN X509 CRL----- -MIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE -CBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME -THlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD -QRcNMTcxMjIwMTcxNDA4WhcNMjcxMjE4MTcxNDA4WjAcMBoCCQDZy/Qp7iAfHxcN -MTcxMjIwMTcxMjU0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA -OTn5Fgb44xtFd9QGtbTElZ3iwdlcOxRHjgQMd+ydzEEZRMzMgb4/NmEsgXAsxbrx -tKmpgll8TblscitkglvGk8s4obi/OtgxNIvn+7pOBTjmrgJkcktBUDEWRbLZjsZx -yH+5teBZ0tH0tVy914QeGitZFV8awK1hlJwlAz9g/jo= ------END X509 CRL----- diff --git a/test/common/ssl/test_data/revoked_cert.pem b/test/common/ssl/test_data/revoked_cert.pem deleted file mode 100644 index 4a3dedb4b4aaa..0000000000000 --- a/test/common/ssl/test_data/revoked_cert.pem +++ /dev/null @@ -1,19 +0,0 @@ ------BEGIN CERTIFICATE----- -MIIDFTCCAn6gAwIBAgIJANnL9CnuIB8fMA0GCSqGSIb3DQEBCwUAMHYxCzAJBgNV -BAYTAlVTMRMwEQYDVQQIEwpDYWxpZm9ybmlhMRYwFAYDVQQHEw1TYW4gRnJhbmNp -c2NvMQ0wCwYDVQQKEwRMeWZ0MRkwFwYDVQQLExBMeWZ0IEVuZ2luZWVyaW5nMRAw -DgYDVQQDEwdUZXN0IENBMB4XDTE3MTIyMDE3MTIzN1oXDTE5MTIyMDE3MTIzN1ow -gYIxCzAJBgNVBAYTAlVTMRMwEQYDVQQIDApDYWxpZm9ybmlhMRYwFAYDVQQHDA1T -YW4gRnJhbmNpc2NvMQ0wCwYDVQQKDARMeWZ0MRkwFwYDVQQLDBBMeWZ0IEVuZ2lu -ZWVyaW5nMRwwGgYDVQQDDBNSZXZva2VkIENlcnRpZmljYXRlMIGfMA0GCSqGSIb3 -DQEBAQUAA4GNADCBiQKBgQDTz3LeIa8b7vz0y0ha9yty0oAzySpVeGBOG82bNa9U -M2dDtfg5/oaTBafKhFNLenMy9KSQHBErXQ5jbWHGQEqceVHOE1qsVptstXUkQqfZ -OkBJvTJZuBxmHD+ly83+vhimHfyp4FZBQ4rQ6INlqRpx4x8aE+oxMnM16petjD6w -5wIDAQABo4GdMIGaMAwGA1UdEwEB/wQCMAAwCwYDVR0PBAQDAgXgMB0GA1UdJQQW -MBQGCCsGAQUFBwMCBggrBgEFBQcDATAeBgNVHREEFzAVghNzZXJ2ZXIxLmV4YW1w -bGUuY29tMB0GA1UdDgQWBBQxiVjNEJfrpCbaKcjpPqsKxRIrOTAfBgNVHSMEGDAW -gBQ7eKRRTxaEkxxIKHoMrSuWQcp9eTANBgkqhkiG9w0BAQsFAAOBgQCR6MiZA1Tk -R39ggdS4c4yYnllDv8IkpBV4OP8+P8uwXJhu4zwZ1j6d6dBcvpYBc4E9XmiEdF67 -+j50/0QOj5aMj/OFFSqUO1CogQmDasepVboLBSyR34QXamgug94hjHeO7lW9oAcr -r+Aq/0AjkspEmTQfioy6IxM4wTnwhFRzGA== ------END CERTIFICATE----- diff --git a/test/common/ssl/test_data/revoked_key.pem b/test/common/ssl/test_data/revoked_key.pem deleted file mode 100644 index 7f9b0a3b5c7c7..0000000000000 --- a/test/common/ssl/test_data/revoked_key.pem +++ /dev/null @@ -1,15 +0,0 @@ ------BEGIN RSA PRIVATE KEY----- -MIICXgIBAAKBgQDTz3LeIa8b7vz0y0ha9yty0oAzySpVeGBOG82bNa9UM2dDtfg5 -/oaTBafKhFNLenMy9KSQHBErXQ5jbWHGQEqceVHOE1qsVptstXUkQqfZOkBJvTJZ -uBxmHD+ly83+vhimHfyp4FZBQ4rQ6INlqRpx4x8aE+oxMnM16petjD6w5wIDAQAB -AoGBAKRczp5hNSlQAytStAsi0qx/fMyyxg8dIl56ZMqUlkGYwgFhLAaU5IkiUlps -5NYlZ0+bWDgcD5a+13OAZecZ7MqmVE3Ud3gV/tkHZdH90zCPx58IiwsIinV5BqP0 -XrSJWgSieCx+Zg8BDE+ZZpfCuOsAVE7VweOQIdiD+U6nwecBAkEA6QldHBPlNKFZ -EHVqL8pkYYub5b2e0l+ZEf6NshB/0m1ryeCJn3LHp/7jUd/9wlDqZtLMlWo1pdPx -K/+9MHtLEQJBAOiuoIWGlXOcVO6UPNqpWrx5pfzv+vMpBJiMvMrBmUwqFxwHbqCP -AB2k5H+xdFany0EvMPjg6D/hJd4yXQDKDHcCQQDagQcm7pi5spgaUJ3SVcmtlQQG -dLfYtf6G2tHtpn7TxfmNftZMBYmjweFPweDkNI60/u8JIl9PL90wzkiMju6hAkBR -LDKFwni610vt2zsLkU89NzcH8XRbhfC7g0WNelKPdpOPTKx0SM7iiJbKUU7juC+5 -Msxj1ppPRq1eQbWeQ95rAkEAgkcunIs1zLMO/trrB3+ILuUcZP5HDlPFQzDQ2ofX -NKA3FbkD1gR47Xz9iRxkclQyZ9mFTg6BQzw3+v8iFRzO1A== ------END RSA PRIVATE KEY----- diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 04f742998eb4b..4fef19d2c2339 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1303,10 +1303,10 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, CRLFilename) { - tls_context: common_tls_context: tls_certificates: - - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } - private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: - crl: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.crl" } + crl: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } )EOF", Network::Address::IpVersion::v4); @@ -1324,8 +1324,8 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, CRLInline) { - tls_context: common_tls_context: tls_certificates: - - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } - private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: crl: { inline_string: "-----BEGIN X509 CRL-----\nMIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE\nCBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME\nTHlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD\nQRcNMTcxMjIwMTcxNDA4WhcNMjcxMjE4MTcxNDA4WjAcMBoCCQDZy/Qp7iAfHxcN\nMTcxMjIwMTcxMjU0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA\nOTn5Fgb44xtFd9QGtbTElZ3iwdlcOxRHjgQMd+ydzEEZRMzMgb4/NmEsgXAsxbrx\ntKmpgll8TblscitkglvGk8s4obi/OtgxNIvn+7pOBTjmrgJkcktBUDEWRbLZjsZx\nyH+5teBZ0tH0tVy914QeGitZFV8awK1hlJwlAz9g/jo=\n-----END X509 CRL-----" } )EOF", @@ -1345,8 +1345,8 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, InvalidCRLInline) { - tls_context: common_tls_context: tls_certificates: - - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_cert.pem" } - private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/revoked_key.pem" } + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: crl: { inline_string: "-----BEGIN X509 CRL-----\nTOTALLY_NOT_A_CRL_HERE\n-----END X509 CRL-----\n" } )EOF", From 5b53667fbd313772153f981f404567ff9f753397 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 15:18:47 -0800 Subject: [PATCH 17/25] Review feedback: move CRL configuration into ca_cert.cfg Signed-off-by: Andrew Dunham --- test/common/ssl/test_data/ca_cert.cfg | 16 ++++++++++++++++ test/common/ssl/test_data/certs.sh | 9 ++++----- test/common/ssl/test_data/crl.cfg | 18 ------------------ 3 files changed, 20 insertions(+), 23 deletions(-) delete mode 100644 test/common/ssl/test_data/crl.cfg diff --git a/test/common/ssl/test_data/ca_cert.cfg b/test/common/ssl/test_data/ca_cert.cfg index 52f9521628f05..55f6f2832db8b 100644 --- a/test/common/ssl/test_data/ca_cert.cfg +++ b/test/common/ssl/test_data/ca_cert.cfg @@ -27,3 +27,19 @@ basicConstraints = critical, CA:TRUE keyUsage = critical, cRLSign, keyCertSign subjectKeyIdentifier = hash authorityKeyIdentifier = keyid:always + +[ca] +default_ca = CA_default + +[CA_default] +database = crl_index.txt +crlnumber = crl_number + +default_days = 3650 +default_crl_days = 3650 +default_md = sha256 +preserve = no +unique_subject = no + +[crl_ext] +authorityKeyIdentifier = keyid:always,issuer:always diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index 1b8b0f001cb32..d22b410d504b6 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -69,13 +69,12 @@ openssl req -new -key expired_key.pem -out expired_cert.csr -config selfsigned_c openssl x509 -req -days -365 -in expired_cert.csr -signkey expired_key.pem -out expired_cert.pem # Initialize information for CRL process -mkdir crl -touch crl/index.txt crl/index.txt.attr -echo 00 > crl/crl_number +touch crl_index.txt crl_index.txt.attr +echo 00 > crl_number # Revoke the certificate and generate a CRL -openssl ca -revoke san_dns_cert.pem -keyfile ca_key.pem -cert ca_cert.pem -config crl.cfg -openssl ca -gencrl -keyfile ca_key.pem -cert ca_cert.pem -out ca_cert.crl -config crl.cfg +openssl ca -revoke san_dns_cert.pem -keyfile ca_key.pem -cert ca_cert.pem -config ca_cert.cfg +openssl ca -gencrl -keyfile ca_key.pem -cert ca_cert.pem -out ca_cert.crl -config ca_cert.cfg # Write session ticket key files openssl rand 80 > ticket_key_a diff --git a/test/common/ssl/test_data/crl.cfg b/test/common/ssl/test_data/crl.cfg deleted file mode 100644 index ee3ac47cc5ae5..0000000000000 --- a/test/common/ssl/test_data/crl.cfg +++ /dev/null @@ -1,18 +0,0 @@ -[ default ] -dir = ./crl - -[ ca ] -default_ca = CA_default - -[ CA_default ] -database = $dir/index.txt -crlnumber = $dir/crl_number - -default_days = 3650 -default_crl_days = 3650 -default_md = sha256 -preserve = no -unique_subject = no - -[ crl_ext ] -authorityKeyIdentifier = keyid:always,issuer:always From ee2530f533ed6696d5ff935537ea7debac80ae2b Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Wed, 24 Jan 2018 17:55:31 -0800 Subject: [PATCH 18/25] Review feedback: add the X509_V_FLAG_CRL_CHECK_ALL flag Signed-off-by: Andrew Dunham --- source/common/ssl/context_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 8edda69889a8c..230295615287e 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -173,7 +173,7 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, } } - X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK); + X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } // use the server's cipher list preferences From c1c66ac7d8ef17852756e388dd84e653f8436019 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 14:07:11 -0800 Subject: [PATCH 19/25] Review feedback: fix test data script and remove unnecessary file Signed-off-by: Andrew Dunham --- test/common/ssl/test_data/certs.sh | 2 +- test/common/ssl/test_data/revoked_cert.cfg | 36 ---------------------- 2 files changed, 1 insertion(+), 37 deletions(-) delete mode 100644 test/common/ssl/test_data/revoked_cert.cfg diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index d22b410d504b6..584e693c82688 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -83,4 +83,4 @@ openssl rand 79 > ticket_key_wrong_len rm *csr rm *srl -rm -r crl +rm -r crl_* diff --git a/test/common/ssl/test_data/revoked_cert.cfg b/test/common/ssl/test_data/revoked_cert.cfg deleted file mode 100644 index 0a8344e35bffb..0000000000000 --- a/test/common/ssl/test_data/revoked_cert.cfg +++ /dev/null @@ -1,36 +0,0 @@ -[req] -distinguished_name = req_distinguished_name -req_extensions = v3_req - -[req_distinguished_name] -countryName = US -countryName_default = US -stateOrProvinceName = California -stateOrProvinceName_default = California -localityName = San Francisco -localityName_default = San Francisco -organizationName = Lyft -organizationName_default = Lyft -organizationalUnitName = Lyft Engineering -organizationalUnitName_default = Lyft Engineering -commonName = Revoked Certificate -commonName_default = Revoked Certificate -commonName_max = 64 - -[v3_req] -basicConstraints = CA:FALSE -keyUsage = nonRepudiation, digitalSignature, keyEncipherment -extendedKeyUsage = clientAuth, serverAuth -subjectAltName = @alt_names -subjectKeyIdentifier = hash - -[v3_ca] -basicConstraints = critical, CA:FALSE -keyUsage = nonRepudiation, digitalSignature, keyEncipherment -extendedKeyUsage = clientAuth, serverAuth -subjectAltName = @alt_names -subjectKeyIdentifier = hash -authorityKeyIdentifier = keyid:always - -[alt_names] -DNS.1 = server1.example.com From f1045df9b3f7f8a87a49ab7fe27a928c9b5d5808 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 14:09:46 -0800 Subject: [PATCH 20/25] Review feedback: move remaining CRL code to immediately after CA code Signed-off-by: Andrew Dunham --- include/envoy/ssl/context_config.h | 22 +++++------ source/common/config/tls_context_json.cc | 7 ++-- source/common/ssl/context_config_impl.h | 16 ++++---- source/common/ssl/context_impl.cc | 48 ++++++++++++------------ 4 files changed, 46 insertions(+), 47 deletions(-) diff --git a/include/envoy/ssl/context_config.h b/include/envoy/ssl/context_config.h index a4f21419e020f..c435454006d8d 100644 --- a/include/envoy/ssl/context_config.h +++ b/include/envoy/ssl/context_config.h @@ -49,6 +49,17 @@ class ContextConfig { */ virtual const std::string& caCertPath() const PURE; + /** + * @return The CRL to check if a cert is revoked. + */ + virtual const std::string& certificateRevocationList() const PURE; + + /** + * @return Path of the certificate revocation list, or "" if the CRL + * was inlined. + */ + virtual const std::string& certificateRevocationListPath() const PURE; + /** * @return The certificate chain used to identify the local side. */ @@ -71,17 +82,6 @@ class ContextConfig { */ virtual const std::string& privateKeyPath() const PURE; - /** - * @return The CRL to check if a cert is revoked. - */ - virtual const std::string& certificateRevocationList() const PURE; - - /** - * @return Path of the certificate revocation list, or "" if the CRL - * was inlined. - */ - virtual const std::string& certificateRevocationListPath() const PURE; - /** * @return The subject alt names to be verified, if enabled. Otherwise, "" */ diff --git a/source/common/config/tls_context_json.cc b/source/common/config/tls_context_json.cc index 96f2d34d1bb3d..1c106056a93cc 100644 --- a/source/common/config/tls_context_json.cc +++ b/source/common/config/tls_context_json.cc @@ -48,6 +48,9 @@ void TlsContextJson::translateCommonTlsContext( validation_context->mutable_trusted_ca()->set_filename( json_tls_context.getString("ca_cert_file", "")); } + if (json_tls_context.hasObject("crl_file")) { + validation_context->mutable_crl()->set_filename(json_tls_context.getString("crl_file", "")); + } if (json_tls_context.hasObject("verify_certificate_hash")) { validation_context->add_verify_certificate_hash( json_tls_context.getString("verify_certificate_hash")); @@ -61,10 +64,6 @@ void TlsContextJson::translateCommonTlsContext( common_tls_context.mutable_tls_params()->add_cipher_suites(std::string{cipher_suite}); } - if (json_tls_context.hasObject("crl_file")) { - validation_context->mutable_crl()->set_filename(json_tls_context.getString("crl_file", "")); - } - const std::string ecdh_curves_str{json_tls_context.getString("ecdh_curves", "")}; for (auto ecdh_curve : StringUtil::splitToken(ecdh_curves_str, ":")) { common_tls_context.mutable_tls_params()->add_ecdh_curves(std::string{ecdh_curve}); diff --git a/source/common/ssl/context_config_impl.h b/source/common/ssl/context_config_impl.h index 2fab32339c3dc..f213b387503bc 100644 --- a/source/common/ssl/context_config_impl.h +++ b/source/common/ssl/context_config_impl.h @@ -25,14 +25,6 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { const std::string& caCertPath() const override { return (ca_cert_path_.empty() && !ca_cert_.empty()) ? INLINE_STRING : ca_cert_path_; } - const std::string& certChain() const override { return cert_chain_; } - const std::string& certChainPath() const override { - return (cert_chain_path_.empty() && !cert_chain_.empty()) ? INLINE_STRING : cert_chain_path_; - } - const std::string& privateKey() const override { return private_key_; } - const std::string& privateKeyPath() const override { - return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; - } const std::string& certificateRevocationList() const override { return certificate_revocation_list_; } @@ -41,6 +33,14 @@ class ContextConfigImpl : public virtual Ssl::ContextConfig { ? INLINE_STRING : certificate_revocation_list_path_; } + const std::string& certChain() const override { return cert_chain_; } + const std::string& certChainPath() const override { + return (cert_chain_path_.empty() && !cert_chain_.empty()) ? INLINE_STRING : cert_chain_path_; + } + const std::string& privateKey() const override { return private_key_; } + const std::string& privateKeyPath() const override { + return (private_key_path_.empty() && !private_key_.empty()) ? INLINE_STRING : private_key_path_; + } const std::vector& verifySubjectAltNameList() const override { return verify_subject_alt_name_list_; }; diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 230295615287e..96af012fba279 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -85,6 +85,30 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, verify_mode = SSL_VERIFY_PEER; } + if (!config.certificateRevocationList().empty()) { + bssl::UniquePtr bio( + BIO_new_mem_buf(const_cast(config.certificateRevocationList().data()), + config.certificateRevocationList().size())); + RELEASE_ASSERT(bio != nullptr); + + // Based on BoringSSL's X509_load_cert_crl_file(). + bssl::UniquePtr list( + PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); + if (list == nullptr) { + throw EnvoyException( + fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); + } + + X509_STORE* store_ctx = SSL_CTX_get_cert_store(ctx_.get()); + for (const X509_INFO* item : list.get()) { + if (item->crl) { + X509_STORE_add_crl(store_ctx, item->crl); + } + } + + X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); + } + if (!config.verifySubjectAltNameList().empty()) { verify_subject_alt_name_list_ = config.verifySubjectAltNameList(); verify_mode = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT; @@ -152,30 +176,6 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, } } - if (!config.certificateRevocationList().empty()) { - bssl::UniquePtr bio( - BIO_new_mem_buf(const_cast(config.certificateRevocationList().data()), - config.certificateRevocationList().size())); - RELEASE_ASSERT(bio != nullptr); - - // Based on BoringSSL's X509_load_cert_crl_file(). - bssl::UniquePtr list( - PEM_X509_INFO_read_bio(bio.get(), nullptr, nullptr, nullptr)); - if (list == nullptr) { - throw EnvoyException( - fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); - } - - X509_STORE* store_ctx = SSL_CTX_get_cert_store(ctx_.get()); - for (const X509_INFO* item : list.get()) { - if (item->crl) { - X509_STORE_add_crl(store_ctx, item->crl); - } - } - - X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); - } - // use the server's cipher list preferences SSL_CTX_set_options(ctx_.get(), SSL_OP_CIPHER_SERVER_PREFERENCE); From 8ce73bf2fed8a3c8f96c94aa02f09e18ae74e3ba Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 14:29:43 -0800 Subject: [PATCH 21/25] Review feedback: Clean up X509_STORE* code and remove unnecessary '-r' from script Signed-off-by: Andrew Dunham --- source/common/ssl/context_impl.cc | 12 +++++++----- test/common/ssl/test_data/certs.sh | 2 +- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/source/common/ssl/context_impl.cc b/source/common/ssl/context_impl.cc index 96af012fba279..6cfc950ba203d 100644 --- a/source/common/ssl/context_impl.cc +++ b/source/common/ssl/context_impl.cc @@ -66,16 +66,18 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, throw EnvoyException( fmt::format("Failed to load trusted CA certificates from {}", config.caCertPath())); } + + X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get()); for (const X509_INFO* item : list.get()) { if (item->x509) { - X509_STORE_add_cert(SSL_CTX_get_cert_store(ctx_.get()), item->x509); + X509_STORE_add_cert(store, item->x509); if (ca_cert_ == nullptr) { X509_up_ref(item->x509); ca_cert_.reset(item->x509); } } if (item->crl) { - X509_STORE_add_crl(SSL_CTX_get_cert_store(ctx_.get()), item->crl); + X509_STORE_add_crl(store, item->crl); } } if (ca_cert_ == nullptr) { @@ -99,14 +101,14 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, fmt::format("Failed to load CRL from {}", config.certificateRevocationListPath())); } - X509_STORE* store_ctx = SSL_CTX_get_cert_store(ctx_.get()); + X509_STORE* store = SSL_CTX_get_cert_store(ctx_.get()); for (const X509_INFO* item : list.get()) { if (item->crl) { - X509_STORE_add_crl(store_ctx, item->crl); + X509_STORE_add_crl(store, item->crl); } } - X509_STORE_set_flags(store_ctx, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); + X509_STORE_set_flags(store, X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL); } if (!config.verifySubjectAltNameList().empty()) { diff --git a/test/common/ssl/test_data/certs.sh b/test/common/ssl/test_data/certs.sh index 584e693c82688..ab7832afdfdbe 100755 --- a/test/common/ssl/test_data/certs.sh +++ b/test/common/ssl/test_data/certs.sh @@ -83,4 +83,4 @@ openssl rand 79 > ticket_key_wrong_len rm *csr rm *srl -rm -r crl_* +rm crl_* From d4c3ccd1bd6e999cd334bbcac0f31698f63e9d8a Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 16:18:39 -0800 Subject: [PATCH 22/25] Review feedback: throw an exception if we have a CRL but no CA certificate Signed-off-by: Andrew Dunham --- source/common/ssl/context_config_impl.cc | 3 +++ test/common/ssl/context_impl_test.cc | 15 +++++++++++++++ test/server/listener_manager_impl_test.cc | 22 ++++++++++++++++++++++ 3 files changed, 40 insertions(+) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index bd21eb9a8076f..09840eb94c823 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -68,6 +68,9 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con tlsVersionFromProto(config.tls_params().tls_maximum_protocol_version(), TLS1_2_VERSION)) { // TODO(htuch): Support multiple hashes. ASSERT(config.validation_context().verify_certificate_hash().size() <= 1); + if (ca_cert_.empty() && !certificate_revocation_list_.empty()) { + throw EnvoyException("Cannot have a CRL without a CA certificate"); + } } const std::string ContextConfigImpl::readDataSource(const envoy::api::v2::DataSource& source, diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index a96f1a938bfb0..618981744d39e 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -290,6 +290,7 @@ TEST_F(SslServerContextImplTicketTest, CRLSuccess) { { "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", + "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem", "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } )EOF"; @@ -302,6 +303,7 @@ TEST_F(SslServerContextImplTicketTest, CRLInvalid) { { "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", + "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem", "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/not_a_crl.crl" } )EOF"; @@ -310,5 +312,18 @@ TEST_F(SslServerContextImplTicketTest, CRLInvalid) { "Failed to load CRL from .*/not_a_crl.crl$"); } +TEST_F(SslServerContextImplTicketTest, CRLWithNoCA) { + std::string json = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem", + "crl_file": "{{ test_rundir }}/test/common/ssl/test_data/not_a_crl.crl" + } + )EOF"; + + EXPECT_THROW_WITH_MESSAGE(loadConfigJson(json), EnvoyException, + "Cannot have a CRL without a CA certificate"); +} + } // namespace Ssl } // namespace Envoy diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index 4fef19d2c2339..cbe4faf0cc3bf 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1306,6 +1306,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, CRLFilename) { - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } crl: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } )EOF", Network::Address::IpVersion::v4); @@ -1327,6 +1328,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, CRLInline) { - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } crl: { inline_string: "-----BEGIN X509 CRL-----\nMIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE\nCBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME\nTHlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD\nQRcNMTcxMjIwMTcxNDA4WhcNMjcxMjE4MTcxNDA4WjAcMBoCCQDZy/Qp7iAfHxcN\nMTcxMjIwMTcxMjU0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA\nOTn5Fgb44xtFd9QGtbTElZ3iwdlcOxRHjgQMd+ydzEEZRMzMgb4/NmEsgXAsxbrx\ntKmpgll8TblscitkglvGk8s4obi/OtgxNIvn+7pOBTjmrgJkcktBUDEWRbLZjsZx\nyH+5teBZ0tH0tVy914QeGitZFV8awK1hlJwlAz9g/jo=\n-----END X509 CRL-----" } )EOF", Network::Address::IpVersion::v4); @@ -1348,6 +1350,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, InvalidCRLInline) { - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } validation_context: + trusted_ca: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" } crl: { inline_string: "-----BEGIN X509 CRL-----\nTOTALLY_NOT_A_CRL_HERE\n-----END X509 CRL-----\n" } )EOF", Network::Address::IpVersion::v4); @@ -1356,5 +1359,24 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, InvalidCRLInline) { EnvoyException, "Failed to load CRL from "); } +TEST_F(ListenerManagerImplWithRealFiltersTest, CRLWithNoCA) { + const std::string yaml = TestEnvironment::substitute(R"EOF( + address: + socket_address: { address: 127.0.0.1, port_value: 1234 } + filter_chains: + - tls_context: + common_tls_context: + tls_certificates: + - certificate_chain: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_cert.pem" } + private_key: { filename: "{{ test_rundir }}/test/common/ssl/test_data/san_dns_key.pem" } + validation_context: + crl: { filename: "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.crl" } + )EOF", + Network::Address::IpVersion::v4); + + EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true), + EnvoyException, "Cannot have a CRL without a CA certificate"); +} + } // namespace Server } // namespace Envoy From 7e07bf06471f0ecc6794a6392c3c95413570c200 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 17:34:19 -0800 Subject: [PATCH 23/25] Review feedback: change exception message to match other errors Signed-off-by: Andrew Dunham --- source/common/ssl/context_config_impl.cc | 3 ++- test/common/ssl/context_impl_test.cc | 4 ++-- test/server/listener_manager_impl_test.cc | 5 +++-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/source/common/ssl/context_config_impl.cc b/source/common/ssl/context_config_impl.cc index 09840eb94c823..666adc2771aa5 100644 --- a/source/common/ssl/context_config_impl.cc +++ b/source/common/ssl/context_config_impl.cc @@ -69,7 +69,8 @@ ContextConfigImpl::ContextConfigImpl(const envoy::api::v2::CommonTlsContext& con // TODO(htuch): Support multiple hashes. ASSERT(config.validation_context().verify_certificate_hash().size() <= 1); if (ca_cert_.empty() && !certificate_revocation_list_.empty()) { - throw EnvoyException("Cannot have a CRL without a CA certificate"); + throw EnvoyException(fmt::format("Failed to load CRL from {} without trusted CA certificates", + certificateRevocationListPath())); } } diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index 618981744d39e..c3a0de85082c4 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -321,8 +321,8 @@ TEST_F(SslServerContextImplTicketTest, CRLWithNoCA) { } )EOF"; - EXPECT_THROW_WITH_MESSAGE(loadConfigJson(json), EnvoyException, - "Cannot have a CRL without a CA certificate"); + EXPECT_THROW_WITH_REGEX(loadConfigJson(json), EnvoyException, + "^Failed to load CRL from .* without trusted CA certificates$"); } } // namespace Ssl diff --git a/test/server/listener_manager_impl_test.cc b/test/server/listener_manager_impl_test.cc index cbe4faf0cc3bf..aa8a088a7d6e9 100644 --- a/test/server/listener_manager_impl_test.cc +++ b/test/server/listener_manager_impl_test.cc @@ -1374,8 +1374,9 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, CRLWithNoCA) { )EOF", Network::Address::IpVersion::v4); - EXPECT_THROW_WITH_MESSAGE(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true), - EnvoyException, "Cannot have a CRL without a CA certificate"); + EXPECT_THROW_WITH_REGEX(manager_->addOrUpdateListener(parseListenerFromV2Yaml(yaml), true), + EnvoyException, + "^Failed to load CRL from .* without trusted CA certificates$"); } } // namespace Server From fa2dfb708a79ad1933bccc7bd9a33c962134a4a1 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 17:56:33 -0800 Subject: [PATCH 24/25] Review feedback: fix missing '^' and add test data comment Signed-off-by: Andrew Dunham --- test/common/ssl/context_impl_test.cc | 2 +- test/common/ssl/test_data/README.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/ssl/context_impl_test.cc b/test/common/ssl/context_impl_test.cc index c3a0de85082c4..cd1d73a8c625f 100644 --- a/test/common/ssl/context_impl_test.cc +++ b/test/common/ssl/context_impl_test.cc @@ -309,7 +309,7 @@ TEST_F(SslServerContextImplTicketTest, CRLInvalid) { )EOF"; EXPECT_THROW_WITH_REGEX(loadConfigJson(json), EnvoyException, - "Failed to load CRL from .*/not_a_crl.crl$"); + "^Failed to load CRL from .*/not_a_crl.crl$"); } TEST_F(SslServerContextImplTicketTest, CRLWithNoCA) { diff --git a/test/common/ssl/test_data/README.md b/test/common/ssl/test_data/README.md index 5b8dcf372fc2c..e75785e04b821 100644 --- a/test/common/ssl/test_data/README.md +++ b/test/common/ssl/test_data/README.md @@ -2,7 +2,8 @@ There are 10 identities: - **CA**: Certificate Authority for **No SAN**, **SAN With URI** and **SAN With DNS**. It has the self-signed certificate *ca_cert.pem*. *ca_key.pem* is its - private key. + private key. Additionally, we create a CRL for this CA (*ca_cert.crl*) that + revokes the certificate *san_dns_cert.pem*. - **Intermediate CA**: Intermediate Certificate Authority, signed by the **CA**. It has the certificate *intermediate_ca_cert.pem". *intermediate_ca_key.pem* is its private key. From cdbb2b9f8e3257dc2d841c202784573fb1e81be2 Mon Sep 17 00:00:00 2001 From: Andrew Dunham Date: Thu, 25 Jan 2018 20:00:56 -0800 Subject: [PATCH 25/25] Review feedback: formatting fixes Signed-off-by: Andrew Dunham --- test/common/ssl/test_data/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/ssl/test_data/README.md b/test/common/ssl/test_data/README.md index e75785e04b821..46fb133641b9a 100644 --- a/test/common/ssl/test_data/README.md +++ b/test/common/ssl/test_data/README.md @@ -2,7 +2,7 @@ There are 10 identities: - **CA**: Certificate Authority for **No SAN**, **SAN With URI** and **SAN With DNS**. It has the self-signed certificate *ca_cert.pem*. *ca_key.pem* is its - private key. Additionally, we create a CRL for this CA (*ca_cert.crl*) that + private key. Additionally, we create a CRL for this CA (*ca_cert.crl*) that revokes the certificate *san_dns_cert.pem*. - **Intermediate CA**: Intermediate Certificate Authority, signed by the **CA**. It has the certificate *intermediate_ca_cert.pem". *intermediate_ca_key.pem*