Report why an online key lookup failed, and cap the keydb load properly

A key-service failure was indistinguishable from "this disc has no key".
Five of the seven exits in the online query returned an empty result with no
log at all, and the one that did log threw away the HTTP status the service
had already handed us — so a 502, an expired token, a rate limit and a
genuine miss all produced the same single line, and the disc was reported as
having no key. Each exit now says which one it was, and the genuine miss is
logged distinctly from every failure.

Nothing new is disclosed: the address that failed the guard, the response
body and the bearer token are all still kept out of the log. The HTTP status
is the service's own response code, and the size cap is a constant.

A malformed key in a returned UK set no longer skips that one element. The
mux trusts any non-empty result as the complete forensic set, so a short set
silently omits an index; the whole reply is rejected instead.

Also make the on-disk keydb cap real. It was a metadata pre-check followed by
an unbounded read, which is not a cap: it is skipped when the stat fails, and
a FIFO or character device reports zero length, so --keydb /dev/zero passed
the guard and then read until the process was OOM-killed (NUL is valid UTF-8,
so it never even failed the decode). Read through the same take() cap the
download path already used, and add the boundary test the load path never had.

CODE_OF_CONDUCT.md

@@ -36,7 +36,7 @@ This Code of Conduct applies within all community spaces, and also applies when
 
 ## Enforcement
 
-Instances of abusive, harassing, or otherwise unacceptable behavior may be reported to the community leaders responsible for enforcement at owner@example.invalid. All complaints will be reviewed and investigated promptly and fairly.
+Instances of abusive, harassing, or otherwise unacceptable behavior may be reported privately to the project maintainer via GitHub at https://github.com/MattJackson. All complaints will be reviewed and investigated promptly and fairly.
 
 All community leaders are obligated to respect the privacy and security of the reporter of any incident.
 

src/keydb_format.rs

