feat: add UCAN handler for listing buckets by alanshaw · Pull Request #13 · fil-forge/hilt

alanshaw · 2026-06-30T13:01:12Z

Adds the /s3/bucket/list handler.

Also a sigv4 and sigv4a verifier.

Depends on:

feat: add UCAN RPC handler stubs #12

Copilot

Pull request overview

Adds first implemented UCAN RPC handler for listing buckets and introduces an internal SigV4/SigV4a verifier (plus presign helper primarily for tests), while centralizing Vault key-path construction in the vault package.

Changes:

Add /s3/bucket/list route with shared SigV4/SigV4a-based authorization and bucket listing logic.
Introduce pkg/sigv4 for parsing/verifying SigV4 + SigV4a and time-bound validation, with unit tests.
Centralize Vault key path helpers (TenantKeyPath, AccessKeyPath) and update API code to use them.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
pkg/vault/paths.go	Adds shared helpers for Vault key paths for tenants and access keys.
pkg/sigv4/sigv4.go	Implements signature parsing, verification dispatch, and time-bound validation.
pkg/sigv4/sign.go	Adds presigning utility (primarily for tests/client-side signing).
pkg/sigv4/hmac.go	Implements SigV4 HMAC signing/verification.
pkg/sigv4/ecdsa.go	Implements SigV4a key derivation and ECDSA signing/verification.
pkg/sigv4/canonical.go	Implements SigV4 canonical request / string-to-sign construction.
pkg/sigv4/sigv4_test.go	Adds known-answer and round-trip tests for SigV4/SigV4a components.
pkg/rpc/list.go	Adds the `/s3/bucket/list` handler and the unit-testable `ListBuckets` function.
pkg/rpc/list_test.go	Adds unit tests for list-buckets authorization + listing behavior.
pkg/rpc/auth.go	Adds shared S3 RPC authorization based on SigV4/SigV4a and Vault-backed access key secrets.
pkg/rpc/rpc.go	Removes the old `/s3/bucket/list` stub from the legacy handler file.
pkg/rpc/rpc_test.go	Updates command/handler registration tests for the new list handler signature.
pkg/fx/rpc_test.go	Updates UCAN server wiring test to construct the list handler with required dependencies.
pkg/api/tenants.go	Switches to centralized Vault tenant key path helper.
pkg/api/access_keys.go	Removes local Vault path helpers and switches to `vault.TenantKeyPath` / `vault.AccessKeyPath`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…store

bajtos

🤖 This review was posted by Claude — automated security review. Advisory only; please verify each item before acting.

Security-focused review of the new authorization surface in this PR (the SigV4/SigV4a verifier, the shared Authorize(), and /s3/bucket/list). The crypto has good test coverage (SigV4 known-answer test, round-trips, tamper/expiry cases), uses constant-time HMAC comparison, and the vault path-helper refactor is byte-for-byte equivalent to what it replaced. The inline comments flag gaps in the auth surface; summary below, most-severe first.

Would fix before merge — authorization lifecycle bypasses (both on the shared Authorize path):

[High] Access-key ExpiresAt is never enforced → expired keys still authenticate. (pkg/rpc/auth.go)
[High] Tenant Status (Disabled/WriteLocked) is never enforced → disabled tenants' keys still work. (pkg/rpc/auth.go)

Signature-coverage / DoS (medium):

[Medium] SigV4a X-Amz-Region-Set (and host) not required to be in SignedHeaders → authz region not bound to the signature for header-auth requests. (pkg/sigv4/sigv4.go)
[Medium, defense-in-depth] Verified signature isn't bound to the invoked operation. (pkg/rpc/auth.go)
[Medium/Low, DoS] Unbounded X-Amz-Region-Set → one provider DB lookup per region. (pkg/rpc/auth.go)

Lower-severity / cleanup (see inline): access-key Buckets scope not applied to ListBuckets (likely by-design — please confirm against the RFC); verbatim/distinguishable auth errors returned to the caller; permission literal duplicating the pkg/api/permissions.go registry.

Two minor notes without a clean inline anchor:

SigV4a X-Amz-Region-Set=* (AWS's all-regions wildcard) is rejected — validateRegion does a literal GetByRegion("*") that never matches. Functional over-rejection, not a security hole.
Presigned requests default to UNSIGNED-PAYLOAD when X-Amz-Content-Sha256 is absent, so the body isn't covered by the signature. Harmless for the GET-only ListBuckets, but a latent risk for future body-carrying handlers (PutObject) that reuse this shared Authorize path.

Generated by Claude Code

bajtos · 2026-07-02T14:51:12Z

+		return nil, fmt.Errorf("invalid access key id %q: %w", sr.AccessKeyID, err)
+	}
+
+	akRec, err := accessKeys.Get(ctx, accessKeyID)


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[High] Expired access keys still authenticate — akRec.ExpiresAt is never checked.

