maskit commented on code in PR #13548:
URL: https://github.com/apache/trafficserver/pull/13548#discussion_r4064669356
##########
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:
Not fixed, deliberately: this is pre-existing and general rather than
specific to raw public keys.
No server-trust configuration participates in pool reuse for X.509 either.
`validate_sni()` and `validate_host_sni()` compare names, `validate_cert()`
compares the *client* certificate, and there is no comparison of the server CA
anywhere. So changing `proxy.config.ssl.client.CA.cert.filename`, or
`verify_server_policy`, equally leaves pooled sessions in place. Nothing drains
the pool on an sni.yaml reload -- `purge_keepalives()` is reached only from an
HttpSM error path -- so the staleness window is the pool lifetime for every one
of these settings, not just the pin set.
The one thing this PR changes here is narrower than it looks: the reuse gate
for raw public key origins already compared SNI only, so the class of staleness
is not new. What is new is that the empty-SNI case now pools at all, because
the old comparison returned false on every call and those sessions were never
reused. That was the bug this PR was asked to fix, so the exposure it opens is
the intended behavior arriving, not a regression.
Fixing it properly means tracking a verification-scope generation on the
pooled session and invalidating on mismatch, which should cover the CA and the
policy as well as the pin set rather than treating raw public keys as a special
case. That is worth doing and I would rather it were a separate change than
bolted onto this one. Note also that PR ATSUnderground#629 addresses the
resumption half of this class but not pooling, so it will not arrive as a side
effect of that work.
##########
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:
Not fixed, deliberately: this is pre-existing and general rather than
specific to raw public keys.
No server-trust configuration participates in pool reuse for X.509 either.
`validate_sni()` and `validate_host_sni()` compare names, `validate_cert()`
compares the *client* certificate, and there is no comparison of the server CA
anywhere. So changing `proxy.config.ssl.client.CA.cert.filename`, or
`verify_server_policy`, equally leaves pooled sessions in place. Nothing drains
the pool on an sni.yaml reload -- `purge_keepalives()` is reached only from an
HttpSM error path -- so the staleness window is the pool lifetime for every one
of these settings, not just the pin set.
The one thing this PR changes here is narrower than it looks: the reuse gate
for raw public key origins already compared SNI only, so the class of staleness
is not new. What is new is that the empty-SNI case now pools at all, because
the old comparison returned false on every call and those sessions were never
reused. That was the bug this PR was asked to fix, so the exposure it opens is
the intended behavior arriving, not a regression.
Fixing it properly means tracking a verification-scope generation on the
pooled session and invalidating on mismatch, which should cover the CA and the
policy as well as the pin set rather than treating raw public keys as a special
case. That is worth doing and I would rather it were a separate change than
bolted onto this one. Note also that PR ATSUnderground#629 addresses the
resumption half of this class but not pooling, so it will not arrive as a side
effect of that work.
--
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]
