Repository navigation
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f08be2a440
ℹ️ 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".
37026f0 to
7d3b768
Compare
7d3b768 to
99ed840
Compare
…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>
99ed840 to
93b2c13
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
93b2c13 to
a30b36c
Compare
a30b36c to
275bf5a
Compare
103f0f7 to
dc4d3b3
Compare
dc4d3b3 to
d7d19a1
Compare
d7d19a1 to
7f48010
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a2d7a76f6
ℹ️ 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".
…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>
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>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Policy PUT can race bucket deletion and leave grants unrevoked, while the dependency remains temporarily pinned to an open PR.
Review effort: Balanced
Findings: 1
Open (4)
What changed in this PR
Adds /s3/bucket/policy RPC support for forwarding S3 bucket-policy operations through Hilt.
Changes:
- Implements authenticated GET, PUT, and DELETE policy handling.
- Adds SigV4 payload hashing and signed preconditions.
- Wires the RPC route, client API, failure mappings, and tests.
| File | Description |
|---|---|
AGENTS.md |
Documents the policy RPC. |
go.mod |
Updates libforge dependency. |
go.sum |
Updates dependency checksums. |
pkg/api/service/bucketpolicy/service.go |
Exposes resolved-bucket policy removal. |
pkg/client/client.go |
Adds the policy RPC client method. |
pkg/fx/rpc.go |
Registers the new route. |
pkg/fx/rpc_test.go |
Verifies route grouping. |
pkg/rpc/failure.go |
Maps policy failures. |
pkg/rpc/failure_test.go |
Tests failure mapping. |
pkg/rpc/policy.go |
Adds the RPC handler. |
pkg/rpc/rpc_test.go |
Verifies command registration. |
pkg/rpc/service/auth/auth.go |
Restricts policy operations to service keys. |
pkg/rpc/service/auth/operation.go |
Classifies policy requests and permissions. |
pkg/rpc/service/auth/operation_test.go |
Tests policy classification. |
pkg/rpc/service/bucket/policy.go |
Implements policy operations. |
pkg/rpc/service/bucket/policy_test.go |
Tests policy behavior. |
pkg/sigv4/sign.go |
Adds payload-hash presigning. |
pkg/sigv4/sigv4.go |
Exposes verified payload hashes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| github.com/docker/docker v28.5.2+incompatible | ||
| github.com/exaring/otelpgx v0.12.0 | ||
| github.com/fil-forge/libforge v0.0.0-20260914100934-63f5b20252fe | ||
| github.com/fil-forge/libforge v0.0.0-20260928131613-0060aa6cb75a |
|
/forge-perf |
forge-perf
Verdict: within noise, judged on the median against its noise band. Positive deltas mean the branch ingests faster. Main runs the current main set; branch swaps in this commit's image. The runs table on the forge-perf page lists each run. |
There was a problem hiding this comment.
Delete doesn't seem to be called by anything? Can we just have a Delete or a Remove method please?
There was a problem hiding this comment.
Dropped Delete alongside Put and Get in 5bd71c9
| // Policy invokes /s3/bucket/policy with a forwarded GetBucketPolicy, | ||
| // PutBucketPolicy or DeleteBucketPolicy request; body is the policy document | ||
| // on a PUT. It returns no delegations. | ||
| func (c *Client) Policy(ctx context.Context, req s3.Request, body []byte, opts ...MethodOption) (*s3bkt.PolicyOK, error) { |
There was a problem hiding this comment.
Can we call this BucketPolicy?
| func (c *Client) Policy(ctx context.Context, req s3.Request, body []byte, opts ...MethodOption) (*s3bkt.PolicyOK, error) { | |
| func (c *Client) BucketPolicy(ctx context.Context, req s3.Request, body []byte, opts ...MethodOption) (*s3bkt.PolicyOK, error) { |
| } | ||
| var opts []bucketpolicysvc.WriteOption | ||
| if unconditional { | ||
| opts = append(opts, bucketpolicysvc.Unconditional()) |
There was a problem hiding this comment.
So, typically I'd expect a functional option, something like bucketpolicysvc.WithPrecondition(ifMatch) that takes the ifMatch parameter (since it's optional) and it wouldn't be passed to Write.
…vice The bucket RPC service takes the management API's bucket policy service, so an RPC policy write goes through the same validation, compare-and-set and key rotation as a policy PUT. Put splits into a public Write that takes an already-resolved tenant and bucket, and the auth package exports HeaderValue for reading signed request headers. Carried over from the closed PR #73 (fil-forge/hilt), without its x-bucket-policy header on CreateBucket. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Ingot forwards S3 GetBucketPolicy, PutBucketPolicy and DeleteBucketPolicy requests as one command; the request's method selects the operation. A service key holding s3:GetBucketPolicy, s3:PutBucketPolicy or s3:DeleteBucketPolicy reaches it, a principal-bound key is refused. A PUT's body must hash to the request's signed payload hash, so nothing on the path can replace the document. If-Match and If-None-Match: * are optional signed headers; without one the write is unconditional, as on AWS. The policy service's rejections (PolicyNotFound, InvalidPrecondition, PreconditionFailed, ConcurrentChange) become receipt failures for Ingot to render. Temporary pin to 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>
…uthorized bucket A write-locked tenant was refused nothing: the authorizer let it through for reads and left writes to handlers that never checked. It now refuses uploads, bucket creation and PutBucketPolicy for such a tenant, keeping reads, listings and deletes, as the management API's TenantStatus contract says. The refusal is OperationNotPermitted, which Ingot already renders as AccessDenied. The bucket policy operation acts on the bucket the authorizer resolved, by DID, instead of looking it up again by name, and a present but empty If-Match or If-None-Match is an invalid precondition rather than an unconditional write. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#52 enforces write-locked in the authorizer and revokes the cached write grants, so this stack no longer carries its own guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n, and test the policy client method GET and HEAD shared the policy branch, so HEAD /bucket?policy was classified as GetBucketPolicy and the policy handler answered it with the document; the operation is GET-only and a HEAD now classifies as the plain bucket operation. The client's Policy method gains the test its neighbours have: the forwarded request and body reach the handler and the ETag and policy come back, and a failure receipt is an error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…e header CreateBucket no longer carries a policy (RFC 30, 113d4ded), so the failure mapping's comment names the one source left, a PutBucketPolicy body. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Bucket-policy ETags travel in the HTTP ETag header; a list cannot carry one per item, so PrincipalPolicy drops the etag field. Callers that need a policy's ETag read it with S3 GetBucketPolicy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
They resolved a tenant's external id and a bucket name for the partner-key policy routes, which are gone. The /s3/bucket/policy handler has both resolved already and calls Write and Remove; the tests now do the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…oneMatch Write took the precondition as a positional *string, nil meaning the create, plus an Unconditional option to waive it. The precondition is now optional: without an option the write is unconditional, as an S3 PutBucketPolicy without either header is; IfMatch(etag) and IfNoneMatch() condition it. precondition in the RPC handler returns the tag and the If-None-Match flag directly. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| // IfMatch makes the write replace the bucket's policy only if its current ETag | ||
| // is etag. | ||
| func IfMatch(etag string) WriteOption { | ||
| return func(in *bucketpolicystore.Input) { in.IfMatch, in.Unconditional = &etag, false } |
There was a problem hiding this comment.
Consider flipping this from Unconditional to Conditional. The zero value of boolean is false, so in go you often see the default for an option worded in a way that makes sense for it to be false. That way when you create a type instance and want the field to assume the default you simply don't set it.
In this case I think the default is for the request to be unconditional, so the default is Conditional: false.
i.e. you go from:
in := bucketpolicystore.Input{
Bucket: bucketID,
Tenant: tenantID,
Policy: doc,
Unconditional: true,
}to:
in := bucketpolicystore.Input{
Bucket: bucketID,
Tenant: tenantID,
Policy: doc,
}It also reads better IMO.
There was a problem hiding this comment.
Done in 9535388: the store input's flag is Conditional, so the zero value is the unconditional write and Write sets nothing unless an option asks for a check.
| // IfNoneMatch makes the write create the bucket's policy only if the bucket | ||
| // has none (If-None-Match: *). | ||
| func IfNoneMatch() WriteOption { | ||
| return func(in *bucketpolicystore.Input) { in.IfMatch, in.Unconditional = nil, false } |
There was a problem hiding this comment.
Consider validating that in.IfMatch is already nil. Currently it will silently overwrite a previous IfMatch option.
There was a problem hiding this comment.
Done in 9535388: WithCondition returns ErrInvalidPrecondition when the input already carries a condition (WriteOption now returns an error, and Write surfaces it before touching the store). Covered by TestWriteCondition.
| // with the current ETag replaces or deletes, and If-None-Match: * creates. | ||
| // Both headers, an If-None-Match other than *, or a header the signature does | ||
| // not cover is [bucketpolicysvc.ErrInvalidPrecondition]. | ||
| func precondition(authz *auth.AuthorizedRequest, headers map[string]string) (ifMatch string, ifNoneMatch bool, err error) { |
There was a problem hiding this comment.
Again, ifNoneMatch bool doesn't really make sense to me. Or at least I would expect the ifMatch return value to hold "*" when this is true.
There was a problem hiding this comment.
Done in 9535388: precondition returns one *bucketpolicysvc.Condition, nil when neither header was sent, IfMatch(tag) for If-Match and IfNoneMatch("*") for If-None-Match. I kept the two apart rather than putting * in ifMatch, since If-Match: * means the opposite in HTTP (any current policy).
| return nil, fmt.Errorf("a DeleteBucketPolicy cannot carry If-None-Match: %w", bucketpolicysvc.ErrInvalidPrecondition) | ||
| } | ||
| // An empty ifMatch is the unconditional delete. | ||
| if err := s.policyWrites.Remove(ctx, tenantID, bucketID, name, ifMatch); err != nil { |
There was a problem hiding this comment.
I'd probably make it a *string to signify optionality.
There was a problem hiding this comment.
Done in 9535388: Remove takes ifMatch *string; nil is the unconditional delete. The handler passes &cond.Match or nil.
| // IfNoneMatch makes the write create the bucket's policy only if the bucket | ||
| // has none (If-None-Match: *). | ||
| func IfNoneMatch() WriteOption { | ||
| return func(in *bucketpolicystore.Input) { in.IfMatch, in.Unconditional = nil, false } |
There was a problem hiding this comment.
I'm not sure about IfNoneMatch as an option name in general - if none match...what? What are we matching against?
Personally, I'd make a WithCondition(c Condition) WriteOption option:
type Condition struct {
Negate bool
Match string
}
func IfNoneMatch(v string) Condition { return Condition{Negate: true, Match: v} }
func IfMatch(v string) Condition { return Condition{Match: v} }
// then you use as:
func main() {
service.Write(ctx, tenant, bucket, policy, WithCondition(IfNoneMatch("*")))
// or
service.Write(ctx, tenant, bucket, policy, WithCondition(IfMatch(etag)))
}However, we don't really ever want to allow none match on anything other than *. We're really talking about the difference between Add and Replace and I'd perhaps make this more explicit and split the method into these two variations instead of having an option that decides the functionality.
There was a problem hiding this comment.
Went with your sketch in 9535388: Condition{Negate, Match}, IfMatch(etag), IfNoneMatch(v) and one WithCondition(c) WriteOption. WithCondition refuses a negated condition other than "*" and a second condition on the same write. I left Write as one method rather than splitting into Add and Replace: the handler also needs the unconditional write for a PutBucketPolicy without a precondition, which fits neither. #96 is rebased on top (43f9b3a), ingot a671ee5 re-pins.
The service takes one WithCondition option carrying a Condition, built by
IfMatch(etag) or IfNoneMatch("*"); a second condition or a negated one other
than "*" is ErrInvalidPrecondition. The RPC handler parses the headers into
that Condition, and Remove takes an optional ETag. The store input's flag is
Conditional, so the zero value is the unconditional write.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Serves S3
GetBucketPolicy,PutBucketPolicyandDeleteBucketPolicyas one Hilt command,/s3/bucket/policy, reached by service keys only. The PUT body must match the signed payload hash, andIf-Match/If-None-Matchare optional signed preconditions.?policyclassified into three operations with their own permissionspkg/rpc/service/bucket/policy.go: operation, body hash check, preconditionsclient.Policyfor Ingotsigv4.PayloadHashand a test presign option/s3/bucket/policyfrom Hilt to Ingot (feat(stack): delegate /s3/bucket/policy from hilt to ingot smelt#65 does it for local stacks)GetBucketPolicy'sETagheaderUnknownBucket🤖 Generated with Claude Code