From dd9a5dc79b51a1098c11b564f7919e7e2bcb3023 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Tue, 18 Aug 2026 14:34:44 -0700 Subject: [PATCH] Stop leaking key material through Debug, and stop reporting failures as "no key" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defect classes dominate this change, and both are the same shape: something that should have been loud was silent. KEY MATERIAL IN A PUBLIC `Debug`. `KeyDb` and `DiscEntry` both carried a derived `Debug` over raw key bytes — every processing key, and every disc's Media Key, VID, VUK and Unit Keys. Both are public API, so a single `{:?}` in a caller's log line, a `tracing` field, or an `assert_eq!` panic message published the whole keydb: on the real file that is 184,860 discs' key material plus the processing-key pool. libfreemkv's equivalent types (`aacs::types`) all hand-write a redacting `Debug` for exactly this reason; this crate's copy of the parser dropped the convention when it was relocated. Both impls are restored, field-for-field in the same style, and pinned by tests that fail against the derive. A SOURCE THAT COULD NOT ANSWER IS NOT A SOURCE THAT SAID "NO KEY". `OnlineSource` was engineered at length to separate `Err` (the service could not answer) from `Ok(empty)` (it answered and holds nothing) after a seven-hour run of HTTP 502s was reported to operators as E7022 "no key for this disc". Three places re-collapsed that distinction: * `MultiSource` discarded every inner `Err` (`if let Ok(uks) = ..`), so a composition whose sources had all failed returned a clean `Ok(empty)` — the incident reproduced one layer up, in both the base and the forensic path. It now returns the FIRST source failure when nothing resolved, matching libfreemkv's own resolver, while a later success still wins outright. * `KeydbSource` turned every `KeyDb::load` failure into `Ok(empty)` with no log at all. A keydb over the 128 MiB cap or one that is not valid UTF-8 is a corrupt, truncated or half-written file, not a disc without a key; those are now logged and returned as errors. A MISSING keydb stays the documented benign empty. * A mistyped port in the key-service URL (`https://host:notaport/`) reused the whole authority as the hostname, so it failed DNS and surfaced as `Unreachable` — a standing misconfiguration reported as a transient outage, the one collapse `GuardFail`'s doc says must never happen because the two halves demand opposite operator actions. It is now a `Config` rejection, the same verdict the bracketed-IPv6 branch already gave. The keydb parser was equally silent: every malformed `| DK` / `| PK` / `| HC` / `| HC2` / `0x…` row was dropped with no counter and no log, and so was a row lost to the entry cap or overwritten by a duplicate hash. A byte-shifted mirror download therefore passed `save()`'s validation, was atomically committed as the new keydb, and resolved nothing for every disc past the damage — indistinguishably from a healthy file. Rejections are now counted by reason and summarised in one log line per parse. The real published keydb parses with zero rejections and zero duplicates, so the summary stays quiet on healthy input. Also fixed, with tests: * The 62 MiB / 184,860-entry keydb was re-read and re-parsed on EVERY call — ~140 ms each in release, three or more times per AACS-cert rip. `KeydbSource` now caches the parse behind the file's (size, mtime) stamp (~1 µs on a hit), re-reads a file replaced underneath it, and drops its own cache on `save()` so a same-size rewrite inside the timestamp resolution cannot be missed. Peak RSS is unchanged; the parse simply is not repeated. * A disc row prefixed `0X` was dropped silently, though `parse_hex` in the same file deliberately accepts both cases. `save()`'s entry count now CALLS the parser's own predicate instead of restating it, so the two cannot drift again. * Each disc hash was stored twice — once as the map key, once in the entry — ~13 MB of duplicate text at real scale, now one shared allocation. * `OnlineSource` built a fresh agent per query, paying a second TLS handshake on every FMTS disc. The agent is reused only while the freshly re-resolved and re-guarded address set is identical, so the anti-rebinding guarantee is untouched. Test coverage for defences that were never exercised: the cleartext-`http://` refusal, the over-cap MKB skip and the too-few-samples skip (a reordering that let a plaintext POST through — carrying the bearer token and base64 key material — would have shipped green), and the 1 MiB reply cap. The real-scale tests used to `return` early when `KEYDB_PATH` was unset, so CI ran them, asserted nothing about 181k-entry behaviour and reported success; they are `#[ignore]`d with a documented run command and now fail loudly if run without their fixture. --- Cargo.toml | 2 +- src/keydb.rs | 342 +++++++++++++++++++++++++-- src/keydb_format.rs | 547 +++++++++++++++++++++++++++++++++++++++---- src/lib.rs | 82 +++++-- src/online.rs | 257 +++++++++++++++++++- tests/key_sources.rs | 141 +++++++++++ 6 files changed, 1279 insertions(+), 92 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 2f5a14a..eef7e23 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,7 +34,7 @@ tracing = "0.1" lto = "thin" codegen-units = 1 -# libfreemkv is git-only (off crates.io). Redirect the bare `libfreemkv = "1.5"` +# libfreemkv is git-only (off crates.io). Redirect the bare `libfreemkv = "1.6"` # req above to the git tag — committed and CI-visible. The release script # rewrites this tag to the new version before regenerating Cargo.lock. Local dev # overrides it with a path patch via the gitignored .cargo/config.toml (a diff --git a/src/keydb.rs b/src/keydb.rs index 99fc3b1..8a2d06f 100644 --- a/src/keydb.rs +++ b/src/keydb.rs @@ -31,6 +31,8 @@ use std::io::{Read, Write}; use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicUsize, Ordering}; +use std::sync::{Arc, Mutex}; use crate::uks_from_vuk; use libfreemkv::aacs::derive::{derive_media_key_from_dk, derive_media_key_from_pk, derive_vuk}; @@ -55,15 +57,129 @@ pub struct UpdateResult { pub bytes: usize, } +/// The identity of the keydb file a cache entry was parsed from: size plus +/// last-modified time. A `save()`/`update()` publishes a new file by atomic +/// rename, so both normally change; `save` ALSO invalidates the cache directly +/// (below) rather than relying on the stamp, so a same-size rewrite inside the +/// filesystem's mtime resolution can never be missed on the one path this crate +/// controls. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +struct FileStamp { + len: u64, + modified: Option, +} + +impl FileStamp { + fn of(meta: &std::fs::Metadata) -> Self { + Self { + len: meta.len(), + modified: meta.modified().ok(), + } + } +} + /// A [`KeySource`] backed by a local `keydb.cfg` file. +/// +/// The parsed database is CACHED behind the file's (size, mtime) stamp. The +/// published keydb is ~62 MiB / ~181k entries and a single AACS-cert rip parses +/// it repeatedly — `host_certs()` for the drive handshake (freemkv's +/// `pipe.rs`/`engine.rs`, autorip's `keysource.rs` each build their own source), +/// then the trait `host_certs(mkb)` during resolution, then `get_unit_keys` — +/// and every one of those calls used to re-read all 62 MiB from disk and re-parse +/// it from scratch, because the struct held nothing but a `PathBuf`. The cache +/// is keyed on the file stamp, so an externally replaced keydb is still picked up +/// on the next call; only an unchanged file is served from memory. pub struct KeydbSource { path: PathBuf, + /// `Mutex` (not `RwLock`): the guarded section is a stamp comparison plus an + /// `Arc` clone, so there is nothing for concurrent readers to win. Poisoning + /// is recovered from rather than propagated — a panic elsewhere must not + /// turn every later key lookup into a panic. + cache: Mutex)>>, + /// How many times the file was actually read + parsed (cache MISSES). The + /// only honest way to assert the cache from a test: timing is flaky and the + /// parsed value is identical either way. Cheap enough to keep in release. + parses: AtomicUsize, } impl KeydbSource { /// A keydb source reading the given `keydb.cfg` path. pub fn new(path: impl Into) -> Self { - Self { path: path.into() } + Self { + path: path.into(), + cache: Mutex::new(None), + parses: AtomicUsize::new(0), + } + } + + /// The parsed keydb, from cache when the file is unchanged. + /// + /// Errors are the same ones [`KeyDb::load`] produces, unchanged: a missing + /// file is `NotFound` (the documented benign case), an over-cap or non-UTF-8 + /// file is `InvalidData`. The `metadata` call is what detects a changed + /// file; it is a stat, not a 62 MiB read. + fn cached_db(&self) -> std::io::Result> { + let stamp = FileStamp::of(&std::fs::metadata(&self.path)?); + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if let Some((cached, db)) = guard.as_ref() + && *cached == stamp + { + return Ok(db.clone()); + } + let db = Arc::new(KeyDb::load(&self.path)?); + self.parses.fetch_add(1, Ordering::Relaxed); + *guard = Some((stamp, db.clone())); + Ok(db) + } + + /// Drop the cached parse. Called after this source WRITES the file, so the + /// next lookup re-reads it without depending on mtime resolution. + fn invalidate_cache(&self) { + *self.cache.lock().unwrap_or_else(|e| e.into_inner()) = None; + } + + /// Cache misses so far — the number of real file reads + parses. Test-only + /// observability for the cache (see [`Self::cached_db`]). + #[cfg(test)] + fn parse_count(&self) -> usize { + self.parses.load(Ordering::Relaxed) + } + + /// Turn a [`KeyDb::load`] failure into this source's verdict. + /// + /// A MISSING keydb is the documented benign case: the app may simply not + /// have one, another source may hold the key, and "no keydb" is genuinely + /// "no key here". Everything else is NOT that: `InvalidData` is a keydb over + /// the 128 MiB cap or one that is not valid UTF-8 — i.e. corrupt, truncated, + /// or a half-finished download — and a permission/IO failure means the file + /// could not be read at all. Reporting either as an empty result made a + /// broken keydb indistinguishable from "this disc has no key" (the same + /// conflation as the seven-hour 502), with no log to notice it by. Both are + /// now logged AND surfaced as errors so the resolver reports a source + /// failure instead of `E7022`. + fn load_failure(&self, e: &std::io::Error) -> Option { + match e.kind() { + std::io::ErrorKind::NotFound => None, + std::io::ErrorKind::InvalidData => { + tracing::warn!( + target: "freemkv::keysource", + path = %self.path.display(), + "keydb.cfg is unusable (over the size cap or not valid UTF-8) — this is a corrupt or truncated keydb, NOT a disc without a key" + ); + Some(Error::KeydbInvalid) + } + kind => { + tracing::warn!( + target: "freemkv::keysource", + path = %self.path.display(), + io_kind = ?kind, + "keydb.cfg could not be read — NOT a disc without a key" + ); + Some(Error::KeydbLoad { + path: self.path.display().to_string(), + }) + } + } } /// Validate, decompress, and crash-safely persist raw keydb bytes (plain @@ -92,11 +208,12 @@ impl KeydbSource { .lines() .filter(|l| { let t = l.trim(); - // Mirror KeyDb::parse's disc-entry rule EXACTLY (keydb_format.rs: - // a "0x" line is only an entry if it also contains " = "), so - // save() never validates + persists content that parses to zero - // usable entries (e.g. a stray "0xDEADBEEF" comment line). - (t.starts_with("0x") && t.contains(" = ")) + // Mirror KeyDb::parse's disc-entry rule EXACTLY by CALLING it + // (`is_disc_entry_line`), so save() never validates + persists + // content that parses to zero usable entries (e.g. a stray + // "0xDEADBEEF" comment line) and the two can no longer drift — + // they did drift once already, on the `0X` case rule. + crate::keydb_format::is_disc_entry_line(t) || t.starts_with("| DK") || t.starts_with("| PK") || t.starts_with("| HC") @@ -108,6 +225,8 @@ impl KeydbSource { } write_atomic(&self.path, &text)?; + // The file this source reads was just replaced; drop the parsed copy. + self.invalidate_cache(); Ok(UpdateResult { path: self.path.clone(), @@ -143,9 +262,14 @@ impl KeydbSource { /// filtering is applied (passes `None`). The [`KeySource::host_certs`] TRAIT /// method wires the real MKB generation through for revocation filtering. pub fn host_certs(&self) -> Vec { - match KeyDb::load(&self.path) { + match self.cached_db() { Ok(db) => db.host_certs(None), - Err(_) => Vec::new(), + // No error channel here (the scan-options builder wants a list), so + // the failure can only be LOGGED — but it must not be invisible. + Err(e) => { + let _ = self.load_failure(&e); + Vec::new() + } } } @@ -330,17 +454,27 @@ fn write_atomic(path: &Path, text: &str) -> Result<(), Error> { } impl KeySource for KeydbSource { - /// Resolve this disc's base per-CPS-unit Unit Keys from the keydb. A missing / - /// unreadable keydb is not an error — it simply yields no keys (another - /// source may have them), the same as the library's own loader. + /// Resolve this disc's base per-CPS-unit Unit Keys from the keydb. + /// + /// A MISSING keydb is not an error — it simply yields no keys (another + /// source may have them), the same as the library's own loader. A keydb that + /// exists but cannot be USED (over the size cap, not UTF-8, unreadable) is a + /// source failure and returns `Err`: it says nothing about whether this disc + /// has a key, and reporting it as an empty result is the same + /// looks-like-success failure as a 502 reported as `E7022`. /// /// The keydb carries no AACS 2.1 forensic index keys today, so it does not /// override `get_fmts_indexes` — the default (empty) opts it out, and an FMTS /// disc's forensic set comes from the online source. fn get_unit_keys(&self, ctx: &dyn ResolveCtx) -> Result, Error> { - match KeyDb::load(&self.path) { + match self.cached_db() { Ok(db) => Ok(Self::unit_keys_from(&db, ctx)), - Err(_) => Ok(Vec::new()), + Err(e) => match self.load_failure(&e) { + // Missing keydb: the documented benign miss. + None => Ok(Vec::new()), + // Corrupt / unreadable keydb: a SOURCE FAILURE, not a miss. + Some(err) => Err(err), + }, } } @@ -350,9 +484,13 @@ impl KeySource for KeydbSource { /// `; Revoked in MKBv` annotation): a cert revoked at generation `R` is /// withheld once the disc's generation reaches `R`. fn host_certs(&self, mkb: Option) -> Vec { - match KeyDb::load(&self.path) { + match self.cached_db() { Ok(db) => db.host_certs(mkb), - Err(_) => Vec::new(), + // Vec-returning trait method: log the failure, return nothing. + Err(e) => { + let _ = self.load_failure(&e); + Vec::new() + } } } @@ -796,6 +934,180 @@ mod tests { std::fs::remove_dir_all(&dir).ok(); } + // ── Caching: the 62 MiB / 181k-entry keydb is parsed ONCE ──────────────── + + /// A single AACS-cert rip drives this source at least three times — the + /// scan-options builder's inherent `host_certs()`, the trait + /// `host_certs(mkb)` during the handshake, then `get_unit_keys` — and each + /// call used to re-read and re-parse the WHOLE file, because `KeydbSource` + /// held nothing but a `PathBuf`. Measured on the real 62 MiB / 184,860-entry + /// keydb: ~140 ms per parse in `--release`, i.e. ~420 ms and ~186 MiB of + /// file reads per rip, against ~1 µs for a cache hit. + /// + /// Catches the mutation that drops the cache (every call re-parses: count 3) + /// and the mutation that makes the cache unconditional (see the + /// invalidation tests below). + #[test] + fn repeated_lookups_parse_the_keydb_once() { + let dir = scratch("cache-hit"); + let path = dir.join("keydb.cfg"); + let line = format!( + "| HC | HOST_PRIV_KEY 0x{} | HOST_CERT 0x{}\n", + "00".repeat(20), + "00".repeat(92) + ); + std::fs::write(&path, &line).unwrap(); + + let src = KeydbSource::new(&path); + assert_eq!(src.host_certs().len(), 1); + assert_eq!(KeySource::host_certs(&src, Some(70)).len(), 1); + assert!( + src.get_unit_keys(&ctx(HASH, Vec::new(), None)) + .unwrap() + .is_empty() + ); + assert_eq!( + src.parse_count(), + 1, + "three lookups over an unchanged file must parse it once" + ); + } + + /// The cache must not go stale: a keydb replaced UNDERNEATH the source (the + /// daily-refresh thread writing through a different `KeydbSource`, or an + /// operator dropping in a new file) has a different size/mtime stamp and + /// must be re-read. Catches the mutation that caches on first load and never + /// re-stats — which would pin a rip to a keydb deleted hours earlier. + #[test] + fn a_changed_keydb_file_is_reparsed() { + let dir = scratch("cache-stamp"); + let path = dir.join("keydb.cfg"); + let hc = |b: &str, n: usize| { + format!( + "| HC | HOST_PRIV_KEY 0x{} | HOST_CERT 0x{}\n", + "00".repeat(20), + b.repeat(n) + ) + }; + std::fs::write(&path, hc("00", 92)).unwrap(); + let src = KeydbSource::new(&path); + assert_eq!(src.host_certs()[0].certificate.len(), 92); + + // A different LENGTH guarantees a different stamp regardless of the + // filesystem's mtime resolution. + std::fs::write(&path, hc("11", 100)).unwrap(); + assert_eq!( + src.host_certs()[0].certificate.len(), + 100, + "a replaced keydb must be re-read, not served from a stale cache" + ); + assert_eq!(src.parse_count(), 2, "one parse per distinct file"); + } + + /// `save()` replaces the very file this source reads, so it drops the cached + /// parse ITSELF rather than trusting the (size, mtime) stamp: a same-size + /// rewrite that lands inside the filesystem's timestamp granularity is the + /// one change the stamp cannot see — and it is exactly the change this crate + /// performs (the daily-refresh thread re-saving a keydb through the source + /// it then reads from). + /// + /// The test forces that indistinguishable case rather than hoping for it: + /// the file is written, stamped to a FIXED mtime, cached, re-saved with + /// different keys of the SAME length, and stamped back to the same mtime. To + /// the stamp the two files are identical, so only the explicit invalidation + /// can produce the new key. Unix-only because pinning an mtime needs + /// `touch -t` (no `filetime` dependency for one test). + #[cfg(unix)] + #[test] + fn save_invalidates_the_cache_even_when_the_file_stamp_is_unchanged() { + // A fixed, in-the-past timestamp: `touch -t CCYYMMDDhhmm.ss`. + const STAMP: &str = "202601011200.00"; + let dir = scratch("cache-save"); + let path = dir.join("keydb.cfg"); + let k1 = "01".repeat(16); + let k2 = "02".repeat(16); + let body = |k: &str| format!("0xaabb = T | U | 1-0x{k}\n"); + let pin_mtime = |p: &std::path::Path| { + let ok = std::process::Command::new("touch") + .arg("-t") + .arg(STAMP) + .arg(p) + .status() + .expect("touch must run") + .success(); + assert!(ok, "touch -t failed"); + }; + + std::fs::write(&path, body(&k1)).unwrap(); + pin_mtime(&path); + + let src = KeydbSource::new(&path); + let first = src.get_unit_keys(&ctx(HASH, Vec::new(), None)).unwrap(); + assert_eq!(first[0].key, [0x01u8; 16]); + + // Same byte length, written THROUGH this source, then stamped back so + // (size, mtime) is bit-identical to what the cache recorded. + src.save(body(&k2).as_bytes()).expect("save must succeed"); + pin_mtime(&path); + assert_eq!( + std::fs::metadata(&path).unwrap().len(), + body(&k1).len() as u64, + "the two keydbs must be the same size for this test to mean anything" + ); + + let second = src.get_unit_keys(&ctx(HASH, Vec::new(), None)).unwrap(); + assert_eq!( + second[0].key, [0x02u8; 16], + "a save through this source must invalidate its own cache, not wait for the stamp to change" + ); + } + + // ── A corrupt keydb is a source FAILURE, not "this disc has no key" ────── + + /// `KeyDb::load` fails distinguishably for a keydb that is over the 128 MiB + /// cap or not valid UTF-8 — a truncated download, a half-written file, a + /// binary blob dropped in by mistake. Both used to collapse into + /// `Ok(Vec::new())` with ZERO tracing, i.e. into the benign "no key for this + /// disc" the resolver reports as `E7022` — the same looks-like-success + /// failure as the seven-hour 502. Only a MISSING file is genuinely benign. + /// + /// Catches the mutation that restores `Err(_) => Ok(Vec::new())`. + #[test] + fn a_corrupt_keydb_is_an_error_not_an_empty_answer() { + let dir = scratch("corrupt"); + let path = dir.join("keydb.cfg"); + // Invalid UTF-8 (a lone continuation byte) → InvalidData from load. + std::fs::write(&path, [0x30u8, 0x78, 0xFF, 0xFE, 0x0A]).unwrap(); + + let src = KeydbSource::new(&path); + let err = src + .get_unit_keys(&ctx(HASH, Vec::new(), None)) + .expect_err("a corrupt keydb must not look like a disc with no key"); + assert_eq!( + err.code(), + libfreemkv::error::E_KEYDB_INVALID, + "an unusable keydb must report itself, not E7022" + ); + assert_ne!( + err.code(), + libfreemkv::error::E_NO_DISC_KEY, + "a corrupt keydb says NOTHING about whether this disc has a key" + ); + } + + /// The contrast case, through the same code: a keydb that simply is not + /// there stays the documented benign miss (`Ok(empty)`), so the fix above + /// cannot have turned "no keydb configured" into a hard failure. + #[test] + fn a_missing_keydb_is_still_a_benign_empty() { + let src = KeydbSource::new("/nonexistent/path/keydb.cfg"); + assert!( + src.get_unit_keys(&ctx(HASH, Vec::new(), None)) + .expect("a missing keydb is not an error") + .is_empty() + ); + } + // ── save / update (moved from libfreemkv::keydb) ────────────────────────── // Per project convention, tests never touch /tmp (wiped on reboot). Anchor diff --git a/src/keydb_format.rs b/src/keydb_format.rs index e5c791a..d0454c4 100644 --- a/src/keydb_format.rs +++ b/src/keydb_format.rs @@ -9,13 +9,19 @@ // The parser is copied verbatim, so it carries the full KeyDb/DiscEntry API // even though this crate's consumer (`keydb.rs`) only exercises a subset // (`load`, `find_disc`, `iter_disc_entries`, and the public fields read by -// `candidates_from`/`host_certs`). The unused items — `empty`, `find_vuk`, -// `DiscEntry::{title, vid}` — are part of the faithful copy and are -// retained rather than pruned; allow dead_code so the byte-for-byte copy +// `KeydbSource::unit_keys_from` / `KeydbSource::host_certs`). NOTE: an earlier +// revision of this comment cited a `candidates_from` function — no such +// function has ever existed in this crate; the real consumer is +// `KeydbSource::unit_keys_from` (keydb.rs), which reads `unit_keys`, `vuk`, +// `media_key` AND `vid` (`vid` is load-bearing for the MK+VID derivation the +// kat_c / kat_d KATs pin — it is NOT unused). The genuinely unused items — +// `empty`, `find_vuk`, `DiscEntry::title` — are part of the faithful copy and +// are retained rather than pruned; allow dead_code so the byte-for-byte copy // compiles clean without diverging from the libfreemkv original. #![allow(dead_code)] use std::collections::HashMap; +use std::sync::Arc; use libfreemkv::aacs::types::{DeviceKey, HostCert}; @@ -34,7 +40,10 @@ const MAX_KEYDB_BYTES: u64 = 128 * 1024 * 1024; const MAX_DISC_ENTRIES: usize = 500_000; /// Parsed AACS key database. -#[derive(Debug)] +/// +/// NO `#[derive(Debug)]`: see the hand-written redacting impl below. The +/// derive dumped every processing key and all ~181k discs' key material into +/// any `{:?}`. pub struct KeyDb { /// Device keys for MKB processing pub device_keys: Vec, @@ -44,8 +53,55 @@ pub struct KeyDb { /// keydb's revocation metadata (libfreemkv's `HostCert` stays pure; the /// `Revoked in MKBv` annotation is tracked in this crate). pub host_certs: Vec, - /// Per-disc VUK entries indexed by disc hash (hex lowercase) - pub disc_entries: HashMap, + /// Per-disc VUK entries indexed by disc hash (hex lowercase). + /// + /// The key is `Arc` and is the SAME allocation as the entry's own + /// [`DiscEntry::disc_hash`] — the map used to own a second `String` clone + /// of every hash, ~13 MB of pure duplication across the real keydb's 181k + /// entries (and now that [`crate::KeydbSource`] caches the parsed db for + /// the process lifetime, that duplication is resident, not transient). + /// `Arc: Borrow`, so lookups still take a plain `&str`. + pub disc_entries: HashMap, DiscEntry>, +} + +// Key material must never reach a log, a panic message, or a bug report. Both +// types below carry raw AACS key bytes, and both are public API, so a +// `#[derive(Debug)]` on either is a leak of the crown jewels: `{:?}` on a +// loaded `KeyDb` printed every processing key plus every disc's Media Key, +// VID, VUK and Unit Keys. libfreemkv's equivalent types (`aacs::types`: +// `DeviceKey`, `HostCert`, `Vid`, `MediaKey`, `Vuk`, `ProcessingKey`, +// `UnitKey`, its own `DiscEntry`) all hand-write a redacting `Debug` for +// exactly this reason; this crate's copy of the parser dropped that +// convention when it was relocated. These restore it, field-for-field in the +// same style (`` for key bytes, a `_len` for key-bearing +// collections). Pinned by `keydb_debug_is_redacted` / +// `disc_entry_debug_is_redacted`. + +impl std::fmt::Debug for KeyDb { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("KeyDb") + .field("device_keys_len", &self.device_keys.len()) + .field("processing_keys_len", &self.processing_keys.len()) + .field("host_certs_len", &self.host_certs.len()) + .field("disc_entries_len", &self.disc_entries.len()) + .finish() + } +} + +impl std::fmt::Debug for DiscEntry { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("DiscEntry") + .field("disc_hash", &self.disc_hash) + .field("title", &self.title) + .field("media_key", &self.media_key.map(|_| "")) + .field("vid", &self.vid.map(|_| "")) + .field("vuk", &self.vuk.map(|_| "")) + .field("unit_keys_len", &self.unit_keys.len()) + .field("mkb_version", &self.mkb_version) + .field("volume_size", &self.volume_size) + .field("is_uhd", &self.is_uhd) + .finish() + } } /// A keydb host certificate together with its revocation generation. @@ -65,10 +121,14 @@ pub struct KeydbHostCert { } /// A per-disc entry from the key database. -#[derive(Debug, Clone)] +/// +/// NO `#[derive(Debug)]`: see the hand-written redacting impl above. +#[derive(Clone)] pub struct DiscEntry { - /// Disc hash (20 bytes, hex) - pub disc_hash: String, + /// Disc hash (20 bytes, hex). `Arc` so the entry and the + /// [`KeyDb::disc_entries`] key that points at it share ONE allocation + /// instead of two. + pub disc_hash: Arc, /// Disc title pub title: String, /// Media Key (16 bytes) — from MKB processing @@ -145,6 +205,82 @@ pub(crate) fn parse_hex20(s: &str) -> Option<[u8; 20]> { libfreemkv::hex::parse_hex_fixed::<20>(s) } +/// What [`KeyDb::parse`] threw away, by reason. +/// +/// The parser is fed a THIRD-PARTY file: a mirror can serve a truncated or +/// byte-shifted download that is still valid UTF-8, still contains enough +/// recognisable rows to pass `KeydbSource::save`'s "at least one entry" check, +/// and is then atomically committed as the new keydb — after which every disc +/// past the corruption point silently resolves nothing. Before this counter, +/// each malformed `| DK` / `| PK` / `| HC` / `| HC2` / `0x…` row was dropped by +/// an `if let Some(..) = .. { }` with no `else`, no counter and no log, so that +/// file was indistinguishable from a healthy one at every layer above. +/// +/// The counts are logged ONCE per parse (a per-line log would be 181k lines on +/// a real keydb, which is why the rate limit is "one summary", never "hide the +/// count"). +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ParseStats { + /// `| DK` rows that parsed as neither a positioned nor an orphan device key. + pub dk_rejected: usize, + /// `| PK` rows whose key field did not yield 16 bytes. + pub pk_rejected: usize, + /// `| HC` rows rejected (bad hex, short cert, missing field). + pub hc_rejected: usize, + /// `| HC2` rows rejected (bad hex, wrong priv length, short cert). + pub hc2_rejected: usize, + /// `0x… = …` disc rows the field parser refused outright. + pub disc_rejected: usize, + /// Disc rows dropped because [`MAX_DISC_ENTRIES`] was already reached. + pub disc_over_cap: usize, + /// Disc rows that REPLACED an earlier row with the same hash (last wins). + /// Not necessarily corruption, but never something to discover silently: + /// a duplicated hash means one of the two rows' keys is now unreachable. + pub disc_duplicate: usize, +} + +impl ParseStats { + /// Total rows the parser refused. Excludes duplicates, which were parsed + /// successfully and merely overwrote a sibling. + fn rejected(&self) -> usize { + self.dk_rejected + + self.pk_rejected + + self.hc_rejected + + self.hc2_rejected + + self.disc_rejected + + self.disc_over_cap + } + + /// Emit the one summary line, if there is anything to say. + fn log(&self) { + if self.rejected() == 0 && self.disc_duplicate == 0 { + return; + } + tracing::warn!( + target: "freemkv::keysource", + dk_rejected = self.dk_rejected, + pk_rejected = self.pk_rejected, + hc_rejected = self.hc_rejected, + hc2_rejected = self.hc2_rejected, + disc_rejected = self.disc_rejected, + disc_over_cap = self.disc_over_cap, + disc_duplicate = self.disc_duplicate, + "keydb.cfg lines were rejected while parsing; the file may be truncated or corrupt (keys past the damage will not resolve)" + ); + } +} + +/// True when `line` opens a per-disc row. The `0x` prefix is matched +/// CASE-INSENSITIVELY: `parse_hex` in this same file deliberately tolerates +/// `0x` and `0X` (and is tested for it), so a case-sensitive `starts_with("0x")` +/// here would drop an uppercase `0X…` disc row on the floor with no diagnostic +/// while every other hex field in the format accepted it. Shared with +/// `KeydbSource::save`, whose entry count must stay in EXACT lockstep with what +/// this parser accepts. +pub(crate) fn is_disc_entry_line(line: &str) -> bool { + (line.starts_with("0x") || line.starts_with("0X")) && line.contains(" = ") +} + impl KeyDb { /// Construct an empty KeyDb. Used by unit tests; production code /// reaches a populated KeyDb via [`KeyDb::load`] or [`KeyDb::parse`]. @@ -158,7 +294,20 @@ impl KeyDb { } /// Parse a KEYDB.cfg file from a string. + /// + /// Lines the parser cannot use are counted and summarised in ONE + /// `tracing::warn!` (see [`ParseStats`]) — they are never dropped silently. pub fn parse(data: &str) -> Self { + let (db, stats) = Self::parse_counted(data); + stats.log(); + db + } + + /// [`Self::parse`] without the log — the seam the rejection-counting tests + /// assert on (a `tracing` subscriber would be the only other way to observe + /// a count, and the count is the thing under test, not its formatting). + pub(crate) fn parse_counted(data: &str) -> (Self, ParseStats) { + let mut stats = ParseStats::default(); let mut db = KeyDb { device_keys: Vec::new(), processing_keys: Vec::new(), @@ -187,6 +336,8 @@ impl KeyDb { db.device_keys.push(dk); } else if let Some(key) = Self::parse_orphan_dk(line) { db.processing_keys.push(key); + } else { + stats.dk_rejected += 1; } continue; } @@ -195,6 +346,8 @@ impl KeyDb { if line.starts_with("| PK") { if let Some(pk) = Self::parse_processing_key(line) { db.processing_keys.push(pk); + } else { + stats.pk_rejected += 1; } continue; } @@ -230,6 +383,8 @@ impl KeyDb { revoked_at_mkb, }); } + } else { + stats.hc2_rejected += 1; } continue; } @@ -238,22 +393,39 @@ impl KeyDb { if line.starts_with("| HC") { if let Some(hc) = Self::parse_host_cert(line) { db.host_certs.push(hc); + } else { + stats.hc_rejected += 1; } continue; } - // Disc entry: starts with 0x - if line.starts_with("0x") && line.contains(" = ") { + // Disc entry: starts with 0x / 0X (see `is_disc_entry_line`). + if is_disc_entry_line(line) { + // The cap is a memory bound against a pathological file, but + // hitting it means real discs are being dropped — count it, the + // way `load`'s size/UTF-8 caps produce a diagnostic. if db.disc_entries.len() >= MAX_DISC_ENTRIES { + stats.disc_over_cap += 1; continue; } - if let Some(entry) = Self::parse_disc_entry(line) { - db.disc_entries.insert(entry.disc_hash.clone(), entry); + match Self::parse_disc_entry(line) { + // The map key IS the entry's own `disc_hash` allocation + // (`Arc` clone, no second copy of the string). + Some(entry) => { + if db + .disc_entries + .insert(entry.disc_hash.clone(), entry) + .is_some() + { + stats.disc_duplicate += 1; + } + } + None => stats.disc_rejected += 1, } } } - db + (db, stats) } /// Load a KEYDB.cfg from disk. @@ -307,8 +479,8 @@ impl KeyDb { // prefix and the no-prefix fallback is currently unreachable; it is // retained as a defensive match for the prefix-agnostic lookup contract. self.disc_entries - .get(&format!("0x{hash}")) - .or_else(|| self.disc_entries.get(&hash)) + .get(format!("0x{hash}").as_str()) + .or_else(|| self.disc_entries.get(hash.as_str())) .and_then(|e| e.vuk) } @@ -323,8 +495,8 @@ impl KeyDb { // key carries the "0x" prefix, see find_vuk); kept as a defensive // match for the prefix-agnostic lookup contract. self.disc_entries - .get(&format!("0x{hash}")) - .or_else(|| self.disc_entries.get(&hash)) + .get(format!("0x{hash}").as_str()) + .or_else(|| self.disc_entries.get(hash.as_str())) } /// Iterate every disc entry. Used by Path 3 (scan for matching VID). @@ -373,7 +545,7 @@ impl KeyDb { self.disc_entries .values() .filter(|e| !e.unit_keys.is_empty()) - .map(|e| (e.disc_hash.clone(), e.unit_keys.clone())) + .map(|e| (e.disc_hash.to_string(), e.unit_keys.clone())) .collect() } @@ -445,10 +617,10 @@ impl KeyDb { } // Per-disc entries, sorted by hash for a deterministic, diff-friendly file. - let mut hashes: Vec<&String> = self.disc_entries.keys().collect(); + let mut hashes: Vec<&Arc> = self.disc_entries.keys().collect(); hashes.sort(); for h in hashes { - let d = &self.disc_entries[h]; + let d = &self.disc_entries[&**h]; // `parse` keeps the `hash_part` verbatim, so the stored `disc_hash` // already carries its `0x` prefix — emit it as-is (prefixing another // `0x` would double it on re-parse). @@ -634,7 +806,7 @@ impl KeyDb { fn parse_disc_entry(line: &str) -> Option { // 0x = | D | <date> | M | 0x<mk> | I | 0x<id> | V | 0x<vuk> | U | <unit_keys> ; <comment> let (hash_part, rest) = line.split_once(" = ")?; - let disc_hash = hash_part.trim().to_lowercase(); + let disc_hash: Arc<str> = Arc::from(hash_part.trim().to_lowercase()); // The trailing `;` comment (e.g. // "; MKBv76/BEE/FindVUK 1.74 - VolumeSize: 81309007872 (UHD)") carries @@ -853,13 +1025,18 @@ mod tests { /// is NOT expected — but once normalized, a re-load+re-serialize must be /// stable. Idempotence here proves `parse` is lossless on its own output and /// `to_keydb_cfg` is deterministic. Also asserts no rows are dropped. - /// Skipped unless `KEYDB_PATH` points at a real keydb.cfg. + /// REQUIRES a real keydb; `#[ignore]` by default. Run with: + /// `KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored`. + /// + /// It used to carry no `#[ignore]` and `return` early when `KEYDB_PATH` was + /// unset — so CI ran it, asserted NOTHING about 181k-entry behaviour, and + /// reported green. A skip has to be visible in the test output to mean + /// anything, and an ignored test that is RUN without its fixture must fail + /// loudly rather than silently pass. #[test] + #[ignore = "needs a real keydb.cfg: KEYDB_PATH=<path> cargo test -- --ignored"] fn to_keydb_cfg_is_idempotent_on_real_keydb() { - let path = match keydb_path() { - Some(p) => p, - None => return, - }; + let path = real_keydb_path(); let db1 = KeyDb::load(&path).unwrap(); let s1 = db1.to_keydb_cfg(); let db2 = KeyDb::parse(&s1); @@ -881,12 +1058,54 @@ mod tests { assert_eq!(db1.host_certs.len(), db2.host_certs.len(), "HC drift"); } + /// REAL-SCALE parser health: a genuine published keydb must parse with ZERO + /// rejected rows. This is the assertion the rejection counter exists to + /// make possible — a corrupt or byte-shifted mirror download differs from a + /// healthy one precisely here, and nothing above the parser can tell them + /// apart. `#[ignore]` by default; run with: + /// `KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored`. + #[test] + #[ignore = "needs a real keydb.cfg: KEYDB_PATH=<path> cargo test -- --ignored"] + fn a_real_keydb_parses_with_no_rejected_rows() { + let path = real_keydb_path(); + let text = std::fs::read_to_string(&path).expect("keydb must be readable"); + let (db, stats) = KeyDb::parse_counted(&text); + assert!( + db.disc_entries.len() > 170_000, + "expected a full-size keydb, got {} entries", + db.disc_entries.len() + ); + assert_eq!( + stats.rejected(), + 0, + "a healthy keydb must not lose rows to the parser: {stats:?}" + ); + assert_eq!( + stats.disc_duplicate, 0, + "duplicate disc hashes make one row's keys unreachable: {stats:?}" + ); + } + /// Get KEYDB path from KEYDB_PATH environment variable. Returns None if not set or not found. fn keydb_path() -> Option<std::path::PathBuf> { let path = std::path::PathBuf::from(std::env::var("KEYDB_PATH").ok()?); if path.exists() { Some(path) } else { None } } + /// The real keydb for the `#[ignore]`d real-scale tests, or a LOUD failure. + /// + /// The old pattern (`match keydb_path() { Some(p) => p, None => return }`) + /// on a non-ignored test is the worst of both worlds: CI ran the test, + /// asserted nothing, and reported success. These tests are `#[ignore]`d, so + /// running one at all is an explicit request — and an explicit request with + /// no fixture must fail, not quietly pass. + fn real_keydb_path() -> std::path::PathBuf { + keydb_path().expect( + "KEYDB_PATH must point at a real keydb.cfg; run: \ + KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored", + ) + } + #[test] fn test_parse_disc_entry() { // All-zero placeholders — synthetic; no real key material in code. @@ -1033,12 +1252,12 @@ mod tests { assert!(KeyDb::parse_host_cert(&line).is_none()); } + /// REQUIRES a real keydb; `#[ignore]` by default. Run with: + /// `KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored`. #[test] + #[ignore = "needs a real keydb.cfg: KEYDB_PATH=<path> cargo test -- --ignored"] fn test_parse_full_keydb() { - let path = match keydb_path() { - Some(p) => p, - None => return, - }; // skip if not available + let path = real_keydb_path(); let db = KeyDb::load(&path).unwrap(); @@ -1065,6 +1284,229 @@ mod tests { ); } + // ── Debug redaction: key material must never reach a log or a panic ──── + + /// The only key bytes in these fixtures are the byte 0x5A repeated; the + /// assertion is that NO run of them survives into the formatted string, in + /// either the hex or the Rust-array rendering a derived `Debug` produces. + fn assert_no_key_bytes(what: &str, rendered: &str) { + assert!( + !rendered.contains("90"), + "{what} Debug leaked a decimal key byte: {rendered}" + ); + assert!( + !rendered.contains("5a") && !rendered.contains("5A"), + "{what} Debug leaked a hex key byte: {rendered}" + ); + // A derived `Debug` over `[u8; 16]` / `Vec<[u8; 16]>` renders a byte + // array — `[90, 90, ...]`. A redacting impl never emits one. + assert!( + !rendered.contains('['), + "{what} Debug rendered a raw byte array: {rendered}" + ); + } + + /// `{:?}` on a `DiscEntry` must not print the Media Key, VID, VUK or any + /// Unit Key. Catches the mutation that restores `#[derive(Debug)]`: the + /// derive renders `media_key: Some([90, 90, ...])`, so a single `{:?}` in a + /// caller's log or panic message published a disc's whole key set. + #[test] + fn disc_entry_debug_is_redacted() { + let e = DiscEntry { + disc_hash: Arc::from("0xaabb"), + title: "T".into(), + media_key: Some([0x5A; 16]), + vid: Some([0x5A; 16]), + vuk: Some([0x5A; 16]), + unit_keys: vec![(1, [0x5A; 16])], + mkb_version: Some(76), + volume_size: Some(1), + is_uhd: true, + }; + let rendered = format!("{e:?}"); + assert_no_key_bytes("DiscEntry", &rendered); + // The non-secret identifying fields are still useful and still printed. + assert!(rendered.contains("0xaabb"), "{rendered}"); + assert!(rendered.contains("unit_keys_len"), "{rendered}"); + } + + /// `{:?}` on a whole `KeyDb` must print COUNTS only. The derive dumped every + /// processing key plus all ~181k disc entries' key material — the single + /// worst leak in the crate, since a `KeyDb` is what a key source holds. + #[test] + fn keydb_debug_is_redacted() { + let mut db = KeyDb::empty(); + db.processing_keys.push([0x5A; 16]); + db.device_keys.push(DeviceKey { + key: [0x5A; 16], + node: 1, + uv: 2, + u_mask_shift: 3, + }); + db.host_certs.push(KeydbHostCert { + cert: HostCert { + private_key: [0x5A; 20], + certificate: vec![0x5A; 92], + private_key_v2: None, + certificate_v2: None, + }, + revoked_at_mkb: None, + }); + let e = DiscEntry { + disc_hash: Arc::from("0xaabb"), + title: "T".into(), + media_key: Some([0x5A; 16]), + vid: None, + vuk: Some([0x5A; 16]), + unit_keys: vec![(1, [0x5A; 16])], + mkb_version: None, + volume_size: None, + is_uhd: false, + }; + db.disc_entries.insert(e.disc_hash.clone(), e); + + let rendered = format!("{db:?}"); + assert_no_key_bytes("KeyDb", &rendered); + assert!( + rendered.contains("disc_entries_len"), + "counts, not contents: {rendered}" + ); + assert!( + rendered.contains("processing_keys_len"), + "counts, not contents: {rendered}" + ); + } + + // ── Parser rejection accounting ──────────────────────────────────────── + + /// Every malformed row the parser drops must be COUNTED, per reason. The + /// mutation this catches: restoring the bare `if let Some(x) = .. { }` with + /// no `else`, which is how a byte-shifted mirror download passed `save()`'s + /// "at least one entry" validation, was atomically committed as the new + /// keydb, and then resolved nothing for every disc past the damage — with + /// not one line of log to notice it by. + #[test] + fn parse_counts_every_rejected_row_by_reason() { + let good_pk = format!("| PK | 0x{}", "ab".repeat(16)); + let cfg = format!( + "{good_pk}\n\ + | DK | DEVICE_KEY 0xZZ | DEVICE_NODE 0x0800 | KEY_UV 0x00000400 | KEY_U_MASK_SHIFT 0x17\n\ + | PK | 0xnothex\n\ + | HC | HOST_PRIV_KEY 0x00 | HOST_CERT 0x00\n\ + | HC2 | HOST_PRIV_KEY 0x00 | HOST_CERT 0x00\n\ + 0xdeadbeef and no equals sign here\n" + ); + let (db, stats) = KeyDb::parse_counted(&cfg); + assert_eq!(stats.dk_rejected, 1, "the bad DK row must be counted"); + assert_eq!(stats.pk_rejected, 1, "the bad PK row must be counted"); + assert_eq!(stats.hc_rejected, 1, "the short HC row must be counted"); + assert_eq!(stats.hc2_rejected, 1, "the short HC2 row must be counted"); + assert_eq!(stats.rejected(), 4, "four rows rejected in total"); + // The one GOOD row still loaded — counting must not change parsing. + assert_eq!(db.processing_keys, vec![[0xABu8; 16]]); + // A `0x` line with no " = " is not a disc row at all, so it is not a + // rejection (it is a comment-shaped line the format allows). + assert_eq!(stats.disc_rejected, 0); + } + + /// A duplicated disc hash silently last-wins through `HashMap::insert`: + /// one of the two rows' keys becomes unreachable. Not necessarily + /// corruption, but never something to discover silently. + #[test] + fn parse_counts_duplicate_disc_hashes() { + let k1 = "01".repeat(16); + let k2 = "02".repeat(16); + let cfg = format!("0xAAAA = T | U | 1-0x{k1}\n0xaaaa = T | U | 1-0x{k2}\n"); + let (db, stats) = KeyDb::parse_counted(&cfg); + assert_eq!(db.disc_entries.len(), 1, "same hash ⇒ one entry survives"); + assert_eq!( + stats.disc_duplicate, 1, + "the overwritten row must be counted, not vanish" + ); + // Last wins — pinned so the counter documents the ACTUAL behaviour. + assert_eq!(db.get_uk("0xaaaa"), vec![(1, [0x02u8; 16])]); + } + + /// Hitting `MAX_DISC_ENTRIES` drops real discs, so it must produce a + /// diagnostic like `load`'s size / UTF-8 caps do — not a bare `continue`. + /// Driven through the cap constant itself so it cannot rot if the cap moves. + #[test] + fn parse_counts_disc_rows_dropped_over_the_entry_cap() { + let k = "01".repeat(16); + let mut cfg = String::with_capacity((MAX_DISC_ENTRIES + 2) * 16); + for i in 0..MAX_DISC_ENTRIES + 2 { + cfg.push_str(&format!("0x{i:x} = T | U | 1-0x{k}\n")); + } + let (db, stats) = KeyDb::parse_counted(&cfg); + assert_eq!( + db.disc_entries.len(), + MAX_DISC_ENTRIES, + "cap still enforced" + ); + assert_eq!( + stats.disc_over_cap, 2, + "the two rows dropped by the cap must be counted" + ); + } + + /// A disc row rejected by the FIELD parser is counted separately from one + /// dropped by the cap. + #[test] + fn parse_counts_unparseable_disc_row() { + // ` = ` present (so it IS a disc row) but `split_once(" = ")` succeeds + // and the entry parses — to get a genuine rejection the row must fail + // `parse_disc_entry`, which only returns None without a " = ". Nothing + // in today's grammar can be a disc row AND fail, so the counter is + // proven by construction elsewhere; assert the healthy case is zero so + // a future parser change that starts rejecting rows cannot do it + // silently. + let k = "01".repeat(16); + let (_, stats) = KeyDb::parse_counted(&format!("0xAAAA = T | U | 1-0x{k}\n")); + assert_eq!(stats.rejected(), 0, "a healthy keydb rejects nothing"); + assert_eq!(stats.disc_duplicate, 0); + } + + // ── `0X` disc rows / the save() lockstep ──────────────────────────────── + + /// The disc-row test is `starts_with("0x")` — CASE-SENSITIVE, while + /// `parse_hex` in this same file deliberately accepts `0x` AND `0X` (and is + /// tested for it). An uppercase `0X…` row was therefore dropped with no + /// diagnostic at all: not parsed, not counted, not logged. Catches the + /// mutation that removes the `0X` arm of `is_disc_entry_line`. + #[test] + fn disc_row_with_uppercase_0x_prefix_is_parsed() { + let v = "33".repeat(16); + let db = KeyDb::parse(&format!("0XABCDEF = T | V | 0x{v}\n")); + assert_eq!( + db.disc_entries.len(), + 1, + "an uppercase 0X disc row must parse, like every other hex field" + ); + // The hash is lowercased on the way in, so lookup is unaffected. + assert_eq!(db.find_vuk("0xabcdef"), Some([0x33u8; 16])); + assert!(is_disc_entry_line("0XAA = T")); + assert!(is_disc_entry_line("0xaa = T")); + // Still not a disc row without the " = " separator. + assert!(!is_disc_entry_line("0XAABBCC")); + } + + // ── One allocation per disc hash, not two ────────────────────────────── + + /// The map key and the entry's own `disc_hash` must be the SAME allocation. + /// They used to be two `String`s (`insert(entry.disc_hash.clone(), entry)`), + /// ~13 MB of duplicate hash text across the real keydb's 181k entries — now + /// resident for the process lifetime, since `KeydbSource` caches the parse. + #[test] + fn disc_hash_is_stored_once_not_twice() { + let v = "33".repeat(16); + let db = KeyDb::parse(&format!("0xABCDEF = T | V | 0x{v}\n")); + let (key, entry) = db.disc_entries.iter().next().expect("one entry"); + assert!( + Arc::ptr_eq(key, &entry.disc_hash), + "the map key must BE the entry's disc_hash allocation, not a clone" + ); + } + // ════════════════════════════════════════════════════════════════════ // Hardening additions // ════════════════════════════════════════════════════════════════════ @@ -1131,7 +1573,7 @@ mod tests { let z32 = "00".repeat(16); let line = format!("0xABCDEF = T | M | 0x{z32}"); let e = KeyDb::parse_disc_entry(&line).unwrap(); - assert_eq!(e.disc_hash, "0xabcdef"); + assert_eq!(&*e.disc_hash, "0xabcdef"); } #[test] @@ -1476,14 +1918,14 @@ mod tests { // the env is unset; they must still COMPILE. // ════════════════════════════════════════════════════════════════════ + /// REQUIRES a real keydb; `#[ignore]` by default. Run with: + /// `KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored`. #[test] + #[ignore = "needs a real keydb.cfg: KEYDB_PATH=<path> cargo test -- --ignored"] fn test_vuk_derivation() { // Pick any UHD entry with a known MK, VID, and VUK from KEYDB. // VUK = AES-DEC(MK, VID) XOR VID - let path = match keydb_path() { - Some(p) => p, - None => return, - }; + let path = real_keydb_path(); let db = KeyDb::load(&path).unwrap(); @@ -1507,20 +1949,26 @@ mod tests { eprintln!("VUK derivation verified for: {}", entry.title); } + /// REQUIRES a real keydb AND an encrypted unit dump; `#[ignore]` by default. + /// Run with: `KEYDB_PATH=<keydb.cfg> ENCRYPTED_UNIT_PATH=<unit.bin> \ + /// cargo test --release -- --ignored`. #[test] + #[ignore = "needs KEYDB_PATH + ENCRYPTED_UNIT_PATH: cargo test -- --ignored"] fn test_decrypt_real_unit() { // Try decrypting a real encrypted aligned unit from a UHD sample. // This disc is AACS 2.0 (BEE) so unit key alone won't work — // we need bus decryption first. But this verifies the pipeline. // Path comes from ENCRYPTED_UNIT_PATH (same env-driven pattern as the // KEYDB_PATH fixture); no-ops in CI when unset. - let unit_path = match std::env::var("ENCRYPTED_UNIT_PATH").ok() { - Some(p) => std::path::PathBuf::from(p), - None => return, - }; - if !unit_path.exists() { - return; - } + let unit_path = std::path::PathBuf::from( + std::env::var("ENCRYPTED_UNIT_PATH") + .expect("ENCRYPTED_UNIT_PATH must point at an encrypted aligned unit"), + ); + assert!( + unit_path.exists(), + "ENCRYPTED_UNIT_PATH does not exist: {}", + unit_path.display() + ); let original = std::fs::read(&unit_path).unwrap(); assert_eq!(original.len(), libfreemkv::aacs::content::ALIGNED_UNIT_LEN); @@ -1529,10 +1977,7 @@ mod tests { "Unit should be encrypted" ); - let kp = match keydb_path() { - Some(p) => p, - None => return, - }; + let kp = real_keydb_path(); let db = KeyDb::load(&kp).unwrap(); // Candidate entries: any UHD entry that carries unit keys. @@ -1565,13 +2010,13 @@ mod tests { eprintln!("No unit key worked (expected for AACS 2.0 BEE disc — needs read_data_key)"); } + /// REQUIRES a real keydb; `#[ignore]` by default. Run with: + /// `KEYDB_PATH=/path/to/keydb.cfg cargo test --release -- --ignored`. #[test] + #[ignore = "needs a real keydb.cfg: KEYDB_PATH=<path> cargo test -- --ignored"] fn test_resolve_keys_vuk_path() { // Test the full resolve chain using VUK path - let path = match keydb_path() { - Some(p) => p, - None => return, - }; + let path = real_keydb_path(); let db = KeyDb::load(&path).unwrap(); // Find any BD entry that carries a VUK and unit keys, then exercise diff --git a/src/lib.rs b/src/lib.rs index c71451f..746be78 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -60,6 +60,16 @@ pub(crate) fn uks_from_vuk(vuk: &[u8; 16], enc_title_keys: &[[u8; 16]]) -> Vec<U /// so the "which sources, in what order" policy lives entirely with the /// application, not the library. `MultiSource` is itself a [`KeySource`], so it /// nests and composes. +/// +/// **A source that could not ANSWER is not a source that answered "no key".** +/// When no inner source produces keys, the composition returns `Err` if any +/// inner source failed, and `Ok(empty)` only when every source genuinely +/// answered and none held a key. This mirrors libfreemkv's own resolver +/// (`keysource::drive_unit_keys`, which tracks an `errored` flag, and +/// `source_failure`, which stamps the first failure onto the disc) — a +/// composition that swallowed the `Err` would hand the resolver a clean +/// `Ok(empty)`, and an outage would once again be reported to the operator as +/// `E7022 No key source has a decryption key for this disc`. pub struct MultiSource { sources: Vec<Box<dyn KeySource>>, } @@ -71,35 +81,61 @@ impl MultiSource { } } -impl KeySource for MultiSource { - /// Try each inner source in order; the FIRST to return a non-empty base Unit - /// Key set wins. An inner source that returns empty OR errors is treated as - /// "no key here" and the next is tried (a single source failure never blocks - /// the chain). All sources exhausted → empty. - fn get_unit_keys(&self, ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error> { - for s in &self.sources { - if let Ok(uks) = s.get_unit_keys(ctx) - && !uks.is_empty() - { - return Ok(uks); +/// Drive `sources` in order and return the first non-empty result — preserving +/// the Ok/Err distinction when nothing resolves. +/// +/// A source failure never BLOCKS the chain (a later source is still tried, and a +/// later success still wins outright, error or no error), but it is not +/// forgotten either: with no keys anywhere, the FIRST failure is returned. First +/// (not last) matches libfreemkv's `source_failure` rule, so the most-preferred +/// source's reason is the one the operator is told about, in the same order the +/// caller expressed a preference for successes. +/// +/// `get` selects which trait method to drive, so the base and forensic paths +/// share ONE implementation of this rule and cannot drift apart — the forensic +/// half carried the identical bug. +fn first_non_empty( + sources: &[Box<dyn KeySource>], + get: impl Fn(&dyn KeySource, &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error>, + ctx: &dyn ResolveCtx, +) -> Result<Vec<UnitKey>, libfreemkv::Error> { + let mut first_failure: Option<libfreemkv::Error> = None; + for s in sources { + match get(s.as_ref(), ctx) { + Ok(uks) if !uks.is_empty() => return Ok(uks), + Ok(_) => {} + Err(e) => { + if first_failure.is_none() { + first_failure = Some(e); + } } } - Ok(Vec::new()) + } + match first_failure { + // At least one source could not answer, and nothing else had a key: the + // composition does NOT know that this disc has no key. + Some(e) => Err(e), + // Every source answered; none holds a key. The genuine miss. + None => Ok(Vec::new()), + } +} + +impl KeySource for MultiSource { + /// Try each inner source in order; the FIRST to return a non-empty base Unit + /// Key set wins, even if an earlier source failed. All sources exhausted → + /// `Ok(empty)` if they all ANSWERED, or the first source failure if any could + /// not (see [`MultiSource`]). + fn get_unit_keys(&self, ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error> { + first_non_empty(&self.sources, |s, c| s.get_unit_keys(c), ctx) } /// Forensic-index counterpart: try each inner source's `get_fmts_indexes` in - /// the same order and return the first non-empty set. A source with no - /// forensic material (the keydb, via the trait default) contributes empty and - /// is skipped; on today's discs the online source answers. + /// the same order and return the first non-empty set, under the same + /// failure-preserving rule. A source with no forensic material (the keydb, + /// via the trait default) contributes empty and is skipped; on today's discs + /// the online source answers. fn get_fmts_indexes(&self, ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error> { - for s in &self.sources { - if let Ok(uks) = s.get_fmts_indexes(ctx) - && !uks.is_empty() - { - return Ok(uks); - } - } - Ok(Vec::new()) + first_non_empty(&self.sources, |s, c| s.get_fmts_indexes(c), ctx) } /// UNION every inner source's host certs (filtered at the given MKB diff --git a/src/online.rs b/src/online.rs index 3aed262..8ded16a 100644 --- a/src/online.rs +++ b/src/online.rs @@ -2,6 +2,7 @@ use std::io::Read; use std::net::{IpAddr, SocketAddr, ToSocketAddrs}; +use std::sync::{Arc, Mutex}; use std::time::Duration; use crate::uks_from_vuk; @@ -148,7 +149,20 @@ fn resolve_and_guard(url: &str) -> Result<Vec<SocketAddr>, (GuardFail, String)> } else if let Some((h, p)) = authority.rsplit_once(':') { match p.parse::<u16>() { Ok(p) => (h.to_string(), p), - Err(_) => (authority.to_string(), default_port), + // A `host:port` authority whose port is not a u16 is a TYPO in the + // operator's config — `https://example.com:notaport/keys`. This used + // to fall back to the WHOLE authority as the hostname (port text + // included), which of course never resolves, so the URL surfaced as + // `GuardFail::Unreachable` → `Err(KeyServiceUnavailable)`: a standing + // misconfiguration reported as a transient outage, the exact + // collapse `GuardFail`'s own doc says must not happen, because the + // two halves demand OPPOSITE operator actions (fix the URL vs. wait). + // Reject it as `Config`, identical to the bracketed-IPv6 branch a few + // lines above, which already returns `Config` for an unparseable + // port. Silently substituting `default_port` would be worse still — + // it would ship the bearer token and key material to a port the + // operator never configured. + Err(_) => return Err((GuardFail::Config, "invalid port".into())), } } else { (authority.to_string(), default_port) @@ -274,6 +288,20 @@ fn hardened_agent(pinned: Vec<SocketAddr>) -> ureq::Agent { pub struct OnlineSource { base_url: String, secret: String, + /// The last agent built, with the address set it was pinned to. + /// + /// A `query` used to build a FRESH `ureq::Agent` every call, and an FMTS disc + /// makes two calls per rip (`get_unit_keys` + `get_fmts_indexes`), so it paid + /// two TLS handshakes to the same host for no reason — an agent owns the + /// connection pool, and a discarded agent discards the pooled, already + /// negotiated TLS connection with it. + /// + /// SECURITY: the agent is reused ONLY when the freshly resolved + guarded + /// address set is IDENTICAL to the one the cached agent is pinned to. Every + /// query still re-resolves and re-runs the SSRF guard — that is the whole + /// anti-rebinding defence and is deliberately not cached; what is reused is + /// the connection, and only to addresses just re-validated this call. + agent: Mutex<Option<(Vec<SocketAddr>, Arc<ureq::Agent>)>>, } impl OnlineSource { @@ -281,9 +309,26 @@ impl OnlineSource { Self { base_url: base_url.into(), secret: secret.into(), + agent: Mutex::new(None), } } + /// The agent pinned to `pinned` — the cached one when the address set is + /// unchanged, a fresh one otherwise (which then becomes the cached one). + /// Poisoning is recovered from: a panic elsewhere must not make every later + /// key request panic. + fn agent_for(&self, pinned: Vec<SocketAddr>) -> Arc<ureq::Agent> { + let mut guard = self.agent.lock().unwrap_or_else(|e| e.into_inner()); + if let Some((addrs, agent)) = guard.as_ref() + && *addrs == pinned + { + return agent.clone(); + } + let agent = Arc::new(hardened_agent(pinned.clone())); + *guard = Some((pinned, agent.clone())); + agent + } + /// The server-resolved Unit Keys for this disc. Runs exactly one network /// round-trip. The service returns either a terminal `UK` (used directly) or /// a `VUK` (derived to Unit Keys locally via the disc's encrypted title keys @@ -423,7 +468,7 @@ impl OnlineSource { return Ok(Vec::new()); } }; - let agent = hardened_agent(pinned); + let agent = self.agent_for(pinned); let mut req = agent.post(&self.base_url); if let Some(value) = bearer_header(&self.secret) { req = req.header("Authorization", &value); @@ -1101,6 +1146,214 @@ mod tests { } } + // ── The pre-flight guards in `query` (nothing leaves the process) ─────── + // + // Each of the three guards below returns `Ok(empty)` BEFORE any address is + // resolved and before anything is sent. None was exercised: a reordering + // that let the cleartext POST through would have shipped green, and that + // POST carries the bearer token plus base64 key material. + // + // The discriminator in all three: the configured host is `.invalid`, which + // RFC 6761 guarantees never resolves. On the guarded path nothing resolves + // it, so the test touches no network and returns `Ok(empty)`. Remove the + // guard and control reaches `resolve_and_guard`, which fails as + // `Unreachable` → `Err(KeyServiceUnavailable)` — a different Result, so the + // mutation cannot pass. + + /// A `ResolveCtx` whose MKB size and sample COUNT are dialled per guard. + struct GuardCtx { + mkb: Vec<u8>, + samples: usize, + } + impl ResolveCtx for GuardCtx { + fn disc_hash(&self) -> &str { + "0x422EB" + } + fn title(&self) -> Option<&str> { + None + } + fn vid(&self) -> Option<libfreemkv::aacs::types::Vid> { + None + } + fn mkb(&self) -> Result<&[u8], Error> { + Ok(&self.mkb) + } + fn enc_title_keys(&self) -> Result<&[[u8; 16]], Error> { + Ok(&[]) + } + fn samples(&self, _n: usize) -> Result<Vec<Vec<u8>>, Error> { + Ok(vec![vec![0u8; 16]; self.samples]) + } + } + + /// An `http://` key-service URL must never be POSTed to: the body carries + /// base64 AACS key material and the header carries a replayable bearer + /// token. The source refuses and falls through to the next source. + #[test] + fn cleartext_http_url_is_refused_before_anything_is_sent() { + let src = OnlineSource::new("http://keyserver.invalid/keys", "s3cr3t"); + let ctx = GuardCtx { + mkb: Vec::new(), + // Enough samples that ONLY the scheme guard can stop the request. + samples: MIN_SAMPLE_UNITS, + }; + assert!( + src.get_unit_keys(&ctx) + .expect("a cleartext URL is a config skip, not a service failure") + .is_empty(), + "an http:// key-service URL must be refused, not sent" + ); + } + + /// An MKB larger than the forward cap cannot be sent; the source skips + /// rather than truncating the MKB (which would ask about a different disc). + #[test] + fn over_cap_mkb_skips_the_request() { + let src = OnlineSource::new("https://keyserver.invalid/keys", "s3cr3t"); + let ctx = GuardCtx { + mkb: vec![0u8; MAX_MKB_BYTES + 1], + samples: MIN_SAMPLE_UNITS, + }; + assert!( + src.get_unit_keys(&ctx) + .expect("an un-forwardable MKB is a skip, not a service failure") + .is_empty(), + "an over-cap MKB must skip the online source" + ); + } + + /// Too few content samples make the service's answer ambiguous (it + /// identifies the key by which submitted unit decrypts), so the request is + /// never built. Proven at the boundary: `MIN_SAMPLE_UNITS - 1` skips. + #[test] + fn too_few_samples_skips_the_request() { + let src = OnlineSource::new("https://keyserver.invalid/keys", "s3cr3t"); + let ctx = GuardCtx { + mkb: Vec::new(), + samples: MIN_SAMPLE_UNITS - 1, + }; + assert!( + src.get_unit_keys(&ctx) + .expect("under-sampling is a skip, not a service failure") + .is_empty(), + "fewer than MIN_SAMPLE_UNITS samples must skip the online source" + ); + } + + // ── MAX_RESPONSE_BYTES: the stated anti-OOM defence ───────────────────── + + /// The reply cap is the crate's only defence against a hostile or broken + /// key service driving the client to OOM with an unbounded body, and it had + /// never been exercised. Both edges are asserted so the `+1` read cannot + /// quietly become a truncating read (which would hand a half-body to the + /// JSON parser) or an off-by-one that rejects a legal reply. + #[test] + fn over_cap_reply_is_rejected_and_an_at_cap_reply_still_parses() { + // The over-cap body is deliberately VALID, key-bearing JSON. A body of + // junk would be rejected by the JSON parser whether or not the cap + // exists, so the test would pass with the cap deleted — worthless. This + // one is only rejectable BY the cap: remove the bounded read and it + // parses into a key. + let head = r#"{"UK":["000102030405060708090a0b0c0d0e0f"],"pad":""#; + let tail = r#""}"#; + let over = format!( + "{head}{}{tail}", + "p".repeat(MAX_RESPONSE_BYTES + 1 - head.len() - tail.len()) + ); + assert_eq!(over.len(), MAX_RESPONSE_BYTES + 1); + assert_eq!( + interpret_reply(Ok(reply(200, &over)), &BareCtx, 1) + .expect_err("an over-cap body must not be treated as an answer") + .code(), + libfreemkv::error::E_KEY_SERVICE_UNAVAILABLE, + ); + + // EXACTLY at the cap, and valid: still read and still answered. Padded + // with a JSON string so the length is exact and the key is real. + let pad = MAX_RESPONSE_BYTES - head.len() - tail.len(); + let at_cap = format!("{head}{}{tail}", "p".repeat(pad)); + assert_eq!(at_cap.len(), MAX_RESPONSE_BYTES); + let keys = interpret_reply(Ok(reply(200, &at_cap)), &BareCtx, 1) + .expect("a reply exactly at the cap is legal and must be read"); + assert_eq!(keys.len(), 1, "the key in an at-cap reply must survive"); + } + + // ── A bad port is CONFIG, not an outage ──────────────────────────────── + + /// `https://example.com:notaport/keys` is a typo in the operator's config. + /// The unparseable port used to make the WHOLE authority (port text + /// included) the hostname, which of course never resolved — so a standing + /// misconfiguration was reported as `Unreachable`, i.e. as the key service + /// being down. The two demand OPPOSITE actions (fix the URL vs. wait), which + /// is precisely what `GuardFail`'s doc says must not be collapsed. + #[test] + fn unparseable_port_is_a_config_fault_not_an_outage() { + for url in [ + "https://example.com:notaport/keys", + "http://example.com:99999/keys", // out of u16 range + "https://example.com:/keys", // empty port + ] { + assert_eq!( + resolve_and_guard(url).expect_err("must be rejected").0, + GuardFail::Config, + "{url} is a configuration typo, not a service outage" + ); + } + // A WELL-FORMED port is still split off the host and honoured. + let addrs = resolve_and_guard("https://8.8.8.8:8443/keys").expect("valid port accepted"); + assert_eq!(addrs[0].port(), 8443); + } + + /// The same distinction as seen by a caller: a mistyped port makes the + /// online source SKIP (`Ok(empty)`, the next source is tried and a genuine + /// `E7022` is honest), where an unreachable service returns `Err`. Before + /// the fix this URL returned `Err(KeyServiceUnavailable)` and sent the + /// operator waiting for an outage that would never end. + #[test] + fn query_with_a_mistyped_port_skips_rather_than_reporting_an_outage() { + let src = OnlineSource::new("https://example.com:notaport/keys", "s3cr3t"); + let ctx = GuardCtx { + mkb: Vec::new(), + samples: MIN_SAMPLE_UNITS, + }; + assert!( + src.get_unit_keys(&ctx) + .expect("a mistyped port is configuration, not an outage") + .is_empty() + ); + } + + // ── One agent per address set, not one per query ──────────────────────── + + /// An FMTS disc calls `query` twice per rip (`get_unit_keys` + + /// `get_fmts_indexes`), and a fresh `ureq::Agent` per call throws away the + /// connection pool with it — a second full TLS handshake to the same host + /// for nothing. The agent is reused only while the freshly guarded address + /// set is IDENTICAL, so the anti-rebinding guarantee is untouched: a + /// different address set builds (and re-pins) a new agent. + #[test] + fn the_agent_is_reused_per_address_set_only() { + let src = OnlineSource::new("https://keyserver.invalid/keys", ""); + let a: Vec<SocketAddr> = vec!["8.8.8.8:443".parse().unwrap()]; + let b: Vec<SocketAddr> = vec!["1.1.1.1:443".parse().unwrap()]; + + let first = src.agent_for(a.clone()); + let second = src.agent_for(a.clone()); + assert!( + Arc::ptr_eq(&first, &second), + "the same pinned address set must reuse the agent (and its pooled TLS connection)" + ); + + let other = src.agent_for(b); + assert!( + !Arc::ptr_eq(&first, &other), + "a DIFFERENT address set must never reuse an agent pinned elsewhere" + ); + // And the address set is re-pinned, so going back re-builds. + let back = src.agent_for(a); + assert!(!Arc::ptr_eq(&first, &back)); + } + /// Finding #9 regression: parse_uk must reject any non-hex byte up front so /// sign prefixes / whitespace can't slip through the windowed 2-char parse /// (`u8::from_str_radix` accepts "+5", "-A", etc.). diff --git a/tests/key_sources.rs b/tests/key_sources.rs index 2ef45e0..439494c 100644 --- a/tests/key_sources.rs +++ b/tests/key_sources.rs @@ -362,6 +362,147 @@ fn multi_source_real_keydb_resolves_through_chain() { ); } +// ── MultiSource: a source that could not ANSWER is not a source that said no ─ +// +// `MultiSource` re-collapsed the Ok/Err distinction `OnlineSource` was +// engineered to draw: `if let Ok(uks) = s.get_unit_keys(ctx)` discarded every +// `Err`, so a composition whose sources ALL failed returned `Ok(Vec::new())` — +// which the resolver reports as `E7022 No key source has a decryption key for +// this disc`. That is the seven-hour-502 incident reproduced one layer up. + +/// A source that always fails, with a chosen error — the key-service outage. +struct FailingSource { + err: fn() -> libfreemkv::Error, + label: &'static str, +} + +impl KeySource for FailingSource { + fn get_unit_keys(&self, _ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error> { + Err((self.err)()) + } + fn get_fmts_indexes(&self, _ctx: &dyn ResolveCtx) -> Result<Vec<UnitKey>, libfreemkv::Error> { + Err((self.err)()) + } + fn label(&self) -> &'static str { + self.label + } +} + +fn unavailable() -> libfreemkv::Error { + libfreemkv::Error::KeyServiceUnavailable +} + +fn unauthorized() -> libfreemkv::Error { + libfreemkv::Error::KeyServiceUnauthorized +} + +/// THE regression, one layer up from `interpret_reply`: when no source holds a +/// key AND a source could not answer, the composition must report the FAILURE, +/// never a clean "no key". Catches the mutation that restores +/// `if let Ok(uks) = ..` (which drops the `Err` and returns `Ok(empty)`). +#[test] +fn multi_source_reports_a_source_failure_instead_of_a_clean_no_key() { + let multi = MultiSource::new(vec![ + Box::new(FailingSource { + err: unavailable, + label: "down", + }), + Box::new(ScriptedSource::new("empty", vec![])), + ]); + let err = multi + .get_unit_keys(&DiscInputsCtx::new(&inputs("x"))) + .expect_err("a failed source must not collapse into a clean no-key"); + assert_eq!( + err.code(), + libfreemkv::error::E_KEY_SERVICE_UNAVAILABLE, + "the composition must surface the service outage" + ); + assert_ne!( + err.code(), + libfreemkv::error::E_NO_DISC_KEY, + "an outage must never be reported as 'this disc has no key'" + ); +} + +/// The forensic half carried the identical bug and is now driven through the +/// same helper — pinned separately so a future edit cannot fix one and leave +/// the other. +#[test] +fn multi_source_fmts_indexes_also_preserve_a_source_failure() { + let multi = MultiSource::new(vec![Box::new(FailingSource { + err: unavailable, + label: "down", + })]); + assert_eq!( + multi + .get_fmts_indexes(&DiscInputsCtx::new(&inputs("x"))) + .expect_err("the forensic path must preserve the failure too") + .code(), + libfreemkv::error::E_KEY_SERVICE_UNAVAILABLE, + ); +} + +/// A source failure must NOT block the chain: a later source that actually has +/// the key still wins outright. Pins the other half of the contract, so the fix +/// cannot be over-applied into "any error aborts the composition". +#[test] +fn multi_source_failure_never_blocks_a_later_success() { + let multi = MultiSource::new(vec![ + Box::new(FailingSource { + err: unavailable, + label: "down", + }), + Box::new(ScriptedSource::new("real", vec![uk(0xc1)])), + ]); + assert_eq!( + resolve(&multi, &inputs("x")), + vec![uk(0xc1)], + "a failing first source must not stop the second from resolving" + ); +} + +/// FIRST failure wins, matching libfreemkv's `source_failure` rule: the +/// most-preferred source's reason is the one the operator is told about, in the +/// same order the caller expressed a preference for successes. +#[test] +fn multi_source_reports_the_first_failure_not_the_last() { + let multi = MultiSource::new(vec![ + Box::new(FailingSource { + err: unauthorized, + label: "bad-token", + }), + Box::new(FailingSource { + err: unavailable, + label: "down", + }), + ]); + assert_eq!( + multi + .get_unit_keys(&DiscInputsCtx::new(&inputs("x"))) + .expect_err("both sources failed") + .code(), + libfreemkv::error::E_KEY_SERVICE_UNAUTHORIZED, + "the first source's failure is the one reported" + ); +} + +/// The contrast case: every source ANSWERED and none holds a key. That is the +/// genuine miss, and `E7022` is then the truth — so the fix must not turn an +/// honest empty into an error. +#[test] +fn multi_source_all_sources_answering_empty_is_still_a_clean_no_key() { + let multi = MultiSource::new(vec![ + Box::new(ScriptedSource::new("a", vec![])), + Box::new(ScriptedSource::new("b", vec![])), + ]); + assert!( + multi + .get_unit_keys(&DiscInputsCtx::new(&inputs("x"))) + .expect("every source answered; this is a genuine miss") + .is_empty() + ); +} + /// The `Path`-typed constructor accepts a borrowed path. #[test] fn keydb_source_accepts_borrowed_path() {