Skip to content

Ensure that ca override does not get lost #7219

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 2 commits into from
Oct 6, 2020
Merged

Ensure that ca override does not get lost #7219

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f59ef13
Ensure that ca override does not get lost
shinrich Sep 25, 2020
7cb6722
Add reset
shinrich Oct 5, 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
13 changes: 7 additions & 6 deletions iocore/net/P_SNIActionPerformer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,21 +193,22 @@ class TunnelDestination : public ActionItem
class VerifyClient : public ActionItem
{
uint8_t mode;
X509_STORE *ca_certs; // owning pointer.
std::string ca_file;
std::string ca_dir;

public:
VerifyClient(uint8_t param, X509_STORE *st = nullptr) : mode(param), ca_certs(st) {}
VerifyClient(const char *param, X509_STORE *st = nullptr) : VerifyClient(atoi(param), st) {}
VerifyClient(uint8_t param, std::string_view file, std::string_view dir) : mode(param), ca_file(file), ca_dir(dir) {}
VerifyClient(const char *param, std::string_view file, std::string_view dir) : VerifyClient(atoi(param), file, dir) {}
~VerifyClient() override;
int
SNIAction(Continuation *cont, const Context &ctx) const override
{
auto ssl_vc = dynamic_cast<SSLNetVConnection *>(cont);
Debug("ssl_sni", "action verify param %d", this->mode);
setClientCertLevel(ssl_vc->ssl, this->mode);
if (ca_certs) {
setClientCertCACerts(ssl_vc->ssl, ca_certs);
}
ssl_vc->set_ca_cert_file(ca_file, ca_dir);
setClientCertCACerts(ssl_vc->ssl, ssl_vc->get_ca_cert_file(), ssl_vc->get_ca_cert_dir());

return SSL_TLSEXT_ERR_OK;
}
bool
Expand Down
15 changes: 15 additions & 0 deletions iocore/net/P_SSLNetVConnection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -467,6 +467,19 @@ class SSLNetVConnection : public UnixNetVConnection, public ALPNSupport, public
sent_cert = send_the_cert;
}

void set_ca_cert_file(std::string_view file, std::string_view dir);

const char *
get_ca_cert_file()
{
return _ca_cert_file.get();
}
const char *
get_ca_cert_dir()
{
return _ca_cert_dir.get();
}

protected:
const IpEndpoint &
_getLocalEndpoint() override
Expand Down Expand Up @@ -519,6 +532,8 @@ class SSLNetVConnection : public UnixNetVConnection, public ALPNSupport, public

// Null-terminated string, or nullptr if there is no SNI server name.
std::unique_ptr<char[]> _serverName;
std::unique_ptr<char[]> _ca_cert_file;
std::unique_ptr<char[]> _ca_cert_dir;
Copy link
Member

@SolidWallOfCode SolidWallOfCode Oct 5, 2020

Choose a reason for hiding this comment

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

These are needed because the SNI config can disappear during a transaction, destroying the original source, correct?


EventIO async_ep{};
};
Expand Down
2 changes: 1 addition & 1 deletion iocore/net/P_SSLUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ void SSLNetVCDetach(SSL *ssl);
SSLNetVConnection *SSLNetVCAccess(const SSL *ssl);

void setClientCertLevel(SSL *ssl, uint8_t certLevel);
void setClientCertCACerts(SSL *ssl, X509_STORE *ca_certs);
void setClientCertCACerts(SSL *ssl, const char *file, const char *dir);
void setTLSValidProtocols(SSL *ssl, unsigned long proto_mask, unsigned long max_mask);

namespace ssl
Expand Down
19 changes: 19 additions & 0 deletions iocore/net/SSLNetVConnection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,8 @@ void
SSLNetVConnection::clear()
{
_serverName.reset();
_ca_cert_file.reset();
Copy link
Member

@SolidWallOfCode SolidWallOfCode Oct 5, 2020

Choose a reason for hiding this comment

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

Need a _ca_cert_dir.reset(); also?

_ca_cert_dir.reset();

if (ssl != nullptr) {
SSL_free(ssl);
Expand Down Expand Up @@ -1921,3 +1923,20 @@ SSLNetVConnection::set_server_name(std::string_view name)
_serverName.reset(n);
}
}

