diff --git a/internal/crypto/devices.go b/internal/crypto/devices.go index b02f8e7..425a8a8 100644 --- a/internal/crypto/devices.go +++ b/internal/crypto/devices.go @@ -29,6 +29,17 @@ type Device struct { // fleet directory will publish it — and empty for devices paired // before the fleet key existed. Cert []byte `json:"cert,omitempty"` + + // Origin is where this device first reached this machine from — the + // Origin header on its enrolment POST, e.g. "http://localhost:7719". + // A hint for telling otherwise-identical rows apart, never an identity: + // it is whatever the browser sent, it is only known for devices that + // enrolled over loopback, and empty is the ordinary state of every + // other row — one paired by ceremony, one admitted on a fleet cert, or + // one from before origins were recorded. First seen wins (SetOrigin), + // and it stays out of the certificate: it is a fact about how this + // machine was reached, not about the device. + Origin string `json:"origin,omitempty"` } // DeviceID derives the identity from the key itself, so an entry cannot @@ -265,6 +276,45 @@ func (s *DeviceStore) SetCert(publicKey, cert []byte) (bool, error) { return false, nil } +// SetOrigin records where a device enrolled from, once, reporting whether it +// wrote. A row that already has an origin keeps it — first seen wins, so a +// key that reaches this daemon from more than one place is labelled by where +// it turned up rather than by wherever its tab last loaded — and an empty +// origin, an unregistered key or a repeat are quiet no-ops, because the +// enrolment endpoint calls this on every page load. +// +// No revocation check, unlike SetCert and Relabel, and the asymmetry is the +// point: those write a credential, this writes a display hint. Revoking +// removes the row itself, so a revoked key has nothing here to stamp. +func (s *DeviceStore) SetOrigin(publicKey []byte, origin string) (bool, error) { + if len(publicKey) != 32 { + return false, fmt.Errorf("crypto: device key must be 32 bytes, got %d", len(publicKey)) + } + if origin == "" { + return false, nil + } + s.mu.Lock() + defer s.mu.Unlock() + devices, err := s.load() + if err != nil { + return false, err + } + for i := range devices { + if !bytes.Equal(devices[i].PublicKey, publicKey) { + continue + } + if devices[i].Origin != "" { + return false, nil + } + devices[i].Origin = origin + if err := s.save(devices); err != nil { + return false, err + } + return true, nil + } + return false, nil +} + // Relabel renames a device and replaces its certificate together, reporting // whether anything changed. Missing key, empty label or empty cert: no change, // no error. diff --git a/internal/crypto/devices_test.go b/internal/crypto/devices_test.go index 8d7a704..e519e4c 100644 --- a/internal/crypto/devices_test.go +++ b/internal/crypto/devices_test.go @@ -483,3 +483,61 @@ func TestRemoveByKeyComparesTheWholeKey(t *testing.T) { t.Fatalf("RemoveByKey of an absent key = %v, %v; want false and no error", ok, err) } } + +// TestSetOriginKeepsTheFirstOrigin: the origin is a first-seen hint, written +// once and never replaced. One device key can legitimately reach this daemon +// from more than one place — the vite dev server today, loopback tomorrow — +// and a field that tracked the latest would make every row read "wherever the +// tab last loaded from", which answers nothing at the moment of revoking. +func TestSetOriginKeepsTheFirstOrigin(t *testing.T) { + dir := t.TempDir() + s := NewDeviceStore(dir) + + key := testKey(t) + if _, err := s.Add("phone", key, nil); err != nil { + t.Fatalf("Add: %v", err) + } + + wrote, err := s.SetOrigin(key, "http://127.0.0.1:7719") + if err != nil || !wrote { + t.Fatalf("SetOrigin on a device with none = %v, %v; want it written", wrote, err) + } + dev, ok, err := s.FindByKey(key) + if err != nil || !ok { + t.Fatalf("FindByKey = %v, %v", ok, err) + } + if dev.Origin != "http://127.0.0.1:7719" { + t.Fatalf("stored origin = %q, want the one just written", dev.Origin) + } + + // A different origin later changes nothing: first seen wins. + wrote, err = s.SetOrigin(key, "http://localhost:7719") + if err != nil || wrote { + t.Fatalf("SetOrigin over an existing origin = %v, %v; want it refused", wrote, err) + } + dev, _, _ = s.FindByKey(key) + if dev.Origin != "http://127.0.0.1:7719" { + t.Fatalf("stored origin = %q; an existing origin was replaced", dev.Origin) + } + + // An empty origin records nothing — it is the ordinary state of a device + // that never enrolled over loopback, not a value. + fresh := testKey(t) + if _, err := s.Add("laptop", fresh, nil); err != nil { + t.Fatalf("Add: %v", err) + } + if wrote, err := s.SetOrigin(fresh, ""); err != nil || wrote { + t.Fatalf("SetOrigin with an empty origin = %v, %v; want a no-op", wrote, err) + } + + // A key nobody paired gets nothing, and no error: the caller's question is + // "did this stamp a row", and for a row that is not there the answer is no. + if wrote, err := s.SetOrigin(testKey(t), "http://127.0.0.1:7719"); err != nil || wrote { + t.Fatalf("SetOrigin for an unregistered key = %v, %v; want a no-op", wrote, err) + } + + // A malformed key is an error, as it is on every other mutation here. + if _, err := s.SetOrigin(key[:16], "http://127.0.0.1:7719"); err == nil { + t.Fatal("SetOrigin with a short key = nil, want an error") + } +} diff --git a/internal/daemon/fleet.go b/internal/daemon/fleet.go index 5c212ce..d465761 100644 --- a/internal/daemon/fleet.go +++ b/internal/daemon/fleet.go @@ -203,6 +203,29 @@ const EnrolPath = "/api/fleet/enrol" // allows for a strictly larger body. const maxEnrolBytes = 4 << 10 +// maxOriginBytes bounds what enrolOrigin will record. A real Origin is a +// scheme, a host and a port; 256 covers any of this daemon's own with room +// to spare. +const maxOriginBytes = 256 + +// enrolOrigin is what enrolment records about where the browser is: the +// request's Origin header, or nothing worth keeping. +// +// It is trusted exactly as far as a hint on a Devices row needs to be, and no +// further — recorded on the local registry row, shown beside the label, never +// minted into the certificate. By the time this runs, provenance +// (checkProvenance) has already refused any Origin that is not one of this +// daemon's own, so the value names which of the daemon's spellings the tab is +// on: 127.0.0.1 or localhost. "null" is the opaque origin — a browser +// declining to say — which is the same fact as no header at all. +func enrolOrigin(r *http.Request) string { + o := r.Header.Get("Origin") + if o == "null" || len(o) > maxOriginBytes { + return "" + } + return o +} + // enrolRequest is what the loopback UI posts: its own device public key, and // nothing else. // @@ -464,6 +487,19 @@ func (s *Server) handleEnrol(w http.ResponseWriter, r *http.Request) { if fresh, ok := s.relabelEnrolled(dev, key, fi); ok { cert = fresh } + // And the row's origin, back-filled when it never had one. This is + // the path every pre-origin row takes on its next page load — the + // browser calls this endpoint on every load — so older rows come + // right on their own rather than being singled out, and a row that + // has an origin keeps it (SetOrigin, first seen wins). Best effort + // like the relabel above; the broadcast is because nothing else + // pushes a list for this, and the screens already looking should not + // hold a row the registry has just outgrown. + if wrote, err := s.identity.Devices.SetOrigin(key, enrolOrigin(r)); err != nil { + s.logger().Warn("could not record this browser's origin", "device", dev.ID, "err", err) + } else if wrote { + s.broadcastDeviceList() + } } else { label := enrolLabel(s.hostname) cert, err = fi.Key.Sign(fleet.DeviceCert{ @@ -485,6 +521,12 @@ func (s *Server) handleEnrol(w http.ResponseWriter, r *http.Request) { } s.logger().Info("enrolled this machine's own browser as a fleet device", "device", dev.ID, "label", dev.Label, "pairedOn", fi.MachineID) + // Where the browser came from, stamped before the broadcast below + // announces the row. Best effort: an enrolment holding a usable + // certificate must not fail over a display hint. + if _, err := s.identity.Devices.SetOrigin(key, enrolOrigin(r)); err != nil { + s.logger().Warn("could not record this browser's origin", "device", dev.ID, "err", err) + } // Nothing is published to the fleet directory, exactly as the pairing // ceremony publishes nothing: device certificates go to the device they // are about and nowhere else (spec/fleet-trust.md, "Device certificates diff --git a/internal/daemon/fleet_test.go b/internal/daemon/fleet_test.go index a3a52e6..f42ab0f 100644 --- a/internal/daemon/fleet_test.go +++ b/internal/daemon/fleet_test.go @@ -224,6 +224,14 @@ func newEnrolServer(t *testing.T, machineID string) (*httptest.Server, *Server, // postEnrol is the request the loopback UI makes: a POST from this daemon's // own origin, authenticated by the session cookie, carrying one key. func postEnrol(t *testing.T, ts *httptest.Server, publicKey []byte) *http.Response { + t.Helper() + return postEnrolFrom(t, ts, publicKey, ts.URL) +} + +// postEnrolFrom is postEnrol from a chosen origin. The daemon owns two +// (127.0.0.1 and localhost, same port), and which of them the tab is on is +// exactly the fact enrolment records. +func postEnrolFrom(t *testing.T, ts *httptest.Server, publicKey []byte, origin string) *http.Response { t.Helper() body, err := json.Marshal(map[string]string{ "publicKey": base64.StdEncoding.EncodeToString(publicKey), @@ -237,7 +245,7 @@ func postEnrol(t *testing.T, ts *httptest.Server, publicKey []byte) *http.Respon } req.Header.Set("Content-Type", "application/json") req.AddCookie(&http.Cookie{Name: tsCookie(ts), Value: tok}) - req.Header.Set("Origin", ts.URL) + req.Header.Set("Origin", origin) req.Header.Set("Sec-Fetch-Site", "same-origin") resp, err := http.DefaultClient.Do(req) if err != nil { @@ -635,6 +643,63 @@ func TestEnrolBackfillsACertificateADeviceHasNone(t *testing.T) { } } +// TestEnrolRecordsWhereTheBrowserCameFrom. +// +// Several loopback tabs enrol as several devices — IndexedDB is scoped to the +// origin, so http://127.0.0.1:7719 and http://localhost:7719 hold different +// keys — and their rows used to be identical: same label, same machine, +// nothing but dates. The Origin header on the enrolment POST is the one fact +// that differs, the daemon already has it, and nothing new is asked for or +// trusted: it is recorded as a hint on the local row, never minted into the +// certificate. +func TestEnrolRecordsWhereTheBrowserCameFrom(t *testing.T) { + ts, srv, _ := newEnrolServer(t, "karns-mbp-a1b2-0f9a12cd") + key := deviceKey(0x5c) + + enrolOK(t, ts, key) + list := devices(t, srv) + if len(list) != 1 || list[0].Origin != ts.URL { + t.Fatalf("registry = %+v, want one row with origin %q", list, ts.URL) + } + + // The wire carries it, so a Devices screen can put it on the row. + wl, err := srv.deviceList() + if err != nil { + t.Fatalf("deviceList: %v", err) + } + if len(wl.Devices) != 1 || wl.Devices[0].Origin != ts.URL { + t.Fatalf("deviceList = %+v, want the enrolment origin on the row", wl.Devices) + } + + // A later load does not rewrite history: the row keeps the origin it + // enrolled from, whichever of the daemon's origins the tab is on today. + other := strings.Replace(ts.URL, "127.0.0.1", "localhost", 1) + if resp := postEnrolFrom(t, ts, key, other); resp.StatusCode != http.StatusOK { + t.Fatalf("re-enrolment from %q = %d, want 200", other, resp.StatusCode) + } + if list := devices(t, srv); list[0].Origin != ts.URL { + t.Errorf("origin after a re-enrolment = %q, want the first one %q", list[0].Origin, ts.URL) + } + + // A row from before origins were recorded picks one up on its next load — + // the browser calls this endpoint on every load, so the backfill is free + // and older rows are never singled out. + older := deviceKey(0x5d) + if _, err := srv.identity.Devices.Add("an older enrolment", older, nil); err != nil { + t.Fatalf("Add: %v", err) + } + if resp := postEnrolFrom(t, ts, older, other); resp.StatusCode != http.StatusOK { + t.Fatalf("enrolling the older row = %d, want 200", resp.StatusCode) + } + dev, ok, err := srv.identity.Devices.FindByKey(older) + if err != nil || !ok { + t.Fatalf("FindByKey = %v, %v", ok, err) + } + if dev.Origin != other { + t.Errorf("older row's origin = %q, want it back-filled to %q", dev.Origin, other) + } +} + // TestDeviceListSaysWhichMachinePairedEachRow. // // A machine on a fleet admits devices two ways — the ones it paired itself, and diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 20c106d..90e7e90 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -1487,6 +1487,7 @@ func (s *Server) deviceList() (wire.DeviceList, error) { PairedAt: d.PairedAt.Unix(), LastSeen: d.LastSeen.Unix(), PairedOn: pairedOn(fleetPub, d.Cert), + Origin: d.Origin, }) } return wire.DeviceList{Devices: infos}, nil diff --git a/internal/wire/control.go b/internal/wire/control.go index dc64cb0..80ec44e 100644 --- a/internal/wire/control.go +++ b/internal/wire/control.go @@ -524,6 +524,15 @@ type DeviceInfo struct { // own — the conservative direction, since it is the one that does not offer // a fleet-wide revoke on the strength of a blob that proved nothing. PairedOn string `json:"pairedOn,omitempty"` + // Origin is where the device first reached this machine from — the Origin + // header on its enrolment POST, recorded on the local row and never minted + // into the certificate (crypto.Device). It exists because several loopback + // tabs enrol as several devices, one per browser origin, and their rows + // are otherwise identical. A hint, not an identity: absent is the ordinary + // state of a device paired by ceremony, admitted on a fleet cert, or + // enrolled before origins were recorded, and a screen must render absent + // as unremarkable rather than suspicious. + Origin string `json:"origin,omitempty"` } // DeviceList answers devices, and follows a revoke that succeeded. diff --git a/web/src/client/protocol.ts b/web/src/client/protocol.ts index 7cac2c7..dd492ba 100644 --- a/web/src/client/protocol.ts +++ b/web/src/client/protocol.ts @@ -138,6 +138,19 @@ export interface DeviceInfo { * revoke on the strength of a blob that proved nothing. */ pairedOn?: string + /** + * Where the device first reached the daemon from — the Origin header on its + * enrolment POST, e.g. "http://localhost:7719" — recorded on the daemon's + * local row and never minted into the certificate. + * + * It exists because several loopback tabs enrol as several devices, one per + * browser origin (IndexedDB scopes the device key to the origin), and their + * rows are otherwise identical. A hint, not an identity: absent is the + * ordinary state of a device paired by ceremony, admitted on a fleet cert, + * or enrolled before origins were recorded, and a screen must render absent + * as unremarkable rather than suspicious. + */ + origin?: string } // Client -> server. diff --git a/web/src/routes/devices.test.tsx b/web/src/routes/devices.test.tsx index 8e490a6..ec1e03c 100644 --- a/web/src/routes/devices.test.tsx +++ b/web/src/routes/devices.test.tsx @@ -187,6 +187,48 @@ describe('DevicesRoute', () => { expect(screen.getByText('2h ago')).toBeTruthy() }) + /** + * The indistinguishable-rows fix (issue #71): several loopback tabs enrol as + * several devices — one per browser origin — and their rows read identically: + * same label, same machine, nothing but dates. The daemon now records the + * origin the enrolment came from and every row carries a short fingerprint of + * its key, so four rows saying "Browser on mb.local" can finally be told + * apart. + */ + it('tells identically-labelled rows apart by origin and fingerprint', async () => { + const { sock } = await mountDevices() + + listed(sock, [ + device({ id: 'aa11bb22cc33', label: 'Browser on mb.local', origin: 'http://127.0.0.1:7719' }), + device({ id: 'dd44ee55ff66', label: 'Browser on mb.local', origin: 'http://localhost:7719' }), + ]) + + // The origin reads as a place, not a URL: the scheme says nothing a reader + // of this list acts on, and the host and port are the whole distinction. + expect(screen.getByText('127.0.0.1:7719')).toBeTruthy() + expect(screen.getByText('localhost:7719')).toBeTruthy() + expect(screen.queryByText('http://127.0.0.1:7719')).toBeNull() + + // The fingerprint is the id's first six characters — the id is already a + // digest of the device key, so nothing new is derived here. + expect(screen.getByText('aa11bb')).toBeTruthy() + expect(screen.getByText('dd44ee')).toBeTruthy() + }) + + it('renders a row with no origin as ordinary, not as suspect', async () => { + const { sock } = await mountDevices() + + // A phone paired by QR has no meaningful origin, and every row from + // before origins were recorded has none either. Both are the ordinary + // state: the row shows its dates and its fingerprint and says nothing + // about what it does not know. + listed(sock, [device({ id: 'aa11bb22cc33', label: 'iPhone' })]) + + expect(screen.getByText('iPhone')).toBeTruthy() + expect(screen.getByText('aa11bb')).toBeTruthy() + expect(screen.queryByText(/unknown/i)).toBeNull() + }) + it('asks before it revokes', async () => { const { sock } = await mountDevices() listed(sock, [device({ id: 'aa11bb22cc33', label: 'iPhone' })]) diff --git a/web/src/routes/devices.tsx b/web/src/routes/devices.tsx index 8a70d03..24b7901 100644 --- a/web/src/routes/devices.tsx +++ b/web/src/routes/devices.tsx @@ -165,6 +165,35 @@ const META_TEXT = 'text-xs/6 whitespace-nowrap tabular-nums text-zinc-500 dark:t /** The quiet per-row control, at rest and while it is armed. */ const ROW_BUTTON = 'text-zinc-500 dark:text-zinc-400' +/** + * How much of a device id a row shows as its fingerprint. + * + * The id is already a digest of the device key, so the row derives nothing + * new — it prints the first six characters, which is enough to tell apart the + * handful of rows one machine holds while staying narrow enough to sit beside + * a label on a phone. It exists because rows can otherwise be identical to + * the word (see the origin note below), and it is on every row, because a row + * on a sibling machine has no origin to be told apart by. + */ +const FINGERPRINT_CHARS = 6 + +/** + * The origin a device enrolled from, said as a place rather than a URL. + * + * The daemon records the Origin header of the enrolment POST, so several + * loopback tabs — each a device of its own, because the device key lives in + * storage the browser scopes per origin — stop reading identically. The + * scheme is dropped because it distinguishes nothing a reader of this list + * acts on; the host and port are the whole difference. Null for a row with no + * origin, which is ordinary rather than suspect — a phone paired by QR, a + * device admitted on the fleet's word, or a row from before origins were + * recorded — and the row simply says nothing about what it does not know. + */ +function enrolledFrom(origin?: string): string | null { + if (origin === undefined || origin === '') return null + return origin.replace(/^[a-z][a-z0-9+.-]*:\/\//i, '') +} + /** * The paired devices, one row each, with the revoke each row carries. * @@ -382,8 +411,20 @@ function DeviceRow({ }) { return (
  • - - {d.label} + {/* + The label and its fingerprint share the flexible slot, so the + fingerprint sits beside the words it disambiguates rather than drifting + into the dates on the right. The label is what truncates; six characters + of digest are the one part of the row that must never be the part that + goes. + */} + + + {d.label} + + + {d.id.slice(0, FINGERPRINT_CHARS)} + {/* Said in the row rather than left to the label, because the label is @@ -397,11 +438,16 @@ function DeviceRow({ )} {/* - Paired first, last seen second, reading right — and the first of - the two is what goes at narrow widths. Which was seen when is the - live fact, the one somebody checks before revoking; when it was - paired is history, and history is what a phone can do without. + Where it enrolled from, then paired, then last seen, reading right — + and the first two are what go at narrow widths. Which was seen when is + the live fact, the one somebody checks before revoking; where a tab + enrolled and when it paired are history, and history is what a phone + can do without — the rows a phone most needs to tell apart are its + siblings with no origin at all, which the fingerprint covers. */} + {enrolledFrom(d.origin) !== null && ( + {enrolledFrom(d.origin)} + )} paired {ago(d.pairedAt)} {ago(d.lastSeen)} {!revocable ? null : armed ? (