diff --git a/CHANGELOG.md b/CHANGELOG.md index c986138..3bb6f74 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## [Unreleased] + +### Fixed + +- The online key service ignores `HTTP_PROXY`/`HTTPS_PROXY`/`ALL_PROXY`: with one set, every key lookup failed (the pinned resolver also answered the proxy's address, so the proxy request went to the key service). + +### Changed + +- `KeydbSource` hands out host certs best first, with no disc input: certs with no `Revoked in MKBv` note in keydb order, then revoked ones by latest revocation. The inherent `host_certs()` and the `KeySource::host_certs` trait method both return this order; the trait ignores its `mkb` argument. + +### Deprecated + +- `KeyDb::host_certs(mkb)`: use `KeyDb::host_certs_ranked`. + +### Added + +- `KeyDb::host_certs_ranked`. +- `check_keyserver_url` with `KeyserverUrlRejection`/`KeyserverUrlFault`: a typed Permanent vs Temporary verdict, so callers need not match `validate_keyserver_url`'s text. +- `test-hooks` feature exposing `set_last_decode_reachability`, to test decode-verdict handling without a network. + ## [1.7.7] — 2026-09-26 ### Maintenance diff --git a/Cargo.toml b/Cargo.toml index 735e8fa..13a9b99 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -30,6 +30,10 @@ zip = { version = "8", default-features = false, features = ["deflate"] } # key material). tracing = "0.1" +[features] +# `set_last_decode_reachability`, for consumers' tests only (enable in dev-dependencies). +test-hooks = [] + [profile.release] lto = "thin" codegen-units = 1 diff --git a/src/keydb.rs b/src/keydb.rs index 6af4633..dc5bba7 100644 --- a/src/keydb.rs +++ b/src/keydb.rs @@ -324,13 +324,11 @@ impl KeydbSource { /// live-drive scan as `DriveCredentials` for the AACS handshake. Empty if /// the keydb is missing/unreadable or carries no host cert. /// - /// Inherent, no-MKB form: this is used by the **scan-options** builder, - /// which runs before the disc's MKB generation is known, so no revocation - /// filtering is applied (passes `None`). The [`KeySource::host_certs`] TRAIT - /// method wires the real MKB generation through for revocation filtering. + /// Best first ([`KeyDb::host_certs_ranked`]): this is what the scan-options + /// builder hands the live-drive handshake as `DriveCredentials`. pub fn host_certs(&self) -> Vec { match self.cached_db() { - Ok(db) => db.host_certs(None), + Ok(db) => db.host_certs_ranked(), // 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) => { @@ -631,11 +629,10 @@ impl KeySource for KeydbSource { } } - // Expose the keydb's host certs through the trait, wiring the disc's MKB generation through - // for revocation filtering. - fn host_certs(&self, mkb: Option) -> Vec { + // The keydb's host certs, best first. `mkb` is ignored: cert selection takes no disc input. + fn host_certs(&self, _mkb: Option) -> Vec { match self.cached_db() { - Ok(db) => db.host_certs(mkb), + Ok(db) => db.host_certs_ranked(), // Vec-returning trait method: log the failure, return nothing. Err(e) => { let _ = self.load_failure(&e); @@ -1192,6 +1189,46 @@ mod tests { ); } + /// Best-first with no disc input: unannotated certs in file order, then revoked + /// ones by latest revocation. The trait ignores its MKB argument. + #[test] + fn host_certs_ranked_orders_unannotated_then_latest_revocation_first() { + let dir = scratch("ranked"); + let path = dir.join("keydb.cfg"); + let hc = |marker: u8, note: &str| { + format!( + "| HC | HOST_PRIV_KEY 0x{} | HOST_CERT 0x{}{}\n", + "00".repeat(20), + format!("{marker:02x}").repeat(92), + note + ) + }; + let cfg = [ + hc(0x68, " ; Revoked in MKBv68"), + hc(0xA1, ""), + hc(0x72, " ; Revoked in MKBv72"), + hc(0xB2, ""), + ] + .concat(); + std::fs::write(&path, cfg).unwrap(); + let src = KeydbSource::new(&path); + let markers = + |certs: Vec| -> Vec { certs.iter().map(|c| c.certificate[0]).collect() }; + let want = vec![0xA1, 0xB2, 0x72, 0x68]; + assert_eq!( + markers(src.host_certs()), + want, + "inherent (production credentials)" + ); + assert_eq!( + markers(KeySource::host_certs(&src, Some(99))), + want, + "trait ignores mkb" + ); + assert_eq!(markers(KeySource::host_certs(&src, None)), want); + std::fs::remove_dir_all(&dir).ok(); + } + /// No keydb → no host credentials, not an error (inherent and trait forms). #[test] fn host_certs_empty_when_keydb_missing() { @@ -1201,8 +1238,8 @@ mod tests { assert!(KeySource::host_certs(&src, Some(68)).is_empty()); } - /// The TRAIT `host_certs` surfaces a `| HC |` row and now wires the MKB - /// generation through. Placeholder all-zero material (never a real key). + /// The TRAIT `host_certs` surfaces a `| HC |` row for any `mkb` argument. + /// Placeholder all-zero material (never a real key). #[test] fn trait_host_certs_returns_keydb_hc_row() { let dir = std::env::temp_dir().join(format!("fmk_hc_{}", std::process::id())); diff --git a/src/keydb_format.rs b/src/keydb_format.rs index fd1f864..0224a11 100644 --- a/src/keydb_format.rs +++ b/src/keydb_format.rs @@ -477,6 +477,7 @@ impl KeyDb { /// is `None` the disc's generation is unknown and cannot be filtered, so /// every cert is returned; certs with no revocation annotation are always /// returned. + #[deprecated(note = "use host_certs_ranked; host cert selection takes no disc input")] pub fn host_certs(&self, mkb: Option) -> Vec { self.host_certs .iter() @@ -489,6 +490,19 @@ impl KeyDb { .collect() } + /// Every host cert, best first, with no disc input: certs with no `Revoked in + /// MKBv` note in file order, then revoked ones by latest revocation (a later + /// revocation is the better bet without knowing the disc's generation). + pub fn host_certs_ranked(&self) -> Vec { + let mut ranked: Vec<&KeydbHostCert> = self.host_certs.iter().collect(); + // Stable sort: ties (every unannotated cert, equal revocations) keep file order. + ranked.sort_by_key(|hc| { + let r = hc.revoked_at_mkb; + (r.is_some(), std::cmp::Reverse(r.unwrap_or(0))) + }); + ranked.into_iter().map(|hc| hc.cert.clone()).collect() + } + /// Standalone keydb accessor: the disc's Volume ID (the keydb `I` token), /// looked up by the same disc-hash form [`Self::find_disc`] accepts. Pure /// file lookup; no crypto/derivation. @@ -1913,6 +1927,7 @@ mod tests { // ── Host-cert revocation: parse + host_certs(mkb) filter ──────────────── #[test] + #[allow(deprecated)] fn host_cert_revoked_parses_and_filters_by_mkb() { let revoked_line = format!( "| HC | HOST_PRIV_KEY 0x{} | HOST_CERT 0x{} ; Revoked in MKBv72", @@ -1942,6 +1957,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn host_cert_without_revocation_included_for_all_mkb() { let line = format!( "| HC | HOST_PRIV_KEY 0x{} | HOST_CERT 0x{}", @@ -1958,6 +1974,7 @@ mod tests { } #[test] + #[allow(deprecated)] fn hc2_revocation_propagates_when_hc_has_none() { // The HC line carries no annotation; the revocation lives on the HC2 // line. The combined cert must still be filtered by that generation @@ -2178,6 +2195,7 @@ mod tests { /// so the returned SET is identified exactly, not merely counted. A cert /// revoked at generation R is usable iff the disc's generation is `< R`. #[test] + #[allow(deprecated)] fn host_certs_filters_several_certs_across_mkb_generations() { let hc = |marker: u8, note: &str| { format!( diff --git a/src/lib.rs b/src/lib.rs index 71667d5..da15f00 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,9 +16,11 @@ mod paths; pub use keydb::{KeydbSource, UpdateResult}; pub use keydb_format::{DiscEntry, KeyDb}; +#[cfg(feature = "test-hooks")] +pub use online::set_last_decode_reachability; pub use online::{ - DecodeReachability, MIN_SAMPLE_UNITS, OnlineSource, take_last_decode_reachability, - validate_keyserver_url, + DecodeReachability, KeyserverUrlFault, KeyserverUrlRejection, MIN_SAMPLE_UNITS, OnlineSource, + check_keyserver_url, take_last_decode_reachability, validate_keyserver_url, }; pub use paths::{default_keydb_path, existing_keydb_path, keydb_search_paths}; @@ -134,9 +136,9 @@ impl KeySource for MultiSource { 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 - /// generation). Without this a composed source would hide an inner source's - /// cert from the OEM cert-auth route — the gap this fixes. + /// UNION every inner source's host certs, in source order (`mkb` is passed + /// through unchanged). Without this a composed source would hide an inner + /// source's cert from the OEM cert-auth route — the gap this fixes. fn host_certs(&self, mkb: Option) -> Vec { self.sources .iter() diff --git a/src/online.rs b/src/online.rs index f4b2994..056dbec 100644 --- a/src/online.rs +++ b/src/online.rs @@ -243,7 +243,55 @@ fn resolve_and_guard(url: &str) -> Result, (GuardFail, String)> /// /// The *config-time* check; [`OnlineSource`] re-guards before each POST, closing the DNS-rebind window. pub fn validate_keyserver_url(url: &str) -> Result<(), String> { - resolve_and_guard(url).map(|_| ()).map_err(|(_, msg)| msg) + check_keyserver_url(url).map_err(|r| r.message) +} + +/// Whether a rejected key-service URL can start working without a config change. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[non_exhaustive] +pub enum KeyserverUrlFault { + /// Bad scheme, host or port, or a non-public address: retrying changes nothing. + Permanent, + /// The host did not resolve (DNS failure, timeout, lookup cap): may succeed later. + Temporary, +} + +/// Why [`check_keyserver_url`] rejected a URL; `message` is [`validate_keyserver_url`]'s text. +#[derive(Debug, Clone, PartialEq, Eq)] +#[non_exhaustive] +pub struct KeyserverUrlRejection { + pub fault: KeyserverUrlFault, + pub message: String, +} + +impl KeyserverUrlRejection { + pub fn is_temporary(&self) -> bool { + self.fault == KeyserverUrlFault::Temporary + } +} + +impl std::fmt::Display for KeyserverUrlRejection { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} + +impl std::error::Error for KeyserverUrlRejection {} + +impl From<(GuardFail, String)> for KeyserverUrlRejection { + fn from((fail, message): (GuardFail, String)) -> Self { + let fault = match fail { + GuardFail::Config => KeyserverUrlFault::Permanent, + GuardFail::Unreachable => KeyserverUrlFault::Temporary, + }; + Self { fault, message } + } +} + +/// [`validate_keyserver_url`] with a typed verdict: [`KeyserverUrlFault::Temporary`] only when +/// the host lookup failed, so a caller can keep the source and retry at request time. +pub fn check_keyserver_url(url: &str) -> Result<(), KeyserverUrlRejection> { + resolve_and_guard(url).map(|_| ()).map_err(Into::into) } // ureq's `ResolvedSocketAddrs` is a fixed 16-slot array; `push`ing a 17th @@ -287,6 +335,9 @@ fn hardened_agent(pinned: Vec) -> ureq::Agent { // a body deadline a stalled reply hangs the POST forever. The JSON key // answer arrives in one read, so a TOTAL cap is the right shape here. .timeout_recv_body(Some(Duration::from_secs(TIMEOUT_SECS))) + // Never the env proxy: PinnedResolver would also answer the proxy's lookup with the key + // service's address, so with HTTP(S)_PROXY/ALL_PROXY set every key lookup failed. + .proxy(None) .build(); // `with_parts`, never `new_with_config` — see [`PinnedResolver`]. ureq::Agent::with_parts(config, DefaultConnector::new(), PinnedResolver(pinned)) @@ -495,6 +546,13 @@ pub fn take_last_decode_reachability() -> Option { LAST_DECODE_REACHABILITY.with(std::cell::Cell::take) } +/// Set this thread's decode-reachability slot, as a real decode POST would. Test hook (feature +/// `test-hooks`): lets a caller test its [`take_last_decode_reachability`] handling offline. +#[cfg(any(test, feature = "test-hooks"))] +pub fn set_last_decode_reachability(outcome: Option) { + LAST_DECODE_REACHABILITY.with(|c| c.set(outcome)); +} + // Map a key-service HTTP status into the operator action it implies: 401/403 // fix credentials, 429 back off, 5xx wait — none of them is "no key" (the // genuine miss is a 200 with an empty body), the original bug this fixes. @@ -994,6 +1052,89 @@ mod tests { ); } + // ureq 3 defaults to `Proxy::try_from_env()`; PinnedResolver would send the proxy connection to + // the key service's address, failing every lookup. Checked in a child so no test mutates env. + #[test] + fn the_key_service_agent_never_uses_an_environment_proxy() { + const CHILD: &str = "FMKV_KS_PROXY_CHILD"; + const NAME: &str = "online::tests::the_key_service_agent_never_uses_an_environment_proxy"; + if std::env::var_os(CHILD).is_some() { + assert!( + hardened_agent(Vec::new()).config().proxy().is_none(), + "the pinned agent picked up a proxy from the environment" + ); + return; + } + let out = std::process::Command::new(std::env::current_exe().unwrap()) + .args([NAME, "--exact", "--test-threads=1"]) + .env(CHILD, "1") + .env("ALL_PROXY", "http://proxy.example:3128") + .env("HTTPS_PROXY", "http://proxy.example:3128") + .env("HTTP_PROXY", "http://proxy.example:3128") + .output() + .unwrap(); + let stdout = String::from_utf8_lossy(&out.stdout); + assert!( + out.status.success() && stdout.contains("1 passed"), + "child run failed:\n{stdout}\n{}", + String::from_utf8_lossy(&out.stderr) + ); + } + + // The typed verdict autorip needs instead of matching message text: a standing fault + // (scheme, host, port, blocked address) is Permanent; only a failed lookup is Temporary. + #[test] + fn check_keyserver_url_types_permanent_rejections() { + for url in [ + "http://8.8.8.8/keys", + "ftp://example.com/keys", + "https:///keys", + "https://8.8.8.8:notaport/keys", + "https://[::1/keys", + "https://127.0.0.1/keys", + "https://169.254.169.254/latest/meta-data", + ] { + let r = check_keyserver_url(url).expect_err(url); + assert_eq!(r.fault, KeyserverUrlFault::Permanent, "{url}"); + assert!(!r.is_temporary(), "{url}"); + assert_eq!(Err(r.message.clone()), validate_keyserver_url(url), "{url}"); + assert_eq!(r.to_string(), r.message); + } + assert_eq!(check_keyserver_url("https://8.8.8.8/keys"), Ok(())); + } + + #[test] + fn a_failed_lookup_is_a_temporary_rejection() { + // Offline: every GuardFail::Unreachable reason resolve_and_guard produces. + for msg in [ + "DNS resolution timed out", + "could not resolve host: failed to lookup address information", + "host did not resolve to any address", + "too many concurrent DNS resolutions in flight for this host", + ] { + let r = KeyserverUrlRejection::from((GuardFail::Unreachable, msg.to_string())); + assert_eq!(r.fault, KeyserverUrlFault::Temporary, "{msg}"); + assert!(r.is_temporary()); + assert_eq!(r.message, msg); + } + let c = KeyserverUrlRejection::from((GuardFail::Config, "URL has no host".into())); + assert_eq!(c.fault, KeyserverUrlFault::Permanent); + } + + // Callers test their verdict mapping with the hook instead of a real DNS lookup. + #[test] + fn the_decode_reachability_hook_plants_what_take_reads() { + set_last_decode_reachability(Some(DecodeReachability::Status(422))); + assert_eq!( + take_last_decode_reachability(), + Some(DecodeReachability::Status(422)) + ); + assert_eq!(take_last_decode_reachability(), None); + set_last_decode_reachability(Some(DecodeReachability::Transport)); + set_last_decode_reachability(None); + assert_eq!(take_last_decode_reachability(), None); + } + // ── bearer_header ────────────────────────────────────────────────────── #[test] diff --git a/tests/key_sources.rs b/tests/key_sources.rs index 107dc84..b1d77b3 100644 --- a/tests/key_sources.rs +++ b/tests/key_sources.rs @@ -138,7 +138,7 @@ fn keydb_source_label_is_keydb() { assert_eq!(KeydbSource::new("/nonexistent/keydb.cfg").label(), "keydb"); } -// ── KeydbSource: MKB-aware host-cert serving from a file ──────────────────── +// ── KeydbSource: host-cert serving from a file ────────────────────────────── #[test] fn keydb_source_serves_host_cert_from_hc_row() { @@ -152,14 +152,13 @@ fn keydb_source_serves_host_cert_from_hc_row() { let path = s.write("keydb.cfg", &line); let src = KeydbSource::new(&path); - // Inherent (no-MKB, scan-options) form. + // Inherent (scan-options) form. assert_eq!( src.host_certs().len(), 1, "inherent host_certs sees the HC row" ); - // Trait form now wires the MKB generation through (no revocation annotation - // → always returned). + // Trait form: `mkb` is ignored; the ranked list is returned. let via_trait = KeySource::host_certs(&src, Some(70)); assert_eq!(via_trait.len(), 1, "trait host_certs sees the HC row"); assert_eq!(via_trait[0].certificate.len(), 92);