You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/09/25 19:26:23 UTC
[PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
bneradt opened a new pull request, #13736:
URL: https://github.com/apache/trafficserver/pull/13736
Since #13677, every certificate context protects its session tickets
with the global ticket keys. For a client that sends no SNI, the
certificate is chosen by destination address, so a ticket issued on one
address now resumes on another address serving a different certificate,
and the client is never shown that certificate. The per-context random
keys that #13677 removed were the only thing keeping those tickets
apart.
This patch derives the ticket keys for a context selected by
destination address from the global keys and a digest of that context's
certificate. A ticket from another certificate then fails its HMAC
check and the client falls back to a full handshake. Servers sharing
the ticket key file and serving the same certificate derive the same
keys, so they keep resuming each other's tickets as #13677 intended.
Contexts not selected by address use the global keys as before.
Fixes: #13735
Co-Authored-By: Claude Opus 5.5 <no...@anthropic.com>
🤖 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4124574159
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -4642,6 +4642,19 @@ SSL Termination
note that OpenSSL session tickets are sensitive to the version of the ca-certificates. Once the
file is changed with new tickets, use :option:`traffic_ctl config reload` to begin using them.
+ For a connection to an address with its own ``dest_ip`` entry in :file:`ssl_multicert.yaml`,
+ the ticket keys are derived from these keys and that entry's certificate. This applies to every
+ client, including one whose SNI selects a different certificate, so a ticket resumes only on
+ addresses whose ``dest_ip`` entries serve the same certificate. Servers sharing this file resume
+ each other's tickets for such an address only if they serve the same certificate. A ``dest_ip``
+ entry with ``action: tunnel`` has no certificate, so connections to it use these keys as they
+ are. Two consequences follow:
Review Comment:
Nit: "Two consequences follow:" now comes straight after the tunnel sentence, so it reads as if the two bullets are consequences of the tunnel exception. Moving the `action: tunnel` sentence below the bullets would fix that.
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +186,30 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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, and so do connections to a tunnel entry, which deliberately has no
+ // certificate and terminates only the clients another entry serves by SNI.
+ const IpEndpoint &ip = this->_getCertLookupEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr && cc->opt != SSLCertContextOption::OPT_TUNNEL) {
Review Comment:
Optional tightening. Tunnel addresses now share the global keys with addresses that have no `dest_ip` entry, which is what master does. As a result, a client that sends no SNI can resume on a tunnel address where its own full handshake always fails (`no suitable signature algorithm`).
Two examples I ran: no-SNI at an address with no entry, then no-SNI at a tunnel address, resumes. So does SNI `foo.com` at a tunnel address followed by no-SNI at an address with no entry.
The per-certificate keys stay separate: a session from a tunnel address does not resume on the `dest_ip` VIPs. So this is master's existing behaviour for addresses without an entry, not something this PR adds.
If you want to close it anyway: on an `OPT_TUNNEL` context, use the global keys only when `SSL_get_servername()` returns a name, and return 0 otherwise. No-SNI clients can't complete a handshake there, so nothing that works today would lose resumption. Fine as a follow-up.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,254 @@
+'''
+Test that TLS session tickets are partitioned by the certificate selected by destination address.
+'''
+# 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
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for load balancer VIPs, one IPv4 pair and one IPv6 pair.
+ No request is routed to an origin: ATS answers each one itself, and only the
+ TLS handshake and the resumption it logs matter here.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
+ _first_vip6 = '2001:db8::1'
+ _second_vip6 = '2001:db8::2'
+ _first_cn = 'random.server.com'
+ _second_cn = 'foo.com'
+ _client = 'tls_resume_cert_partition_client.py'
+
+ def __init__(self) -> None:
+ '''Configure the ATS processes and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ Test.Setup.Copy(self._client)
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._started = False
+ self._second_legs: list[tuple['Process', str, bool]] = []
+
+ for tls in ('1.3', '1.2'):
+ self._add_run(
+ f'A no-SNI TLS {tls} session resumes on the certificate that issued it',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-first'),
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ f'A no-SNI TLS {tls} session does not resume on a different certificate',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/cross-{tls}-first'),
+ self._leg(self.ts, self._second_ip, f'/cross-{tls}-second'),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A PROXY protocol session resumes on the VIP whose certificate issued it',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-same-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-same-second', self._first_vip),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ 'A PROXY protocol session does not resume on a VIP with a different certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-cross-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-cross-second', self._second_vip),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'An IPv6 PROXY protocol session does not resume on a VIP with a different certificate',
Review Comment:
Heads-up: on macOS this file now fails often, mostly in this run. In 13 runs, 8 failed with `SSLEOFError: UNEXPECTED_EOF_WHILE_READING`, and 7 of those failures were in this IPv6 run. Linux CI is green.
Every failure has the same ATS-side pattern: `proxy protocol was enabled, but Proxy Protocol header was not present`, followed by `SSL accept ... wrong version number`. The cause looks older than this PR. In `SSLNetVConnection::read_raw_data()`, `haveCheckedProxyProtocol = true` is set on the first read even when it returns no bytes (EAGAIN). A PROXY header that arrives after that first read is then handed to OpenSSL as TLS.
Having the client wait 200 ms between connect and sending the header makes it fail 10 of 10 times. With the `ssl` debug tag on, the extra logging changes the timing and it passes 100 of 100.
The line comes from #12290 and isn't in this diff, so this doesn't block the PR. Setting the flag only once `r > 0` would probably fix it, as a separate change. Alternatively, the client could send the PROXY header and ClientHello in a single write so the test doesn't depend on the timing.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
Review Comment:
Verified, resolved.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4109247537
##########
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:
Resolved, the new wording matches the code. I left a separate note on the `records.yaml` sentence about SNI clients.
##########
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:
Resolved, the counter looks good. I left a follow-up in the new review on the Warning firing once per process and on its wording.
##########
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:
Resolved. Reusing the `EVP_MD_CTX` plus the note on why nothing is cached is fine by me; caching can wait for a follow-up.
##########
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:
Verified, resolved. It builds cleanly with the field and alias gone.
##########
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:
Resolved.
##########
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:
Resolved, and the dual-certificate note matches `load_certs`.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,188 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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 = '''
+Test that a session issued on one dest_ip-selected certificate is not resumed
+on a different one. Such connections send no SNI, so the server name in the
+session id context cannot distinguish them.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that resumption is partitioned by server certificate for no-SNI connections.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ The second address is the IPv6 loopback because it needs no interface
+ alias on any platform, unlike 127.0.0.2.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '[::1]'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS process, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._add_same_cert_run()
+ self._add_cross_cert_run()
+ self._add_shared_key_run()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {
+ 'headers': 'GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+
+ # Two certificates on one listener, chosen by destination address rather
+ # than by SNI. Neither entry sets ssl_ca_name, so the only thing that
+ # differs between the two connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_ip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
+
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.exec_thread.autoconfig.scale': 1.0,
+ 'proxy.config.ssl.server.session_ticket.enable': 1,
+ 'proxy.config.ssl.server.ticket_key.filename': self.ticket_file,
+ })
+ return ts
+
+ def _client_command(self, ip: str, session_arg: str, ts: 'Process' = None) -> str:
+ '''
+ Build a shell command that connects to the given address without sending an SNI.
+
+ :param ip: Destination address to connect to, which selects the certificate.
+ :param session_arg: The s_client argument saving or offering a session.
+ :param ts: The ATS process to connect to, by default the first one.
+ :return: Shell command performing the connection.
+ '''
+ ts = ts or self.ts
+ # The IPv6 loopback is on lo0 everywhere, whereas 127.0.0.2 needs an alias on macOS.
+ port = ts.Variables.ssl_portv6 if ip.startswith('[') else ts.Variables.ssl_port
+ request = 'printf "GET / HTTP/1.1\\r\\nHost: example.com\\r\\nConnection: close\\r\\n\\r\\n"'
+ # No -servername, so no SNI extension is sent and dest_ip selects the cert.
+ return (f'{request} | openssl s_client -connect {ip}:{port} -noservername '
+ f'{session_arg} -tls1_3 -ign_eof')
Review Comment:
Verified, resolved. The Python client and the TLS 1.2/1.3, PROXY and log-field runs all pass here.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4109300145
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
Review Comment:
Restored in `d7d2806b13`. `# limitations under the License.` is back.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != 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.
+ Metrics::Counter::increment(ssl_rsb.total_tickets_no_certificate);
+ static std::once_flag warned;
Review Comment:
Switched to `SiteThrottledWarning()` and dropped the `std::call_once`, so later addresses and ones that appear after a reload still get logged, at a bounded rate. The message is now worded generally: "could not bind session ticket keys to the certificate for %s, not using session tickets".
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
doc/admin-guide/monitoring/statistics/core/ssl.en.rst:
##########
@@ -170,6 +170,13 @@ SSL/TLS
.. ts:stat:: global proxy.process.ssl.total_tickets_created integer
:type: counter
+.. ts:stat:: global proxy.process.ssl.total_tickets_no_certificate integer
+ :type: counter
+
+ The number of session ticket operations skipped because the certificate context selected by
Review Comment:
Updated: "…has no certificate to bind the ticket keys to, or deriving the keys from it failed." It also notes that `action: tunnel` entries are not counted, per the change on your tunnel comment.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -4642,6 +4642,16 @@ SSL Termination
note that OpenSSL session tickets are sensitive to the version of the ca-certificates. Once the
file is changed with new tickets, use :option:`traffic_ctl config reload` to begin using them.
+ For a connection to an address with its own ``dest_ip`` entry in :file:`ssl_multicert.yaml`,
+ the ticket keys are derived from these keys and that entry's certificate, so a ticket resumes
+ only where the same certificate is served. Servers sharing this file resume each other's tickets
Review Comment:
Taken, close to your wording: the derivation "applies to every client, including one whose SNI selects a different certificate, so a ticket resumes only on addresses whose `dest_ip` entries serve the same certificate." Agreed that this matches releases before #13677, so the design stays.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr) {
+ cert_keyblock = ticket_keyblock_for_certificate(*keyblock, *cc);
+ if (cert_keyblock == nullptr) {
Review Comment:
Took the alternative. A context with `opt == OPT_TUNNEL` is now treated like an address with no context of its own: it uses the global keys and is not counted. SNI clients served by another entry on that address resume as they did on master, and the warning no longer fires for a deliberate tunnel. The derived-key path, and the counter and warning for a genuine binding failure, now apply only to terminating entries. The docs say this in both places.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
Review Comment:
Added an IPv6 VIP pair, `2001:db8::1` / `2001:db8::2`, sent as PROXY TCP6, in the run "An IPv6 PROXY protocol session does not resume on a VIP with a different certificate". I checked that it covers the fix: with `_lookupContextByIP()` put back to `ip.sa = proxy_protocol_dst->sa`, that run fails because the second VIP is served the default certificate instead of `foo.com`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
src/iocore/net/P_SSLNetVConnection.h:
##########
@@ -337,6 +337,11 @@ class SSLNetVConnection : public UnixNetVConnection,
const IpEndpoint &
_getLocalEndpoint() override
{
+ // The certificate is chosen by the PROXY destination address when there is one, and the
Review Comment:
Renamed it to `_getCertLookupEndpoint()` across the SSL and both QUIC overrides. The pure virtual now has a doc comment: it is the local socket address, except where the certificate is chosen by another address such as a PROXY destination, and the ticket keys follow the same address.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
Review Comment:
Updated: "Test that TLS session tickets are partitioned by the certificate selected by destination address."
🤖 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4108986703
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,188 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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 = '''
+Test that a session issued on one dest_ip-selected certificate is not resumed
+on a different one. Such connections send no SNI, so the server name in the
+session id context cannot distinguish them.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that resumption is partitioned by server certificate for no-SNI connections.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ The second address is the IPv6 loopback because it needs no interface
+ alias on any platform, unlike 127.0.0.2.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '[::1]'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS process, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._add_same_cert_run()
+ self._add_cross_cert_run()
+ self._add_shared_key_run()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {
+ 'headers': 'GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+
+ # Two certificates on one listener, chosen by destination address rather
+ # than by SNI. Neither entry sets ssl_ca_name, so the only thing that
+ # differs between the two connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_ip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
+
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.exec_thread.autoconfig.scale': 1.0,
+ 'proxy.config.ssl.server.session_ticket.enable': 1,
+ 'proxy.config.ssl.server.ticket_key.filename': self.ticket_file,
+ })
+ return ts
+
+ def _client_command(self, ip: str, session_arg: str, ts: 'Process' = None) -> str:
+ '''
+ Build a shell command that connects to the given address without sending an SNI.
+
+ :param ip: Destination address to connect to, which selects the certificate.
+ :param session_arg: The s_client argument saving or offering a session.
+ :param ts: The ATS process to connect to, by default the first one.
+ :return: Shell command performing the connection.
+ '''
+ ts = ts or self.ts
+ # The IPv6 loopback is on lo0 everywhere, whereas 127.0.0.2 needs an alias on macOS.
+ port = ts.Variables.ssl_portv6 if ip.startswith('[') else ts.Variables.ssl_port
+ request = 'printf "GET / HTTP/1.1\\r\\nHost: example.com\\r\\nConnection: close\\r\\n\\r\\n"'
+ # No -servername, so no SNI extension is sent and dest_ip selects the cert.
+ return (f'{request} | openssl s_client -connect {ip}:{port} -noservername '
+ f'{session_arg} -tls1_3 -ign_eof')
Review Comment:
Both addressed. `s_client` is replaced by a small Python client, `tls_resume_cert_partition_client.py`. It sends no SNI, verifies against the test CAs so it can report the served certificate's CN, optionally sends a PROXY v1 header, and runs both legs in one process. Since it uses Python's `ssl` module rather than whatever `openssl` binary is first on `PATH`, the LibreSSL problem goes away.
The same-certificate and cross-certificate runs now exist for both `-tls 1.2` and `1.3`, alongside the PROXY and shared-key runs.
🤖 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4109246450
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
Review Comment:
This update dropped the last line of the license header, `# limitations under the License.`, so the block now ends mid-sentence at "...permissions and". RAT still passes, but please restore it before merge.
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -4642,6 +4642,16 @@ SSL Termination
note that OpenSSL session tickets are sensitive to the version of the ca-certificates. Once the
file is changed with new tickets, use :option:`traffic_ctl config reload` to begin using them.
+ For a connection to an address with its own ``dest_ip`` entry in :file:`ssl_multicert.yaml`,
+ the ticket keys are derived from these keys and that entry's certificate, so a ticket resumes
+ only where the same certificate is served. Servers sharing this file resume each other's tickets
Review Comment:
This reads as if the binding follows the certificate actually served, but it follows the `dest_ip` entry's certificate for every client, SNI clients included. I ran a client sending SNI `foo.com` to two VIPs whose `dest_ip` entries have different certificates. Both legs were served `foo.com`, but the session never resumed across them (0 of 7 tries). On master it resumed every time.
That matches releases before #13677, where each `dest_ip` address had its own random keys, so I don't think the design needs to change. The doc should just say it, e.g. "...the ticket keys are derived from these keys and that entry's certificate, for every client including those whose SNI selects a different certificate, so a ticket resumes only on addresses whose `dest_ip` entries serve the same certificate."
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != 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.
+ Metrics::Counter::increment(ssl_rsb.total_tickets_no_certificate);
+ static std::once_flag warned;
Review Comment:
Because `warned` is a function-level static, this fires once for the whole process. I tried two `dest_ip` entries without a certificate and got a single Warning naming only the first address. A second address, or one that turns up after a config reload, only ever shows in the counter.
`SiteThrottledWarning()` (`include/tscore/Diags.h`) might fit better. It repeats at a bounded rate per call site, so later addresses still show up without flooding the log, and the HTTP/2 sessions already use it.
Also, this path covers more than a missing certificate: `X509_digest()`, `EVP_MD_CTX_new()` and the derivations can fail too. Consider wording the message more generally, e.g. "could not bind session ticket keys to the certificate for %s".
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
+ _first_cn = 'random.server.com'
+ _second_cn = 'foo.com'
+ _client = 'tls_resume_cert_partition_client.py'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS processes, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ Test.Setup.Copy(self._client)
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._started = False
+ self._second_legs: list[tuple['Process', str, bool]] = []
+
+ for tls in ('1.3', '1.2'):
+ self._add_run(
+ f'A no-SNI TLS {tls} session resumes on the certificate that issued it',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-first'),
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ f'A no-SNI TLS {tls} session does not resume on a different certificate',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/cross-{tls}-first'),
+ self._leg(self.ts, self._second_ip, f'/cross-{tls}-second'),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A PROXY protocol session resumes on the VIP whose certificate issued it',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-same-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-same-second', self._first_vip),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ 'A PROXY protocol session does not resume on a VIP with a different certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-cross-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-cross-second', self._second_vip),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A no-SNI session resumes on another server sharing the ticket keys and certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/shared-first'),
+ self._leg(self.ts2, self._first_ip, '/shared-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_log_checks()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {'headers': 'GET / HTTP/1.1\r\nHost: example.com\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True, enable_proxy_protocol=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+ # Only for the client, which verifies so that it can report which certificate it was served.
+ ts.addSSLfile('ssl/signer.pem')
+
+ # Certificates chosen by destination address rather than by SNI. Neither
+ # certificate is chosen by name, so the only thing that differs between two
+ # connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "[{self._second_ip}]"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "{self._first_vip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_vip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
Review Comment:
Nit: every request in the test gets a 404 because this `map /` never matches, so the origin server is never reached. The assertions don't depend on the response, so the origin and this remap line could be dropped. Or fix the map if you want a 200.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
Review Comment:
Since this PR fixes the IPv6 PROXY destination truncation in `_lookupContextByIP()`, it would be nice to cover it here. An IPv6 documentation VIP pair (e.g. `2001:db8::1` / `2001:db8::2`) sent as PROXY TCP6 would do. Both VIPs here are IPv4, so nothing in the suite exercises that fix.
##########
src/iocore/net/P_SSLNetVConnection.h:
##########
@@ -337,6 +337,11 @@ class SSLNetVConnection : public UnixNetVConnection,
const IpEndpoint &
_getLocalEndpoint() override
{
+ // The certificate is chosen by the PROXY destination address when there is one, and the
Review Comment:
Nit: with this override, `_getLocalEndpoint()` returns the PROXY destination rather than the socket address, while the QUIC overrides still return `local_addr`. Its only caller is `processSessionTicket()`, so nothing breaks today. A name like `_getCertLookupEndpoint()`, or a doc comment on the pure virtual in `TLSSessionResumptionSupport.h`, would keep a future caller from assuming it is the socket address.
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr) {
+ cert_keyblock = ticket_keyblock_for_certificate(*keyblock, *cc);
+ if (cert_keyblock == nullptr) {
Review Comment:
A related edge case: a `dest_ip` entry with `action: tunnel` has no certificate. So on a non-transparent port, tickets are now off for every connection to that address, including SNI clients served a real certificate from another entry. With SNI `foo.com` on such an address, master resumed and this branch didn't, and it logged the Warning. It's a rare setup, so a sentence in the docs would do. Alternatively, a context that deliberately has no certificate could be left out of the "no certificate" count.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
Review Comment:
Nit: this module docstring is still the original one-liner. `Test.Summary` below describes the full matrix of runs now.
##########
doc/admin-guide/monitoring/statistics/core/ssl.en.rst:
##########
@@ -170,6 +170,13 @@ SSL/TLS
.. ts:stat:: global proxy.process.ssl.total_tickets_created integer
:type: counter
+.. ts:stat:: global proxy.process.ssl.total_tickets_no_certificate integer
+ :type: counter
+
+ The number of session ticket operations skipped because the certificate context selected by
Review Comment:
Same point as on the Warning: this counter also increments when the certificate digest, the digest context allocation or a key derivation fails, not only when there is no certificate. It might be worth saying "has no certificate to bind the ticket keys to, or deriving the keys from it failed".
##########
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:
Verified, resolved. With the global key names put back, the cross-cert legs log `1 2`, and the new log checks catch it. With your change they log `0 0`.
##########
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:
Verified, resolved. Disabling the override makes the cross-VIP run fail (`reused=True`, logged `1 2`), so the test covers it. The IPv6 truncation fix also checks out at runtime; I left a note in the new review about adding an IPv6 VIP to the test.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4109300720
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
+ _first_cn = 'random.server.com'
+ _second_cn = 'foo.com'
+ _client = 'tls_resume_cert_partition_client.py'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS processes, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ Test.Setup.Copy(self._client)
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._started = False
+ self._second_legs: list[tuple['Process', str, bool]] = []
+
+ for tls in ('1.3', '1.2'):
+ self._add_run(
+ f'A no-SNI TLS {tls} session resumes on the certificate that issued it',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-first'),
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ f'A no-SNI TLS {tls} session does not resume on a different certificate',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/cross-{tls}-first'),
+ self._leg(self.ts, self._second_ip, f'/cross-{tls}-second'),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A PROXY protocol session resumes on the VIP whose certificate issued it',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-same-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-same-second', self._first_vip),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ 'A PROXY protocol session does not resume on a VIP with a different certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-cross-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-cross-second', self._second_vip),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A no-SNI session resumes on another server sharing the ticket keys and certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/shared-first'),
+ self._leg(self.ts2, self._first_ip, '/shared-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_log_checks()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {'headers': 'GET / HTTP/1.1\r\nHost: example.com\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True, enable_proxy_protocol=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+ # Only for the client, which verifies so that it can report which certificate it was served.
+ ts.addSSLfile('ssl/signer.pem')
+
+ # Certificates chosen by destination address rather than by SNI. Neither
+ # certificate is chosen by name, so the only thing that differs between two
+ # connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "[{self._second_ip}]"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "{self._first_vip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_vip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
Review Comment:
Dropped the origin server and the remap line. The class docstring now says ATS answers each request itself and only the handshake and its logged resumption matter.
🤖 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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4124576972
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != 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.
+ Metrics::Counter::increment(ssl_rsb.total_tickets_no_certificate);
+ static std::once_flag warned;
Review Comment:
Verified, resolved. At the default throttle interval that is one line per 60 s for the site, which the unthrottled counter backs up.
##########
doc/admin-guide/monitoring/statistics/core/ssl.en.rst:
##########
@@ -170,6 +170,13 @@ SSL/TLS
.. ts:stat:: global proxy.process.ssl.total_tickets_created integer
:type: counter
+.. ts:stat:: global proxy.process.ssl.total_tickets_no_certificate integer
+ :type: counter
+
+ The number of session ticket operations skipped because the certificate context selected by
Review Comment:
Verified, resolved.
##########
doc/admin-guide/files/records.yaml.en.rst:
##########
@@ -4642,6 +4642,16 @@ SSL Termination
note that OpenSSL session tickets are sensitive to the version of the ca-certificates. Once the
file is changed with new tickets, use :option:`traffic_ctl config reload` to begin using them.
+ For a connection to an address with its own ``dest_ip`` entry in :file:`ssl_multicert.yaml`,
+ the ticket keys are derived from these keys and that entry's certificate, so a ticket resumes
+ only where the same certificate is served. Servers sharing this file resume each other's tickets
Review Comment:
Resolved. One ordering nit on the new tunnel sentence, left in the new review.
##########
src/iocore/net/TLSSessionResumptionSupport.cc:
##########
@@ -103,18 +187,32 @@ TLSSessionResumptionSupport::processSessionTicket(SSL *ssl, unsigned char *keyna
SSLCertificateConfig::scoped_config lookup;
SSLTicketKeyConfig::scoped_config params;
- // Get the IP address to look up the keyblock
- const IpEndpoint &ip = this->_getLocalEndpoint();
- SSLCertContext *cc = lookup->find(ip);
- ssl_ticket_key_block *keyblock = nullptr;
- if (cc == nullptr || cc->keyblock == nullptr) {
- // Try the default
- keyblock = params->default_global_keyblock;
- } else {
- keyblock = cc->keyblock.get();
- }
+ ssl_ticket_key_block *keyblock = params->default_global_keyblock;
ink_release_assert(keyblock != nullptr && keyblock->num_keys > 0);
+ // 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.
+ const IpEndpoint &ip = this->_getLocalEndpoint();
+ SSLCertContext *cc = lookup->find(ip);
+ unique_ssl_ticket_key_block cert_keyblock{nullptr, ticket_block_free};
+ if (cc != nullptr) {
+ cert_keyblock = ticket_keyblock_for_certificate(*keyblock, *cc);
+ if (cert_keyblock == nullptr) {
Review Comment:
Verified, resolved. SNI `foo.com` to a tunnel `dest_ip` now resumes on TLS 1.2 and 1.3, with no warning. I left one optional follow-up in the new review about no-SNI clients on tunnel addresses.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
Review Comment:
Verified, resolved. I restored the truncating copy and the run failed 2/2 on the second VIP's certificate.
##########
src/iocore/net/P_SSLNetVConnection.h:
##########
@@ -337,6 +337,11 @@ class SSLNetVConnection : public UnixNetVConnection,
const IpEndpoint &
_getLocalEndpoint() override
{
+ // The certificate is chosen by the PROXY destination address when there is one, and the
Review Comment:
Verified, resolved. No references to the old name are left in the tree.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
Review Comment:
Resolved.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4124578724
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,252 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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
+
+import os
+import re
+import sys
+
+Test.Summary = '''
+Test that a session ticket issued on one dest_ip-selected certificate is not
+resumed on a different one, while servers sharing the ticket keys and the
+certificate still resume each other's tickets.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that ticket resumption is partitioned by the certificate selected by destination address.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ Each run checks what the client observes, which certificate each leg is
+ served and whether it resumed, and the log checks that ATS records the
+ resumption the same way. The second address is the IPv6 loopback because it
+ needs no interface alias on any platform, unlike 127.0.0.2. For the PROXY
+ protocol runs the destination comes from the PROXY header, so documentation
+ addresses stand in for two load balancer VIPs.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '::1'
+ _first_vip = '192.0.2.1'
+ _second_vip = '192.0.2.2'
+ _first_cn = 'random.server.com'
+ _second_cn = 'foo.com'
+ _client = 'tls_resume_cert_partition_client.py'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS processes, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ Test.Setup.Copy(self._client)
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._started = False
+ self._second_legs: list[tuple['Process', str, bool]] = []
+
+ for tls in ('1.3', '1.2'):
+ self._add_run(
+ f'A no-SNI TLS {tls} session resumes on the certificate that issued it',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-first'),
+ self._leg(self.ts, self._first_ip, f'/same-{tls}-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ f'A no-SNI TLS {tls} session does not resume on a different certificate',
+ tls,
+ self._leg(self.ts, self._first_ip, f'/cross-{tls}-first'),
+ self._leg(self.ts, self._second_ip, f'/cross-{tls}-second'),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A PROXY protocol session resumes on the VIP whose certificate issued it',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-same-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-same-second', self._first_vip),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_run(
+ 'A PROXY protocol session does not resume on a VIP with a different certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/proxy-cross-first', self._first_vip),
+ self._leg(self.ts, self._first_ip, '/proxy-cross-second', self._second_vip),
+ resumed=False,
+ cns=(self._first_cn, self._second_cn))
+ self._add_run(
+ 'A no-SNI session resumes on another server sharing the ticket keys and certificate',
+ '1.3',
+ self._leg(self.ts, self._first_ip, '/shared-first'),
+ self._leg(self.ts2, self._first_ip, '/shared-second'),
+ resumed=True,
+ cns=(self._first_cn, self._first_cn))
+ self._add_log_checks()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {'headers': 'GET / HTTP/1.1\r\nHost: example.com\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True, enable_proxy_protocol=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+ # Only for the client, which verifies so that it can report which certificate it was served.
+ ts.addSSLfile('ssl/signer.pem')
+
+ # Certificates chosen by destination address rather than by SNI. Neither
+ # certificate is chosen by name, so the only thing that differs between two
+ # connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "[{self._second_ip}]"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "{self._first_vip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_vip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
Review Comment:
Verified, resolved. ATS answers 404 itself, and the TLS 1.3 tickets are still captured.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#issuecomment-5940618109
Cherry-picked to the 10.2.x branch as 768e4eab5a29e6df5c7cc3f57274b033336a8090 for the 10.2.1 release.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bind session ticket keys to the dest_ip certificate [trafficserver]
Posted by "bryancall (via GitHub)" <gi...@apache.org>.
bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4108827072
##########
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:
A small precision point on this comment and the PR description. `lookup->find(ip)` only returns a context for an address with an explicit `dest_ip` entry; `*` is stored under the name key and never matched here. So this path runs for every connection to such an address, SNI connections included, and those are bound to the address's certificate rather than the certificate SNI selected. The code can't tell whether SNI was sent at this point.
That's reasonable behavior, but the comment reads as if it only applies to no-SNI clients. Maybe something like: "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, so bind the global keys to that context's certificate."
The PR description is fine on the `cc == nullptr` side ("Contexts not selected by address use the global keys as before").
##########
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:
Because the derived block keeps the global key names, a ticket issued under a different certificate still matches by name in `_getSessionInformation()`. That function increments `total_tickets_verified`, calls `_setResumptionType(RESUMED_FROM_SESSION_TICKET)` and returns 2. OpenSSL only checks the HMAC after the callback returns; the check fails and it does a full handshake, but the resumption type is already set.
I ran the new test with `%<cqssr> %<cqssrt>` added to the log format. On the cross-cert leg, `s_client` reports `New, TLSv1.3`, but ATS logs `1 2`. So `cqssr`/`cqssrt`, `TSVConnIsSslReused()` and `proxy.process.ssl.total_tickets_verified` all over-report whenever a ticket from another certificate is offered.
Could the key name be derived as well? For example, the first 16 bytes of `SHA256("ATS session ticket name" || key_name || cert_digest)`. That stays deterministic across servers and keeps the rotation order at the same index. A ticket from another certificate would then take the `keyname is not consistent` path and count as `total_tickets_not_found`. It would also be worth asserting in the test that `cqssr` is 0 on the cross-cert leg.
##########
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:
Not blocking: this recomputes on every ticket operation. That's an `X509_digest`, two SHA-256 passes per key with a fresh `EVP_MD_CTX` each, and an alloc/free of the block. A standalone benchmark of the same steps measured about 1.4 µs per derivation with one key. A resumed TLS 1.3 handshake does about three of them (one decrypt, two new tickets), which comes to roughly 10% on top of the ECDHE cost of a resumed handshake.
Caching in `cc->keyblock` wouldn't work directly: the ticket keys reload independently of `ssl_multicert` (file change, `TSSslTicketKeyUpdate`), and a non-null `cc->keyblock` means "explicit keys" at line 180. One option is to compute the certificate digest once per context at load time and cache derived blocks in `SSLTicketParams`, keyed by digest and rebuilt on ticket reload. A cheaper step is to reuse one `EVP_MD_CTX` across the keys. Either way, a note that recomputing per call is what keeps rotation correct would stop someone caching it naively later.
##########
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:
For a connection on a proxy-protocol port, `SSLNetVConnection::_lookupContextByIP()` chooses the certificate by the PROXY destination address (`get_proxy_protocol_dst_addr()`). Here, `cc` comes from `_getLocalEndpoint()`, which is the socket's `local_addr`. The two can disagree.
Consider a load balancer sending two VIPs, each with its own `dest_ip` certificate, to one ATS listener address:
- If the listener address has no `dest_ip` entry, `cc` is null and both VIPs use the global keys.
- If it has one, both VIPs bind to that entry's certificate.
Either way, the separation this PR adds doesn't follow the certificate actually served. Could the lookup here use the same address selection as `_lookupContextByIP()`? A run on the proxy-protocol SSL port would cover it.
##########
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:
This condition is per-context and permanent: every handshake to that address loses tickets for the life of the process, and the only signal is a `Dbg` line that's off by default. It can happen with a `dest_ip` entry that has no certificate on the `SSL_CTX`, e.g. `action: tunnel`. Every other outcome in this file increments an `ssl_rsb.*` counter (`total_tickets_not_found`, `total_tickets_verified`, ...). Could this get one as well, plus possibly a one-time `Warning()` naming the address? That way an operator can see tickets being turned off.
##########
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:
Null is also returned when `X509_digest()` or either `derive_ticket_secret()` call fails, not only when there is no certificate. The caller treats all of these the same way, so it would help to list them here.
##########
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:
Nothing sets `SSLCertContext::keyblock` any more. Since #13677, only the three-argument constructor is used (`SSLUtils.cc` lines 1780/1789/1803), so the four-argument constructor has no callers. `cc->keyblock == nullptr` is therefore always true here, and the `else keyblock = cc->keyblock.get()` branch above is unreachable. Either drop the dead branch and field, or simplify this condition to `cc != nullptr` with a note. As written, it suggests there is still a configured per-context key path.
##########
tests/gold_tests/tls/tls_resume_cert_partition.test.py:
##########
@@ -0,0 +1,188 @@
+'''
+Test that a TLS session is not resumed against a different server certificate when no SNI is sent.
+'''
+# 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 = '''
+Test that a session issued on one dest_ip-selected certificate is not resumed
+on a different one. Such connections send no SNI, so the server name in the
+session id context cannot distinguish them.
+'''
+
+Test.SkipUnless(Condition.HasOpenSSLVersion('1.1.1'))
+
+
+class TlsResumeCertPartition:
+ '''
+ Test that resumption is partitioned by server certificate for no-SNI connections.
+
+ A client that sends no SNI has its certificate chosen by destination
+ address, so the server name cannot tell two such connections apart.
+ Session tickets for every certificate are protected by the same globally
+ configured ticket keys, so a ticket carries nothing tying it to the
+ certificate that issued it unless the keys used for it depend on that
+ certificate. Without that, a ticket issued on one address resumes on another
+ address serving a different certificate, and the client skips the
+ certificate it would otherwise have been shown.
+
+ The second address is the IPv6 loopback because it needs no interface
+ alias on any platform, unlike 127.0.0.2.
+ '''
+
+ _first_ip = '127.0.0.1'
+ _second_ip = '[::1]'
+
+ def __init__(self) -> None:
+ '''Configure the origin server, ATS process, and test runs.'''
+ Test.Setup.Copy('file.ticket')
+ self.ticket_file = os.path.join(Test.RunDirectory, 'file.ticket')
+ self._configure_server()
+ self.ts = self._configure_ts('ts')
+ # A second instance sharing the ticket key file stands in for another server in a fleet.
+ self.ts2 = self._configure_ts('ts2')
+ self._add_same_cert_run()
+ self._add_cross_cert_run()
+ self._add_shared_key_run()
+
+ def _configure_server(self) -> None:
+ '''Configure the origin server with a simple response.'''
+ server = Test.MakeOriginServer('server')
+ request_header = {
+ 'headers': 'GET / HTTP/1.1\r\nHost: example.com\r\nConnection: close\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': 'hello'
+ }
+ server.addResponse('sessionlog.json', request_header, response_header)
+ self.server = server
+
+ def _configure_ts(self, name: str) -> 'Process':
+ '''
+ Configure an ATS process with a different certificate per destination address.
+
+ :param name: Name of the ATS process.
+ :return: The configured ATS process.
+ '''
+ ts = Test.MakeATSProcess(name, enable_tls=True)
+ ts.addSSLfile('ssl/server.pem')
+ ts.addSSLfile('ssl/server.key')
+ ts.addSSLfile('ssl/signed-foo.pem')
+ ts.addSSLfile('ssl/signed-foo.key')
+
+ # Two certificates on one listener, chosen by destination address rather
+ # than by SNI. Neither entry sets ssl_ca_name, so the only thing that
+ # differs between the two connections is which certificate is served.
+ ts.Disk.ssl_multicert_yaml.AddLines(
+ f"""
+ssl_multicert:
+ - dest_ip: "{self._first_ip}"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+ - dest_ip: "{self._second_ip}"
+ ssl_cert_name: signed-foo.pem
+ ssl_key_name: signed-foo.key
+ - dest_ip: "*"
+ ssl_cert_name: server.pem
+ ssl_key_name: server.key
+""".split("\n"))
+
+ ts.Disk.remap_config.AddLine(f'map / http://127.0.0.1:{self.server.Variables.Port}')
+
+ ts.Disk.records_config.update(
+ {
+ 'proxy.config.ssl.server.cert.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.ssl.server.private_key.path': f'{ts.Variables.SSLDir}',
+ 'proxy.config.exec_thread.autoconfig.scale': 1.0,
+ 'proxy.config.ssl.server.session_ticket.enable': 1,
+ 'proxy.config.ssl.server.ticket_key.filename': self.ticket_file,
+ })
+ return ts
+
+ def _client_command(self, ip: str, session_arg: str, ts: 'Process' = None) -> str:
+ '''
+ Build a shell command that connects to the given address without sending an SNI.
+
+ :param ip: Destination address to connect to, which selects the certificate.
+ :param session_arg: The s_client argument saving or offering a session.
+ :param ts: The ATS process to connect to, by default the first one.
+ :return: Shell command performing the connection.
+ '''
+ ts = ts or self.ts
+ # The IPv6 loopback is on lo0 everywhere, whereas 127.0.0.2 needs an alias on macOS.
+ port = ts.Variables.ssl_portv6 if ip.startswith('[') else ts.Variables.ssl_port
+ request = 'printf "GET / HTTP/1.1\\r\\nHost: example.com\\r\\nConnection: close\\r\\n\\r\\n"'
+ # No -servername, so no SNI extension is sent and dest_ip selects the cert.
+ return (f'{request} | openssl s_client -connect {ip}:{port} -noservername '
+ f'{session_arg} -tls1_3 -ign_eof')
Review Comment:
Two small things here:
- Only TLS 1.3 is exercised, but `processSessionTicket()` also handles TLS 1.2 tickets. A `-tls1_2` variant of the same-cert and cross-cert runs would cover that; `tls_sni_ticket.test.py` already matches `Reused, TLSv1.2`.
- `-noservername` depends on the `openssl` binary on `PATH`, while `Condition.HasOpenSSLVersion` checks the library ATS links. On macOS, `/usr/bin/openssl` is LibreSSL, which prints usage, so the test fails instead of skipping unless Homebrew OpenSSL comes first on `PATH`.
##########
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:
Two operational effects that might be worth a line in the docs or release notes:
- With dual RSA and ECDSA certificates on one line, `SSL_CTX_get0_certificate()` returns the last certificate loaded. Servers only derive the same keys if they list the certificates in the same order.
- Renewing the certificate changes the digest. That invalidates outstanding tickets for that address, and servers can't resume each other's tickets while a certificate rollout is only partly done.
--
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: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org