amadan
Harden local identity bindings and share Keymail continuation
13 files changed,
+483
−48
connect/README.md+34 −3connect/api.go+5 −0connect/linkstore/flows.go+39 −26connect/linkstore/migrations/0002_person_binding.sql+1 −0connect/linkstore/store.go+24 −5connect/linkstore/store_test.go+68 −11connect/navigation.go+10 −2connect/navigation_negative_test.go+126 −0connect/navigation_test.go+30 −0connect/oauth.go+91 −0connect/oauth_test.go+48 −0connect/routes.go+2 −1connect/templates/oauth.html+5 −0
CI cancelled — superseded by a newer push — run details
diff --git a/connect/README.md b/connect/README.md| index f64b797..6cc18bf 100644 |
| --- a/connect/README.md |
| +++ b/connect/README.md |
| @@ -9,6 +9,9 @@ with `DecodeCredentials(settings, origin)` once you know the app's origin. |
| The variables end in `_HOME_ISSUER`, `_HOME_CONSUMER_ID`, `_HOME_KEYS_BASE64` |
| and `_HOME_DEK_BASE64`. All empty means disabled; a partial configuration fails. |
| Use a dedicated random 32-byte storage key and a distinct namespace per app. |
| +Keep this key stable: it protects proof state and keyed local-person bindings. |
| +Replacing it without a migration makes existing links unusable; rotation requires |
| +an explicit migration with both keys, which this package does not yet provide. |
| Apply `handoff.Migrations` and `linkstore.Migrations` using your framework's |
| migration package. The latter's namespace is `linkstore.SchemaNamespace`. |
| @@ -20,7 +23,20 @@ Construct `Bridge` with the hooks in `Config`. Read the actual session cookie; |
| the supplied authentication time. `LookupLocalSession` reads an existing |
| local account. `LookupPerson` uses its supplied SQL query object and rechecks |
| current admission; return `sql.ErrNoRows` for missing or disallowed accounts. |
| -Neither hook creates accounts, samples or identity mappings. |
| +Neither hook creates accounts, samples or identity mappings. The returned label |
| +must be the canonical verified local address, never an editable display name. |
| +Local IDs must never be reused, even for a new account with the same address. |
| +Delete the account's `home_identity_links` rows in the same transaction as the |
| +local account. The shared schema cannot impose a foreign key on app-owned tables. |
| +A keyed binding detects reuse with a different address at every proof stage and |
| +resolution. Address changes require explicit removal and fresh linking; old or |
| +unbound links fail closed, rather than adopting the current account. |
| + |
| +Session cookie values must be exactly 43 characters of canonical unpadded |
| +base64url encoding of 32 bytes. `Session.AuthTime` is the method's actual |
| +verification time. `Session.At` is immutable verified-session creation time, |
| +never last-seen time or request time: legacy magic-link sessions can use it when |
| +`AuthTime` is absent. Keymail sessions cannot use that fallback. |
| On every authenticated app request, resolve Home sessions through |
| `Bridge.ResolveSession`. Use the returned local ID for ownership and grants. |
| @@ -31,11 +47,24 @@ existing behavior in the app's local authentication path. |
| `Routes` supplies shared navigation, local reauthentication markers and the |
| narrow callback exception to your app's CSRF middleware. Mount its methods |
| alongside `Bridge.Start`, `Callback`, `Finish` and `ConfirmPage`. The local |
| -plugin's successful return must be `LocalCompletePath`. Render shared pages |
| +plugin's successful return must be `LocalCompletePath`. Its pending cookie must |
| +be `__Host-rastrillo_pending`, and the local entry is `/signin/local`. |
| +Mount exactly `Bridge.Callback` at `POST /auth/assertion`: this path alone bypasses |
| +CSRF protection because the bridge verifies its browser-bound assertion. |
| +`LocalLandingPath` optionally sets a validated local destination after ordinary |
| +local sign-in (default `/`); explicit account-link navigation takes precedence. Render shared pages |
| inside the app's existing Estilo layout; all paths work without JavaScript. |
| Apps with additional content-security directives must supply their existing |
| policy with the validated Home issuer added to `form-action`. |
| +Wrap the trusted local plugin's Begin handler with `OAuthContinuation` when it |
| +can redirect to Keymail. Render the supplied destination with `OAuthPage` and the |
| +shared `home-oauth-head` and `home-oauth` templates inside your existing layout. |
| +The helper validates the client origin, `/auth/callback`, scope and PKCE method, |
| +then ends the POST with a no-store continuation page so Safari does not apply |
| +form-action to Keymail's redirect chain. It does not discover or trust arbitrary |
| +request-provided destinations. Keep the existing local plugin callback route. |
| + |
| ## Extraction record |
| The link proof protocol, bridge, freshness rules and navigation originate in |
| @@ -49,7 +78,9 @@ Changes are the neutral session DTO and app lookup hooks, new `home_*` link |
| tables, app-selected encryption namespace, and shared rendering/navigation. |
| The local-person foreign key becomes a transaction-scoped existence/admission |
| lookup, because apps use different local identity tables. Every link write |
| -and subsequent resolution still checks the current local account. |
| +and subsequent resolution still checks the current local account. Additive |
| +migration 0002 adds a keyed person binding; earlier unbound rows fail closed. |
| +OAuth continuation also derives from Docs `internal/docs/auth_navigation.go`. |
| The original Docs state-machine tests were ported with synthetic session and |
| local-account hooks. The app-owned content-preservation test now checks an |
diff --git a/connect/api.go b/connect/api.go| index 8868425..91aee15 100644 |
| --- a/connect/api.go |
| +++ b/connect/api.go |
| @@ -13,10 +13,15 @@ import ( |
| const HomeMethod = "home-assertion" |
| +// Session times must come from the verified stored session, never request or last-seen time. |
| +// AuthTime is the method verification time; At is immutable session creation time. |
| type Session struct { |
| Subject, Method string |
| AuthTime, At time.Time |
| } |
| + |
| +// Person.Label is the canonical verified local address, never a display name. |
| +// Local IDs must never be reused, including for a new account with the same address. |
| type Person struct { |
| ID int64 |
| Label string |
diff --git a/connect/linkstore/flows.go b/connect/linkstore/flows.go| index 91e579b..567fa3c 100644 |
| --- a/connect/linkstore/flows.go |
| +++ b/connect/linkstore/flows.go |
| @@ -3,6 +3,7 @@ package linkstore |
| import ( |
| "bytes" |
| "context" |
| + "crypto/hmac" |
| "database/sql" |
| "encoding/json" |
| "errors" |
| @@ -13,16 +14,17 @@ import ( |
| ) |
| type flow struct { |
| - Version int `json:"v"` |
| - PersonID int64 `json:"person"` |
| - SessionHash string `json:"session"` |
| - LocalAuth int64 `json:"local_auth"` |
| - Issuer string `json:"issuer"` |
| - NonceHash string `json:"nonce,omitempty"` |
| - Subject string `json:"subject,omitempty"` |
| - HomeAuth int64 `json:"home_auth,omitempty"` |
| - ReturnPath string `json:"return"` |
| - Expires int64 `json:"expires"` |
| + PersonBinding string `json:"person_binding"` |
| + Version int `json:"v"` |
| + PersonID int64 `json:"person"` |
| + SessionHash string `json:"session"` |
| + LocalAuth int64 `json:"local_auth"` |
| + Issuer string `json:"issuer"` |
| + NonceHash string `json:"nonce,omitempty"` |
| + Subject string `json:"subject,omitempty"` |
| + HomeAuth int64 `json:"home_auth,omitempty"` |
| + ReturnPath string `json:"return"` |
| + Expires int64 `json:"expires"` |
| } |
| func (s *Store) purpose(stage, id string) string { |
| @@ -83,15 +85,18 @@ func (s *Store) load(ctx context.Context, tx *sql.Tx, stage, id string, high int |
| func (s *Store) matches(f flow, local Local, now, high int64) bool { |
| return validLocal(local, now, high) && f.PersonID == local.PersonID && f.SessionHash == hash(local.SessionToken) && f.LocalAuth == local.AuthTime.Unix() && fresh(f.LocalAuth, now, high) |
| } |
| -func (s *Store) personExists(ctx context.Context, tx *sql.Tx, id int64) (bool, error) { |
| +func (s *Store) currentBinding(ctx context.Context, tx *sql.Tx, id int64) (string, error) { |
| label, err := s.lookup(ctx, tx, id) |
| if errors.Is(err, sql.ErrNoRows) { |
| - return false, nil |
| + return "", nil |
| } |
| if err != nil { |
| - return false, ErrStorage |
| + return "", ErrStorage |
| + } |
| + if label == "" { |
| + return "", nil |
| } |
| - return label != "", nil |
| + return s.personBinding(id, label), nil |
| } |
| // Start is called only by an explicit same-origin local-account link POST. |
| @@ -108,11 +113,11 @@ func (s *Store) Start(ctx context.Context, local Local, returnPath string, now t |
| if !validLocal(local, now.Unix(), high) { |
| return "", refuse(tx) |
| } |
| - exists, err := s.personExists(ctx, tx, local.PersonID) |
| + binding, err := s.currentBinding(ctx, tx, local.PersonID) |
| if err != nil { |
| return "", err |
| } |
| - if !exists { |
| + if binding == "" { |
| return "", refuse(tx) |
| } |
| var count int |
| @@ -130,7 +135,7 @@ func (s *Store) Start(ctx context.Context, local Local, returnPath string, now t |
| if expires <= high { |
| return "", refuse(tx) |
| } |
| - f := flow{Version: 1, PersonID: local.PersonID, SessionHash: hash(local.SessionToken), LocalAuth: local.AuthTime.Unix(), Issuer: s.issuer, ReturnPath: safePath(returnPath), Expires: expires} |
| + f := flow{PersonBinding: binding, Version: 1, PersonID: local.PersonID, SessionHash: hash(local.SessionToken), LocalAuth: local.AuthTime.Unix(), Issuer: s.issuer, ReturnPath: safePath(returnPath), Expires: expires} |
| if err := s.put(ctx, tx, "init", hash(secret), f); err != nil { |
| return "", err |
| } |
| @@ -162,6 +167,13 @@ func (s *Store) BindNonce(ctx context.Context, secret string, local Local, nonce |
| if !s.matches(f, local, now.Unix(), high) || !handoff.ValidNonce(nonce) || f.NonceHash != "" { |
| return refuse(tx) |
| } |
| + binding, err := s.currentBinding(ctx, tx, local.PersonID) |
| + if err != nil { |
| + return err |
| + } |
| + if binding == "" || !hmac.Equal([]byte(binding), []byte(f.PersonBinding)) { |
| + return refuse(tx) |
| + } |
| f.NonceHash = hash(nonce) |
| if err := s.put(ctx, tx, "init", id, f); err != nil { |
| return err |
| @@ -191,11 +203,11 @@ func (s *Store) Complete(ctx context.Context, secret string, local Local, home H |
| if !s.matches(f, local, now.Unix(), high) || !handoff.ValidNonce(home.Nonce) || f.NonceHash != hash(home.Nonce) || home.Issuer != s.issuer || !validSubject(home.Subject) || !fresh(home.AuthTime.Unix(), now.Unix(), high) { |
| return "", refuse(tx) |
| } |
| - exists, err := s.personExists(ctx, tx, local.PersonID) |
| + binding, err := s.currentBinding(ctx, tx, local.PersonID) |
| if err != nil { |
| return "", err |
| } |
| - if !exists { |
| + if binding == "" || !hmac.Equal([]byte(binding), []byte(f.PersonBinding)) { |
| return "", refuse(tx) |
| } |
| pending, err := newSecret() |
| @@ -245,11 +257,11 @@ func (s *Store) Pending(ctx context.Context, secret string, local Local, now tim |
| if !s.matches(f, local, now.Unix(), high) || !validPending(f, now.Unix(), high) { |
| return Pending{}, refuse(tx) |
| } |
| - exists, err := s.personExists(ctx, tx, local.PersonID) |
| + binding, err := s.currentBinding(ctx, tx, local.PersonID) |
| if err != nil { |
| return Pending{}, err |
| } |
| - if !exists { |
| + if binding == "" || !hmac.Equal([]byte(binding), []byte(f.PersonBinding)) { |
| return Pending{}, refuse(tx) |
| } |
| if err := s.put(ctx, tx, "pending", id, f); err != nil { |
| @@ -284,26 +296,27 @@ func (s *Store) Confirm(ctx context.Context, secret string, local Local, now tim |
| if !s.matches(f, local, now.Unix(), high) || !validPending(f, now.Unix(), high) { |
| return Pending{}, refuse(tx) |
| } |
| - exists, err := s.personExists(ctx, tx, local.PersonID) |
| + binding, err := s.currentBinding(ctx, tx, local.PersonID) |
| if err != nil { |
| return Pending{}, err |
| } |
| - if !exists { |
| + if binding == "" || !hmac.Equal([]byte(binding), []byte(f.PersonBinding)) { |
| return Pending{}, refuse(tx) |
| } |
| - _, err = tx.ExecContext(ctx, "INSERT INTO home_identity_links (issuer, subject, person_id) VALUES (?, ?, ?) ON CONFLICT DO NOTHING", s.issuer, f.Subject, f.PersonID) |
| + _, err = tx.ExecContext(ctx, "INSERT INTO home_identity_links (issuer, subject, person_id, person_binding) VALUES (?, ?, ?, ?) ON CONFLICT DO NOTHING", s.issuer, f.Subject, f.PersonID, f.PersonBinding) |
| if err != nil { |
| return Pending{}, ErrStorage |
| } |
| var person int64 |
| - err = tx.QueryRowContext(ctx, "SELECT person_id FROM home_identity_links WHERE issuer = ? AND subject = ?", s.issuer, f.Subject).Scan(&person) |
| + var storedBinding string |
| + err = tx.QueryRowContext(ctx, "SELECT person_id, person_binding FROM home_identity_links WHERE issuer = ? AND subject = ?", s.issuer, f.Subject).Scan(&person, &storedBinding) |
| if errors.Is(err, sql.ErrNoRows) { |
| return Pending{}, refuse(tx) |
| } |
| if err != nil { |
| return Pending{}, ErrStorage |
| } |
| - if person != f.PersonID { |
| + if person != f.PersonID || !hmac.Equal([]byte(storedBinding), []byte(f.PersonBinding)) { |
| return Pending{}, refuse(tx) |
| } |
| if err := commit(tx); err != nil { |
diff --git a/connect/linkstore/migrations/0002_person_binding.sql b/connect/linkstore/migrations/0002_person_binding.sql| new file mode 100644 |
| index 0000000..e9c78cd |
| --- /dev/null |
| +++ b/connect/linkstore/migrations/0002_person_binding.sql |
| @@ -0,0 +1 @@ |
| +ALTER TABLE home_identity_links ADD COLUMN person_binding TEXT NOT NULL DEFAULT ''; |
diff --git a/connect/linkstore/store.go b/connect/linkstore/store.go| index edd5593..544c1ff 100644 |
| --- a/connect/linkstore/store.go |
| +++ b/connect/linkstore/store.go |
| @@ -4,11 +4,13 @@ package linkstore |
| import ( |
| "context" |
| + "crypto/hmac" |
| "crypto/rand" |
| "crypto/sha256" |
| "database/sql" |
| "embed" |
| "encoding/base64" |
| + "encoding/binary" |
| "encoding/hex" |
| "errors" |
| "net/url" |
| @@ -64,6 +66,8 @@ type Store struct { |
| seal *seal.Sealer |
| origin, issuer string |
| lookup LookupPerson |
| + bindingKey []byte |
| + namespace string |
| } |
| // New uses a dedicated handoff/storage DEK, never an application content key. |
| @@ -76,7 +80,7 @@ func New(db *sql.DB, dek []byte, origin, issuer, namespace string, lookup Lookup |
| if err != nil { |
| return nil, ErrRefused |
| } |
| - return &Store{db: db, seal: sealer, origin: origin, issuer: issuer, lookup: lookup}, nil |
| + return &Store{db: db, seal: sealer, origin: origin, issuer: issuer, lookup: lookup, bindingKey: append([]byte(nil), dek...), namespace: namespace}, nil |
| } |
| func validOrigin(v string) bool { |
| @@ -178,8 +182,8 @@ func (s *Store) Resolve(ctx context.Context, issuer, subject string) (int64, str |
| return 0, "", ErrRefused |
| } |
| var id int64 |
| - var address string |
| - err := s.db.QueryRowContext(ctx, `SELECT person_id FROM home_identity_links WHERE issuer = ? AND subject = ?`, issuer, subject).Scan(&id) |
| + var address, binding string |
| + err := s.db.QueryRowContext(ctx, `SELECT person_id, person_binding FROM home_identity_links WHERE issuer = ? AND subject = ?`, issuer, subject).Scan(&id, &binding) |
| if errors.Is(err, sql.ErrNoRows) { |
| return 0, "", ErrRefused |
| } |
| @@ -193,7 +197,7 @@ func (s *Store) Resolve(ctx context.Context, issuer, subject string) (int64, str |
| if err != nil { |
| return 0, "", ErrStorage |
| } |
| - if id <= 0 || address == "" { |
| + if id <= 0 || address == "" || !hmac.Equal([]byte(binding), []byte(s.personBinding(id, address))) { |
| return 0, "", ErrRefused |
| } |
| return id, address, nil |
| @@ -211,7 +215,22 @@ type Queryer interface { |
| QueryRowContext(context.Context, string, ...any) *sql.Row |
| } |
| -// LookupPerson returns a label for an existing local person. It must never |
| +// LookupPerson returns the canonical verified local address, never a display name. |
| +// IDs must never be reused; deletion must remove links in the same transaction. It must never |
| // create one, and must use q rather than opening a separate transaction. |
| // Missing or currently inadmissible people return sql.ErrNoRows. |
| type LookupPerson func(ctx context.Context, q Queryer, id int64) (string, error) |
| + |
| +// personBinding prevents a stale numeric link from resolving a different address. |
| +// Length-prefixed fields keep the namespace, origin, ID and address unambiguous. |
| +func (s *Store) personBinding(id int64, address string) string { |
| + m := hmac.New(sha256.New, s.bindingKey) |
| + for _, field := range []string{"home-connect/person-binding/v1", s.namespace, s.origin, s.issuer} { |
| + _ = binary.Write(m, binary.BigEndian, uint64(len(field))) |
| + _, _ = m.Write([]byte(field)) |
| + } |
| + _ = binary.Write(m, binary.BigEndian, id) |
| + _ = binary.Write(m, binary.BigEndian, uint64(len(address))) |
| + _, _ = m.Write([]byte(address)) |
| + return hex.EncodeToString(m.Sum(nil)) |
| +} |
diff --git a/connect/linkstore/store_test.go b/connect/linkstore/store_test.go| index ca1c5fe..f965b76 100644 |
| --- a/connect/linkstore/store_test.go |
| +++ b/connect/linkstore/store_test.go |
| @@ -36,12 +36,14 @@ func fixture(t *testing.T) (*Store, string) { |
| if err != nil { |
| t.Fatal(err) |
| } |
| - migration, err := migrations.ReadFile("migrations/0001_links.sql") |
| - if err != nil { |
| - t.Fatal(err) |
| - } |
| - if _, err := d.Exec(string(migration)); err != nil { |
| - t.Fatal(err) |
| + for _, name := range []string{"0001_links.sql", "0002_person_binding.sql"} { |
| + migration, err := migrations.ReadFile("migrations/" + name) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := d.Exec(string(migration)); err != nil { |
| + t.Fatal(err) |
| + } |
| } |
| s, err := New(d, bytes.Repeat([]byte{42}, 32), origin, issuer, "oficina/docs", testLookup) |
| if err != nil { |
| @@ -125,10 +127,8 @@ func TestExplicitLinkPreservesPeopleAndContent(t *testing.T) { |
| if _, err := s.db.Exec("UPDATE people SET email = 'new@example.com' WHERE id = 1"); err != nil { |
| t.Fatal(err) |
| } |
| - _, address, err := s.Resolve(ctx, issuer, "opaque-home-b") |
| - if err != nil || address != "new@example.com" { |
| - t.Fatalf("stale address %q %v", address, err) |
| - } |
| + _, _, err := s.Resolve(ctx, issuer, "opaque-home-b") |
| + refused(t, err) |
| _, _, err = s.Resolve(ctx, "https://other.example", "opaque-home-b") |
| refused(t, err) |
| if flowCount(t, s) != 0 { |
| @@ -280,7 +280,7 @@ func TestConflictConsumedWithoutReplacingEitherMapping(t *testing.T) { |
| } |
| _, err = s.Confirm(ctx, cookie, tc.local, testNow) |
| refused(t, err) |
| - if _, err := s.db.Exec("INSERT INTO home_identity_links VALUES (?,?,?)", issuer, "subject-one", 1); err != nil { |
| + if _, err := s.db.Exec("INSERT INTO home_identity_links VALUES (?,?,?,?)", issuer, "subject-one", 1, s.personBinding(1, "one@example.com")); err != nil { |
| t.Fatal(err) |
| } |
| } |
| @@ -588,3 +588,60 @@ func TestResolveRechecksAppAdmissionAndDoesNotRevealLookupErrors(t *testing.T) { |
| t.Fatal("lookup error escaped", err) |
| } |
| } |
| + |
| +func TestPersonBindingRefusesReusedIDAtEveryStage(t *testing.T) { |
| + for _, stage := range []string{"bind", "complete", "pending", "confirm", "resolve"} { |
| + t.Run(stage, func(t *testing.T) { |
| + s, _ := fixture(t) |
| + l := local(1) |
| + secret, err := s.Start(ctx, l, "/", testNow) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + nonce := nonce(42) |
| + if stage != "bind" { |
| + if err := s.BindNonce(ctx, secret, l, nonce, testNow); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if stage != "bind" && stage != "complete" { |
| + secret, err = s.Complete(ctx, secret, l, Home{Issuer: issuer, Subject: "person", Nonce: nonce, AuthTime: testNow}, testNow) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if stage == "resolve" { |
| + if _, err := s.Confirm(ctx, secret, l, testNow); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + if _, err := s.db.Exec("DELETE FROM people WHERE id=1; INSERT INTO people VALUES(1,'replacement@example.com')"); err != nil { |
| + t.Fatal(err) |
| + } |
| + switch stage { |
| + case "bind": |
| + err = s.BindNonce(ctx, secret, l, nonce, testNow) |
| + case "complete": |
| + _, err = s.Complete(ctx, secret, l, Home{Issuer: issuer, Subject: "person", Nonce: nonce, AuthTime: testNow}, testNow) |
| + case "pending": |
| + _, err = s.Pending(ctx, secret, l, testNow) |
| + case "confirm": |
| + _, err = s.Confirm(ctx, secret, l, testNow) |
| + case "resolve": |
| + _, _, err = s.Resolve(ctx, issuer, "person") |
| + } |
| + refused(t, err) |
| + if stage != "resolve" && flowCount(t, s) != 0 { |
| + t.Fatal("rejected proof was not consumed") |
| + } |
| + }) |
| + } |
| +} |
| +func TestUnboundLegacyMappingFailsClosed(t *testing.T) { |
| + s, _ := fixture(t) |
| + if _, err := s.db.Exec("INSERT INTO home_identity_links(issuer,subject,person_id) VALUES(?,?,?)", issuer, "legacy", 1); err != nil { |
| + t.Fatal(err) |
| + } |
| + _, _, err := s.Resolve(ctx, issuer, "legacy") |
| + refused(t, err) |
| +} |
diff --git a/connect/navigation.go b/connect/navigation.go| index 23e4319..c3adc89 100644 |
| --- a/connect/navigation.go |
| +++ b/connect/navigation.go |
| @@ -25,6 +25,7 @@ type localLinkMarker struct { |
| } |
| type localLinkNavigation struct { |
| + landingPath string |
| sealer handoff.Sealer |
| sessionCookie string |
| readSession func(*http.Request) (*Session, error) |
| @@ -39,7 +40,14 @@ func newLocalLinkNavigation(sealer handoff.Sealer, c RoutesConfig) *localLinkNav |
| if err != nil || u.Scheme != "https" || u.Host == "" || u.User != nil || u.Path != "" || u.RawQuery != "" || u.Fragment != "" || sealer == nil || c.ReadSession == nil { |
| return nil |
| } |
| - return &localLinkNavigation{sealer: sealer, sessionCookie: c.SessionCookie, readSession: c.ReadSession, namespace: c.Namespace, origin: c.Origin, host: u.Host, now: time.Now, fail: func(w http.ResponseWriter, r *http.Request, status int) { c.Render(w, r, "failed", "", status) }} |
| + landing := c.LocalLandingPath |
| + if landing == "" { |
| + landing = "/" |
| + } |
| + if !handoff.SafePath(landing) { |
| + return nil |
| + } |
| + return &localLinkNavigation{landingPath: landing, sealer: sealer, sessionCookie: c.SessionCookie, readSession: c.ReadSession, namespace: c.Namespace, origin: c.Origin, host: u.Host, now: time.Now, fail: func(w http.ResponseWriter, r *http.Request, status int) { c.Render(w, r, "failed", "", status) }} |
| } |
| func localNavigationCookie(w http.ResponseWriter, name, value string, age int) { |
| @@ -136,7 +144,7 @@ func (n *localLinkNavigation) Complete(w http.ResponseWriter, r *http.Request) { |
| return |
| } |
| now := n.now() |
| - target := "/" |
| + target := n.landingPath |
| if n.marker(r, now.Unix()) { |
| count := 0 |
| for _, c := range r.Cookies() { |
diff --git a/connect/navigation_negative_test.go b/connect/navigation_negative_test.go| new file mode 100644 |
| index 0000000..05d2e4b |
| --- /dev/null |
| +++ b/connect/navigation_negative_test.go |
| @@ -0,0 +1,126 @@ |
| +package connect |
| + |
| +import ( |
| + "encoding/base64" |
| + "encoding/json" |
| + "errors" |
| + "net/http" |
| + "net/http/httptest" |
| + "strings" |
| + "testing" |
| + "time" |
| +) |
| + |
| +func navigationRequest(path string) *http.Request { |
| + return httptest.NewRequest("GET", navigationOrigin+path, nil) |
| +} |
| +func navigationMarker(t *testing.T, n *localLinkNavigation) *http.Cookie { |
| + t.Helper() |
| + w := httptest.NewRecorder() |
| + n.Begin(w, navigationRequest("/account/home")) |
| + for _, c := range w.Result().Cookies() { |
| + if c.Name == localLinkNavigationCookie && c.MaxAge > 0 { |
| + return c |
| + } |
| + } |
| + t.Fatal("marker missing") |
| + return nil |
| +} |
| +func assertNavigation(t *testing.T, n *localLinkNavigation, r *http.Request, want string) { |
| + t.Helper() |
| + rec := httptest.NewRecorder() |
| + n.Complete(rec, r) |
| + if rec.Code != 303 || rec.Header().Get("Location") != want || rec.Header().Get("Cache-Control") != "no-store" { |
| + t.Fatalf("navigation status=%d target=%q wanted%q", rec.Code, rec.Header().Get("Location"), want) |
| + } |
| + cleared := false |
| + for _, c := range rec.Result().Cookies() { |
| + if c.Name == localLinkNavigationCookie && c.MaxAge < 0 { |
| + cleared = true |
| + } |
| + } |
| + if !cleared { |
| + t.Fatal("marker not cleared") |
| + } |
| +} |
| + |
| +func TestLocalNavigationRejectsInvalidMarkersAndOtherHosts(t *testing.T) { |
| + local, n := navigationFixture(t) |
| + now := time.Now().Truncate(time.Second) |
| + local.At = now |
| + n.now = func() time.Time { return now } |
| + cookie := &http.Cookie{Name: n.sessionCookie, Value: "verified"} |
| + valid := navigationMarker(t, n).Value |
| + positive := navigationRequest(localLinkCompletePath) |
| + positive.AddCookie(cookie) |
| + positive.AddCookie(&http.Cookie{Name: localLinkNavigationCookie, Value: valid}) |
| + assertNavigation(t, n, positive, "/account/home") |
| + plain := func(m localLinkMarker) string { b, _ := json.Marshal(m); return string(b) } |
| + base := localLinkMarker{1, "link-start", now.Unix(), now.Unix() + 600} |
| + badJSON := []string{ |
| + "{}", `{"v":1,"v":1,"action":"link-start","issued":1,"expires":2}`, |
| + plain(base) + " {}", strings.TrimSuffix(plain(base), "}") + `,"unknown":true}`, |
| + } |
| + for _, m := range []localLinkMarker{{2, "link-start", now.Unix(), now.Unix() + 600}, {1, "other", now.Unix(), now.Unix() + 600}, {1, "link-start", 0, now.Unix() + 600}, {1, "link-start", now.Unix() + 1, now.Unix() + 600}, {1, "link-start", now.Unix() - 600, now.Unix()}, {1, "link-start", now.Unix(), now.Unix() + 601}} { |
| + badJSON = append(badJSON, plain(m)) |
| + } |
| + badValues := []string{"", strings.Repeat("a", 1025), valid + "=", "%" + valid, valid[:len(valid)-2]} |
| + for _, p := range badJSON { |
| + sealed, err := n.sealer.Seal(n.purpose(), []byte(p)) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + badValues = append(badValues, base64.RawURLEncoding.EncodeToString(sealed)) |
| + } |
| + otherPurpose, err := n.sealer.Seal(n.namespace+".local-link-navigation.v1:https://other.example", []byte(plain(base))) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + badValues = append(badValues, base64.RawURLEncoding.EncodeToString(otherPurpose)) |
| + for _, value := range badValues { |
| + r := navigationRequest(localLinkCompletePath) |
| + r.AddCookie(cookie) |
| + r.AddCookie(&http.Cookie{Name: localLinkNavigationCookie, Value: value}) |
| + assertNavigation(t, n, r, "/") |
| + } |
| + for _, change := range []func(*http.Request){func(r *http.Request) { r.Host = "other.example" }, func(r *http.Request) { r.URL.RawQuery = "return_to=https://evil.example" }, func(r *http.Request) { r.URL.Path = "/wrong" }} { |
| + r := navigationRequest(localLinkCompletePath) |
| + r.AddCookie(cookie) |
| + r.AddCookie(&http.Cookie{Name: localLinkNavigationCookie, Value: valid}) |
| + change(r) |
| + assertNavigation(t, n, r, "/") |
| + } |
| + // A valid marker expires independently of the browser cookie's MaxAge. |
| + n.now = func() time.Time { return now.Add(600 * time.Second) } |
| + r := navigationRequest(localLinkCompletePath) |
| + r.AddCookie(cookie) |
| + r.AddCookie(&http.Cookie{Name: localLinkNavigationCookie, Value: valid}) |
| + assertNavigation(t, n, r, "/") |
| +} |
| + |
| +type navigationBrokenSealer struct{} |
| + |
| +func (navigationBrokenSealer) Seal(string, []byte) ([]byte, error) { |
| + return nil, errors.New("private failure") |
| +} |
| +func (navigationBrokenSealer) Open(string, []byte) ([]byte, error) { |
| + return nil, errors.New("private failure") |
| +} |
| +func TestLocalNavigationStorageFailureAndMethod(t *testing.T) { |
| + _, n := navigationFixture(t) |
| + n.sealer = navigationBrokenSealer{} |
| + rec := httptest.NewRecorder() |
| + n.Begin(rec, navigationRequest("/account/home")) |
| + if rec.Code != 503 || strings.Contains(rec.Body.String(), "private") || rec.Header().Get("Location") != "" { |
| + t.Fatal("storage failure published/leaked navigation") |
| + } |
| + for _, fn := range []http.HandlerFunc{n.Begin, n.Complete} { |
| + rec = httptest.NewRecorder() |
| + r := navigationRequest(localLinkCompletePath) |
| + r.Method = "POST" |
| + fn(rec, r) |
| + if rec.Code != 405 || rec.Header().Get("Allow") != "GET" || rec.Header().Get("Cache-Control") != "no-store" { |
| + t.Fatal("wrong method accepted") |
| + } |
| + } |
| +} |
diff --git a/connect/navigation_test.go b/connect/navigation_test.go| index 0ea147a..5153217 100644 |
| --- a/connect/navigation_test.go |
| +++ b/connect/navigation_test.go |
| @@ -100,3 +100,33 @@ func TestNavigationPurposeIsolationAndFixedAction(t *testing.T) { |
| } |
| } |
| } |
| + |
| +func TestNavigationLocalLandingPathIsSafeAndDoesNotOverrideLink(t *testing.T) { |
| + _, n := navigationFixture(t) |
| + config := RoutesConfig{Namespace: n.namespace, Origin: n.origin, SessionCookie: n.sessionCookie, ReadSession: n.readSession, Render: func(w http.ResponseWriter, r *http.Request, state, label string, status int) { w.WriteHeader(status) }} |
| + for _, target := range []string{"https://other.example/", "//other.example/", "/\\other.example/"} { |
| + config.LocalLandingPath = target |
| + if newLocalLinkNavigation(n.sealer, config) != nil { |
| + t.Fatal("unsafe local landing accepted", target) |
| + } |
| + } |
| + config.LocalLandingPath = "/continue" |
| + land := newLocalLinkNavigation(n.sealer, config) |
| + if land == nil { |
| + t.Fatal("safe local landing refused") |
| + } |
| + r := navigationRequest(localLinkCompletePath) |
| + w := httptest.NewRecorder() |
| + land.Complete(w, r) |
| + if w.Header().Get("Location") != "/continue" { |
| + t.Fatal("local landing ignored") |
| + } |
| + marker := navigationMarker(t, land) |
| + r.AddCookie(marker) |
| + r.AddCookie(&http.Cookie{Name: land.sessionCookie, Value: "verified"}) |
| + w = httptest.NewRecorder() |
| + land.Complete(w, r) |
| + if w.Header().Get("Location") != "/account/home" { |
| + t.Fatal("landing replaced explicit link navigation") |
| + } |
| +} |
diff --git a/connect/oauth.go b/connect/oauth.go| new file mode 100644 |
| index 0000000..d306720 |
| --- /dev/null |
| +++ b/connect/oauth.go |
| @@ -0,0 +1,91 @@ |
| +package connect |
| + |
| +import ( |
| + "bytes" |
| + "net/http" |
| + "net/url" |
| + "strings" |
| +) |
| + |
| +// OAuthContinuation ends the local POST before navigating to Keymail. Safari |
| +// otherwise applies form-action to the entire external redirect chain. |
| +// Wrap only the trusted local authentication plugin's Begin handler. |
| +func OAuthContinuation(origin string, next http.HandlerFunc, render func(http.ResponseWriter, *http.Request, string)) http.HandlerFunc { |
| + return func(w http.ResponseWriter, r *http.Request) { |
| + if next == nil || render == nil { |
| + http.Error(w, "Sign-in unavailable.", http.StatusServiceUnavailable) |
| + return |
| + } |
| + result := &oauthResponse{header: make(http.Header)} |
| + next(result, r) |
| + destination := result.header.Get("Location") |
| + external := destination != "" && (!strings.HasPrefix(destination, "/") || strings.HasPrefix(destination, "//")) |
| + if result.status == http.StatusSeeOther && external { |
| + if !keymailDestination(destination, origin) { |
| + w.Header().Set("Cache-Control", "no-store") |
| + http.Error(w, "Sign-in unavailable.", http.StatusBadGateway) |
| + return |
| + } |
| + for _, cookie := range result.header.Values("Set-Cookie") { |
| + w.Header().Add("Set-Cookie", cookie) |
| + } |
| + w.Header().Set("Cache-Control", "no-store") |
| + w.Header().Set("Referrer-Policy", "no-referrer") |
| + render(w, r, destination) |
| + return |
| + } |
| + for name, values := range result.header { |
| + for _, value := range values { |
| + w.Header().Add(name, value) |
| + } |
| + } |
| + if result.status == 0 { |
| + result.status = http.StatusOK |
| + } |
| + w.WriteHeader(result.status) |
| + _, _ = w.Write(result.body.Bytes()) |
| + } |
| +} |
| + |
| +func keymailDestination(destination, origin string) bool { |
| + u, err := url.Parse(destination) |
| + if err != nil || u.Scheme != "https" || u.Host == "" || u.User != nil || u.Fragment != "" || u.Path != "/oauth/authorize" || u.RawPath != "" { |
| + return false |
| + } |
| + q, err := url.ParseQuery(u.RawQuery) |
| + if err != nil { |
| + return false |
| + } |
| + for _, key := range []string{"client_id", "redirect_uri", "scope", "code_challenge_method", "state", "code_challenge"} { |
| + if len(q[key]) != 1 || q.Get(key) == "" { |
| + return false |
| + } |
| + } |
| + origin = strings.TrimSuffix(origin, "/") |
| + return q.Get("client_id") == origin && q.Get("redirect_uri") == origin+"/auth/callback" && q.Get("scope") == "identify" && q.Get("code_challenge_method") == "S256" |
| +} |
| + |
| +type oauthResponse struct { |
| + header http.Header |
| + status int |
| + body bytes.Buffer |
| +} |
| + |
| +func (w *oauthResponse) Header() http.Header { return w.header } |
| +func (w *oauthResponse) WriteHeader(status int) { |
| + if w.status == 0 { |
| + w.status = status |
| + } |
| +} |
| +func (w *oauthResponse) Write(p []byte) (int, error) { |
| + if w.status == 0 { |
| + w.WriteHeader(http.StatusOK) |
| + } |
| + return w.body.Write(p) |
| +} |
| + |
| +type OAuthData struct{ URL, Title, ActionLabel string } |
| + |
| +func OAuthPage(destination string) OAuthData { |
| + return OAuthData{URL: destination, Title: "Continue signing in", ActionLabel: "Continue to Keymail"} |
| +} |
diff --git a/connect/oauth_test.go b/connect/oauth_test.go| new file mode 100644 |
| index 0000000..7d88a07 |
| --- /dev/null |
| +++ b/connect/oauth_test.go |
| @@ -0,0 +1,48 @@ |
| +package connect |
| + |
| +import ( |
| + "net/http" |
| + "net/http/httptest" |
| + "net/url" |
| + "strings" |
| + "testing" |
| +) |
| + |
| +func oauthTarget() string { |
| + return "https://keymail.example/oauth/authorize?" + url.Values{"client_id": {configOrigin}, "redirect_uri": {configOrigin + "/auth/callback"}, "scope": {"identify"}, "code_challenge_method": {"S256"}, "state": {"private-state"}, "code_challenge": {"challenge"}}.Encode() |
| +} |
| +func TestOAuthContinuationPreservesPolicyCookiesAndLocalResponses(t *testing.T) { |
| + for _, destination := range []string{oauthTarget(), "/signin?sent=1"} { |
| + w := httptest.NewRecorder() |
| + w.Header().Set("Content-Security-Policy", "form-action 'self'") |
| + rendered := "" |
| + h := OAuthContinuation(configOrigin, func(w http.ResponseWriter, r *http.Request) { |
| + w.Header().Add("Set-Cookie", "pending=private; Path=/; Secure; HttpOnly") |
| + http.Redirect(w, r, destination, 303) |
| + }, func(w http.ResponseWriter, r *http.Request, target string) { rendered = target; w.WriteHeader(200) }) |
| + h(w, httptest.NewRequest("POST", configOrigin+"/signin", nil)) |
| + if w.Header().Get("Content-Security-Policy") != "form-action 'self'" || len(w.Result().Cookies()) != 1 { |
| + t.Fatal("policy or pending cookie changed") |
| + } |
| + if strings.HasPrefix(destination, "https:") { |
| + if w.Code != 200 || rendered != destination || w.Header().Get("Location") != "" || w.Header().Get("Cache-Control") != "no-store" || w.Header().Get("Referrer-Policy") != "no-referrer" { |
| + t.Fatal("OAuth POST did not terminate", w.Code, w.Header()) |
| + } |
| + } else if w.Code != 303 || rendered != "" || w.Header().Get("Location") != destination { |
| + t.Fatal("local flow changed") |
| + } |
| + } |
| +} |
| +func TestOAuthContinuationRefusesUnboundDestinationsWithoutLeakingState(t *testing.T) { |
| + for _, destination := range []string{"//keymail.example/oauth/authorize", strings.Replace(oauthTarget(), "https:", "http:", 1), strings.Replace(oauthTarget(), "scope=identify", "scope=other", 1), oauthTarget() + "&client_id=other", oauthTarget() + "#fragment", "https://user@keymail.example/oauth/authorize", "https://keymail.example/other", strings.Replace(oauthTarget(), "S256", "plain", 1)} { |
| + w := httptest.NewRecorder() |
| + h := OAuthContinuation(configOrigin, func(w http.ResponseWriter, r *http.Request) { |
| + w.Header().Set("Set-Cookie", "private-cookie=private") |
| + http.Redirect(w, r, destination, 303) |
| + }, func(http.ResponseWriter, *http.Request, string) { t.Fatal("invalid redirect rendered") }) |
| + h(w, httptest.NewRequest("POST", configOrigin+"/signin", nil)) |
| + if w.Code != 502 || w.Header().Get("Location") != "" || len(w.Result().Cookies()) != 0 || strings.Contains(w.Body.String(), "private") { |
| + t.Fatal("unsafe destination accepted or leaked", w.Code) |
| + } |
| + } |
| +} |
diff --git a/connect/routes.go b/connect/routes.go| index 8c689aa..86baacf 100644 |
| --- a/connect/routes.go |
| +++ b/connect/routes.go |
| @@ -14,6 +14,7 @@ import ( |
| const LocalCompletePath = localLinkCompletePath |
| type RoutesConfig struct { |
| + LocalLandingPath string |
| Namespace, Origin, Issuer, SessionCookie string |
| FormPolicy string |
| DEK []byte |
| @@ -31,7 +32,7 @@ type Routes struct { |
| } |
| func NewRoutes(b *Bridge, c RoutesConfig) (*Routes, error) { |
| - if b == nil || c.ReadSession == nil || c.SignOut == nil || c.LocalSignin == nil || c.Protect == nil || c.Render == nil || c.Origin != b.origin || c.Issuer != b.issuer || c.SessionCookie != b.sessionCookie || c.Namespace != b.namespace || sha256.Sum256(c.DEK) != b.keyHash { |
| + if (c.LocalLandingPath != "" && !handoff.SafePath(c.LocalLandingPath)) || b == nil || c.ReadSession == nil || c.SignOut == nil || c.LocalSignin == nil || c.Protect == nil || c.Render == nil || c.Origin != b.origin || c.Issuer != b.issuer || c.SessionCookie != b.sessionCookie || c.Namespace != b.namespace || sha256.Sum256(c.DEK) != b.keyHash { |
| return nil, errors.New("home connect: invalid routes") |
| } |
| s, err := seal.New(c.DEK, c.Namespace) |
diff --git a/connect/templates/oauth.html b/connect/templates/oauth.html| new file mode 100644 |
| index 0000000..12aef7a |
| --- /dev/null |
| +++ b/connect/templates/oauth.html |
| @@ -0,0 +1,5 @@ |
| +{{define "home-oauth-head"}}<meta http-equiv="refresh" content="0;url={{.URL}}">{{end}} |
| +{{define "home-oauth"}} |
| +<header rst-page-header><div rst-page-header-titles><h1>{{.Title}}</h1></div></header> |
| +<div rst-box><a rst-btn="primary" href="{{.URL}}">{{.ActionLabel}}</a></div> |
| +{{end}} |