@@ -265,12 +265,19 @@ impl KeyDb {
     /// [`KeyDb`] rather than an error — callers needing a non-empty db must
     /// check the parsed contents.
     pub fn load(path: &std::path::Path) -> std::io::Result<Self> {
-        // Stat-and-cap before reading so a hostile/corrupt file can't force an
+        // Read through a hard `take` cap rather than trusting a stat. A
-        // unbounded allocation. A file strictly over the cap is rejected (a
+        // `metadata` pre-check is only advisory: it is skipped entirely when the
-        // file exactly at MAX_KEYDB_BYTES is accepted, matching the `>` guard
+        // stat fails, and it reports len 0 for a FIFO or character device, so
-        // and libfreemkv's original at-cap-is-allowed semantics).
+        // `--keydb /dev/zero` passed the guard and then read forever (NUL is
-        if let Ok(meta) = std::fs::metadata(path) {
+        // valid UTF-8, so it did not even fail the decode — it just grew until
-            if meta.len() > MAX_KEYDB_BYTES {
+        // the process was OOM-killed). Reading one byte past the cap accepts an
+        // exactly-at-cap file and rejects anything larger, matching
+        // `read_capped_to_string` on the download path.
+        use std::io::Read;
+        let f = std::fs::File::open(path)?;
+        let mut buf = Vec::new();
+        f.take(MAX_KEYDB_BYTES + 1).read_to_end(&mut buf)?;
+        if buf.len() as u64 > MAX_KEYDB_BYTES {
             return Err(std::io::Error::new(
                 std::io::ErrorKind::InvalidData,
                 format!(
@@ -279,8 +286,12 @@ impl KeyDb {
                 ),
             ));
         }
-        }
+        let data = String::from_utf8(buf).map_err(|_| {
-        let data = std::fs::read_to_string(path)?;
+            std::io::Error::new(
+                std::io::ErrorKind::InvalidData,
+                format!("keydb.cfg is not valid UTF-8: {}", path.display()),
+            )
+        })?;
         Ok(Self::parse(&data))
     }
 
@@ -727,6 +738,43 @@ impl KeyDb {
 mod tests {
     use super::*;
 
+    /// `load` must enforce `MAX_KEYDB_BYTES` through a bounded read, not a stat.
+    ///
+    /// Regression: the cap used to be a `std::fs::metadata` pre-check followed by
+    /// an unbounded `read_to_string`. That is not a cap — it is skipped when the
+    /// stat fails, and a FIFO or character device reports len 0, so the guard
+    /// passed and the read never terminated. NUL bytes are valid UTF-8, so
+    /// `--keydb /dev/zero` did not even fail the decode; it allocated until the
+    /// process died. Asserts both edges of the boundary so a future cap change
+    /// cannot quietly re-introduce an off-by-one.
+    #[test]
+    fn load_enforces_the_size_cap_at_both_edges() {
+        let dir = std::env::temp_dir()
+            .join("fmkv-keydb-cap")
+            .join(format!("{}", std::process::id()));
+        std::fs::create_dir_all(&dir).unwrap();
+
+        // Exactly at the cap is accepted. Use a comment line so `parse` is happy
+        // and the content is valid UTF-8 of a known length.
+        let at = dir.join("at_cap.cfg");
+        let body = vec![b'#'; MAX_KEYDB_BYTES as usize];
+        std::fs::write(&at, &body).unwrap();
+        assert!(
+            KeyDb::load(&at).is_ok(),
+            "a file exactly at MAX_KEYDB_BYTES must load"
+        );
+
+        // One byte over is rejected as InvalidData, not truncated and not OOM.
+        let over = dir.join("over_cap.cfg");
+        let mut body = vec![b'#'; MAX_KEYDB_BYTES as usize];
+        body.push(b'#');
+        std::fs::write(&over, &body).unwrap();
+        let err = KeyDb::load(&over).expect_err("over-cap file must be rejected");
+        assert_eq!(err.kind(), std::io::ErrorKind::InvalidData);
+
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
     /// `to_keydb_cfg` is the exact inverse of `parse`: parse a known line set,
     /// serialize it, re-parse, and every field survives — device key, processing
     /// key, host cert (priv key + cert + revocation), and the per-disc M/I(vid)/V/U

src/online.rs

@@ -281,7 +281,20 @@ impl OnlineSource {
         // request (and the bearer token) to an internal/metadata host.
         let pinned = match resolve_and_guard(&self.base_url) {
             Ok(addrs) => addrs,
-            Err(_) => return Vec::new(),
+            Err(_) => {
+                // Log THAT the URL was rejected, never WHY: the guard's message
+                // names the resolved address, and an internal address must not
+                // reach a log an operator may paste into a bug report. The
+                // static label is enough to separate "misconfigured/blocked
+                // key-service URL" from "this disc has no key", which is the
+                // only distinction the operator needs here.
+                tracing::warn!(
+                    target: "freemkv::keysource",
+                    phase = "keyserver_post",
+                    "key-service URL failed the address guard; skipping the online source"
+                );
+                return Vec::new();
+            }
         };
         let agent = hardened_agent(pinned);
         let mut req = agent.post(&self.base_url);
@@ -298,13 +311,30 @@ impl OnlineSource {
         let post_t0 = std::time::Instant::now();
         let resp = match req.send_json(body) {
             Ok(r) => r,
-            Err(_) => {
+            Err(e) => {
-                tracing::warn!(
+                // Record the HTTP status when the service actually answered:
+                // 401/403 (bad or expired token), 429 (rate limited) and
+                // 5xx (service down) are completely different operator actions,
+                // and collapsing them into one line is why a 502 was read as
+                // "this disc has no key". The status is the service's own
+                // response code — it carries no key material and no address.
+                // A transport error has no status; report it as such rather
+                // than inventing one.
+                match e {
+                    ureq::Error::Status(code, _) => tracing::warn!(
+                        target: "freemkv::keysource",
+                        phase = "keyserver_post",
+                        http_status = code,
+                        elapsed_ms = post_t0.elapsed().as_millis() as u64,
+                        "key service returned an HTTP error; no key from online"
+                    ),
+                    ureq::Error::Transport(_) => tracing::warn!(
                         target: "freemkv::keysource",
                         phase = "keyserver_post",
                         elapsed_ms = post_t0.elapsed().as_millis() as u64,
-                    "keyserver request failed (timeout, network, or HTTP error)"
+                        "key service unreachable (connect/timeout/TLS); no key from online"
-                );
+                    ),
+                }
                 return Vec::new();
             }
         };
@@ -324,11 +354,27 @@ impl OnlineSource {
             .is_err()
             || buf.len() > MAX_RESPONSE_BYTES
         {
+            // Length only — never the body, which carries base64 key material.
+            tracing::warn!(
+                target: "freemkv::keysource",
+                phase = "keyserver_post",
+                cap = MAX_RESPONSE_BYTES,
+                "key-service reply was unreadable or over the size cap; no key from online"
+            );
             return Vec::new();
         }
         let json: serde_json::Value = match serde_json::from_slice(&buf) {
             Ok(j) => j,
-            Err(_) => return Vec::new(),
+            Err(_) => {
+                // Never log the parse error or the payload: a serde message
+                // quotes the offending input, which here is key material.
+                tracing::warn!(
+                    target: "freemkv::keysource",
+                    phase = "keyserver_post",
+                    "key-service reply was not valid JSON; no key from online"
+                );
+                return Vec::new();
+            }
         };
         // `UK` is an ARRAY of hex keys (the service always returns an array now,
         // even of one). A single element is the base Unit Key. A full set (one per
@@ -343,12 +389,32 @@ impl OnlineSource {
                     out.push(UnitKey::new(0, k));
                 }
             } else if let Some(arr) = uk.as_array() {
+                // A forensic set is only usable COMPLETE: the mux trusts any
+                // non-empty result as the whole set and never assumes 32 (see
+                // `get_fmts_indexes`). Skipping an unparseable element would
+                // hand back a short set that silently omits an index, so a
+                // malformed element rejects the whole reply — that is a service
+                // bug, not a usable answer.
+                let mut bad = false;
                 for (i, v) in arr.iter().enumerate() {
-                    if let Some(k) = v.as_str().and_then(parse_uk) {
+                    match v.as_str().and_then(parse_uk) {
-                        out.push(UnitKey::new(i as u32, k));
+                        Some(k) => out.push(UnitKey::new(i as u32, k)),
+                        None => {
+                            bad = true;
+                            break;
                         }
                     }
                 }
+                if bad {
+                    tracing::warn!(
+                        target: "freemkv::keysource",
+                        phase = "keyserver_post",
+                        keys = arr.len(),
+                        "key-service returned a malformed key in the UK set; rejecting the whole reply"
+                    );
+                    return Vec::new();
+                }
+            }
             if !out.is_empty() {
                 return out;
             }
@@ -356,10 +422,28 @@ impl OnlineSource {
         // A VUK is derived to the terminal keys locally, via the disc's
         // encrypted title keys from the context — the library owns the crypto.
         if let Some(vuk) = json.get("VUK").and_then(|u| u.as_str()).and_then(parse_uk) {
-            if let Ok(enc) = ctx.enc_title_keys() {
+            match ctx.enc_title_keys() {
-                return uks_from_vuk(&vuk, enc);
+                Ok(enc) => return uks_from_vuk(&vuk, enc),
+                Err(_) => {
+                    tracing::warn!(
+                        target: "freemkv::keysource",
+                        phase = "keyserver_post",
+                        "key-service returned a VUK but the disc's encrypted title keys \
+                         are unreadable; cannot derive unit keys"
+                    );
+                    return Vec::new();
                 }
             }
+        }
+        // The genuine miss. Logged distinctly from every failure above so an
+        // operator can tell "the service has no key for this disc" from "the
+        // service did not answer" — collapsing the two is what made a 502 look
+        // like a missing key.
+        tracing::info!(
+            target: "freemkv::keysource",
+            phase = "keyserver_post",
+            "key service has no key for this disc"
+        );
         Vec::new()
     }
 }