accesskey.Record carries ExpiresAt *time.Time ("when the access key expires"), but Authorize looks the key up here and never compares it against time.Now(). The only time check, sigv4.ValidateTimeBounds (line 86), bounds the signature's freshness (presigned window / clock skew), not the credential lifetime. This RPC is authorized solely by SigV4 + the DB record + permission — no delegation-expiry gates it (the invocation is issued by the provider, and the server has no proof-verification layer).

Failure: a key whose ExpiresAt is in the past keeps listing buckets (and, as write handlers adopt this shared path, keeps full access) given any freshly-signed request. Suggest rejecting when akRec.ExpiresAt != nil && time.Now().After(*akRec.ExpiresAt).

Generated by Claude Code

bajtos · 2026-07-02T14:51:16Z

+		return nil, fmt.Errorf("request signature is no longer valid: %w", err)
+	}
+
+	tenantRec, err := tenants.Get(ctx, akRec.Tenant)


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[High] Disabled / write-locked tenants still authenticate — tenantRec.Status is never checked.

Authorize fetches the tenant here but reads only .Provider, never .Status. Disabled is the explicit lock-out state — DeleteTenant refuses to run unless the tenant is already Disabled ("tenant must be disabled before deletion"), so the lifecycle is Active → Disabled → delete.

Failure: an operator disables a tenant (abuse / non-payment / pending deletion) but its access keys keep authenticating and listing buckets. Because Authorize is the shared choke point for all S3 handlers, this also leaves the upcoming create/delete-bucket handlers open for disabled tenants. Suggest rejecting non-Active tenants here (at minimum Disabled; WriteLocked could still permit reads like this one).

Generated by Claude Code

bajtos · 2026-07-02T14:51:22Z

+	} else if auth := headers.Get("Authorization"); strings.HasPrefix(auth, "AWS4-") {
+		algorithm, credential, signedHeaders, signature = parseAuthorization(auth)
+		date = headers.Get(amzDate)
+		regionSet = headers.Get(amzRegionSet)


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Medium] SigV4a authorization region isn't bound to the signature (header-auth path).

For header-authenticated SigV4a, the region that later drives validateRegion comes from this X-Amz-Region-Set header, but canonicalHeaders (canonical.go:51) only folds in headers listed in SignedHeaders, and Parse never requires x-amz-region-set (or host) to be present in that list (the only validation is the non-emptiness check at line 135). So the region-set value is not covered by the signature.

Failure: an on-path attacker replaying a captured, still-valid SigV4a header-auth request (no secret needed) within the ±15-min skew window can rewrite X-Amz-Region-Set to steer the request to a different region the tenant's provider serves — the signature still verifies. Presigned SigV4a is safe (the region-set is a signed query param). AWS mandates both host and X-Amz-Region-Set be signed; suggest rejecting requests that omit them from SignedHeaders.

Generated by Claude Code

bajtos · 2026-07-02T14:51:27Z

+		log.Error("loading access key secret", zap.Error(err))
+		return nil, err
+	}
+	if err := sigv4.Verify(sr, secret); err != nil {


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Medium — defense-in-depth] The verified signature isn't bound to the invoked operation.

Authorize verifies the caller signed some request (method + URL) but neither it nor ListBuckets checks that the signed request is actually a ListBuckets shape (method/path, or the ?x-id=ListBuckets marker the tests use). The UCAN command (/s3/bucket/list) and the signed args.Request are independent.

Failure: a presigned URL a client created for a different S3 operation (e.g. GetObject) verifies here and authorizes a full bucket listing if the key holds s3:ListAllMyBuckets. Practical severity is reduced by the issuer == tenantRec.Provider gate (line 99) — only the trusted provider can submit these invocations — so this guards against a buggy/confused provider rather than an arbitrary URL holder. Still worth binding the signature to the operation for shared auth infra.

Generated by Claude Code

bajtos · 2026-07-02T14:51:32Z

