feat: optional root user by alanshaw · Pull Request #12 · fil-forge/versitygw

alanshaw · 2026-09-11T10:13:46Z

Makes the root user optional - this is not supported in Ingot since it has no ability to locally authorize a user.

Copilot

🟡 Changes recommended

The authentication path has a critical empty-credential fallback issue, and the public entry point still rejects the advertised optional-root configuration.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds optional root-user matching and IAM fallback across authentication paths.

Changes:

Adds root configuration and matching helpers.
Updates standard, presigned, and POST authentication.
Adds enabled/disabled root-user tests.

File summaries

File	Summary	Review
`s3api/middlewares/root-user_test.go`	Tests enabled and disabled root behavior.	—
`s3api/middlewares/presign-auth.go`	Applies root matching to presigned authentication.	—
`s3api/middlewares/object-post-auth.go`	Applies root matching to POST authentication.	—
`s3api/middlewares/authentication.go`	Defines root configuration and account resolution.	Moderate (2 votes): Public entry-point validation still rejects empty root credentials. Critical (3 votes): Empty credentials can fall through to IAM and resolve as an admin account.

Review details

Files reviewed: 4/4 changed files
Comments generated: 2
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Unresolved critical findings remain in ACL ownership, root credential validation, and empty-owner handling.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

s3api/server.go:78

This constructor comment repeats the same inaccurate promise: an empty access key is rejected before IAM lookup by accounts.getAccount, so not every access key resolves through IAM in rootless mode. Limit the statement to non-empty access keys (or explicitly mention empty keys are invalid).

// New constructs the S3 API server. The gateway runs without a root account
// unless WithRootUser is passed: every access key then resolves through iam.

Files reviewed: 9/9 changed files
Comments generated: 4
Review effort level: Lite

