You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Revanth14 (via GitHub)" <gi...@apache.org> on 2026/10/02 01:11:00 UTC
[PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Revanth14 opened a new pull request, #2098:
URL: https://github.com/apache/iceberg-go/pull/2098
## Summary
Fixes #2089.
Prevent the REST transport from reapplying catalog credentials and custom header defaults when following redirects to unconfigured origins.
- Restrict managed authentication to the configured catalog origin.
- Allow custom header defaults only for the catalog and configured OAuth endpoint origins.
- Compare request URLs against configured origins so custom transports need not populate `Response.Request`.
## Validation
- `go test ./catalog/rest`
- Relevant redirect and OAuth tests pass with `-race`.
- Regression tests cover cross-origin redirects, synthetic redirects without `Response.Request`, same-origin authentication, and custom headers for a separate OAuth endpoint.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211015658
##########
catalog/rest/rest_internal_test.go:
##########
@@ -102,6 +102,207 @@ func TestSignerDoesNotSignCrossOriginRedirect(t *testing.T) {
require.Empty(t, gotToken, "the redirect target must not receive the session token")
}
+// TestCredentialsNotSentOnCrossOriginRedirect pins that the OAuth bearer token
+// and user-supplied default headers stay on the origin that started the request,
+// while a same-origin redirect keeps them.
+func TestCredentialsNotSentOnCrossOriginRedirect(t *testing.T) {
+ type seen struct {
+ hit bool
+ auth, apiKey, custom, agent string
+ }
+ record := func(s *seen, r *http.Request) {
+ s.hit = true
+ s.auth = r.Header.Get("Authorization")
+ s.apiKey = r.Header.Get("X-Api-Key")
+ s.custom = r.Header.Get("X-Custom")
+ s.agent = r.Header.Get("User-Agent")
+ }
+
+ var first, other, sameLanding seen
+ second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ record(&other, r)
+ w.WriteHeader(http.StatusOK)
+ }))
+ defer second.Close()
+
+ mux := http.NewServeMux()
+ mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+ json.NewEncoder(w).Encode(map[string]any{"defaults": map[string]any{}, "overrides": map[string]any{}})
+ })
+ mux.HandleFunc("/cross", func(w http.ResponseWriter, r *http.Request) {
+ record(&first, r)
+ http.Redirect(w, r, second.URL+"/landing", http.StatusTemporaryRedirect)
+ })
+ mux.HandleFunc("/same", func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, "/landing", http.StatusTemporaryRedirect)
+ })
+ mux.HandleFunc("/landing", func(w http.ResponseWriter, r *http.Request) {
+ record(&sameLanding, r)
+ w.WriteHeader(http.StatusOK)
+ })
+ srv := httptest.NewServer(mux)
+ defer srv.Close()
+
+ cat, err := NewCatalog(context.Background(), "rest", srv.URL,
+ WithOAuthToken("SECRET-CATALOG-TOKEN"),
+ WithHeaders(map[string]string{"X-Custom": "SECRET-CUSTOM"}),
+ WithAdditionalProps(iceberg.Properties{"header.X-Api-Key": "SECRET-API-KEY"}))
+ require.NoError(t, err)
+
+ get := func(path string) {
Review Comment:
Good catch, fixed. In the reworked `TestCrossOriginRedirectNotFollowed`, `get` takes the subtest's `t` and calls `t.Helper()`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211012281
##########
catalog/rest/rest.go:
##########
@@ -1046,6 +1072,8 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
session := &sessionTransport{
RoundTripper: baseTransport,
defaultHeaders: http.Header{},
+ catalogOrigin: r.baseURI,
+ authOrigin: opts.authUri,
Review Comment:
Done in this PR. All three clients refuse cross-origin redirects: the catalog client and both OAuth token clients, including the separate one built for `WithOAuthTLSConfig`. A redirect is followed only while it stays on the origin of the original request. The error wraps `ErrRESTError` and names both origins, and the 10-redirect limit is kept.
I didn't add an `authOrigin` exception. Each chain stays on the origin it started on, so token requests stay on the token endpoint's origin and catalog requests stay on the catalog origin.
Tests:
- `TestTokenRequestCrossOriginRedirectNotFollowed`: `client_secret` never reaches the other origin, through the catalog token endpoint, `WithAuthURI`, or `WithOAuthTLSConfig`.
- `TestDropTableCrossOriginRedirectNotFollowed`: neither the other origin nor a different table is contacted.
- `TestCrossOriginRedirectNotFollowed`: also covers same-origin redirects and the loop limit.
Each client's `CheckRedirect` was removed in turn to confirm its test fails. The behavior change is called out in the description.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211009285
##########
catalog/rest/rest.go:
##########
@@ -1046,6 +1072,8 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
session := &sessionTransport{
RoundTripper: baseTransport,
defaultHeaders: http.Header{},
+ catalogOrigin: r.baseURI,
Review Comment:
I don't think this happens. `init` runs `fetchConfig`, which applies the `uri` override to `r.baseURI`, and only then calls `createSession` for the long-lived session, so `catalogOrigin` is already the post-config origin. Only the short-lived bootstrap session that `fetchConfig` opens for `GET /v1/config` sees the original origin, and it's closed when `fetchConfig` returns.
Since the gate now relies on that order, I added `TestConfigURIOverrideKeepsCredentials`: `/v1/config` returns a `uri` on a second origin, and the test checks that the second origin gets the credentials and `header.*` values for a static token, client credentials (with the token fetched from the new origin) and a signer. If `createSession` is moved before `fetchConfig`, all three cases fail.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211018613
##########
catalog/rest/rest.go:
##########
@@ -1076,6 +1104,13 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
}
}
+ 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[http.CanonicalHeaderKey(k)] = v
Review Comment:
Done.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4210981401
##########
catalog/rest/rest.go:
##########
@@ -282,12 +296,21 @@ func defaultedPort(u *url.URL) string {
const emptyStringHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // net/http strips Authorization from cross-origin redirect hops, but this
Review Comment:
Thanks. updated.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211004683
##########
catalog/rest/rest.go:
##########
@@ -305,7 +326,12 @@ func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
r.Header.Set(k, v)
}
- if s.signer != nil && (s.signingOrigin == nil || sameOrigin(s.signingOrigin, r.URL)) {
+ // A signer only signs requests to the configured catalog origin: a request
+ // to a different origin (e.g. a redirect hop) is left unsigned, so the
+ // signer's Authorization header and any session token never reach an
+ // unconfigured host. The guard lives here, in core, so it covers every
+ // signer, including one installed verbatim via WithSigner.
+ if s.signer != nil && toCatalog {
Review Comment:
Documented on `WithSigner`: a signer only runs for requests to the catalog origin, so a token endpoint set with `WithAuthURI` gets unsigned requests. This isn't new in this PR: the old `signingOrigin` was always equal to `catalogOrigin` and behaved the same way.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4195553909
##########
catalog/rest/rest.go:
##########
@@ -262,12 +273,22 @@ func defaultedPort(u *url.URL) string {
}
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // 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
+ // runs after that stripping, so credentials and user-supplied headers are
+ // only applied for the configured origins.
+ toCatalog := s.catalogOrigin == nil || sameOrigin(s.catalogOrigin, r.URL)
Review Comment:
A nil `catalogOrigin` here means every host is treated as the catalog, so a hand-built or zero-value `sessionTransport` would send the bearer and signature everywhere. `createSession` always sets it today so it's latent, but for a security gate I'd rather it fail closed: treat nil as "trust nothing," or assert non-nil in `createSession` and drop the nil branch.
##########
catalog/rest/rest.go:
##########
@@ -305,7 +326,12 @@ func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
r.Header.Set(k, v)
}
- if s.signer != nil && (s.signingOrigin == nil || sameOrigin(s.signingOrigin, r.URL)) {
+ // A signer only signs requests to the configured catalog origin: a request
+ // to a different origin (e.g. a redirect hop) is left unsigned, so the
+ // signer's Authorization header and any session token never reach an
+ // unconfigured host. The guard lives here, in core, so it covers every
+ // signer, including one installed verbatim via WithSigner.
+ if s.signer != nil && toCatalog {
Review Comment:
Dropping `signingOrigin` for `toCatalog` is the right call, it was always equal to `catalogOrigin`. One asymmetry worth a line of godoc on `WithSigner`: a custom signer now never runs outside the catalog origin, including a signer meant to sign the token request on a separate `authUri` (headers get the `authOrigin` carve-out, signing doesn't). Fine as a decision, just document it so nobody expects their STS-style signer to fire on the IdP hop.
##########
catalog/rest/rest.go:
##########
@@ -1046,6 +1072,8 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
session := &sessionTransport{
RoundTripper: baseTransport,
defaultHeaders: http.Header{},
+ catalogOrigin: r.baseURI,
Review Comment:
This freezes `catalogOrigin` at the pre-config `baseURI`, but `fetchConfig` can reassign `r.baseURI` from a server-advertised `uri` override after the session exists. When that origin differs, every later request is suddenly cross-origin: no bearer, no signature, no `header.*`, so the catalog 401s on every call. That's a hard regression for gateway/LB deployments that hand back the real backend host, and it worked before this PR. I'd make the trusted origin track `r.baseURI` after the config merge (an atomic pointer, or a `func() *url.URL` the transport reads), and add a test where `/v1/config` returns a `uri` on a second origin and asserts it still gets auth.
##########
catalog/rest/rest.go:
##########
@@ -1046,6 +1072,8 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
session := &sessionTransport{
RoundTripper: baseTransport,
defaultHeaders: http.Header{},
+ catalogOrigin: r.baseURI,
+ authOrigin: opts.authUri,
Review Comment:
The gate stops at headers, but neither this client nor the OAuth token client in `setupOAuthManager` sets `CheckRedirect`, so cross-origin hops are still followed. The one that worries me is the token endpoint: a 307 replays the `client_credentials` form body, `client_secret` and all, to the other origin, and header gating can't touch the body. A hop can also 307 back to a catalog path and pick the bearer back up. I'd add a `CheckRedirect` that refuses (or returns `ErrUseLastResponse` on) any hop that isn't same-origin or `authOrigin`, on both clients, then the header gate becomes defense-in-depth. More on why I'm treating this as blocking in the top-level comment.
##########
catalog/rest/rest.go:
##########
@@ -1076,6 +1104,13 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
}
}
+ 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[http.CanonicalHeaderKey(k)] = v
Review Comment:
`defaultHeaders.Values(k)` hands back the backing slice, not a copy, so `builtinHeaders[ck]` aliases `defaultHeaders[ck]`. Nothing mutates these today so it's latent, but if the default-header loop ever assigns the slice onto the request directly, the request, `builtinHeaders` and `defaultHeaders` share one array and a downstream `Header.Add` or a concurrent request can scribble into session state. One line closes it:
```go
session.builtinHeaders[http.CanonicalHeaderKey(k)] = slices.Clone(v)
```
##########
catalog/rest/rest.go:
##########
@@ -262,12 +273,22 @@ func defaultedPort(u *url.URL) string {
}
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // net/http strips Authorization on redirect only for a new hostname (a
Review Comment:
This is still a bit off, same as the earlier pass flagged: net/http drops Authorization only when the host differs; it doesn't compare port or scheme, and in current Go a subdomain hop (`foo.example.com` to `example.com`) is also stripped, not kept. The thing that actually makes this gate load-bearing is that net/http never strips custom headers like `X-Api-Key` at all. I'd narrow the wording to that.
##########
catalog/rest/rest.go:
##########
@@ -1076,6 +1104,13 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
}
}
+ session.builtinHeaders = http.Header{}
+ for _, k := range []string{"X-Client-Version", "Content-Type", "User-Agent", headerIcebergAccessDelegation} {
Review Comment:
This key list is a second copy of the four `Set` calls above, and `headerIcebergAccessDelegation` is a const here but a literal there. Add a fifth built-in header and it silently stops crossing origins with no test to catch it. I'd hoist one `builtinHeaderKeys` slice and drive both the defaults and this subset from it.
##########
catalog/rest/rest_internal_test.go:
##########
@@ -102,6 +102,207 @@ func TestSignerDoesNotSignCrossOriginRedirect(t *testing.T) {
require.Empty(t, gotToken, "the redirect target must not receive the session token")
}
+// TestCredentialsNotSentOnCrossOriginRedirect pins that the OAuth bearer token
+// and user-supplied default headers stay on the origin that started the request,
+// while a same-origin redirect keeps them.
+func TestCredentialsNotSentOnCrossOriginRedirect(t *testing.T) {
+ type seen struct {
+ hit bool
+ auth, apiKey, custom, agent string
+ }
+ record := func(s *seen, r *http.Request) {
+ s.hit = true
+ s.auth = r.Header.Get("Authorization")
+ s.apiKey = r.Header.Get("X-Api-Key")
+ s.custom = r.Header.Get("X-Custom")
+ s.agent = r.Header.Get("User-Agent")
+ }
+
+ var first, other, sameLanding seen
+ second := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+ record(&other, r)
+ w.WriteHeader(http.StatusOK)
+ }))
+ defer second.Close()
+
+ mux := http.NewServeMux()
+ mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) {
+ json.NewEncoder(w).Encode(map[string]any{"defaults": map[string]any{}, "overrides": map[string]any{}})
+ })
+ mux.HandleFunc("/cross", func(w http.ResponseWriter, r *http.Request) {
+ record(&first, r)
+ http.Redirect(w, r, second.URL+"/landing", http.StatusTemporaryRedirect)
+ })
+ mux.HandleFunc("/same", func(w http.ResponseWriter, r *http.Request) {
+ http.Redirect(w, r, "/landing", http.StatusTemporaryRedirect)
+ })
+ mux.HandleFunc("/landing", func(w http.ResponseWriter, r *http.Request) {
+ record(&sameLanding, r)
+ w.WriteHeader(http.StatusOK)
+ })
+ srv := httptest.NewServer(mux)
+ defer srv.Close()
+
+ cat, err := NewCatalog(context.Background(), "rest", srv.URL,
+ WithOAuthToken("SECRET-CATALOG-TOKEN"),
+ WithHeaders(map[string]string{"X-Custom": "SECRET-CUSTOM"}),
+ WithAdditionalProps(iceberg.Properties{"header.X-Api-Key": "SECRET-API-KEY"}))
+ require.NoError(t, err)
+
+ get := func(path string) {
Review Comment:
`get` closes over the outer `t`, so a `require` failure inside a subtest calls `FailNow` on the parent `t` from the subtest goroutine: that's the "FailNow from a goroutine other than the test" footgun, and it reports at the wrong location. I'd pass the subtest `t` in via `get := func(t *testing.T, path string)` with a `t.Helper()`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4211001620
##########
catalog/rest/rest.go:
##########
@@ -262,12 +273,22 @@ func defaultedPort(u *url.URL) string {
}
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // 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
+ // runs after that stripping, so credentials and user-supplied headers are
+ // only applied for the configured origins.
+ toCatalog := s.catalogOrigin == nil || sameOrigin(s.catalogOrigin, r.URL)
Review Comment:
Agreed, done. `toCatalog` is now `s.catalogOrigin != nil && sameOrigin(...)`, so a hand-built or zero-value `sessionTransport` adds no auth header, signature or user-supplied defaults. `TestSessionTransportNilCatalogOriginTrustsNothing` covers it. The hand-built fixtures in the transport and metrics-reporter tests now set the origin they actually send to.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#issuecomment-6045162026
addressed the feedback, appreciate your another pass
@laskoviymishka @zeroshade
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4168426529
##########
catalog/rest/rest.go:
##########
@@ -1122,6 +1147,7 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
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()
Review 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:
```go
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.
##########
catalog/rest/rest.go:
##########
@@ -282,12 +296,21 @@ func defaultedPort(u *url.URL) string {
const emptyStringHash = "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // net/http strips Authorization from cross-origin redirect hops, but this
Review 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.
```suggestion
// 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
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4210999105
##########
catalog/rest/rest.go:
##########
@@ -262,12 +273,22 @@ func defaultedPort(u *url.URL) string {
}
func (s *sessionTransport) RoundTrip(r *http.Request) (*http.Response, error) {
+ // net/http strips Authorization on redirect only for a new hostname (a
Review Comment:
Rewrote it. The deeper issue was the premise: net/http's redirect stripping only covers headers on the request passed to `Client.Do`, and those are copied before the first send. The bearer, the signature and the `header.*` values are added by this transport on every hop, so net/http never strips them, and custom headers like `X-Api-Key` aren't stripped in any case. The comment now says that, and that the new `CheckRedirect` keeps clients from following a cross-origin hop in the first place, which makes this gate a second line of defense.
On the subdomain point: Go 1.27's `shouldCopyHeaderOnRedirect` keeps `Authorization` for the same host or a subdomain of the original. So `example.com` → `foo.example.com` keeps it, and `foo.example.com` → `example.com` (a hop to the parent) drops it. The old wording was about the first case, but the new comment no longer depends on that detail.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(rest): prevent credential and default header leaks on cross-origin redirects [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #2098:
URL: https://github.com/apache/iceberg-go/pull/2098#discussion_r4210993288
##########
catalog/rest/rest.go:
##########
@@ -1122,6 +1147,7 @@ func (r *Catalog) createSession(ctx context.Context, opts *options) (*http.Clien
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()
Review Comment:
Fixed in the previous push: `builtinHeaders` is built after the `WithHeaders` / `header.*` loops. The override case is now covered in `TestCredentialsNotSentCrossOrigin`, which fails if the copy is taken before the overrides.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org