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() {