tls: Add CRL support [WIP]#2077
Conversation
Signed-off-by: Andrew Dunham <adunham@stripe.com>
…text Signed-off-by: Andrew Dunham <adunham@stripe.com>
adunham-stripe
force-pushed
the
adunham/add-crl-support
branch
from
2d8354d to
21193cb
Compare
|
Worth noting that I haven't tested this actually works yet! Wiring up the CRL configuration seems sane, but testing that it handles revoked certs properly hasn't yet been tested. That being said, the OpenSSL-specific bits are pretty small and live here: https://github.com/envoyproxy/envoy/pull/2077/files#diff-1e0c33cf507deb2dbbf2e81e22dfbd3eR95 I'll try and get a very simple forward-proxy-alike test going that just uses self-signed everything and verify that it works; mostly wanted to get this up here for initial feedback. EDIT: also worth noting, I'm probably going to submit a PR to add support for multiple CRLs, if this PR gets merged. It's not too much more work to add that, but I figure that I'll start with this 😄 |
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
For testing, I would add some tests in envoy/test/common/ssl/connection_impl_test.cc. This is where all the integration-with-boringSSL tests end up, for better or for worse.
| private_key_file_(config.tls_certificates().empty() | ||
| ? "" | ||
| : config.tls_certificates()[0].private_key().filename()), | ||
| certificate_revocation_list_(config.validation_context().crl().filename()), |
There was a problem hiding this comment.
The Datasource could either provide a filename or inline data. Please handle both. Instead of the ContextConfigImpl returning a path, it should just return the data. If the source is a file, read it here in the constructor. Then this class can probably hold a bssl::UniquePtr<X509_CRL> instead of std::string, and any invalid-data-errors will be caught sooner.
There was a problem hiding this comment.
Hum. I'll admit that I used Datasource because trusted_ca was one, but it appears that field doesn't support non-file data either (see here). I'm happy to add support for inline data here, if you'd like, but do you think it's worth dropping support for inline data entirely?
There was a problem hiding this comment.
Yeah, I think the reason that many of the fields don't support inline data yet is that the boringssl API makes it really easy to just give it a file path and never touch the file (eg SSL_CTX_use_PrivateKey_file()). In this case, since you're doing the file open manually, it seems less clear.
But I think at some point, a separate cleanup pass will be needed to make everything support inline data. If you want to defer fixing this until then, I'm ok with that.
But if you do defer it, please rename the variables/functions to be clear that they hold a path/filename (eg private_key_file_/privateKeyFile()).
There was a problem hiding this comment.
I think @htuch wrote the original code that didn't handle inline data. Any comment on if/when to handle this?
There was a problem hiding this comment.
Either way, I just pushed a commit that should parse the CRL up-front, and uses a memory BIO to handle inline data. I'm not super happy with it, since it involves exposing the bssl::UniquePtr<X509_CRL> in the public interface of class ContextConfig.
I'll write tests next!
… support for inline data Signed-off-by: Andrew Dunham <adunham@stripe.com>
|
@adunham-stripe friendly ping. Are you planning in picking this up or should we close for now? |
|
going to go ahead and close this for now. We can reopen when we want to work on it again. |
|
@mattklein123 @ggreenway - Apologies for dropping off the face of the earth there! I've pushed a bunch of commits that implement this feature, add test data, and add tests. I decided not to add support for multiple CRL files, and instead added the ability to load multiple CRLs from the same file (mimicking the way that nginx handles this). The only remaining question I have is whether it seems reasonable to set the Let me know if this works for you folks, or what else you'd like to see! |
|
@adunham-stripe unfortunately GH UI is not letting me reopen this PR. Sorry. :( Do you mind opening a fresh PR and just refer back to this one? |
|
Done! #2255 |
Part of #2077 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Description: This PR allows platform implementations of a generic (and potentially persistent) key-value store to be registered with the Engine. Internal components can look up these implementations by name. The internal implementation and wiring in this PR is platform agnostic, but this PR adds a public API for Java/Kotlin only, initially. iOS and other public APIs to follow in upcoming PRs. Part of #2077. Risk Level: Low Testing: New Unit + Integration Coverage Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
Part of #2077 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Description: This PR allows platform implementations of a generic (and potentially persistent) key-value store to be registered with the Engine. Internal components can look up these implementations by name. The internal implementation and wiring in this PR is platform agnostic, but this PR adds a public API for Java/Kotlin only, initially. iOS and other public APIs to follow in upcoming PRs. Part of #2077. Risk Level: Low Testing: New Unit + Integration Coverage Signed-off-by: Mike Schore <mike.schore@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>
3 participants
Description:
Adds support for CRLs. Fixes issue #1496
API Changes:
See Data Plane PR: envoyproxy/data-plane-api#249
Risk Level: Medium (TLS is scary!)
Testing:
I could use some help figuring out how to test this; I'm still pretty new to the Envoy codebase, so pointers are appreciated!