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