Conversation
… env vars Resolves painful auth bootstrap UX where the admin token was only shown once on first start with no recovery path short of deleting the auth DB. - ARC_AUTH_BOOTSTRAP_TOKEN: use a known token value on first run instead of generating a random one — enables reproducible/automated deployments - ARC_AUTH_FORCE_BOOTSTRAP: wipe all tokens and recreate admin with the provided value — recovery path when admin token has been lost - Both values require minimum 32 characters; stored as bcrypt hash Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…all) Deleting all tokens on ARC_AUTH_FORCE_BOOTSTRAP was dangerous — a bad actor with env var access could lock out all legitimate admins. Changed to additive recovery: a new 'arc-recovery' admin token is added without touching existing tokens. Legitimate admins retain access and can revoke the recovery token if it was injected maliciously. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to pre-configure an initial admin token via environment variables and provides a recovery path to force-reset tokens. The changes include updates to the configuration loading, the main entry point, and the authentication manager. Feedback suggests wrapping the destructive reset operation in a database transaction to ensure atomicity and refactoring duplicated token creation logic into a shared helper method to improve maintainability.
…otstrap methods
- Fix TOCTOU race in EnsureInitialToken/EnsureInitialTokenWithValue: replace
COUNT→INSERT two-step with a single atomic INSERT...WHERE NOT EXISTS, so
concurrent node startups can never produce duplicate admin tokens
- Extract insertToken private helper: CreateToken and CreateTokenWithValue
shared ~90% identical DB insertion code; now both delegate to one place
- Fix stale ForceBootstrap config comment (previously said "delete all tokens",
now correctly says "add recovery token without removing existing ones")
- Fix misleading comment in ForceAddRecoveryToken ("rotate it to new value"
was not implemented; comment now reflects the actual no-op behavior)
- Add 13 test cases covering CreateTokenWithValue, EnsureInitialTokenWithValue,
and ForceAddRecoveryToken including concurrent safety and idempotency
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the review @gemini-code-assist! Both findings have been addressed in the latest push (743d193): High: ForceResetWithToken transaction safety — The entire Medium: Code duplication between CreateToken and CreateTokenWithValue — Extracted a private Additionally, the TOCTOU race in token initialization was fixed: the Could you re-review with the latest commits? |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a bootstrap and recovery mechanism for admin tokens via environment variables, allowing for pre-configured tokens and a recovery path when locked out. The implementation includes new configuration options, core authentication logic updates, and comprehensive tests. Feedback highlights a discrepancy in the documentation regarding token preservation during recovery and suggests a minor refactor to remove an unused parameter in a private helper method.
| - Minimum 32 characters required | ||
|
|
||
| ### `ARC_AUTH_FORCE_BOOTSTRAP` | ||
| When set to `true` alongside `ARC_AUTH_BOOTSTRAP_TOKEN`, deletes ALL existing tokens and recreates admin with the provided value. |
There was a problem hiding this comment.
There is a significant discrepancy between the documentation and the implementation. The documentation states that ARC_AUTH_FORCE_BOOTSTRAP deletes ALL existing tokens, but the implementation in internal/auth/auth.go (line 865) and the banner in cmd/arc/main.go (line 545) explicitly state that existing tokens are preserved. Please update the documentation to reflect the actual behavior.
| When set to `true` alongside `ARC_AUTH_BOOTSTRAP_TOKEN`, deletes ALL existing tokens and recreates admin with the provided value. | |
| When set to `true` alongside `ARC_AUTH_BOOTSTRAP_TOKEN`, adds a new recovery admin token without removing existing tokens. |
internal/auth/auth.go
Outdated
|
|
||
| // insertToken inserts a pre-hashed token into the database. | ||
| // It is the shared implementation used by CreateToken and CreateTokenWithValue. | ||
| func (am *AuthManager) insertToken(tokenValue, hash, prefix, name, description, permissions string, expiresAt *time.Time) error { |
There was a problem hiding this comment.
The tokenValue parameter is unused in the insertToken function. Since this is a private helper method, it should be removed to keep the signature clean.
| func (am *AuthManager) insertToken(tokenValue, hash, prefix, name, description, permissions string, expiresAt *time.Time) error { | |
| func (am *AuthManager) insertToken(hash, prefix, name, description, permissions string, expiresAt *time.Time) error { |
internal/auth/auth.go
Outdated
| return "", fmt.Errorf("failed to hash token: %w", err) | ||
| } | ||
|
|
||
| if err := am.insertToken(token, hash, tokenPrefix(token), name, description, permissions, expiresAt); err != nil { |
There was a problem hiding this comment.
Update the call to insertToken to match the suggested signature change (removing the unused token argument).
| if err := am.insertToken(token, hash, tokenPrefix(token), name, description, permissions, expiresAt); err != nil { | |
| if err := am.insertToken(hash, tokenPrefix(token), name, description, permissions, expiresAt); err != nil { |
internal/auth/auth.go
Outdated
| return "", fmt.Errorf("failed to hash token: %w", err) | ||
| } | ||
|
|
||
| if err := am.insertToken(tokenValue, hash, tokenPrefix(tokenValue), name, description, permissions, expiresAt); err != nil { |
There was a problem hiding this comment.
Update the call to insertToken to match the suggested signature change (removing the unused tokenValue argument).
| if err := am.insertToken(tokenValue, hash, tokenPrefix(tokenValue), name, description, permissions, expiresAt); err != nil { | |
| if err := am.insertToken(hash, tokenPrefix(tokenValue), name, description, permissions, expiresAt); err != nil { |
… progress doc - insertToken signature had tokenValue as first arg but never used it; removed per Gemini review - docs/progress file still described ForceBootstrap as deleting all tokens; updated to reflect actual additive behavior Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Addressed the remaining findings in 08ceea4: Medium: unused High: docs/progress file described wrong behavior — The progress doc still said The other four comments (ForceResetWithToken transaction, code duplication, and the two related call site suggestions) were already resolved in the previous push — |
Summary
ARC_AUTH_BOOTSTRAP_TOKEN: Set a known admin token value at deploy time instead of catching a random one from logs on first start. Idempotent — no-op if tokens already exist.ARC_AUTH_FORCE_BOOTSTRAP: Recovery path — when set alongsideARC_AUTH_BOOTSTRAP_TOKEN, wipes all tokens and creates a new admin. Useful when the admin token has been lost.Closes the feedback: "The admin token only shows once in logs on first start. If you miss it or need to regenerate, disabling auth also removes the token management routes so there's no way back in without deleting the auth DB."
Test plan
ARC_AUTH_BOOTSTRAP_TOKEN=<32+char value>— verify token works for API callsARC_AUTH_FORCE_BOOTSTRAP— verify token still works (no-op)ARC_AUTH_FORCE_BOOTSTRAP=true+ newARC_AUTH_BOOTSTRAP_TOKEN— verify old token rejected, new token worksgo test ./internal/auth/... -vpasses