From 44cd58005a641054d237d7a103772a451877c95b Mon Sep 17 00:00:00 2001 From: Ilker Manap Date: Sun, 4 Oct 2026 10:26:29 +0200 Subject: [PATCH] Count sign-in attempts per address and client, guard the sign-in form with a token, ration both forms per address A wrong password from anywhere counted against the address, so ten locked its owner out. Attempts are now counted per address and client, with a looser bound per address alone. The sign-in form carries a token matched by a cookie (login CSRF). POST /login and /signup are rationed per client address, a hash waits at most ten seconds for a turn, and server init writes signup_open: false. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 10 +++ internal/account/account_test.go | 50 +++++++++++ internal/account/password.go | 28 ++++-- internal/account/service.go | 35 ++++++-- internal/account/throttle_bound_test.go | 8 +- internal/cli/serverinit.go | 4 +- internal/web/account.go | 28 ++++-- internal/web/formguard.go | 111 ++++++++++++++++++++++++ internal/web/signup_test.go | 45 ++++++++++ internal/web/templates/account.html | 1 + test/e2e/signup_test.go | 14 +++ 11 files changed, 312 insertions(+), 22 deletions(-) create mode 100644 internal/web/formguard.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 029478e..eedae6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -126,6 +126,16 @@ APIs change between minors without a deprecation period. agents' long-lived streams); the client port holds at most `max_connections` (10,000) at once, and a message from an agent at most 16 MB. +- **The sign-in and signup forms resist scripts.** Ten wrong passwords from + anywhere locked an address's owner out for the window; attempts are now + counted per address and client, so somebody guessing elsewhere cannot lock + the owner out, with a looser count per address alone so guesses spread + over many clients are still bounded. The sign-in form carries a token its + cookie matches, so another site cannot sign a visitor into an account of + its choosing. Posting either form is rationed per client address (sign-in + 30 a minute, signup 5), and a password hash that cannot get a turn within + ten seconds answers "busy" instead of holding the request. `server init` + now writes `signup_open: false`. ### Everything that can be changed on a running cluster, from the console diff --git a/internal/account/account_test.go b/internal/account/account_test.go index 9dfa8d3..36ca51d 100644 --- a/internal/account/account_test.go +++ b/internal/account/account_test.go @@ -398,3 +398,53 @@ func TestGuessingTheCurrentPasswordIsBounded(t *testing.T) { t.Errorf("guessing was unbounded: %v", err) } } + +func TestSomebodyGuessingElsewhereCannotLockTheOwnerOut(t *testing.T) { + svc, st := newStoreAndUser(t) + ctx := context.Background() + must(t, st.ApproveUser(ctx, "usr-1", "proj", "ilker", time.Now())) + cfg := DefaultConfig() + cfg.MaxAttempts = 3 + svc = New(st, cfg, slog.New(slog.DiscardHandler)) + + attacker, owner := WithClient(ctx, "203.0.113.9"), WithClient(ctx, "198.51.100.7") + for range 5 { + _, _, _ = svc.SignIn(attacker, "ayse@example.com", "nope") + } + if _, _, err := svc.SignIn(attacker, "ayse@example.com", "nope"); !errors.Is(err, ErrTooManyTries) { + t.Errorf("the guessing client was not held back: %v", err) + } + if _, _, err := svc.SignIn(owner, "ayse@example.com", goodPassword); err != nil { + t.Errorf("the owner, elsewhere, was locked out: %v", err) + } + // Spread over many clients, guessing is still bounded per address. + held := false + for i := range 40 { + c := WithClient(ctx, "192.0.2."+strings.Repeat("1", 1+i%3)+string(rune('0'+i%10))) + if _, _, err := svc.SignIn(c, "ayse@example.com", "nope"); errors.Is(err, ErrTooManyTries) { + held = true + } + } + if !held { + t.Error("guesses from many clients were not bounded") + } +} + +func TestAHashThatCannotGetATurnSaysTheServerIsBusy(t *testing.T) { + was := argonWait + argonWait = 10 * time.Millisecond + t.Cleanup(func() { argonWait = was }) + // Every turn taken. + n := cap(argonSlots) + for range n { + argonSlots <- struct{}{} + } + t.Cleanup(func() { + for range n { + <-argonSlots + } + }) + if _, err := HashPassword(goodPassword); !errors.Is(err, ErrBusy) { + t.Errorf("a hash with no turn: %v", err) + } +} diff --git a/internal/account/password.go b/internal/account/password.go index f19be13..5cc8e64 100644 --- a/internal/account/password.go +++ b/internal/account/password.go @@ -17,6 +17,7 @@ import ( "fmt" "runtime" "strings" + "time" "golang.org/x/crypto/argon2" ) @@ -57,7 +58,10 @@ func HashPassword(password string) (string, error) { if _, err := rand.Read(salt); err != nil { return "", err } - sum := idKey([]byte(password), salt, argonTime, argonMemory, argonThreads, argonKeyLen) + sum, err := idKey([]byte(password), salt, argonTime, argonMemory, argonThreads, argonKeyLen) + if err != nil { + return "", err + } b64 := base64.RawStdEncoding.EncodeToString return fmt.Sprintf("$argon2id$v=%d$m=%d,t=%d,p=%d$%s$%s", @@ -94,7 +98,10 @@ func VerifyPassword(password, encoded string) (bool, error) { return false, ErrBadHash } - got := idKey([]byte(password), salt, time, memory, threads, uint32(len(want))) + got, err := idKey([]byte(password), salt, time, memory, threads, uint32(len(want))) + if err != nil { + return false, err + } return subtle.ConstantTimeCompare(got, want) == 1, nil } @@ -119,8 +126,19 @@ func init() { // Beyond the bound, attempts wait their turn instead. var argonSlots = make(chan struct{}, max(2, runtime.NumCPU()/2)) -func idKey(password, salt []byte, time, memory uint32, threads uint8, keyLen uint32) []byte { - argonSlots <- struct{}{} +// ErrBusy is a hash that waited its turn too long: the server is answering +// more sign-ins than it can hash, and says so rather than hold the request. +var ErrBusy = errors.New("too many sign-ins at once; try again in a moment") + +// argonWait bounds the wait for a turn; a variable so a test can shorten it. +var argonWait = 10 * time.Second + +func idKey(password, salt []byte, t, memory uint32, threads uint8, keyLen uint32) ([]byte, error) { + select { + case argonSlots <- struct{}{}: + case <-time.After(argonWait): + return nil, ErrBusy + } defer func() { <-argonSlots }() - return argon2.IDKey(password, salt, time, memory, threads, keyLen) + return argon2.IDKey(password, salt, t, memory, threads, keyLen), nil } diff --git a/internal/account/service.go b/internal/account/service.go index 2c5e345..20ea1e0 100644 --- a/internal/account/service.go +++ b/internal/account/service.go @@ -138,7 +138,15 @@ func (s *Service) SignUp(ctx context.Context, email, name, password string) erro // the CSRF token that must accompany the forms in it. func (s *Service) SignIn(ctx context.Context, email, password string) (token, csrf string, err error) { email = store.NormalizeEmail(email) - if !s.allowAttempt(email) { + // Counted per address and the client trying it, so somebody guessing + // from elsewhere cannot lock the owner out of their own account; and per + // address alone, more loosely, so guesses spread over many clients are + // still bounded. + byClient := email + if c := clientFrom(ctx); c != "" { + byClient = email + "|" + c + } + if !s.allowAttempt(byClient, s.cfg.MaxAttempts) || !s.allowAttempt("all|"+email, 10*s.cfg.MaxAttempts) { return "", "", ErrTooManyTries } @@ -154,11 +162,15 @@ func (s *Service) SignIn(ctx context.Context, email, password string) (token, cs } ok, err := VerifyPassword(password, u.PasswordHash) + if errors.Is(err, ErrBusy) { + // Not an answer about the password: said as what it is. + return "", "", err + } if err != nil || !ok { return "", "", ErrBadCredentials } // The password was right, so this was not an attack. - s.clearAttempts(email) + s.clearAttempts(byClient) // State is checked after the password, on purpose. Checking first would // tell anyone who guesses an address whether it has a pending account. @@ -224,7 +236,7 @@ func (s *Service) CheckCSRF(sess store.Session, submitted string) bool { // Per address rather than per IP: an attacker has many addresses to come from // and only one to guess against, and limiting by IP would lock out everyone // behind one office NAT the moment somebody fat-fingers their password. -func (s *Service) allowAttempt(email string) bool { +func (s *Service) allowAttempt(email string, limit int) bool { now := s.now() s.mu.Lock() defer s.mu.Unlock() @@ -251,7 +263,20 @@ func (s *Service) allowAttempt(email string) bool { return true } w.count++ - return w.count <= s.cfg.MaxAttempts + return w.count <= limit +} + +type clientKey struct{} + +// WithClient says which client a request comes from — its address — for the +// sign-in throttle. +func WithClient(ctx context.Context, client string) context.Context { + return context.WithValue(ctx, clientKey{}, client) +} + +func clientFrom(ctx context.Context) string { + c, _ := ctx.Value(clientKey{}).(string) + return c } // maxTrackedAddresses bounds the throttle's memory. Unbounded, it grew by one @@ -307,7 +332,7 @@ type PasswordChanger interface { func (s *Service) ChangePassword(ctx context.Context, pw PasswordChanger, u store.User, current, next string, ) error { - if !s.allowAttempt("pw:" + u.ID) { + if !s.allowAttempt("pw:"+u.ID, s.cfg.MaxAttempts) { return ErrTooManyTries } ok, err := VerifyPassword(current, u.PasswordHash) diff --git a/internal/account/throttle_bound_test.go b/internal/account/throttle_bound_test.go index 11796b1..3a0003e 100644 --- a/internal/account/throttle_bound_test.go +++ b/internal/account/throttle_bound_test.go @@ -12,22 +12,22 @@ func TestSprayingAddressesCannotGrowTheThrottleOrEscapeIt(t *testing.T) { // The target is being guessed at. for i := 0; i < s.cfg.MaxAttempts; i++ { - s.allowAttempt("victim@example.com") + s.allowAttempt("victim@example.com", s.cfg.MaxAttempts) } // Then the table is flooded with made-up addresses. for i := 0; i < maxTrackedAddresses+100; i++ { - s.allowAttempt(fmt.Sprintf("x%d@example.com", i)) + s.allowAttempt(fmt.Sprintf("x%d@example.com", i), s.cfg.MaxAttempts) } if n := len(s.attempts); n > maxTrackedAddresses { t.Errorf("the table grew to %d", n) } - if s.allowAttempt("victim@example.com") { + if s.allowAttempt("victim@example.com", s.cfg.MaxAttempts) { t.Error("flooding the table reset the target's throttle") } // Once the window passes, everything expires and sign-ins work again. now = now.Add(s.cfg.AttemptWindow + time.Second) - if !s.allowAttempt("new@example.com") { + if !s.allowAttempt("new@example.com", s.cfg.MaxAttempts) { t.Error("a new address was refused after the flood expired") } } diff --git a/internal/cli/serverinit.go b/internal/cli/serverinit.go index 408a183..08d931a 100644 --- a/internal/cli/serverinit.go +++ b/internal/cli/serverinit.go @@ -480,7 +480,9 @@ control: auth: enabled: true store: "%s" # a SQLite path, or @file holding a PostgreSQL address - signup_open: true + # The signup form: accounts it makes wait for an administrator's approval, + # but anyone reaching port 8080 can make them. Off until you want it. + signup_open: false # Applied to any key no scope gives a limit to. Without them, a project # created without limits is metered by nothing at all. default_rpm: 60 diff --git a/internal/web/account.go b/internal/web/account.go index 60c36b3..11b85bd 100644 --- a/internal/web/account.go +++ b/internal/web/account.go @@ -135,10 +135,12 @@ func NewAccountPages(a Accounts, k KeyOwner, pw account.PasswordChanger, func (p *AccountPages) Register(mux *http.ServeMux) { if p.accounts.SignupOpen() { mux.HandleFunc("GET /signup", p.signupForm) - mux.HandleFunc("POST /signup", p.signup) + // Each signup costs a password hash and a row: a few a minute from + // one address is a person, more is a script. + mux.HandleFunc("POST /signup", rationed(newRation(5, 10), p.signup)) } mux.HandleFunc("GET /login", p.loginForm) - mux.HandleFunc("POST /login", p.login) + mux.HandleFunc("POST /login", rationed(newRation(30, 30), p.login)) mux.HandleFunc("POST /logout", p.logout) mux.HandleFunc("GET /profile", p.profile) mux.HandleFunc("POST /profile/keys", p.createKey) @@ -196,25 +198,37 @@ type loginData struct { Email, Error string SignupOpen bool Version string + // CSRF is the form's token, matched by a cookie. + CSRF string } -func (p *AccountPages) loginForm(w http.ResponseWriter, _ *http.Request) { +func (p *AccountPages) loginForm(w http.ResponseWriter, r *http.Request) { noStore(w) - render(w, "login", loginData{SignupOpen: p.accounts.SignupOpen(), Version: p.version}) + render(w, "login", loginData{SignupOpen: p.accounts.SignupOpen(), Version: p.version, CSRF: setLoginToken(w, r)}) } func (p *AccountPages) login(w http.ResponseWriter, r *http.Request) { noStore(w) email := strings.TrimSpace(r.FormValue("email")) + if !loginTokenOK(r) { + // Not this site's form: another one posting a visitor into an + // account of its choosing, or a form left open past its hour. + w.WriteHeader(http.StatusBadRequest) + render(w, "login", loginData{Email: email, SignupOpen: p.accounts.SignupOpen(), Version: p.version, + Error: "the form had expired; sign in again", CSRF: setLoginToken(w, r)}) + return + } - token, csrf, err := p.accounts.SignIn(r.Context(), email, r.FormValue("password")) + ctx := account.WithClient(r.Context(), clientAddr(r)) + token, csrf, err := p.accounts.SignIn(ctx, email, r.FormValue("password")) if err != nil { - d := loginData{Email: email, SignupOpen: p.accounts.SignupOpen(), Version: p.version} + d := loginData{Email: email, SignupOpen: p.accounts.SignupOpen(), Version: p.version, CSRF: setLoginToken(w, r)} switch { case errors.Is(err, account.ErrBadCredentials), errors.Is(err, account.ErrNotApproved), errors.Is(err, account.ErrSuspended), - errors.Is(err, account.ErrTooManyTries): + errors.Is(err, account.ErrTooManyTries), + errors.Is(err, account.ErrBusy): d.Error = err.Error() default: p.log.Error("sign-in failed", "err", err) diff --git a/internal/web/formguard.go b/internal/web/formguard.go new file mode 100644 index 0000000..a47aad4 --- /dev/null +++ b/internal/web/formguard.go @@ -0,0 +1,111 @@ +package web + +import ( + "crypto/rand" + "crypto/subtle" + "encoding/hex" + "net" + "net/http" + "sync" + "time" +) + +// The sign-in and signup forms are the two a browser posts before it has a +// session: nothing ties them to the person in front of it, and nothing slows +// a script posting them. Two guards: a token the form carries and a cookie +// matches, so another site cannot sign a visitor into an account of its +// choosing; and a ration per client address, so a script cannot spend the +// password hasher, the database or an address's attempts at the rate it can +// send. + +// loginCSRFCookie is the sign-in form's token, before there is a session. +const loginCSRFCookie = "llmgw_login_csrf" + +// newFormToken is a token for one form. +func newFormToken() string { + b := make([]byte, 16) + _, _ = rand.Read(b) + return hex.EncodeToString(b) +} + +// setLoginToken gives the sign-in form its token and the browser its match. +func setLoginToken(w http.ResponseWriter, r *http.Request) string { + t := newFormToken() + http.SetCookie(w, &http.Cookie{ + Name: loginCSRFCookie, Value: t, Path: "/login", + HttpOnly: true, Secure: isHTTPS(r), SameSite: http.SameSiteStrictMode, MaxAge: 3600, + }) + return t +} + +// loginTokenOK reports whether a posted sign-in form carries the token its +// cookie does. +func loginTokenOK(r *http.Request) bool { + c, err := r.Cookie(loginCSRFCookie) + if err != nil || c.Value == "" { + return false + } + return subtle.ConstantTimeCompare([]byte(c.Value), []byte(r.FormValue("csrf"))) == 1 +} + +// ration is a token bucket per client address. +type ration struct { + mu sync.Mutex + rate float64 // a second + burst float64 + by map[string]*allowance +} + +type allowance struct { + tokens float64 + at time.Time +} + +func newRation(perMinute, burst float64) *ration { + return &ration{rate: perMinute / 60, burst: burst, by: map[string]*allowance{}} +} + +func (q *ration) allow(client string, now time.Time) bool { + q.mu.Lock() + defer q.mu.Unlock() + if len(q.by) > 50_000 { + for k, v := range q.by { + if now.Sub(v.at).Seconds()*q.rate >= q.burst { + delete(q.by, k) + } + } + } + a, ok := q.by[client] + if !ok { + a = &allowance{tokens: q.burst, at: now} + q.by[client] = a + } + a.tokens = min(q.burst, a.tokens+now.Sub(a.at).Seconds()*q.rate) + a.at = now + if a.tokens < 1 { + return false + } + a.tokens-- + return true +} + +// rationed refuses a client past its ration with 429 before next runs. +func rationed(q *ration, next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if !q.allow(clientAddr(r), time.Now()) { + w.Header().Set("Retry-After", "60") + http.Error(w, "too many attempts from this address; wait a minute", http.StatusTooManyRequests) + return + } + next(w, r) + } +} + +// clientAddr is the address a request came from, without its port. +func clientAddr(r *http.Request) string { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return r.RemoteAddr + } + return host +} diff --git a/internal/web/signup_test.go b/internal/web/signup_test.go index 886363f..8370688 100644 --- a/internal/web/signup_test.go +++ b/internal/web/signup_test.go @@ -52,3 +52,48 @@ func TestASignupSaysWhatToDoIfTheAddressHadAnAccount(t *testing.T) { t.Errorf("the done page leaves someone who already had an account stuck: %s", body) } } + +func TestTheSignInFormWantsItsOwnTokenAndRationsAnAddress(t *testing.T) { + mux := http.NewServeMux() + NewAccountPages(signupAccounts{}, nil, nil, "v0", slog.New(slog.DiscardHandler)).Register(mux) + post := func(form url.Values, cookie *http.Cookie) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodPost, "/login", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.RemoteAddr = "203.0.113.9:1234" + if cookie != nil { + req.AddCookie(cookie) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, req) + return rec + } + // No token: another site's form. + if rec := post(url.Values{"email": {"a@b.c"}, "password": {"x"}}, nil); rec.Code != http.StatusBadRequest { + t.Errorf("a sign-in without the form's token: %d", rec.Code) + } + // The form's token, matched by its cookie, reaches the sign-in. + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/login", nil)) + var cookie *http.Cookie + for _, c := range rec.Result().Cookies() { + if c.Name == loginCSRFCookie { + cookie = c + } + } + if cookie == nil || !strings.Contains(rec.Body.String(), cookie.Value) { + t.Fatal("the form carries no token") + } + if rec := post(url.Values{"email": {"a@b.c"}, "password": {"x"}, "csrf": {cookie.Value}}, cookie); rec.Code == http.StatusBadRequest { + t.Errorf("the form's own token was refused") + } + // A script posting it is rationed. + limited := false + for range 40 { + if post(url.Values{"csrf": {cookie.Value}}, cookie).Code == http.StatusTooManyRequests { + limited = true + } + } + if !limited { + t.Error("forty sign-ins a moment from one address were all taken") + } +} diff --git a/internal/web/templates/account.html b/internal/web/templates/account.html index acd9cab..9d023cb 100644 --- a/internal/web/templates/account.html +++ b/internal/web/templates/account.html @@ -41,6 +41,7 @@ an operator to reset it.

{{if .Error}}

{{.Error}}

{{end}}
+ diff --git a/test/e2e/signup_test.go b/test/e2e/signup_test.go index b57aa3c..31b0a7c 100644 --- a/test/e2e/signup_test.go +++ b/test/e2e/signup_test.go @@ -54,6 +54,12 @@ func (b *browser) get(path string) (int, string) { func (b *browser) post(path string, form url.Values) (int, string, string) { b.t.Helper() + // The sign-in form carries a token its page set, as a browser's would. + if path == "/login" && form.Get("csrf") == "" { + _, page := b.get("/login") + form = cloneValues(form) + form.Set("csrf", csrf(b.t, page)) + } req, _ := http.NewRequestWithContext(context.Background(), http.MethodPost, b.base+path, strings.NewReader(form.Encode())) req.Header.Set("Content-Type", "application/x-www-form-urlencoded") @@ -613,3 +619,11 @@ func TestChatSendNeedsItsToken(t *testing.T) { } } } + +func cloneValues(v url.Values) url.Values { + out := url.Values{} + for k, vs := range v { + out[k] = append([]string{}, vs...) + } + return out +}