Skip to content

feat(s3): serve the bucket policy operations through Hilt's /s3/bucket/policy - #194

Open
pyropy wants to merge 21 commits into
srdjan/feat/iam-hilt-error-mappingsfrom
srdjan/feat/iam-bucket-policy-ops
Open

pyropy wants to merge 21 commits into
srdjan/feat/iam-hilt-error-mappingsfrom
srdjan/feat/iam-bucket-policy-ops

Conversation

@pyropy

@pyropy pyropy commented Sep 28, 2026 •

Copy link
Copy Markdown

Answers GET/PUT/DELETE /{bucket}?policy ahead of versitygw's route table and forwards them to Hilt's /s3/bucket/policy. Versitygw's own controller rejects Forge policy documents and cannot carry preconditions or an ETag.

🤖 Generated with Claude Code

@pyropy

pyropy commented Sep 28, 2026

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-01T14:11:22.597912Z 919e46f Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 06b3229ff3

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread policy_routes.go
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from 06b3229 to 2a810d4 Compare September 30, 2026 15:22
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch 2 times, most recently from 7dd416b to 51b19bd Compare October 1, 2026 12:11
@pyropy

pyropy commented Oct 1, 2026

Copy link
Copy Markdown
Author

@codex review

@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from 51b19bd to bed7fc2 Compare October 1, 2026 12:15

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 51b19bdd05

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread inmem/store.go
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from d68d3f3 to 4f776ff Compare October 1, 2026 12:58
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from 4f776ff to 919e46f Compare October 1, 2026 14:06
@pyropy

pyropy commented Oct 1, 2026

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 919e46f58b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from 919e46f to 1443e96 Compare October 1, 2026 14:24
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch 2 times, most recently from 3c66ea4 to e66184b Compare October 1, 2026 15:18
pyropy added a commit to fil-one/fil-one that referenced this pull request Oct 1, 2026
The gateway renders a refused x-bucket-policy header with the code a
refused PutBucketPolicy body gets, MalformedPolicy, from
fil-forge/ingot#194 on; InvalidArgument stays accepted until that is on
main.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from e66184b to 7db9029 Compare October 2, 2026 11:19
@pyropy
pyropy marked this pull request as ready for review October 2, 2026 11:19
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch 2 times, most recently from db3ff71 to cff35ee Compare October 2, 2026 13:01
pyropy added a commit to fil-one/fil-one that referenced this pull request Oct 2, 2026
The gateway renders a refused x-bucket-policy header with the code a
refused PutBucketPolicy body gets, MalformedPolicy, from
fil-forge/ingot#194 on; InvalidArgument stays accepted until that is on
main.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
pyropy added a commit to fil-one/fil-one that referenced this pull request Oct 7, 2026
The gateway renders a refused x-bucket-policy header with the code a
refused PutBucketPolicy body gets, MalformedPolicy, from
fil-forge/ingot#194 on; InvalidArgument stays accepted until that is on
main.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Comment thread bucketauthority/service.go Outdated
return nil, ErrPreconditionFailed
case "InvalidPrecondition":
return nil, fmt.Errorf("%w: %s", ErrInvalidPrecondition, namedErr.Error())
case "ConcurrentChange":

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the constants for these?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in bf3ed39: the four literals now use bucketpolicysvc.{PolicyNotFound,PreconditionFailed,ConcurrentChange}ErrorName and InvalidPreconditionName. The comment justifying the literals was wrong — hilt/pkg/rpc/service/bucket already imports the service package, so it was in our build graph anyway. Dropped it.

Comment thread server.go
opts = append(opts, s3api.WithRoute(http.MethodGet, web.WellKnownDIDPath, didDocumentHandler(doc)))
// The bucket policy operations are answered here, ahead of the S3 route
// table: versitygw's PutBucketPolicy controller validates the body as an
// AWS-shaped document, which a Forge policy is not, and its backend seam

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

which a Forge policy is not,

Is it not? Why is it not?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do use similar format as AWS bucket policies, but it's not 100% compatible. Here's an example of how principals are listed in AWS bucket policy:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Sid": "editors",
      "Effect": "Allow",
      "Principal": {
        "AWS": [
          "arn:aws:iam::111122223333:user/alice",
          "arn:aws:iam::111122223333:user/bob"
        ]
      },
      "Action": ["s3:GetObject", "s3:PutObject", "s3:ListBucket"],
      "Resource": ["arn:aws:s3:::photos", "arn:aws:s3:::photos/*"]
    },
    {
      "Sid": "readers",
      "Effect": "Allow",
      "Principal": { "AWS": "arn:aws:iam::111122223333:root" },
      "Action": ["s3:GetObject", "s3:ListBucket"],
      "Resource": ["arn:aws:s3:::photos", "arn:aws:s3:::photos/*"]
    },
    {
      "Sid": "noWritesForBob",
      "Effect": "Deny",
      "Principal": { "AWS": "arn:aws:iam::111122223333:user/bob" },
      "Action": "s3:PutObject",
      "Resource": "arn:aws:s3:::photos/*"
    }
  ]
}

