Refresh default TLS context on secret update#13342
Conversation
Default server certificate secret updates could rebuild the TLS contexts for CN/SAN lookups while leaving the default/no-SNI context pointing at the old SSL_CTX. Operators could update cert material on disk and through the secret API, but new handshakes without a more specific match could still serve the stale certificate. This updates runtime context refresh to cover address/default lookup entries owned by the same ssl_multicert policy and retains the default context while callers create new TLS sessions. This also adds an AuTest that updates a plugin-loaded default certificate and verifies the next no-SNI handshake sees the new certificate. Fixes: apache#9562
| 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}/../', | ||
| 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}/../', |
moonchen
left a comment
moonchen
left a comment
There was a problem hiding this comment.
👍 to covering the default/no-SNI context on secret updates. A couple of issues with the new refresh loop, plus some smalls.
| } 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) { |
There was a problem hiding this comment.
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.
| } | ||
| if ((*policy_iter)->addr && strcmp((*policy_iter)->addr, "*") == 0) { | ||
| lookup->setDefaultContext(ctx); | ||
| this->_set_handshake_callbacks(ctx.get()); |
There was a problem hiding this comment.
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.
|
|
||
| QUICCertConfig::scoped_config server_cert; | ||
| SSL *ssl = SSL_new(server_cert->defaultContext()); | ||
| auto default_ctx = server_cert->defaultContext(); |
There was a problem hiding this comment.
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.
| defaultContext() const | ||
| { | ||
| return ssl_default.get(); | ||
| std::lock_guard<std::mutex> lock(default_ctx_mutex); |
There was a problem hiding this comment.
This needs #include <mutex> — the header only includes <shared_mutex>, so std::mutex/std::lock_guard currently compile through transitive includes.
|
|
||
| shared_SSL_CTX ssl_default; | ||
| bool is_valid = true; | ||
| mutable shared_SSL_CTX ssl_default; |
There was a problem hiding this comment.
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.
| // Return the last-resort default TLS context if there is no name or address match. | ||
| SSL_CTX * | ||
| shared_SSL_CTX | ||
| defaultContext() const |
There was a problem hiding this comment.
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.
| key_file = 'signed-bar.key' | ||
| plugin = 'ssl_secret_load_test.so' | ||
|
|
||
| def __init__(self): |
There was a problem hiding this comment.
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).
|
|
||
| import os | ||
|
|
||
| Test.Summary = ''' |
There was a problem hiding this comment.
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.
Default server certificate secret updates could rebuild the TLS
contexts for CN/SAN lookups while leaving the default/no-SNI context
pointing at the old SSL_CTX. Operators could update cert material on
disk and through the secret API, but new handshakes without a more
specific match could still serve the stale certificate.
This updates runtime context refresh to cover address/default lookup
entries owned by the same ssl_multicert policy and retains the
default context while callers create new TLS sessions. This also adds
an AuTest that updates a plugin-loaded default certificate and
verifies the next no-SNI handshake sees the new certificate.
Fixes: #9562