From 55ef44e74e76e5f67ef0bc11883fa4dabf2f6742 Mon Sep 17 00:00:00 2001 From: Matthew Jackson <1085847+MattJackson@users.noreply.github.com> Date: Thu, 17 Sep 2026 22:37:38 -0700 Subject: [PATCH] comment-guard: trim over-cap comment blocks to caps (comments only) Restructure 10 over-cap comment blocks in keydb.rs and online.rs to fit comment-guard's caps (inline // asides <=3 lines, /// item-doc prose <=8) by tightening wording. No code changes; behavior unchanged. --- src/keydb.rs | 40 +++++++++++++++------------------------- src/online.rs | 46 +++++++++++++++++----------------------------- 2 files changed, 32 insertions(+), 54 deletions(-) diff --git a/src/keydb.rs b/src/keydb.rs index 105c56a..f0e3858 100644 --- a/src/keydb.rs +++ b/src/keydb.rs @@ -20,10 +20,9 @@ use libfreemkv::keysource::ResolveCtx; use libfreemkv::{Error, KeySource}; use crate::keydb_format::KeyDb; -// Upper bound on decompressed keydb size (decompression-bomb cap): a tiny -// zip/gz could otherwise inflate to GiB and OOM the refresh thread. The SAME -// constant `KeyDb::load` uses as the on-disk load cap — defined once in -// keydb_format so the two can never drift apart. +// Decompression-bomb cap on decompressed keydb size: a tiny zip/gz could +// inflate to GiB and OOM the refresh thread. Same constant `KeyDb::load` +// uses on-disk, defined once in keydb_format so the two can't drift. use crate::keydb_format::MAX_KEYDB_BYTES; /// Result of a KEYDB save/update -- path written, entry count, and byte size. @@ -105,14 +104,9 @@ impl CacheEntry { // the past when we stamped it, so any later write bumps the mtime // past our stamp and is detected. Trust it. Some(age) => age >= granularity, - // No mtime at all (an mtime-less filesystem) OR an mtime in the - // FUTURE relative to stamped_at (clock skew) — `duration_since` - // yielded None either way. The mtime is useless as a change - // discriminator, so fall back to the inode identity (dev+ino, which - // an atomic rename always changes) and treat the entry as settled - // only once THIS observation has itself aged past the granularity. - // Without this, such a file's cache entry was NEVER settled, forcing - // a full ~62 MiB re-parse on every get_unit_keys/host_certs call. + // No mtime (mtime-less FS) or a future mtime (clock skew): `duration_since` + // yields None, so the mtime can't discriminate change — settle via inode + // identity once this observation has itself aged past the granularity. None => std::time::SystemTime::now() .duration_since(self.stamped_at) .is_ok_and(|age| age >= granularity), @@ -295,11 +289,9 @@ impl KeydbSource { let entries = text .lines() - // Mirror KeyDb::parse's ACCEPTANCE rule EXACTLY by running the same - // parsers (not just a `| DK`/`| PK`/`| HC` prefix check): a row with - // the right prefix but malformed hex / a short cert parses to - // nothing, so counting it by prefix let unparseable content slip - // past the `entries > 0` guard. See docs/keydb.md#save-mirror-parse. + // Mirror KeyDb::parse by running the same parsers, not a prefix check: a + // right-prefix row with bad hex / short cert parses to nothing, so a prefix + // count let junk past `entries > 0`. See docs/keydb.md#save-mirror-parse. .filter(|l| crate::keydb_format::is_parseable_entry_line(l)) .count(); @@ -1008,10 +1000,9 @@ mod tests { assert_eq!(KeydbSource::new("/nonexistent/keydb.cfg").label(), "keydb"); } - // A cache entry whose stamp has NO mtime (mtime-less filesystem) or a mtime - // in the FUTURE (clock skew) must not be stuck un-settled forever — that - // forced a full ~62 MiB re-parse on every lookup. It now settles via the - // inode-identity fallback once the observation has aged past the granularity. + // A stamp with no mtime (mtime-less FS) or a future mtime (clock skew) must + // not be stuck un-settled forever — it now settles via the inode-identity + // fallback once the observation has aged past the granularity. #[test] fn is_settled_falls_back_when_mtime_is_absent_or_in_the_future() { use std::time::{Duration, SystemTime}; @@ -1500,10 +1491,9 @@ mod tests { fn save_rejects_prefixed_but_unparseable_dk_pk_hc_rows() { let dir = scratch("save-prefixed-junk"); let src = KeydbSource::new(dir.join("k.cfg")); - // Each line has a valid keydb PREFIX but content no parser accepts: - // - PK: not 16 bytes of hex - // - DK: DEVICE_KEY not valid hex - // - HC: cert far shorter than the 92-byte minimum + // Each line has a valid keydb PREFIX but content no parser accepts: PK not + // 16 bytes of hex, DK's DEVICE_KEY not valid hex, HC cert far shorter than + // the 92-byte minimum. let junk = b"| PK | 0xnothex\n\ | DK | DEVICE_KEY 0xZZ | DEVICE_NODE 0x0800 | KEY_UV 0x00000400 | KEY_U_MASK_SHIFT 0x17\n\ | HC | HOST_PRIV_KEY 0x00 | HOST_CERT 0x0011\n"; diff --git a/src/online.rs b/src/online.rs index 53ec52d..64dc9b2 100644 --- a/src/online.rs +++ b/src/online.rs @@ -117,12 +117,9 @@ enum GuardFail { // returns pinned socket addrs or a rejection reason + message. SECURITY: the // message names the address — log only at config time, never in `query`. fn resolve_and_guard(url: &str) -> Result, (GuardFail, String)> { - // ONLY https is accepted. The POST body carries base64 key material and a - // replayable bearer token, so cleartext `http://` is refused — and refused - // HERE, in the shared guard, so `validate_keyserver_url` catches it ONCE at - // config time rather than every rip tripping `query`'s runtime https check - // and reporting a transient error for a permanent misconfiguration. Any - // non-https scheme is a standing operator fault (`Config`), never an outage. + // ONLY https: the POST body carries base64 key material and a replayable + // bearer token, so cleartext `http://` is refused here in the shared guard — + // a standing operator fault (`Config`) caught once at config time, not per rip. let Some(authority) = url.strip_prefix("https://") else { return Err(( GuardFail::Config, @@ -138,10 +135,9 @@ fn resolve_and_guard(url: &str) -> Result, (GuardFail, String)> let (host, port): (String, u16) = if let Some(stripped) = authority.strip_prefix('[') { match stripped.split_once(']') { Some((h, after)) => { - // The only thing allowed after `]` is an optional `:port`. A - // non-empty tail that is NOT `:port` (e.g. `[::1]extra`) is - // garbage — reject it as a `Config` fault instead of silently - // dropping it and connecting on the default port. + // The only thing allowed after `]` is an optional `:port`. A non-empty + // tail that isn't `:port` (e.g. `[::1]extra`) is garbage — reject as + // `Config` rather than silently drop it and connect on the default port. let p = if after.is_empty() { default_port } else if let Some(port_str) = after.strip_prefix(':') { @@ -175,12 +171,9 @@ fn resolve_and_guard(url: &str) -> Result, (GuardFail, String)> let addrs: Vec = { use std::sync::mpsc; const DNS_TIMEOUT: Duration = Duration::from_secs(10); - // Each resolver thread can hang for the OS timeout and is never joined, - // so a black-holed keyserver leaks one thread+stack per attempt. Cap the - // outstanding ones — but PER HOST, not process-globally: a global cap - // lets 4 hung lookups to one dead keyserver starve resolution of a - // healthy DIFFERENT keyserver. Over the per-host cap, report only THAT - // host unreachable. `BTreeMap::new` is const, so no lazy init is needed. + // Resolver threads can hang for the OS timeout and are never joined, leaking + // a thread+stack per attempt — so cap outstanding ones PER HOST (a global cap + // lets one dead keyserver starve a healthy different one). Const-init map. const MAX_DNS_THREADS_PER_HOST: usize = 4; static DNS_INFLIGHT: Mutex> = Mutex::new(std::collections::BTreeMap::new()); @@ -245,15 +238,13 @@ fn resolve_and_guard(url: &str) -> Result, (GuardFail, String)> } /// Validate a key-service base URL before it is handed to [`OnlineSource`]. -/// Requires an `https` scheme (cleartext `http` is rejected as a `Config` -/// fault — see `resolve_and_guard`), extracts the host, and rejects any host that -/// is — or resolves to — a loopback / link-local (incl. the 169.254.169.254 -/// cloud-metadata endpoint) / RFC1918 / ULA / other non-public address (SSRF / -/// metadata-exfiltration guard). Returns `Ok(())` on success so a caller can -/// gate `OnlineSource` construction; the error string explains the rejection. +/// Requires `https` (cleartext `http` is rejected as a `Config` fault — see +/// `resolve_and_guard`), extracts the host, and rejects any host that is — or +/// resolves to — loopback / link-local (incl. 169.254.169.254 cloud metadata) +/// / RFC1918 / ULA / other non-public address (SSRF guard). Returns `Ok(())` +/// so a caller can gate `OnlineSource`; the error string says why. /// -/// This is the *config-time* check; [`OnlineSource`] re-resolves and -/// re-guards the host again before each POST, closing the DNS-rebind window. +/// 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) } @@ -1027,11 +1018,8 @@ mod tests { } // Cleartext http:// must be refused at CONFIG time (validate_keyserver_url), - // not per-rip: the body carries base64 key material and a replayable bearer - // token, and `query` hard-refuses non-https at runtime. Catching it once - // here turns a permanent misconfig into a config error instead of a - // transient failure on every rip. A public host proves it's the SCHEME - // being rejected, not the address guard. + // not per-rip — the body carries key material + a replayable token. A public + // host proves it's the SCHEME being rejected, not the address guard. #[test] fn non_https_scheme_is_rejected_at_config_time() { // A perfectly reachable public host — only the http:// scheme is wrong.