amadan
Enrolling a passkey needs a fresh session, or a stolen cookie is a permanent lockout
rastrillo's RegisterBegin asks only for a valid session, and for most apps that is right. It is not right here, because of what the Gate and §5.2 add up to: there is no operator reset, by design. The vector, spelled out because it is not obvious. Somebody with a stolen session cookie — good for 30 days — visits /passkey/enrol and enrols THEIR authenticator. From that moment the real owner's magic link lands on the Gate and stops, because the Gate demands a passkey they do not have, and they have no recovery codes because they never enrolled. A compromise that would have expired with the cookie becomes a permanent lockout with nobody to appeal to. /passkey/enrol and both register endpoints now sit behind RequireFresh at ten minutes — longer than the codes page's five, because enrolling means reading a page, deciding, and completing an authenticator gesture, where the codes page is one click on a decision already made. It costs a person who signed in a week ago one more sign-in: RequireFresh redirects a stale GET to the sign-in page with reauth=1, so it is a detour and not a dead end, and the test asserts that rather than just the refusal. It costs an attacker the inbox, which is the point. Verified by breaking it: swapping RequireFresh for Require fails with "a day-old session began a passkey enrolment". FOR PAUL, since this is a tradeoff the spec does not settle rather than something it decides: §5.2 says a passkey is "optional per person" and says nothing about how recently you must have proved yourself to add one. I have taken the safer side because the failure it prevents is unrecoverable and the cost is one extra sign-in. Easy to reverse.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 files changed,
+74
−3
internal/home/app.go+9 −3internal/home/passkey.go+24 −0internal/home/passkey_test.go+41 −0
CI passed — run details
diff --git a/internal/home/app.go b/internal/home/app.go| index c3bd0a7..05ba127 100644 |
| --- a/internal/home/app.go |
| +++ b/internal/home/app.go |
| @@ -160,14 +160,20 @@ func (a *App) Router() http.Handler { |
| // with script off, because recovery is exactly the moment WebAuthn |
| // is not working. |
| r.Get(confirmPath, a.handlePasskeyConfirm) |
| - r.Post("/passkey/register/begin", a.passkeys.RegisterBegin) |
| - r.Post("/passkey/register/finish", a.passkeys.RegisterFinish) |
| r.Post("/passkey/stepup/begin", a.passkeys.StepUpBegin) |
| r.Post("/passkey/stepup/finish", a.passkeys.StepUpFinish) |
| r.Post("/passkey/signin/begin", a.passkeys.SignInBegin) |
| r.Post("/passkey/signin/finish", a.passkeys.SignInFinish) |
| r.Post("/passkey/signin/recovery", a.passkeys.SignInRecovery) |
| - r.Get("/passkey/enrol", a.handlePasskeyEnrol) |
| + // Enrolling changes who can get in, and there is no operator reset, |
| + // so it wants a recent session rather than merely a valid one — see |
| + // enrolFreshness for the lockout this closes. |
| + r.Group(func(r chi.Router) { |
| + r.Use(a.sessions.RequireFresh(enrolFreshness)) |
| + r.Get("/passkey/enrol", a.handlePasskeyEnrol) |
| + r.Post("/passkey/register/begin", a.passkeys.RegisterBegin) |
| + r.Post("/passkey/register/finish", a.passkeys.RegisterFinish) |
| + }) |
| // The codes page shows ten codes once, so it wants a recent session |
| // rather than merely a valid one. |
diff --git a/internal/home/passkey.go b/internal/home/passkey.go| index f0ba80b..f5791a1 100644 |
| --- a/internal/home/passkey.go |
| +++ b/internal/home/passkey.go |
| @@ -40,6 +40,30 @@ const confirmPath = "/passkey/confirm" |
| // silently. |
| const codesFreshness = 5 * time.Minute |
| +// enrolFreshness is how recent a session must be to ADD a passkey. |
| +// |
| +// rastrillo's own RegisterBegin asks only for a valid session, and for |
| +// most apps that is right. It is not right here, because of what the |
| +// Gate plus §5.2 add up to: there is no operator reset, by design. |
| +// |
| +// The vector, spelled out because it is not obvious. Somebody with a |
| +// stolen session cookie — good for 30 days — visits /passkey/enrol and |
| +// enrols THEIR authenticator. From that moment the real owner's magic |
| +// link lands on the Gate and stops, because the Gate demands a passkey |
| +// they do not have; and they have no recovery codes, because they never |
| +// enrolled. A compromise that would have expired with the cookie becomes |
| +// a permanent lockout with nobody to appeal to. |
| +// |
| +// Requiring freshness costs a person who signed in a week ago one more |
| +// sign-in: RequireFresh redirects a stale GET to the sign-in page with |
| +// reauth=1, so it is a detour and not a dead end. It costs an attacker |
| +// the inbox, which is the whole point. |
| +// |
| +// Ten minutes rather than the codes page's five: enrolling means reading |
| +// a page, deciding, and completing an authenticator gesture, where the |
| +// codes page is one click on a decision already made. |
| +const enrolFreshness = 10 * time.Minute |
| + |
| // passkeyConfig is the config the app builds. |
| // |
| // It is returned rather than inlined so a test can assert on what the |
diff --git a/internal/home/passkey_test.go b/internal/home/passkey_test.go| index dcd1562..8eb6d65 100644 |
| --- a/internal/home/passkey_test.go |
| +++ b/internal/home/passkey_test.go |
| @@ -309,3 +309,44 @@ func TestRecoveryCodeProducesAFreshSession(t *testing.T) { |
| "changed, the note on codesFreshness needs revisiting", pw.Code) |
| } |
| } |
| + |
| +// A stolen long-lived session cookie must not be able to enrol its own |
| +// passkey. With no operator reset (§5.2), enrolment by an attacker turns |
| +// a compromise that would have expired with the cookie into a permanent |
| +// lockout: the owner's next magic link lands on the Gate and stops, and |
| +// they have no recovery codes because they never enrolled. |
| +func TestEnrolmentNeedsAFreshSession(t *testing.T) { |
| + app := newTestApp(t) |
| + cookie := signInFully(t, app, "person@example.invalid") |
| + |
| + // Fresh, straight after signing in: the normal path still works. |
| + if rec := postJSON(t, app, "/passkey/register/begin", cookie, nil); rec.Code != http.StatusOK { |
| + t.Fatalf("a fresh session got %d from register/begin, want 200", rec.Code) |
| + } |
| + |
| + // Aged past the window. Both columns, because sessions.Fresh falls |
| + // back to At when AuthTime is zero. |
| + old := time.Now().Add(-24 * time.Hour).UTC().Format(time.RFC3339) |
| + if _, err := app.db.Writer().Exec( |
| + `UPDATE sessions SET auth_time = ?, created_at = ?`, old, old); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + if rec := postJSON(t, app, "/passkey/register/begin", cookie, nil); rec.Code == http.StatusOK { |
| + t.Error("a day-old session began a passkey enrolment") |
| + } |
| + if rec := postJSON(t, app, "/passkey/register/finish", cookie, |
| + map[string]string{"clientDataJSON": "x", "attestationObject": "y"}); rec.Code == http.StatusOK { |
| + t.Error("a day-old session finished a passkey enrolment") |
| + } |
| + |
| + // The page itself redirects to re-authenticate rather than 404ing or |
| + // 403ing: a detour, not a dead end. |
| + req := httptest.NewRequest(http.MethodGet, "/passkey/enrol", nil) |
| + req.AddCookie(cookie) |
| + w := httptest.NewRecorder() |
| + app.Router().ServeHTTP(w, req) |
| + if w.Code != http.StatusSeeOther && w.Code != http.StatusFound { |
| + t.Errorf("GET /passkey/enrol with a stale session = %d, want a redirect to re-auth", w.Code) |
| + } |
| +} |