Merge branch 'fix/g09' into fix/integrated

internal/ipc/api.go

@@ -362,6 +362,12 @@ type Speaker struct {
 	Name     string    `json:"name"`
 	Enrolled time.Time `json:"enrolled"`
 	Samples  int       `json:"samples"`
+	// Damaged — the stored row's metadata did not read back cleanly. The
+	// voiceprint is still there; the name, sample count or enrolment time is
+	// not trustworthy. A surface should say so rather than render a corrupt
+	// row as a profile enrolled from zero samples, which is what a real
+	// minimal enrolment looks like.
+	Damaged bool `json:"damaged,omitempty"`
 }
 
 // EnrollSpeakerResp — the profile that was written.
@@ -804,14 +810,22 @@ type DayPlan struct {
 }
 
 // storeEncryptionKeyReq — the passkey-derived secret used to wrap the store
-// encryption key at enrollment time. Called by mavweb after RegisterFinish.
+// encryption key. Called by mavweb after a verified assertion.
 //
 // Secret is the 32-byte WebAuthn PRF output, NOT the credential public key.
 // The field used to carry the public key and that was the bug: a public key
 // sits in passkeys.json next to the wrapped blob, so the blob protected
 // nothing. See internal/webauthn/keywrap.go.
+//
+// Explicit says the operator asked for the cold-start key to be written, as
+// opposed to it being a side effect of asserting a passkey. Without the flag
+// the daemon writes only when no blob exists yet. Rewriting on every assertion
+// is what let a page-level compromise substitute its own PRF value and have
+// the daemon re-wrap the real database key under it, and what let a second
+// authenticator silently replace the first one's blob.
 type storeEncryptionKeyReq struct {
-	Secret []byte `json:"secret"`
+	Secret   []byte `json:"secret"`
+	Explicit bool   `json:"explicit,omitempty"`
 }
 
 // unlockReq — the passkey-derived secret for unwrapping the store encryption

internal/ipc/client.go

@@ -413,9 +413,14 @@ func (c *Client) AssertStepUp(ctx context.Context) error {
 }
 
 // StoreEncryptionKey wraps the daemon's at-rest key under secret, the 32-byte
