maskit commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r4064610129
##########
src/iocore/net/SSLNetVConnection.cc:
##########
@@ -1283,6 +1283,18 @@ SSLNetVConnection::_sslStartHandShake(int event, int
&err)
SSL_set_verify(this->ssl, SSL_VERIFY_PEER, verify_callback);
+#if TS_USE_RPK
+ // Offer and/or pin RFC 7250 raw public keys when this next hop is
configured for them.
+ // Both are advertised alongside X.509, so a next hop that doesn't (yet)
support RPK -- a
+ // normal state during a rolling upgrade -- negotiates down to a
certificate exchange.
+ if (nps && (nps->client_rpk_enabled || nps->server_rpk_ca)) {
+ if (!ssl_client_setup_rpk(this->ssl, nps->client_rpk_enabled,
nps->server_rpk_ca)) {
Review Comment:
Fixed in 4febe1ac. Confirmed: the key is
`{sni_addr}:{SSL_CTX}:{verify_str}`, and the pinned set is reachable from
neither term. It is deliberately SSL-level rather than SSL_CTX-level ex_data,
and `SSLConfigParams::getCTX()` keys contexts on
`{ca_bundle_file}:{ca_bundle_path}` plus the client certificate, so replacing
`server_rpk_ca` rotates neither the context pointer nor the policy string. The
cache is also never flushed on reload.
Worth noting the asymmetry, since it is what makes this specific to raw
public keys rather than inherited: an X.509 trust change *does* rotate the
context pointer and so retires its cached sessions, while a pin-set change
rotated nothing.
The set is now identified in the key. Two choices in the fix worth flagging.
I digest the key material rather than using the parsed object's address, even
though the existing key already contains a raw `SSL_CTX` pointer, because an
address rotates on every reload and would make any `traffic_ctl config reload`
needlessly dump every cached origin session; a content digest means an
unchanged pin file keeps its sessions and a changed one loses them. And I added
the term inside `get_verify_str()` rather than at the two sites that build the
key, since both already call it -- the insert in `ssl_new_session_callback()`
and the lookup before `SSL_connect()` -- so they cannot drift apart. It is
taken once at config load into `NextHopProperty::server_rpk_ca_digest`, so
there is no per-handshake hashing.
Covered by a unit test that the digest is content-derived, stable across a
reparse of identical contents, and distinct for a different or a superset key
set, plus an autest assertion that the logged lookup key carries the term. I
verified the unit test fails when the digest ignores its input.
--
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]
