zeroshade commented on code in PR #1999:
URL: https://github.com/apache/iceberg-go/pull/1999#discussion_r3960998750
##########
catalog/rest/rest.go:
##########
@@ -1114,6 +1115,11 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
return nil, nil, err
}
+ // Sign with the S3 credentials carried in the catalog
properties when
+ // present, rather than only the AWS default credential
chain.
+ if creds, ok :=
staticCredsFromProps(opts.additionalProps); ok {
+ cfg.Credentials = creds
Review Comment:
**major** — The fix itself is pinned by no test - mutation of the credential
wiring leaves the suite green
TestStaticCredsFromProps exercises only the pure helper
staticCredsFromProps; nothing asserts that createSession actually installs the
returned provider onto cfg.Credentials. The load-bearing line `cfg.Credentials
= creds` can be deleted without any test noticing, so the regression this PR
fixes can silently return. The package already has the harness to pin it:
TestSigv4ConcurrentSigners (rest_internal_test.go:1133) stands up an httptest
server and calls NewCatalog(WithSigV4(), WithSigV4RegionSvc(...)). A test that
scrubs AWS_* env, passes creds only via WithAdditionalProps, and asserts the
captured Authorization header contains the props access key would pin it in ~15
lines. I wrote exactly that as a probe and it passes on this head, so the test
is cheap and available.
<details><summary>Evidence</summary>
```text
Mutated rest.go in place, replacing `if creds, ok :=
staticCredsFromProps(opts.additionalProps); ok { cfg.Credentials = creds }`
with `if _, ok := staticCredsFromProps(opts.additionalProps); ok { _ = ok }`
(helper still called, effect removed). Result: `go test ./catalog/rest/
-timeout=300s` -> `ok github.com/apache/iceberg-go/catalog/rest 5.946s`. Entire
package green with the fix neutralized. By contrast the helper test IS
non-vacuous: swapping the provider args to
NewStaticCredentialsProvider(secretKey, accessKey, ...) turns it red -> `Test:
TestStaticCredsFromProps ... Diff: --- Expected -AK +++ Actual +SK / FAIL`.
Restored via git checkout -- catalog/rest/rest.go.
```
</details>
##########
catalog/rest/rest.go:
##########
@@ -1126,6 +1132,17 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
return cl, cleanup, nil
}
+// staticCredsFromProps returns a static credentials provider built from the S3
+// access-key properties, or ok=false when no key pair is present.
+func staticCredsFromProps(props iceberg.Properties) (aws.CredentialsProvider,
bool) {
+ accessKey, secretKey := props[iceio.S3AccessKeyID],
props[iceio.S3SecretAccessKey]
+ if accessKey == "" || secretKey == "" {
+ return nil, false
Review Comment:
**major** — A partial s3.* credential pair silently signs the request as a
different principal
staticCredsFromProps returns ok=false whenever either half of the key pair
is empty, and the caller then leaves cfg.Credentials pointing at the AWS
default chain. An operator who sets s3.access-key-id but mistypes or omits
s3.secret-access-key gets no error and no warning - the catalog request is
signed with whatever ambient identity happens to be present (env vars, shared
credentials file, instance role). Explicitly configuring a catalog credential
and then being authenticated as a completely different principal is a
surprising, hard-to-debug outcome; the resulting 403 (or worse, an unintended
success under a more privileged ambient role) gives the user no pointer to the
real cause. The new test actively enshrines the behavior ('a lone access key
must not produce a provider') rather than questioning it. Suggest returning an
error when exactly one of s3.access-key-id / s3.secret-access-key is present,
naming the missing property; the fully-empty case should keep falling through to
the default chain. Calibrated against SECURITY-THREAT-MODEL.md this is a
credential-selection correctness bug, not a trust-boundary violation, so major
rather than blocking.
<details><summary>Evidence</summary>
```text
Probe TestProbe_PartialPropsCreds_FallsBackToAmbient: env
AWS_ACCESS_KEY_ID=AKIAAMBIENTENVKEY, props {s3.access-key-id:
AKIAPROPSONLYACCESSKEY} (secret omitted). Output: `NewCatalog err = <nil>` and
`SIGNED WITH: AWS4-HMAC-SHA256
Credential=AKIAAMBIENTENVKEY/20260908/us-east-1/s3tables/aws4_request, ...`.
The explicitly configured AKIAPROPSONLYACCESSKEY was silently discarded and a
different identity signed the request.
```
</details>
##########
catalog/rest/rest.go:
##########
@@ -1114,6 +1115,11 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
return nil, nil, err
}
+ // Sign with the S3 credentials carried in the catalog
properties when
+ // present, rather than only the AWS default credential
chain.
+ if creds, ok :=
staticCredsFromProps(opts.additionalProps); ok {
+ cfg.Credentials = creds
Review Comment:
**minor** — Server /v1/config overrides can now select the SigV4 signing
identity (NOT a vulnerability - flagged for intent confirmation)
createSession is called twice: once from fetchConfig with client-only
properties, and once from init with the merged map after fetchConfig folds in
the server's defaults and overrides (rest.go:1012-1016, and maps.Copy(cfg,
rsp.Overrides) at the end of fetchConfig). Because the new code reads
opts.additionalProps, the second session's signing identity can be chosen by
the REST server via a /v1/config override of s3.access-key-id /
s3.secret-access-key. I checked this against SECURITY-THREAT-MODEL.md before
filing and it is explicitly NOT a vulnerability: the REST control plane is a
trusted input, the doc states 'a malicious REST catalog server sending
dangerous endpoints is [not a vuln] ... credential-selection bugs are often
correctness or specification issues', and no secret reaches a new audience (the
server already knows the credentials it injected). Raising it only so the
maintainer confirms the asymmetry is intended - the bootstrap /v1/config
request signs with the client ide
ntity while every subsequent request may sign with a server-selected one.
##########
catalog/rest/rest.go:
##########
@@ -1114,6 +1115,11 @@ func (r *Catalog) createSession(ctx context.Context,
opts *options) (*http.Clien
return nil, nil, err
}
+ // Sign with the S3 credentials carried in the catalog
properties when
+ // present, rather than only the AWS default credential
chain.
Review Comment:
**minor** — New credential precedence is user-visible but undocumented
This PR establishes a three-tier precedence for SigV4 signing identity -
WithAwsConfig > s3.* catalog properties > AWS default credential chain - and
changes behavior for existing users: someone who today sets s3.access-key-id
for FileIO while relying on an ambient role for catalog signing will, after
this change, start signing catalog requests with the s3.* identity. Nothing in
the PR documents this. website/src/configuration.md lists
catalog.<name>.rest.sigv4-enabled / signing-name / signing-region (lines 59-61)
but says nothing about which credentials sign; and WithSigV4 (options.go:91),
WithSigV4RegionSvc (options.go:98) and WithAdditionalProps (options.go:156)
have no doc comment at all. A sentence in the configuration.md SigV4 rows plus
a godoc line on WithSigV4 would cover it.
--
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]
