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