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