Skip to content

Rework server side SSL_CTX creation to better handle dual_cert mismatches #6483

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.

Sign up for GitHub

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

Merged
shinrich merged 7 commits into from
Mar 12, 2020
Merged

Rework server side SSL_CTX creation to better handle dual_cert mismatches #6483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ab7de42
Rework server side SSL_CTX creation to better handle dual_cert name m…
shinrich Feb 20, 2020
37df431
Adjust for boringssl
shinrich Mar 5, 2020
5fd7646
Address Maskit's comments
shinrich Mar 9, 2020
f4c5746
checkpoint
shinrich Mar 10, 2020
c729d56
Address Alan comments
shinrich Mar 10, 2020
36292af
Fix cert_update test
shinrich Mar 10, 2020
0034294
Remove stale comments
shinrich Mar 11, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion iocore/net/P_SSLCertLookup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ struct SSLCertContext {
{
}
SSLCertContext(shared_SSL_CTX sc, shared_SSLMultiCertConfigParams u)
: ctx_mutex(), ctx(sc), opt(u->opt), userconfig(nullptr), keyblock(nullptr)
: ctx_mutex(), ctx(sc), opt(u->opt), userconfig(u), keyblock(nullptr)
Copy link
Member

@maskit maskit Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this PR is for refactoring but not a bug fix. Is this change a part of the refactoring? It seems more like a bug fix.

Copy link
Member Author

@shinrich shinrich Mar 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR is fixing a bug with the dual cert processing as a means to get me back into the code to prepare for bigger dynamic cert loading changes. I ran across this issue while writing the autest. It is independent of the rest of the changes and could be pulled out separately if needed.

Copy link
Member

@maskit maskit Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification. I was afraid of that reverting a refactoring commit also reverts a fix (this line). I'm not sure how much this change and other changes are related, so I'll leave it up to your judgement.

Copy link
Member Author

@shinrich shinrich Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled it out into separate PR.

Copy link
Member Author

@shinrich shinrich Mar 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it was needed only in this code change. Without copy initializing userconfig, the cert_update test would fail.

{
}
SSLCertContext(shared_SSL_CTX sc, shared_SSLMultiCertConfigParams u, shared_ssl_ticket_key_block kb)
Expand Down
26 changes: 20 additions & 6 deletions iocore/net/P_SSLUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@
#include "records/I_RecCore.h"
#include "P_SSLCertLookup.h"

#include <set>
#include <map>

struct SSLConfigParams;
class SSLNetVConnection;

Expand All @@ -54,28 +57,39 @@ ssl_curve_id SSLGetCurveNID(SSL *ssl);
class SSLMultiCertConfigLoader
{
public:
struct CertLoadData {
std::vector<std::string> cert_names_list, key_list, ca_list, ocsp_list;
};
SSLMultiCertConfigLoader(const SSLConfigParams *p) : _params(p) {}
virtual ~SSLMultiCertConfigLoader(){};

bool load(SSLCertLookup *lookup);

virtual SSL_CTX *default_server_ssl_ctx();
virtual SSL_CTX *init_server_ssl_ctx(std::vector<X509 *> &certList, const SSLMultiCertConfigParams *sslMultCertSettings);

static bool load_certs(SSL_CTX *ctx, std::vector<X509 *> &certList, const SSLConfigParams *params,
const SSLMultiCertConfigParams *ssl_multi_cert_params);
virtual SSL_CTX *init_server_ssl_ctx(CertLoadData const &data, const SSLMultiCertConfigParams *sslMultCertSettings,
std::set<std::string> &names);

static bool load_certs(SSL_CTX *ctx, CertLoadData const &data, const SSLConfigParams *params,
const SSLMultiCertConfigParams *sslMultCertSettings);
bool load_certs_and_cross_reference_names(std::vector<X509 *> &cert_list, CertLoadData &data, const SSLConfigParams *params,
const SSLMultiCertConfigParams *sslMultCertSettings,
std::set<std::string> &common_names,
std::unordered_map<int, std::set<std::string>> &unique_names);
static bool set_session_id_context(SSL_CTX *ctx, const SSLConfigParams *params,
const SSLMultiCertConfigParams *sslMultCertSettings);

static bool index_certificate(SSLCertLookup *lookup, SSLCertContext const &cc, X509 *cert, const char *certname);
static bool index_certificate(SSLCertLookup *lookup, SSLCertContext const &cc, const char *sni_name);
static int check_server_cert_now(X509 *cert, const char *certname);
static void clear_pw_references(SSL_CTX *ssl_ctx);

protected:
const SSLConfigParams *_params;

bool _store_single_ssl_ctx(SSLCertLookup *lookup, shared_SSLMultiCertConfigParams sslMultCertSettings, shared_SSL_CTX ctx,
std::set<std::string> &names);

private:
virtual SSL_CTX *_store_ssl_ctx(SSLCertLookup *lookup, const shared_SSLMultiCertConfigParams ssl_multi_cert_params);
virtual bool _store_ssl_ctx(SSLCertLookup *lookup, shared_SSLMultiCertConfigParams ssl_multi_cert_params);
virtual void _set_handshake_callbacks(SSL_CTX *ctx);
};

Expand Down
112 changes: 34 additions & 78 deletions iocore/net/QUICMultiCertConfigLoader.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ QUICMultiCertConfigLoader::default_server_ssl_ctx()
}

