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