zeroshade commented on code in PR #2060: URL: https://github.com/apache/iceberg-go/pull/2060#discussion_r4158472896
########## catalog/rest/signer.go: ########## @@ -0,0 +1,121 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "context" + "fmt" + "net/http" + "sync" +) + +// SignerNameSigV4 is the scheme under which the AWS SigV4 backend registers +// itself (see catalog/rest/sigv4). It is also the value carried by the +// rest.sigv4-enabled property path. It is exported so the backend can register +// under the exact same name the core looks up, rather than a duplicated string +// literal that could silently drift. +const SignerNameSigV4 = "sigv4" + +// RequestSigner signs an outgoing catalog HTTP request in place, for example +// with AWS SigV4. Implementations live in optional sub-packages (see +// github.com/apache/iceberg-go/catalog/rest/sigv4) so the core REST client +// depends on no cloud SDK. Signing runs inside the session transport's +// RoundTrip, after auth headers are applied, and may use req.Context. +type RequestSigner interface { + SignRequest(req *http.Request) error +} + +// SignerConfig carries the signing parameters the REST core knows about, +// deliberately free of any cloud-SDK types. A SignerFactory turns it into a +// concrete RequestSigner. +type SignerConfig struct { + // Region is the signing region (SigV4 signing-region). + Region string + // Service is the signing service name (SigV4 signing-name), e.g. + // "execute-api", "s3tables". + Service string +} + +// SignerFactory builds a RequestSigner from core configuration. A signing +// backend registers one under a scheme name via RegisterSigner, so that a +// blank import of the backend package is enough to enable property- or +// option-driven signing (e.g. WithSigV4 / rest.sigv4-enabled). +type SignerFactory func(ctx context.Context, cfg SignerConfig) (RequestSigner, error) + +var ( + signerMu sync.RWMutex + signerRegistry = map[string]SignerFactory{} +) + +// RegisterSigner registers a signer factory under name (e.g. "sigv4"). It is +// intended to be called from a backend package's init function. Registering +// the same name twice replaces the previous factory. It panics if factory is +// nil, since a nil factory is a programming error that would otherwise surface +// only later, as a failure when signing is resolved. +func RegisterSigner(name string, factory SignerFactory) { + if factory == nil { + panic(fmt.Sprintf("rest: RegisterSigner: nil factory for %q", name)) + } + + signerMu.Lock() + defer signerMu.Unlock() + signerRegistry[name] = factory +} + +func lookupSigner(name string) (SignerFactory, bool) { + signerMu.RLock() + defer signerMu.RUnlock() + f, ok := signerRegistry[name] + + return f, ok +} + +// resolveSigner determines the request signer for a session. It returns +// (nil, nil) when signing is not configured, and a helpful error when SigV4 is +// requested but no backend has been imported. +// +// Precedence: +// - An explicit WithSigner is used verbatim (the caller fully built it), so it +// bypasses the sigv4-enabled / signing-region / signing-name settings and any +// server-provided overrides. +// - Otherwise, when SigV4 is enabled (WithSigV4 / WithSigV4RegionSvc or the +// rest.sigv4-enabled property), a WithSignerFactory (e.g. sigv4.WithAwsConfig) +// builds the signer if one was supplied, else the registered "sigv4" backend +// does. Both receive the resolved region/service, which by this point already +// include any /v1/config overrides fetchConfig folded into opts. +func resolveSigner(ctx context.Context, opts *options) (RequestSigner, error) { + if opts.signer != nil { + return opts.signer, nil + } + + if !opts.enableSigv4 { + return nil, nil + } Review Comment: Nothing pins this precedence. No test calls `WithSigner`, so this first branch is never exercised through `NewCatalog`. Nothing asserts that `WithSignerFactory` on its own leaves requests unsigned either, and that is the main-parity property the factory design relies on (on main, `WithAwsConfig` without `WithSigV4` did not sign). Please add one table-driven test over `resolveSigner`: - `WithSigner`, SigV4 off: returns that signer - factory, SigV4 off: returns nil - factory, SigV4 on: calls the factory with the opts region/service - no factory, SigV4 on, no backend registered: returns the import error ########## website/src/configuration.md: ########## @@ -71,11 +71,31 @@ The most option-rich surface. Source: [`catalog/rest/options.go`](https://github | Group | Options | |---|---| | Authentication | `WithCredential`, `WithOAuthToken`, `WithAuthManager`, `WithAuthURI`, `WithScope`, `WithAudience`, `WithResource` | -| AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithAwsConfig` | +| AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSigner` | Review Comment: `WithSignerFactory` is missing here. It is the hook `sigv4.WithAwsConfig` is built on, and the one to use for a custom signer that should keep following the signing region/service. ```suggestion | AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSignerFactory`, `WithSigner` | ``` ########## website/src/configuration.md: ########## @@ -71,11 +71,31 @@ The most option-rich surface. Source: [`catalog/rest/options.go`](https://github | Group | Options | |---|---| | Authentication | `WithCredential`, `WithOAuthToken`, `WithAuthManager`, `WithAuthURI`, `WithScope`, `WithAudience`, `WithResource` | -| AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithAwsConfig` | +| AWS SigV4 | `WithSigV4`, `WithSigV4RegionSvc`, `WithSigner` | | HTTP | `WithHeaders`, `WithTLSConfig`, `WithOAuthTLSConfig`, `WithCustomTransport` | | Catalog routing | `WithPrefix`, `WithWarehouseLocation`, `WithMetadataLocation` | | Pass-through | `WithAdditionalProps` | +> **AWS SigV4 is an optional backend.** `catalog/rest` no longer links the AWS +> SDK. To sign REST requests with SigV4, pull in the +> [`catalog/rest/sigv4`](https://github.com/apache/iceberg-go/blob/main/catalog/rest/sigv4/sigv4.go) +> sub-package one of two ways: +> +> - Ambient credentials (the AWS default chain): add a blank import +> `_ "github.com/apache/iceberg-go/catalog/rest/sigv4"` and enable signing with +> `WithSigV4` / `WithSigV4RegionSvc` or the `rest.sigv4-enabled` property. +> - Explicit config: pass `sigv4.WithAwsConfig(cfg)` to `NewCatalog` alongside +> `WithSigV4` / `WithSigV4RegionSvc` (no blank import needed). It supplies the +> `aws.Config`; the signing region and service still come from those options +> (or a server `/v1/config` override), not from `cfg`. Review Comment: `not from cfg` is wrong for the region on the `WithSigV4()` path. `WithSigV4` sets no region, and `buildSigner` (`sigv4/sigv4.go:78-80`) only overrides `awscfg.Region` when `SignerConfig.Region` is non-empty, so the request is signed with `cfg.Region`. Main had the same fallback, and the `sigv4.WithAwsConfig` godoc already describes it correctly; only this text is off. ```suggestion > - Explicit config: pass `sigv4.WithAwsConfig(cfg)` to `NewCatalog` alongside > `WithSigV4` / `WithSigV4RegionSvc` (no blank import needed). It supplies the > `aws.Config`; the signing service comes from those options (or a server > `/v1/config` override), and so does the region when one is set. Otherwise > the region falls back to `cfg.Region`. ``` ########## catalog/rest/sigv4/sigv4_internal_test.go: ########## @@ -0,0 +1,306 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package sigv4 + +import ( + "bytes" + "context" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + + "github.com/apache/iceberg-go/catalog/rest" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/config" + "github.com/aws/aws-sdk-go-v2/credentials" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestSigner(t *testing.T) *signer { + t.Helper() + + cfg, err := config.LoadDefaultConfig(context.Background(), func(o *config.LoadOptions) error { + o.Credentials = credentials.StaticCredentialsProvider{ + Value: aws.Credentials{ + AccessKeyID: "test-access-key", + SecretAccessKey: "test-secret-key", + }, + } + + return nil + }) + require.NoError(t, err) + cfg.Region = "us-east-1" + + return newSigner(cfg, "s3") +} + +func TestEmptyStringHash(t *testing.T) { + t.Parallel() + + h := sha256.New() + assert.Equal(t, hex.EncodeToString(h.Sum(nil)), emptyStringHash) +} + +func TestSignRequestEmptyBodyContentHash(t *testing.T) { + t.Parallel() + + s := newTestSigner(t) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://example.com/test";, nil) + require.NoError(t, err) + require.NoError(t, s.SignRequest(req)) + + assert.Equal(t, emptyStringHash, req.Header.Get("x-amz-content-sha256")) + assert.NotEmpty(t, req.Header.Get("Authorization"), "SigV4 should set the Authorization header") +} + +func TestSignRequestBodyContentHash(t *testing.T) { + t.Parallel() + + s := newTestSigner(t) + body := []byte(`{"test": "data"}`) + sum := sha256.Sum256(body) + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://example.com/test";, bytes.NewReader(body)) + require.NoError(t, err) + require.NoError(t, s.SignRequest(req)) + + assert.Equal(t, hex.EncodeToString(sum[:]), req.Header.Get("x-amz-content-sha256")) +} + +func TestSignRequestNilGetBodyReturnsError(t *testing.T) { + t.Parallel() + + s := newTestSigner(t) + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://example.com/test";, bytes.NewReader([]byte(`{}`))) + require.NoError(t, err) + req.GetBody = nil // a hand-built request whose body cannot be re-read + + err = s.SignRequest(req) + require.Error(t, err) + assert.Contains(t, err.Error(), "GetBody", "error should explain the body is not re-readable") +} + +type closeTrackingReadCloser struct { + *bytes.Reader + closeErr error + closed bool +} + +func (r *closeTrackingReadCloser) Close() error { + r.closed = true + + return r.closeErr +} + +func TestSignRequestClosesClonedBody(t *testing.T) { + t.Parallel() + + s := newTestSigner(t) + body := []byte(`{"test": "data"}`) + var cloned *closeTrackingReadCloser + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://example.com/test";, bytes.NewReader(body)) + require.NoError(t, err) + req.GetBody = func() (io.ReadCloser, error) { + cloned = &closeTrackingReadCloser{Reader: bytes.NewReader(body)} + + return cloned, nil + } + + require.NoError(t, s.SignRequest(req)) + require.NotNil(t, cloned) + assert.True(t, cloned.closed) +} + +func TestSignRequestReturnsClonedBodyCloseError(t *testing.T) { + t.Parallel() + + s := newTestSigner(t) + closeErr := errors.New("close failed") + body := []byte(`{"test": "data"}`) + var cloned *closeTrackingReadCloser + + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://example.com/test";, bytes.NewReader(body)) + require.NoError(t, err) + req.GetBody = func() (io.ReadCloser, error) { + cloned = &closeTrackingReadCloser{Reader: bytes.NewReader(body), closeErr: closeErr} + + return cloned, nil + } + + err = s.SignRequest(req) + require.ErrorIs(t, err, closeErr) + require.NotNil(t, cloned) + assert.True(t, cloned.closed) +} + +func TestSignRequestConcurrent(t *testing.T) { + t.Parallel() + + // POSTs with a body so the payload-hashing path (GetBody clone + SHA-256) + // runs concurrently on a single shared signer, exercising the shared v4 + // signer and aws.Config under the race detector. + s := newTestSigner(t) + body := []byte(`{"test":"data"}`) + var wg sync.WaitGroup + for range 20 { + wg.Go(func() { + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://example.com/test";, bytes.NewReader(body)) + if err == nil { + err = s.SignRequest(req) + } + if err != nil { + t.Error(err) + } + }) + } + wg.Wait() +} + +// TestRegisteredBackendEnablesSigV4 verifies that importing this package (its +// init registers the sigv4 signer) lets rest.WithSigV4RegionSvc resolve without +// the caller supplying an explicit signer, and that the resolved signer +// actually signs outbound catalog requests end to end. +func TestRegisteredBackendEnablesSigV4(t *testing.T) { + // t.Setenv precludes t.Parallel; static credentials let the registered + // factory's config.LoadDefaultConfig resolve offline (no EC2 IMDS lookup). + t.Setenv("AWS_ACCESS_KEY_ID", "test-access-key") + t.Setenv("AWS_SECRET_ACCESS_KEY", "test-secret-key") + t.Setenv("AWS_REGION", "us-east-1") + + var gotAuth, gotSHA string + mux := http.NewServeMux() + srv := httptest.NewServer(mux) + defer srv.Close() + + mux.HandleFunc("/v1/config", func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + gotSHA = r.Header.Get("x-amz-content-sha256") + json.NewEncoder(w).Encode(map[string]any{ + "defaults": map[string]any{}, "overrides": map[string]any{}, + }) + }) + + cat, err := rest.NewCatalog(context.Background(), "rest", srv.URL, + rest.WithSigV4RegionSvc("us-east-1", "s3")) + require.NoError(t, err) + require.NotNil(t, cat) + t.Cleanup(func() { _ = cat.Close() }) + + // The registered backend must actually sign the bootstrap /v1/config request, + // not merely let catalog construction succeed. + assert.Contains(t, gotAuth, "AWS4-HMAC-SHA256", "request should carry a SigV4 Authorization header") + assert.NotEmpty(t, gotSHA, "request should carry x-amz-content-sha256") +} + +// TestWithAwsConfigUsesConfiguredRegionService verifies that sigv4.WithAwsConfig +// signs with the region/service from WithSigV4RegionSvc, not the aws.Config's +// own region. Before WithAwsConfig became a signer factory it froze the scope at +// option-construction time, so the natural migration (keep WithSigV4RegionSvc, +// swap rest.WithAwsConfig for sigv4.WithAwsConfig) silently signed for the wrong +// scope and the server rejected it with a 403. Review Comment: These lines describe an intermediate state of this PR (a pre-built signer / three-argument `WithAwsConfig`) that never shipped, so they will not mean anything to readers on main. Please drop them, and the matching sentence in `rest_internal_test.go:1890-1892`; keep only what the test asserts. Also lines 266-267 below: `TestConcurrentSignedCatalogRequests` does not restore the old `TestSigv4ConcurrentSigners` coverage. It builds one catalog per goroutine and sends only body-less `GET /v1/config`. Shared-signer body hashing is now covered by `TestSignRequestConcurrent`, so reword or drop that claim. ########## catalog/rest/signer.go: ########## @@ -0,0 +1,121 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package rest + +import ( + "context" + "fmt" + "net/http" + "sync" +) + +// SignerNameSigV4 is the scheme under which the AWS SigV4 backend registers +// itself (see catalog/rest/sigv4). It is also the value carried by the +// rest.sigv4-enabled property path. It is exported so the backend can register +// under the exact same name the core looks up, rather than a duplicated string +// literal that could silently drift. +const SignerNameSigV4 = "sigv4" Review Comment: `resolveSigner` only ever looks up `SignerNameSigV4` (line 113), so `RegisterSigner("anything-else", f)` is accepted and never consulted. The name parameter plus an exported constant read like a multi-scheme registry that does not exist. The doc is also off: `rest.sigv4-enabled` carries `"true"`, not this name. Since this becomes public API, either drop the name (e.g. `RegisterSigV4(factory SignerFactory)`) and unexport the constant, or keep the shape and state on `RegisterSigner` that only `SignerNameSigV4` is consulted. -- 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]
