diff --git a/docs/onboarding.md b/docs/onboarding.md index d162ead1..dc73a43d 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -218,11 +218,29 @@ its cache expires — `auth.cache_ttl`, 30 seconds by default. That is the price keeping the database off the request path, and it is the number to lower if you need revocation to bite faster. +## Passwords + +People change their own from `/profile`. It asks for the current one — a session +is what someone gets from a borrowed laptop, and the password is what they do not +have — and it signs every other browser out, which is the point if they are +changing it because something went wrong. + +There is no self-service reset, because this gateway sends no email and a reset +link with nowhere to send it is not a reset. When someone loses theirs: + +``` +llmgw users reset-password usr-k4mq… --password "a temporary passphrase" --store … +``` + +Tell them out of band and have them change it from their profile. Their sessions +are closed — if they lost the password, the reason might be that someone else has +it. Their API keys keep working; those are separate credentials. + ## What this does not do -- **No email.** No approval notice, no password reset, no address verification. - Someone who forgets their password needs you, and there is no command for that - yet — `llmgw users create` is the workaround, which changes the address. +- **No email.** No approval notice, no self-service password reset, no address + verification. The loop is manual on purpose at this size; if it stops being + the right size, that is the thing to automate. - **No SSO.** Accounts are local, with local passwords. - **No roles.** Every approved account can do the same things: make its own keys and nothing else. Administration is the CLI, and having a shell here is what diff --git a/internal/account/account_test.go b/internal/account/account_test.go index 8f52c754..eea84b61 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -310,3 +310,87 @@ func must(t *testing.T, err error) { t.Fatal(err) } } + +func TestChangingAPasswordNeedsTheOldOne(t *testing.T) { + // A session is what an attacker gets from a borrowed laptop or a stolen + // cookie. The password is what they do not have, and without this check the + // first thing they would do is take the account. + svc, st := newStoreAndUser(t) + ctx := context.Background() + must(t, st.ApproveUser(ctx, "usr-1", "proj", "ilker", time.Now())) + u, _ := st.UserByID(ctx, "usr-1") + + if err := svc.ChangePassword(ctx, st, u, "not the password", "a brand new passphrase"); !errors.Is(err, ErrBadCredentials) { + t.Errorf("a wrong current password was accepted: %v", err) + } + // And the old one still works. + if _, _, err := svc.SignIn(ctx, "ayse@example.com", goodPassword); err != nil { + t.Errorf("the password changed anyway: %v", err) + } +} + +func TestChangingAPasswordSignsOutEverywhere(t *testing.T) { + // Leaving other sessions open would mean a password change does nothing + // about the reason someone is changing it. + svc, st := newStoreAndUser(t) + ctx := context.Background() + must(t, st.ApproveUser(ctx, "usr-1", "proj", "ilker", time.Now())) + + laptop, _, _ := svc.SignIn(ctx, "ayse@example.com", goodPassword) + phone, _, _ := svc.SignIn(ctx, "ayse@example.com", goodPassword) + + u, _ := st.UserByID(ctx, "usr-1") + must(t, svc.ChangePassword(ctx, st, u, goodPassword, "a brand new passphrase")) + + for name, token := range map[string]string{"laptop": laptop, "phone": phone} { + if _, _, err := svc.Resolve(ctx, token); !errors.Is(err, store.ErrNoSession) { + t.Errorf("the %s session survived the change: %v", name, err) + } + } + // The new password works and the old one does not. + if _, _, err := svc.SignIn(ctx, "ayse@example.com", "a brand new passphrase"); err != nil { + t.Errorf("the new password does not work: %v", err) + } + if _, _, err := svc.SignIn(ctx, "ayse@example.com", goodPassword); !errors.Is(err, ErrBadCredentials) { + t.Errorf("the old password still works: %v", err) + } +} + +func TestANewPasswordMustActuallyBeNew(t *testing.T) { + svc, st := newStoreAndUser(t) + ctx := context.Background() + must(t, st.ApproveUser(ctx, "usr-1", "proj", "ilker", time.Now())) + u, _ := st.UserByID(ctx, "usr-1") + + if err := svc.ChangePassword(ctx, st, u, goodPassword, goodPassword); !errors.Is(err, ErrSamePassword) { + t.Errorf("changing a password to itself: %v", err) + } + if err := svc.ChangePassword(ctx, st, u, goodPassword, "short"); !errors.Is(err, ErrWeakPassword) { + t.Errorf("a short new password was accepted: %v", err) + } +} + +func TestGuessingTheCurrentPasswordIsBounded(t *testing.T) { + // Someone with a stolen session should not get unlimited attempts at the + // password that would let them keep it. + st := newStore(t) + ctx := context.Background() + hash, _ := HashPassword(goodPassword) + must(t, st.CreateUser(ctx, store.User{ + ID: "usr-1", Email: "a@example.com", Name: "A", + PasswordHash: hash, State: store.UserPending, CreatedAt: time.Now(), + })) + must(t, st.ApproveUser(ctx, "usr-1", "proj", "ilker", time.Now())) + + cfg := DefaultConfig() + cfg.MaxAttempts = 3 + svc := New(st, cfg, slog.New(slog.DiscardHandler)) + u, _ := st.UserByID(ctx, "usr-1") + + for range 3 { + _ = svc.ChangePassword(ctx, st, u, "wrong", "a brand new passphrase") + } + if err := svc.ChangePassword(ctx, st, u, "wrong", "a brand new passphrase"); !errors.Is(err, ErrTooManyTries) { + t.Errorf("guessing was unbounded: %v", err) + } +} diff --git a/internal/account/service.go b/internal/account/service.go index 0a8da42e..2bfc3d4a 100644 --- a/internal/account/service.go +++ b/internal/account/service.go @@ -264,3 +264,67 @@ func randomID(prefix string) string { return prefix + "-" + strings.ToLower( base32.StdEncoding.WithPadding(base32.NoPadding).EncodeToString(buf[:])) } + +// ErrSamePassword says the new password is the old one. +var ErrSamePassword = errors.New("that is the password you already have") + +// PasswordChanger is the extra storage a password change needs. +type PasswordChanger interface { + SetPassword(ctx context.Context, userID, hash string) error + DeleteUserSessions(ctx context.Context, userID string) error +} + +// ChangePassword replaces a person's password and signs them out everywhere. +// +// The current password is required even though the caller already holds a +// session. A session is what an attacker gets from a borrowed laptop or a stolen +// cookie; the password is what they do not have, and without this check the +// first thing they would do is take the account. +// +// Every session is closed, this browser's included. Leaving the others open +// would mean a password change does nothing about the reason someone is +// changing it. The caller re-issues a session for the browser in front of them. +func (s *Service) ChangePassword(ctx context.Context, pw PasswordChanger, + u store.User, current, next string, +) error { + if !s.allowAttempt("pw:" + u.ID) { + return ErrTooManyTries + } + ok, err := VerifyPassword(current, u.PasswordHash) + if err != nil || !ok { + return ErrBadCredentials + } + s.clearAttempts("pw:" + u.ID) + + if same, _ := VerifyPassword(next, u.PasswordHash); same { + return ErrSamePassword + } + hash, err := HashPassword(next) + if err != nil { + return err + } + if err := pw.SetPassword(ctx, u.ID, hash); err != nil { + return err + } + if err := pw.DeleteUserSessions(ctx, u.ID); err != nil { + return err + } + s.log.Info("a password was changed", "user", u.ID) + return nil +} + +// OpenSession starts a session for a person who has just proved who they are by +// some other means — immediately after changing their password, which closed +// the one they were using. +func (s *Service) OpenSession(ctx context.Context, userID string) (token, csrf string, err error) { + if token, err = store.NewSessionToken(); err != nil { + return "", "", err + } + if csrf, err = store.NewSessionToken(); err != nil { + return "", "", err + } + if err := s.store.CreateSession(ctx, userID, token, csrf, s.cfg.SessionTTL); err != nil { + return "", "", err + } + return token, csrf, nil +} diff --git a/internal/cli/users.go b/internal/cli/users.go index e9583244..e643fe05 100644 --- a/internal/cli/users.go +++ b/internal/cli/users.go @@ -32,7 +32,7 @@ func newUsersCmd() *cobra.Command { "Stop an account working, keeping it and its history"), newUserStateCmd(open, "reinstate", store.UserActive, "Let a suspended account work again"), - newUserCreateCmd(open)) + newUserCreateCmd(open), newUserResetPasswordCmd(open)) return cmd } @@ -142,6 +142,46 @@ func newUserStateCmd(open userOpener, verb string, state store.UserState, short } } +func newUserResetPasswordCmd(open userOpener) *cobra.Command { + var password string + cmd := &cobra.Command{ + Use: "reset-password ", + Short: "Set a new password for someone who lost theirs", + Long: "There is no self-service reset, because this gateway sends no email and\n" + + "a reset link with nowhere to send it is not a reset. Set one here, tell\n" + + "them out of band, and have them change it from their profile.\n\n" + + "Every session they have open is closed: if they lost the password, the\n" + + "reason might be that somebody else has it.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + if password == "" { + return fmt.Errorf("--password is required") + } + s, err := open(cmd) + if err != nil { + return err + } + defer s.Close() + + hash, err := account.HashPassword(password) + if err != nil { + return err + } + if err := s.SetPassword(cmd.Context(), args[0], hash); err != nil { + return err + } + if err := s.DeleteUserSessions(cmd.Context(), args[0]); err != nil { + return err + } + fmt.Fprintf(cmd.OutOrStdout(), "password set for %s\n", args[0]) + cmd.PrintErrln("Their sessions were closed. Their API keys still work.") + return nil + }, + } + cmd.Flags().StringVar(&password, "password", "", "the new password") + return cmd +} + func newUserCreateCmd(open userOpener) *cobra.Command { var project, password string cmd := &cobra.Command{ diff --git a/internal/router/handlers.go b/internal/router/handlers.go index e20e1375..ca1f65e1 100644 --- a/internal/router/handlers.go +++ b/internal/router/handlers.go @@ -106,7 +106,7 @@ func (r *Router) Handler() http.Handler { // Signup, sign-in and the profile page. These are browser surfaces // authenticated by a session cookie, not by an API key, which is why // they are exempt below. - web.NewAccountPages(r.accounts, r.store, version.Version, r.log).Register(mux) + web.NewAccountPages(r.accounts, r.store, r.store, version.Version, r.log).Register(mux) } if r.auth == nil { diff --git a/internal/store/users.go b/internal/store/users.go index fc014c6d..6881d7ea 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -197,3 +197,21 @@ func (s *SQLite) scanUser(sc scanner) (User, error) { u.LastLoginAt = fromNullMillis(lastLogin) return u, nil } + +// SetPassword replaces an account's password hash. +// +// It does not close sessions; that is the caller's decision, because the two +// reasons to change a password want different things. Someone rotating theirs +// wants their other devices signed out. An operator resetting a forgotten one +// wants the same, but also has nobody to keep signed in. +func (s *SQLite) SetPassword(ctx context.Context, userID, hash string) error { + res, err := s.db.ExecContext(ctx, `UPDATE users SET password_hash = ? WHERE id = ?`, + hash, userID) + if err != nil { + return err + } + if n, _ := res.RowsAffected(); n == 0 { + return ErrNotFound + } + return nil +} diff --git a/internal/web/account.go b/internal/web/account.go index cdb8ab5b..d9cfff43 100644 --- a/internal/web/account.go +++ b/internal/web/account.go @@ -26,6 +26,9 @@ type Accounts interface { SignOut(ctx context.Context, token string) error CheckCSRF(sess store.Session, submitted string) bool SignupOpen() bool + ChangePassword(ctx context.Context, pw account.PasswordChanger, + u store.User, current, next string) error + OpenSession(ctx context.Context, userID string) (token, csrf string, err error) } // KeyOwner is the store half of the profile page. @@ -39,18 +42,21 @@ type KeyOwner interface { // AccountPages serves signup, sign-in and the profile. type AccountPages struct { - accounts Accounts - keys KeyOwner - version string - log *slog.Logger + accounts Accounts + keys KeyOwner + passwords account.PasswordChanger + version string + log *slog.Logger } // NewAccountPages builds the handlers. -func NewAccountPages(a Accounts, k KeyOwner, version string, log *slog.Logger) *AccountPages { +func NewAccountPages(a Accounts, k KeyOwner, pw account.PasswordChanger, + version string, log *slog.Logger, +) *AccountPages { if log == nil { log = slog.Default() } - return &AccountPages{accounts: a, keys: k, version: version, log: log} + return &AccountPages{accounts: a, keys: k, passwords: pw, version: version, log: log} } // Register mounts the pages. They are all browser surfaces and none of them @@ -67,6 +73,7 @@ func (p *AccountPages) Register(mux *http.ServeMux) { mux.HandleFunc("GET /profile", p.profile) mux.HandleFunc("POST /profile/keys", p.createKey) mux.HandleFunc("POST /profile/keys/{id}/revoke", p.revokeKey) + mux.HandleFunc("POST /profile/password", p.changePassword) } // --- signup --- @@ -170,11 +177,16 @@ func (p *AccountPages) logout(w http.ResponseWriter, r *http.Request) { type limitRow struct{ Name, Value string } type profileData struct { - User store.User - Keys []store.Key - Limits []limitRow - NewKey string - Error string + User store.User + Keys []store.Key + Limits []limitRow + NewKey string + Error string + + PasswordChanged bool + PasswordError string + MinPassword int + CSRF string BaseURL string Version string @@ -251,12 +263,17 @@ func (p *AccountPages) revokeKey(w http.ResponseWriter, r *http.Request) { func (p *AccountPages) renderProfile(w http.ResponseWriter, r *http.Request, sess store.Session, user store.User, newKey, errMsg string, ) { - d := profileData{ - User: user, NewKey: newKey, Error: errMsg, - BaseURL: baseURL(r), - Version: p.version, - Now: time.Now().UTC().Format(time.RFC3339), - } + p.render(w, r, sess, user, profileData{NewKey: newKey, Error: errMsg}) +} + +func (p *AccountPages) render(w http.ResponseWriter, r *http.Request, + sess store.Session, user store.User, d profileData, +) { + d.User = user + d.MinPassword = account.MinPasswordLen + d.BaseURL = baseURL(r) + d.Version = p.version + d.Now = time.Now().UTC().Format(time.RFC3339) // The form token comes from the cookie, never from whatever the request // happened to carry: echoing a submitted value back into the next form // would let an attacker choose both halves of the comparison. @@ -391,3 +408,82 @@ func noStore(w http.ResponseWriter) { w.Header().Set("X-Content-Type-Options", "nosniff") w.Header().Set("X-Frame-Options", "DENY") } + +func (p *AccountPages) changePassword(w http.ResponseWriter, r *http.Request) { + noStore(w) + sess, user, ok := p.session(r) + if !ok { + http.Redirect(w, r, "/login", http.StatusSeeOther) + return + } + if !p.accounts.CheckCSRF(sess, r.FormValue("csrf")) { + http.Error(w, "bad request", http.StatusBadRequest) + return + } + + err := p.accounts.ChangePassword(r.Context(), p.passwords, user, + r.FormValue("current"), r.FormValue("new")) + if err != nil { + d := profileData{} + switch { + case errors.Is(err, account.ErrBadCredentials): + d.PasswordError = "that is not your current password" + case errors.Is(err, account.ErrWeakPassword), + errors.Is(err, account.ErrSamePassword), + errors.Is(err, account.ErrTooManyTries): + d.PasswordError = err.Error() + default: + p.log.Error("could not change a password", "err", err) + d.PasswordError = "something went wrong; try again" + } + p.render(w, r, sess, user, d) + return + } + + // The change closed every session including this one, which is the point. + // Open a new one for the browser in front of us, so the person who just + // proved their password is not thrown back to the sign-in page. + token, csrf, err := p.accounts.OpenSession(r.Context(), user.ID) + if err != nil { + p.log.Error("could not reopen a session", "err", err) + clearSessionCookie(w, r) + http.Redirect(w, r, "/login", http.StatusSeeOther) + return + } + setSessionCookie(w, r, token) + setCSRFCookie(w, r, csrf) + + // The new CSRF cookie is set on this response, so the forms rendered now + // must carry the same value rather than the one the request arrived with. + newSess, newUser, rerr := p.accounts.Resolve(r.Context(), token) + if rerr != nil { + http.Redirect(w, r, "/profile", http.StatusSeeOther) + return + } + d := profileData{PasswordChanged: true} + d.CSRF = csrf + p.renderWithCSRF(w, r, newSess, newUser, d) +} + +// renderWithCSRF is render for the one case where the token in the cookie jar +// is not yet the one the browser has: the response that sets a new one. +func (p *AccountPages) renderWithCSRF(w http.ResponseWriter, r *http.Request, + sess store.Session, user store.User, d profileData, +) { + csrf := d.CSRF + d.User = user + d.MinPassword = account.MinPasswordLen + d.BaseURL = baseURL(r) + d.Version = p.version + d.Now = time.Now().UTC().Format(time.RFC3339) + d.CSRF = csrf + _ = sess + + keys, err := p.keys.KeysForUser(r.Context(), user.ID) + if err != nil { + p.log.Error("could not list keys", "err", err) + } + d.Keys = keys + d.Limits = p.limitsFor(r.Context(), user) + render(w, "profile", d) +} diff --git a/internal/web/templates/account.html b/internal/web/templates/account.html index 2439c863..aebecd3a 100644 --- a/internal/web/templates/account.html +++ b/internal/web/templates/account.html @@ -102,6 +102,21 @@ minute. To rotate: create the new key, switch over, wait for "last used" on the old one to stop moving, then revoke it.