void
SSLNetVConnection::set_ca_cert_file(std::string_view file, std::string_view dir)
{
if (file.size()) {
char *n = new char[file.size() + 1];
std::memcpy(n, file.data(), file.size());
n[file.size()] = '\0';
_ca_cert_file.reset(n);
}
if (dir.size()) {
char *n = new char[dir.size() + 1];
std::memcpy(n, dir.data(), dir.size());
n[dir.size()] = '\0';
_ca_cert_dir.reset(n);
}
}
3 changes: 2 additions & 1 deletion iocore/net/SSLSNIConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,8 @@ SNIConfigParams::loadSNIConfig()
ai->actions.push_back(std::make_unique<ControlH2>(item.offer_h2.value()));
}
if (item.verify_client_level != 255) {
ai->actions.push_back(std::make_unique<VerifyClient>(item.verify_client_level, item.verify_client_ca_certs.release()));
ai->actions.push_back(
std::make_unique<VerifyClient>(item.verify_client_level, item.verify_client_ca_file, item.verify_client_ca_dir));
}
if (item.host_sni_policy != 255) {
ai->actions.push_back(std::make_unique<HostSniPolicy>(item.host_sni_policy));
Expand Down
24 changes: 16 additions & 8 deletions iocore/net/SSLUtils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,8 @@ set_context_cert(SSL *ssl)
// Reset the ticket callback if needed
SSL_CTX_set_tlsext_ticket_key_cb(ctx.get(), ssl_callback_session_ticket);
#endif
// After replacing the SSL_CTX, make sure the overriden ca_cert_file is still set
setClientCertCACerts(ssl, netvc->get_ca_cert_file(), netvc->get_ca_cert_dir());
} else {
found = false;
}
Expand Down Expand Up @@ -1146,19 +1148,25 @@ setClientCertLevel(SSL *ssl, uint8_t certLevel)
}

void
setClientCertCACerts(SSL *ssl, X509_STORE *ca_certs)
setClientCertCACerts(SSL *ssl, const char *file, const char *dir)
{
if ((file != nullptr && file[0] != '\0') || (dir != nullptr && dir[0] != '\0')) {
#if defined(SSL_set1_verify_cert_store)
// The set0 version will take ownership of the X509_STORE object
X509_STORE *ctx = X509_STORE_new();
if (X509_STORE_load_locations(ctx, file && file[0] != '\0' ? file : nullptr, dir && dir[0] != '\0' ? dir : nullptr)) {
SSL_set0_verify_cert_store(ssl, ctx);
}

// It is necessasry to use set1 and not set0 here. SSL_free() calls ssl_cert_free(), which calls
// X509_STORE_free() on the ca_certs store. Hard to see how set0 could ever actually be useful, in
// what scenario would SSL objects never be freed?
//
SSL_set1_verify_cert_store(ssl, ca_certs);

// SSL_set_client_CA_list takes ownership of the STACK_OF(X509) structure
// So we load it each time to pass into the SSL object
if (file != nullptr && file[0] != '\0') {
SSL_set_client_CA_list(ssl, SSL_load_client_CA_file(file));
}
Copy link
Contributor

@ywkaras ywkaras Sep 28, 2020

Choose a reason for hiding this comment

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

I'm really lost with all this. Doesn't this just set the names of the CAs that are sent to the client, not the CA certs used to validate? How is this relevant to Vinith's (or my) test, where the curl command only has a single cert it can send in any case? Seems like there should be a way to avoid having to read a disk file on every single TLS handshake.

Copy link
Member Author

@shinrich shinrich Sep 28, 2020

Choose a reason for hiding this comment

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

True. I added this when debugging the issue. Based on the documentation, it seemed like there were cases when the client would only look for appropriate intermediates if this was set. Since we set this in the SSL_CTX case, I added it here. Turns out it didn't fix the issue (at least not alone). The ssl->cert->verify_store rewriting was the main problem.

#else
ink_assert(!"Configuration checking should prevent reaching this code");
ink_assert(!"Configuration checking should prevent reaching this code");
#endif
}
}

