Allow ESI shared-template caching behind basic auth - #1070
Conversation
ChristianPavilonis
changed the title
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The edge-auth marker is carried consistently through all four adapters, and the focused cache and authentication tests pass. I found two medium-risk follow-ups around preserving the marker's meaning and documenting the new cache exception. The remaining comments are cleanup nitpicks for the repeated and overly long explanations.
Operator documentation
docs/guide/configuration.md:1612 still says authorization bypasses the template cache unconditionally. It should distinguish pass-through and repeated values, which still bypass, from one edge-validated Basic credential, which may now share a template. Please also document that the credential remains forwarded and that an origin using it must declare Vary: Authorization.
| let authorization_value_count = req.headers().get_all(header::AUTHORIZATION).iter().count(); | ||
| let request_had_authorization = match authorization_value_count { | ||
| 0 => false, | ||
| 1 => req |
There was a problem hiding this comment.
🔧 Bind the exemption to the Authorization value that passed authentication
This checks only that one Authorization value exists and that the marker is present. Request filters run after AuthMiddleware, and DataDome can replace Authorization through X-DataDome-request-headers because that name is not forbidden by is_forbidden_filter_header. The replacement leaves this marker intact, so a publisher-origin credential can be treated as edge-terminated and reach the shared-template lookup/store path.
Please store a digest of the uniquely validated raw value in EdgeTerminatedAuthorization and require it to match here, or clear the marker whenever a later filter mutates Authorization. A regression should cover authentication followed by header replacement.
| /// Marker recording that this request's `Authorization` header was consumed and | ||
| /// validated by a Trusted Server handler at the edge. | ||
| /// | ||
| /// The shared template cache refuses every request carrying `Authorization`, | ||
| /// because an authorized response must never become a reader-neutral template. | ||
| /// That rule exists for credentials bound for the publisher origin, whose | ||
| /// response content TS cannot reason about. | ||
| /// | ||
| /// A credential this edge terminated is a different case. [`enforce_basic_auth`] | ||
| /// runs as middleware ahead of routing, so a request that reaches a handler for a | ||
| /// gated path has necessarily already satisfied that same handler. Every reader | ||
| /// able to look up a template stored from such a request has authenticated | ||
| /// against the same credential, so reuse is not a cross-reader disclosure. | ||
| /// | ||
| /// Absence of this marker on a request that still carries `Authorization` means | ||
| /// the credential is pass-through, and the template cache continues to refuse it. | ||
| /// | ||
| /// # Invariants | ||
| /// | ||
| /// The private field makes [`enforce_basic_auth`] the only code that can produce | ||
| /// this marker. It grants shared-template eligibility to a request that would | ||
| /// otherwise be refused, so being unforgeable outside this module is the whole | ||
| /// point: a caller cannot assert "already authenticated" without having actually | ||
| /// checked. [`enforce_basic_auth`] also clears any inherited marker before it | ||
| /// decides, so the value can never outlive the check that produced it. |
There was a problem hiding this comment.
⛏ Nitpick: shorten the marker documentation
Most of this repeats the function documentation and explains privacy that the type already enforces. The invariant fits in a few lines.
| /// Marker recording that this request's `Authorization` header was consumed and | |
| /// validated by a Trusted Server handler at the edge. | |
| /// | |
| /// The shared template cache refuses every request carrying `Authorization`, | |
| /// because an authorized response must never become a reader-neutral template. | |
| /// That rule exists for credentials bound for the publisher origin, whose | |
| /// response content TS cannot reason about. | |
| /// | |
| /// A credential this edge terminated is a different case. [`enforce_basic_auth`] | |
| /// runs as middleware ahead of routing, so a request that reaches a handler for a | |
| /// gated path has necessarily already satisfied that same handler. Every reader | |
| /// able to look up a template stored from such a request has authenticated | |
| /// against the same credential, so reuse is not a cross-reader disclosure. | |
| /// | |
| /// Absence of this marker on a request that still carries `Authorization` means | |
| /// the credential is pass-through, and the template cache continues to refuse it. | |
| /// | |
| /// # Invariants | |
| /// | |
| /// The private field makes [`enforce_basic_auth`] the only code that can produce | |
| /// this marker. It grants shared-template eligibility to a request that would | |
| /// otherwise be refused, so being unforgeable outside this module is the whole | |
| /// point: a caller cannot assert "already authenticated" without having actually | |
| /// checked. [`enforce_basic_auth`] also clears any inherited marker before it | |
| /// decides, so the value can never outlive the check that produced it. | |
| /// Marks a request whose single `Authorization` value Trusted Server validated. | |
| /// | |
| /// The shared template cache may exempt this value from its normal authorization | |
| /// bypass. [`enforce_basic_auth`] clears any existing marker before checking and | |
| /// inserts a new marker only after successful authentication. |
| // Recorded before the request is consumed by the origin send: the template cache gate | ||
| // below needs it, and an authorized response must never become a shared | ||
| // template. | ||
| let request_had_authorization = req.headers().contains_key(header::AUTHORIZATION); | ||
| // | ||
| // A credential this edge already terminated is exempt. Basic auth runs as | ||
| // middleware ahead of routing, so reaching here on a gated path means the same | ||
| // handler already validated the request; every reader that can look the template | ||
| // up has satisfied that same credential. Without the marker the credential is | ||
| // pass-through to the origin and still disqualifies. See | ||
| // [`crate::auth::EdgeTerminatedAuthorization`]. |
There was a problem hiding this comment.
⛏ Nitpick: keep the cache-gate explanation local and short
The longer authentication argument already lives in auth.rs. This site only needs to explain the local distinction.
| // Recorded before the request is consumed by the origin send: the template cache gate | |
| // below needs it, and an authorized response must never become a shared | |
| // template. | |
| let request_had_authorization = req.headers().contains_key(header::AUTHORIZATION); | |
| // | |
| // A credential this edge already terminated is exempt. Basic auth runs as | |
| // middleware ahead of routing, so reaching here on a gated path means the same | |
| // handler already validated the request; every reader that can look the template | |
| // up has satisfied that same credential. Without the marker the credential is | |
| // pass-through to the origin and still disqualifies. See | |
| // [`crate::auth::EdgeTerminatedAuthorization`]. | |
| // Capture cache eligibility before the origin send consumes the request. A marked, | |
| // uniquely valued Authorization header was already validated by edge auth; | |
| // unmarked or repeated values remain pass-through and bypass sharing. |
| // Takes the request mutably because `enforce_basic_auth` marks requests | ||
| // whose credential it consumed itself; the shared template cache gate | ||
| // reads that marker later. | ||
| match enforce_basic_auth(&self.settings, ctx.request_mut()) { |
There was a problem hiding this comment.
⛏ Nitpick: remove the repeated middleware comment
The mutable request and call name already make this clear, while enforce_basic_auth owns the full explanation. I would remove this comment here and from the Axum, Cloudflare, and Spin copies.
| // Takes the request mutably because `enforce_basic_auth` marks requests | |
| // whose credential it consumed itself; the shared template cache gate | |
| // reads that marker later. | |
| match enforce_basic_auth(&self.settings, ctx.request_mut()) { | |
| match enforce_basic_auth(&self.settings, ctx.request_mut()) { |
2 participants
Summary
Changes
Test plan
Checklist
No linked issue for this PR, per request.