Repository navigation
refactor: remove root account acess - #142
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Removes versitygw “root” account support so all requests must resolve via the Hilt-backed IAM integration, and updates configuration, tests, and documentation to reflect the new contract.
Changes:
- Disable versitygw root user by passing an empty
RootUserConfigand removing root credentials from config/validation. - Update S3 frontend and reqscope docs/comments to remove “root request bypass” assumptions.
- Add a unit test pinning that plain (non-
did:key) access keys are rejected without consulting Hilt; bumpversitygwdependency.
Reviewed changes
Copilot reviewed 16 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| server.go | Disables root user config in S3 API wiring; removes root credential validation and updates comments. |
| s3frontend/bucket.go | Updates ACL/policy comments and tenant attribution assumptions now that root is gone. |
| itest/testdata/config-smallblob.yaml | Removes root credential fields from sample config. |
| itest/testdata/config-retention.yaml | Removes root credential fields from sample config. |
| itest/testdata/config-mpttl.yaml | Removes root credential fields from sample config. |
| internal/reqscope/reqscope.go | Updates comments to remove “root bypassed IAM” wording. |
| iam/service_test.go | Adds test asserting plain access keys are rejected and don’t contact Hilt. |
| iam/service.go | Updates module docs/comments to reflect “every key goes through IAM” semantics. |
| go.mod | Bumps github.com/fil-forge/versitygw to a newer pseudo-version. |
| go.sum | Updates checksums for the versitygw bump. |
| docs/diagrams.md | Updates diagrams/narrative to remove root path; documents root as disabled. |
| config/server.go | Removes root access/secret from ServerConfig. |
| config/config_test.go | Updates config test helper to stop providing root access/secret. |
| config/config.go | Removes root access/secret from top-level config, mapping, and validation. |
| README.md | Updates description to “every S3 request” now that root is removed. |
| DESIGN_NOTES.md | Updates design notes to reflect no root account and new failure mode. |
| CLAUDE.md | Updates internal architecture/config docs for rootless behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
The authentication refactor and repository-wide authorization documentation changes warrant final human review.
Review details
Suppressed comments (3)
DESIGN_NOTES.md:178
- These docs now make the no-root invariant explicit, but the PR leaves source comments describing root-account requests as live paths (
tenantkey/tenantkey.go:31-33,blockstore/forge.go:45-52and208-214, ands3frontend/copy.go:237-247). That is now misleading; please update or remove those references so the authentication and proof-store contract is consistent throughout the repository.
- There is **no root account**: versitygw's built-in root user is disabled
(an empty `RootUserConfig`), so every access key resolves through hilt. A
key hilt has not issued is rejected as InvalidAccessKeyId.
docs/diagrams.md:56
- This edge now says the
/s3/request/authorizeendpoint is used for every request, butiam.Service.GetUserAccountForRequestfirst accepts requests throughauthorizeLocalwhen the derived key and proof chains are cached (iam/service.go:174-184), so no Hilt endpoint is called on that path. Please qualify the diagram as Hilt authorization on a local-cache miss (or otherwise mention the cached fast path).
ingot -->|"/s3/request/authorize (every request)<br/>/s3/bucket/info (lazy chain completion)<br/>/s3/bucket/create, delete, list"| hilt
docs/diagrams.md:684
- This is broader than the implementation:
/healthand/.well-known/did.jsonare mounted outside the S3 auth chain, and malformed or rejected access keys never receivereqscope.ProofStore. Please qualify this as applying to successfully authenticated S3 access-key requests so the architecture note does not imply that every HTTP request has Forge retrieval authority.
- Every request carries a proof store: versitygw's root account is disabled,
so no auth path skips the hilt-backed IAM lookup.
- Files reviewed: 17/18 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
itest failure is expected until #143 merges |
351e446 to
fbade15
Compare
Published from fil-forge/ingot#142 - Digest: `sha256:43eca8e52b1b3125f9c88e4ef94d49b27bc2b7895e68ab7429824db93d8999e1` - Commit: fil-forge/ingot@5c13d14 - Publish run: https://github.com/fil-forge/ingot/actions/runs/34843131975 Merging rewrites [`nodes/dev/apps/versions.env`](https://github.com/fil-forge/infra-nodes/blob/main/nodes/dev/apps/versions.env) and queues the image. The node picks it up on its next reconcile pass, waits for a safe proving window and then restarts the service, so the deploy happens well after this merges. Co-authored-by: fil-forge-bot[bot] <318653112+fil-forge-bot[bot]@users.noreply.github.com>
Ingot does not support root access - it has no way to locally authorize a user to use the Forge network. Depends on: * fil-forge/ingot#142
Removes root account access (which was totally broken anyway).
i.e. IAM auth was skipped, but every Forge-facing handler would fail on a root request.
Depends on:
refs https://linear.app/filecoin-foundation/issue/FIL-913