While in Forge it would look more like this:

{
  "Statement": [
    {
      "Sid": "editors",
      "Effect": "Allow",
      "Principal": ["alice", "bob"],
      "Action": ["s3:GetObject", "s3:PutObject", "s3:ListBucket"]
    },
    {
      "Sid": "readers",
      "Effect": "Allow",
      "Principal": "*",
      "Action": ["s3:GetObject", "s3:ListBucket"]
    },
    {
      "Sid": "no-writes-for-bob",
      "Effect": "Deny",
      "Principal": ["bob"],
      "Action": ["s3:PutObject"]
    }
  ]
}

pyropy and others added 21 commits October 9, 2026 14:34
…t/policy

GET, PUT and DELETE on /{bucket}?policy are answered ahead of the S3
route table: the signed request and, on a PUT, the body are forwarded to
Hilt as /s3/bucket/policy, which authenticates and authorizes them. The
routes sit ahead of versitygw's controllers because its PutBucketPolicy
validates the body as an AWS-shaped document, which a Forge policy is
not, and its backend seam carries neither preconditions nor an ETag. The
bucket authority maps Hilt's policy rejections; the routes render them
as NoSuchBucketPolicy, MalformedPolicy, PreconditionFailed,
InvalidRequest and OperationAborted, and a CreateBucket's refused
x-bucket-policy header now renders as MalformedPolicy too, the RFC's one
mapping. The in-memory store answers NoSuchBucketPolicy and NotImplemented.

Temporary pins to hilt branch srdjan/feat/iam-policy-command
(fil-forge/hilt#89) and libforge branch
srdjan/feat/s3-bucket-policy-command (fil-forge/libforge#80); re-pin
before merge.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A CreateBucket whose x-bucket-policy header Hilt refuses answered
InternalError: the create path mapped only the bucket-exists names. The
bucket authority now carries the refusal as ErrMalformedPolicy and the
frontend renders it with the code a refused PutBucketPolicy body gets.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Picks up the principal removal that strips its policies before locking the
principal, and the policy store lock-order fixes. Temporary pin to a draft
branch; re-pin to hilt main before this leaves draft.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…alidAccessKeyId

The policy routes mount ahead of versitygw's auth middleware, which is what
turns ErrNoSuchUser into InvalidAccessKeyId everywhere else, so an unknown
or expired key fell through to AccessDenied here.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…enarios run

The all-permission test key named s3:GetBucketPolicy, s3:PutBucketPolicy and
s3:DeleteBucketPolicy, which the published hilt image does not know until
fil-forge/hilt#58 merges, so every forge-mode scenario failed at key
creation with 422 InvalidPermission. The three now join the key only under
the same condition that runs the bucket policy scenario: a hilt override, or
INGOT_ITEST_IAM=1.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
MemStore.BucketPolicy answered NoSuchBucketPolicy (or NotImplemented on a
PUT) for any bucket name, existing or not. It now checks the registry's
buckets first and returns ErrNotFound, as the production authority does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…r span

The policy routes mount ahead of every s3api.WithMiddleware entry and the S3
route table's own steps, so a ?policy request skipped the bucket CORS
headers, its server span, and its error body dropped the request IDs
already in the headers. The handler now applies the bucket CORS rules,
renders errors with the request IDs, and a ?policy request runs the tracing
middleware on its own route; any other request still gets its span once,
from the gateway middleware.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The policy operations go to Hilt's /s3/bucket/policy directly, ahead of
versitygw's route table and iam.Service. The context diagram now lists the
command, the authorization section names the exception, and the package
map points policy_routes.go (and server.go) at both diagrams.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Temporary pin to hilt branch iam-policy-command (stack #86 tip after the
2026-10-02 review round), re-pin before merge.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…header

fil-one/RFC#30 now names the bucket policy fields Statement, Sid, Effect,
Principal and Action with effects Allow and Deny, and CreateBucket no longer
carries an x-bucket-policy header, so Hilt's create never answers
InvalidBucketPolicy. The test documents use the new names, and the
create-path MalformedPolicy mapping goes; the policy routes still render a
refused PutBucketPolicy body as MalformedPolicy.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Temporary pin to the hilt stack branch srdjan/feat/iam-policy-command.
Re-pin to hilt main before this PR leaves draft.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Temporary pin to the hilt stack branch srdjan/feat/iam-policy-command,
which renames the client's Policy to BucketPolicy. Re-pin to hilt main
before this PR leaves draft.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Use the bucketpolicy service's exported error names instead of string
literals, and call the client's renamed BucketPolicy. The service package
is already a transitive dependency through hilt's rpc bucket package, so
matching literally avoided nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pyropy
pyropy force-pushed the srdjan/feat/iam-bucket-policy-ops branch from bf3ed39 to 93ee5dd Compare October 9, 2026 12:36
@pyropy
pyropy requested a review from alanshaw October 9, 2026 12:36

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants