Skip to content

authproxy: strip request-body framing from auth sub-requests#13333

Merged
moonchen merged 1 commit into
apache:masterfrom
moonchen:authproxy-strip-body-framing
Jul 12, 2026
Merged

authproxy: strip request-body framing from auth sub-requests#13333
moonchen merged 1 commit into
apache:masterfrom
moonchen:authproxy-strip-body-framing

Conversation

@moonchen

@moonchen moonchen commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

The head/range/redirect auth transforms in the authproxy plugin copy the
whole client request and then force it bodyless (method override +
Content-Length: 0) to probe the auth server, but left Transfer-Encoding,
Trailer, and Expect in place. A chunked or Expect: 100-continue client
request therefore produced a self-contradictory sub-request: a bodyless
HEAD/GET still advertising a body.

ATS honors the framing and sets up a request-body tunnel for a body that never
arrives, stalling the probe until the inactivity timeout; and
proxy.config.http.reject_head_with_content rejects a HEAD that declares
content outright. This strips Transfer-Encoding, Trailer, and Expect
when normalizing the sub-request to bodyless, in all three transforms
(head, range, redirect).

A small HttpRemoveMimeHeader() helper is added to utils.{h,cc} that
destroys every instance of a named field (these fields can be repeated across
multiple lines).

The head/range/redirect auth transforms copy the whole client request and
then force it bodyless (method override + Content-Length: 0) to probe the
auth server, but left Transfer-Encoding, Trailer, and Expect in place. A
chunked or Expect: 100-continue client request therefore produced a
self-contradictory sub-request: a bodyless HEAD/GET still advertising a body.

ATS honors the framing and sets up a request-body tunnel for a body that
never arrives, stalling the probe until the inactivity timeout; and
proxy.config.http.reject_head_with_content rejects a HEAD that declares
content outright. Strip Transfer-Encoding, Trailer, and Expect when
normalizing the sub-request to bodyless.
Copilot AI review requested due to automatic review settings June 25, 2026 15:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the authproxy plugin’s auth sub-request generation by ensuring “bodyless” probes (HEAD / Range GET / redirected requests with Content-Length: 0) don’t retain request-body framing headers copied from the client request, which can otherwise cause stalls or outright rejection.

Changes:

  • Add HttpRemoveMimeHeader() utility to remove all instances of a named header field.
  • Strip Transfer-Encoding, Trailer, and Expect when normalizing auth sub-requests to bodyless.
  • Apply the same framing cleanup across the head/range/redirect auth transforms.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
plugins/authproxy/utils.h Declares a new helper for removing all instances of a header field.
plugins/authproxy/utils.cc Implements HttpRemoveMimeHeader() via repeated find/destroy.
plugins/authproxy/authproxy.cc Uses the new helper to remove body-framing headers for bodyless auth sub-requests.

Comment thread plugins/authproxy/authproxy.cc
Comment thread plugins/authproxy/authproxy.cc
Comment thread plugins/authproxy/authproxy.cc
@bryancall bryancall added the authproxy authproxy plugin label Jun 29, 2026
@bryancall bryancall added this to the 11.0.0 milestone Jun 29, 2026
@bryancall bryancall added the Bug label Jun 29, 2026
@bryancall bryancall self-requested a review June 29, 2026 22:25

@bryancall bryancall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. The stripping order is correct, since Transfer-Encoding, Trailer, and Expect are removed before Content-Length is zeroed across all three sub-request transforms. The new HttpRemoveMimeHeader() re-fetches a fresh field location each iteration, so there is no iterator invalidation when a header appears more than once.

I have one non-blocking note. The fix ships without a test for the two failure modes the description calls out, which are a chunked or Expect: 100-continue request stalling the body tunnel, and reject_head_with_content rejecting a HEAD with content. An autest driving those cases and asserting that the auth sub-request arrives bodyless would guard against a future regression, but I do not consider it merge-gating.

@moonchen moonchen merged commit 364668f into apache:master Jul 12, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this to For v10.2.0 in ATS v10.2.x Jul 12, 2026
@cmcfarlen cmcfarlen moved this from For v10.2.0 to Picked v10.2.0 in ATS v10.2.x Jul 13, 2026
@cmcfarlen cmcfarlen modified the milestones: 11.0.0, 10.2.0 Jul 13, 2026
@cmcfarlen

Copy link
Copy Markdown
Contributor

Cherry-picked to 10.2.x

cmcfarlen pushed a commit that referenced this pull request Jul 13, 2026
The head/range/redirect auth transforms copy the whole client request and
then force it bodyless (method override + Content-Length: 0) to probe the
auth server, but left Transfer-Encoding, Trailer, and Expect in place. A
chunked or Expect: 100-continue client request therefore produced a
self-contradictory sub-request: a bodyless HEAD/GET still advertising a body.

ATS honors the framing and sets up a request-body tunnel for a body that
never arrives, stalling the probe until the inactivity timeout; and
proxy.config.http.reject_head_with_content rejects a HEAD that declares
content outright. Strip Transfer-Encoding, Trailer, and Expect when
normalizing the sub-request to bodyless.

(cherry picked from commit 364668f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

authproxy authproxy plugin Bug

Projects

Status: Picked v10.2.0

Development

Successfully merging this pull request may close these issues.

4 participants