bryancall commented on code in PR #13736:
URL: https://github.com/apache/trafficserver/pull/13736#discussion_r4108827072
##########
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:
A small precision point on this comment and the PR description.
`lookup->find(ip)` only returns a context for an address with an explicit
`dest_ip` entry; `*` is stored under the name key and never matched here. So
this path runs for every connection to such an address, SNI connections
included, and those are bound to the address's certificate rather than the
certificate SNI selected. The code can't tell whether SNI was sent at this
point.
That's reasonable behavior, but the comment reads as if it only applies to
no-SNI clients. Maybe something like: "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, so bind the global keys to that context's certificate."
The PR description is fine on the `cc == nullptr` side ("Contexts not
selected by address use the global keys as before").
##########
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:
Because the derived block keeps the global key names, a ticket issued under
a different certificate still matches by name in `_getSessionInformation()`.
That function increments `total_tickets_verified`, calls
`_setResumptionType(RESUMED_FROM_SESSION_TICKET)` and returns 2. OpenSSL only
checks the HMAC after the callback returns; the check fails and it does a full
handshake, but the resumption type is already set.
I ran the new test with `%<cqssr> %<cqssrt>` added to the log format. On the
cross-cert leg, `s_client` reports `New, TLSv1.3`, but ATS logs `1 2`. So
`cqssr`/`cqssrt`, `TSVConnIsSslReused()` and
`proxy.process.ssl.total_tickets_verified` all over-report whenever a ticket
from another certificate is offered.
Could the key name be derived as well? For example, the first 16 bytes of
`SHA256("ATS session ticket name" || key_name || cert_digest)`. That stays
deterministic across servers and keeps the rotation order at the same index. A
ticket from another certificate would then take the `keyname is not consistent`
path and count as `total_tickets_not_found`. It would also be worth asserting
in the test that `cqssr` is 0 on the cross-cert leg.
##########
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:
Not blocking: this recomputes on every ticket operation. That's an
`X509_digest`, two SHA-256 passes per key with a fresh `EVP_MD_CTX` each, and
an alloc/free of the block. A standalone benchmark of the same steps measured
about 1.4 µs per derivation with one key. A resumed TLS 1.3 handshake does
about three of them (one decrypt, two new tickets), which comes to roughly 10%
on top of the ECDHE cost of a resumed handshake.
Caching in `cc->keyblock` wouldn't work directly: the ticket keys reload
independently of `ssl_multicert` (file change, `TSSslTicketKeyUpdate`), and a
non-null `cc->keyblock` means "explicit keys" at line 180. One option is to
compute the certificate digest once per context at load time and cache derived
blocks in `SSLTicketParams`, keyed by digest and rebuilt on ticket reload. A
cheaper step is to reuse one `EVP_MD_CTX` across the keys. Either way, a note
that recomputing per call is what keeps rotation correct would stop someone
caching it naively later.
##########
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:
For a connection on a proxy-protocol port,
`SSLNetVConnection::_lookupContextByIP()` chooses the certificate by the PROXY
destination address (`get_proxy_protocol_dst_addr()`). Here, `cc` comes from
`_getLocalEndpoint()`, which is the socket's `local_addr`. The two can disagree.
Consider a load balancer sending two VIPs, each with its own `dest_ip`
certificate, to one ATS listener address:
- If the listener address has no `dest_ip` entry, `cc` is null and both VIPs
use the global keys.
- If it has one, both VIPs bind to that entry's certificate.
Either way, the separation this PR adds doesn't follow the certificate
actually served. Could the lookup here use the same address selection as
`_lookupContextByIP()`? A run on the proxy-protocol SSL port would cover it.
##########
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:
This condition is per-context and permanent: every handshake to that address
loses tickets for the life of the process, and the only signal is a `Dbg` line
that's off by default. It can happen with a `dest_ip` entry that has no
certificate on the `SSL_CTX`, e.g. `action: tunnel`. Every other outcome in
this file increments an `ssl_rsb.*` counter (`total_tickets_not_found`,
`total_tickets_verified`, ...). Could this get one as well, plus possibly a
one-time `Warning()` naming the address? That way an operator can see tickets
being turned off.
##########
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:
Null is also returned when `X509_digest()` or either
`derive_ticket_secret()` call fails, not only when there is no certificate. The
caller treats all of these the same way, so it would help to list them here.
##########
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:
Nothing sets `SSLCertContext::keyblock` any more. Since #13677, only the
three-argument constructor is used (`SSLUtils.cc` lines 1780/1789/1803), so the
four-argument constructor has no callers. `cc->keyblock == nullptr` is
therefore always true here, and the `else keyblock = cc->keyblock.get()` branch
above is unreachable. Either drop the dead branch and field, or simplify this
condition to `cc != nullptr` with a note. As written, it suggests there is
still a configured per-context key path.
##########
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:
Two small things here:
- Only TLS 1.3 is exercised, but `processSessionTicket()` also handles TLS
1.2 tickets. A `-tls1_2` variant of the same-cert and cross-cert runs would
cover that; `tls_sni_ticket.test.py` already matches `Reused, TLSv1.2`.
- `-noservername` depends on the `openssl` binary on `PATH`, while
`Condition.HasOpenSSLVersion` checks the library ATS links. On macOS,
`/usr/bin/openssl` is LibreSSL, which prints usage, so the test fails instead
of skipping unless Homebrew OpenSSL comes first on `PATH`.
##########
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:
Two operational effects that might be worth a line in the docs or release
notes:
- With dual RSA and ECDSA certificates on one line,
`SSL_CTX_get0_certificate()` returns the last certificate loaded. Servers only
derive the same keys if they list the certificates in the same order.
- Renewing the certificate changes the digest. That invalidates outstanding
tickets for that address, and servers can't resume each other's tickets while a
certificate rollout is only partly done.
--
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]
