From 3b43f05a8367acdea03d54828760b48e53fac568 Mon Sep 17 00:00:00 2001 From: Minseong Choi Date: Sat, 4 Jul 2026 20:47:33 +0900 Subject: [PATCH] refactor(api): drop dead login concurrency limiter and reconcile passwordless comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The passwordless migration (b330d77) removed the password-login route, leaving concurrencyLimiter — its bcrypt concurrency cap — with no caller, and scattered stale "local-password" / "change-password" references through the surviving auth code's comments. - Remove the dead concurrencyLimiter (type + newConcurrencyLimiter + acquire): no caller, no struct field, no test. Reword the one streamLimiter doc that contrasted against it. - Realign comments in repo.go, pgrepo.go, session.go, util.go to the passwordless reality: staff lookups feed email-OTP / passkey / setup redeem, not a password compare; RevokeUserSessionsExcept and DeleteAllPasskeyCredentialsForUser are retained (uncalled) for the P5 account-remediation path (#78); "local sessions" no longer implies a password. Comments and dead code only; no behavior change. Full WSL test tree green. --- internal/api/api.go | 44 ++--------------------------------------- internal/api/pgrepo.go | 15 ++++++++------ internal/api/repo.go | 42 ++++++++++++++++++++------------------- internal/api/session.go | 18 ++++++++--------- internal/api/util.go | 3 ++- 5 files changed, 44 insertions(+), 78 deletions(-) diff --git a/internal/api/api.go b/internal/api/api.go index 895fbcb..97462d3 100644 --- a/internal/api/api.go +++ b/internal/api/api.go @@ -622,54 +622,14 @@ func (c *cooldownLimiter) release(name string, reservedAt time.Time) { } } -// ---- login concurrency cap ---- - -// concurrencyLimiter bounds how many holders may run a guarded section at once. It -// backs the public login route's bcrypt cap (handleLogin): a buffered channel of n -// tokens; acquire takes one WITHOUT blocking (returning ok=false when the section -// is already full), release returns it. Unlike cooldownLimiter — a per-key time -// window — this bounds simultaneity, not frequency, which is the right shape for a -// CPU-costly section a flood would otherwise pin every core running. A non-positive -// cap disables it (acquire always admits, release is a no-op), mirroring the "zero -// disables" idiom of WakeCooldown and MaxRunningServers. -type concurrencyLimiter struct { - slots chan struct{} -} - -// newConcurrencyLimiter builds a limiter admitting at most n concurrent holders. A -// non-positive n yields a disabled limiter (nil slots) that admits everyone. -func newConcurrencyLimiter(n int) *concurrencyLimiter { - if n <= 0 { - return &concurrencyLimiter{} - } - return &concurrencyLimiter{slots: make(chan struct{}, n)} -} - -// acquire tries to take a slot without blocking. It returns a release func and true -// on success, or nil and false when the section is already at capacity. The disabled -// limiter (nil slots) always admits and returns a no-op release. release MUST be -// called exactly once on the success path, so it reads naturally as `release, ok := -// l.acquire(); if !ok { shed }; defer/inline release()`. -func (l *concurrencyLimiter) acquire() (release func(), ok bool) { - if l.slots == nil { - return func() {}, true - } - select { - case l.slots <- struct{}{}: - return func() { <-l.slots }, true - default: - return nil, false - } -} - // ---- per-principal stream cap ---- // streamLimiter bounds how many concurrent guarded sections a single KEY may hold at // once. It backs the per-principal SSE stream cap (console + build-log relays): each // relay blocks for the life of a client's attachment and, under a stalled reader, // pins a goroutine plus a kube-apiserver follow connection, so an unbounded number of -// them from one principal is a control-plane connection-exhaustion vector. Unlike the -// login concurrencyLimiter (a single global semaphore), this counts per key. A +// them from one principal is a control-plane connection-exhaustion vector. Unlike a +// single global semaphore, this counts per key. A // non-positive max disables it (acquire always admits, release is a no-op), the same // "zero disables" idiom as the other levers. type streamLimiter struct { diff --git a/internal/api/pgrepo.go b/internal/api/pgrepo.go index da0f4a8..d88aacb 100644 --- a/internal/api/pgrepo.go +++ b/internal/api/pgrepo.go @@ -722,7 +722,7 @@ func (p *PGRepo) IsProtectedAdminLink(ctx context.Context, mcUUID string) (bool, return ok, err } -// ---- local-password auth (spec §B) ---- +// ---- staff account lookups (spec §B, passwordless) ---- // UserByUsername loads a staff login projection by username, or ErrNotFound. // The account is passwordless — staff authenticate via email-OTP / passkey, so @@ -836,9 +836,10 @@ func (p *PGRepo) RevokeSession(ctx context.Context, tokenHash string) error { return err } -// RevokeUserSessionsExcept revokes every live session of a user except -// keepTokenHash — the change-password flow logs out the account's other devices -// while keeping the current one. +// RevokeUserSessionsExcept revokes every live session of a user except keepTokenHash +// — logs out an account's other devices while keeping the current one. Its original +// caller (the change-password flow) was removed in the passwordless migration; it is +// retained for the account-remediation path (P5, #78) and currently has no caller. func (p *PGRepo) RevokeUserSessionsExcept(ctx context.Context, userID, keepTokenHash string) error { _, err := p.db.ExecContext(ctx, `UPDATE sessions SET revoked_at = now() @@ -1030,8 +1031,10 @@ func (p *PGRepo) DeletePasskeyCredential(ctx context.Context, userID, id string) // DeleteAllPasskeyCredentialsForUser unbinds every passkey a user holds. Unlike the // single-credential delete this does NOT report ErrNotFound on zero rows: removing all of // a user's passkeys when they have none is a successful no-op, since "the user holds no -// passkeys" is exactly the intended post-condition. The change-password flow calls it so a -// passkey planted through a transiently-hijacked session cannot survive the remediation. +// passkeys" is exactly the intended post-condition. It is the remediation that stops a +// passkey planted through a transiently-hijacked session from surviving; its original +// caller (the change-password flow) was removed in the passwordless migration, so it is +// currently uncalled, retained for the account-remediation/reset path (P5, #78). func (p *PGRepo) DeleteAllPasskeyCredentialsForUser(ctx context.Context, userID string) error { _, err := p.db.ExecContext(ctx, `DELETE FROM webauthn_credentials WHERE user_id = $1`, userID) diff --git a/internal/api/repo.go b/internal/api/repo.go index cbe5461..bf4e79f 100644 --- a/internal/api/repo.go +++ b/internal/api/repo.go @@ -92,9 +92,9 @@ type StaffUser struct { // be public (unlike a session token), so it is safe at rest. CredentialID is the // authenticator's globally-unique handle (base64url) and PublicKey the COSE key // (base64); SignCount is the uint32 signature counter captured at registration. -// LastUsedAt is nil until an assertion is verified — the login/step-up path that -// would stamp it is out of scope for this enrollment-only slice (deferred), so it -// stays nil through the flow this type backs. +// LastUsedAt is nil until an assertion stamps it. The passkey login door now exists +// (Public /auth/passkey/login/{begin,finish}, #72), but no path yet writes +// last_used_at, so in practice it stays nil; wiring the stamp is a follow-up there. type PasskeyCredential struct { ID string UserID string @@ -352,11 +352,13 @@ type Repo interface { // can only unbind their OWN credential. No matching (user, id) row → ErrNotFound, // so a stale or cross-user id cannot silently no-op as success. DeletePasskeyCredential(ctx context.Context, userID, id string) error - // DeleteAllPasskeyCredentialsForUser unbinds every passkey a user holds. The - // change-password flow calls it so a passkey planted via a transiently-hijacked - // session does not survive the remediation (password reset + session revoke) as a - // standing login foothold. Removing zero rows is success, not an error — an account - // with no passkeys is the intended post-condition either way. + // DeleteAllPasskeyCredentialsForUser unbinds every passkey a user holds — the + // remediation that stops a passkey planted via a transiently-hijacked session from + // surviving as a standing login foothold. Its original caller, the change-password + // flow, was removed in the passwordless migration, so it currently has no production + // caller; it is retained for the account-remediation/reset path (P5, #78). Removing + // zero rows is success, not an error — an account with no passkeys is the intended + // post-condition either way. DeleteAllPasskeyCredentialsForUser(ctx context.Context, userID string) error // ---- player game-login: username-collision reclaim (spec §B3) ---- @@ -398,18 +400,18 @@ type Repo interface { // name to protect). Keyed by UUID — the only identity velocity knows. IsProtectedAdminLink(ctx context.Context, mcUUID string) (bool, error) - // ---- local-password auth (spec §B) ---- + // ---- staff account lookups (spec §B, passwordless) ---- // UserByUsername loads the login projection of a staff account by its unique - // username, or ErrNotFound. The caller compares PasswordHash itself so the - // anti-enumeration dummy-hash compare runs even on a miss; a player row (NULL - // password_hash → empty PasswordHash) is returned too and is rejected by the - // caller's hash compare, never by leaking "no such user". + // username, or ErrNotFound. Its caller is the `felis breakGlass` recovery TUI, + // which resolves an Owner/Operator username before sending an email-OTP — there is + // no password compare (the account is passwordless). A non-staff (role='user') row + // resolves too; callers that require staff enforce the role themselves. UserByUsername(ctx context.Context, username string) (*StaffUser, error) - // UserByID loads the same staff projection by user id, or ErrNotFound. The - // change-password flow uses it to re-verify the caller's current password: the - // session yields a user id, not a username, so this is the id-keyed counterpart - // of UserByUsername. + // UserByID loads the same staff projection by user id, or ErrNotFound. Callers hold + // a session (which yields a user id, not a username) and need the account behind it + // — e.g. setup redeem/status resolving the lockdown session's owner. It is the + // id-keyed counterpart of UserByUsername. UserByID(ctx context.Context, id string) (*StaffUser, error) // UserByEmail resolves a VERIFIED email address to its login projection, // case-insensitively, or ErrNotFound (spec §B email-first login). It is the @@ -419,9 +421,9 @@ type Repo interface { // else's login by typing their email. Matching is on lower(email) to align with // the users_verified_email_unique partial index (migration 0010), which // guarantees at most one verified row per normalized address, so the result is - // unambiguous. A player row (empty PasswordHash) resolves too — email-first - // login is passwordless and does not consult the hash — unlike the password - // path, which this deliberately does not gate on. + // unambiguous. A player (role='user') row resolves too — email-first login is + // passwordless and role-agnostic here; the door that consumes this result decides + // what each role may do. UserByEmail(ctx context.Context, email string) (*StaffUser, error) // UpsertOwner creates or resets the single Owner account direct-to-Postgres // (the `felis setup` / `felis breakGlass` recovery path). role is forced to diff --git a/internal/api/session.go b/internal/api/session.go index 9a56c83..da1c93c 100644 --- a/internal/api/session.go +++ b/internal/api/session.go @@ -15,23 +15,23 @@ import ( "time" ) -// Local-password sessions (spec §B). The remote face authenticates statelessly -// with a Cloudflare-Access JWT and sets no cookie; local-password auth, used on -// op.console when Zero Trust is not configured (and as the demo's primary web -// login), needs a server-minted session. We store only the sha-256 of the opaque -// cookie value, mirroring how service tokens are stored, so a database read never -// yields a usable cookie. +// Local sessions (spec §B, passwordless). The remote face authenticates statelessly +// with a Cloudflare-Access JWT and sets no cookie; the passwordless console login +// (email-OTP / passkey / setup redeem), used on op.console when Zero Trust is not +// configured (and as the demo's primary web login), needs a server-minted session. +// We store only the sha-256 of the opaque cookie value, mirroring how service tokens +// are stored, so a database read never yields a usable cookie. const ( // sessionCookieName is the host-only session cookie. It carries no Domain // attribute, so an op.console session is never sent to the player console. sessionCookieName = "felis_session" - // sessionTTL bounds a local-password session. Staff re-authenticate after it. + // sessionTTL bounds a local session. Staff re-authenticate after it. sessionTTL = 12 * time.Hour ) // LocalAuthEnabledKey is the platform_settings key that gates whether -// local-password sessions are honored. It is flipped on by `felis breakGlass` +// local sessions are honored. It is flipped on by `felis breakGlass` // direct-to-Postgres at first-run and read live per-request, so enabling local // auth needs no pod roll. Exported so the break-glass writer and this // per-request reader share one source of truth instead of drifting copies. @@ -109,7 +109,7 @@ func hostIsAdminConsole(r *http.Request, rootDomain, adminHostname string) bool } // SessionAuth is the composite ExternalAuth for the web face. It prefers a -// local-password session cookie and otherwise delegates to the remote JWT +// local session cookie and otherwise delegates to the remote JWT // verifier, so both auth models coexist on one face: // // - No cookie → delegate to Delegate (the Cloudflare-Access JWT path). diff --git a/internal/api/util.go b/internal/api/util.go index 9a61de5..dbf4337 100644 --- a/internal/api/util.go +++ b/internal/api/util.go @@ -12,7 +12,8 @@ const maxBodyBytes = 1 << 20 // 1 MiB // requireJSONContentType rejects a request whose body is not declared // application/json, returning 415 before any decode. It guards the credential-bearing -// auth writes (login, change-password) against a cross-site forgery: an HTML form can +// auth writes (email-OTP, passkey, op-login, setup redeem) against a cross-site +// forgery: an HTML form can // only POST as application/x-www-form-urlencoded, multipart/form-data, or text/plain // — never JSON — and a cross-site fetch that forces application/json triggers a CORS // preflight this API never answers, so neither form can be forged off-origin. The