Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 14 additions & 4 deletions src/iocore/net/P_SSLCertLookup.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,8 +134,8 @@ struct SSLCertLookup : public ConfigInfo {
std::unique_ptr<SSLContextStorage> ssl_storage;
std::unique_ptr<SSLContextStorage> ec_storage;

shared_SSL_CTX ssl_default;
bool is_valid = true;
mutable shared_SSL_CTX ssl_default;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can this move to the private section next to default_ctx_mutex? Nothing accesses it directly anymore, and keeping it public leaves the door open for the kind of unlocked read this PR fixed in SSLStats.cc.

bool is_valid = true;

int insert(const char *name, SSLCertContext const &cc);
int insert(const IpEndpoint &address, SSLCertContext const &cc);
Expand All @@ -154,10 +154,18 @@ struct SSLCertLookup : public ConfigInfo {
SSLCertContext *find(const std::string &name, SSLCertContextType ctxType = SSLCertContextType::GENERIC) const;

// Return the last-resort default TLS context if there is no name or address match.
SSL_CTX *
shared_SSL_CTX
defaultContext() const

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One optional suggestion: the default ctx is now stored twice (the "*" entry and ssl_default) under two different locks, and this one is exclusive on every no-SNI accept. Caching a pointer to the "*" entry's SSLCertContext and delegating to its getCtx()/setCtx() (shared_mutex) would drop both the duplicate state and the exclusive lock.

{
return ssl_default.get();
std::lock_guard<std::mutex> lock(default_ctx_mutex);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This needs #include <mutex> — the header only includes <shared_mutex>, so std::mutex/std::lock_guard currently compile through transitive includes.

return ssl_default;
}

void
setDefaultContext(shared_SSL_CTX ctx) const
{
std::lock_guard<std::mutex> lock(default_ctx_mutex);
ssl_default = std::move(ctx);
}

unsigned count(SSLCertContextType ctxType = SSLCertContextType::GENERIC) const;
Expand All @@ -170,6 +178,8 @@ struct SSLCertLookup : public ConfigInfo {
~SSLCertLookup() override;

private:
mutable std::mutex default_ctx_mutex;

// Map cert_secret name to lookup keys
std::unordered_map<std::string, std::vector<std::string>> cert_secret_registry;
};
Expand Down
3 changes: 2 additions & 1 deletion src/iocore/net/QUICPacketHandler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,8 @@ QUICPacketHandlerIn::_recv_packet(int /* event ATS_UNUSED */, UDPPacket *udp_pac
QUICConnectionId new_cid;

QUICCertConfig::scoped_config server_cert;
SSL *ssl = SSL_new(server_cert->defaultContext());
auto default_ctx = server_cert->defaultContext();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not for this PR, but worth a note in the description or a follow-up issue: QUICCertConfig builds its own SSLCertLookup that only a full reload rebuilds, so secret updates still leave QUIC serving the stale default cert.

SSL *ssl = SSL_new(default_ctx.get());

quiche_conn *quiche_con = quiche_conn_new_with_tls(
new_cid, new_cid.length(), retry_token.original_dcid(), retry_token.original_dcid().length(), &udp_packet->to.sa,
Expand Down
9 changes: 5 additions & 4 deletions src/iocore/net/SSLNetVConnection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1108,7 +1108,8 @@ SSLNetVConnection::_sslStartHandShake(int event, int &err)
}
ats_ip_nptop(&dst, ipb1, sizeof(ipb1));
ats_ip_nptop(&src, ipb2, sizeof(ipb2));
DbgPrint(dbg_ctl_ssl, "IP context is %p for [%s] -> [%s], default context %p", cc, ipb2, ipb1, lookup->defaultContext());
auto default_ctx = lookup->defaultContext();
DbgPrint(dbg_ctl_ssl, "IP context is %p for [%s] -> [%s], default context %p", cc, ipb2, ipb1, default_ctx.get());
}

// Escape if this is marked to be a tunnel.
Expand All @@ -1130,7 +1131,7 @@ SSLNetVConnection::_sslStartHandShake(int event, int &err)
// Attach the default SSL_CTX to this SSL session. The default context is never going to be able
// to negotiate a SSL session, but it's enough to trampoline us into the SNI callback where we
// can select the right server certificate.
this->_make_ssl_connection(lookup->defaultContext());
this->_make_ssl_connection(lookup->defaultContext().get());
}

if (this->ssl == nullptr) {
Expand Down Expand Up @@ -2005,8 +2006,8 @@ SSLNetVConnection::_lookupContextByIP()
return nullptr;
}
ats_ip_nptop(&src, ipb2, sizeof(ipb2));
DbgPrint(dbg_ctl_proxyprotocol, "IP context is %p for [%s] -> [%s], default context %p", cc, ipb2, ipb1,
lookup->defaultContext());
auto default_ctx = lookup->defaultContext();
DbgPrint(dbg_ctl_proxyprotocol, "IP context is %p for [%s] -> [%s], default context %p", cc, ipb2, ipb1, default_ctx.get());
}
} else if (0 == safe_getsockname(this->get_socket(), &ip.sa, &namelen)) {
cc = lookup->find(ip);
Expand Down
5 changes: 3 additions & 2 deletions src/iocore/net/SSLStats.cc
Original file line number Diff line number Diff line change
Expand Up @@ -273,12 +273,13 @@ SSLInitializeStatistics()
// Acquire the loaded SSL certificate configuration to enumerate ciphers and groups.
// This must be called AFTER SSLCertificateConfig::startup().
SSLCertificateConfig::scoped_config lookup;
if (!lookup || !lookup->ssl_default) {
auto default_ctx = lookup ? lookup->defaultContext() : nullptr;
if (!default_ctx) {
Dbg(dbg_ctl_ssl, "No SSL configuration, skipping cipher/group statistics initialization");
return;
}

SSL_CTX *ctx = lookup->ssl_default.get();
SSL_CTX *ctx = default_ctx.get();
SSL *ssl = SSL_new(ctx);
STACK_OF(SSL_CIPHER) *ciphers = SSL_get_ciphers(ssl);

Expand Down
18 changes: 15 additions & 3 deletions src/iocore/net/SSLUtils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1733,6 +1733,18 @@ SSLMultiCertConfigLoader::update_ssl_ctx(const std::string &secret_name)
if (!ctx) {
retval = false;
} else {
// Address and default-context lookups are owned by the same policy, but
// are not part of the certificate CN/SAN name sets below.
for (unsigned i = 0; i < lookup->count(loadingctx.ctx_type); ++i) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This predicate matches every entry the policy owns, including the unique-SAN entries — everything from one ssl_multicert line shares the same userconfig. Those get stamped with the common ctx here and only repaired by the unique_names loop below, and if that rebuild fails, init_server_ssl_ctx returns an empty vector (goto fail), so if (!unique_ctx) never runs and they permanently keep the wrong cert while update_ssl_ctx returns true. Since the goal is just the addr-keyed entry, find("*", ...) (or ats_ip_pton + find(ep) for a non-* addr) would update exactly that entry and keep the "old ctx or new ctx, never a wrong one" behavior. The comment above also doesn't match the code — these entries are the CN/SAN name sets below.

SSLCertContext *cc = lookup->get(i, loadingctx.ctx_type);
if (cc && cc->ctx_type == loadingctx.ctx_type && cc->userconfig.get() == policy_iter->get()) {
cc->setCtx(ctx);
}
}
if ((*policy_iter)->addr && strcmp((*policy_iter)->addr, "*") == 0) {
lookup->setDefaultContext(ctx);
this->_set_handshake_callbacks(ctx.get());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unlike the load path, this lookup is live, so the new ctx is reachable by net threads (cc->setCtx above, setDefaultContext here) before the callbacks are installed. SSL_new copies cert_cb, so a handshake landing in that window runs with no cert selection at all. Installing the callbacks right after the ctx is created, before any setCtx, closes the window.

}
for (auto const &name : common_names) {
SSLCertContext *cc = lookup->find(name, loadingctx.ctx_type);
if (cc && cc->userconfig.get() == policy_iter->get()) {
Expand Down Expand Up @@ -1787,8 +1799,8 @@ SSLMultiCertConfigLoader::_store_single_ssl_ctx(SSLCertLookup *lookup, const sha
if (strcmp(sslMultCertSettings->addr, "*") == 0) {
Dbg(dbg_ctl_ssl_load, "Addr is '*'; setting %p to default", ctx.get());
if (lookup->insert(sslMultCertSettings->addr, SSLCertContext(ctx, ctx_type, sslMultCertSettings, keyblock)) >= 0) {
inserted = true;
lookup->ssl_default = ctx;
inserted = true;
lookup->setDefaultContext(ctx);
this->_set_handshake_callbacks(ctx.get());
}
} else {
Expand Down Expand Up @@ -1890,7 +1902,7 @@ SSLMultiCertConfigLoader::load(SSLCertLookup *lookup, bool firstLoad)
// We *must* have a default context even if it can't possibly work. The default context is used to
// bootstrap the SSL handshake so that we can subsequently do the SNI lookup to switch to the real
// context.
if (lookup->ssl_default == nullptr) {
if (lookup->defaultContext() == nullptr) {
shared_SSLMultiCertConfigParams sslMultiCertSettings(new SSLMultiCertConfigParams);
sslMultiCertSettings->addr = ats_strdup("*");
if (!this->_store_ssl_ctx(lookup, sslMultiCertSettings)) {
Expand Down
151 changes: 151 additions & 0 deletions tests/gold_tests/tls/tls_secret_update_default.test.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
'''
Test default server certificate updates through TSSslSecretSet/TSSslSecretUpdate.
'''
# Licensed to the Apache Software Foundation (ASF) under one
# or more contributor license agreements. See the NOTICE file
# distributed with this work for additional information
# regarding copyright ownership. The ASF licenses this file
# to you under the Apache License, Version 2.0 (the
# "License"); you may not use this file except in compliance
# with the License. You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

import os

Test.Summary = '''

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Optional: tls_check_cert_select_plugin.test.py already has the same plugin/wildcard-policy/CopyAs+touch scaffolding — a no-SNI run added there might cover this without a second copy of the update dance.

Verify that secret updates refresh the default server SSL_CTX used without SNI.
'''


class TestDefaultSecretUpdate:
'''Verify default server certificate updates through the secret API.'''

initial_cert = 'signed-bar.pem'
updated_cert = 'signed2-bar.pem'
key_file = 'signed-bar.key'
plugin = 'ssl_secret_load_test.so'

def __init__(self):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Type annotations on these methods, please — matching the other class-style tests in this directory (e.g. def __init__(self) -> None: in tls_origin_post_abort.test.py).

'''Configure the ATS process, origin server, and test runs.'''
self._configure_server()
self._configure_traffic_server()
self._add_initial_certificate_run()
self._add_mtime_delay_run()
self._add_certificate_update_run()
self._add_secret_update_wait_run()
self._add_updated_certificate_run()

def _configure_server(self):
'''Configure the origin server.'''
server = Test.MakeOriginServer('server')
self._server = server

request_header = {'headers': 'GET / HTTP/1.1\r\nHost: doesnotmatter\r\n\r\n', 'timestamp': '1469733493.993', 'body': ''}
response_header = {'headers': 'HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n', 'timestamp': '1469733493.993', 'body': ''}
server.addResponse('sessionlog.json', request_header, response_header)
return server

def _configure_traffic_server(self):
'''Configure the Traffic Server process.'''
ts = Test.MakeATSProcess('ts', enable_tls=True)
self._ts = ts

ts.addSSLfile(f'ssl/{self.initial_cert}')
ts.addSSLfile(f'ssl/{self.updated_cert}')
ts.addSSLfile(f'ssl/{self.key_file}')
Test.PrepareTestPlugin(os.path.join(Test.Variables.AtsTestPluginsDir, self.plugin), ts)

ts.Disk.records_config.update(
{
'proxy.config.diags.debug.enabled': 1,
'proxy.config.diags.debug.tags': 'ssl_secret_load_test|ssl|ssl_config_updateCTX',
'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}/../',
'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}/../',
Comment on lines +69 to +70
'proxy.config.exec_thread.autoconfig.scale': 1.0,
'proxy.config.url_remap.pristine_host_hdr': 1,
})

ts.Disk.ssl_multicert_yaml.AddLines(
[
'ssl_multicert:',
' - dest_ip: "*"',
f' ssl_cert_name: {self.initial_cert}',
f' ssl_key_name: {self.key_file}',
])
ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self._server.Variables.Port}')
return ts

def _add_initial_certificate_run(self):
'''Verify the original default certificate before the secret update.'''
tr = self._add_curl_run(
'Initial default certificate',
expected_cn='signer.yahoo.com',
unexpected_cn='signer2.yahoo.com',
expected_description='Initial cert uses signer.',
unexpected_description='Initial cert is not updated yet.')
tr.Processes.Default.StartBefore(self._server)
tr.Processes.Default.StartBefore(self._ts)
return tr

def _add_mtime_delay_run(self):
'''Make the replacement certificate mtime differ from the original.'''
tr = Test.AddTestRun('Make the cert mtime differ')
tr.Processes.Default.Command = 'sleep 2'
tr.Processes.Default.ReturnCode = 0
self._keep_processes_running(tr)
return tr

def _add_certificate_update_run(self):
'''Replace the plugin backing certificate file.'''
tr = Test.AddTestRun('Update the plugin backing certificate')
tr.Setup.CopyAs(f'ssl/{self.updated_cert}', '.', f'{self._ts.Variables.SSLDir}/{self.initial_cert}')
tr.Processes.Default.Command = f'touch {self._ts.Variables.SSLDir}/{self.initial_cert}'
tr.Processes.Default.ReturnCode = 0
self._keep_processes_running(tr)
return tr

def _add_secret_update_wait_run(self):
'''Wait for the test plugin to update the certificate secret.'''
tr = Test.AddTestRun('Wait for the secret update')
tr.Processes.Default.Command = 'echo awaiting secret update'
tr.Processes.Default.ReturnCode = 0
await_update = tr.Processes.Process('await_update', 'sleep 30')
await_update.Ready = When.FileContains(self._ts.Disk.traffic_out.Name, 'update cert for secret')
tr.Processes.Default.StartBefore(await_update)
self._keep_processes_running(tr)
return tr

def _add_updated_certificate_run(self):
'''Verify the updated default certificate after the secret update.'''
return self._add_curl_run(
'Updated default certificate',
expected_cn='signer2.yahoo.com',
unexpected_cn='signer.yahoo.com',
expected_description='Updated cert uses signer2.',
unexpected_description='Updated cert no longer uses signer.')

def _add_curl_run(self, name, expected_cn, unexpected_cn, expected_description, unexpected_description):
'''Add a curl run against ATS without SNI.'''
tr = Test.AddTestRun(name)
self._keep_processes_running(tr)
tr.MakeCurlCommand(
f"-k -v --http1.1 -H 'host: doesnotmatter' https://127.0.0.1:{self._ts.Variables.ssl_port}/", ts=self._ts)
tr.Processes.Default.ReturnCode = 0
tr.Processes.Default.Streams.All = Testers.ContainsExpression(f'issuer:.*CN={expected_cn}', expected_description)
tr.Processes.Default.Streams.All += Testers.ExcludesExpression(f'issuer:.*CN={unexpected_cn}', unexpected_description)
return tr

def _keep_processes_running(self, tr):
'''Keep ATS and the origin server running after a TestRun.'''
tr.StillRunningAfter = self._ts
tr.StillRunningAfter = self._server


TestDefaultSecretUpdate()