-
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 21 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 |
---|---|---|
@@ -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,83 @@ | ||
#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 { | ||
// Prevent a dependency loop with the forward declaration. | ||
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> BoringSslPrivateKeyMethodSharedPtr; | ||
|
||
class PrivateKeyConnection { | ||
public: | ||
virtual ~PrivateKeyConnection() {} | ||
}; | ||
|
||
typedef std::unique_ptr<PrivateKeyConnection> PrivateKeyConnectionPtr; | ||
|
||
class PrivateKeyMethodProvider { | ||
public: | ||
virtual ~PrivateKeyMethodProvider() {} | ||
|
||
/** | ||
* Get a private key operations instance from the provider. | ||
* @param ssl a SSL connection object. | ||
* @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 instance. | ||
*/ | ||
virtual PrivateKeyConnectionPtr getPrivateKeyConnection(SSL* ssl, | ||
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. Why is this called 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. The idea was that this part corresponds to a single connection. I agree the naming isn't very good here. I'll try to come up with a better name. 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've been looking at this for a while, but I cannot figure out why do we need 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. The functionality could be contained in the provider. I remember thinking about that approach too, but this seemed bit more straightforward because of the memory management (it's not possible that the callback comes after the socket was destroyed) and easier thinking about thread data access in the provider. If we remove the // throws an error if the registration fails
void registerPrivateKeyConnection(SSL* ssl, PrivateKeyConnectionCallbacks& cb, Event::Dispatcher& dispatcher) PURE;
void unregisterPrivateKeyConnection(SSL* ssl) PURE; Internally the provider would need to keep track of registered SSL objects (which it just borrows from the SSL sockets). A benefit of this approach would be avoiding of the naming issue. :-) I think I'll experiment a bit with this to get a better idea how it will work. 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 that's true (but I agree that the current model would be acceptable if it was). In a security-oriented use case, the private key will be loaded in a separate process, or even on a separate host, which means that you can receive result of the computation long after the socket was destroyed. |
||
PrivateKeyConnectionCallbacks& cb, | ||
Event::Dispatcher& dispatcher) PURE; | ||
|
||
/** | ||
* Get the private key methods from the provider. | ||
* @return the private key methods associated with this provider and | ||
* configuration. | ||
*/ | ||
virtual BoringSslPrivateKeyMethodSharedPtr getBoringSslPrivateKeyMethod() PURE; | ||
}; | ||
|
||
typedef std::shared_ptr<PrivateKeyMethodProvider> PrivateKeyMethodProviderSharedPtr; | ||
|
||
/** | ||
* A manager for finding correct user-provided functions for handling BoringSSL private key | ||
* operations. | ||
*/ | ||
class PrivateKeyMethodManager { | ||
public: | ||
virtual ~PrivateKeyMethodManager() {} | ||
|
||
/** | ||
* Finds and returns a private key operations provider for BoringSSL. | ||
* | ||
* @param message a protobuf message object containing a | ||
* PrivateKeyMethod message. | ||
* @param private_key_method_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 PrivateKeyMethodProvider the private key operations provider, or nullptr if | ||
* no provider can be used with the context configuration. | ||
*/ | ||
virtual PrivateKeyMethodProviderSharedPtr | ||
createPrivateKeyMethodProvider(const envoy::api::v2::auth::PrivateKeyMethod& message, | ||
Envoy::Server::Configuration::TransportSocketFactoryContext& | ||
private_key_method_provider_context) 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. Nit: |
||
}; | ||
|
||
} // namespace Ssl | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
#pragma once | ||
|
||
#include <functional> | ||
#include <string> | ||
|
||
#include "envoy/common/pure.h" | ||
|
||
namespace Envoy { | ||
namespace Ssl { | ||
|
||
class PrivateKeyConnectionCallbacks { | ||
public: | ||
virtual ~PrivateKeyConnectionCallbacks() {} | ||
|
||
/** | ||
* Callback function which is called when the asynchronous private key | ||
* operation has been completed (with either success or failure). The | ||
* provider will communicate the success status when SSL_do_handshake() | ||
* is called the next time. | ||
*/ | ||
virtual void complete() PURE; | ||
}; | ||
|
||
} // namespace Ssl | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
#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 PrivateKeyMethodProviderInstanceFactory { | ||
public: | ||
virtual ~PrivateKeyMethodProviderInstanceFactory() {} | ||
virtual PrivateKeyMethodProviderSharedPtr | ||
createPrivateKeyMethodProviderInstance(const envoy::api::v2::auth::PrivateKeyMethod& message, | ||
Server::Configuration::TransportSocketFactoryContext& | ||
private_key_method_provider_context) PURE; | ||
virtual std::string name() const PURE; | ||
}; | ||
|
||
} // namespace Ssl | ||
} // namespace Envoy |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
#include "common/ssl/tls_certificate_config_impl.h" | ||
|
||
#include "envoy/common/exception.h" | ||
#include "envoy/server/transport_socket_config.h" | ||
|
||
#include "common/common/empty_string.h" | ||
#include "common/common/fmt.h" | ||
|
@@ -22,9 +23,37 @@ TlsCertificateConfigImpl::TlsCertificateConfigImpl( | |
.value_or(private_key_.empty() ? EMPTY_STRING : INLINE_STRING)), | ||
password_(Config::DataSource::read(config.password(), true, api)), | ||
password_path_(Config::DataSource::getPath(config.password()) | ||
.value_or(password_.empty() ? EMPTY_STRING : INLINE_STRING)) { | ||
.value_or(password_.empty() ? EMPTY_STRING : INLINE_STRING)) {} | ||
|
||
if (certificate_chain_.empty() || private_key_.empty()) { | ||
TlsCertificateConfigImpl::TlsCertificateConfigImpl( | ||
const envoy::api::v2::auth::TlsCertificate& config, Api::Api& api, | ||
bool expect_private_key_method) | ||
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 we fit this into one or two constructors? 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. The reason for the private constructor was an attempt to avoid copying the initialization code into the two public constructors. When the constructor is called from SDS initialization, the error handling logic goes bit differently, because there is no possibility to set the private key method (the But you're right that the logic is complex. I'll look if I can make it simpler. |
||
: TlsCertificateConfigImpl(config, api) { | ||
{ | ||
if (!expect_private_key_method) { | ||
if (certificate_chain_.empty() || private_key_.empty()) { | ||
throw EnvoyException(fmt::format("Failed to load incomplete certificate from {}, {}", | ||
certificate_chain_path_, private_key_path_)); | ||
} | ||
} | ||
} | ||
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 the constructor above be reduced to this?
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 true that if we convert the |
||
} | ||
|
||
TlsCertificateConfigImpl::TlsCertificateConfigImpl( | ||
const envoy::api::v2::auth::TlsCertificate& config, | ||
Server::Configuration::TransportSocketFactoryContext& factory_context, Api::Api& api) | ||
: TlsCertificateConfigImpl(config, api, true) { | ||
if (config.has_private_key_method()) { | ||
if (config.has_private_key()) { | ||
throw EnvoyException(fmt::format( | ||
"Certificate configuration can't have both private_key and private_key_method")); | ||
} | ||
private_key_method_ = | ||
factory_context.sslContextManager() | ||
.privateKeyMethodManager() | ||
.createPrivateKeyMethodProvider(config.private_key_method(), factory_context); | ||
} | ||
if (certificate_chain_.empty() || (private_key_.empty() && private_key_method_ == nullptr)) { | ||
throw EnvoyException(fmt::format("Failed to load incomplete certificate from {}, {}", | ||
certificate_chain_path_, private_key_path_)); | ||
} | ||
|
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.
Nit: could you move it right after
private_key
? We don't necessarily need to add things at the end, and I believe that auto-generated docs would benefit from those 2 fields being next to each other.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.
Ok.