Add shared-password auth for protected routes#787
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements password-based authentication for ingress route protection as an alternative to OIDC. The change introduces a Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/ingress/client.go`:
- Around line 413-425: In replaceRoute, avoid clobbering concurrent updates by
not re-encoding the stale route parameter; instead fetch the latest entity (gr
:= c.eac.Get...), decode it to an HttpRoute, apply only the intended auth-field
mutation(s) from the incoming route onto that freshly fetched route, re-encode
that merged route (use route.Encode on the merged object) and call
c.eac.Replace(ctx, attrs, gr.Entity().Revision()) so Replace uses the current
revision; alternatively, if you prefer optimistic concurrency, carry the
caller's original revision through to Replace (pass the original revision
instead of gr.Entity().Revision()) so Replace fails on concurrent
modification—implement one of these two approaches in replaceRoute, referencing
replaceRoute, c.eac.Get, gr.Entity().Revision(), route.Encode and c.eac.Replace.
In `@cli/commands/auth_provider.go`:
- Around line 189-190: The lookup by name is ambiguous because the code (e.g.,
calls to ic.GetOIDCProvider and ic.GetPasswordProvider in auth_provider.go)
assumes names are globally unique across OIDC and password kinds; fix by adding
an explicit provider kind discriminator or enforcing cross-kind uniqueness at
creation: either (A) add/require a --kind/Type field on commands that resolve
providers (update show/remove/route protect and parsing of opts.Name to use a
new lookup like ic.GetProviderByNameAndKind or call
ic.GetOIDCProvider/ic.GetPasswordProvider based on the discriminator), or (B)
when adding providers, validate uniqueness across both kinds so existing
name-based lookups remain safe (update the add commands' duplicate checks). Also
update all affected call sites referenced (lines ~189, 226-254, 277-279,
311-349) to use the chosen approach so password providers are addressable when
an OIDC provider has the same name.
In `@cli/commands/route_protect.go`:
- Around line 162-164: The current ctx.Printf warning about opts.ClaimHeader
prints to stdout (ctx.Printf) which breaks JSON output; change this so the
warning goes to stderr or becomes a validation error: replace the
ctx.Printf("Warning: --claim-header is ignored for password providers\n") call
in the route_protect command handling (the opts.ClaimHeader check) with either a
stderr print (e.g., use the CLI context's stderr-printing helper such as
ctx.Errorf/ctx.PrintErrf or write to os.Stderr) or return an error/validation
failure before emitting the JSON payload so consumers of the JSON output are not
polluted.
In `@servers/httpingress/password_auth.go`:
- Around line 83-92: The returnPath is user-controlled and is currently echoed
into HTML and used for post-login redirects; sanitize and validate it before
use. In serveLoginForm (and the POST login/redirect logic referenced around
lines 95-98 and 107-128), normalize the incoming returnPath with path.Clean,
ensure it is an app-local path that starts with "/" and does not contain a
scheme/host (reject or strip values containing "://" or leading "//"), then
HTML-escape the final value (e.g., html.EscapeString) before inserting into
loginFormHTML; for redirects use the validated local path only (do not redirect
to external URLs). Ensure the same validation/escaping is applied wherever the
return target is parsed and used after login.
- Around line 147-156: The code currently falls back to a cached handler on
s.eac.Get(ctx, string(route.PasswordProvider)) errors, allowing stale
credentials to remain valid; change this to fail-closed by removing the
fallback: when Get returns an error, acquire s.passwordMu (write lock), delete
any entry for passwordHandlers[key] to invalidate the cache, release the lock,
log the error (include route.Host and the error) and return nil,
fmt.Errorf("failed to get password provider: %w", err) instead of returning the
cached handler; update the s.passwordMu usage to use Lock/Unlock when deleting
and remove the RLock/RUnlock fallback path that returns h.
- Around line 202-206: When getOrCreatePasswordHandler returns an error, the
current code logs and calls next(w,r) which bypasses protection; change the
error path in the request handling (the block where handler, err :=
s.getOrCreatePasswordHandler(...)) to stop forwarding the request and instead
return an error response (e.g., write a 5xx or a denial HTTP response) after
logging. Specifically, in the handler creation error branch (referencing
getOrCreatePasswordHandler, the handler variable, s.Log.Error and next), remove
the call to next and write an appropriate http error status/body (e.g.,
http.StatusServiceUnavailable or http.StatusInternalServerError) then return so
the protected app is not served unauthenticated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 485dd9be-e0c3-4415-a46d-72cfb74f834c
📒 Files selected for processing (17)
api/ingress/client.goapi/ingress/ingress_v1alpha/schema.gen.goapi/ingress/schema.ymlblackbox/harness/miren.goblackbox/route_password_test.gocli/commands/auth_provider.gocli/commands/auth_provider_password.gocli/commands/commands.gocli/commands/route_protect.gocli/commands/route_show.gocli/commands/route_unprotect.gopkg/labs/features.yamlpkg/labs/labs.gen.gopkg/oidc/session.goservers/httpingress/httpingress.goservers/httpingress/password_auth.goservers/httpingress/password_auth_test.go
evanphx
changed the base branch from
main
to
mir-1079-remove-globalrouter-labs-flag
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/commands/auth_provider_password.go (1)
14-77: 💤 Low valueConsider adding
--format jsonsupport for consistency.Unlike other auth provider commands (
list,show), this command only outputs human-readable format. For IaC/scripting scenarios, JSON output would be useful.💡 Suggested addition
func AuthProviderAddPassword(ctx *Context, opts struct { Name string `position:"0" usage:"Name for this password provider" required:"true"` Password string `long:"password" description:"Password to protect routes with" required:"true"` Update bool `long:"update" description:"Overwrite an existing provider with the same name (rotates password)"` + FormatOptions ConfigCentric }) error {Then add JSON output handling similar to other commands.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/commands/auth_provider_password.go` around lines 14 - 77, AuthProviderAddPassword currently only prints a human-readable NamedValueList; add a --format option (e.g., Format string `long:"format" choice:"human" choice:"json" default:"human"`) to the opts struct and branch on opts.Format after creating the provider: when format=="json" marshal the created provider (or a small result object with Name and Type) to JSON and write via ctx.Printf, otherwise keep the existing ui.NewNamedValueList rendering; ensure the JSON branch returns proper errors on marshal failure and that output is identical to other auth provider commands (use the same JSON shape those commands produce).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/commands/auth_provider_password.go`:
- Around line 14-77: AuthProviderAddPassword currently only prints a
human-readable NamedValueList; add a --format option (e.g., Format string
`long:"format" choice:"human" choice:"json" default:"human"`) to the opts struct
and branch on opts.Format after creating the provider: when format=="json"
marshal the created provider (or a small result object with Name and Type) to
JSON and write via ctx.Printf, otherwise keep the existing ui.NewNamedValueList
rendering; ensure the JSON branch returns proper errors on marshal failure and
that output is identical to other auth provider commands (use the same JSON
shape those commands produce).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e37e8aed-eb40-4f39-8bcf-978adbcb9f23
📒 Files selected for processing (6)
api/ingress/client.gocli/commands/auth_provider.gocli/commands/auth_provider_password.gocli/commands/route_protect.goservers/httpingress/password_auth.goservers/httpingress/password_auth_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- servers/httpingress/password_auth.go
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Claude and Paul here. We did a paired read across this PR and 786 (the WAF one), with notes from yesterday's product standup where the composable-middleware direction came up. That conversation is what's framing how we're thinking about shape.
What we want to put on the table is some shape asymmetry that's showing up between the three protections. After both PRs land, OIDC and password sit on the route as entity-backed refs (with mutual exclusion enforced manually via Replace), and WAF sits as a plain int. Each of these really wants the same shape: a named, reusable entity the route refs.
Here's the cleanup we're picturing on the auth side. Collapse oidc_provider and password_provider into one auth_provider kind with a type: oidc | password discriminator. The route gets a single auth_provider ref. Mutual exclusion is automatic (one slot, can't double-attach). The Replace-vs-Update dance disappears. Middleware composition becomes one authMiddleware that dispatches on type. Type-specific nullable fields would normally be a smell, but the sparse-attribute entity model handles it fine; you just don't set what doesn't apply.
We're expecting the internal representation might trend toward generic middleware later, but for now the focus is just getting the UX and config shape clean and extensible. The schema reshape sets that up without committing us to anything bigger yet.
CodeRabbit's pass caught real items including an XSS + open redirect on returnPath and a fail-open path in the password middleware (same fail-open shape exists in the OIDC middleware, parallel fix worth doing). Agreed on all of them. Those need fixing regardless of which way the architectural conversation goes.
Marking REQUEST_CHANGES on the auth_provider consolidation specifically. Once that pivot lands we'll come back through for the rest of the review properly.
|
@phinze I'm glad you pointed this out, as I worked through the problem I had the same instinct but opt'd to push through. I'll go back and rework it to have a single auth_provider ref. |
evanphx
force-pushed
the
mir-886-add-shared-password-auth-for-protected-pages
branch
from
8a1878f to
2fa189c
Compare
evanphx
changed the base branch from
mir-1079-remove-globalrouter-labs-flag
to
mir-1081-remove-routeoidc-labs-flag
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/commands/route_unprotect.go (1)
54-63: 💤 Low valueMethod naming is misleading but behavior is correct.
The condition correctly checks both provider fields. However,
DetachOIDCProviderFromRouteclearing both OIDC and password provider fields makes its name misleading—it actually acts as a "clear all protection" operation. Consider renaming toClearRouteProtectionorDetachAllProvidersFromRoutefor clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/commands/route_unprotect.go` around lines 54 - 63, The method DetachOIDCProviderFromRoute is misnamed because it clears both OIDC and password providers; rename it to a clear-all name (e.g., ClearRouteProtection or DetachAllProvidersFromRoute) and update this call site (ic.DetachOIDCProviderFromRoute(ctx, route)) to the new name; also rename the implementation and update all other references (interfaces, callers, tests and docs) to the new identifier so the name accurately reflects that it clears all protection fields on the route.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@blackbox/route_password_test.go`:
- Around line 162-166: The status parsing currently ignores fmt.Sscanf's return
values so a failed parse leaves code==0; update the parsing in the helper that
builds statusStr/status code (and mirror the same change in httpGetWithCookie)
to check Sscanf's (n, err) results and handle failures explicitly: if err != nil
or n != 1, return an error (or call t.Fatalf from the test helper) instead of
silently using 0; alternatively replace fmt.Sscanf(statusStr, "%d", &code) with
strconv.Atoi(statusStr) and handle the error by failing/returning so malformed
status lines don't masquerade as HTTP 0.
---
Nitpick comments:
In `@cli/commands/route_unprotect.go`:
- Around line 54-63: The method DetachOIDCProviderFromRoute is misnamed because
it clears both OIDC and password providers; rename it to a clear-all name (e.g.,
ClearRouteProtection or DetachAllProvidersFromRoute) and update this call site
(ic.DetachOIDCProviderFromRoute(ctx, route)) to the new name; also rename the
implementation and update all other references (interfaces, callers, tests and
docs) to the new identifier so the name accurately reflects that it clears all
protection fields on the route.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e2483ed6-6338-4241-95c4-4d99c12061e2
📒 Files selected for processing (20)
api/ingress/client.goapi/ingress/ingress_v1alpha/schema.gen.goapi/ingress/schema.ymlblackbox/harness/miren.goblackbox/route_password_test.gocli/commands/auth_provider.gocli/commands/auth_provider_password.gocli/commands/commands.gocli/commands/route_protect.gocli/commands/route_show.gocli/commands/route_unprotect.godocs/command-sidebar.jsondocs/docs/command/auth-provider-add-password.mddocs/docs/command/auth-provider.mddocs/docs/command/route-protect.mddocs/docs/commands.mdpkg/oidc/session.goservers/httpingress/httpingress.goservers/httpingress/password_auth.goservers/httpingress/password_auth_test.go
✅ Files skipped from review due to trivial changes (6)
- docs/docs/command/route-protect.md
- docs/docs/command/auth-provider-add-password.md
- docs/command-sidebar.json
- docs/docs/commands.md
- docs/docs/command/auth-provider.md
- api/ingress/ingress_v1alpha/schema.gen.go
🚧 Files skipped from review as they are similar to previous changes (8)
- cli/commands/route_protect.go
- servers/httpingress/httpingress.go
- api/ingress/schema.yml
- cli/commands/route_show.go
- servers/httpingress/password_auth_test.go
- cli/commands/auth_provider.go
- servers/httpingress/password_auth.go
- pkg/oidc/session.go
| statusStr := lines[len(lines)-1] | ||
| var code int | ||
| fmt.Sscanf(statusStr, "%d", &code) | ||
| body := strings.Join(lines[:len(lines)-1], "\n") | ||
| return code, body |
There was a problem hiding this comment.
Check fmt.Sscanf return value for robustness.
fmt.Sscanf returns the number of items successfully scanned. If parsing fails, code remains 0, which could mask real failures as "HTTP 0" rather than failing the test explicitly.
🛠️ Proposed fix
statusStr := lines[len(lines)-1]
var code int
- fmt.Sscanf(statusStr, "%d", &code)
+ if _, err := fmt.Sscanf(statusStr, "%d", &code); err != nil {
+ t.Fatalf("failed to parse HTTP status code from %q: %v", statusStr, err)
+ }
body := strings.Join(lines[:len(lines)-1], "\n")
return code, bodyApply the same fix to httpGetWithCookie at line 223.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| statusStr := lines[len(lines)-1] | |
| var code int | |
| fmt.Sscanf(statusStr, "%d", &code) | |
| body := strings.Join(lines[:len(lines)-1], "\n") | |
| return code, body | |
| statusStr := lines[len(lines)-1] | |
| var code int | |
| if _, err := fmt.Sscanf(statusStr, "%d", &code); err != nil { | |
| t.Fatalf("failed to parse HTTP status code from %q: %v", statusStr, err) | |
| } | |
| body := strings.Join(lines[:len(lines)-1], "\n") | |
| return code, body |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@blackbox/route_password_test.go` around lines 162 - 166, The status parsing
currently ignores fmt.Sscanf's return values so a failed parse leaves code==0;
update the parsing in the helper that builds statusStr/status code (and mirror
the same change in httpGetWithCookie) to check Sscanf's (n, err) results and
handle failures explicitly: if err != nil or n != 1, return an error (or call
t.Fatalf from the test helper) instead of silently using 0; alternatively
replace fmt.Sscanf(statusStr, "%d", &code) with strconv.Atoi(statusStr) and
handle the error by failing/returning so malformed status lines don't masquerade
as HTTP 0.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/route_show.go`:
- Around line 64-87: Replace the current blind Decode flow (which calls
ic.GetEntityStore().GetById into an OidcProvider and falls back to
PasswordProvider) with a GetByIdWithEntity lookup and explicit kind checks: call
ic.GetEntityStore().GetByIdWithEntity(ctx, route.AuthProvider, nil), handle
cond.ErrNotFound exactly as before, then inspect ent.Entity() with
entity.Is(..., ingress_v1alpha.KindOidcProvider) and
ingress_v1alpha.KindPasswordProvider; only then allocate oidcProvider or
pwProvider and call Decode on the correct struct and set protectionType to
"oidc" or "password" respectively. Ensure error wrapping uses the same message
("failed to get auth provider: %w") when non-NotFound errors occur.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1dca799f-4b2b-42dd-b588-6cd16b876d63
📒 Files selected for processing (12)
api/ingress/client.goapi/ingress/ingress_v1alpha/schema.gen.goapi/ingress/schema.ymlcli/commands/auth_provider.gocli/commands/route_protect.gocli/commands/route_show.gocli/commands/route_unprotect.goservers/httpingress/httpingress.goservers/httpingress/oidc.goservers/httpingress/oidc_test.goservers/httpingress/password_auth.goservers/httpingress/password_auth_test.go
✅ Files skipped from review due to trivial changes (1)
- servers/httpingress/oidc_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/ingress/schema.yml
- servers/httpingress/password_auth_test.go
- cli/commands/auth_provider.go
- servers/httpingress/password_auth.go
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
Claude and Paul here, follow-up to last round. Took it for a spin in dev too.
Consolidation landed cleanly, sanitizer holds up under real probes, fail-closed change tested. Nice reshape.
One loose thread from round 1: OIDC side still falls back to the cached handler on store errors while the password side now evicts. Worth aligning so the two don't disagree about revocation behavior. Happy to punt to a follow-up ticket if you'd rather keep this PR scoped.
evanphx
force-pushed
the
mir-1081-remove-routeoidc-labs-flag
branch
from
07ad137 to
2a890dc
Compare
evanphx
force-pushed
the
mir-886-add-shared-password-auth-for-protected-pages
branch
from
9177dc8 to
56b44af
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return "/" | ||
| } | ||
| cleaned := path.Clean("/" + raw) | ||
| if !strings.HasPrefix(cleaned, "/") { |
| } | ||
|
|
||
| h.logger.Info("password authentication successful", "host", h.route.Host) | ||
| http.Redirect(w, r, returnPath, http.StatusFound) |
Add a password_provider entity type parallel to oidc_provider that protects routes with a shared password and browser login form. After entering the correct password, the user gets a 24h encrypted session cookie. Key changes: - New password_provider entity kind with name and bcrypt password_hash - Password provider CRUD in ingress client with route attachment - HTML login form served by ingress middleware at well-known paths - CLI: auth provider add-password, list/show/remove support both types - route protect auto-detects provider type (OIDC vs password) - Mutual exclusion: attaching one provider type clears the other - Use Replace instead of Update when clearing ref fields to avoid stale refs from Encode() skipping empty fields - Blackbox harness: SetEnv support for lab-gated CLI commands
… route replace - Sanitize user-controlled return path to prevent XSS and open redirects - Return 503 instead of serving unprotected content when handler creation fails - Fail closed on provider lookup errors instead of using stale cached credentials - Refactor replaceRoute into read-modify-write to avoid overwriting concurrent changes - Enforce cross-kind uniqueness for auth provider names at creation time - Return error instead of stdout warning for --claim-header in JSON mode
…_provider on HttpRoute Replace the two separate ref fields on http_route with a single polymorphic auth_provider ref that can point to either an OIDC or password provider entity. The provider entity kinds themselves are unchanged. - Schema: single auth_provider ref replaces oidc_provider + password_provider - Client: unified AttachAuthProviderToRoute/DetachAuthProviderFromRoute methods - httpingress: new authMiddleware dispatches to OIDC or password handler based on entity kind, all auth check points and tunnel rejection use AuthProvider - CLI: route protect/show/unprotect and auth provider remove updated
GetById decodes without checking entity kind, so a password provider was always decoded as an empty OIDC provider. Use GetByIdWithEntity and check the entity kind before decoding.
Match the password handler behavior: evict the cached handler and return an error instead of falling back to a stale cached handler. This ensures consistent revocation behavior after provider rotation.
authMiddleware already fetches the entity to determine its kind. Pass that entity directly to getOrCreate*Handler and *Middleware instead of having them re-fetch from the entity store, eliminating a redundant eac.Get per request and a TOCTOU window.
The --password flag is no longer required. When omitted, the CLI prompts interactively with masked input. The @file syntax reads the password from a file. Both avoid exposing the password in shell history and process listings.
evanphx
force-pushed
the
mir-886-add-shared-password-auth-for-protected-pages
branch
from
56b44af to
b4d4f54
Compare
3 participants
Summary
password_providerentity type (parallel tooidc_provider) that protects HTTP routes with a shared password and a browser login formSessionManager)auth provider add-password, unifiedlist/show/removeacross both provider types,route protect --providerauto-detects typeReplaceinstead of merge-basedUpdateto correctly remove stale entity refsTest plan
servers/httpingress/password_auth_test.go)blackbox/route_password_test.go)make lintpasses (no new issues)Closes MIR-886