{{else}}

You have no keys yet.

{{end}} +

Password

+{{if .PasswordChanged}}

Your password was changed. Any other browser you were +signed in on has been signed out.

{{end}} +{{if .PasswordError}}

{{.PasswordError}}

{{end}} +
+ + + + +
+

Changing it signs out every other browser, which is the point if +you are changing it because something went wrong.

+

Your limits

{{if .Limits}} diff --git a/llmgw-plan.md b/llmgw-plan.md index eb8bd2e4..4f9e6efb 100644 --- a/llmgw-plan.md +++ b/llmgw-plan.md @@ -1412,8 +1412,8 @@ The launch path is now built too: caller send its whole allowance at the end of one window and again at the start of the next, so the observed rate doubles exactly when the system is least able to take it. -- [x] Accounts: signup, operator approval, sessions and a profile page where a - person issues and revokes their own keys. Approval stays on the CLI — +- [x] Accounts: signup, operator approval, sessions, password changes, and a + profile page where a person issues and revokes their own keys. Approval stays on the CLI — having a shell on the control plane is what makes someone an administrator, and there are no roles to get wrong. - [x] Onboarding: one-time claim links (the key is sealed with the link's own diff --git a/test/e2e/signup_test.go b/test/e2e/signup_test.go index 57668991..3e266816 100644 --- a/test/e2e/signup_test.go +++ b/test/e2e/signup_test.go @@ -317,3 +317,81 @@ func signUpAndApprove(t *testing.T, b *browser, path, email string) string { } return id } + +func TestChangingAPasswordKeepsThisBrowserAndDropsTheOthers(t *testing.T) { + // Both halves matter. Signing every session out is the security property; + // keeping this one is what stops the person who just proved their password + // from being thrown back to the sign-in page. + h, path := signupHarness(t) + + laptop := newBrowser(t, h.router.URL) + signUpAndApprove(t, laptop, path, "ayse@example.com") + + // A second browser, signed in as the same person. + phone := newBrowser(t, h.router.URL) + if code, _, _ := phone.post("/login", url.Values{ + "email": {"ayse@example.com"}, "password": {password}, + }); code != http.StatusSeeOther { + t.Fatalf("second sign-in returned %d", code) + } + if code, _ := phone.get("/profile"); code != http.StatusOK { + t.Fatalf("the second browser was not signed in: %d", code) + } + + _, profile := laptop.get("/profile") + const newPassword = "an entirely different passphrase" + code, body, _ := laptop.post("/profile/password", url.Values{ + "csrf": {csrf(t, profile)}, "current": {password}, "new": {newPassword}, + }) + if code != http.StatusOK { + t.Fatalf("changing the password returned %d", code) + } + if !strings.Contains(body, "password was changed") { + t.Errorf("the page does not confirm the change") + } + + // This browser is still signed in, and its forms still work — which they + // only do if the new CSRF token reached the page that was just rendered. + newToken := csrf(t, body) + if code, _, _ := laptop.post("/profile/keys", url.Values{ + "csrf": {newToken}, "name": {"after-change"}, + }); code != http.StatusOK { + t.Errorf("the form on the page after a password change was refused: %d", code) + } + + // The other browser is not. + if code, _ := phone.get("/profile"); code != http.StatusSeeOther { + t.Errorf("the other browser kept its session: %d", code) + } + + // And the old password is gone. + fresh := newBrowser(t, h.router.URL) + if code, _, _ := fresh.post("/login", url.Values{ + "email": {"ayse@example.com"}, "password": {password}, + }); code != http.StatusUnauthorized { + t.Errorf("the old password still signs in: %d", code) + } + if code, _, _ := fresh.post("/login", url.Values{ + "email": {"ayse@example.com"}, "password": {newPassword}, + }); code != http.StatusSeeOther { + t.Errorf("the new password does not sign in: %d", code) + } +} + +func TestAWrongCurrentPasswordChangesNothing(t *testing.T) { + h, path := signupHarness(t) + b := newBrowser(t, h.router.URL) + signUpAndApprove(t, b, path, "ayse@example.com") + + _, profile := b.get("/profile") + _, body, _ := b.post("/profile/password", url.Values{ + "csrf": {csrf(t, profile)}, "current": {"wrong"}, "new": {"something else entirely"}, + }) + if !strings.Contains(body, "not your current password") { + t.Error("the page does not say why it was refused") + } + // Still signed in, still on the old password. + if code, _ := b.get("/profile"); code != http.StatusOK { + t.Errorf("a failed change signed the person out: %d", code) + } +}
LimitValue