Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions internal/crypto/devices.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
58 changes: 58 additions & 0 deletions internal/crypto/devices_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
}
42 changes: 42 additions & 0 deletions internal/daemon/fleet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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{
Expand All @@ -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
Expand Down
67 changes: 66 additions & 1 deletion internal/daemon/fleet_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions internal/daemon/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 9 additions & 0 deletions internal/wire/control.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
13 changes: 13 additions & 0 deletions web/src/client/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
42 changes: 42 additions & 0 deletions web/src/routes/devices.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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' })])
Expand Down
Loading