-
Notifications
You must be signed in to change notification settings - Fork 5.4k
tls: Add CRL support #2255
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
tls: Add CRL support #2255
Changes from all commits
d4f4c64
26a9abc
40e9985
3599a9d
c1b339b
99abed1
6d15f1f
6914839
f05137d
518de3c
a680597
0db0cac
8eb8991
0a9dfba
71063a8
47150bc
5b53667
ee2530f
c1c66ac
f1045df
8ce73bf
d4c3ccd
7e07bf0
fa2dfb7
cdbb2b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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())), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is one more thing missing here. You should throw an exception (down in the function body) if CRL is set, but there is no CA. All v1 tests are doing this, which obviously shouldn't work.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good point - I've pushed a commit that adds this, along with some tests. |
||
| cert_chain_(config.tls_certificates().empty() | ||
| ? "" | ||
| : readDataSource(config.tls_certificates()[0].certificate_chain(), true)), | ||
|
|
@@ -66,6 +68,10 @@ 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(fmt::format("Failed to load CRL from {} without trusted CA certificates", | ||
| certificateRevocationListPath())); | ||
| } | ||
| } | ||
|
|
||
| const std::string ContextConfigImpl::readDataSource(const envoy::api::v2::DataSource& source, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
||
|
|
@@ -284,5 +285,45 @@ TEST_F(SslServerContextImplTicketTest, TicketKeyInlineStringFailTooSmall) { | |
| EXPECT_THROW(loadConfigV2(cfg), EnvoyException); | ||
| } | ||
|
|
||
| TEST_F(SslServerContextImplTicketTest, CRLSuccess) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please also add a test case for invalid crl (empty file maybe?)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! I think an empty file is actually okay (list with 0 elements), so I added a file with invalid PEM data. |
||
| 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", | ||
| "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"; | ||
|
|
||
| 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", | ||
| "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"; | ||
|
|
||
| EXPECT_THROW_WITH_REGEX(loadConfigJson(json), EnvoyException, | ||
| "^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_REGEX(loadConfigJson(json), EnvoyException, | ||
| "^Failed to load CRL from .* without trusted CA certificates$"); | ||
| } | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| -----BEGIN X509 CRL----- | ||
| MIIBbDCB1gIBATANBgkqhkiG9w0BAQsFADB2MQswCQYDVQQGEwJVUzETMBEGA1UE | ||
| CBMKQ2FsaWZvcm5pYTEWMBQGA1UEBxMNU2FuIEZyYW5jaXNjbzENMAsGA1UEChME | ||
| THlmdDEZMBcGA1UECxMQTHlmdCBFbmdpbmVlcmluZzEQMA4GA1UEAxMHVGVzdCBD | ||
| QRcNMTgwMTI0MjI1MzUxWhcNMjgwMTIyMjI1MzUxWjAcMBoCCQDzgo6yT9d50BcN | ||
| MTgwMTI0MjI1MzQ0WqAOMAwwCgYDVR0UBAMCAQAwDQYJKoZIhvcNAQELBQADgYEA | ||
| C5J09IOxdXzNEhkxgBu5uptb/l5NCZ3ajf1dYUkQsd0v7JBIx/LOz5XtluXJlet7 | ||
| OCCFs4a9UmCFGJoGgSAKTJX/FckprBUTnqRBfKxqGuJ/mM0ff+dMuu75yapvBrIT | ||
| ys5oVHYIdL8rk0SyIvmx20/hhu5g5AGL35Wku2YVCR4= | ||
| -----END X509 CRL----- |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| -----BEGIN X509 CRL----- | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you replace this with inline test?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I can do that easily since the v1 config doesn't support inline data, AFAIK. Happy to write out a temporary file during the test, perhaps?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's fine to replace the existing v1 test with v2 test, since that's the recommended syntax anyway... Also, there should be v2 tests for success / invalid content (i.e. replacement for this file) / invalid file path.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added success / failure cases to the v2 tests - do you want me to just remove the v1 tests in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Up to you, I'm fine either way. |
||
| TOTALLY_NOT_A_CRL_HERE | ||
| -----END X509 CRL----- | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v1 APIs are deprecated, so I'm not sure if we're supposed to keep adding things here? cc @mattklein123 @htuch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point v2 is required, v1 is optional but not blocked. If folks want to add and doc no problem.