-
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 6 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,8 @@ 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/struct.proto"; | ||
import "google/protobuf/wrappers.proto"; | ||
|
||
import "validate/validate.proto"; | ||
|
@@ -102,6 +104,19 @@ message TlsParameters { | |
repeated string ecdh_curves = 4; | ||
} | ||
|
||
message PrivateKeyMethod { | ||
// Private key operations provider name. The name must match a | ||
// supported private key operations provider type. | ||
string provider_name = 1 [(validate.rules).string.min_bytes = 1]; | ||
|
||
// Private key operations provider specific configuration. | ||
oneof config_type { | ||
google.protobuf.Struct config = 2; | ||
|
||
google.protobuf.Any typed_config = 4; | ||
} | ||
} | ||
|
||
message TlsCertificate { | ||
// The TLS certificate chain. | ||
core.DataSource certificate_chain = 1; | ||
|
@@ -118,6 +133,10 @@ message TlsCertificate { | |
|
||
// [#not-implemented-hide:] | ||
repeated core.DataSource signed_certificate_timestamp = 5; | ||
|
||
// BoringSSL private key operations. This is an alternative to private_key field. This can't be | ||
// marked as "oneof" due to API compatibility reasons. | ||
PrivateKeyMethod private_key_method = 6; | ||
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: could you move it right after 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. |
||
} | ||
|
||
message TlsSessionTicketKeys { | ||
|
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,91 @@ | ||
#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 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. |
||
|
||
/** | ||
* Associate the private key operations instance with a SSL connection. | ||
* @param ssl a SSL connection object. The BoringSSL private key API | ||
* doesn't allow passing user data to the asynchronous functions as a | ||
* function parameter, so this enables the private key method provider | ||
* to use SSL connection custom data fields instead. | ||
*/ | ||
virtual bool associateWithSsl(SSL* ssl) 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. Why isn't this part of 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 removed the |
||
}; | ||
|
||
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 instance. | ||
*/ | ||
virtual PrivateKeyOperationsPtr getPrivateKeyOperations(SSL* ssl, | ||
PrivateKeyOperationsCallbacks& 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<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 message a protobuf message object containing a | ||
* PrivateKeyMethod message. | ||
* @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 PrivateKeyOperationsProvider 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::PrivateKeyMethod& message, | ||
Envoy::Server::Configuration::TransportSocketFactoryContext& | ||
private_key_provider_context) PURE; | ||
}; | ||
|
||
} // 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::PrivateKeyMethod& message, | ||
Server::Configuration::TransportSocketFactoryContext& private_key_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,33 @@ 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()) { | ||
private_key_method_ = | ||
factory_context.sslContextManager() | ||
.privateKeyOperationsManager() | ||
.createPrivateKeyOperationsProvider(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.
Why is the numbering here non-contiguous?
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.
It's a copy-paste error from the move to cert. I'll fix this.