-// WebAuthn PRF output for the freshly enrolled credential.
-func (c *Client) StoreEncryptionKey(ctx context.Context, secret []byte) error {
-	return c.call(ctx, MethodStoreEncryptionKey, storeEncryptionKeyReq{Secret: secret}, nil)
+// WebAuthn PRF output for the asserted credential.
+//
+// explicit marks an operator-requested write. False means "write it only if
+// there is nothing there yet": a blob already on disk is left alone, because
+// rewriting it on every assertion is how an attacker-chosen PRF value, or a
+// second authenticator, replaces the one thing that opens the database.
+func (c *Client) StoreEncryptionKey(ctx context.Context, secret []byte, explicit bool) error {
+	return c.call(ctx, MethodStoreEncryptionKey, storeEncryptionKeyReq{Secret: secret, Explicit: explicit}, nil)
 }
 
 // Unlock hands the daemon the PRF secret so it can unwrap its at-rest key and

internal/ipc/server.go

@@ -529,7 +529,11 @@ type Server struct {
 
 // WrapKeyFunc — wraps the store encryption key under the passkey-derived
 // secret (a 32-byte WebAuthn PRF output) and persists the wrapped blob.
-type WrapKeyFunc func(ctx context.Context, secret []byte) error
+//
+// explicit distinguishes "the operator asked for the cold-start key to be
+// written" from "a passkey was asserted". Only the first may overwrite a blob
+// that is already there; see cmd/mavend/keyfile.go.
+type WrapKeyFunc func(ctx context.Context, secret []byte, explicit bool) error
 
 // UnlockFunc — unwraps the store encryption key using the passkey-derived
 // secret and completes daemon initialization.
@@ -996,7 +1000,7 @@ func (s *Server) dispatch(ctx context.Context, req Request) (json.RawMessage, er
 			if err := unmarshalParams(req.Params, &p); err != nil {
 				return nil, err
 			}
-			return marshalResult(nil), s.WrapKeyFn(ctx, p.Secret)
+			return marshalResult(nil), s.WrapKeyFn(ctx, p.Secret, p.Explicit)
 		}
 		return nil, fmt.Errorf("%w: %s", ErrUnknownMethod, req.Method)
 

internal/ipc/unlock_test.go

@@ -40,8 +40,13 @@ func TestUnlockDeliversSecretToHook(t *testing.T) {
 		secret[i] = byte(i + 1)
 	}
 	var gotUnlock, gotWrap []byte
+	var gotExplicit bool
 	srv.UnlockFn = func(_ context.Context, s []byte) error { gotUnlock = bytes.Clone(s); return nil }
-	srv.WrapKeyFn = func(_ context.Context, s []byte) error { gotWrap = bytes.Clone(s); return nil }
+	srv.WrapKeyFn = func(_ context.Context, s []byte, explicit bool) error {
+		gotWrap = bytes.Clone(s)
+		gotExplicit = explicit
+		return nil
+	}
 
 	ctx := context.Background()
 	if err := cli.Unlock(ctx, secret); err != nil {
@@ -50,12 +55,24 @@ func TestUnlockDeliversSecretToHook(t *testing.T) {
 	if !bytes.Equal(gotUnlock, secret) {
 		t.Errorf("UnlockFn got %x, want %x", gotUnlock, secret)
 	}
-	if err := cli.StoreEncryptionKey(ctx, secret); err != nil {
+	if err := cli.StoreEncryptionKey(ctx, secret, true); err != nil {
 		t.Fatalf("StoreEncryptionKey: %v", err)
 	}
 	if !bytes.Equal(gotWrap, secret) {
 		t.Errorf("WrapKeyFn got %x, want %x", gotWrap, secret)
 	}
+	// The explicit flag rides the same request. Without it the daemon cannot
+	// tell "he asked for the cold-start key to be rewritten" from "a passkey
+	// was asserted", and rewrites the blob on every step-up.
+	if !gotExplicit {
+		t.Error("WrapKeyFn got explicit=false, want the flag to cross the wire")
+	}
+	if err := cli.StoreEncryptionKey(ctx, secret, false); err != nil {
+		t.Fatalf("StoreEncryptionKey: %v", err)
+	}
+	if gotExplicit {
+		t.Error("WrapKeyFn got explicit=true for an implicit wrap")
+	}
 }
 
 // A refusal from the daemon hook — a wrong passkey, or no prior assertion —
@@ -78,7 +95,7 @@ func TestUnlockUnwiredIsUnknownMethod(t *testing.T) {
 	if err := cli.Unlock(ctx, bytes.Repeat([]byte{1}, 32)); err == nil {
 		t.Error("Unlock succeeded with no UnlockFn wired")
 	}
-	if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{1}, 32)); err == nil {
+	if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{1}, 32), false); err == nil {
 		t.Error("StoreEncryptionKey succeeded with no WrapKeyFn wired")
 	}
 }
@@ -100,7 +117,7 @@ func TestLockedCheckDefaultDenies(t *testing.T) {
 	unlocked := false
 	srv.UnlockFn = func(context.Context, []byte) error { unlocked = true; return nil }
 	srv.StepUp = func(context.Context) error { return nil }
-	srv.WrapKeyFn = func(context.Context, []byte) error { return nil }
+	srv.WrapKeyFn = func(context.Context, []byte, bool) error { return nil }
 
 	ctx := context.Background()
 	// A store method must be refused while locked.
@@ -108,7 +125,7 @@ func TestLockedCheckDefaultDenies(t *testing.T) {
 		t.Error("a store read went through while locked")
 	}
 	// Key wrapping is NOT on the allowlist: a locked daemon has no key to wrap.
-	if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{2}, 32)); err == nil {
+	if err := cli.StoreEncryptionKey(ctx, bytes.Repeat([]byte{2}, 32), false); err == nil {
 		t.Error("StoreEncryptionKey was allowed while locked")
 	}
 	// The unlock flow itself must still work.

internal/memory/store.go

@@ -7,6 +7,20 @@ import (
 	"sync"
 )
 
+// NonRecallPrefix — rows whose id starts with this are excluded from Search by
+// every backend. Speaker voiceprints live in the same vector table as notes and
+// facts (internal/speaker writes them under this prefix), and they are not
+// recall material: a voiceprint has no text to read back and surfacing one as a
+// note hit leaks a name attached to a biometric.
+//
+// Reading them through Catalog.ByPrefix was documented as what keeps them out
+// of recall. It is not. It controls how speaker code reads its own rows and
+// says nothing about what Search scores. What actually kept them out was that
+// cosine returns 0 on a width mismatch, so a 192-dim voiceprint scored 0
+// against a 384-dim query. That is a coincidence of two model choices — some
+// x-vector exports are 384-dim — and not an invariant. This is the invariant.
+const NonRecallPrefix = "speaker:"
+
 // Result is a single search hit.
 type Result struct {
 	ID    string
@@ -93,7 +107,14 @@ func (s *InMemoryStore) ByPrefix(_ context.Context, prefix string) ([]Record, er
 		if !strings.HasPrefix(it.id, prefix) {
 			continue
 		}
-		out = append(out, Record{ID: it.id, Vec: append([]float32(nil), it.vec...), Meta: it.meta})
+		// Copy the metadata too. Returning it.meta by reference let a caller
+		// mutating the returned map edit the stored row, and the persistent
+		// backend unmarshals fresh, so the two disagreed.
+		meta := make(map[string]string, len(it.meta))
+		for k, v := range it.meta {
+			meta[k] = v
+		}
+		out = append(out, Record{ID: it.id, Vec: append([]float32(nil), it.vec...), Meta: meta})
 	}
 	return out, nil
 }
@@ -127,6 +148,9 @@ func (s *InMemoryStore) Search(_ context.Context, vec []float32, topK int) ([]Re
 
 	scores := make([]scored, 0, len(s.items))
 	for _, it := range s.items {
+		if strings.HasPrefix(it.id, NonRecallPrefix) {
+			continue
+		}
 		score := cosine(vec, it.vec)
 		scores = append(scores, scored{id: it.id, score: score, meta: it.meta})
 	}

internal/memory/store_test.go

@@ -80,3 +80,75 @@ func TestCosineEdgeCases(t *testing.T) {
 		t.Errorf("dot(1,2;1,2) = %f, want 5", c)
 	}
 }
+
+// The in-memory backend has to hide voiceprints from recall exactly like the
+// persistent one, or a test passing here says nothing about the daemon.
+func TestInMemorySearchSkipsVoiceprints(t *testing.T) {
+	ctx := context.Background()
+	s := NewInMemoryStore()
+	if err := s.Insert(ctx, "note:1", []float32{0, 1}, map[string]string{"text": "заметка"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := s.Insert(ctx, NonRecallPrefix+"kami", []float32{1, 0}, map[string]string{"name": "Ками"}); err != nil {
+		t.Fatal(err)
+	}
+	got, err := s.Search(ctx, []float32{1, 0}, 10)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(got) != 1 || got[0].ID != "note:1" {
+		t.Fatalf("Search = %+v, want just the note", got)
+	}
+	recs, err := s.ByPrefix(ctx, NonRecallPrefix)
+	if err != nil || len(recs) != 1 {
+		t.Fatalf("ByPrefix = %+v, %v; want the voiceprint", recs, err)
+	}
+}
+
+// ByPrefix hands back a copy of the metadata. It used to return the stored map
+// by reference, so a caller editing a returned Record silently edited the row,
+// and the persistent backend did not behave that way.
+func TestInMemoryByPrefixCopiesMeta(t *testing.T) {
+	ctx := context.Background()
+	s := NewInMemoryStore()
+	if err := s.Insert(ctx, "speaker:kami", []float32{1, 0}, map[string]string{"name": "Ками"}); err != nil {
+		t.Fatal(err)
+	}
+	recs, err := s.ByPrefix(ctx, "speaker:")
+	if err != nil || len(recs) != 1 {
+		t.Fatalf("ByPrefix = %+v, %v", recs, err)
+	}
+	recs[0].Meta["name"] = "не Ками"
+
+	again, err := s.ByPrefix(ctx, "speaker:")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if again[0].Meta["name"] != "Ками" {
+		t.Errorf("the stored row was edited through the returned map: %q", again[0].Meta["name"])
+	}
+}
+
+// Insert upserts. This is not only a speaker-profile concern: every user of the
+// in-memory store used to accumulate a second row for a re-indexed id, and the
+// stale copy stayed searchable.
+func TestInMemoryInsertUpserts(t *testing.T) {
+	ctx := context.Background()
+	s := NewInMemoryStore()
+	if err := s.Insert(ctx, "note:1", []float32{1, 0}, map[string]string{"text": "старое"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := s.Insert(ctx, "note:1", []float32{0, 1}, map[string]string{"text": "новое"}); err != nil {
+		t.Fatal(err)
+	}
+	got, err := s.Search(ctx, []float32{1, 0}, 10)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(got) != 1 {
+		t.Fatalf("Search returned %d rows, want 1 (the old copy is still searchable)", len(got))
+	}
+	if got[0].Meta["text"] != "новое" {
+		t.Errorf("row = %q, want the replacement", got[0].Meta["text"])
+	}
+}

internal/speaker/recognizer.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strconv"
 	"time"
 
 	"github.com/kami/maven/internal/audio"
@@ -140,14 +141,18 @@ func (r *Recognizer) Get(ctx context.Context, id string) (Profile, error) {
 // Forget deletes a profile. This is the one operation that must always work:
 // a voiceprint is data about a person, and "перестань узнавать её" has to
 // actually remove it, not mark it inactive.
+//
+// Forgetting a profile that is not there is not an error, matching
+// memory.Catalog.Delete. It used to read the row first and answer ErrNotFound,
+// which meant the layer documented as the one that must always work was the
+// layer reintroducing a failure: a surface retrying a forget after a partial
+// failure got an error on the second try, for a voiceprint that was already
+// gone. The caller asked for it to be gone and it is gone.
 func (r *Recognizer) Forget(ctx context.Context, id string) error {
 	id = NormalizeID(id)
 	if !ValidID(id) {
 		return fmt.Errorf("%w: %q", ErrBadID, id)
 	}
-	if _, err := r.Get(ctx, id); err != nil {
-		return err
-	}
 	if err := r.cat.Delete(ctx, Prefix+id); err != nil {
 		return fmt.Errorf("speaker: forget %q: %w", id, err)
 	}
@@ -170,6 +175,12 @@ func (r *Recognizer) embed(ctx context.Context, a audio.Audio) ([]float32, error
 // profileFromRecord reads a stored row back into a Profile. A row with
 // unreadable metadata still yields a usable voiceprint — the vector is the part
 // that matters, and losing a name should not lose the enrolment.
+//
+// It also says when the metadata did not read cleanly. Without that, a row
+// whose samples count is "12x" and whose name is missing came back as a
+// plausible profile called by its own id with 0 samples, which is exactly what
+// a real minimal enrolment looks like. Damaged is the difference between "he
+// enrolled badly" and "this row is broken".
 func profileFromRecord(rec memory.Record) Profile {
 	p := Profile{
 		ID:   trimPrefix(rec.ID),
@@ -178,15 +189,24 @@ func profileFromRecord(rec memory.Record) Profile {
 		Name: rec.Meta["name"],
 	}
 	if s := rec.Meta["samples"]; s != "" {
-		p.Samples = atoi(s)
+		n, err := strconv.Atoi(s)
+		if err != nil || n < 0 {
+			p.Damaged = true
+		} else {
+			p.Samples = n
+		}
 	}
 	if ts := rec.Meta["enrolled"]; ts != "" {
-		if t, err := time.Parse(time.RFC3339, ts); err == nil {
+		t, err := time.Parse(time.RFC3339, ts)
+		if err != nil {
+			p.Damaged = true
+		} else {
 			p.Enrolled = t
 		}
 	}
 	if p.Name == "" {
 		p.Name = p.ID
+		p.Damaged = true
 	}
 	return p
 }
@@ -197,16 +217,3 @@ func trimPrefix(id string) string {
 	}
 	return id
 }
-
-// atoi is a tolerant small-integer parse: metadata that is not a number reads
-// as 0 rather than failing the whole listing.
-func atoi(s string) int {
-	n := 0
-	for _, r := range s {
-		if r < '0' || r > '9' {
-			return 0
-		}
-		n = n*10 + int(r-'0')
-	}
-	return n
-}

internal/speaker/speaker.go

@@ -31,6 +31,14 @@
 // There are also no enrolment samples. So Recognizer runs against Disabled and
 // every Identify answers ErrDisabled until a model lands.
 //
+// "Recognition is blocked but enrolment works" is not true and this package
+// used to imply it. Enroll embeds every sample before it stores anything
+// (enroll.go, "Embed first, store second"), so with no model it fails on the
+// first sample with ErrDisabled and nothing is ever stored. List then returns
+// an empty list forever and Forget has nothing to delete. Without an embedder
+// all three operations are no-ops, and mavend does not wire the wire methods
+// at all in that state.
+//
 // The MFCC + GMM "simplest floor" in the plan document is refused rather than
 // deferred. A hand-rolled spectral distance would identify people confidently
 // and wrongly, and its output would be written into facts as "Ками said this".
@@ -46,11 +54,15 @@ import (
 	"time"
 
 	"github.com/kami/maven/internal/audio"
+	"github.com/kami/maven/internal/memory"
 )
 
 // Prefix — the id prefix speaker profiles carry in the shared vector table.
-// It is what ByPrefix enumerates and what keeps voiceprints out of note recall.
-const Prefix = "speaker:"
+// It is what ByPrefix enumerates, and what every Store implementation filters
+// out of Search so note recall cannot rank a voiceprint. The constant lives in
+// internal/memory because the store layer has to know it and cannot import this
+// package.
+const Prefix = memory.NonRecallPrefix
 
 // DefaultThreshold — cosine similarity a match must beat to be a match.
 //
@@ -133,6 +145,12 @@ type Profile struct {
 	Samples  int       `json:"samples"`
 	Dim      int       `json:"dim"`
 
+	// Damaged marks a row whose stored metadata did not read back cleanly —
+	// an unparsable sample count or timestamp, or a missing name. The
+	// voiceprint is still usable, but the row should be listed as damaged
+	// rather than as a plausible profile enrolled from nothing.
+	Damaged bool `json:"damaged,omitempty"`
+
 	// Vec is the voiceprint. Not serialised to any surface: a listing tells him
 	// who is enrolled, it does not hand out the biometric itself.
 	Vec []float32 `json:"-"`

internal/speaker/speaker_test.go

@@ -269,8 +269,58 @@ func TestForgetRemovesTheVoiceprint(t *testing.T) {
 	if _, err := r.Get(ctx, "guest"); !errors.Is(err, ErrNotFound) {
 		t.Errorf("Get after Forget = %v, want ErrNotFound", err)
 	}
-	if err := r.Forget(ctx, "guest"); !errors.Is(err, ErrNotFound) {
-		t.Errorf("second Forget = %v, want ErrNotFound", err)
+	// Forgetting twice is not an error. A surface retrying after a partial
+	// failure must not be told the voice it asked to remove is missing.
+	if err := r.Forget(ctx, "guest"); err != nil {
+		t.Errorf("second Forget = %v, want nil", err)
+	}
+}
+
+// A row whose metadata is corrupt lists as damaged, not as a plausible profile
+// enrolled from zero samples. Those two used to look identical.
+func TestCorruptMetadataListsAsDamaged(t *testing.T) {
+	r, cat := newRec(t, &fakeEmbedder{vec: []float32{1, 0}})
+	ctx := context.Background()
+	if err := cat.Insert(ctx, Prefix+"kami", []float32{1, 0}, map[string]string{
+		"kind":     "speaker",
+		"samples":  "12x",
+		"enrolled": "yesterday",
+	}); err != nil {
+		t.Fatal(err)
+	}
+	ps, err := r.List(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ps) != 1 {
+		t.Fatalf("List returned %d profiles, want 1", len(ps))
+	}
+	p := ps[0]
+	if !p.Damaged {
+		t.Errorf("profile %+v is not marked damaged", p)
+	}
+	if p.Samples != 0 || !p.Enrolled.IsZero() {
+		t.Errorf("unreadable metadata was parsed anyway: samples %d, enrolled %v", p.Samples, p.Enrolled)
+	}
+	if len(p.Vec) != 2 {
+		t.Errorf("the voiceprint was dropped: %v", p.Vec)
+	}
+}
+
+// A clean row is not damaged. Without this the flag could be set always and
+// the test above would still pass.
+func TestCleanProfileIsNotDamaged(t *testing.T) {
+	r, _ := newRec(t, &fakeEmbedder{vec: []float32{1, 0}})
+	ctx := context.Background()
+	if _, err := r.Enroll(ctx, "kami", "Ками", enrolSamples(3, 4)); err != nil {
+		t.Fatal(err)
+	}
+	ps, err := r.List(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(ps) != 1 || ps[0].Damaged {
+		t.Fatalf("List = %+v, want one undamaged profile", ps)
 	}
 }
 

internal/store/memory.go

@@ -64,11 +64,17 @@ func (m *MemoryStore) Insert(ctx context.Context, id string, vec []float32, meta
 
 // Search returns the topK nearest rows by cosine similarity. A full scan; see
 // the type doc for why that's fine at this scale.
+//
+// Rows under memory.NonRecallPrefix are excluded in SQL. They are speaker
+// voiceprints sharing this table, and note recall must not rank them; see that
+// constant for why the previous arrangement only appeared to do this.
 func (m *MemoryStore) Search(ctx context.Context, vec []float32, topK int) ([]memory.Result, error) {
 	if topK <= 0 {
 		topK = 10
 	}
-	rows, err := m.db.QueryContext(ctx, `SELECT id, vec, meta FROM memory_vectors`)
+	rows, err := m.db.QueryContext(ctx,
+		`SELECT id, vec, meta FROM memory_vectors WHERE id NOT LIKE ? ESCAPE '\'`,
+		escapeLike(memory.NonRecallPrefix)+"%")
 	if err != nil {
 		return nil, fmt.Errorf("memory: scan: %w", err)
 	}

internal/store/memory_test.go

@@ -3,7 +3,10 @@ package store
 import (
 	"context"
 	"path/filepath"
+	"strings"
 	"testing"
+
+	"github.com/kami/maven/internal/memory"
 )
 
 func newMemTestStore(t *testing.T) *Store {
@@ -101,3 +104,38 @@ func TestMemoryStorePersistsAcrossReopen(t *testing.T) {
 		t.Fatalf("memory did not survive reopen: %v", got)
 	}
 }
+
+// A voiceprint sharing the vector table must never rank as a note hit. The
+// dimensions match here on purpose: what used to hide these rows was cosine
+// returning 0 on a width mismatch, which is a property of two model choices and
+// not of the store.
+func TestMemoryStoreSearchSkipsVoiceprints(t *testing.T) {
+	ctx := context.Background()
+	m := newMemTestStore(t).VectorMemory()
+
+	if err := m.Insert(ctx, "note:1", []float32{0, 1, 0}, map[string]string{"text": "заметка"}); err != nil {
+		t.Fatal(err)
+	}
+	if err := m.Insert(ctx, memory.NonRecallPrefix+"kami", []float32{1, 0, 0}, map[string]string{"name": "Ками"}); err != nil {
+		t.Fatal(err)
+	}
+
+	got, err := m.Search(ctx, []float32{1, 0, 0}, 10)
+	if err != nil {
+		t.Fatalf("Search: %v", err)
+	}
+	for _, r := range got {
+		if strings.HasPrefix(r.ID, memory.NonRecallPrefix) {
+			t.Fatalf("recall returned a voiceprint: %+v", r)
+		}
+	}
+	if len(got) != 1 || got[0].ID != "note:1" {
+		t.Fatalf("Search = %+v, want just the note", got)
+	}
+
+	// The row is still there for the speaker code that owns it.
+	recs, err := m.ByPrefix(ctx, memory.NonRecallPrefix)
+	if err != nil || len(recs) != 1 {
+		t.Fatalf("ByPrefix = %+v, %v; want the voiceprint", recs, err)
+	}
+}

internal/webauthn/keywrap.go

@@ -24,7 +24,18 @@
 //
 // v1 blobs are still readable, so an existing deployment opens and can be
 // re-wrapped, and UnwrapKey reports which format it read so the caller can
-// say so out loud. Nothing writes v1 any more.
+// say so out loud. Nothing writes v1 any more. Reading one needs the
+// credential public key, which only cmd/mavweb still has: its assertion
+// handler retries a failed PRF unwrap with it, because otherwise a box
+// enrolled before v2 could never cold-start again.
+//
+// # What the wrapped blob's security rests on
+//
+// The PRF secret is stable for the lifetime of the credential and it reaches
+// mavend inside an HTTP request body. Unlike a signature it does not expire.
+// One copy in a proxy log, a devtools HAR, or a crash dump is permanent
+// offline access to whatever this blob wraps. Nothing on this path may log the
+// secret, and nothing does.
 //
 // # Blob format
 //
@@ -171,8 +182,16 @@ func unwrap(body, secret []byte, info string, aad []byte, wantSecretLen int) ([]
 	if len(body) < saltLen+nonceLen+1 {
 		return nil, fmt.Errorf("%w: blob too short (%d)", ErrKeyUnwrap, len(body))
 	}
-	if wantSecretLen > 0 && len(secret) != wantSecretLen {
-		return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, ErrSecretLen)
+	// The v2 side is held to exactly what WrapKey demands, all-zero included.
+	// Letting the two ends disagree about what a valid secret is would leave
+	// a blob that can be opened by material that could never have sealed it.
+	if wantSecretLen > 0 {
+		if len(secret) != wantSecretLen {
+			return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, ErrSecretLen)
+		}
+		if err := checkSecret(secret); err != nil {
+			return nil, fmt.Errorf("%w: %v", ErrKeyUnwrap, err)
+		}
 	}
 
 	salt := body[:saltLen]

internal/webauthn/keywrap_test.go

@@ -229,3 +229,18 @@ func TestUnwrapRejectsOversizeBlob(t *testing.T) {
 		t.Fatalf("err = %v, want ErrBlobTooLong", err)
 	}
 }
+
+// WrapKey refuses an all-zero secret because a blob wrapped under one is a
+// blob anyone can open. The v2 unwrap side must refuse it for the same reason:
+// if the two ends disagree about what a valid secret is, a blob can be opened
+// by material that could never have sealed it.
+func TestUnwrapV2RefusesAnAllZeroSecret(t *testing.T) {
+	key := bytes.Repeat([]byte{1}, 32)
+	blob, err := WrapKey(key, bytes.Repeat([]byte{2}, 32))
+	if err != nil {
+		t.Fatalf("WrapKey: %v", err)
+	}
+	if _, _, err := UnwrapKey(blob, make([]byte, 32)); !errors.Is(err, ErrKeyUnwrap) {
+		t.Fatalf("UnwrapKey with an all-zero secret = %v, want refusal", err)
+	}
+}