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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
