bneradt commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4108985753
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -54,6 +58,72 @@ namespace
{
DbgCtl dbg_ctl_ssl_session_ticket{"ssl_session_ticket"};
+using unique_ssl_ticket_key_block = std::unique_ptr<ssl_ticket_key_block, void
(*)(void *)>;
+
+/** Derive one ticket secret from a global secret and a certificate digest.
+
+ @return Whether the derivation succeeded.
+ */
+bool
+derive_ticket_secret(std::string_view label, const unsigned char *secret,
size_t secret_len, const unsigned char *cert_digest,
+ unsigned cert_digest_len, unsigned char *out, size_t
out_len)
+{
+ unsigned char md[EVP_MAX_MD_SIZE];
+ unsigned md_len = 0;
+ EVP_MD_CTX *md_ctx = EVP_MD_CTX_new();
+ bool const ok = md_ctx != nullptr && EVP_DigestInit_ex(md_ctx,
EVP_sha256(), nullptr) == 1 &&
+ EVP_DigestUpdate(md_ctx, label.data(), label.size()) == 1 &&
EVP_DigestUpdate(md_ctx, secret, secret_len) == 1 &&
+ EVP_DigestUpdate(md_ctx, cert_digest, cert_digest_len) == 1
&& EVP_DigestFinal_ex(md_ctx, md, &md_len) == 1 &&
+ md_len >= out_len;
+
+ EVP_MD_CTX_free(md_ctx);
+ if (ok) {
+ memcpy(out, md, out_len);
+ }
+ OPENSSL_cleanse(md, sizeof(md));
+ return ok;
+}
+
+/** Derive the keys protecting the tickets of one certificate context from the
global keys.
+
+ The global keys are shared by every certificate context, so on their own a
ticket issued under
+ one certificate would resume under any other. Mixing in a digest of the
context's certificate
+ confines each ticket to contexts serving that certificate, while servers
that share both the
+ ticket keys and the certificate still derive the same keys and resume each
other's tickets.
+ The key names are kept so that key rotation works as it does for the
global keys.
+
+ @return The derived keys, or null if the context has no certificate to
bind them to.
+ */
+unique_ssl_ticket_key_block
+ticket_keyblock_for_certificate(const ssl_ticket_key_block &global,
SSLCertContext &cc)
+{
+ unique_ssl_ticket_key_block derived{nullptr, ticket_block_free};
+ shared_SSL_CTX ctx = cc.getCtx();
+ X509 *cert = ctx ? SSL_CTX_get0_certificate(ctx.get())
: nullptr;
+ unsigned char cert_digest[EVP_MAX_MD_SIZE];
+ unsigned cert_digest_len = 0;
+
+ if (cert == nullptr || X509_digest(cert, EVP_sha256(), cert_digest,
&cert_digest_len) != 1) {
+ return derived;
+ }
+
+ derived.reset(ticket_block_alloc(global.num_keys));
+ for (unsigned i = 0; i < global.num_keys; ++i) {
+ ssl_ticket_key_t const &from = global.keys[i];
+ ssl_ticket_key_t &to = derived->keys[i];
+
+ memcpy(to.key_name, from.key_name, sizeof(to.key_name));
Review Comment:
Done in `645773ca18`. The key names are now derived too (`SHA256("ATS
session ticket key name" || key_name || cert_digest)`, truncated to 16 bytes),
and each derived key keeps the index of the key it came from, so rotation order
is unchanged. A ticket from another certificate now takes the `keyname is not
consistent` path and counts as `total_tickets_not_found`.
The test logs `%<cqup> %<cqssr> %<cqssrt>` and asserts `0 0` for every
cross-certificate leg and `1 2` for every resumed leg. On `619af921d3` the
cross legs log `1 2`, which is the misreport you found.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -115,6 +185,20 @@ TLSSessionResumptionSupport::processSessionTicket(SSL
*ssl, unsigned char *keyna
}
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // A context selected by destination address is chosen for clients that send
no SNI, so the
+ // server name cannot keep its tickets apart from another address's. Bind
the global keys to
+ // the context's certificate so a ticket only resumes where that certificate
is served.
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr && cc->keyblock == nullptr) {
Review Comment:
Done. `SSLNetVConnection` now has one `_proxy_protocol_dst_endpoint()`
helper, used by both `_lookupContextByIP()` and the `_getLocalEndpoint()`
override, so the ticket keys are chosen by the same address as the certificate.
The test has two PROXY protocol runs on the proxy-protocol SSL port. One
listener address fronts two documentation VIPs, `192.0.2.1` and `192.0.2.2`,
with different `dest_ip` certificates. A session resumes on its own VIP and
does a full handshake on the other. On `619af921d3` the cross-VIP leg resumed.
While factoring this out I noticed the old `_lookupContextByIP()` copied the
address with `ip.sa = *proxy_protocol_dst_addr`. That copies only a 16-byte
`sockaddr`, so an IPv6 PROXY destination was truncated before the lookup. The
helper returns the whole `IpEndpoint`, so that is fixed as well. I did not add
an IPv6 PROXY run for it.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -115,6 +185,20 @@ TLSSessionResumptionSupport::processSessionTicket(SSL
*ssl, unsigned char *keyna
}
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // A context selected by destination address is chosen for clients that send
no SNI, so the
Review Comment:
Reworded, close to your suggestion: "The context is looked up by destination
address for every connection. A client that sends no SNI has nothing else to
keep its tickets apart from another address's, so bind the global keys to that
context's certificate. Connections to an address with no context of its own use
the global keys as they are." The PR description now says "an address with its
own certificate context" rather than implying no-SNI only.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -115,6 +185,20 @@ TLSSessionResumptionSupport::processSessionTicket(SSL
*ssl, unsigned char *keyna
}
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // A context selected by destination address is chosen for clients that send
no SNI, so the
+ // server name cannot keep its tickets apart from another address's. Bind
the global keys to
+ // the context's certificate so a ticket only resumes where that certificate
is served.
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr && cc->keyblock == nullptr) {
+ cert_keyblock = ticket_keyblock_for_certificate(*keyblock, *cc);
+ if (cert_keyblock == nullptr) {
+ // Neither issue nor accept a ticket that is not bound to a certificate.
+ Dbg(dbg_ctl_ssl_session_ticket, "no certificate to bind the ticket keys
to, not using a ticket");
Review Comment:
Added `proxy.process.ssl.total_tickets_no_certificate` (documented in
`ssl.en.rst`), incremented on every skipped ticket operation. There is also a
one-time `Warning()` naming the address: "session tickets are disabled for
<addr>: its certificate context has no certificate to bind them to". It fires
once per process through `std::call_once`, so a misconfigured address cannot
flood the log. The counter keeps counting after the first warning.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -115,6 +185,20 @@ TLSSessionResumptionSupport::processSessionTicket(SSL
*ssl, unsigned char *keyna
}
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // A context selected by destination address is chosen for clients that send
no SNI, so the
+ // server name cannot keep its tickets apart from another address's. Bind
the global keys to
+ // the context's certificate so a ticket only resumes where that certificate
is served.
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr && cc->keyblock == nullptr) {
+ cert_keyblock = ticket_keyblock_for_certificate(*keyblock, *cc);
Review Comment:
Took the cheaper step: the three derivations per key now reuse one
`EVP_MD_CTX` instead of allocating one per call. I also added a note on
`ticket_keyblock_for_certificate()` saying the keys are derived per call on
purpose, because the global keys reload independently of `ssl_multicert` and a
cached derivation would keep using rotated-out keys. I left the digest cache
keyed in `SSLTicketParams` for a follow-up if the cost shows up in practice.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -115,6 +185,20 @@ TLSSessionResumptionSupport::processSessionTicket(SSL
*ssl, unsigned char *keyna
}
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // A context selected by destination address is chosen for clients that send
no SNI, so the
+ // server name cannot keep its tickets apart from another address's. Bind
the global keys to
+ // the context's certificate so a ticket only resumes where that certificate
is served.
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr && cc->keyblock == nullptr) {
Review Comment:
Removed. `SSLCertContext::keyblock`, its four-argument constructor, the copy
lines and the `shared_ssl_ticket_key_block` alias are gone.
`processSessionTicket()` now starts from `default_global_keyblock` and derives
whenever `cc != nullptr`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -54,6 +58,72 @@ namespace
{
DbgCtl dbg_ctl_ssl_session_ticket{"ssl_session_ticket"};
+using unique_ssl_ticket_key_block = std::unique_ptr<ssl_ticket_key_block, void
(*)(void *)>;
+
+/** Derive one ticket secret from a global secret and a certificate digest.
+
+ @return Whether the derivation succeeded.
+ */
+bool
+derive_ticket_secret(std::string_view label, const unsigned char *secret,
size_t secret_len, const unsigned char *cert_digest,
+ unsigned cert_digest_len, unsigned char *out, size_t
out_len)
+{
+ unsigned char md[EVP_MAX_MD_SIZE];
+ unsigned md_len = 0;
+ EVP_MD_CTX *md_ctx = EVP_MD_CTX_new();
+ bool const ok = md_ctx != nullptr && EVP_DigestInit_ex(md_ctx,
EVP_sha256(), nullptr) == 1 &&
+ EVP_DigestUpdate(md_ctx, label.data(), label.size()) == 1 &&
EVP_DigestUpdate(md_ctx, secret, secret_len) == 1 &&
+ EVP_DigestUpdate(md_ctx, cert_digest, cert_digest_len) == 1
&& EVP_DigestFinal_ex(md_ctx, md, &md_len) == 1 &&
+ md_len >= out_len;
+
+ EVP_MD_CTX_free(md_ctx);
+ if (ok) {
+ memcpy(out, md, out_len);
+ }
+ OPENSSL_cleanse(md, sizeof(md));
+ return ok;
+}
+
+/** Derive the keys protecting the tickets of one certificate context from the
global keys.
+
+ The global keys are shared by every certificate context, so on their own a
ticket issued under
+ one certificate would resume under any other. Mixing in a digest of the
context's certificate
+ confines each ticket to contexts serving that certificate, while servers
that share both the
+ ticket keys and the certificate still derive the same keys and resume each
other's tickets.
+ The key names are kept so that key rotation works as it does for the
global keys.
+
+ @return The derived keys, or null if the context has no certificate to
bind them to.
Review Comment:
Updated: the `@return` now lists all three null cases: no certificate, a
failed certificate digest, and a failed key derivation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -54,6 +58,72 @@ namespace
{
DbgCtl dbg_ctl_ssl_session_ticket{"ssl_session_ticket"};
+using unique_ssl_ticket_key_block = std::unique_ptr<ssl_ticket_key_block, void
(*)(void *)>;
+
+/** Derive one ticket secret from a global secret and a certificate digest.
+
+ @return Whether the derivation succeeded.
+ */
+bool
+derive_ticket_secret(std::string_view label, const unsigned char *secret,
size_t secret_len, const unsigned char *cert_digest,
+ unsigned cert_digest_len, unsigned char *out, size_t
out_len)
+{
+ unsigned char md[EVP_MAX_MD_SIZE];
+ unsigned md_len = 0;
+ EVP_MD_CTX *md_ctx = EVP_MD_CTX_new();
+ bool const ok = md_ctx != nullptr && EVP_DigestInit_ex(md_ctx,
EVP_sha256(), nullptr) == 1 &&
+ EVP_DigestUpdate(md_ctx, label.data(), label.size()) == 1 &&
EVP_DigestUpdate(md_ctx, secret, secret_len) == 1 &&
+ EVP_DigestUpdate(md_ctx, cert_digest, cert_digest_len) == 1
&& EVP_DigestFinal_ex(md_ctx, md, &md_len) == 1 &&
+ md_len >= out_len;
+
+ EVP_MD_CTX_free(md_ctx);
+ if (ok) {
+ memcpy(out, md, out_len);
+ }
+ OPENSSL_cleanse(md, sizeof(md));
+ return ok;
+}
+
+/** Derive the keys protecting the tickets of one certificate context from the
global keys.
+
+ The global keys are shared by every certificate context, so on their own a
ticket issued under
+ one certificate would resume under any other. Mixing in a digest of the
context's certificate
+ confines each ticket to contexts serving that certificate, while servers
that share both the
+ ticket keys and the certificate still derive the same keys and resume each
other's tickets.
+ The key names are kept so that key rotation works as it does for the
global keys.
+
+ @return The derived keys, or null if the context has no certificate to
bind them to.
+ */
+unique_ssl_ticket_key_block
+ticket_keyblock_for_certificate(const ssl_ticket_key_block &global,
SSLCertContext &cc)
+{
+ unique_ssl_ticket_key_block derived{nullptr, ticket_block_free};
+ shared_SSL_CTX ctx = cc.getCtx();
+ X509 *cert = ctx ? SSL_CTX_get0_certificate(ctx.get())
: nullptr;
Review Comment:
Added both to the `proxy.config.ssl.server.ticket_key.filename` entry in
`records.yaml.en.rst`, together with a sentence on the certificate binding
itself. Certificate renewal invalidates outstanding tickets for the address,
and a partial rollout splits resumption. With dual RSA and ECDSA certificates,
the certificate loaded last is used, so servers must list them in the same
order.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
