-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: support BoringSSL private key async functionality #6326
Changes from 4 commits
3ab2e1d
84b6fe1
127bff5
e58ddfd
8d91d59
0bd6f25
5e01b30
8717cb2
7f0e0bc
4855c9e
b25f6a6
b7f4fa3
f67c1a1
b24ceb2
ce312ec
3eacd75
81afc58
ccc8fbe
3e8a3c3
59d93f6
eb1feaa
f7ccf0b
dace6bb
1c5630d
c92dfd6
86ecba9
3cbe3a5
976a469
789368a
50d6450
9146b40
8a688f6
455be73
14b6d33
cc54803
12680f6
16ca2a1
6d5cf6f
e3a66fe
6ac9b30
caee5ad
1f1c997
2ef535f
c4a5836
2498b0f
cd28e08
d7e9e5a
9b31b1a
c9ec2bc
9a321eb
7ec959e
f35cca5
0a51246
808d046
3f57152
ed99c59
4d1ceca
e4cab89
5db871c
e0e4ed7
d28d44b
b79e532
abf8758
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 |
---|---|---|
|
@@ -10,6 +10,7 @@ option go_package = "auth"; | |
import "envoy/api/v2/core/base.proto"; | ||
import "envoy/api/v2/core/config_source.proto"; | ||
|
||
import "google/protobuf/any.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
import "validate/validate.proto"; | ||
|
@@ -147,6 +148,15 @@ message TlsSessionTicketKeys { | |
repeated core.DataSource keys = 1 [(validate.rules).repeated .min_items = 1]; | ||
} | ||
|
||
message PrivateKeyOperations { | ||
// Private key operations provider name. The name must match a | ||
// supported private key operations provider type. | ||
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. Typically we are making these names reverse DNS, e.g. |
||
string provider_name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// Private key operations provider specific configuration. | ||
google.protobuf.Any typed_config = 2; | ||
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. @lizan should we continue to require 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 think the answer to this thread is we should support both, since |
||
} | ||
|
||
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 +325,8 @@ message CommonTlsContext { | |
repeated string alpn_protocols = 4; | ||
|
||
reserved 5; | ||
|
||
PrivateKeyOperations private_key_operations = 9; | ||
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. This seems effectively a 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. Yes, this should definitely be a part of Ideally, this would be a Thoughts? (Using 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. We can't do 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 was under the impression that you can add 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you have any thoughts on how this can work for OpenSSL implementations? 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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/auth:cert_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/auth/cert.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this to break an include dependency loop? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Ok. |
||
|
||
/** | ||
* 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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::auth::PrivateKeyOperations& message, | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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/auth/cert.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 envoy::api::v2::auth::PrivateKeyOperations& message, | ||
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.