SSL_CTX *
QUICMultiCertConfigLoader::init_server_ssl_ctx(std::vector<X509 *> &cert_list, const SSLMultiCertConfigParams *multi_cert_params)
QUICMultiCertConfigLoader::init_server_ssl_ctx(SSLMultiCertConfigLoader::CertLoadData const &data,
const SSLMultiCertConfigParams *multi_cert_params, std::set<std::string> &names)
{
const SSLConfigParams *params = this->_params;

Expand All @@ -92,7 +93,7 @@ QUICMultiCertConfigLoader::init_server_ssl_ctx(std::vector<X509 *> &cert_list, c
}

if (multi_cert_params->cert) {
if (!SSLMultiCertConfigLoader::load_certs(ctx, cert_list, params, multi_cert_params)) {
if (!SSLMultiCertConfigLoader::load_certs(ctx, data, params, multi_cert_params)) {
goto fail;
}
}
Expand Down Expand Up @@ -152,26 +153,6 @@ QUICMultiCertConfigLoader::init_server_ssl_ctx(std::vector<X509 *> &cert_list, c

SSL_CTX_set_alpn_select_cb(ctx, QUICMultiCertConfigLoader::ssl_select_next_protocol, nullptr);

#if TS_USE_TLS_OCSP
if (SSLConfigParams::ssl_ocsp_enabled) {
QUICConfDebug("SSL OCSP Stapling is enabled");
SSL_CTX_set_tlsext_status_cb(ctx, ssl_callback_ocsp_stapling);
const char *cert_name = multi_cert_params ? multi_cert_params->cert.get() : nullptr;

for (auto cert : cert_list) {
if (!ssl_stapling_init_cert(ctx, cert, cert_name, nullptr)) {
Warning("failed to configure SSL_CTX for OCSP Stapling info for certificate at %s", cert_name);
}
}
} else {
QUICConfDebug("SSL OCSP Stapling is disabled");
}
#else
if (SSLConfigParams::ssl_ocsp_enabled) {
Warning("failed to enable SSL OCSP Stapling; this version of OpenSSL does not support it");
}
#endif /* TS_USE_TLS_OCSP */

if (SSLConfigParams::init_ssl_ctx_cb) {
SSLConfigParams::init_ssl_ctx_cb(ctx, true);
}
Expand All @@ -180,85 +161,60 @@ QUICMultiCertConfigLoader::init_server_ssl_ctx(std::vector<X509 *> &cert_list, c

fail:
SSLReleaseContext(ctx);
for (auto cert : cert_list) {
X509_free(cert);
}

return nullptr;
}

SSL_CTX *
bool
QUICMultiCertConfigLoader::_store_ssl_ctx(SSLCertLookup *lookup, const shared_SSLMultiCertConfigParams multi_cert_params)
{
bool retval = true;
std::vector<X509 *> cert_list;
shared_SSL_CTX ctx(this->init_server_ssl_ctx(cert_list, multi_cert_params.get()), SSL_CTX_free);
shared_ssl_ticket_key_block keyblock = nullptr;
bool inserted = false;

if (!ctx || !multi_cert_params) {
lookup->is_valid = false;
return nullptr;
}
SSLMultiCertConfigLoader::CertLoadData data;
std::set<std::string> common_names;
std::unordered_map<int, std::set<std::string>> unique_names;
const SSLConfigParams *params = this->_params;
this->load_certs_and_cross_reference_names(cert_list, data, params, multi_cert_params.get(), common_names, unique_names);

const char *certname = multi_cert_params->cert.get();
for (auto cert : cert_list) {
if (0 > SSLMultiCertConfigLoader::check_server_cert_now(cert, certname)) {
for (size_t i = 0; i < cert_list.size(); i++) {
const char *current_cert_name = data.cert_names_list[i].c_str();
if (0 > SSLMultiCertConfigLoader::check_server_cert_now(cert_list[i], current_cert_name)) {
/* At this point, we know cert is bad, and we've already printed a
descriptive reason as to why cert is bad to the log file */
QUICConfDebug("Marking certificate as NOT VALID: %s", certname);
QUICConfDebug("Marking certificate as NOT VALID: %s", current_cert_name);
lookup->is_valid = false;
}
}

// Index this certificate by the specified IP(v6) address. If the address is "*", make it the default context.
if (multi_cert_params->addr) {
if (strcmp(multi_cert_params->addr, "*") == 0) {
if (lookup->insert(multi_cert_params->addr, SSLCertContext(ctx, multi_cert_params, keyblock)) >= 0) {
inserted = true;
lookup->ssl_default = ctx;
this->_set_handshake_callbacks(ctx.get());
}
} else {
IpEndpoint ep;

if (ats_ip_pton(multi_cert_params->addr, &ep) == 0) {
QUICConfDebug("mapping '%s' to certificate %s", (const char *)multi_cert_params->addr, (const char *)certname);
if (lookup->insert(ep, SSLCertContext(ctx, multi_cert_params, keyblock)) >= 0) {
inserted = true;
}
} else {
Error("'%s' is not a valid IPv4 or IPv6 address", (const char *)multi_cert_params->addr);
lookup->is_valid = false;
}
}
}
shared_SSL_CTX ctx(this->init_server_ssl_ctx(data, multi_cert_params.get(), common_names), SSL_CTX_free);

// Insert additional mappings. Note that this maps multiple keys to the same value, so when
// this code is updated to reconfigure the SSL certificates, it will need some sort of
// refcounting or alternate way of avoiding double frees.
QUICConfDebug("importing SNI names from %s", (const char *)certname);
for (auto cert : cert_list) {
if (SSLMultiCertConfigLoader::index_certificate(lookup, SSLCertContext(ctx, multi_cert_params), cert, certname)) {
inserted = true;
}
}
shared_ssl_ticket_key_block keyblock = nullptr;

if (inserted) {
if (SSLConfigParams::init_ssl_ctx_cb) {
SSLConfigParams::init_ssl_ctx_cb(ctx.get(), true);
}
if (!ctx || !multi_cert_params || !this->_store_single_ssl_ctx(lookup, multi_cert_params, ctx, common_names)) {
lookup->is_valid = false;
retval = false;
}

if (!inserted) {
SSLReleaseContext(ctx.get());
ctx = nullptr;
for (auto iter = unique_names.begin(); retval && iter != unique_names.end(); ++iter) {
size_t i = iter->first;

SSLMultiCertConfigLoader::CertLoadData single_data;
single_data.cert_names_list.push_back(data.cert_names_list[i]);
single_data.key_list.push_back(i < data.key_list.size() ? data.key_list[i] : "");
single_data.ca_list.push_back(i < data.ca_list.size() ? data.ca_list[i] : "");
single_data.ocsp_list.push_back(i < data.ocsp_list.size() ? data.ocsp_list[i] : "");

shared_SSL_CTX unique_ctx(this->init_server_ssl_ctx(single_data, multi_cert_params.get(), iter->second), SSL_CTX_free);
if (!unique_ctx || !this->_store_single_ssl_ctx(lookup, multi_cert_params, unique_ctx, iter->second)) {
lookup->is_valid = false;
retval = false;
}
}

for (auto &i : cert_list) {
X509_free(i);
}

return ctx.get();
return retval;
}

void
Expand Down
6 changes: 4 additions & 2 deletions iocore/net/QUICMultiCertConfigLoader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,12 @@ class QUICMultiCertConfigLoader : public SSLMultiCertConfigLoader
QUICMultiCertConfigLoader(const SSLConfigParams *p) : SSLMultiCertConfigLoader(p) {}

virtual SSL_CTX *default_server_ssl_ctx() override;
virtual SSL_CTX *init_server_ssl_ctx(std::vector<X509 *> &cert_list, const SSLMultiCertConfigParams *multi_cert_params) override;
// override;
SSL_CTX *init_server_ssl_ctx(SSLMultiCertConfigLoader::CertLoadData const &data,
const SSLMultiCertConfigParams *sslMultCertSettings, std::set<std::string> &names) override;

private:
virtual SSL_CTX *_store_ssl_ctx(SSLCertLookup *lookup, const shared_SSLMultiCertConfigParams multi_cert_params) override;
bool _store_ssl_ctx(SSLCertLookup *lookup, shared_SSLMultiCertConfigParams ssl_multi_cert_params) override;
virtual void _set_handshake_callbacks(SSL_CTX *ssl_ctx) override;
static int ssl_select_next_protocol(SSL *ssl, const unsigned char **out, unsigned char *outlen, const unsigned char *in,
unsigned inlen, void *);
Expand Down
Loading