/**
Expand Down
22 changes: 3 additions & 19 deletions iocore/net/YamlSNIConfig.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,6 @@ YamlSNIConfig::loader(const char *cfgFilename)
return ts::Errata();
}

void
YamlSNIConfig::X509StoreDeleter::operator()(X509_STORE *store)
{
if (store) {
X509_STORE_free(store);
}
}

void
YamlSNIConfig::Item::EnableProtocol(YamlSNIConfig::TLSProtocol proto)
{
Expand Down Expand Up @@ -98,12 +90,7 @@ YamlSNIConfig::Item::EnableProtocol(YamlSNIConfig::TLSProtocol proto)
}
}

VerifyClient::~VerifyClient()
{
if (ca_certs) {
X509_STORE_free(ca_certs);
}
}
VerifyClient::~VerifyClient() {}

TsEnumDescriptor LEVEL_DESCRIPTOR = {{{"NONE", 0}, {"MODERATE", 1}, {"STRICT", 2}}};
TsEnumDescriptor POLICY_DESCRIPTOR = {{{"DISABLED", 0}, {"PERMISSIVE", 1}, {"ENFORCED", 2}}};
Expand Down Expand Up @@ -207,11 +194,8 @@ template <> struct convert<YamlSNIConfig::Item> {
if (!dir.empty() && (dir[0] != '/')) {
dir = RecConfigReadConfigDir() + '/' + dir;
}
YamlSNIConfig::X509UniqPtr ctx{X509_STORE_new()};
if (!X509_STORE_load_locations(ctx.get(), file.empty() ? nullptr : file.c_str(), dir.empty() ? nullptr : dir.c_str())) {
throw YAML::ParserException(n.Mark(), "cannot load CA certs in file=" + file + " dir=" + dir);
}
item.verify_client_ca_certs.reset(ctx.release());
item.verify_client_ca_file = file;
item.verify_client_ca_dir = dir;
#endif
}

Expand Down
12 changes: 2 additions & 10 deletions iocore/net/YamlSNIConfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,6 @@ TSDECL(http2);
TSDECL(host_sni_policy);
#undef TSDECL

// Predeclare OpenSSL X509 store struct.
struct x509_store_st;

const int start = 0;
struct YamlSNIConfig {
enum class Action {
Expand All @@ -73,17 +70,12 @@ struct YamlSNIConfig {

YamlSNIConfig() {}

struct X509StoreDeleter {
void operator()(x509_store_st *);
};

using X509UniqPtr = std::unique_ptr<x509_store_st, X509StoreDeleter>;

struct Item {
std::string fqdn;
std::optional<bool> offer_h2; // Has no value by default, so do not initialize!
uint8_t verify_client_level = 255;
X509UniqPtr verify_client_ca_certs;
std::string verify_client_ca_file;
std::string verify_client_ca_dir;
uint8_t host_sni_policy = 255;
std::string tunnel_destination;
bool tunnel_decrypt = false;
Expand Down
10 changes: 8 additions & 2 deletions tests/gold_tests/tls/tls_client_verify3.test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@
'proxy.config.ssl.TLSv1_3': 0
})

ts.Disk.ssl_multicert_config.AddLine(
'ssl_cert_name={0}/bbb-signed.pem ssl_key_name={0}/bbb-signed.key'.format(Test.TestDirectory + "/ssl")
)
ts.Disk.ssl_multicert_config.AddLine(
'dest_ip=* ssl_cert_name=server.pem ssl_key_name=server.key'
)
Expand All @@ -61,6 +64,9 @@
'- fqdn: bbb.com',
' verify_client: STRICT',
' verify_client_ca_certs: bbb-ca.pem',
'- fqdn: bbb-signed',
' verify_client: STRICT',
' verify_client_ca_certs: bbb-ca.pem',
'- fqdn: ccc.com',
' verify_client: STRICT',
' verify_client_ca_certs:',
Expand All @@ -85,8 +91,8 @@
tr.StillRunningAfter = ts
tr.StillRunningAfter = server
tr.Processes.Default.Command = (
"curl -v -k --tls-max 1.2 --cert {1}.pem --key {1}.key --resolve 'bbb.com:{0}:127.0.0.1'" +
" https://bbb.com:{0}/xyz"
"curl -v -k --tls-max 1.2 --cert {1}.pem --key {1}.key --resolve 'bbb-signed:{0}:127.0.0.1'" +
" https://bbb-signed:{0}/xyz"
).format(ts.Variables.ssl_port, Test.TestDirectory + "/ssl/bbb-signed")
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = Testers.ExcludesExpression("error", "Check response")
Expand Down