-
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
ssl: add support for SNI. #1984
Changes from all commits
76709a0
d04afe2
0f984ce
47e3df7
227589b
75abf26
0fecd6b
68541bf
33142cc
ca9eb0b
7d1b1af
fc9705f
a2f657c
0dc9b85
348e6e8
7ce1bbb
1733d92
909ada7
1dbe7af
00bf76d
5c6e262
ce8ccc6
0a25690
c29cd5b
37e083d
7f18d00
80be681
12e1260
3f21609
c11713e
8dc1702
fa77d69
eb3ff75
afc3b62
cd854a2
aa5098b
da1dbaa
4ec1786
33d27e7
757abeb
5a2fc2a
ea6660a
dd3cf76
f0a48d4
ddbaf41
6901dc0
f519595
a43e74c
5700d0c
57ed156
5520293
4ac9fdf
122f450
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 |
---|---|---|
|
@@ -24,19 +24,30 @@ class ContextManager { | |
|
||
/** | ||
* Builds a ServerContext from a ServerContextConfig. | ||
* The skip_context_update parameter is used for fast-path (avoiding lock & context lookup) | ||
* on listeners with a single filter chain and no SNI restrictions. | ||
*/ | ||
virtual ServerContextPtr createSslServerContext(Stats::Scope& scope, | ||
ServerContextConfig& config) PURE; | ||
virtual ServerContextPtr createSslServerContext(const std::string& listener_name, | ||
const std::vector<std::string>& server_names, | ||
Stats::Scope& scope, ServerContextConfig& config, | ||
bool skip_context_update) PURE; | ||
|
||
/** | ||
* Find ServerContext for a given listener and server_name. | ||
* @return ServerContext or nullptr in case there is no match. | ||
*/ | ||
virtual ServerContext* findSslServerContext(const std::string& listener_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. This may be out of scope for this change, but should the ServerContextImpl has a reference to it's ConnectionHandler, then this function could be on ConnectionHandler and not need a listener_name param? This feels awkward because the ServerContext already exists under a listener/ConnectionHandler, so needing a separate listener lookup is strange. 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, agreed. This is related to my comments about making this lockless and adding TODO. I'm willing live with this to get this shipped, but ultimately don't like the global namespace for this. Really we want listener -> set of contexts -> global context manager. Will let the two of you sort it out. 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 and no. While this indeed could be done much better for this specific use case, we're going to add SDS shortly and then it's going to get a bit more complex. Personally, I'd defer any optimizations and/or refactoring until SDS is committed. 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 ok with that. |
||
const std::string& server_name) const PURE; | ||
|
||
/** | ||
* @return the number of days until the next certificate being managed will expire. | ||
*/ | ||
virtual size_t daysUntilFirstCertExpires() PURE; | ||
virtual size_t daysUntilFirstCertExpires() const PURE; | ||
|
||
/** | ||
* Iterate through all currently allocated contexts. | ||
*/ | ||
virtual void iterateContexts(std::function<void(Context&)> callback) PURE; | ||
virtual void iterateContexts(std::function<void(const Context&)> callback) PURE; | ||
}; | ||
|
||
} // namespace Ssl | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,8 +28,8 @@ int ContextImpl::sslContextIndex() { | |
} | ||
|
||
ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, ContextConfig& config) | ||
: parent_(parent), ctx_(SSL_CTX_new(TLS_method())), scope_(scope), | ||
stats_(generateStats(scope)) { | ||
: parent_(parent), ctx_(SSL_CTX_new(TLS_method())), scope_(scope), stats_(generateStats(scope)), | ||
ecdh_curves_(config.ecdhCurves()) { | ||
RELEASE_ASSERT(ctx_); | ||
|
||
int rc = SSL_CTX_set_ex_data(ctx_.get(), sslContextIndex(), this); | ||
|
@@ -40,8 +40,8 @@ ContextImpl::ContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, Contex | |
fmt::format("Failed to initialize cipher suites {}", config.cipherSuites())); | ||
} | ||
|
||
if (!SSL_CTX_set1_curves_list(ctx_.get(), config.ecdhCurves().c_str())) { | ||
throw EnvoyException(fmt::format("Failed to initialize ECDH curves {}", config.ecdhCurves())); | ||
if (!SSL_CTX_set1_curves_list(ctx_.get(), ecdh_curves_.c_str())) { | ||
throw EnvoyException(fmt::format("Failed to initialize ECDH curves {}", ecdh_curves_)); | ||
} | ||
|
||
int verify_mode = SSL_VERIFY_NONE; | ||
|
@@ -266,7 +266,7 @@ SslStats ContextImpl::generateStats(Stats::Scope& store) { | |
POOL_HISTOGRAM_PREFIX(store, prefix))}; | ||
} | ||
|
||
size_t ContextImpl::daysUntilFirstCertExpires() { | ||
size_t ContextImpl::daysUntilFirstCertExpires() const { | ||
int daysUntilExpiration = getDaysUntilExpiration(ca_cert_.get()); | ||
daysUntilExpiration = | ||
std::min<int>(getDaysUntilExpiration(cert_chain_.get()), daysUntilExpiration); | ||
|
@@ -276,7 +276,7 @@ size_t ContextImpl::daysUntilFirstCertExpires() { | |
return daysUntilExpiration; | ||
} | ||
|
||
int32_t ContextImpl::getDaysUntilExpiration(const X509* cert) { | ||
int32_t ContextImpl::getDaysUntilExpiration(const X509* cert) const { | ||
if (cert == nullptr) { | ||
return std::numeric_limits<int>::max(); | ||
} | ||
|
@@ -287,7 +287,7 @@ int32_t ContextImpl::getDaysUntilExpiration(const X509* cert) { | |
return 0; | ||
} | ||
|
||
std::string ContextImpl::getCaCertInformation() { | ||
std::string ContextImpl::getCaCertInformation() const { | ||
if (ca_cert_ == nullptr) { | ||
return ""; | ||
} | ||
|
@@ -296,7 +296,7 @@ std::string ContextImpl::getCaCertInformation() { | |
getDaysUntilExpiration(ca_cert_.get())); | ||
} | ||
|
||
std::string ContextImpl::getCertChainInformation() { | ||
std::string ContextImpl::getCertChainInformation() const { | ||
if (cert_chain_ == nullptr) { | ||
return ""; | ||
} | ||
|
@@ -305,9 +305,9 @@ std::string ContextImpl::getCertChainInformation() { | |
getDaysUntilExpiration(cert_chain_.get())); | ||
} | ||
|
||
std::string ContextImpl::getSerialNumber(X509* cert) { | ||
std::string ContextImpl::getSerialNumber(const X509* cert) { | ||
ASSERT(cert); | ||
ASN1_INTEGER* serial_number = X509_get_serialNumber(cert); | ||
ASN1_INTEGER* serial_number = X509_get_serialNumber(const_cast<X509*>(cert)); | ||
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. openssl sigh :) |
||
BIGNUM num_bn; | ||
BN_init(&num_bn); | ||
ASN1_INTEGER_to_BN(serial_number, &num_bn); | ||
|
@@ -355,10 +355,20 @@ bssl::UniquePtr<SSL> ClientContextImpl::newSsl() const { | |
return ssl_con; | ||
} | ||
|
||
ServerContextImpl::ServerContextImpl(ContextManagerImpl& parent, Stats::Scope& scope, | ||
ServerContextConfig& config, Runtime::Loader& runtime) | ||
: ContextImpl(parent, scope, config), runtime_(runtime), | ||
ServerContextImpl::ServerContextImpl(ContextManagerImpl& parent, const std::string& listener_name, | ||
const std::vector<std::string>& server_names, | ||
Stats::Scope& scope, ServerContextConfig& config, | ||
bool skip_context_update, Runtime::Loader& runtime) | ||
: ContextImpl(parent, scope, config), listener_name_(listener_name), | ||
server_names_(server_names), skip_context_update_(skip_context_update), runtime_(runtime), | ||
session_ticket_keys_(config.sessionTicketKeys()) { | ||
SSL_CTX_set_select_certificate_cb( | ||
ctx_.get(), [](const SSL_CLIENT_HELLO* client_hello) -> ssl_select_cert_result_t { | ||
ContextImpl* context_impl = static_cast<ContextImpl*>( | ||
SSL_CTX_get_ex_data(SSL_get_SSL_CTX(client_hello->ssl), sslContextIndex())); | ||
return dynamic_cast<ServerContextImpl*>(context_impl)->processClientHello(client_hello); | ||
}); | ||
|
||
if (!config.caCertFile().empty()) { | ||
bssl::UniquePtr<STACK_OF(X509_NAME)> list(SSL_load_client_CA_file(config.caCertFile().c_str())); | ||
if (nullptr == list) { | ||
|
@@ -404,7 +414,7 @@ ServerContextImpl::ServerContextImpl(ContextManagerImpl& parent, Stats::Scope& s | |
int rc = EVP_DigestInit(&md, EVP_sha256()); | ||
RELEASE_ASSERT(rc == 1); | ||
|
||
// Hash the CommonName/SANs in the server certificate. This makes sure that | ||
// Hash the CommonName/SANs of the server certificate. This makes sure that | ||
// sessions can only be resumed to a certificate for the same name, but allows | ||
// resuming to unique certs in the case that different Envoy instances each have | ||
// their own certs. | ||
|
@@ -469,12 +479,78 @@ ServerContextImpl::ServerContextImpl(ContextManagerImpl& parent, Stats::Scope& s | |
RELEASE_ASSERT(rc == 1); | ||
} | ||
|
||
// Hash configured SNIs for this context, so that sessions cannot be resumed across different | ||
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 totally follow this. Should this be "so that sessions cannot be resumed across listeners with different configurations?" What is the downside of allowing resumption if the SNI cert matches? 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, except Imagine certificate for 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 maybe we are using different lingo or I'm just confused. I think by far the most common use of SNI is going to be a single filter chain with multiple certificates, where host/authority header allows the virtual host in the route table to be selected. Very few people would regularly configure a full HTTP connection manager per SNI host I think. AFAICT the below check basically says that we don't allow resumption unless the filter chain has all the same server names that it had before. This seems wrong to me? 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. Well, but that's not what the data-plane-api allows you to do, really. Per current (and frozen) specification, cc @htuch 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 see. I think I totally missed that part of how we did the protos. I'm concerned users will find that burdensome since it will potentially require a different route/RDS table for each sever name. I now understand why you are calling this a hack w/o full filter chain matching support. My concern here is that once this code goes out we are actually providing the behavior that I originally thought people would want (multiple server names -> 1 connection manager). Can we really take it away once we give it to them? At minimum we would have to put it through the full deprecation cycle. This is a tough one. I'm wondering if it's worth it for me to help out and try to get the multiple filter chains working? 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. @PiotrSikora I talked offline with @htuch and he had the idea that to help with back-compat, we should check that all the filter chains are identical and throw if they aren't. Then in the future we can relax this. I think that will make me OK with this. I also now think I understand why the multiple filter chain stuff is complicated. You basically have to complete the handshake before you do any filter chain processing/selection. I can have a look at all of this offline and come back with some implementation notes for the future if that would be useful. 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. FWIW, I was planning to use multiple filter chains heavily. That decision isn't set in stone, but it's I was planning to model my configuration. 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. @ggreenway we are going to implement multiple filter chains, it's just a matter of when. It's pretty urgent that we get base SNI support merged so I think this approach is fine to start with as long as we future proof it. Then I will likely look at the multiple filter chain aspect after this merges. 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. sgtm |
||
// filter chains, even when using the same server certificate. | ||
for (const auto& name : server_names_) { | ||
rc = EVP_DigestUpdate(&md, name.data(), name.size()); | ||
RELEASE_ASSERT(rc == 1); | ||
} | ||
|
||
rc = EVP_DigestFinal(&md, session_context_buf, &session_context_len); | ||
RELEASE_ASSERT(rc == 1); | ||
rc = SSL_CTX_set_session_id_context(ctx_.get(), session_context_buf, session_context_len); | ||
RELEASE_ASSERT(rc == 1); | ||
} | ||
|
||
ssl_select_cert_result_t | ||
ServerContextImpl::processClientHello(const SSL_CLIENT_HELLO* client_hello) { | ||
if (skip_context_update_) { | ||
return ssl_select_cert_success; | ||
} | ||
|
||
std::string server_name; | ||
const uint8_t* data; | ||
size_t len; | ||
|
||
if (SSL_early_callback_ctx_extension_get(client_hello, TLSEXT_TYPE_server_name, &data, &len)) { | ||
// Based on BoringSSL's ext_sni_parse_clienthello(). | ||
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. Any reason we can't use the boring ssl function here? Is it worth putting in a TODO here to replace with a boringssl helper? cc @davidben 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. Few reasons:
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 SGTM. Maybe just add a TODO to circle back and upstream. 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. Done. |
||
// Match on empty SNI instead of rejecting connection in case we cannot process the extension. | ||
// TODO(PiotrSikora): figure out if we can upstream this to BoringSSL. | ||
CBS extension; | ||
CBS_init(&extension, data, len); | ||
CBS server_name_list, host_name; | ||
uint8_t name_type; | ||
if (CBS_get_u16_length_prefixed(&extension, &server_name_list) && | ||
CBS_get_u8(&server_name_list, &name_type) && | ||
CBS_get_u16_length_prefixed(&server_name_list, &host_name) && | ||
CBS_len(&server_name_list) == 0 && CBS_len(&extension) == 0 && | ||
name_type == TLSEXT_NAMETYPE_host_name && CBS_len(&host_name) != 0 && | ||
CBS_len(&host_name) <= TLSEXT_MAXLEN_host_name && !CBS_contains_zero_byte(&host_name)) { | ||
server_name.assign(reinterpret_cast<const char*>(CBS_data(&host_name)), CBS_len(&host_name)); | ||
} | ||
} | ||
|
||
ServerContext* new_ctx = parent_.findSslServerContext(listener_name_, server_name); | ||
|
||
// Reject connection if we didn't find a match. | ||
if (new_ctx == nullptr) { | ||
stats_.fail_no_sni_match_.inc(); | ||
return ssl_select_cert_error; | ||
} | ||
|
||
// Update context if it changed. | ||
if (new_ctx != this) { | ||
ServerContextImpl* new_impl = dynamic_cast<ServerContextImpl*>(new_ctx); | ||
new_impl->updateConnectionContext(client_hello->ssl); | ||
} | ||
|
||
return ssl_select_cert_success; | ||
} | ||
|
||
void ServerContextImpl::updateConnectionContext(SSL* ssl) { | ||
ASSERT(ctx_); | ||
|
||
SSL_set_SSL_CTX(ssl, ctx_.get()); | ||
ASSERT(SSL_CTX_get_ex_data(ctx_.get(), sslContextIndex()) == this); | ||
|
||
// Update SSL-level settings and parameters that are inherited from SSL_CTX during SSL_new(). | ||
SSL_set_verify(ssl, SSL_CTX_get_verify_mode(ctx_.get()), SSL_CTX_get_verify_callback(ctx_.get())); | ||
|
||
int rc = SSL_set1_curves_list(ssl, ecdh_curves_.c_str()); | ||
ASSERT(rc == 1); | ||
UNREFERENCED_PARAMETER(rc); | ||
} | ||
|
||
int ServerContextImpl::sessionTicketProcess(SSL*, uint8_t* key_name, uint8_t* iv, | ||
EVP_CIPHER_CTX* ctx, HMAC_CTX* hmac_ctx, int encrypt) { | ||
const EVP_MD* hmac = EVP_sha256(); | ||
|
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.
Please document the meaning/effect of skip_context_update
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.
Done.