bryancall commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r4001928312
##########
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:
Agreed, and I verified this rather than taking it on faith. The origin
session cache key is `sni_addr : SSL_CTX : get_verify_str(ssl)`, and
`get_verify_str()` is `policy:property` with no pin-set identity in it. The
`SSL_CTX` pointer does not rescue it either: client contexts are cached on
`SSLConfigParams` keyed by certificate, key and CA paths, and a `sni.yaml`-only
reload does not rebuild those, so the pointer is stable across a
`server_rpk_ca` change. Same key, session resumes, no Certificate message, pin
never rechecked. Raised in the review as blocking: it needs the pin-set
identity in the key, or an explicit documented limitation.
##########
src/proxy/http/ConnectingEntry.cc:
##########
@@ -98,7 +99,16 @@ ConnectingEntry::state_http_server_open(int event, void
*data)
auto event = CONNECT_EVENT_TXN;
void *event_data = new_session;
- if (!validate_server_certificate_hostname(new_session->get_netvc(),
(*entry)->get_outbound_sni_for_cert_verification())) {
+ NetVConnection *session_netvc = new_session->get_netvc();
+ // A raw public key origin was authenticated by the next hop's pin
set, which belongs to
+ // the sni.yaml entry the outbound SNI selected, so only a
transaction that would send the
+ // same SNI may join this session.
+ bool const acceptable =
+ origin_pinned_raw_public_key(session_netvc) ?
+ ServerSessionPool::validate_sni(*entry, session_netvc) :
+ validate_server_certificate_hostname(session_netvc,
(*entry)->get_outbound_sni_for_cert_verification());
Review Comment:
The description is accurate, but I do not think this one is this PR to fix.
A pooled origin session surviving a config reload is existing behavior:
rotating a CA does not drain X.509 origin sessions either, since reuse rechecks
the name against the certificate already presented rather than reverifying the
chain. What differs for raw public keys is that the pin set is the whole of the
authentication, so the consequence is larger even though the mechanism is not
new. A documentation line rather than a change here.
##########
src/proxy/http/HttpSessionManager.cc:
##########
@@ -48,8 +48,20 @@ DbgCtl dbg_ctl_http_ss{"http_ss"};
bool
validate_session_origin_cert(HttpSM *sm, PoolableSession *session)
{
- return !session->is_multiplexing() ||
- validate_server_certificate_hostname(session->get_netvc(),
sm->get_outbound_sni_for_cert_verification());
+ if (!session->is_multiplexing()) {
+ return true;
+ }
+
+ NetVConnection *netvc = session->get_netvc();
+ if (origin_pinned_raw_public_key(netvc)) {
+ // A raw public key carries no SAN, so the next hop's pin set stood in for
the name check. That
+ // set belongs to the sni.yaml entry the outbound SNI selected, so this
session is only reusable
+ // for a request that would send the same SNI. Deliberately not gated on
the SNI match mask or
+ // on NAME being in verify_server_properties: the pin set is the whole of
what authenticated
+ // this origin.
Review Comment:
Same as the ConnectingEntry thread: accurate description, existing behavior
rather than something introduced here. Worth noting that the comment
immediately above this call already says the pin set is the whole of what
authenticated the origin, which is exactly why the stale-pin-set case reads
worse for raw public keys than for X.509. Documenting that limitation would
close it out.
--
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]