+// validateRegion confirms the tenant's provider serves one of the regions the
+// request is scoped to, returning the matched region.
+func validateRegion(ctx context.Context, providers provider.Store, regions []string, tenantProvider did.DID) (string, error) {
+	for _, r := range regions {


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Medium/Low — DoS] Unbounded X-Amz-Region-Set → one provider store lookup per region.

splitRegionSet applies no cap or dedup, and this loop does a sequential providers.GetByRegion (a Postgres SELECT … WHERE region=$1) for every region until one matches the tenant's provider.

Failure: a request with a large comma-separated region set forces N sequential DB round-trips, each holding a pooled connection — a ~1 MB header is hundreds of thousands of queries for a single request. In isolation this is post-auth (needs a valid signature); chained with the region-set-not-signed gap (sigv4.go) it becomes credential-less on a captured SigV4a header-auth request. Cheap fix: cap the region-set length in splitRegionSet / Parse.

Generated by Claude Code

bajtos · 2026-07-02T14:51:38Z

+		if opts.Cursor != nil {
+			listOpts = append(listOpts, bucket.WithCursor(*opts.Cursor))
+		}
+		return buckets.ListByTenant(ctx, auth.Tenant.ID, listOpts...)


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Low — likely by-design, please confirm against the RFC] ListBuckets ignores the access key's Buckets scope.

This returns every bucket in the tenant and never filters by auth.AccessKey.Buckets (the bucket.WithIDs(...) option exists and is used for exactly this in pkg/api/access_keys.go). Evidence suggests it is intended: permissions.go maps s3:ListAllMyBuckets → nil ("bucket-level actions … enforced directly by Ingot/Hilt"), object-level scoping is enforced via per-bucket delegation subjects rather than handler filtering, and AWS's ListAllMyBuckets is account-global and non-scopable.

Flagging only so you can confirm against the RFC that a bucket-scoped key is meant to see all of the tenant's bucket names / regions / creation dates.

Generated by Claude Code

bajtos · 2026-07-02T14:51:42Z

+		ok, err := ListBuckets(req.Context(), log, accessKeys, tenants, buckets, providers, secrets, req.Invocation().Issuer(), req.Task().Arguments())
+		if err != nil {
+			log.Error("list buckets failed", zap.Error(err))
+			return res.SetFailure(err)


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Low] Verbatim, distinguishable auth errors surfaced to the caller.

Authorize returns distinct messages for "unknown access key" vs "invalid request signature" and wraps raw vault/store errors; this SetFailure(err) passes them back. If that reaches the client it enables access-key-DID enumeration and leaks internal error detail (vault/store errors).

Value is limited (access-key IDs are semi-public — they appear in every X-Amz-Credential) and impact depends on whether SetFailure serializes the message over the wire. Suggest returning a generic "access denied" to the caller and keeping the detail in logs.

Generated by Claude Code

bajtos · 2026-07-02T14:51:45Z

+)
+
+// permListAllMyBuckets is the S3 permission required to list a tenant's buckets.
+const permListAllMyBuckets = "s3:ListAllMyBuckets"


🤖 This comment was posted by Claude — automated security review. Advisory only; please verify before acting.

[Low — cleanup] Permission literal re-implements the existing registry.

This hardcodes "s3:ListAllMyBuckets" and does a manual slices.Contains rather than reusing the canonical set in pkg/api/permissions.go (s3PermissionCommands / validS3Permission). The two lists can drift over time. Minor, but consider centralizing the permission check so handlers and the creation-time validator share one source of truth.

Generated by Claude Code

bajtos

Besides the comments posted by Claude above (most of them seem legitimate to me), I have a few comments of my own, see below.

I'll be AFK in the next two weeks; I think it will be best to not wait for my return and ask other @fil-forge/engineering team members to approve.

bajtos · 2026-07-02T14:53:17Z

