review fixes: keyring load resilience, publish order, pinned wingetcreate, QR partial decode

Post-review corrections to the security-hardening branch before merging
to main:

- keyring: sanitize names on load instead of rejecting the file. One
  out-of-policy entry in a hand-edited keys.json made the whole registry
  unloadable, and callers that fall back to an empty registry on error
  (GUI rename) would then save and destroy every other entry.
- publish: crates.io order published keyroost-resolve before its
  dependency keyroost-transport, which fails the first tag run; same
  ordering corrected in the TODO runbook.
- publish: pin wingetcreate.exe to a versioned release and verify its
  SHA-256 before running it with the winget token — matches the SHA
  pinning applied to every action in the same file.
- qr: one malformed otpauth payload no longer discards accounts decoded
  from the other QR codes in the same image; it becomes a skip, and the
  underlying error is surfaced only when nothing in the image parses.
- keyroostctl: `import --qr` now prints the "QR i of n" notice for
  multi-code Google Authenticator exports (a clean 1-account import of
  QR 1 read as "migration complete"), and the 0-account error no longer
  suggests import-file, which would equally find nothing.
- SECURITY.md: scope the zeroize-on-drop claim to what is implemented;
  imported seed buffers are tracked in TODO-hardening.md instead.
- udev: correct the ID_SECURITY_TOKEN attribution (fido_id builtin, not
  hwdb) and document that the generic rule needs systemd-udevd.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Author: framefilter

.github/workflows/publish.yml

@@ -75,7 +75,7 @@ jobs:
           version="${TAG#v}"
           for crate in keyroost-proto keyroost-hid keyroost-keyring keyroost-rsakey \
                        keyroost-ctap keyroost-oath keyroost-openpgp keyroost-piv \
-                       keyroost-import keyroost-resolve keyroost-qr keyroost-transport \
+                       keyroost-import keyroost-transport keyroost-resolve keyroost-qr \
                        keyroostctl keyroost; do
             if curl -fsSL "https://crates.io/api/v1/crates/${crate}/${version}" >/dev/null 2>&1; then
               echo "${crate} ${version} already on crates.io — skipping"
@@ -219,7 +219,17 @@ jobs:
         run: |
           $version = $Env:TAG.TrimStart('v')
           $url = "https://github.com/$Env:GITHUB_REPOSITORY/releases/download/$Env:TAG/keyroost-$Env:TAG-windows-x86_64.zip"
