diff --git a/Makefile b/Makefile index 1486a70..c3e8d7b 100644 --- a/Makefile +++ b/Makefile @@ -149,8 +149,13 @@ eval-models: # stt-fixtures — regenerate the golden STT audio in cmd/mavsttd/testdata from # the piper voices (#288). The committed WAVs are synthesised, never recorded, -# so this is the only way they should ever change. TestGoldenAudioTranscription -# then scores them against ggml-small; it self-skips when the model is absent. +# so this is the only way they should ever change. The spoken text is read out +# of testdata/golden_v1.json, so edit the transcript there and rerun this. +# +# test-stt-golden runs both golden tests: TestGoldenAudioTranscription, which +# scores the fixtures against ggml-small and self-skips when the model is +# absent, and TestGoldenFixturesAreCanonical, which checks the committed audio +# and the manifest with no model at all. stt-fixtures: ./scripts/gen-stt-fixtures.sh diff --git a/cmd/mavend/keyfile.go b/cmd/mavend/keyfile.go new file mode 100644 index 0000000..15f9ecb --- /dev/null +++ b/cmd/mavend/keyfile.go @@ -0,0 +1,111 @@ +package main + +// Writing the wrapped-key blob (Vikunja #14). +// +// The blob is the only thing that opens the database on a cold-started box, so +// the two rules here are about not losing it. +// +// # It is rewritten on every assertion, so the write must be atomic +// +// mavweb calls StoreEncryptionKey after every successful assertion, not only +// after enrolment. os.WriteFile truncates in place: a power cut or an OOM kill +// between the truncate and the write left a zero-length blob and no previous +// contents, on the path of every routine step-up. Write to a temp file in the +// same directory, fsync it, rename over the target, then fsync the directory. +// +// # Only one authenticator can hold the cold-start key +// +// A blob is wrapped under one credential's PRF output and nothing else opens +// it. mavweb sends an empty allowCredentials list and the credential store +// keeps more than one passkey, so an unconditional rewrite meant the last +// authenticator to assert silently locked out every other one — including the +// backup hardware key enrolled for exactly the cold-start case. So: a blob +// that already opens under this secret and already wraps this key is left +// alone, a v1 blob is upgraded in place, and a v2 blob belonging to a +// different credential is refused rather than overwritten. + +import ( + "bytes" + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/kami/maven/internal/webauthn" +) + +// errForeignBlob — the wrapped key on disk belongs to another credential. +// Refusing is the point: overwriting would lock that authenticator out. +var errForeignBlob = errors.New("wrapped key belongs to a different credential") + +// wrapKeyToFile wraps key under secret and persists it at path, unless the +// blob already there says not to. Reports whether it wrote anything. +func wrapKeyToFile(path string, key, secret []byte) (wrote bool, err error) { + existing, err := os.ReadFile(path) + switch { + case err == nil: + plain, version, uerr := webauthn.UnwrapKey(existing, secret) + switch { + case uerr == nil && version == webauthn.BlobV2 && bytes.Equal(plain, key): + // Already wrapped under this secret, around this key. The + // common case on every assertion after the first. + return false, nil + case uerr != nil && version == webauthn.BlobV2: + return false, fmt.Errorf("%w: %s does not open under this assertion's PRF output, so another passkey holds the cold-start key; delete it deliberately to re-wrap", errForeignBlob, path) + } + // A v1 blob (upgrade it), or a v2 blob wrapping a stale key under + // this same secret (the key was rotated). Both are rewrites. + case errors.Is(err, os.ErrNotExist): + // First wrap. + default: + return false, fmt.Errorf("read wrapped key: %w", err) + } + + blob, err := webauthn.WrapKey(key, secret) + if err != nil { + return false, fmt.Errorf("wrap encryption key: %w", err) + } + if err := writeFileAtomic(path, blob, 0o600); err != nil { + return false, fmt.Errorf("write wrapped key: %w", err) + } + return true, nil +} + +// writeFileAtomic writes data to path so that a reader sees either the whole +// new file or the whole old one, never a truncated blob. +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + f, err := os.CreateTemp(dir, filepath.Base(path)+".tmp*") + if err != nil { + return err + } + tmp := f.Name() + defer os.Remove(tmp) // no-op once the rename succeeded + + if err := f.Chmod(perm); err != nil { + f.Close() + return err + } + if _, err := f.Write(data); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + if err := f.Close(); err != nil { + return err + } + if err := os.Rename(tmp, path); err != nil { + return err + } + // The rename itself needs to reach the disk, or a crash can resurrect the + // old directory entry pointing at a file that is gone. + d, err := os.Open(dir) + if err != nil { + return err + } + defer d.Close() + return d.Sync() +} diff --git a/cmd/mavend/keyfile_test.go b/cmd/mavend/keyfile_test.go new file mode 100644 index 0000000..bf3cb1e --- /dev/null +++ b/cmd/mavend/keyfile_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "bytes" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/kami/maven/internal/webauthn" +) + +func wrapPath(t *testing.T) string { + t.Helper() + return filepath.Join(t.TempDir(), "db_key.wrapped") +} + +// The first wrap writes a v2 blob that opens under the same secret. +func TestWrapKeyToFileWritesAnOpenableBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{1}, 32) + secret := bytes.Repeat([]byte{2}, 32) + + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want a write", wrote, err) + } + blob, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read blob: %v", err) + } + plain, version, err := webauthn.UnwrapKey(blob, secret) + if err != nil || version != webauthn.BlobV2 || !bytes.Equal(plain, key) { + t.Fatalf("UnwrapKey = %x, %v, %v", plain, version, err) + } + if fi, err := os.Stat(path); err != nil || fi.Mode().Perm() != 0o600 { + t.Fatalf("mode = %v (%v), want 0600", fi.Mode().Perm(), err) + } +} + +// A blob that already wraps this key under this secret is left alone. Without +// this every assertion rewrote the one file that opens the database. +func TestWrapKeyToFileSkipsAnIdenticalBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{3}, 32) + secret := bytes.Repeat([]byte{4}, 32) + + if _, err := wrapKeyToFile(path, key, secret); err != nil { + t.Fatalf("first wrap: %v", err) + } + before, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil { + t.Fatalf("second wrap: %v", err) + } + if wrote { + t.Error("rewrote a blob that already opens under this secret") + } + after, _ := os.ReadFile(path) + if !bytes.Equal(before, after) { + t.Error("the blob changed on a no-op wrap") + } +} + +// Two enrolled authenticators, two PRF secrets, one blob. The second must not +// silently lock the first one out — the backup passkey enrolled for exactly +// the cold-start case is the one thing that used to stop working. +func TestWrapKeyToFileRefusesAnotherCredentialsBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{5}, 32) + phone := bytes.Repeat([]byte{6}, 32) + yubikey := bytes.Repeat([]byte{7}, 32) + + if _, err := wrapKeyToFile(path, key, phone); err != nil { + t.Fatalf("first wrap: %v", err) + } + before, _ := os.ReadFile(path) + + wrote, err := wrapKeyToFile(path, key, yubikey) + if !errors.Is(err, errForeignBlob) { + t.Fatalf("wrapKeyToFile = %v, %v; want errForeignBlob", wrote, err) + } + after, _ := os.ReadFile(path) + if !bytes.Equal(before, after) { + t.Fatal("the second authenticator overwrote the first one's blob") + } + if _, _, err := webauthn.UnwrapKey(after, phone); err != nil { + t.Fatalf("the first authenticator can no longer open the blob: %v", err) + } +} + +// A v1 blob is the pre-#14 format. It is upgraded in place rather than +// refused, because that is the only way off a format that protects nothing. +func TestWrapKeyToFileUpgradesALegacyBlob(t *testing.T) { + path := wrapPath(t) + key := bytes.Repeat([]byte{8}, 32) + secret := bytes.Repeat([]byte{9}, 32) + + // A v1 blob is a v2 blob with the magic stripped and the v1 info string; + // the package writes no v1, so build one the only way a test can: wrap + // v2 under a public key, then hand the file a body with no magic. What + // matters here is only that UnwrapKey classifies it as v1. + v2, err := webauthn.WrapKey(key, secret) + if err != nil { + t.Fatalf("WrapKey: %v", err) + } + legacy := v2[7:] // drop the magic + if err := os.WriteFile(path, legacy, 0o600); err != nil { + t.Fatalf("write legacy blob: %v", err) + } + if _, version, _ := webauthn.UnwrapKey(legacy, secret); version != webauthn.BlobV1 { + t.Fatalf("fixture is not read as v1 (got %v)", version) + } + + wrote, err := wrapKeyToFile(path, key, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want the legacy blob upgraded", wrote, err) + } + blob, _ := os.ReadFile(path) + if _, version, err := webauthn.UnwrapKey(blob, secret); err != nil || version != webauthn.BlobV2 { + t.Fatalf("after upgrade: version %v, err %v", version, err) + } +} + +// A rotated at-rest key under the same credential is a rewrite, not a no-op. +func TestWrapKeyToFileRewritesARotatedKey(t *testing.T) { + path := wrapPath(t) + secret := bytes.Repeat([]byte{10}, 32) + old := bytes.Repeat([]byte{11}, 32) + fresh := bytes.Repeat([]byte{12}, 32) + + if _, err := wrapKeyToFile(path, old, secret); err != nil { + t.Fatalf("first wrap: %v", err) + } + wrote, err := wrapKeyToFile(path, fresh, secret) + if err != nil || !wrote { + t.Fatalf("wrapKeyToFile = %v, %v; want the rotated key written", wrote, err) + } + blob, _ := os.ReadFile(path) + plain, _, err := webauthn.UnwrapKey(blob, secret) + if err != nil || !bytes.Equal(plain, fresh) { + t.Fatalf("blob still wraps the old key (%v)", err) + } +} + +// The write never truncates the target in place, so a crash mid-write cannot +// leave a zero-length blob where the only copy of the wrapped key was. +func TestWriteFileAtomicLeavesNoTempFilesAndReplacesWhole(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "db_key.wrapped") + + if err := os.WriteFile(path, bytes.Repeat([]byte{0xaa}, 67), 0o600); err != nil { + t.Fatalf("seed: %v", err) + } + // Hold the old inode. A rename gives it a new one; a truncating write + // would keep it. + oldInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + + want := bytes.Repeat([]byte{0xbb}, 67) + if err := writeFileAtomic(path, want, 0o600); err != nil { + t.Fatalf("writeFileAtomic: %v", err) + } + got, err := os.ReadFile(path) + if err != nil || !bytes.Equal(got, want) { + t.Fatalf("content = %x (%v)", got, err) + } + newInfo, err := os.Stat(path) + if err != nil { + t.Fatalf("stat: %v", err) + } + if os.SameFile(oldInfo, newInfo) { + t.Error("the target was written in place, not renamed over") + } + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("readdir: %v", err) + } + if len(entries) != 1 { + t.Errorf("directory holds %d entries, want just the blob (a temp file leaked)", len(entries)) + } +} diff --git a/cmd/mavend/main.go b/cmd/mavend/main.go index a384933..62b98e2 100644 --- a/cmd/mavend/main.go +++ b/cmd/mavend/main.go @@ -25,7 +25,7 @@ // When a passkey credential is enrolled AND no env key is set, the daemon // starts in LOCKED mode: the IPC server runs but rejects all store methods // except MethodAssertStepUp and MethodUnlock. A passkey assertion followed -// by MethodUnlock (with the same credential's public key) unwraps the at-rest +// by MethodUnlock (with that credential's WebAuthn PRF output) unwraps the at-rest // AES-256 key from a wrapped blob on disk (HKDF-SHA256 + AES-GCM) and opens // the encrypted store. After unlock, the daemon wires voice, loop, and // delivery and runs normally. @@ -34,10 +34,13 @@ // starts unlocked from the env key (pre-unlock behavior). Enrolling a passkey // while unlocked calls MethodStoreEncryptionKey to wrap the env key and // persist the wrapped blob — enabling cold-start unlock on the next boot -// after the env key is removed. +// after the env key is removed. That write happens once, when no blob +// exists; replacing an existing one takes an explicit request, see +// cmd/mavend/keyfile.go. package main import ( + "bytes" "context" "encoding/json" "errors" @@ -48,6 +51,7 @@ import ( "os" "os/signal" "sync" + "sync/atomic" "syscall" "time" @@ -164,11 +168,28 @@ func run(args []string) error { var st *store.Store var envKeyBytes []byte // kept for WrapKeyFn (enrollment wraps this key) + // dbKey — the plaintext at-rest key, once the daemon has one. Set at boot + // in env-key mode and inside UnlockFn after a cold start. WrapKeyFn reads + // it from an IPC goroutine, hence the atomic: srv's function fields are + // installed before Serve and must not be reassigned afterwards. + var dbKey atomic.Pointer[[]byte] + + // wrappedPath resolves the blob location the same way for both the read + // at boot and every write, so a default-path deployment cannot wrap to + // one file and unwrap from another. + wrappedPath := func() string { + if *wrappedKeyPath != "" { + return *wrappedKeyPath + } + return cfg.DefaultWrappedKeyPath() + } + if !locked { // Normal boot: env key or plaintext (dev/CI) if envKey != nil { envKeyBytes = make([]byte, len(envKey)) copy(envKeyBytes, envKey) + dbKey.Store(&envKeyBytes) st, err = store.OpenEncrypted(ctx, cfg.DBPath, cfg.DBTmpfs, envKey) } else { st, err = store.Open(ctx, cfg.DBPath) @@ -387,27 +408,44 @@ func run(args []string) error { wireSpeaker(srv, st, cfg) } - // WrapKeyFn — wraps the env key under the passkey PRF secret and persists - // the wrapped blob. Only wired when the daemon has the key in memory (env - // key mode). Called by mavweb after passkey enrollment. + // WrapKeyFn — wraps the at-rest key under the passkey PRF secret and + // persists the wrapped blob. Called by mavweb after every assertion. + // + // It is wired in locked mode too, not only in env-key mode, and that is + // what makes a v1 blob recoverable. A box enrolled before Vikunja #14 + // cold-starts through the legacy public-key retry in mavweb, and the + // StoreEncryptionKey that follows rewrites the blob as v2. Without this + // the only escape from a v1 blob was putting MAVEN_DB_KEY back in the + // environment, which is the thing cold-start unlock exists to avoid. // // webauthn.WrapKey refuses anything that is not a 32-byte PRF output, so // an authenticator without PRF support produces no wrapped file at all // rather than a file that looks protected and is not. - if envKeyBytes != nil { - srv.WrapKeyFn = func(ctx context.Context, secret []byte) error { - blob, err := webauthn.WrapKey(envKeyBytes, secret) + if envKeyBytes != nil || locked { + srv.WrapKeyFn = func(ctx context.Context, secret []byte, explicit bool) error { + kp := dbKey.Load() + if kp == nil { + return errors.New("wrap encryption key: the daemon is locked and has no key yet (unlock first)") + } + wp := wrappedPath() + // Asserting a passkey is not a request to rewrite the cold-start + // key. Without this an assertion carrying a substituted PRF value + // re-wrapped the real database key under it, and a second + // authenticator silently replaced the first one's blob. + if !explicit { + if _, err := os.Stat(wp); err == nil { + return nil + } else if !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("check wrapped key: %w", err) + } + } + wrote, err := wrapKeyToFile(wp, *kp, secret) if err != nil { - return fmt.Errorf("wrap encryption key: %w", err) + return err } - wp := *wrappedKeyPath - if wp == "" { - wp = cfg.DefaultWrappedKeyPath() + if wrote { + log.Printf("mavend: wrapped encryption key under this passkey's PRF output → %s", wp) } - if err := os.WriteFile(wp, blob, 0o600); err != nil { - return fmt.Errorf("write wrapped key: %w", err) - } - log.Printf("mavend: wrapped encryption key with passkey credential (%d bytes)", len(blob)) return nil } } @@ -428,15 +466,17 @@ func run(args []string) error { return nil // already unlocked; the caller does not need to know } - // The wire cannot authenticate its caller — the socket is - // same-uid — so the unlock path requires a passkey assertion - // that mavweb verified cryptographically first. Without this, - // MethodUnlock is reachable by anything on the box. + // Depth, not a boundary. MethodAssertStepUp is AuthRead, so + // anything that can open the same-uid socket can flip the + // session and reach MethodUnlock. What actually stops a local + // attacker is the 32-byte PRF output they do not have, and that + // was true before this check. What this check stops is an + // accidental unlock attempt from an unrelated local caller. if !passkeySess.IsStepUp() { return errors.New("unlock: no verified passkey assertion (assert first)") } - wp := *wrappedKeyPath + wp := wrappedPath() blob, err := os.ReadFile(wp) if err != nil { return fmt.Errorf("read wrapped key: %w", err) @@ -446,8 +486,11 @@ func run(args []string) error { return fmt.Errorf("unwrap key: %w", err) } if version == webauthn.BlobV1 { - log.Printf("SECURITY: %s was unwrapped from a %s blob. The wrapping key is derived from the credential PUBLIC key, which mavweb also writes to its passkeys.json — anyone holding both files can recover the database key with no authenticator. Re-enroll the passkey on an authenticator that supports the PRF extension to rewrite it as v2.", wp, version) + log.Printf("SECURITY: %s was unwrapped from a %s blob. The wrapping key is derived from the credential PUBLIC key, which mavweb also writes to its passkeys.json — anyone holding both files can recover the database key with no authenticator. Use the \"rewrite cold-start key\" button on /auth/webauthn with a PRF-capable authenticator to replace it with a v2 blob.", wp, version) } + // WrapKeyFn needs the key to be able to rewrite the blob later. + keyCopy := bytes.Clone(key) + dbKey.Store(&keyCopy) // Open the store with the unwrapped key. st, err = store.OpenEncrypted(ctx, cfg.DBPath, cfg.DBTmpfs, key) if err != nil { diff --git a/cmd/mavend/speaker.go b/cmd/mavend/speaker.go index 4585908..f717400 100644 --- a/cmd/mavend/speaker.go +++ b/cmd/mavend/speaker.go @@ -3,14 +3,17 @@ // // # What is actually wired here, and what is not // -// The enrolment plumbing is real: profiles are stored, listed and deleted, and -// the wire methods exist as soon as a speaker block is configured. The -// recognising half is NOT, and cannot be on this box, because there is no -// speaker-embedding model on disk — no ECAPA, no x-vector, no titanet, no -// wespeaker, nothing in /mnt/hdd1/llms but text ggufs. Until one is downloaded, -// newSpeakerEmbedder returns nil, internal/speaker falls back to -// speaker.Disabled, and every Identify answers ErrDisabled. The daemon logs -// which half is off at startup rather than pretending. +// Nothing is, on this box. There is no speaker-embedding model on disk — no +// ECAPA, no x-vector, no titanet, no wespeaker, nothing in /mnt/hdd1/llms but +// text ggufs. Until one is downloaded, newSpeakerEmbedder returns nil. +// +// Without an embedder the capability has no runnable half. This comment used to +// say enrolment was real and only recognition was blocked, and the startup log +// said the same. Both were wrong: Recognizer.Enroll embeds every sample before +// it stores anything, so with no model it fails on the first sample and nothing +// is ever stored, which leaves List empty forever and Forget with nothing to +// delete. So the gate is cfg.Speaker.Recognizes() — enabled AND a model path — +// and a box without one gets no speaker methods, not three no-ops. // // This is deliberately not papered over with a hand-rolled MFCC floor. A // biometric that is confidently wrong writes false claims about named people @@ -53,17 +56,25 @@ type speakerWiring struct { // starts working with no change to the store, the protocol, the auth table or // the handlers. See the plan document for what to download. func newSpeakerEmbedder(cfg *config.SpeakerConfig) speaker.Embedder { - if cfg == nil || cfg.ModelPath == "" { - return nil - } - log.Printf("speaker: model_path %q is configured but no embedding backend is built yet; "+ - "enrolment and deletion work, recognition does not (Vikunja #255)", cfg.ModelPath) + _ = cfg return nil } // newSpeakerWiring builds the recognizer, or nil when the capability is off. func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring { - if cfg == nil || cfg.Speaker == nil || !cfg.Speaker.Enabled { + if cfg == nil || cfg.Speaker == nil { + return nil + } + if !cfg.Speaker.Recognizes() { + // Recognizes() was written as the gate and documented as one, and then + // never called. "enabled": true with no model_path used to wire all + // three methods and log "enrolment on", which is the one config shape + // where the operator most needs to be told otherwise. + if cfg.Speaker.Enabled { + log.Print("speaker: enabled but no model_path, so there is nothing to embed with; " + + "enrol, list and forget would all be no-ops, staying off " + + "(see docs/plans/10-speaker-recognition.md)") + } return nil } if st == nil { @@ -81,8 +92,8 @@ func newSpeakerWiring(st *store.Store, cfg *config.Config) *speakerWiring { if rec.Enabled() { log.Printf("speaker: recognition on, threshold %.2f", rec.Threshold()) } else { - log.Print("speaker: enrolment on, recognition BLOCKED — no speaker-embedding model " + - "on this box (see docs/plans/10-speaker-recognition.md)") + log.Printf("speaker: model_path %q is configured but no embedding backend is built yet, "+ + "so enrol, list and forget are all no-ops (Vikunja #255)", cfg.Speaker.ModelPath) } return &speakerWiring{rec: rec} } @@ -114,7 +125,7 @@ func (w *speakerWiring) forget(ctx context.Context, req ipc.ForgetSpeakerReq) er // toWireSpeaker drops the voiceprint. A listing says who is enrolled; it does // not hand the biometric back out over the socket. func toWireSpeaker(p speaker.Profile) ipc.Speaker { - return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples} + return ipc.Speaker{ID: p.ID, Name: p.Name, Enrolled: p.Enrolled, Samples: p.Samples, Damaged: p.Damaged} } // speakerErr maps the package sentinels onto the wire vocabulary so a surface @@ -123,6 +134,11 @@ func speakerErr(err error) error { switch { case err == nil: return nil + case errors.Is(err, speaker.ErrDisabled): + // Not a core failure. The capability is present on the wire but has no + // embedding model behind it, which is the same thing an unconfigured + // method says, so say it the same way. + return ipc.ErrUnknownMethod case errors.Is(err, speaker.ErrNotFound): return ipc.ErrNoFact case errors.Is(err, speaker.ErrBadID), diff --git a/cmd/mavend/speaker_wiring_test.go b/cmd/mavend/speaker_wiring_test.go new file mode 100644 index 0000000..96fd8bd --- /dev/null +++ b/cmd/mavend/speaker_wiring_test.go @@ -0,0 +1,50 @@ +package main + +import ( + "errors" + "testing" + + "github.com/kami/maven/internal/config" + "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/speaker" +) + +// "enabled": true with no model_path used to wire all three methods and log +// "enrolment on". Nothing behind them works without an embedder, so the +// capability stays off and the socket answers "no such method". +func TestSpeakerStaysOffWithoutAModelPath(t *testing.T) { + srv := &ipc.Server{} + cfg := &config.Config{Speaker: &config.SpeakerConfig{Enabled: true}} + + wireSpeaker(srv, nil, cfg) + + if srv.EnrollSpeakerFn != nil || srv.ListSpeakersFn != nil || srv.ForgetSpeakerFn != nil { + t.Error("speaker methods were wired with nothing to embed with") + } +} + +// The gate is Recognizes(), so a disabled block with a model path is off too. +func TestSpeakerStaysOffWhenDisabled(t *testing.T) { + srv := &ipc.Server{} + cfg := &config.Config{Speaker: &config.SpeakerConfig{ModelPath: "/nope/ecapa.onnx"}} + + wireSpeaker(srv, nil, cfg) + + if srv.EnrollSpeakerFn != nil { + t.Error("speaker methods were wired for a disabled block") + } +} + +// ErrDisabled is "this capability is off", not "core broke". It used to fall +// through speakerErr's default and reach the surface as an opaque failure. +func TestSpeakerErrMapsDisabledToUnknownMethod(t *testing.T) { + if got := speakerErr(speaker.ErrDisabled); !errors.Is(got, ipc.ErrUnknownMethod) { + t.Errorf("speakerErr(ErrDisabled) = %v, want ErrUnknownMethod", got) + } + if got := speakerErr(speaker.ErrNotFound); !errors.Is(got, ipc.ErrNoFact) { + t.Errorf("speakerErr(ErrNotFound) = %v, want ErrNoFact", got) + } + if got := speakerErr(nil); got != nil { + t.Errorf("speakerErr(nil) = %v", got) + } +} diff --git a/cmd/mavsttd/golden_test.go b/cmd/mavsttd/golden_test.go index 0783a83..a547e11 100644 --- a/cmd/mavsttd/golden_test.go +++ b/cmd/mavsttd/golden_test.go @@ -2,10 +2,22 @@ package main // Golden-audio STT tests (Vikunja #288). // -// These push real audio through the real whisper.cpp binding, so a bad model -// path, a wrong language hint, a broken resample or a regressed silence gate -// is caught by `make test` rather than by the owner talking to a daemon that -// mishears him. +// These push real audio through the real whisper.cpp binding. What that +// covers, precisely, is two things: the model still transcribes known speech +// well enough for the router to act on it, and the silence gate still lets real +// speech through. A regression in either shows up in `make test` rather than in +// the owner talking to a daemon that mishears him. +// +// It is worth being exact about what is NOT covered, because this comment used +// to claim more. Nothing here resamples: audio.PCMFromWAV refuses anything that +// is not 16 kHz mono s16, the fixtures arrive at 16 kHz from ffmpeg, and there +// is no conversion step between the WAV and whisper_full. Nothing here +// exercises language selection either: the hint comes out of the manifest +// already correct and goes straight into the request, so how mavsttd chooses a +// language is untested. And a wrong model path is not caught when it is the +// default one, because a box without the model skips; an explicitly set +// MAVEN_WHISPER_MODEL that does not exist is a failure, since that is a +// mistake and not an absence. // // The fixtures are piper-synthesised, not recorded — see // scripts/gen-stt-fixtures.sh. Nothing of the owner's voice is committed, and @@ -47,7 +59,10 @@ type goldenCase struct { Lang string `json:"lang"` Text string `json:"text"` Keywords []string `json:"keywords"` - MaxWER float64 `json:"max_wer"` + // MeasuredWER is what this case scored when the ceiling was last set, so + // a model swap is a diff to a recorded number rather than silence. + MeasuredWER float64 `json:"measured_wer"` + MaxWER float64 `json:"max_wer"` } type goldenManifest struct { @@ -152,6 +167,14 @@ func containsSeq(hyp, want []string) bool { return false } +// looseWordMatch reports whether got is want, or an inflection of it. +// +// A shared prefix alone is not enough. "воды" retains three runes, so "водка" +// used to satisfy the ru_fact keyword and the test passed on whisper hearing +// "выпил водки". "disk" retains "dis", which "display", "distance" and +// "discuss" all match. So the hypothesis is also capped in length: a case +// ending adds a rune or two, it does not add a syllable. Short words get no +// slack at all, because there is nothing left of them after a prefix cut. func looseWordMatch(got, want string) bool { if got == want { return true @@ -166,6 +189,13 @@ func looseWordMatch(got, want string) bool { if n < 3 || len(g) < n { return false } + extra := 2 + if len(w) <= 4 { + extra = 0 + } + if len(g) > len(w)+extra { + return false + } return string(g[:n]) == string(w[:n]) } @@ -176,6 +206,11 @@ func TestGoldenAudioTranscription(t *testing.T) { model := goldenModelPath() if _, err := os.Stat(model); err != nil { + // An explicit override that points at nothing is a mistake, not a box + // without the model. Skipping there made a typo look like a pass. + if os.Getenv("MAVEN_WHISPER_MODEL") != "" { + t.Fatalf("MAVEN_WHISPER_MODEL=%s does not exist: %v", model, err) + } t.Skipf("whisper model %s absent (%v) — set MAVEN_WHISPER_MODEL or see AGENTS.md", model, err) } @@ -192,7 +227,9 @@ func TestGoldenAudioTranscription(t *testing.T) { path := filepath.Join("testdata", c.WAV) raw, err := os.ReadFile(path) if err != nil { - t.Skipf("fixture %s absent (%v) — run scripts/gen-stt-fixtures.sh", path, err) + // Not a skip. A fixture the generator failed to write is a + // broken checkout, and skipping made `make test` green on one. + t.Fatalf("fixture %s absent (%v) — run scripts/gen-stt-fixtures.sh", path, err) } format, pcm, err := audio.PCMFromWAV(raw) if err != nil { @@ -221,9 +258,12 @@ func TestGoldenAudioTranscription(t *testing.T) { if missing := missingKeywords(c.Keywords, hyp); len(missing) > 0 { t.Errorf("%s: missing keywords %v in %q", c.WAV, missing, resp.Text) } - if wer := wordErrorRate(ref, hyp); wer > c.MaxWER { - t.Errorf("%s: WER %.2f > %.2f\n want: %q\n got: %q", c.WAV, wer, c.MaxWER, c.Text, resp.Text) + wer := wordErrorRate(ref, hyp) + if wer > c.MaxWER { + t.Errorf("%s: WER %.2f > %.2f (measured %.2f when the ceiling was set)\n want: %q\n got: %q", + c.WAV, wer, c.MaxWER, c.MeasuredWER, c.Text, resp.Text) } + t.Logf("%s: WER %.2f (ceiling %.2f, was %.2f)", c.WAV, wer, c.MaxWER, c.MeasuredWER) }) } } @@ -253,27 +293,29 @@ func TestGoldenFixturesAreCanonical(t *testing.T) { } // The fixture must clear mavsttd's own silence gate, otherwise the // model test below would be asserting on a gated empty string. - if reason := gateReason(pcmToF32(pcm), whisperSampleRate, 300, 0.01); reason != "" { + if reason := gateReason(pcmSamples(pcm), whisperSampleRate, 300, 0.01); reason != "" { t.Errorf("%s: would be gated as %s", path, reason) } if len(c.Keywords) == 0 { t.Errorf("%s: manifest case has no keywords", c.Name) } + // An empty reference makes wordErrorRate return 1 for every + // hypothesis, so the WER assertion fires with nothing useful to say. + if len(normalizeTranscript(c.Text)) == 0 { + t.Errorf("%s: manifest case has no reference text", c.Name) + } + if c.Lang != "ru" && c.Lang != "en" { + t.Errorf("%s: lang %q is not one of the two languages mavsttd is run with", c.Name, c.Lang) + } if c.MaxWER <= 0 || c.MaxWER > 1 { t.Errorf("%s: max_wer %v outside (0,1]", c.Name, c.MaxWER) } + if c.MeasuredWER > c.MaxWER { + t.Errorf("%s: measured_wer %v is above max_wer %v, so the ceiling was never met", c.Name, c.MeasuredWER, c.MaxWER) + } } } -func pcmToF32(b []byte) []float32 { - out := make([]float32, len(b)/2) - for i := range out { - s := int16(b[i*2]) | int16(b[i*2+1])<<8 - out[i] = float32(s) / 32768.0 - } - return out -} - // --- matcher unit tests (no model, no fixtures) ---------------------------- func TestNormalizeTranscript(t *testing.T) { @@ -320,4 +362,30 @@ func TestMissingKeywords(t *testing.T) { if got := missingKeywords([]string{"часть"}, hyp2); len(got) != 1 { t.Fatalf("missingKeywords = %v, want %q reported missing", got, "часть") } + // A prefix is not a word. These are different words that share one, and + // each of them used to satisfy the keyword it is paired with. + different := [][2]string{ + {"воды", "Я выпил водки."}, + {"disk", "check the display"}, + {"disk", "we should discuss it"}, + {"server", "a serverless function"}, + } + for _, d := range different { + if got := missingKeywords([]string{d[0]}, normalizeTranscript(d[1])); len(got) != 1 { + t.Errorf("keyword %q was satisfied by %q", d[0], d[1]) + } + } + // And the inflections still pass, which is the whole point of the loose + // match. + same := [][2]string{ + {"воды", "выпил воду"}, + {"напомни", "напомните мне"}, + {"календарю", "по календаре"}, + {"restart", "restarted the server"}, + } + for _, d := range same { + if got := missingKeywords([]string{d[0]}, normalizeTranscript(d[1])); len(got) != 0 { + t.Errorf("keyword %q was not matched by %q", d[0], d[1]) + } + } } diff --git a/cmd/mavsttd/testdata/golden_v1.json b/cmd/mavsttd/testdata/golden_v1.json index 870a4d5..5e7b4b9 100644 --- a/cmd/mavsttd/testdata/golden_v1.json +++ b/cmd/mavsttd/testdata/golden_v1.json @@ -1,5 +1,6 @@ { - "note": "Golden STT fixtures. Audio is piper-synthesised, not recorded — see scripts/gen-stt-fixtures.sh. Regenerate with that script; do not hand-edit `wav`.", + "note": "Golden STT fixtures. Audio is piper-synthesised, not recorded — see scripts/gen-stt-fixtures.sh, which reads `text` from this file and synthesises from it. This is the only source of the spoken words; regenerate with that script and do not hand-edit `wav`.", + "wer_note": "max_wer is set just above what each case actually measures against ggml-small, recorded in `measured_wer` on 2026-08-01. A flat 0.34 over a five-word reference tolerated two wrong words and left most of the range unguarded. A model swap should show up as a diff to these numbers, not as silence: rerun `make test-stt-golden`, read the logged transcript, and move both fields together.", "cases": [ { "name": "ru_reminder", @@ -7,7 +8,8 @@ "lang": "ru", "text": "напомни мне через час позвонить маме", "keywords": ["напомни", "час", "позвонить"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 }, { "name": "ru_fact", @@ -15,7 +17,8 @@ "lang": "ru", "text": "отметь что я выпил воды", "keywords": ["отметь", "воды"], - "max_wer": 0.34 + "measured_wer": 0.2, + "max_wer": 0.25 }, { "name": "ru_query", @@ -23,7 +26,8 @@ "lang": "ru", "text": "что у меня сегодня по календарю", "keywords": ["сегодня", "календарю"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 }, { "name": "en_act", @@ -31,7 +35,8 @@ "lang": "en", "text": "restart the web server and check the disk space", "keywords": ["restart", "server", "disk"], - "max_wer": 0.34 + "measured_wer": 0.0, + "max_wer": 0.1 } ] } diff --git a/cmd/mavsttd/whisper_handler.go b/cmd/mavsttd/whisper_handler.go index b8cdf93..bc58cbd 100644 --- a/cmd/mavsttd/whisper_handler.go +++ b/cmd/mavsttd/whisper_handler.go @@ -66,6 +66,19 @@ func gateReason(samples []float32, rate, minMs int, minRMS float64) string { return "" } +// pcmSamples converts canonical s16le little-endian PCM to the float32 range +// whisper wants. Shared with the golden tests: they used to carry their own +// copy, so a regression here (a /32767 divisor, a byte order slip) left the +// assertion that the fixtures clear the silence gate green. +func pcmSamples(b []byte) []float32 { + out := make([]float32, len(b)/2) + for i := range out { + s := int16(b[i*2]) | int16(b[i*2+1])<<8 + out[i] = float32(s) / 32768.0 + } + return out +} + func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeReq) (worker.TranscribeResp, error) { if err := ctx.Err(); err != nil { return worker.TranscribeResp{}, fmt.Errorf("whisper: context done before transcribe: %w", err) @@ -75,12 +88,7 @@ func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeRe return worker.TranscribeResp{}, fmt.Errorf("whisper: empty audio") } - nSamples := len(a.Bytes) / 2 - samples := make([]float32, nSamples) - for i := 0; i < nSamples; i++ { - s := int16(a.Bytes[i*2]) | int16(a.Bytes[i*2+1])<<8 - samples[i] = float32(s) / 32768.0 - } + samples := pcmSamples(a.Bytes) // Silence gate: drop non-speech before whisper hallucinates on it. if reason := gateReason(samples, whisperSampleRate, h.minMs, h.minRMS); reason != "" { @@ -111,7 +119,7 @@ func (h *whisperHandler) Transcribe(ctx context.Context, req worker.TranscribeRe ch := make(chan result, 1) cSamples := (*C.float)(unsafe.Pointer(&samples[0])) go func() { - ch <- result{code: int(C.whisper_full(h.ctx, params, cSamples, C.int(nSamples)))} + ch <- result{code: int(C.whisper_full(h.ctx, params, cSamples, C.int(len(samples))))} }() select { case r := <-ch: diff --git a/cmd/mavwaked/main.go b/cmd/mavwaked/main.go index 60968f2..65e4a55 100644 --- a/cmd/mavwaked/main.go +++ b/cmd/mavwaked/main.go @@ -14,9 +14,12 @@ // // While a reply is playing the capture side is muted (half-duplex): without // it, Maven's own voice comes back in through the mic and she answers -// herself. -barge-in punches one hole in that gate — sustained energy well -// above the speaker's leak level cuts playback so he can talk over her. It is -// off by default because the threshold is room-specific; see playback.go. +// herself. -barge-in punches one hole in that gate — sustained energy above +// -barge-in-rms cuts playback so he can talk over her. It is off by default +// because the threshold is room-specific; see playback.go. The threshold is a +// raw frame RMS and has no reference to what the speaker actually leaks, so +// the daemon logs the mean energy of the frames it suppressed while speaking. +// Set -barge-in-rms from those numbers rather than by guessing. // // usage: // mavwaked # default ALSA device, 127.0.0.1:9100 @@ -130,7 +133,15 @@ func run(args []string) error { var barge bargeInConfig if *bargeIn { barge = bargeInConfig{RMS: float64(*bargeRMS) / 10000.0, Frames: *bargeFrames} - log.Printf("mavwaked: barge-in on (rms %.4f x %d frames)", barge.RMS, barge.Frames) + if barge.Enabled() { + log.Printf("mavwaked: barge-in on (rms %.4f x %d frames)", barge.RMS, barge.Frames) + } else { + // The log used to say "barge-in on (rms 0.0000 x 5)" here and then + // nothing happened, because Enabled needs a positive threshold. + log.Printf("mavwaked: -barge-in was passed but rms %.4f x %d frames disables it; "+ + "both must be above zero, so barge-in is OFF", + barge.RMS, barge.Frames) + } } sess := newSession(vad, newAplayPlayer(), &voiceSender{vc: vc}, *lang, barge) diff --git a/cmd/mavwaked/playback.go b/cmd/mavwaked/playback.go index 7bc5e6d..01fd69d 100644 --- a/cmd/mavwaked/playback.go +++ b/cmd/mavwaked/playback.go @@ -33,6 +33,11 @@ import ( "github.com/kami/maven/internal/audio" ) +// playbackMargin is the slack over the reply's own duration before a stuck +// aplay is killed. Enough for ALSA to open the device and drain its buffer, +// short enough that a busy device does not cost her a turn. +const playbackMargin = 2 * time.Second + // player plays one reply at a time and can be cut off mid-utterance. type player interface { // Play starts playback of a, replacing anything already playing, and @@ -87,6 +92,13 @@ func (p *aplayPlayer) Play(a audio.Audio) { p.playing = true p.mu.Unlock() + // Bound the mute window by the reply itself. Playing() gates all capture + // now, so a wedged aplay does not merely go silent, it makes her deaf for + // as long as the flag is set. The old ceiling was a flat 30s inherited + // from the fire-and-forget version, where it only bounded a leaked + // goroutine. A reply cannot legitimately take longer than it lasts. + limit := time.Duration(a.Duration()*float64(time.Second)) + playbackMargin + go func() { if _, err := stdin.Write(a.Bytes); err != nil { // Broken pipe is the expected outcome of Stop(). @@ -101,8 +113,9 @@ func (p *aplayPlayer) Play(a audio.Audio) { if err != nil { log.Printf("mavwaked: aplay: %v", err) } - case <-time.After(30 * time.Second): - log.Printf("mavwaked: aplay timeout, killing") + case <-time.After(limit): + log.Printf("mavwaked: aplay did not finish %.1fs of audio within %s, killing (capture was muted the whole time)", + a.Duration(), limit) if pr := cmd.Process; pr != nil { _ = pr.Kill() } diff --git a/cmd/mavwaked/session.go b/cmd/mavwaked/session.go index 21e7a68..24de887 100644 --- a/cmd/mavwaked/session.go +++ b/cmd/mavwaked/session.go @@ -7,6 +7,7 @@ package main import ( "context" "log" + "time" "github.com/kami/maven/internal/audio" ) @@ -42,6 +43,20 @@ type session struct { lang string barge bargeInConfig + // now is the clock, swapped in tests. The round-trip backlog is measured + // in wall time, because that is the only thing that says how much room + // went into the pipe while the daemon was thinking. + now func() time.Time + + // discard is how many buffered frames still have to be thrown away + // before capture means anything again. See dispatch. + discard int + + // recent holds the last few frames seen during playback, so the ones + // that proved he was interrupting can be replayed into the VAD after the + // barge-in reset instead of being clipped off the front of his sentence. + recent [][]byte + // loudFrames counts consecutive over-threshold frames seen while she is // speaking. Reset whenever a frame falls back under the threshold, and // whenever playback ends. @@ -49,14 +64,28 @@ type session struct { // counters, read by tests and logged on the way out. suppressed int // frames dropped because she was speaking + dropped int // frames dropped as round-trip backlog bargeIns int // times playback was cut because he spoke over her sent int // utterances shipped to the daemon + + // loudSum and loudSeen accumulate the energy of suppressed frames, so + // the operator can read what the room actually measures and set + // -barge-in-rms from data instead of guessing. + loudSum float64 + loudSeen int } func newSession(vad *VAD, p player, s utteranceSender, lang string, barge bargeInConfig) *session { - return &session{vad: vad, player: p, sender: s, lang: lang, barge: barge} + return &session{vad: vad, player: p, sender: s, lang: lang, barge: barge, now: time.Now} } +// frameDuration is the wall time one captured frame represents. +const frameDuration = defaultFrameMs * time.Millisecond + +// suppressLogEvery — how many suppressed frames between energy reports. 200 +// frames is six seconds of her talking, so this is roughly one line per reply. +const suppressLogEvery = 200 + // feed processes one 30ms PCM frame. // // While the player is running the capture side is muted: the VAD is not fed @@ -65,26 +94,52 @@ func newSession(vad *VAD, p player, s utteranceSender, lang string, barge bargeI // energy well above the speaker's leak level cuts playback, and capture // resumes on the very next frame with a clean VAD. func (s *session) feed(ctx context.Context, frame []byte) error { + // Backlog first, before anything looks at this frame. These are frames + // the microphone captured while the round-trip blocked; they arrive in a + // burst at pipe speed and they are not a command, not an answer and not + // an interruption. + if s.discard > 0 { + s.discard-- + s.dropped++ + return nil + } + if s.player.Playing() { s.suppressed++ + rms := frameRMS(PCMToI16(frame)) + s.loudSum += rms + s.loudSeen++ + if s.loudSeen >= suppressLogEvery { + // The doc comment asks for energy "well above the speaker's leak + // level" and never says what that is. This is what it is. + log.Printf("mavwaked: suppressed %d frames while speaking, mean rms %.4f (barge-in threshold %.4f)", + s.loudSeen, s.loudSum/float64(s.loudSeen), s.barge.RMS) + s.loudSum, s.loudSeen = 0, 0 + } if !s.barge.Enabled() { return nil } - if frameRMS(PCMToI16(frame)) < s.barge.RMS { + if rms < s.barge.RMS { s.loudFrames = 0 + s.recent = s.recent[:0] return nil } s.loudFrames++ + s.keepRecent(frame) if s.loudFrames < s.barge.Frames { return nil } // He is talking over her. Cut her off, drop the VAD state that - // accumulated from the echo, and start listening for real. + // accumulated from the echo, and start listening for real — starting + // with the frames that proved he was talking. Those used to be + // thrown away, which clipped the first 150ms off his interruption, + // and on a short one that is the whole first word. s.player.Stop() s.bargeIns++ s.loudFrames = 0 s.vad.Reset() log.Printf("mavwaked: barge-in — stopped playback") + s.replayRecent() return nil } @@ -102,22 +157,77 @@ func (s *session) feed(ctx context.Context, frame []byte) error { return s.dispatch(ctx, utt) } +// keepRecent stores a copy of one barge-in trigger frame, keeping at most +// barge.Frames of them. +func (s *session) keepRecent(frame []byte) { + if len(s.recent) >= s.barge.Frames { + copy(s.recent, s.recent[1:]) + s.recent = s.recent[:len(s.recent)-1] + } + s.recent = append(s.recent, append([]byte(nil), frame...)) +} + +// replayRecent feeds the trigger frames back into the freshly reset VAD, so +// his interruption starts where he started it. +// +// Feed cannot complete an utterance here: closing one needs silenceMs of +// trailing quiet and these frames are all above the barge-in threshold, which +// is far above the VAD floor. Any utterance it did return would be a fragment +// of a sentence he is still speaking, so it is not dispatched. +func (s *session) replayRecent() { + for _, f := range s.recent { + s.vad.Feed(PCMToI16(f)) + } + s.recent = s.recent[:0] +} + // dispatch ships a complete utterance and plays whatever comes back. +// +// Every return path here has to deal with the backlog. Nothing reads the +// microphone while Send is in flight, so the audio piles up in arecord's pipe +// and the kernel buffer, and it arrives in a burst the moment this returns. A +// round-trip is p50 2.7s through the LLM router, which is around 90 frames of +// room, of him finishing his sentence, of the television. +// +// This used to reset the VAD on the reply path only, and for the wrong reason: +// the comment said the VAD had been accumulating during the round-trip, when +// in fact its state is exactly what Feed left it as. The two paths that had no +// reset are the ones that mattered, because neither of them starts playback +// and so neither is covered by the half-duplex gate. A text-only turn fed the +// whole backlog straight into the VAD, and a Send error did the same on every +// failed turn, so a dead socket drove a retry loop off nothing but backlog. func (s *session) dispatch(ctx context.Context, utt audio.Audio) error { log.Printf("mavwaked: utterance complete (%.2fs, %d bytes), sending...", utt.Duration(), len(utt.Bytes)) + start := s.now() reply, err := s.sender.Send(ctx, utt, s.lang) - s.sent++ + defer s.dropBacklog(start) if err != nil { return err } + // Count what was shipped, not what was attempted. This used to run + // before the error check, so failed round-trips counted as sent. + s.sent++ if len(reply.Bytes) == 0 { log.Printf("mavwaked: empty reply audio (text only)") return nil } - // The VAD has been accumulating from the buffered mic stream while the - // round-trip blocked. None of it is a command — reset before the - // speaker opens, so the first post-reply frame starts clean. - s.vad.Reset() s.player.Play(reply) return nil } + +// dropBacklog resets the VAD and arranges for the frames captured during the +// round-trip to be thrown away as they arrive. +// +// Discarding them is also what keeps barge-in honest. The Frames guard is +// documented as "long enough that a door or a cough does not cut her off", +// which assumes the frames are real time. Draining a backlog delivers five +// frames in microseconds, so without this she could be cut off by audio +// recorded before she started speaking. +func (s *session) dropBacklog(start time.Time) { + s.vad.Reset() + s.loudFrames = 0 + s.recent = s.recent[:0] + if elapsed := s.now().Sub(start); elapsed > 0 { + s.discard = int(elapsed / frameDuration) + } +} diff --git a/cmd/mavwaked/session_test.go b/cmd/mavwaked/session_test.go index 8fa94d6..da6940b 100644 --- a/cmd/mavwaked/session_test.go +++ b/cmd/mavwaked/session_test.go @@ -5,6 +5,7 @@ import ( "errors" "math" "testing" + "time" "github.com/kami/maven/internal/audio" ) @@ -280,3 +281,147 @@ func TestBargeInConfigEnabled(t *testing.T) { } } } + +// slowSender models the real thing: a round-trip takes wall-clock time, and +// the microphone keeps recording into a pipe nobody is reading. +type slowSender struct { + fakeSender + clock *time.Time + took time.Duration +} + +func (s *slowSender) Send(ctx context.Context, utt audio.Audio, lang string) (audio.Audio, error) { + *s.clock = s.clock.Add(s.took) + return s.fakeSender.Send(ctx, utt, lang) +} + +// newSlowSession wires a session whose round-trip takes took of wall time. +func newSlowSession(barge bargeInConfig, reply audio.Audio, err error, took time.Duration) (*session, *fakePlayer, *slowSender) { + now := time.Unix(0, 0) + p := &fakePlayer{} + snd := &slowSender{fakeSender: fakeSender{reply: reply, err: err}, clock: &now, took: took} + sess := newSession(NewVAD(0, 0, 0, 0), p, snd, "ru", barge) + sess.now = func() time.Time { return now } + return sess, p, snd +} + +// A text-only turn starts no playback, so the half-duplex gate does not cover +// it. The backlog captured during the round-trip has to be dropped anyway, or +// three seconds of room arrives at pipe speed and becomes a command. +func TestSessionDropsBacklogAfterAnEmptyReply(t *testing.T) { + sess, p, snd := newSlowSession(bargeInConfig{}, audio.Audio{Format: audio.PCM16kMono}, nil, 3*time.Second) + + speakThenPause(t, sess) + if p.plays != 0 || len(snd.sent) != 1 { + t.Fatalf("plays = %d, sent = %d; want one text-only turn", p.plays, len(snd.sent)) + } + // The tail of speakThenPause already spent a couple of them. + if want := int(3 * time.Second / frameDuration); sess.discard+sess.dropped != want { + t.Fatalf("discard %d + dropped %d frames, want %d (3s of backlog)", sess.discard, sess.dropped, want) + } + + // The burst: the whole backlog, all of it him still talking. + loud := frameAt(0.35) + for i := 0; i < sess.discard; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if len(snd.sent) != 1 { + t.Errorf("the backlog was sent as a second utterance (sent = %d)", len(snd.sent)) + } + if sess.dropped == 0 { + t.Error("no frames were counted as backlog") + } +} + +// Same on the error path. A dead daemon used to seed the next spurious trigger +// on every failed turn, so a dead socket drove a retry loop off backlog alone. +func TestSessionDropsBacklogAfterASendError(t *testing.T) { + sess, _, _ := newSlowSession(bargeInConfig{}, audio.Audio{}, errors.New("boom"), 3*time.Second) + + // Not speakThenPause: the dispatch returns the send error, which that + // helper treats as fatal. + loud := frameAt(0.35) + for i := 0; i < (defaultSpeechMs+defaultFrameMs-1)/defaultFrameMs+5; i++ { + _ = sess.feed(context.Background(), loud) + } + for i := 0; i < (defaultSilenceMs+defaultFrameMs-1)/defaultFrameMs+2; i++ { + _ = sess.feed(context.Background(), silentBytes()) + } + if sess.discard == 0 { + t.Fatal("a failed round-trip left the backlog to be fed into the VAD") + } +} + +// Barge-in must not be triggerable by the backlog. Those frames are him +// finishing the sentence he started before she answered, delivered in +// microseconds, and the five-frame guard assumes real time. +func TestSessionBacklogCannotBargeIn(t *testing.T) { + sess, p, _ := newSlowSession(bargeInConfig{RMS: 0.12, Frames: 5}, replyAudio(), nil, 3*time.Second) + + speakThenPause(t, sess) + if p.plays != 1 || !p.Playing() { + t.Fatalf("plays = %d, playing = %v; want the reply playing", p.plays, p.Playing()) + } + + loud := frameAt(0.35) + backlog := sess.discard + if backlog < 5 { + t.Fatalf("discard = %d, want a real backlog", backlog) + } + for i := 0; i < backlog; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if p.stops != 0 { + t.Fatalf("she was cut off by audio recorded before she started speaking (stops = %d)", p.stops) + } + + // Real-time speech after the backlog still interrupts her. + for i := 0; i < 5; i++ { + if err := sess.feed(context.Background(), loud); err != nil { + t.Fatal(err) + } + } + if p.stops != 1 { + t.Fatalf("stops = %d, want 1 — barge-in must still work after the backlog", p.stops) + } +} + +// The frames that proved he was interrupting are replayed into the VAD, so his +// first word is not clipped. Five trigger frames plus five real ones reach the +// 300ms speech threshold; without the replay the first five are lost and no +// utterance is produced at all. +func TestSessionReplaysTheBargeInTriggerFrames(t *testing.T) { + sess, p, snd := newTestSession(bargeInConfig{RMS: 0.12, Frames: 5}) + speakThenPause(t, sess) + + veryLoud := frameAt(0.35) + for i := 0; i < 5; i++ { + _ = sess.feed(context.Background(), veryLoud) + } + if p.stops != 1 { + t.Fatalf("expected barge-in, stops = %d", p.stops) + } + if p.Playing() { + t.Fatal("fake player still playing after Stop") + } + + speechFrames := (defaultSpeechMs + defaultFrameMs - 1) / defaultFrameMs + for i := 0; i < speechFrames-5; i++ { + if err := sess.feed(context.Background(), veryLoud); err != nil { + t.Fatal(err) + } + } + silenceFrames := (defaultSilenceMs+defaultFrameMs-1)/defaultFrameMs + 2 + for i := 0; i < silenceFrames; i++ { + if err := sess.feed(context.Background(), silentBytes()); err != nil { + t.Fatal(err) + } + } + if len(snd.sent) != 2 { + t.Fatalf("sent %d utterances, want 2 — the 150ms that triggered barge-in was clipped", len(snd.sent)) + } +} diff --git a/cmd/mavweb/passkey_prf_test.go b/cmd/mavweb/passkey_prf_test.go index 4c5c95d..915551f 100644 --- a/cmd/mavweb/passkey_prf_test.go +++ b/cmd/mavweb/passkey_prf_test.go @@ -32,17 +32,28 @@ type fakeKeyIPC struct { unlockCalls int wrapCalls int unlockErr error + wrapExplicit bool + // opensWith, when set, is the only secret Unlock accepts. It stands in + // for a wrapped blob on disk: everything else gets unlockErr. + opensWith []byte } func (f *fakeKeyIPC) Unlock(_ context.Context, secret []byte) error { f.unlockCalls++ f.unlockSecret = bytes.Clone(secret) + if f.opensWith != nil { + if bytes.Equal(secret, f.opensWith) { + return nil + } + return errors.New("unwrap key: decrypt failed (wrong credential?)") + } return f.unlockErr } -func (f *fakeKeyIPC) StoreEncryptionKey(_ context.Context, secret []byte) error { +func (f *fakeKeyIPC) StoreEncryptionKey(_ context.Context, secret []byte, explicit bool) error { f.wrapCalls++ f.wrapSecret = bytes.Clone(secret) + f.wrapExplicit = explicit return nil } @@ -137,6 +148,13 @@ func (a *prfAuthenticator) register(t *testing.T, h *PasskeyHandle) { // assert drives POST /assert/finish with a valid assertion and the given // base64url PRF result. func (a *prfAuthenticator) assert(t *testing.T, h *PasskeyHandle, prf string) *httptest.ResponseRecorder { + t.Helper() + return a.assertExplicit(t, h, prf, false) +} + +// assertExplicit is assert with control over the explicit flag the rewrite +// button sets. +func (a *prfAuthenticator) assertExplicit(t *testing.T, h *PasskeyHandle, prf string, explicit bool) *httptest.ResponseRecorder { t.Helper() _, chal, err := h.rp.AssertionOptions() if err != nil { @@ -152,6 +170,7 @@ func (a *prfAuthenticator) assert(t *testing.T, h *PasskeyHandle, prf string) *h body, _ := json.Marshal(map[string]any{ "challenge": chal, "prf": prf, + "explicit": explicit, "credential": map[string]any{ "id": b64u(a.credID), "type": "public-key", @@ -253,8 +272,8 @@ func TestAssertSucceedsWhenUnlockFails(t *testing.T) { if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) } - if key.unlockCalls != 1 { - t.Errorf("unlock attempted %d times, want 1", key.unlockCalls) + if key.unlockCalls == 0 { + t.Error("unlock was never attempted") } } @@ -291,3 +310,100 @@ func TestPasskeyPageRequestsAndPostsPRF(t *testing.T) { } } } + +// A box enrolled before Vikunja #14 has a v1 blob wrapped under the credential +// PUBLIC key. The PRF secret cannot open it, and this handler is the only +// caller of Unlock, so without the legacy retry that box stays locked forever +// while a perfectly good passkey is asserted at it. +func TestLegacyV1BlobStillColdStarts(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + pub, _, err := h.store.Lookup(b64u(auth.credID)) + if err != nil { + t.Fatalf("lookup: %v", err) + } + // The daemon only opens under the public key — a v1 blob. + key.opensWith = pub + + secret := bytes.Repeat([]byte{9}, 32) + if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.unlockCalls != 2 { + t.Fatalf("unlock attempted %d times, want 2 (PRF, then the legacy public key)", key.unlockCalls) + } + if !bytes.Equal(key.unlockSecret, pub) { + t.Fatal("the legacy retry did not send the credential public key, so a v1 box can never cold-start again") + } +} + +// The PRF secret is tried first and, when it works, the public key is never +// sent. The legacy retry is a one-way door out of v1, not a fallback offered +// to every assertion. +func TestPRFUnlockNeverFallsBackWhenItWorks(t *testing.T) { + secret := bytes.Repeat([]byte{7}, 32) + key := &fakeKeyIPC{opensWith: secret} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assert(t, h, b64u(secret)); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.unlockCalls != 1 { + t.Fatalf("unlock attempted %d times, want 1", key.unlockCalls) + } +} + +// Wrapping the at-rest key is an explicit act, never a side effect of a +// step-up. A page POSTing a substituted prf on a routine assertion must not +// make the daemon re-wrap the database key under it. +func TestPlainAssertionAsksForNoRewrite(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assert(t, h, b64u(bytes.Repeat([]byte{5}, 32))); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if key.wrapCalls != 1 { + t.Fatalf("wrapCalls = %d, want 1", key.wrapCalls) + } + if key.wrapExplicit { + t.Fatal("a plain step-up asked the daemon to rewrite the cold-start key") + } +} + +// The rewrite button, and only the rewrite button, sets explicit. +func TestRewriteButtonAsksForAnExplicitWrap(t *testing.T) { + key := &fakeKeyIPC{} + h := newPRFHandle(t, key) + auth := newPRFAuthenticator(t) + auth.register(t, h) + + if w := auth.assertExplicit(t, h, b64u(bytes.Repeat([]byte{6}, 32)), true); w.Code != http.StatusOK { + t.Fatalf("AssertFinish: %d %s", w.Code, w.Body.String()) + } + if !key.wrapExplicit { + t.Fatal("the explicit flag did not reach the daemon, so the rewrite button cannot work") + } +} + +// The page is the only place the explicit flag originates. If the button or +// the field goes away, rewriting a cold-start key becomes impossible with +// nothing failing. +func TestPasskeyPageHasTheRewriteButton(t *testing.T) { + for _, want := range []string{ + "rewrite cold-start key", + "explicit:!!explicit", + "async function rewrapKey()", + } { + if !strings.Contains(passkeyPageHTML, want) { + t.Errorf("the passkey page no longer contains %q", want) + } + } +} diff --git a/cmd/mavweb/webauthn.go b/cmd/mavweb/webauthn.go index 8192d9f..6717e8c 100644 --- a/cmd/mavweb/webauthn.go +++ b/cmd/mavweb/webauthn.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "fmt" "log" "net/http" @@ -23,7 +24,7 @@ type assertIPC interface { // is *ipc.Client; in-process CoreAPI adapters do not implement it. When nil, // StoreEncryptionKey and Unlock are silently skipped. type keyIPC interface { - StoreEncryptionKey(ctx context.Context, secret []byte) error + StoreEncryptionKey(ctx context.Context, secret []byte, explicit bool) error Unlock(ctx context.Context, secret []byte) error } @@ -86,8 +87,10 @@ const passkeyPageHTML = `{{template "shellTop" "passkey"}}
+Rewriting the cold-start key points it at the passkey you assert next. Every other enrolled passkey stops being able to unlock a cold-booted daemon.
{{template "shellBottom"}} ` func (h *PasskeyHandle) RegisterBegin(w http.ResponseWriter, r *http.Request) { @@ -201,7 +211,20 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { // getClientExtensionResults(). Empty when the authenticator has no // PRF extension: cold-start unlock is then unavailable and we say so // rather than falling back to something weaker. + // + // Known property, accepted deliberately: this value is supplied by + // the client and is NOT covered by the assertion signature. WebAuthn + // client extension outputs never are, and binding one would need a + // per-assertion salt, which would make the wrapped blob unopenable on + // the next boot. Nothing here can tell a real PRF output from 32 + // bytes a compromised page chose. What limits the damage is that the + // daemon refuses to rewrite an existing blob unless the operator + // asked for it — see Explicit below and cmd/mavend/keyfile.go. PRF string `json:"prf"` + // Explicit marks the "rewrite cold-start key" button rather than a + // plain step-up. Only then may the daemon replace a blob that is + // already on disk. + Explicit bool `json:"explicit"` } if err := json.NewDecoder(r.Body).Decode(&body); err != nil { http.Error(w, "bad request: "+err.Error(), http.StatusBadRequest) @@ -235,32 +258,22 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { } } - // Cold-start unlock and key wrapping, both keyed on the PRF secret that - // this assertion just produced. The secret is used here and dropped; it is - // never stored on this side. + // Cold-start unlock and key wrapping, both keyed on the PRF secret this + // assertion just produced. The secret is used here and dropped; it is + // never stored on this side, and it must never be logged — unlike a + // signature it does not expire, so one copy in a proxy log or a HAR file + // is permanent access to the wrapped blob. // // Order matters: unlock first (if the daemon is locked there is nothing to - // wrap yet), then re-wrap, which writes the blob on the first assertion - // after enrolment and is a harmless rewrite afterwards. Both are - // best-effort — the assertion itself is valid either way. + // wrap yet), then wrap. Both are best-effort, because the assertion itself + // is valid either way. if h.encryptFn != nil { - secret, err := webauthn.DecodePRFResult(body.PRF) - switch { - case err != nil: + if secret, err := webauthn.DecodePRFResult(body.PRF); err != nil { log.Printf("webauthn: no usable PRF secret from credential %s: %v", credID, err) - default: + } else { ctx, cancel := context.WithTimeout(r.Context(), 10*time.Second) defer cancel() - if err := h.encryptFn.Unlock(ctx, secret); err != nil { - log.Printf("webauthn: unlock via credential %s: %v", credID, err) - } else { - log.Printf("webauthn: daemon unlocked via credential %s", credID) - } - if err := h.encryptFn.StoreEncryptionKey(ctx, secret); err != nil { - log.Printf("webauthn: wrap encryption key: %v", err) - } else { - log.Printf("webauthn: encryption key wrapped for credential %s", credID) - } + h.coldStart(ctx, credID, secret, body.Explicit) } } @@ -272,3 +285,56 @@ func (h *PasskeyHandle) AssertFinish(w http.ResponseWriter, r *http.Request) { log.Printf("webauthn: asserted credential %s", credID) json.NewEncoder(w).Encode(map[string]string{"credential_id": credID}) } + +// coldStart unlocks a locked daemon with this assertion's PRF output and then +// asks it to wrap the at-rest key. Never fatal: a locked or unreachable daemon +// does not invalidate the step-up. +// +// # The legacy retry +// +// A box enrolled before Vikunja #14 has a v1 blob, wrapped under the +// credential PUBLIC key. The PRF secret cannot open it, and this handler is +// the only caller of Unlock, so without a second attempt that box could never +// cold-start again: it would sit locked while a perfectly good passkey was +// asserted, and the only way back in would be putting MAVEN_DB_KEY into the +// environment — the exact thing cold-start unlock exists to avoid. +// +// So a failed PRF unlock is retried with the public key from the credential +// store. That is not a weaker fallback being offered to new deployments: +// nothing writes v1 any more, and a v2 blob does not open under a public key +// either. It is a one-way door out of the old format, and the operator is told +// to walk through it. +func (h *PasskeyHandle) coldStart(ctx context.Context, credID string, secret []byte, explicit bool) { + legacy := false + err := h.encryptFn.Unlock(ctx, secret) + if err != nil && !errors.Is(err, ipc.ErrUnknownMethod) { + if pub, _, lerr := h.store.Lookup(credID); lerr == nil && len(pub) > 0 { + if err2 := h.encryptFn.Unlock(ctx, pub); err2 == nil { + err, legacy = nil, true + } + } + } + switch { + case errors.Is(err, ipc.ErrUnknownMethod): + // Env-key mode: the daemon was never locked and has no UnlockFn. Not + // a failure, and the old code logged it as one on every assertion. + case err != nil: + log.Printf("webauthn: unlock via credential %s failed: %v", credID, err) + case legacy: + log.Printf("SECURITY: webauthn: daemon unlocked from a LEGACY v1 wrapped key using credential %s. That blob is derived from the credential public key, which sits in passkeys.json beside it, so it protects nothing. Press \"rewrite cold-start key\" on this page to replace it with a v2 blob.", credID) + default: + log.Printf("webauthn: daemon reports unlocked, credential %s", credID) + } + + // explicit=false means "write the blob only if there is none". The daemon + // enforces that; sending the flag is the whole of this side's part in it. + switch err := h.encryptFn.StoreEncryptionKey(ctx, secret, explicit); { + case err == nil && explicit: + log.Printf("webauthn: cold-start key rewritten under credential %s", credID) + case err == nil: + case errors.Is(err, ipc.ErrUnknownMethod): + // No key to wrap: a plaintext dev store, or a daemon still locked. + default: + log.Printf("webauthn: wrap encryption key: %v", err) + } +} diff --git a/internal/ipc/api.go b/internal/ipc/api.go index 0355caa..37bb285 100644 --- a/internal/ipc/api.go +++ b/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 diff --git a/internal/ipc/client.go b/internal/ipc/client.go index 0bbbc38..f374998 100644 --- a/internal/ipc/client.go +++ b/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 diff --git a/internal/ipc/server.go b/internal/ipc/server.go index b03cf76..6853174 100644 --- a/internal/ipc/server.go +++ b/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) diff --git a/internal/ipc/unlock_test.go b/internal/ipc/unlock_test.go index 14bdb2e..7d3d541 100644 --- a/internal/ipc/unlock_test.go +++ b/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. diff --git a/internal/memory/store.go b/internal/memory/store.go index d47f80c..5077e47 100644 --- a/internal/memory/store.go +++ b/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}) } diff --git a/internal/memory/store_test.go b/internal/memory/store_test.go index 06856fb..62c69a7 100644 --- a/internal/memory/store_test.go +++ b/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"]) + } +} diff --git a/internal/speaker/recognizer.go b/internal/speaker/recognizer.go index 73b3776..d881de5 100644 --- a/internal/speaker/recognizer.go +++ b/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 -} diff --git a/internal/speaker/speaker.go b/internal/speaker/speaker.go index 25f880b..2fc9b28 100644 --- a/internal/speaker/speaker.go +++ b/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:"-"` diff --git a/internal/speaker/speaker_test.go b/internal/speaker/speaker_test.go index 6c9ed77..5af12b1 100644 --- a/internal/speaker/speaker_test.go +++ b/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) } } diff --git a/internal/store/memory.go b/internal/store/memory.go index 3d0527a..bc83648 100644 --- a/internal/store/memory.go +++ b/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) } diff --git a/internal/store/memory_test.go b/internal/store/memory_test.go index 3e57243..8ba2b09 100644 --- a/internal/store/memory_test.go +++ b/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) + } +} diff --git a/internal/webauthn/keywrap.go b/internal/webauthn/keywrap.go index c056cb2..7129ea4 100644 --- a/internal/webauthn/keywrap.go +++ b/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] diff --git a/internal/webauthn/keywrap_test.go b/internal/webauthn/keywrap_test.go index f2814e8..c3d2816 100644 --- a/internal/webauthn/keywrap_test.go +++ b/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) + } +} diff --git a/models/stt b/models/stt new file mode 120000 index 0000000..b983fa3 --- /dev/null +++ b/models/stt @@ -0,0 +1 @@ +/home/kami/apps/Maven/models/stt \ No newline at end of file diff --git a/models/tts b/models/tts new file mode 120000 index 0000000..66782fb --- /dev/null +++ b/models/tts @@ -0,0 +1 @@ +/home/kami/apps/Maven/models/tts \ No newline at end of file diff --git a/scripts/gen-stt-fixtures.sh b/scripts/gen-stt-fixtures.sh index 98d6dae..5f430dc 100755 --- a/scripts/gen-stt-fixtures.sh +++ b/scripts/gen-stt-fixtures.sh @@ -10,6 +10,13 @@ # Usage: # scripts/gen-stt-fixtures.sh # +# The spoken text is NOT written here. It is read out of +# cmd/mavsttd/testdata/golden_v1.json, which is the same file the test scores +# against. It used to live in both places, so editing this script and running +# make stt-fixtures left the manifest describing audio that no longer existed — +# and at a WER ceiling of 0.34 over a five-word reference, a one-word drift +# passed silently. Punctuation does not matter: normalizeTranscript strips it. +# # Voices are picked up from, in order, $PIPER_VOICE_RU / $PIPER_VOICE_EN, then # the repo's models/tts, then ~/esp-server/voices. The English voice is not # vendored; if it is missing the English fixture is skipped and the existing @@ -34,6 +41,26 @@ ru="$(pick_voice "${PIPER_VOICE_RU:-}" "$root/models/tts/ru_RU-irina-medium.onnx } en="$(pick_voice "${PIPER_VOICE_EN:-}" "$root/models/tts/en_US-lessac-medium.onnx" "$HOME/esp-server/voices/en_US-lessac-medium.onnx")" || en="" +manifest="$root/cmd/mavsttd/testdata/golden_v1.json" +command -v jq >/dev/null || { echo "jq is required to read $manifest" >&2; exit 1; } +[ -f "$manifest" ] || { echo "missing $manifest" >&2; exit 1; } + +# case_text