+	if cfg.RootUserAccess == "" && !cfg.iamBackendConfigured() {
+		return fmt.Errorf("root user access and secret key must be provided when no IAM backend is configured")



 	// initilaze the default value setter middleware
-	app.Use("*", middlewares.SetDefaultValues(root, region))
+	app.Use("*", middlewares.SetDefaultValues(server.Router.root, region))


Copilot

🟡 Changes recommended

Unresolved empty-key, LDAP lookup, and disabled-root ACL ownership issues must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (3)

auth/iam.go:216

This predicate is now used by every backend's CreateAccount as well as lookup. With the root account disabled, an empty access key therefore passes the duplicate-root check; the admin CreateUser path validates only the role and can persist an account that all request authentication paths reject before IAM lookup. Reject empty account.Access during account creation (or in the admin controller) instead of using the root-match predicate for this validation.

func isRootAccess(root Account, access string) bool {
	return root.Access != "" && access == root.Access

auth/iam_ldap.go:137

When root is disabled, isRootAccess(ld.rootAcc, account.Access) is false for an empty access key. The admin CreateUser path passes the unmarshaled account here without validating Access, so rootless LDAP IAM can now create an empty-key entry even though authentication rejects empty keys before lookup. Reject empty account.Access before this root check (and use the same validation across the IAM backends) instead of creating an unusable entry.

	if isRootAccess(ld.rootAcc, account.Access) {

auth/iam_vault.go:193

When root is disabled, isRootAccess(vt.rootAcc, account.Access) is false for an empty access key. The admin CreateUser path passes the unmarshaled account here without validating Access, so rootless Vault IAM can now write an empty-key secret entry even though authentication rejects empty keys before lookup. Reject empty account.Access before this root check (and use the same validation across the IAM backends) instead of creating an unusable entry.

	if isRootAccess(vt.rootAcc, account.Access) {

Files reviewed: 20/20 changed files
Comments generated: 4
Review effort level: Lite

Copilot

🟡 Changes recommended

Unresolved critical API and root-credential issues, plus a moderate Helm deployment issue, block approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

embedgw/embedgw.go:58

Rootless startup is still blocked for Helm deployments: chart/templates/secret.yaml requires both credential values and chart/templates/deployment.yaml always injects those secret keys. With IAM enabled but no root credentials, the chart fails before validateRootUser can accept the IAM-backed configuration, so the chart templates/values need to be updated with this feature.

	// authentication. Leave both RootUserAccess and RootUserSecret empty to
	// run without a root account, so every access key resolves through the
	// configured IAM backend; that requires one of the IAM backends below,
	// since single-account mode has no other account to authenticate.

Files reviewed: 21/21 changed files
Comments generated: 3
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

A critical Helm credential-wiring issue for rootless IAM deployments remains unresolved.

Get a fresh assessment by requesting another Copilot review.

Review details

Files reviewed: 23/23 changed files
Comments generated: 1
Review effort level: Lite

Copilot

🟡 Changes recommended

Helm backend detection and rootless IAM bootstrap documentation need correction before approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

chart/templates/deployment.yaml:9

This check treats iam.enabled as proof that an IAM backend is configured, but the gateway validates the actual trigger fields. For example, iam.enabled=true with a non-internal type and no extraEnv trigger still starts with no backend and is rejected by validateRootUser; conversely, a valid LDAP/Vault/etc. backend supplied through the documented extraEnv is rejected when iam.enabled=false. Validate the rendered backend configuration (or explicitly restrict and validate supported types) rather than this boolean alone.

{{- if and (not (include "versitygw.rootCredentialsEnabled" .)) (not .Values.iam.enabled) }}
  {{- fail "root credentials (auth.accessKey and auth.secretKey, or auth.existingSecret) are required unless iam.enabled=true" }}

Files reviewed: 30/30 changed files
Comments generated: 1
Review effort level: Lite

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

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: * fil-forge/versitygw#12

feat: optional root user

20cc07e

alanshaw requested a lite review from Copilot September 11, 2026 10:13

Copilot started reviewing on behalf of alanshaw September 11, 2026 10:18 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread s3api/middlewares/authentication.go

Comment thread s3api/middlewares/authentication.go Outdated

alanshaw mentioned this pull request Sep 11, 2026

refactor: remove root account acess fil-forge/ingot#142

Merged

feat: move root user config to options

d20705f

alanshaw requested a lite review from Copilot September 11, 2026 10:58

Copilot started reviewing on behalf of alanshaw September 11, 2026 10:59 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

refactor: address review comments

9802a83

alanshaw requested a lite review from Copilot September 11, 2026 13:46

Copilot started reviewing on behalf of alanshaw September 11, 2026 13:47 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread auth/iam_ldap.go

Comment thread s3api/server.go Outdated

Comment thread auth/iam_internal.go

Comment thread auth/iam_s3_object.go

refactor: address review comments

713608a

alanshaw requested a lite review from Copilot September 11, 2026 14:09

Copilot started reviewing on behalf of alanshaw September 11, 2026 14:11 View session

Copilot AI reviewed Sep 11, 2026

View reviewed changes

Comment thread auth/iam.go Outdated

Comment thread s3api/admin-server.go

Comment thread s3api/server.go

alanshaw and others added 3 commits September 11, 2026 15:34

Update isRootAccess to validate Secret field

39953c1

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

chore: gofmt

f041164

fix: tests

680a511

alanshaw requested a lite review from Copilot September 14, 2026 11:12

Copilot started reviewing on behalf of alanshaw September 14, 2026 11:14 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread embedgw/embedgw.go

feat: update helm chart to allow optional root credentials

0a88ed3

alanshaw requested a lite review from Copilot September 14, 2026 11:41

Copilot started reviewing on behalf of alanshaw September 14, 2026 11:47 View session

Copilot AI reviewed Sep 14, 2026

View reviewed changes

Comment thread chart/README.md Outdated

Update README with authentication and IAM instructions

cc86e1b

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

alanshaw added this pull request to stack #14 September 14, 2026 12:22

alanshaw removed this pull request from stack #14 October 9, 2026 09:13

Navigation Menu

Repository navigation

feat: optional root user - #12

feat: optional root user#12
alanshaw wants to merge 9 commits into
ash/fix/expect-continue-size-capfrom
ash/feat/optional-root-user

alanshaw commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if cfg.RootUserAccess == "" && !cfg.iamBackendConfigured() {
		return fmt.Errorf("root user access and secret key must be provided when no IAM backend is configured")

Conversation

alanshaw commented Sep 11, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (3)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants