tls: support BoringSSL private key async functionality #6326
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -147,6 +147,11 @@ message TlsSessionTicketKeys { | |
| repeated core.DataSource keys = 1 [(validate.rules).repeated .min_items = 1]; | ||
| } | ||
|
|
||
| message PrivateKeyOperations { | ||
| string private_key_provider = 1; | ||
| core.ConfigSource private_key_provider_config = 2; | ||
|
There was a problem hiding this comment. Starting from first principles on this review (just noticed it when you tagged me). How come we need to use There was a problem hiding this comment. That makes sense. I added a patch which does just that. I was bit unsure if the plan is to use the combined Struct/Any approach for new extension types, or just go with Any. I'm currently thinking that the configuration conversion would be done in the private key provider extensions something like this: Ssl::PrivateKeyOperationsProviderSharedPtr TestPrivateKeyOperationsFactory::createPrivateKeyOperationsProviderInstance(
const envoy::api::v2::auth::PrivateKeyOperations& message,
Server::Configuration::TransportSocketFactoryContext& private_key_provider_context) {
ProtobufTypes::MessagePtr proto_config = std::make_unique<test::TestPrivateKeyOperationsProto>();
Config::Utility::translateOpaqueConfig(message.typed_config(), ProtobufWkt::Struct(), *proto_config);
const test::TestPrivateKeyOperationsProto config = MessageUtil::downcastAndValidate<const test::TestPrivateKeyOperationsProto&>(*proto_config);
return std::make_shared<TestPrivateKeyOperationsProvider>(config);
}There was a problem hiding this comment. Yeah, you can factor some of this out into an extension factory base, you can see what was done for L7 filters at https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/common/factory_base.h. |
||
| } | ||
|
|
||
| message CertificateValidationContext { | ||
| // TLS certificate data containing certificate authority certificates to use in verifying | ||
| // a presented peer certificate (e.g. server certificate for clusters or client certificate | ||
|
|
@@ -315,6 +320,8 @@ message CommonTlsContext { | |
| repeated string alpn_protocols = 4; | ||
|
|
||
| reserved 5; | ||
|
|
||
| PrivateKeyOperations private_key_operations = 9; | ||
|
There was a problem hiding this comment. This seems effectively a There was a problem hiding this comment. Yes, this should definitely be a part of Ideally, this would be a Thoughts? (Using There was a problem hiding this comment. We can't do There was a problem hiding this comment. I was under the impression that you can add There was a problem hiding this comment. Wire compatible yes, but it breaks Go language bindings. We've had a request from the gRPC team to avoid doing this gratuitously, this seems a reasonable accommodation to make. See #6271. There was a problem hiding this comment. Ah, I wasn't aware of that, thanks! Yes, it makes sense to accommodate them (though, it's unfortunate that language bindings are source of this restriction). There was a problem hiding this comment. Ok, I'll move the private key operations provider to TlsCertificate. I'll have to set up the SSL socket bit differentlythen (because I'll need to initialize the private key methods for each |
||
| } | ||
|
|
||
| message UpstreamTlsContext { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/ssl/certificate_validation_context_config.h" | ||
| #include "envoy/ssl/private_key/private_key.h" | ||
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| namespace Envoy { | ||
|
|
@@ -69,6 +70,12 @@ class ContextConfig { | |
| * @param callback callback that is executed by context config. | ||
| */ | ||
| virtual void setSecretUpdateCallback(std::function<void()> callback) PURE; | ||
|
|
||
| /** | ||
| * @return the configured BoringSSL private key operations provider. | ||
|
There was a problem hiding this comment. Do you have any thoughts on how this can work for OpenSSL implementations? There was a problem hiding this comment. It doesn't - private key operations are a BoringSSL concept. OpenSSL uses a bit similar mechanism called asynchronous mode, which makes almost all operations potentially asynchronous. It's up to the backend (an OpenSSL engine) to implement the operations in an asynchronous way. OpenSSL provides a file descriptor to tell when the operation is ready. The benefit of OpenSSL model is that many more operations can be performed asynchronously and also the possibility to have dynamic shared objects (engines) with a specified ABI to run the asynchronous operations. Also, if an asynchronous engine isn't used, the library falls back to synchronous operation. The drawback is that the OpenSSL library needs to be dynamically linked to Envoy in order to load external engines. Also, the I wrote a patch on top of @bdecoste's OpenSSL Envoy patches some time back to enable the OpenSSL asynchronous mode: There was a problem hiding this comment. Is there any hope of unifying these behind a common Envoy abstraction? That would be ideal, but obviously not if it needs to be a mega-project to make happen. Thoughts? There was a problem hiding this comment. It's my understanding that the BoringSSL and OpenSLL support will be implemented as separate TLS extensions. This means that the way the library is used will stay "hidden" in the implementations. I think the abstraction could be done in two different possible scopes:
The problem with the first item is that it's already pretty well abstracted: there is a bunch of OpenSSL engines out there, and the benefit of Envoy knowing about them would be mainly to pass them configuration values. Envoy can use the dynamic engines quite easily (the only requirement is linking the OpenSSL library dynamically into Envoy), and there is The second item would mean in a sense that there would be feature parity between OpenSSL and BoringSSL implementations regarding private key operations. The OpenSSL would be pretty handicapped in this case, since the async engine support is more complete than BoringSSL private key operations. Using the QAT HW acceleration as an example, OpenSSL QAT engine (https://github.com/intel/QAT_Engine) can accelerate also EDCHE and some crypto operations, while the BoringSSL private key support is limited to RSA decryption and RSA+ECDSA signing. I think people would gravitate using the full OpenSSL engines rather than writing the Envoy-specific OpenSSL "mini-engines". TL;DR: it feels to me that having a common abstraction for OpenSSL engines and BoringSSL private key operations might not have enough benefits compared to letting the TLS extensions set up the asynchronous operations as they please. However, I've had only limited exposure to OpenSSL engines, so it could be there are other considerations that I'm not aware of. Maybe there would be a special Envoy configuration Protobuf type for the TLS extensions, so that they could be configured separately? This could mean OpenSSL protobuf configuration message having a submessage for setting up the requested OpenSSL engine(s). There was a problem hiding this comment. @ipuustin thanks for the analysis. My thinking was that if X wanted to write a new private key offload extension Y for X's HSM, then it would be nice to have Y work no matter whether you go with BoringSSL or OpenSSL TLS. If there are already a number of OpenSSL engines out there that only require a light wrapping for Envoy, and OpenSSL is strictly more expressive than BoringSSL, I think there's not much hope or point of reuse. So, I think this is resolved, we go with your current approach. Thanks again for taking the time to provide such a detailed discussion. |
||
| */ | ||
| virtual const Envoy::Ssl::PrivateKeyOperationsProviderSharedPtr | ||
| privateKeyOperationsProvider() const PURE; | ||
| }; | ||
|
|
||
| class ClientContextConfig : public virtual ContextConfig { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "private_key_interface", | ||
| hdrs = ["private_key.h"], | ||
| external_deps = ["ssl"], | ||
| deps = [ | ||
| ":private_key_callbacks_interface", | ||
| "//include/envoy/event:dispatcher_interface", | ||
| "@envoy_api//envoy/api/v2/core:config_source_cc", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "private_key_config_interface", | ||
| hdrs = ["private_key_config.h"], | ||
| deps = [ | ||
| ":private_key_interface", | ||
| "//include/envoy/registry", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "private_key_callbacks_interface", | ||
| hdrs = ["private_key_callbacks.h"], | ||
| external_deps = ["ssl"], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| #pragma once | ||
|
|
||
| #include <functional> | ||
| #include <string> | ||
|
|
||
| #include "envoy/api/v2/core/config_source.pb.h" | ||
| #include "envoy/common/pure.h" | ||
| #include "envoy/event/dispatcher.h" | ||
| #include "envoy/ssl/private_key/private_key_callbacks.h" | ||
|
|
||
| #include "openssl/ssl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Server { | ||
| namespace Configuration { | ||
| class TransportSocketFactoryContext; | ||
|
There was a problem hiding this comment. Is this to break an include dependency loop? There was a problem hiding this comment. Yes, that's why this is needed -- Bazel will complain if this is done with a real dependency. There was a problem hiding this comment. Arguably we could do some header refactoring to fix this, but I think this is OK if this is a one off. There was a problem hiding this comment. Ok. Just for the record, the dep loop would be this: |
||
| } // namespace Configuration | ||
| } // namespace Server | ||
|
|
||
| namespace Ssl { | ||
|
|
||
| typedef std::shared_ptr<SSL_PRIVATE_KEY_METHOD> PrivateKeyMethodSharedPtr; | ||
|
|
||
| class PrivateKeyOperations { | ||
| public: | ||
| virtual ~PrivateKeyOperations() {} | ||
|
There was a problem hiding this comment.
|
||
|
|
||
| /** | ||
| * Get the private key asynchronous methods from the provider, if the context can be handled by | ||
| * the provider. | ||
| * @param ssl a SSL connection object. | ||
| * @return private key methods, or nullptr if regular TLS processing should happen. | ||
| */ | ||
| virtual PrivateKeyMethodSharedPtr getPrivateKeyMethods(SSL* ssl) PURE; | ||
| }; | ||
|
|
||
| typedef std::unique_ptr<PrivateKeyOperations> PrivateKeyOperationsPtr; | ||
|
|
||
| class PrivateKeyOperationsProvider { | ||
| public: | ||
| virtual ~PrivateKeyOperationsProvider() {} | ||
|
|
||
| /** | ||
| * Get a private key operations instance from the provider. | ||
| * @param cb a callbacks object, whose "complete" method will be invoked when the asynchronous | ||
| * processing is complete. | ||
| * @param dispatcher supplies the owning thread's dispatcher. | ||
| * @return the private key operations. | ||
| */ | ||
| virtual PrivateKeyOperationsPtr getPrivateKeyOperations(PrivateKeyOperationsCallbacks& cb, | ||
| Event::Dispatcher& dispatcher) PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<PrivateKeyOperationsProvider> PrivateKeyOperationsProviderSharedPtr; | ||
|
|
||
| /** | ||
| * A manager for finding correct user-provided functions for handling BoringSSL private key | ||
| * operations. | ||
| */ | ||
| class PrivateKeyOperationsManager { | ||
| public: | ||
| virtual ~PrivateKeyOperationsManager() {} | ||
|
|
||
| /** | ||
| * Finds and returns a private key operations provider for BoringSSL. | ||
| * | ||
| * @param config_source a protobuf message object containing a TLS config source. | ||
| * @param config_name a name that uniquely refers to the private key operations provider. | ||
| * @param private_key_provider_context context that provides components for creating and | ||
| * initializing connections for keyless TLS etc. | ||
|
There was a problem hiding this comment. Nit: |
||
| * @return TlsPrivateKeyOperationsProvider the private key operations provider, or nullptr if | ||
| * no provider can be used with the context configuration. | ||
| */ | ||
| virtual PrivateKeyOperationsProviderSharedPtr createPrivateKeyOperationsProvider( | ||
| const envoy::api::v2::core::ConfigSource& config_source, const std::string& config_name, | ||
| Server::Configuration::TransportSocketFactoryContext& private_key_provider_context) PURE; | ||
| }; | ||
|
|
||
| // typedef std::shared_ptr<PrivateKeyOperationsManager> PrivateKeyOperationsManagerSharedPtr; | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| #pragma once | ||
|
|
||
| #include <functional> | ||
| #include <string> | ||
|
|
||
| #include "envoy/common/pure.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Ssl { | ||
|
|
||
| enum class PrivateKeyOperationStatus { | ||
| Success, | ||
| Failure, | ||
| }; | ||
|
|
||
| class PrivateKeyOperationsCallbacks { | ||
| public: | ||
| virtual ~PrivateKeyOperationsCallbacks() {} | ||
|
|
||
| /** | ||
| * Callback function which is called when the asynchronous private key | ||
| * operation has been completed. | ||
| * @param status is "Success" or "Failure" depending on whether the private key operation was | ||
| * successful or not. | ||
| */ | ||
| virtual void complete(PrivateKeyOperationStatus status) PURE; | ||
|
There was a problem hiding this comment. I'm a bit worried about the case where the connection (and Do we have any protections against that? There was a problem hiding this comment. Originally the idea was that the |
||
| }; | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/api/v2/core/config_source.pb.h" | ||
| #include "envoy/registry/registry.h" | ||
| #include "envoy/ssl/private_key/private_key.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Ssl { | ||
|
|
||
| // Base class which the private key operation provider implementations can register. | ||
|
|
||
| class PrivateKeyOperationsProviderInstanceFactory { | ||
| public: | ||
| virtual ~PrivateKeyOperationsProviderInstanceFactory() {} | ||
| virtual PrivateKeyOperationsProviderSharedPtr createPrivateKeyOperationsProviderInstance( | ||
| const std::string name, const envoy::api::v2::core::ConfigSource& config_source, | ||
| Server::Configuration::TransportSocketFactoryContext& private_key_provider_context) PURE; | ||
| virtual std::string name() const PURE; | ||
| }; | ||
|
|
||
| } // namespace Ssl | ||
| } // namespace Envoy |
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.
Are these going to be BoringSSL specific or an opaque placeholder for both (incompatible) types of private key operations (OpenSSL engines and BoringSSL)?
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.
OpenSSL engines are much more than just private key operations, and I don't think treating them as alternatives is going to result in a clean API.