Refuse an OIDC sign-in from creating the very first user
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user