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.
online.rs:
- validate_keyserver_url/resolve_and_guard now reject any non-https:// scheme
as a Config fault, so a cleartext misconfig is caught ONCE at config time
instead of failing every rip via query()'s runtime https check.
- is_blocked_ip: add 198.18.0.0/15 (RFC 2544 benchmarking) and 192.0.0.0/24
(RFC 6890 IETF protocol assignments, incl. 192.0.0.170/171) to the SSRF guard.
- DNS in-flight cap is now PER HOST, not process-global, so black-holed lookups
to one dead keyserver can't starve resolution of a healthy different keyserver.
- reject a non-empty, non-":port" tail after a bracketed IPv6 authority
(e.g. https://[::1]extra/) instead of silently dropping it.
keydb.rs / keydb_format.rs:
- MAX_KEYDB_BYTES defined once in keydb_format and re-used by keydb.rs.
- CacheEntry::is_settled falls back to inode identity + stamped_at age when the
mtime is absent (mtime-less FS) or in the future (clock skew), so the ~62 MiB
keydb no longer re-parses on every lookup.
- save()'s entry-count guard counts DK/PK/HC/HC2 rows via the real parsers
(is_parseable_entry_line), so a prefixed-but-malformed row can't pass entries>0.
Tests added for each fix.
OnlineSource now records the HTTP outcome of its actual /decode POST — a
Status(code) for any answer (200/404/422/429/5xx) or Transport for a
connect/timeout/DNS/TLS failure — in a per-thread slot, readable via the new
take_last_decode_reachability() / DecodeReachability. This lets a caller
(autorip) classify a no-key result from the REAL decode instead of firing a
second, redundant probe against the POST-only endpoint. No change to key
resolution itself. Targets 1.7.2.
Several tests asserted only key[0] (the first byte), which a byte-transposition bug could pass — assert the full 16-byte key everywhere, including a KAT that recomputes the expected VUK-derived keys directly. test_parse_disc_entry used identical all-zero placeholders for M/I/V/U, which couldn't catch a field mix-up; give each field a distinct byte pattern and assert every field.
Docs: keydb-format.md claimed DiscEntry::title was genuinely unused dead code, but it's read by to_keydb_cfg and the redacting Debug impl — correct the comment to only list empty/find_vuk as truly unused. Also cross-reference the two independent MAX_KEYDB_BYTES constants (keydb.rs's decompression cap and keydb_format.rs's on-disk load cap) so they're kept in sync, and bump Cargo.lock's version metadata.
Two disc-side reads were silently swallowed with unwrap_or(&[]): a failure reading the disc's encrypted title keys or its MKB looked identical to a disc that genuinely had none, hiding real I/O/parse errors from the operator. Log a warning (mirroring online.rs's existing pattern) before falling back to empty in both cases.
Also, a stored unit key numbered 0 (an invalid CPS unit number; valid numbering is >= 1) was mapped via num.saturating_sub(1) onto idx 0, silently colliding with and shadowing a legitimate unit 1. Skip unit number 0 instead of mis-mapping it, and add a regression test.
cached_db held self.cache locked across the full KeyDb::load_counted parse of the ~62 MiB keydb, so a reparse (triggered by a changed mtime) stalled every other worker waiting on the cache. Take a brief lock for the fast-path settled-cache-hit check, release it, parse outside the lock, then re-acquire and double-check: if a peer installed the same settled stamp while we parsed, adopt their entry and drop our redundant parse rather than racing a lost update. Stamp and bytes still come from the one already-open file handle, preserving the one-open-one-identity invariant.
Each DNS lookup for the SSRF guard spawns a thread that is never joined and can hang for the full OS resolver timeout, so a black-holed keyserver leaks one thread+stack per attempt. Track outstanding resolver threads with an atomic counter; over a cap of 4, report the host as unreachable instead of spawning another. Decrement only when the (possibly long-hung) lookup actually returns, so the cap reflects threads truly in flight.
The IPv6 SSRF guard already re-checked IPv4-mapped/-compatible addresses, but 6to4 (2002::/16) and Teredo (2001:0000::/32) also tunnel an embedded IPv4 and were passed through untouched, letting an attacker wrap 127.0.0.1 or a cloud metadata address in either encapsulation to bypass the block list. Decode both forms and re-check the embedded IPv4 through is_blocked_ip. Add a test covering 6to4/Teredo-wrapped loopback and metadata addresses, plus a 6to4-wrapped public IP that must still be allowed.
keydb.cfg carries AACS key material plus the host private key/cert. write_atomic renamed a File::create()'d temp file onto it, so the final mode depended on umask and could leave the keys world-readable. On Unix, open the temp file with mode 0600 explicitly via OpenOptionsExt; non-Unix keeps the plain create().
CI/lint hardening since v1.6.14: clippy on stable with the MSRV single-sourced
from Cargo.toml, cargo-deny dependency-audit gate with first-party git sources
allowed (#15), and audience-based comment-guard (#17). No library code changes.
Declared rust-version and the ci.yml/qa.yml toolchain pins both sit at 1.88 (the
edition-2024 floor once zip 8 and let-chains are accounted for), so CI builds on
exactly the MSRV: an accidental bump to a newer toolchain fails CI by design and
moving the floor takes a deliberate pin edit. The uninlined_format_args allow
reconciles 1.88's clippy with the allow-by-default it became in 1.97+.
Adds targeted unit tests for previously-uncovered branches: keydb load
failure classification (invalid-UTF-8 / unreadable path), zip extraction
success/no-cfg-member paths, write_atomic create/rename failure modes,
keydb_format hex-digit-parsing edge cases, iter_disc_entries, to_keydb_cfg
HC2 emission and empty-title/comment-field combinations, MultiSource::label,
and the online key-service client's SSRF-guard malformed-authority and
unresolvable-host paths, UK-as-string and VUK-derivation reply handling,
and get_fmts_indexes. Local line coverage moves from ~90.3% to ~92.1%.
Add an additive coverage job to ci.yml that mirrors the test job's
sibling-checkout layout and pinned 1.97.0 toolchain, runs cargo llvm-cov,
and uploads to Codecov tokenlessly (fail_ci_if_error:false). Nothing in
the release cascade depends on it. Also add codecov.yml (informational,
no PR comment) and a README with the coverage badge.
Adds a 'changelog' job that fails when Cargo.toml's version has no matching
'## [X.Y.Z]' heading in CHANGELOG.md, hard on main and PRs into main, warning
on dev/qa — so the unified release can't ship a crate with a blank changelog.
parse_disc_entry's U arm stripped a `; MKBv…` comment before hex-parsing the
value, but the M/I/V arms did not. When a real keydb.cfg disc row's LAST field
is M/I/V (no U after it), the comment glues onto that value and parse_hex16 of
"0x… ; MKBv64" fails — silently dropping a media/id/VUK key the file actually
holds, so the disc reports no key. Strip the comment in M/I/V too (a hex16 never
contains ';', so it is safe), with a red-before-green regression test.
The round-2 guard tests used `keyserver.invalid` — RFC 6761 reserved and
guaranteed non-resolvable, which is the correct property. But the shared
leak-guard net lists `.invalid` among the private-infra TLD suffixes
(.internal/.local/.lan/.corp), so every such host tripped it and turned the
repo red on push — the exact cost the precommit script's own comment records
from 1.6.3. `.test` is equally RFC-6761-reserved and equally non-resolvable,
carries the same test guarantee, and is not in the infra net. No behaviour
change; the tests assert the identical never-resolves path.
Round 1's parse cache keyed on `(len, modified)`, which is not a file
identity. mtime is stored at whole-second resolution on HFS+, on many
container overlays and on most network filesystems, so an external
writer — the daily-refresh job, a second freemkv process, an editor, a
sync tool — replacing keydb.cfg within one second with a file of the
same byte length left the stamp bit-identical and the source served the
superseded parse for the rest of the process's life. Retired or
since-corrected keys, reported as success. The doc claiming "an
externally replaced keydb is still picked up on the next call" was false
in exactly that case.
Two discriminators, chosen so neither costs a 62 MiB content hash (the
141 ms x 3+ loads per rip the cache removed):
1. dev + ino. Every atomic publisher (temp + fsync + rename, including
this crate's save()) creates a new inode, so the stamp differs
unconditionally, whatever the clock did.
2. A settle window. An entry is trusted only once the file has been
quiet for the mtime granularity (2 s); the proof that this closes
the residual in-place case, and the network-clock caveats, are in
CacheEntry::is_settled. Cost: re-parsing for up to 2 s after each
write of a file that is rewritten daily.
What it still does NOT catch, now stated in the doc instead of implied
away: a writer that forges the timestamp back to the cached value
(touch -t, rsync --times, a backup restore) into the same inode at the
same length. save() therefore keeps invalidating directly.
Also fixed, all in the same cache:
- The corruption warning was swallowed. ParseStats::log runs inside
KeyDb::parse and a cache hit skips parse, so a damaged keydb warned
once per process and was then served in silence to every later rip.
It is re-emitted on every hit that serves the file; an intact keydb
still says nothing.
- TOCTOU. cached_db stat'd the path and then separately loaded the
path — two resolutions of one name with no atomicity, so a rename in
the gap filed one file's stamp beside another file's keys. The file
is opened once; the stamp is an fstat on that handle and the bytes
come from it.
- Memory residency is recorded rather than quietly widened: a hit
holds every disc's key material plus host-cert private keys for the
process's life, unzeroized. Nothing in this crate zeroizes on drop
and that predates the cache; closing it means zeroize-on-drop across
KeyDb, DiscEntry and libfreemkv's aacs::types, which is cross-crate
work and not smuggled in behind a cache fix.
On KeydbHostCert's derived Debug: audited, NOT a leak. libfreemkv's
HostCert hand-writes a redacting Debug (private_key: "<redacted>",
certificate_len), so `{:?}` on a host cert prints no key bytes. But that
is an external guarantee this crate inherits silently, and the existing
redaction test only covered the outer types. It is now pinned here, for
host certs and device keys, verified red against a libfreemkv patched to
derive Debug instead.
Two KEYDB_PATH-gated tests that could not fail are fixed too:
test_decrypt_real_unit fell off the end after trying every key (a no-op
decrypt_unit passed it), and test_resolve_keys_vuk_path returned early
with zero assertions when the fixture had no qualifying entry.
CHANGELOG: entries for this and for dd9a5dc, including the previously
unrecorded Arc<str> break to KeyDb::disc_entries / DiscEntry::disc_hash.
Round 1 separated a config fault from a transient one and then routed
them to OPPOSITE severities. `GuardFail::Config` returned
`Ok(Vec::new())` from `query` while `GuardFail::Unreachable` returned
`Err`, so a mistyped port — `https://host:notaport/` — produced one
`warn!` and a benign-looking empty, which `MultiSource::first_non_empty`
cannot distinguish from "no source has a row for this disc". The disc
was reported as E7022 "no key for this disc". That is the seven-hour-502
collapse with the polarity reversed, and worse: an outage self-heals, a
typo in a config file does not.
Every way of failing to ASK is now `Err(KeyServiceUnavailable)`, logged
at error level with text that names it as a URL to fix rather than an
outage to wait out. That covers the address-guard rejection (bad port,
malformed URL, a host resolving to a non-public address) and the
cleartext `http://` refusal, which had the same silent shape.
The two input-shaped guards — an over-cap MKB, too few content samples —
deliberately stay `Ok(empty)`: they are properties of this disc, not
faults, and calling them an outage would mislabel them in the other
direction. The line between the three cases is now written down at the
guards themselves.
Not a rip-breaking change: an `Err` never blocks the chain, because
`first_non_empty` still returns any later source's key set outright and
surfaces a failure only when nothing anywhere resolved.
Also: the agent-reuse cache key is now sorted and deduped. It was an
ordered `Vec<SocketAddr>` compare, so a round-robin keyserver — the one
deployment the connection reuse exists for — reordered its A/AAAA
records and missed the cache on every query, silently paying a fresh TLS
handshake each time. Set equality is also the honest predicate for
"the same validated addresses".
Note for libfreemkv: there is no `KeyServiceMisconfigured` code, so a
permanent fault borrows the transient E7028. E7028's "the source never
answered" is exactly right here; only its "retry later" hint is wrong.
Matthew Jackson