fix(catalog/rest): sign SigV4 with credentials from catalog properties

[iremcaginyurtturk]

Problem

The REST catalog's SigV4 signer builds its aws.Config only from config.LoadDefaultConfig (the AWS default credential chain), so the s3.* credential properties passed to the catalog are ignored for request signing.

As a result, connecting to an AWS SigV4 REST catalog (S3 Tables at s3tables.<region>.amazonaws.com/iceberg, or the Glue Iceberg REST endpoint) fails with no EC2 IMDS role found unless AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN are also present in the environment — even when the caller already supplied those credentials via s3.access-key-id / s3.secret-access-key / s3.session-token.

This is the same gap tracked for the Python client in apache/iceberg-python#2070.

Fix

In createSession, when no explicit aws.Config was provided (WithAwsConfig), build a static credentials provider from the s3.* credential properties when a key pair is present, and fall back to the default chain otherwise. An explicitly provided aws.Config still takes precedence.

if creds, ok := staticCredsFromProps(opts.additionalProps); ok {
    cfg.Credentials = creds
}

Testing

• New unit test TestStaticCredsFromProps (full key pair → provider with session token; lone access key → no provider; empty → no provider).
• Verified end-to-end against a live AWS S3 Tables bucket: create/replace/merge succeed with credentials supplied only via catalog properties and no AWS_* environment variables.
• go test ./catalog/rest/ passes.

[zeroshade]

The SigV4-from-properties fix genuinely works and does not silently downgrade to unsigned, but the wiring line is pinned by no test (package stays green when it is deleted) and a partial credential pair silently signs as the ambient identity instead of erroring.

Re-review verification: 0 of 1 prior findings confirmed fixed at ffa55bb (each verified by mutating the fix and observing the suite go red, not by taking the claim on trust).

Verification performed

go build ./catalog/... (OK); go vet ./catalog/rest/ (clean); go test ./catalog/rest/ -timeout=300s -> ok 5.908s; go test ./catalog/rest/ -run TestStaticCredsFromProps -v -> PASS. Mutation 1: neutralized `cfg.Credentials = creds` -> full package STILL ok 5.946s (fix unpinned). Mutation 2: swapped access/secret args in NewStaticCredentialsProvider -> TestStaticCredsFromProps FAILS (helper test is non-vacuous). Throwaway probe catalog/rest/pr1999_probe_test.go (5 tests: props-reach-signer, no-creds-anywhere, partial-creds, precedence, server-override) all ran and were then deleted. Final: git status --porcelain empty, go vet ./catalog/hive/ clean.

---

This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg Go maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.

More on how Apache Iceberg Go handles maintainer review: CONTRIBUTING.md.

[iremcaginyurtturk]

Thanks for the review — both points addressed in cca7c83:

1. Wiring now pinned by a test. Added TestSigV4SignsWithPropsCredentials, which drives a request through a test server and asserts the SigV4 Authorization header contains Credential=<props access key>/. Neutralizing the cfg.Credentials = creds assignment makes the test fail (falls back to the default chain → no EC2 IMDS role found), so the wiring is no longer deletable while green.

2. Partial credentials now error. staticCredsFromProps returns an error when only one of s3.access-key-id / s3.secret-access-key is set, instead of silently signing as the ambient identity. Neither set still returns (nil, nil) so the default chain is used (e.g. instance role); both set builds the static provider. Covered by the updated TestStaticCredsFromProps.

[iremcaginyurtturk]

All four review findings addressed:

1. (major) Fix pinned by no test — cca7c83 adds TestSigV4SignsWithPropsCredentials: it stands up an httptest server, passes creds only via WithAdditionalProps, and asserts the captured Authorization header contains the props access key (Credential=AKIDEXAMPLEPROPS/). Verified by mutation — deleting cfg.Credentials = creds makes it fail (no EC2 IMDS role found).

2. (major) Partial s3.* pair signs as a different principal — cca7c83: staticCredsFromProps now returns an error when exactly one of s3.access-key-id / s3.secret-access-key is set, naming the required properties. Fully-empty still falls through to the default chain. TestStaticCredsFromProps updated to assert the error cases instead of enshrining the old behavior.

3. (minor) Undocumented precedence — 48164f5: documented the WithAwsConfig > s3.* props > default-chain order on WithSigV4 (godoc) and in the rest.sigv4-enabled row of website/src/configuration.md.

4. (minor) Server /v1/config override can select the signing identity — confirming intent: this is acceptable. The REST control plane is a trusted input per SECURITY-THREAT-MODEL.md and no secret reaches a new audience (the server already knows any creds it injected). If you'd prefer the signing identity be pinned to client-supplied properties regardless of server overrides, I'm happy to capture the client creds before fetchConfig folds in the overrides — just let me know.

Full catalog/rest suite passes; go vet clean.

[laskoviymishka]

Nice, this is basically there. The props-credential path works, and the two things flagged last round are both handled now:

• TestSigV4SignsWithPropsCredentials pins the wiring end to end. It asserts the Authorization header carries Credential=AKIDEXAMPLEPROPS/, which only holds when the props creds are actually signing rather than the default chain.
• staticCredsFromProps now errors on a lone access-key or secret-key instead of silently signing as a different identity, and both cases are covered.

The one thing I'd still fix is the token-only case: a lone s3.session-token with no access/secret falls into the empty-pair branch and returns (nil, nil), so it silently drops to the default chain. We already have internal/awsconfig.ValidateStaticCredentials for exactly this check. It's what io/gocloud/s3 and catalog/glue use, and it returns the ErrIncompleteStaticCredentials sentinel, so delegating to it closes the gap and makes the error errors.Is-detectable in one move instead of a bare fmt.Errorf. Worth a token-only test case too.

Not blocking, but while we're here: sourcing the signing creds from s3.* diverges from Java, which uses rest.access-key-id / rest.secret-access-key. You've documented s3.* as intentional so I'm fine with it, but an operator coming from the Java client configuring rest.* will silently fall through to the default chain. Might be worth a line in the godoc calling that out, or accepting rest.* as an alias down the line. wdyt?

The token-only fix is the one I'd still like to see; everything else is optional. This is close.

[zeroshade]

REQUEST_CHANGES. The 09-08 wiring and documentation feedback is addressed: TestSigV4SignsWithPropsCredentials pins the fix end to end, the lone-access/lone-secret/token-only cases now return ErrIncompleteStaticCredentials, and precedence is documented — WithAwsConfig wins, complete property credentials override the default chain, and an absent tuple preserves the ambient chain. The identity cutover for users who already set s3.* while relying on ambient catalog credentials is explicit in both the godoc and the configuration reference, which is what I asked for. Server /v1/config overrides selecting the post-bootstrap identity stays within the trusted-control-plane model.

Two credential-boundary problems remain; details inline. The first is in scope under SECURITY-THREAT-MODEL.md: "a secret or delegated credential reaching a new audience" is an explicit reassess trigger that overrides the trusted-control-plane downgrade.

Non-blocking: catalog/rest/rest_internal_test.go:138 discards the response without closing the body.

[zeroshade]

Re-reviewed at d401049. Both blockers from my 09-21 review are fixed:

• staticCredsFromProps (rest.go:1180-1198) now takes one namespace whole and never fills a missing field from the other. The mixed-partial and s3-pair + rest-token cases are covered.
• Cross-origin redirect hops are no longer signed (sameOrigin guard, rest.go:327). I removed the guard locally and TestSigv4DoesNotSignCrossOriginRedirect failed with the second server receiving Credential=AKIDEXAMPLEPROPS/..., so the test pins it.

The response-body close and @laskoviymishka's token-only / ValidateStaticCredentials / rest.* alias points are in too. What remains is inline and non-blocking: credential precedence vs Java/pyiceberg, stale "resolved per field" wording, optional test style.

Merge order vs #2060: this lands first. #2060 removes the inline SigV4 path from createSession, so it will need to carry the property credentials through SignerConfig into catalog/rest/sigv4, keep the signingOrigin check in core RoundTrip around SignRequest, and port these tests.

[zeroshade]

Non-blocking, but worth settling before this ships because it is user-visible. With both namespaces set, s3.* signs and rest.* is ignored (pinned by the "s3.* keys take precedence over rest.* aliases" case in the test).

Java's AwsProperties.restCredentialsProvider() signs only with rest.access-key-id / rest.secret-access-key / rest.session-token, then client.credentials-provider, then the default chain. It never reads s3.*. pyiceberg signs with client.access-key-id etc. So if someone sets s3.* for FileIO (MinIO, or a scoped data principal) and rest.* for the catalog, catalog requests get signed with the data credentials. The same happens if a server returns s3.* in /v1/config defaults/overrides, since those end up in additionalProps.

I'd flip this to rest.* > s3.* > default chain: swap these two entries and invert the precedence assertion in TestStaticCredsFromProps. This builds on @laskoviymishka's Java-parity note. Fine here if you prefer, otherwise a follow-up.

[zeroshade]

This is stale after d401049: resolution is no longer per field. staticCredsFromProps uses the s3.* tuple if any s3.* key is set (and errors if that tuple is incomplete), otherwise the rest.* tuple. Fields never mix across namespaces. The same wording is in the const comment at rest.go:97-99 and in configuration.md:59.

Suggested text: "If any s3.* credential key is set, only the s3.* tuple is used and it must be complete; otherwise the rest.* tuple is used."

[zeroshade]

Apart from the stale "resolved per field" wording (see options.go), this row is in the CLI YAML table, and the CLI config cannot set s3.* or rest.* credentials. config.RestOptions only has sigv4-enabled / signing-name / signing-region, and cmd/iceberg/main.go:392-398 only maps those. I'd keep this row as "Enable AWS SigV4 signing for REST." and move the credential-resolution text to the REST catalog options section (the AWS SigV4 row at line 74), where WithAdditionalProps can actually supply these keys.

[zeroshade]

Optional, as @laskoviymishka noted earlier: this reads better as a table-driven test with t.Parallel(), which matches the rest of the file. Not blocking.