+// canonicalRequest builds the AWS canonical request string per the SigV4 spec.
+func (s *SignedRequest) canonicalRequest() string {


Have you considered using an existing SigV4-verification implementation?

According to Claude, there are two options - SeaweedFS or MinIO, but MinIO is no longer mantained.

https://github.com/seaweedfs/seaweedfs/blob/master/weed/s3api/auth_signature_v4.go

Also:

If you roll your own verifier:

always compare with hmac.Equal/subtle.ConstantTimeCompare;

pre-escape the URI exactly as AWS expects — per pkg.go.dev, AWS "recommend[s] that you explicitly escape the request when using this signer outside of the SDK to prevent possible signature mismatch … by setting URL.Opaque on the request … in the form of: ///";

enforce a ≤15-minute X-Amz-Date window and reject stale/future requests to stop replay;

test against AWS's published aws4_testsuite vectors.

Have you considered using an existing SigV4-verification implementation?

I did, neither of those implement Sigv4a though...

test against AWS's published aws4_testsuite vectors.

Now that is interesting. That resource no longer exists on the AWS documentation site AFAICT...I have found the page on wayback machine and will use it though.

bajtos · 2026-07-02T14:54:28Z

+// AccessKeyPath is the vault key under which an access key's private key is
+// stored, scoped beneath its tenant.
+func AccessKeyPath(tenantID, accessKeyID did.DID) string {
+	return TenantKeyPath(tenantID) + "/access/" + accessKeyID.String()


As I commented in one of the other requests, can we use a more specific name than "access"?

Suggested change

return TenantKeyPath(tenantID) + "/access/" + accessKeyID.String()

return TenantKeyPath(tenantID) + "/access-key/" + accessKeyID.String()

Yep, tracked in #23 - I'll get there!

bajtos · 2026-07-02T15:00:33Z

+		ok, err := ListBuckets(req.Context(), log, accessKeys, tenants, buckets, providers, secrets, req.Invocation().Issuer(), req.Task().Arguments())
+		if err != nil {
+			log.Error("list buckets failed", zap.Error(err))
+			return res.SetFailure(err)


Note that in Ingot, we will need to distinguish different errors so that we can return a canonical S3 error response. See https://docs.aws.amazon.com/AmazonS3/latest/developerguide/ErrorResponses.html

Something to keep in mind when designing the RPC error responses in Hilt.

Implements the `/s3/request/authorize` UCAN RPC handler and refactors shared SigV4/SigV4a request authorization into an injectable service, enabling reuse across S3 RPC handlers (e.g., list buckets) and aligning permission-to-command delegation behavior across the REST and RPC surfaces. **Changes:** - Adds a new `/s3/request/authorize` handler that authenticates an S3 request, derives a verification key, and mints per-action Forge command delegations for the gateway. - Refactors request authorization into `pkg/rpc/service/auth.Authorizer` and updates existing RPC handlers (notably bucket listing) to depend on it. - Extracts/exports S3 permission mapping utilities into `pkg/s3perm` for reuse by both REST access-key creation and RPC request authorization.

Adds docs for Claude and humans. Depends on: * #16 --------- Co-authored-by: Srdjan <stankovic.srdjo@gmail.com>

Adds a client library for Hilt that Ingot can import and use. Depends on: * #16

…ckets-handler

alanshaw added 2 commits June 30, 2026 13:57

feat: add UCAN handler for listing buckets

d58ce03

refactor: clean up

74fe170

alanshaw requested a review from Copilot June 30, 2026 15:48

Copilot started reviewing on behalf of alanshaw June 30, 2026 15:48 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Comment thread pkg/sigv4/sigv4.go Outdated

Comment thread pkg/sigv4/sigv4.go

Comment thread pkg/sigv4/sigv4.go

alanshaw added 2 commits June 30, 2026 17:34

feat: add derive key and verify with derived key methods

7a15f89

fix: fail if content sha header is missing

6081348

alanshaw requested review from Peeja, bajtos, frrist and pyropy June 30, 2026 17:03

feat: implement /s3/request/authorize handler

684fee7

alanshaw mentioned this pull request Jul 1, 2026

feat: implement /s3/request/authorize handler #15

Merged

alanshaw added 3 commits July 1, 2026 14:11

feat: add remaining UCAN handlers

5937d68

fix: language

2f33674

fix: upload service delegations come from config not the delegations …

3312be5

…store

bajtos reviewed Jul 2, 2026

View reviewed changes

alanshaw changed the base branch from ash/feat/ucan-rpc-stubs to main July 2, 2026 14:58

bajtos reviewed Jul 2, 2026

View reviewed changes

alanshaw changed the base branch from main to ash/feat/ucan-rpc-stubs July 2, 2026 15:05

alanshaw and others added 7 commits July 2, 2026 19:14

test: test against aws-sig-v4-test-suite

d146d7d

docs: add agent and human docs (#17)

4bacee7

Adds docs for Claude and humans. Depends on: * #16 --------- Co-authored-by: Srdjan <stankovic.srdjo@gmail.com>

feat: add client library (#18)

e5aabc2

Adds a client library for Hilt that Ingot can import and use. Depends on: * #16

Merge branch 'ash/feat/remaining-ucan-handlers' into ash/feat/list-bu…

d8a0e51

…ckets-handler

refactor: address deferred feedback and other cleanup

709219e

chore: gofmt

ff21658

alanshaw closed this Jul 6, 2026

alanshaw deleted the ash/feat/list-buckets-handler branch July 6, 2026 09:24

		// canonicalRequest builds the AWS canonical request string per the SigV4 spec.
		func (s *SignedRequest) canonicalRequest() string {

	return TenantKeyPath(tenantID) + "/access/" + accessKeyID.String()
	return TenantKeyPath(tenantID) + "/access-key/" + accessKeyID.String()

Uh oh!

Conversation

alanshaw commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alanshaw commented Jun 30, 2026 •

edited

Loading