-          curl.exe -fsSL -o wingetcreate.exe https://aka.ms/wingetcreate/latest
+          # Pinned + hash-checked for the same reason the actions above are
+          # SHA-pinned: this executable runs with the winget PAT in env.
+          # Hash is from wingetcreate.exe.txt on the same release.
+          $wcVersion = "v1.12.8.0"
+          $wcSha256 = "8bd738851b524885410112678e3771b341c5c716de60fbbecb88ab0a363ed85d"
+          curl.exe -fsSL -o wingetcreate.exe "https://github.com/microsoft/winget-create/releases/download/$wcVersion/wingetcreate.exe"
+          $actual = (Get-FileHash wingetcreate.exe -Algorithm SHA256).Hash.ToLower()
+          if ($actual -ne $wcSha256) {
+            Write-Error "wingetcreate.exe hash mismatch: got $actual, expected $wcSha256"
+            exit 1
+          }
           .\wingetcreate.exe update Framefilter.Keyroost `
             --version $version `
             --urls $url `

SECURITY.md

@@ -29,8 +29,9 @@ What keyroost defends against:
   and bounded; a fuzzing device gets an error, not a hang or a panic.
 - **Accidental secret disclosure by the tool itself.** Secrets are never
   written to disk; `--debug` traces redact secret-bearing command bodies;
-  secret-typed memory (PINs, seeds, RSA components, session tokens) is
-  zeroized on drop where Rust allows; secrets are accepted via env/stdin
+  PINs, CTAP session secrets, and RSA key components are zeroized on drop
+  (imported TOTP seeds passing through vault/QR import buffers are not yet
+  — tracked in TODO-hardening.md); secrets are accepted via env/stdin
   rather than argv.
 
 What keyroost does **not** defend against:

TODO-hardening.md

@@ -22,6 +22,9 @@ done and committed on this branch.
 
 - [x] Zeroize `PinUvAuthToken` and the CTAP shared secrets on drop
 - [x] Zeroize CLI-side secret strings (`read_secret` / `gather_secret` returns)
+- [ ] Zeroize imported TOTP seeds: `BulkEntry.secret`, the decrypted Aegis
+      vault plaintext, and GA migration entry buffers currently drop without
+      wiping (SECURITY.md is scoped to match until this lands)
 
 ## Phase 4 — documentation
 
@@ -57,8 +60,9 @@ index propagation:
 
 1. `keyroost-proto`, `keyroost-hid`, `keyroost-keyring`, `keyroost-rsakey`
 2. `keyroost-ctap`, `keyroost-oath`, `keyroost-openpgp`, `keyroost-piv`,
-   `keyroost-import`, `keyroost-resolve`
-3. `keyroost-qr`, `keyroost-transport`
+   `keyroost-import`
+3. `keyroost-transport` (needs proto/oath/openpgp/piv), then
+   `keyroost-resolve` (needs transport) and `keyroost-qr` (needs import)
 4. `keyroostctl`, `keyroost`
 
 Afterwards `cargo install keyroostctl` / `cargo install keyroost` work for
@@ -78,6 +82,17 @@ anyone with the Linux build prerequisites from the README.
 - [ ] **Branch/tag protection** — repo settings, must be done in the GitHub
       UI by an admin: protect `main` (require PR + green CI), protect `v*`
       tags (maintainers only; tag push is release authority).
+- [ ] **GUI: move slow imports off the frame loop** — QR image decode and
+      Aegis scrypt decryption run synchronously inside `update()`, freezing
+      the window (seconds for a stock vault, minutes at the scrypt caps);
+      route through the existing job mechanism, mindful that the worker
+      thread serializes device I/O.
+- [ ] **Wayland clipboard clear** — the conditional clear reads via
+      arboard's X11 backend; on pure-Wayland sessions without XWayland
+      clipboard sync it fails open and never clears. No complete fix known
+      (wl-data-control is wlroots-only); document or detect.
+- [ ] **CI cache for fuzz/audit jobs** — both `cargo install` cargo-fuzz /
+      cargo-audit from source every run; cache the binaries.
 - [x] **Clipboard conditional clear** — done via arboard (already in the
       tree through eframe): clears only when the clipboard still holds the
       copied code; fails open if unreadable.

crates/keyroost-keyring/src/lib.rs

@@ -230,13 +230,15 @@ impl Keyring {
             Ok(s) => {
                 let mut ring: Keyring =
                     serde_json::from_str(&s).map_err(|e| KeyringError::Parse(e.to_string()))?;
-                // `add` validates names before they ever reach disk, so
-                // re-validate on the way back in: a hand-edited file could
-                // otherwise inject ANSI escape sequences that get echoed to
-                // the terminal in error messages and listings. Free-text
-                // fields are sanitized rather than rejected.
+                // `add` validates names before they ever reach disk; on the
+                // way back in every field — including the name — is sanitized
+                // rather than rejected. Rejecting would make one hand-edited
+                // entry render the whole registry unloadable, and callers
+                // that fall back to an empty registry on error would then
+                // overwrite keys.json and destroy every other entry on the
+                // next save.
                 for entry in &mut ring.keys {
-                    validate_name(&entry.name)?;
+                    strip_control_chars(&mut entry.name);
                     strip_control_chars(&mut entry.serial);
                     for field in [&mut entry.vendor, &mut entry.aaguid, &mut entry.note]
                         .into_iter()
@@ -475,18 +477,22 @@ mod tests {
     }
 
     #[test]
-    fn load_rejects_invalid_names_and_strips_control_chars() {
+    fn load_sanitizes_invalid_names_and_strips_control_chars() {
         let dir = std::env::temp_dir().join(format!("keyroost-test-{}", std::process::id()));
         std::fs::create_dir_all(&dir).unwrap();
         let path = dir.join("keys.json");
 
-        // A name with an ANSI escape must fail validation on load.
+        // A name with an ANSI escape is sanitized, not fatal: one bad
+        // hand-edited entry must never make the whole registry unloadable
+        // (an unwrap_or_default + save would wipe every other entry).
         std::fs::write(
             &path,
-            "{\"keys\":[{\"name\":\"evil\\u001b[31m\",\"serial\":\"S1\"}]}",
+            "{\"keys\":[{\"name\":\"evil\\u001b[31m\",\"serial\":\"S1\"},{\"name\":\"good\",\"serial\":\"S2\"}]}",
         )
         .unwrap();
-        assert!(Keyring::load_from(&path).is_err());
+        let k = Keyring::load_from(&path).unwrap();
+        assert_eq!(k.keys[0].name, "evil[31m");
+        assert_eq!(k.keys[1].name, "good");
 
         // Control chars in free-text fields are stripped, not fatal.
         std::fs::write(

crates/keyroost-qr/src/lib.rs

@@ -111,30 +111,65 @@ pub fn entries_from_image(bytes: &[u8]) -> Result<QrImport, QrError> {
         batch: None,
     };
     let mut any_otpauth = false;
+    // A malformed payload must not abort the whole image: a screenshot can
+    // hold several QR codes and one damaged code shouldn't discard the
+    // accounts decoded from the others. Failures only become the result
+    // when no payload in the image yielded anything.
+    let mut payload_err: Option<String> = None;
+    let mut any_parsed = false;
     for text in &texts {
         if migration::is_migration_uri(text) {
             any_otpauth = true;
-            let m = migration::parse(text).map_err(|e| QrError::Payload(e.to_string()))?;
-            import.entries.extend(m.entries);
-            import.skipped.extend(m.skipped);
-            import.batch = import.batch.or(m.batch);
+            match migration::parse(text) {
+                Ok(m) => {
+                    any_parsed = true;
+                    import.entries.extend(m.entries);
+                    import.skipped.extend(m.skipped);
+                    import.batch = import.batch.or(m.batch);
+                }
+                Err(e) => {
+                    payload_err.get_or_insert(e.to_string());
+                    import.skipped.push(migration::Skipped {
+                        label: "damaged QR code".into(),
+                        reason: "payload could not be parsed",
+                    });
+                }
+            }
         } else if text.trim_start().starts_with("otpauth://") {
             any_otpauth = true;
-            let parsed: OtpAuth = keyroost_import::parse_otpauth(text.trim())
-                .map_err(|e| QrError::Payload(e.to_string()))?;
-            import.entries.push(BulkEntry {
-                issuer: parsed.issuer,
-                account: parsed.account,
-                secret: parsed.secret,
-                algorithm: parsed.algorithm,
-                digits: parsed.digits,
-                time_step: parsed.time_step,
-            });
+            match keyroost_import::parse_otpauth(text.trim()) {
+                Ok(parsed) => {
+                    let parsed: OtpAuth = parsed;
+                    any_parsed = true;
+                    import.entries.push(BulkEntry {
+                        issuer: parsed.issuer,
+                        account: parsed.account,
+                        secret: parsed.secret,
+                        algorithm: parsed.algorithm,
+                        digits: parsed.digits,
+                        time_step: parsed.time_step,
+                    });
+                }
+                Err(e) => {
+                    payload_err.get_or_insert(e.to_string());
+                    import.skipped.push(migration::Skipped {
+                        label: "damaged QR code".into(),
+                        reason: "payload could not be parsed",
+                    });
+                }
+            }
         }
     }
     if !any_otpauth {
         return Err(QrError::NotOtpauth);
     }
+    if !any_parsed {
+        if let Some(e) = payload_err {
+            // Every otpauth payload in the image was unreadable — surface
+            // the first underlying error instead of an empty success.
+            return Err(QrError::Payload(e));
+        }
+    }
     Ok(import)
 }
 

crates/keyroostctl/src/main.rs

@@ -1351,7 +1351,22 @@ fn run() -> Result<(), Box<dyn std::error::Error>> {
                 for s in &import.skipped {
                     eprintln!("skipped {:?}: {}", s.label, s.reason);
                 }
+                // A GA export can span several QR images; a clean single-slot
+                // import of QR 1 must not read as "migration complete".
+                if let Some((i, n)) = import.batch {
+                    eprintln!(
+                        "note: this is QR {} of {} in the export — import the other images too",
+                        i + 1,
+                        n
+                    );
+                }
                 match import.entries.len() {
+                    0 => {
+                        return Err(
+                            "QR decoded, but no account could be imported (see skips above)"
+                                .into(),
+                        )
+                    }
                     1 => import.entries.into_iter().next().unwrap(),
                     n => {
                         return Err(format!(

udev/70-keyroost-fido.rules

@@ -12,11 +12,17 @@
 # instead of relying on group membership.
 
 # Generic FIDO/U2F devices matched by HID usage page (0xF1D0) — works for any
-# vendor as long as their report descriptor follows the spec. systemd's hwdb
-# sets ID_SECURITY_TOKEN=1 from the usage page; matching on it (rather than
-# bInterfaceClass=="03", which is *every* HID device) keeps keyboard/mouse
-# hidraw nodes from becoming user-accessible — raw read access to a keyboard
-# node is a keylogger.
+# vendor as long as their report descriptor follows the spec. systemd-udevd's
+# fido_id builtin (60-fido-id.rules, systemd v244+) sets ID_SECURITY_TOKEN=1
+# from the usage page; matching on it (rather than bInterfaceClass=="03",
+# which is *every* HID device) keeps keyboard/mouse hidraw nodes from
+# becoming user-accessible — raw read access to a keyboard node is a
+# keylogger.
+#
+# Caveat: on non-systemd udev (eudev — Alpine, Devuan, some Gentoo) fido_id
+# never runs, so this generic rule matches nothing and only the explicit
+# vendor rules below apply. Keys from other vendors on such systems need
+# their own vendor-ID rule added here.
 KERNEL=="hidraw*", SUBSYSTEM=="hidraw", \
     ENV{ID_SECURITY_TOKEN}=="1", \
     TAG+="uaccess", GROUP="plugdev", MODE="0660"