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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
