From fc9f47cc8d84adc8da83950cbfa68c87e77e77e4 Mon Sep 17 00:00:00 2001 From: Niklas Ye Date: Fri, 2 Oct 2026 09:12:32 +0200 Subject: [PATCH] Refuse an OIDC sign-in from creating the very first user MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the race terdut-operator#1 found: /api/bootstrap and OIDC auto-provisioning both key off the same signal (SELECT COUNT(*) FROM users) with no coordination between them, so an otherwise-ordinary OIDC sign-in against a freshly-created, not-yet-bootstrapped install could create user #1 itself and take the one slot /api/bootstrap expects to win uncontested (terdut-operator's own design, DESIGN.md §1/§6, assumes it is the only caller). The operator has no way to recover from losing that race -- it never gets a credential, and nothing it owns can clear the occupying user row. resolveSSOUser now checks the same gate handleBootstrap already does, right where it's about to create a brand-new user (an identity nobody has linked yet, that also matches no existing local account by email) -- not anywhere else, since every other sign-in on an already-bootstrapped install is unaffected. New sso_error code `not_bootstrapped`: the person sees "this install is still setting up, try again in a moment" and a second attempt once something has actually bootstrapped succeeds normally, same as any other first sign-in. Does not fix the other half of that issue (BootstrapStateLost's own "delete and recreate" instructions still don't work once something has occupied the slot some other way) -- this closes the specific race, not every path to that state. --- README.md | 2 +- internal/api/oidc.go | 25 ++++++++++++++++++++++ internal/api/oidc_test.go | 39 +++++++++++++++++++++++++++++++++++ internal/web/static/js/app.js | 1 + 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 5e17b6c..dd6b7b7 100644 --- a/README.md +++ b/README.md @@ -793,7 +793,7 @@ attempted. | `POST` | `/api/oidc/device/token` | `{"device_code"}` → `202 {"status":"pending"}`, then `200` with the session cookie once approved (once only). `410` with `{"error":"expired"}` or `{"error":"denied"}`; `429 {"error":"slow_down"}` if polled faster than `interval` | | `POST` | `/api/oidc/device/approve` | **session** — `{"user_code"}`. Approves a pending device login as the caller. `403` for an API key; `404` for an unknown, expired or already decided code | | `POST` | `/api/oidc/device/deny` | **session** — `{"user_code"}`. Refuses it | -| `GET` | `/api/oidc/callback` | Where the provider sends the browser back. Sets the session cookie and redirects to `/`, or to `/?sso_error=` — one of `denied`, `expired`, `failed`, `unavailable`, `not_allowed`, `no_email`, `email_conflict`, `disabled` | +| `GET` | `/api/oidc/callback` | Where the provider sends the browser back. Sets the session cookie and redirects to `/`, or to `/?sso_error=` — one of `denied`, `expired`, `failed`, `unavailable`, `not_allowed`, `no_email`, `email_conflict`, `disabled`, `not_bootstrapped` (no user exists on this install yet — sign in again once something has called `/api/bootstrap`) | | `POST` | `/api/logout` | Ends the session and clears the cookie | | `GET` | `/api/me` | The caller: `{user, has_password}` | diff --git a/internal/api/oidc.go b/internal/api/oidc.go index cc15bf0..c38e317 100644 --- a/internal/api/oidc.go +++ b/internal/api/oidc.go @@ -48,6 +48,17 @@ const ( ssoNoEmail ssoError = "no_email" // the provider sent no email address ssoEmailConflict ssoError = "email_conflict" // a local account has this email and cannot be linked ssoDisabled ssoError = "disabled" // the linked account is disabled + // ssoNotBootstrapped: this identity has no existing account, and no user + // exists on this install yet either -- creating one here would race + // POST /api/bootstrap for the one gitops-managed installs expect to win + // it (terdut-operator's own DESIGN.md §1, §6), which has no way to + // recover if it loses. The person sees this for at most as long as it + // takes whatever is bootstrapping this install to finish; signing in + // again afterward hits the ordinary first-sign-in path. Found by + // terdut-operator#1: nothing stopped an otherwise-ordinary OIDC sign-in + // from quietly winning this race against an operator that assumed it + // was the only caller. + ssoNotBootstrapped ssoError = "not_bootstrapped" ) // handleAuthConfig says how this server can be signed in to, so the login form @@ -366,6 +377,20 @@ func resolveSSOUser(ctx context.Context, tx *sql.Tx, cfg config.OIDC, id *oidc.I return 0, ssoEmailConflict } case errors.Is(err, sql.ErrNoRows): + // Creating the very first user is /api/bootstrap's own job (same + // gate, same table: SELECT COUNT(*) FROM users in handleBootstrap). + // An identity nobody has linked yet, on an install with no users at + // all, is exactly the race terdut-operator#1 found: whoever gets + // here first wins a slot the other side has no way to recover from + // losing. Refusing it here costs an otherwise-ordinary sign-in + // nothing but a retry once bootstrap has actually run. + var userCount int + if err := tx.QueryRowContext(ctx, "SELECT COUNT(*) FROM users").Scan(&userCount); err != nil { + return 0, err + } + if userCount == 0 { + return 0, ssoNotBootstrapped + } userID, err = createSSOUser(ctx, tx, id) if err != nil { return 0, err diff --git a/internal/api/oidc_test.go b/internal/api/oidc_test.go index ef8343a..983dc9c 100644 --- a/internal/api/oidc_test.go +++ b/internal/api/oidc_test.go @@ -543,6 +543,45 @@ func TestSSO_DisabledUserIsRefused(t *testing.T) { } } +// TestSSO_FirstUserIsRefusedUntilBootstrap is terdut-operator#1: an +// otherwise-ordinary OIDC sign-in against a brand-new, not-yet-bootstrapped +// install must not be allowed to create the first user and win the race +// POST /api/bootstrap expects to win uncontested. Built directly over +// api.NewRouter rather than newSSOTS/newTS, both of which bootstrap before +// a test body ever runs -- exactly the state this test needs to not have yet. +func TestSSO_FirstUserIsRefusedUntilBootstrap(t *testing.T) { + idp := newFakeIdP(t) + database := newTestDB(t) + srv := httptest.NewServer(api.NewRouter(database, api.NotifyConfig{PublicURL: "http://terdut.test"}, ssoConfig(idp), "test")) + t.Cleanup(srv.Close) + + first := newBrowser(t, srv.URL) + first.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } + if loc := signInSSO(t, idp, first, alice); loc != "/?sso_error=not_bootstrapped" { + t.Fatalf("sent to %q, want not_bootstrapped", loc) + } + + // Bootstrap the install for real, the way terdut-operator's own + // reconcileBootstrap does. + resp, err := http.Post(srv.URL+"/api/bootstrap", "application/json", + strings.NewReader(`{"username":"admin","email":"admin@test.com"}`)) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + if resp.StatusCode != http.StatusCreated { + t.Fatalf("bootstrap: %d", resp.StatusCode) + } + + // The same identity, signing in again, is this install's ordinary first + // SSO user now -- no longer refused. + second := newBrowser(t, srv.URL) + second.CheckRedirect = func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse } + if loc := signInSSO(t, idp, second, alice); loc != "/" { + t.Errorf("sent to %q after bootstrap, want success", loc) + } +} + func TestSSO_NoEmailIsRefused(t *testing.T) { idp := newFakeIdP(t) s := newSSOTS(t, idp) diff --git a/internal/web/static/js/app.js b/internal/web/static/js/app.js index 17ff5c3..ace9f8c 100644 --- a/internal/web/static/js/app.js +++ b/internal/web/static/js/app.js @@ -355,6 +355,7 @@ function ssoErrorText(code, name) { no_email: `${sso} did not send an email address for you, which terdut needs.`, email_conflict: 'An account with your email address already exists and could not be linked to this sign-in. Ask an administrator.', disabled: 'Your account is disabled. Ask an administrator.', + not_bootstrapped: 'This install is still setting up. Try again in a moment.', }[code] || `Signing in with ${sso} failed.`; }