Conversation
Extends the tenant management RFC with principals, bucket policies, and principal-bound access keys, implementing the fil-one bucket policies ADR (IAM M2) on Hilt and Ingot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The only caller was the one-off migration sweep, which loops over the idempotent single-principal PUT instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Cache consistency, revocation recovery, concurrency, and API contract gaps must be resolved before implementation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Extends Forge tenant management with principal-based IAM enforced by Hilt and Ingot.
Changes:
- Adds principals, service credentials, and principal-bound access keys.
- Defines bucket policies, authorization, and cache invalidation.
- Specifies schema, migration, and example workflows.
File summaries
| File | Description |
|---|---|
rfcs/2026-09-forge-s3-tenant-iam.md |
Defines the proposed Forge S3 tenant IAM architecture and APIs. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 11
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A principal is a (tenant, userId) row with no key material; per-request delegations are signed with the tenant key. Narrowings reach the gateway through a new Swarf /principal/invalidate command and firehose event, published by Hilt's service identity from a configured publisher list. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Cache invalidation races and incomplete cross-gateway bucket deletion can preserve revoked access.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
rfcs/2026-09-forge-s3-tenant-iam.md:152
- Including
s3:ListAllMyBucketsineffective(p, b)means the set is never empty, contradicting both the omitted-empty-buckets rule and authorization step 6. An existing but inaccessible bucket would consequently take the 403 path instead ofUnknownBucket/404. Keep this tenant-level action outside the per-bucket effective set.
plus `s3:ListAllMyBuckets` (see [action vocabulary](#action-vocabulary)). A bucket with no policy has an empty effective set for every principal. The service credential is not a principal and is not evaluated against policies.
rfcs/2026-09-forge-s3-tenant-iam.md:66
- Both minting paths return the only copy of the secret once, but retries return either a credential-less 200 or 409, and there is no delete/rotation route. If Hilt commits and the response is lost, the console cannot recover the credential and migration or tenant setup is permanently stuck. Define an idempotency/replay mechanism or a reset/rotation operation before relying on this flow.
- MUST be returned once, in the body of the call that minted it, as `serviceCredential: { accessKeyId, secretAccessKey }`.
- MAY be minted for a tenant that has none through `POST /tenants/{tenantId}/service-credential`, which returns it the same way and answers 409 when one exists. This is the migration path for existing tenants.
rfcs/2026-09-forge-s3-tenant-iam.md:141
- This item is under “reject with 422” but says the same condition is reported as 404, leaving the API contract contradictory. Keep document-validation failures in the list and state the cross-tenant/not-found bucket response separately.
- a bucket that belongs to another tenant, reported as 404.
rfcs/2026-09-forge-s3-tenant-iam.md:502
- In PostgreSQL,
UNIQUE (bucket_id, principal)permits multiple rows whereprincipal IS NULL, so it does not enforce one wildcard index entry per bucket as intended. UseNULLS NOT DISTINCTon supported PostgreSQL versions, or separate partial unique indexes for explicit and wildcard principals.
UNIQUE (bucket_id, principal)
- Files reviewed: 1/1 changed files
- Comments generated: 4
- Review effort level: Balanced
|
@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: bfce0390e5
ℹ️ 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".
…locked publish Service credentials become a list per tenant with mint, list, and delete routes, so a lost mint response or a rotation is a second mint and a delete. A policy write publishes an invalidation for every principal whose effective set changed in either direction, so a widening reaches a warm gateway cache. The write publishes inside its transaction with the policy, key, or principal row locked, and the authorize path reads those rows with a shared lock, so a gateway cannot refill its cache from the old policy between publish and commit. The Tenant API bucket create is dropped: a bucket without a policy is reachable by service credentials only, and the console writes the policy right after creating the bucket over S3. With it goes the Ingot requirement to register buckets learned from bucket-info. Bucket deletion states the one-gateway assumption it relies on. ListAllMyBuckets leaves the per-bucket effective set, the cross-tenant bucket case is a 404 on its own, the two bucket-configuration reads are authorized at Hilt on every request, and the wildcard index row gets a partial unique index. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The invalidation invocation names Hilt as its own subject, the shape the revoke command already uses, because the standard validator refuses a foreign subject with no proofs. Existing keys are removed by `hilt migrate iam` before the schema migration, which now refuses to run while a key remains. A service credential stores its accessKeyId as its name so the schema is unchanged. Principal removal holds the principal row lock across idempotent cleanup steps instead of claiming one transaction over tables and the vault. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The implementation mirrors the parent RFC's service-key path on /s3/request/authorize: each per-request delegation is keyed to itself and Ingot obtains the chain through the key's stored delegations over /s3/bucket/info, so the two key kinds answer in one shape. Bucket creation issues the named principals' delegations after the policy write rather than in one transaction with it; a failure still deletes the bucket with everything the create wrote. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A write whose revocations reach Swarf but whose commit fails left Ingot refilling its cache from the old delegations, and the console's retry republished identical revoke invocations that Swarf deduplicated, so the change waited for midnight. Ingot now records revoked CIDs until the next UTC midnight and serves, without caching, any authorize response that carries one. The failure and retry text moves into one Error recovery subsection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@alanshaw @bajtos I've done another round of changes to the RFC. Most notable changes are:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2cd4a6c94
ℹ️ 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".
|
|
||
| **A response that outran a write.** An authorize response that left Hilt before a write took its locks can arrive at Ingot after the write's revocations. Its delegations are in the set, so Ingot serves that one request under the old state and caches nothing. Swarf polls its store once per second before emitting a record, so the response must stay in flight longer than that interval for this to occur. | ||
|
|
||
| **Restart.** Ingot keeps the set in memory and a restart clears it together with the cache. A restart between a failed commit and its successful retry refills the cache from the stored delegations, and the retry's duplicate revocations emit nothing, so that key sees the change at midnight. |
There was a problem hiding this comment.
If a write publishes successfully but its database commit fails, and Ingot restarts before the retry, this sequence explicitly allows the restarted gateway to cache the old delegations and then receive no event when the retry commits because Swarf suppresses the duplicate revocations. A narrowing or deletion can therefore be acknowledged while the old access remains usable until midnight, contradicting the stated firehose-latency staleness bound; the retry needs a newly emitted invalidation or the revoked-CID state must survive restarts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It's not possible to re-emit the invalidations.
|
|
||
| 1. Publishes a revocation for every delegation of each of the principal's keys, in one Swarf request. | ||
| 2. Removes the principal from every statement naming it. A statement left with no principal is deleted, and a policy left with no statement is deleted. | ||
| 3. Deletes the principal's keys: rows, then vault entries. |
There was a problem hiding this comment.
If the database commit fails after these vault deletions, PostgreSQL can restore the key rows and delegations but cannot restore the externally deleted private keys. The principal then appears intact after rollback but /s3/request/authorize cannot read its signing keys, contradicting the following claim that a failed removal changes nothing. Vault deletion needs a post-commit/outbox workflow or another compensating mechanism rather than occurring before commit.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A policy edit rewrites the delegations of every key bound to a changed principal over the edited bucket, so the principal needs no identity of its own for a revocation to target, and "touches no key" no longer describes the mechanism. Matches RFC fil-one/RFC#30. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
alanshaw
left a comment
alanshaw
left a comment
There was a problem hiding this comment.
This is nearly there, there's a couple of things we have to figure out before I can approve this though. Please see the comments.
|
|
||
| 1. The storage system computes a principal's effective actions on a bucket from that bucket's policy alone: `allow` minus `deny`, with explicit `deny` taking precedence. | ||
| 2. A policy edit changes the authority of every key bound to an affected principal without reissuing any key. | ||
| 3. Before Hilt acknowledges a policy change, it publishes to Swarf every revocation required by a narrowing of a principal's effective actions or by a widening on a bucket the key already reaches. Ingot's existing firehose consumer then clears the affected caches within firehose latency. |
There was a problem hiding this comment.
within firehose latency
Not sure I understand what this means?
There was a problem hiding this comment.
I think that Claude inferred we're using AWS Firehose; will remove this sentance.
| 2. A policy edit changes the authority of every key bound to an affected principal without reissuing any key. | ||
| 3. Before Hilt acknowledges a policy change, it publishes to Swarf every revocation required by a narrowing of a principal's effective actions or by a widening on a bucket the key already reaches. Ingot's existing firehose consumer then clears the affected caches within firehose latency. | ||
| 4. The console signs member traffic, including presigned URLs, with a key bound to that member's principal, while retaining service keys for traffic with no member actor. | ||
| 5. One policy model governs every principal. Existing keys continue to work, and a tenant can migrate to principals without reissuing keys or introducing a window in which the network rejects requests. |
There was a problem hiding this comment.
You might want to define terms before this since you're using many of them here.
| | Body | Kind | Authority | | ||
| | ---------------------------------------------- | ------------------- | ----------------------------------------------------------------------------- | | ||
| | `{ name, permissions, buckets?, expiresAt? }` | Service key | The `permissions` and `buckets` in the request, as the parent RFC specifies. | | ||
| | `{ name, principalId, expiresAt? }` | Principal-bound key | Whatever the bucket policies give the principal, held as delegations that follow the policies. | |
There was a problem hiding this comment.
Might be an idea to have a discriminator key or value e.g.
type AccessKeyBody =
| { type: 'service', name: string, permissions: string[], buckets?: string[], expiresAt?: string }
| { type: 'principal', name: string, principalId: string, expiresAt?: string }type AccessKeyBody =
| { $service: { name: string, permissions: string[], buckets?: string[], expiresAt?: string } }
| { $principal: { name: string, principalId: string, expiresAt?: string } }The latter is easier to model in a typed language.
There was a problem hiding this comment.
While I do not disagree, I would prefer to keep the current access key creation contract unchanged.
|
|
||
| ### Tenant creation | ||
|
|
||
| `PUT /tenants/{tenantId}` is unchanged. Its body currently names the region, and Hilt binds the tenant to the provider that serves it. This RFC does not change Hilt's tenant-to-region model; changes that allow a tenant to span multiple Forge regions are tracked separately in [FIL-1133](https://linear.app/filecoin-foundation/issue/FIL-1133). As today, the console creates the tenant and then creates a service key with no bucket list for bucket provisioning. Nothing in the IAM model below depends on a tenant serving exactly one region. |
There was a problem hiding this comment.
I don't think you really need any of this explanation for this RFC...maybe just the first sentence. IMO it is implicit that if you're not describing a change in this document then it does not change.
IDK if you want to say something about not being able to assign permissions to principals that belong to other tenants...I don't think this is something you can do with AWS anyway so perhaps it doesn't matter.
| } | ||
| ] | ||
| } | ||
| ``` |
There was a problem hiding this comment.
Have you given any thought to how a policy is encoded deterministically so that you can easily determine equivalency of two policies? ...also useful for the etag.
Might be overkill but you might want to encode with IPLD and transform the arrays into sets (I don't think the order actually matters for statement lists, principal lists or action lists - i.e. they are sets not arrays).
Example schema in typescript:
interface Policy {
statement: {
[link: Link<Statement>]: {}
}
}
interface Statement {
sid?: string
effect: 'allow' | 'deny'
principal: '*' | { [name: string]: {} }
action: { [name: string]: {} }
}When the statements, principals and actions are defined as objects we gain determinism. IPLD will sort object keys so that the order you added them does not matter, you also gain deduplication for free.
i.e. you encode (deterministically) each statement and use set of CIDs as the policy statements, then you can deterministically encode a policy and use that CID as the Etag, for example.
So, for example if someone gives you either one of:
{
"statement": [{
"effect": "allow",
"principal": ["a", "b"],
"action": ["s3:PutObject", "s3:GetObject"]
}]
}or
{
"statement": [{
"effect": "allow",
"action": ["s3:PutObject", "s3:GetObject", "s3:PutObject"],
"principal": ["b", "a"]
}]
}...it'll encode to the same value and produce the same hash (CID).
There was a problem hiding this comment.
I haven't considered this, but I love the idea ❤️
|
|
||
| - an action outside the [policy vocabulary](#action-vocabulary); in particular `s3:CreateBucket`, `s3:DeleteBucket`, and `s3:ListAllMyBuckets`. `s3:*` is accepted and stands for the whole vocabulary, | ||
| - a `principal` that is neither the string `"*"` nor a non-empty list of ids of live principals of the tenant. `"*"` inside a list is rejected; the wildcard has one spelling, | ||
| - an empty `statement` list or a statement with an empty `action` list. The caller deletes the policy instead, |
There was a problem hiding this comment.
So, not reject with 422? Oh, you mean the caller needs to send a DELETE request instead. Worth clarifying.
There was a problem hiding this comment.
So, not reject with 422? Oh, you mean the caller needs to send a DELETE request instead. Worth clarifying.
Yes, since the empty statement and or empty action are invalid, caller would need to issue an explicit DELETE request.
|
|
||
| Buckets are created and deleted over S3 only with a service key. `CreateBucket` cannot be granted by a bucket policy because the target bucket does not exist yet. This RFC also deliberately excludes `DeleteBucket` from the policy vocabulary so that bucket lifecycle operations remain service-key-only, matching the ADR's decision that member keys cannot create or delete buckets. A principal-bound key therefore receives `AccessDenied` from both operations. `aws s3 mb` with a member's key fails, and members create buckets through the console. The user-facing S3 documentation needs to state this limitation. | ||
|
|
||
| A new bucket's policy is carried on the create request in the Forge-specific `x-bucket-policy` header as base64-encoded JSON; AWS does not define this header. Ingot forwards the S3 request to Hilt in `/s3/bucket/create` as it does today, headers included, so the header requires no new Ingot behavior. Hilt decodes it, validates it exactly as `PUT .../policy` would, creates the bucket, writes the policy, and issues every key of each named principal its delegations over the new bucket. If the policy write or the issuance fails, Hilt deletes the bucket with whatever it had written. No bucket outlives a failed write of the policy its create request carried. The header MUST be among the request's `SignedHeaders`; Hilt refuses a create whose policy header is present and unsigned, since otherwise anything on the path could replace the document. A document that fails validation refuses the create with `InvalidBucketPolicy`, which Ingot renders as `InvalidArgument` (400). Ingot caps the request head at 8 KB, leaving roughly 5 KB for the decoded JSON policy under the expected request headers. The console MUST check the encoded request size before sending the create and refuse to submit a request that would exceed Ingot's header limit; an oversized request may otherwise be rejected by Ingot before it reaches Hilt. |
There was a problem hiding this comment.
Why would we not use PutBucketPolicy for this?
What if I'm creating a bucket with aws s3 mb, can I not then add a policy? I have to go via the console?
Do we need /tenants/{tenantId}/buckets/{bucketName}/policy? Why don't we just add a new command for policy management /s3/bucket/policy and have Ingot invoke it when it receives a PutBucketPolicy or GetBucketPolicy request?
There was a problem hiding this comment.
Why don't we just add a new command for policy management /s3/bucket/policy and have Ingot invoke it when it receives a PutBucketPolicy or GetBucketPolicy request?
I like that idea, that could work 👍🏻
| ```jsonc | ||
| { | ||
| "name": "laptop", // unique per principal | ||
| "principalId": "8f2c...", // makes the key principal-bound |
There was a problem hiding this comment.
Why is
principalIdin the request body and not in the URL path?I just realized that the reason we include
principalIdinside the access key creation body is to reuse the same access key creation route for both service and principle bound keys.
By this reasoning should we not also reuse the same endpoint for listing a principal's keys and not create a new one?
|
|
||
| **Swarf rejects the publish, or the publish fails.** Hilt returns 500 and commits nothing. The old state remains in force, so no principal receives authority beyond the committed state. | ||
|
|
||
| **Swarf accepts the revocations and the commit fails.** The delegations are revoked at Swarf and still stored in Hilt. Ingot consumes the revocations, drops each affected key's per-key cache, and records each revoked CID in an in-memory set kept until the next UTC midnight plus clock skew, the same horizon as the cache. On the key's next request Ingot misses the cache, calls Hilt, and receives the same stored delegations. Their CIDs are in the set, so Ingot authorizes the request from the response and caches nothing. Every request from that key reaches Hilt until a committed write returns delegations with new CIDs. The member holds exactly the access the committed state grants, at one Hilt round trip per request. |
There was a problem hiding this comment.
Wait so you're saying Ingot keeps a set of CIDs it knows are revoked, and will authorize the request but not cache the proofs if they are revoked? I don't think that makes sense 😬. How does Ingot distinguish between these and proofs that have genuinely been revoked - like when a key is deleted?
I think the revoke needs to happen inside the transaction and rollback if it fails.
There was a problem hiding this comment.
Wait so you're saying Ingot keeps a set of CIDs it knows are revoked, and will authorize the request but not cache the proofs if they are revoked? I don't think that makes sense 😬.
Yes, that's correct. Ingot rejects to cache outdated proofs, but continues to defer the authorization to Hilt. Ingot will only cache proofs once the Hilt state is consisted with the published proof chain. Motivation for this type of behaviour is that ultimately Hilt keeps the ultimate authority over what get's authorized, and in my opinion requests should be authorized or not depending on current Hilt state.
How does Ingot distinguish between these and proofs that have genuinely been revoked - like when a key is deleted?
In both cases (proofs don't match cached revocations; proofs match cached revocations) keys have been genuinely revoked, but in former case Hilt state is not yet consistent. Note that this state only happens when the transaction commit fails.
I think the revoke needs to happen inside the transaction and rollback if it fails.
We do revocations inside the transaction, but we can't rollback publishing the revocations over Swarf. In case of the transaction commit failure revocations will be published, but the transaction commit needs to retried.
There was a problem hiding this comment.
Ah, ok. I guess that makes sense.
Co-authored-by: ash <alan138@gmail.com>
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>
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>
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>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
Extends the tenant management RFC with principals, bucket policies, and principal-bound access keys, implementing the fil-one bucket policies ADR (proposed in fil-one/fil-one#696) on Hilt and Ingot.
📖 Preview