Conversation
zeroshade
left a comment
There was a problem hiding this comment.
This closes #2089 as scoped. sessionTransport.RoundTrip now sends the auth-manager header only to the catalog origin, and WithHeaders/header.* defaults only to the catalog and OAuth-endpoint origins. The two new leak tests fail on d66ab48 and pass at ab8b1b3, also when merged with current main (incl. #2060).
Smaller observations
-
Follow-up, pre-existing, not blocking (probably its own issue): this gates headers, but redirects are still followed. Verified at head:
- a 307 from the token endpoint replays the
client_credentialsform,client_secretincluded, to the other origin, and the token that origin returns is then used against the catalog; - a cross-origin hop's response is decoded as the catalog's;
- a hop can 307 back to any catalog path (e.g.
DELETE …/tables/x?purgeRequested=true), which then goes out with the bearer.
A
CheckRedirectthat refuses cross-origin hops on both the catalog client and the OAuth token client (oauthClient) would close all three. Nothing needs composing: the package always builds its ownhttp.Client, and custom transports sit undersessionTransport. For comparison, Java fails closed on authenticated hops: httpclient5'sDefaultRedirectStrategy.isRedirectAllowedwon't follow a cross-authority redirect that carriesAuthorization. - a 307 from the token endpoint replays the
-
builtinHeadersis cloned before the operator overrides, so cross-origin hops revert overridden built-in keys (inline). -
The
RoundTripcomment overstates net/http's redirect stripping (inline). -
Nit: with a signer set,
signingOriginalways equalscatalogOrigin(still true after #2060). The signer gate could usetoCatalogand the field could go, giving one origin check for auth, headers and signing as #2089 suggested.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Iceberg maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Iceberg handles maintainer review:
CONTRIBUTING.md.
| const emptyStringHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855" | ||
|
|
||
| func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| // net/http strips Authorization from cross-origin redirect hops, but this |
There was a problem hiding this comment.
Nit: net/http's redirect stripping is narrower than this reads. It only drops Authorization it copied from the first request, and only for a new hostname. A port or scheme change, or a hop to a subdomain, keeps it (shouldCopyHeaderOnRedirect). That's why this gate is needed even for the port-only redirect the new test uses.
| // net/http strips Authorization from cross-origin redirect hops, but this | |
| // net/http strips Authorization on redirect only for a new hostname (a | |
| // port or scheme change, or a hop to a subdomain, keeps it), but this |
| session.defaultHeaders.Set("Content-Type", "application/json") | ||
| session.defaultHeaders.Set("User-Agent", "GoIceberg/"+iceberg.Version()) | ||
| session.defaultHeaders.Set(headerIcebergAccessDelegation, defaultAccessDelegation) | ||
| session.builtinHeaders = session.defaultHeaders.Clone() |
There was a problem hiding this comment.
Minor: this snapshot is taken before the WithHeaders / header.* loops below, so a cross-origin hop gets the built-in default for any key the operator overrode. I checked this at ab8b1b3 with WithHeaders({"User-Agent": "corp-agent/1"}) and header.X-Iceberg-Access-Delegation=remote-signing. The same-origin request sends those values, but the hop sends GoIceberg/… and vended-credentials. Before this PR the hop got the overrides. It also doesn't match the field doc ("the subset of defaultHeaders"). Building it after the override loops, from just the built-in keys, keeps the two consistent:
session.builtinHeaders = http.Header{}
for _, k := range []string{"X-Client-Version", "Content-Type", "User-Agent", headerIcebergAccessDelegation} {
if v := session.defaultHeaders.Values(k); len(v) > 0 {
session.builtinHeaders[k] = v
}
}If reverting to the defaults is intended, the field doc should say "built-in defaults" instead.
Summary
Fixes #2089.
Prevent the REST transport from reapplying catalog credentials and custom header defaults when following redirects to unconfigured origins.
Response.Request.Validation
go test ./catalog/rest-race.Response.Request, same-origin authentication, and custom headers for a separate OAuth endpoint.