diff --git a/README.md b/README.md index 3558733..902fd34 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,4 @@ internal/web routing, templates, static assets, confirm orchestration ## Known follow-ups -- An abandoned passkey signup leaves an orphan account holding that username (can't log in, blocks - re-signup). Add cleanup or upsert-on-begin. -- Passkey `navigator.credentials` create/get is only verifiable in a real browser / virtual - authenticator — not covered by automated tests. +- (none currently) diff --git a/internal/auth/handlers.go b/internal/auth/handlers.go index 5a4bb89..62c810e 100644 --- a/internal/auth/handlers.go +++ b/internal/auth/handlers.go @@ -45,17 +45,20 @@ func (s *Service) RequireSSO(next http.Handler) http.Handler { // --- Passkey registration --- -// RegisterBegin starts a passkey registration ceremony. If a user is logged -// in, the passkey is added to that account; otherwise a new account is created -// from the posted email. +// RegisterBegin starts a passkey registration ceremony. If a user is logged in, +// the passkey is added to that account. Otherwise this begins a NEW account +// signup: the account is NOT persisted yet — we reserve its id (which becomes +// the stable WebAuthn user handle) and stash the pending username, and only +// write the row once a credential is verified at RegisterFinish. That keeps an +// abandoned ceremony from leaving an orphan account that squats the username. func (s *Service) RegisterBegin(w http.ResponseWriter, r *http.Request) { ctx := r.Context() - var u *store.User - var name string // optional label, only for adding a passkey to an existing account + var waUser *webauthnUser + var name string // passkey label — only when adding to an existing account if uid := s.CurrentUserID(ctx); uid != 0 { - var err error - if u, err = s.store.GetUserByID(ctx, uid); err != nil { + u, err := s.store.GetUserByID(ctx, uid) + if err != nil { httpError(w, http.StatusInternalServerError, "load user") return } @@ -64,28 +67,38 @@ func (s *Service) RegisterBegin(w http.ResponseWriter, r *http.Request) { } _ = json.NewDecoder(r.Body).Decode(&body) name = NormalizePasskeyName(body.Name) + // Existing credentials are loaded so the authenticator excludes them. + if waUser, err = s.loadWebAuthnUser(ctx, u); err != nil { + httpError(w, http.StatusInternalServerError, "load credentials") + return + } } else { username, displayName, ok := readCreds(r) if !ok { httpError(w, http.StatusBadRequest, "username must be 3–32 chars (letters, digits, _ . -)") return } - created, err := s.store.CreateUser(ctx, username, displayName) - if errors.Is(err, store.ErrDuplicate) { + // Fail fast on a taken name (authoritatively re-checked at finish's insert). + if _, err := s.store.GetUserByUsername(ctx, username); err == nil { httpError(w, http.StatusConflict, "username already taken — choose another or sign in") return - } - if err != nil { - httpError(w, http.StatusInternalServerError, "create user") + } else if !errors.Is(err, store.ErrNotFound) { + httpError(w, http.StatusInternalServerError, "check username") return } - u = created - } - - waUser, err := s.loadWebAuthnUser(ctx, u) - if err != nil { - httpError(w, http.StatusInternalServerError, "load credentials") - return + reservedID, err := s.store.ReserveUserID(ctx) + if err != nil { + httpError(w, http.StatusInternalServerError, "reserve account") + return + } + // In-memory only — no row is written until finish verifies a credential. + u := &store.User{ID: reservedID, Username: username} + if displayName != "" { + u.DisplayName = &displayName + } + waUser = &webauthnUser{user: u} + s.Sessions.Put(ctx, sessKeyWANewName, username) + s.Sessions.Put(ctx, sessKeyWANewDN, displayName) } // Prefer discoverable (resident-key) credentials so the holder can sign in @@ -98,7 +111,7 @@ func (s *Service) RegisterBegin(w http.ResponseWriter, r *http.Request) { httpError(w, http.StatusInternalServerError, "begin registration") return } - if err := s.stashCeremony(ctx, u.ID, sessionData); err != nil { + if err := s.stashCeremony(ctx, waUser.user.ID, sessionData); err != nil { httpError(w, http.StatusInternalServerError, "save ceremony") return } @@ -106,7 +119,10 @@ func (s *Service) RegisterBegin(w http.ResponseWriter, r *http.Request) { writeJSON(w, options) } -// RegisterFinish completes a passkey registration and logs the user in. +// RegisterFinish completes a passkey registration and logs the user in. For a +// deferred new-account ceremony it verifies the credential FIRST, then writes +// the account row (with the reserved id) — so a failed or abandoned ceremony +// never persists an account. func (s *Service) RegisterFinish(w http.ResponseWriter, r *http.Request) { ctx := r.Context() uid, sessionData, ok := s.popCeremony(ctx) @@ -114,17 +130,32 @@ func (s *Service) RegisterFinish(w http.ResponseWriter, r *http.Request) { httpError(w, http.StatusBadRequest, "no registration in progress") return } - u, err := s.store.GetUserByID(ctx, uid) - if err != nil { - httpError(w, http.StatusInternalServerError, "load user") - return - } - waUser, err := s.loadWebAuthnUser(ctx, u) - if err != nil { - httpError(w, http.StatusInternalServerError, "load credentials") - return + // A pending username marks a new-account ceremony (id reserved, row not yet + // written); its absence means we're adding a passkey to an existing account. + newName := s.Sessions.PopString(ctx, sessKeyWANewName) + newDisplay := s.Sessions.PopString(ctx, sessKeyWANewDN) + + var u *store.User + var waUser *webauthnUser + if newName != "" { + u = &store.User{ID: uid, Username: newName} + if newDisplay != "" { + u.DisplayName = &newDisplay + } + waUser = &webauthnUser{user: u} // in-memory; no row yet + } else { + var err error + if u, err = s.store.GetUserByID(ctx, uid); err != nil { + httpError(w, http.StatusInternalServerError, "load user") + return + } + if waUser, err = s.loadWebAuthnUser(ctx, u); err != nil { + httpError(w, http.StatusInternalServerError, "load credentials") + return + } } + // Verify the credential before writing anything durable. cred, err := s.wa.FinishRegistration(waUser, *sessionData, r) if err != nil { httpError(w, http.StatusBadRequest, "registration failed: "+err.Error()) @@ -135,8 +166,22 @@ func (s *Service) RegisterFinish(w http.ResponseWriter, r *http.Request) { httpError(w, http.StatusInternalServerError, "marshal credential") return } - name := s.Sessions.GetString(ctx, sessKeyWAName) - s.Sessions.Remove(ctx, sessKeyWAName) + + if newName != "" { + // The credential is proven — now persist the account with its reserved id. + created, err := s.store.CreateUserWithID(ctx, uid, newName, newDisplay) + if errors.Is(err, store.ErrDuplicate) || errors.Is(err, store.ErrUsernameReserved) { + httpError(w, http.StatusConflict, "username already taken — choose another or sign in") + return + } + if err != nil { + httpError(w, http.StatusInternalServerError, "create account") + return + } + u = created + } + + name := s.Sessions.PopString(ctx, sessKeyWAName) if err := s.store.AddCredential(ctx, u.ID, cred.ID, blob, name); err != nil { httpError(w, http.StatusInternalServerError, "store credential") return diff --git a/internal/auth/service.go b/internal/auth/service.go index 18b0fd6..86c8b75 100644 --- a/internal/auth/service.go +++ b/internal/auth/service.go @@ -18,10 +18,15 @@ import ( const ( sessKeyUserID = "user_id" // int64: the authenticated user sessKeyLoginID = "login_id" // string: the logins-row id backing this session - sessKeyWAUID = "wa_uid" // int64: user mid-ceremony + sessKeyWAUID = "wa_uid" // int64: user (or reserved id) mid-ceremony sessKeyWAData = "wa_data" // []byte: marshaled webauthn.SessionData sessKeyWAName = "wa_name" // string: pending passkey name for the in-flight registration - sessKeyNext = "next" // string: post-auth redirect target + // Deferred passkey signup: a logged-out register ceremony carries the pending + // account's username/display name (present only for new-account ceremonies). + // The account row is written at finish, once a credential is verified. + sessKeyWANewName = "wa_new_username" // string: pending new-account username + sessKeyWANewDN = "wa_new_display" // string: pending new-account display name + sessKeyNext = "next" // string: post-auth redirect target ) // Service wires WebAuthn, the data store, and session management. diff --git a/internal/core/endpoints_auth_test.go b/internal/core/endpoints_auth_test.go index ba3208c..3209b83 100644 --- a/internal/core/endpoints_auth_test.go +++ b/internal/core/endpoints_auth_test.go @@ -1,10 +1,13 @@ package core import ( + "errors" "net/http" "net/http/httptest" "net/url" "testing" + + "github.com/jleight/meshtender/internal/store" ) // Black-box coverage for the auth host's non-visual POST endpoints: password @@ -94,6 +97,33 @@ func TestWebAuthnBeginRateLimited(t *testing.T) { } } +// TestPasskeyBeginDefersAccount: a logged-out register/begin issues a challenge +// but does NOT persist an account — the row is only written once a credential is +// verified at finish (which needs a real authenticator, covered by the e2e +// suite). Regression for the pre-release audit finding + README follow-up that an +// abandoned passkey signup left an orphan account squatting the username. +func TestPasskeyBeginDefersAccount(t *testing.T) { + t.Parallel() + st, ctx, ts, h := splitServer(t) + + resp := jsonPost(t, ts.URL+"/api/register/begin", h.auth, `{"username":"ghost","displayName":"Ghost"}`) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("register/begin = %d, want 200 (challenge issued)", resp.StatusCode) + } + if _, err := st.GetUserByUsername(ctx, "ghost"); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("begin persisted an account for an unfinished ceremony (err=%v)", err) + } + + // A name already taken is still rejected up front. + authSSO(t, ts, h, "taken") + dup := jsonPost(t, ts.URL+"/api/register/begin", h.auth, `{"username":"taken"}`) + dup.Body.Close() + if dup.StatusCode != http.StatusConflict { + t.Fatalf("register/begin for taken username = %d, want 409", dup.StatusCode) + } +} + // assertAccountOK asserts a 303 back to /account carrying a success ("ok") flash. func assertAccountOK(t *testing.T, resp *http.Response, label string) { t.Helper() diff --git a/internal/e2e/harness_test.go b/internal/e2e/harness_test.go index 4e635df..116af5a 100644 --- a/internal/e2e/harness_test.go +++ b/internal/e2e/harness_test.go @@ -125,10 +125,26 @@ func rootReachableHosts() hostLayout { return hostLayout{app: "app." + browserHost(), auth: "auth." + browserHost(), root: browserHost()} } -// newE2EServer stands up the app on a 0.0.0.0 listener so the browser container -// can connect back to it, and returns both address forms. An optional hostLayout -// selects which surface the browser can reach (defaults to the app surface). +// newE2EServer stands up the app (plain HTTP) on a 0.0.0.0 listener so the +// browser container can connect back to it, and returns both address forms. An +// optional hostLayout selects which surface the browser can reach (defaults to +// the app surface). func newE2EServer(t *testing.T, layout ...hostLayout) *e2eServer { + return buildE2EServer(t, false, layout...) +} + +// newE2ETLSServer is like newE2EServer but serves HTTPS (a self-signed cert) and +// marks cookies Secure. WebAuthn ceremonies require a secure context, which a +// plain-HTTP non-localhost origin is not; the browser accepts the self-signed +// cert via Security.setIgnoreCertificateErrors (see the passkey test). +func newE2ETLSServer(t *testing.T, layout ...hostLayout) *e2eServer { + return buildE2EServer(t, true, layout...) +} + +// buildE2EServer is the shared constructor. It listens first so the WebAuthn RP +// origins can carry the actual (dynamic) port, then builds the surfaces. secure +// selects HTTPS + Secure cookies. +func buildE2EServer(t *testing.T, secure bool, layout ...hostLayout) *e2eServer { t.Helper() hosts := defaultHosts() if len(layout) > 0 { @@ -145,45 +161,58 @@ func newE2EServer(t *testing.T, layout ...hostLayout) *e2eServer { _, _ = rand.Read(masterKey[:]) idSvc, _ := identity.LoadOrCreate(ctx, st, masterKey) + // Listen up front so the RP origins can include the concrete port. + ln, err := net.Listen("tcp", "0.0.0.0:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + port := ln.Addr().(*net.TCPAddr).Port + + scheme := "http" + if secure { + scheme = "https" + } + origin := func(h string) string { return fmt.Sprintf("%s://%s:%d", scheme, h, port) } + // The server runs across three distinct hosts so the Dispatcher can tell the // surfaces apart. The browser can navigate only the one named browserHost() // (host.docker.internal) — the sole alias the container resolves back here; the - // layout decides which surface that is (app by default, auth for account tests). - appHost, authHost, rootHost := hosts.app, hosts.auth, hosts.root + // layout decides which surface that is. The RP ID is browserHost(): a WebAuthn + // ceremony's origin host is that name (or a subdomain of it), so it's a valid + // RP ID, and every surface origin is allowed. authSvc, err := auth.New(st, st.Pool(), auth.Config{ - RPID: "localhost", RPDisplayName: "test", RPOrigins: []string{"http://localhost"}, - AppHost: appHost, AuthHost: authHost, RootHost: rootHost, + RPID: browserHost(), RPDisplayName: "test", + RPOrigins: []string{origin(hosts.app), origin(hosts.auth), origin(hosts.root)}, + AppHost: hosts.app, AuthHost: hosts.auth, RootHost: hosts.root, + Secure: secure, }) if err != nil { t.Fatalf("auth: %v", err) } srv, err := core.NewServer(st, authSvc, idSvc, &config.Config{ - PrimaryHost: appHost, AuthHost: authHost, RootHost: rootHost, + PrimaryHost: hosts.app, AuthHost: hosts.auth, RootHost: hosts.root, Secure: secure, }) if err != nil { t.Fatalf("server: %v", err) } - ln, err := net.Listen("tcp", "0.0.0.0:0") - if err != nil { - t.Fatalf("listen: %v", err) - } ts := httptest.NewUnstartedServer(srv.Handler()) ts.Listener.Close() // drop the default 127.0.0.1 listener ts.Listener = ln - ts.Start() + if secure { + ts.StartTLS() + } else { + ts.Start() + } t.Cleanup(ts.Close) - port := ln.Addr().(*net.TCPAddr).Port - // ts.URL reflects the 0.0.0.0 listener; rewrite it to loopback so the - // host-side Go client dials a concrete address. - ts.URL = fmt.Sprintf("http://127.0.0.1:%d", port) + ts.URL = fmt.Sprintf("%s://127.0.0.1:%d", scheme, port) return &e2eServer{ store: st, ctx: ctx, ts: ts, hostURL: ts.URL, - browserURL: fmt.Sprintf("http://%s:%d", browserHost(), port), + browserURL: fmt.Sprintf("%s://%s:%d", scheme, browserHost(), port), } } diff --git a/internal/e2e/webauthn_test.go b/internal/e2e/webauthn_test.go new file mode 100644 index 0000000..b11d95b --- /dev/null +++ b/internal/e2e/webauthn_test.go @@ -0,0 +1,109 @@ +//go:build browser + +package e2e + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/chromedp/cdproto/security" + "github.com/chromedp/cdproto/webauthn" + "github.com/chromedp/chromedp" + + "github.com/jleight/meshtender/internal/store" +) + +// virtualAuthenticator enables the WebAuthn CDP domain and installs a virtual +// authenticator that auto-approves user presence/verification, so +// navigator.credentials.create/get succeed headlessly. It also tells the browser +// to accept the harness's self-signed cert (WebAuthn needs a secure context, and +// an HTTPS origin — even with an untrusted cert — qualifies, whereas plain HTTP +// on a non-localhost host does not). +func virtualAuthenticator(ctx context.Context) error { + return chromedp.Run(ctx, + security.SetIgnoreCertificateErrors(true), + webauthn.Enable(), + chromedp.ActionFunc(func(ctx context.Context) error { + _, err := webauthn.AddVirtualAuthenticator(&webauthn.VirtualAuthenticatorOptions{ + Protocol: webauthn.AuthenticatorProtocolCtap2, + Transport: webauthn.AuthenticatorTransportInternal, + HasResidentKey: true, + HasUserVerification: true, + IsUserVerified: true, + AutomaticPresenceSimulation: true, + }).Do(ctx) + return err + }), + ) +} + +// waitForUser polls until the account exists (the ceremony's server side runs +// asynchronously after the click) or the deadline passes. +func waitForUser(t *testing.T, e *e2eServer, username string) *store.User { + t.Helper() + deadline := time.Now().Add(20 * time.Second) + for { + u, err := e.store.GetUserByUsername(e.ctx, username) + if err == nil { + return u + } + if !errors.Is(err, store.ErrNotFound) { + t.Fatalf("GetUserByUsername: %v", err) + } + if time.Now().After(deadline) { + return nil + } + time.Sleep(200 * time.Millisecond) + } +} + +// TestPasskeySignupCeremony drives the real passkey signup end to end through a +// virtual authenticator: the browser runs navigator.credentials.create on the +// live signup page, and only when the credential is verified at finish does the +// account get written (the deferred-creation flow). This is the browser-level +// proof of item 3 that the Go tests can't give (they can't complete a ceremony). +func TestPasskeySignupCeremony(t *testing.T) { + e := newE2ETLSServer(t, authReachableHosts()) + ctx, cancel, watch := startBrowser(t) + defer cancel() + + if err := virtualAuthenticator(ctx); err != nil { + t.Fatalf("set up virtual authenticator: %v", err) + } + + const username = "passkeysignup" + // Pre-condition: no such account yet. + if _, err := e.store.GetUserByUsername(e.ctx, username); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("account should not exist before the ceremony (err=%v)", err) + } + + if err := chromedp.Run(ctx, + chromedp.Navigate(e.browserURL+"/signup"), + chromedp.WaitVisible("#signup-passkey-btn", chromedp.ByID), + chromedp.SendKeys("#username", username, chromedp.ByID), + chromedp.Click("#signup-passkey-btn", chromedp.ByID), + ); err != nil { + t.Fatalf("drive signup page: %v", err) + } + + u := waitForUser(t, e, username) + if u == nil { + // Surface the on-page status for diagnostics. + var status string + _ = chromedp.Run(ctx, chromedp.Text("#passkey-status", &status, chromedp.ByID, chromedp.AtLeast(0))) + t.Fatalf("account was not created by the passkey ceremony; page status = %q", status) + } + + // A verified credential must have been persisted for the new account. + creds, err := e.store.GetCredentials(e.ctx, u.ID) + if err != nil { + t.Fatalf("GetCredentials: %v", err) + } + if len(creds) != 1 { + t.Fatalf("new account has %d credentials, want exactly 1", len(creds)) + } + + watch.assertClean(t) // the signup page + ceremony must not trip the CSP +} diff --git a/internal/store/users.go b/internal/store/users.go index 2f5f27f..87e49dc 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -53,11 +53,39 @@ func scanUser(row pgx.Row) (*User, error) { return &u, nil } -// CreateUser inserts a new user and returns it. displayName may be empty (then -// stored as NULL). Returns ErrDuplicate if the username is already taken, or -// ErrUsernameReserved if it was recently released by someone else and is still -// within its cooldown. +// CreateUser inserts a new user with a database-assigned id and returns it. +// displayName may be empty (then stored as NULL). Returns ErrDuplicate if the +// username is already taken, or ErrUsernameReserved if it was recently released +// by someone else and is still within its cooldown. func (s *Store) CreateUser(ctx context.Context, username, displayName string) (*User, error) { + return s.createUser(ctx, 0, username, displayName) +} + +// ReserveUserID consumes and returns the next users.id from the identity +// sequence without inserting a row. The deferred passkey signup uses it to fix +// the account's id — and therefore its WebAuthn user handle, which is the id +// encoded as 8 bytes — at BeginRegistration, then writes the row with +// CreateUserWithID only once a credential is verified at finish. An abandoned +// ceremony merely leaves a gap in the sequence; no orphan account is created. +func (s *Store) ReserveUserID(ctx context.Context) (int64, error) { + var id int64 + if err := s.pool.QueryRow(ctx, + `SELECT nextval(pg_get_serial_sequence('users', 'id'))`).Scan(&id); err != nil { + return 0, fmt.Errorf("reserve user id: %w", err) + } + return id, nil +} + +// CreateUserWithID inserts a new user with a caller-reserved id (from +// ReserveUserID). See CreateUser for the error contract. +func (s *Store) CreateUserWithID(ctx context.Context, id int64, username, displayName string) (*User, error) { + return s.createUser(ctx, id, username, displayName) +} + +// createUser is the shared insert. id == 0 lets the identity column assign one; +// a nonzero id is forced via OVERRIDING SYSTEM VALUE (the column is GENERATED +// ALWAYS). +func (s *Store) createUser(ctx context.Context, id int64, username, displayName string) (*User, error) { var dn *string if displayName != "" { dn = &displayName @@ -71,9 +99,13 @@ func (s *Store) CreateUser(ctx context.Context, username, displayName string) (* if reserved { return nil, ErrUsernameReserved } - u, err := scanUser(s.pool.QueryRow(ctx, - `INSERT INTO users (username, display_name) VALUES ($1, $2) RETURNING `+userCols, - username, dn)) + query := `INSERT INTO users (username, display_name) VALUES ($1, $2) RETURNING ` + userCols + args := []any{username, dn} + if id != 0 { + query = `INSERT INTO users (id, username, display_name) OVERRIDING SYSTEM VALUE VALUES ($1, $2, $3) RETURNING ` + userCols + args = []any{id, username, dn} + } + u, err := scanUser(s.pool.QueryRow(ctx, query, args...)) if isUniqueViolation(err) { return nil, ErrDuplicate } diff --git a/internal/store/users_reserve_test.go b/internal/store/users_reserve_test.go new file mode 100644 index 0000000..f33e552 --- /dev/null +++ b/internal/store/users_reserve_test.go @@ -0,0 +1,60 @@ +package store + +import ( + "errors" + "testing" +) + +// TestReserveUserIDAndCreateWithID covers the deferred passkey-signup primitives: +// ReserveUserID hands out increasing ids without writing rows, and +// CreateUserWithID later persists the row at exactly that id (so the WebAuthn +// user handle, which is the id, still resolves). +func TestReserveUserIDAndCreateWithID(t *testing.T) { + t.Parallel() + st, ctx := orgTestStore(t) + + id1, err := st.ReserveUserID(ctx) + if err != nil { + t.Fatalf("reserve 1: %v", err) + } + id2, err := st.ReserveUserID(ctx) + if err != nil { + t.Fatalf("reserve 2: %v", err) + } + if id2 <= id1 { + t.Fatalf("reserved ids not increasing: %d then %d", id1, id2) + } + + // Reserving must not create any rows (that's the whole point — no orphans). + var n int + if err := st.pool.QueryRow(ctx, `SELECT count(*) FROM users`).Scan(&n); err != nil { + t.Fatalf("count users: %v", err) + } + if n != 0 { + t.Fatalf("ReserveUserID created %d user rows, want 0", n) + } + + // Create at the reserved id; it must round-trip through GetUserByID — the path + // discoverable login uses to resolve a credential's handle back to an account. + u, err := st.CreateUserWithID(ctx, id2, "deferred", "Deferred User") + if err != nil { + t.Fatalf("CreateUserWithID: %v", err) + } + if u.ID != id2 { + t.Fatalf("created id = %d, want reserved %d", u.ID, id2) + } + got, err := st.GetUserByID(ctx, id2) + if err != nil || got.Username != "deferred" { + t.Fatalf("GetUserByID(%d) = %+v, %v", id2, got, err) + } + // The first real account still bootstraps instance caps. + if !u.CapManageUsers || !u.CapManageCatalog { + t.Fatalf("first account should bootstrap caps, got %+v", u) + } + + // A duplicate username is rejected even via the explicit-id path. + id3, _ := st.ReserveUserID(ctx) + if _, err := st.CreateUserWithID(ctx, id3, "deferred", ""); !errors.Is(err, ErrDuplicate) { + t.Fatalf("duplicate username = %v, want ErrDuplicate", err) + } +}