fix(secret-rotator): HTTP/WS timeouts, entropy floor, orphan-token logging

Addresses three review findings, all real bugs rather than style:

1. No timeouts anywhere. The Python this replaces passed timeout=30 on every
   call; ureq 3's default agent has none and tungstenite has no read timeout,
   so a hung endpoint would stall the Job silently until activeDeadlineSeconds
   with no useful error — and the cluster demonstrably has hanging paths (the
   ingress-LB hairpin). Now: a shared ureq::Agent with timeout_global(30s)
   threaded through every OpenBao call, and a 30s read timeout set on the
   TcpStream under tungstenite's socket (handling both plain and rustls
   streams).

2. RANDOM_BYTES=0 would generate a near-empty secret, store it, and report
   success — the same silent-misconfig class assert_fields_exist guards. Now
   floored at 16 bytes (128 bits); every real rotator uses 32 or 50.

3. If mint succeeds but the OpenBao store fails, the freshly minted token is
   valid for its full lifespan, referenced nowhere, and never auto-revoked
   (revocation only targets the OLD stored id). The store error path now logs
   the orphan's refresh-token id — recovered from its own JWT — so it can be
   revoked by hand, and makes clear the consumer is unaffected because the old
   token is still stored and valid.

clippy -D warnings clean; 10 tests pass; scratch image builds (4.92 MB) and
all error paths exit non-zero.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

secret-rotator/.dockerignore

@@ -0,0 +1 @@
+target/

secret-rotator/Cargo.lock

@@ -497,6 +497,8 @@ dependencies = [
 name = "secret-rotator"
 version = "0.1.0"
 dependencies = [
+ "base64",
+ "getrandom 0.4.3",
  "serde_json",
  "tungstenite",
  "ureq",

secret-rotator/Cargo.toml

@@ -4,6 +4,8 @@ version = "0.1.0"
 edition = "2024"
 
 [dependencies]
+base64 = "0.22.1"
+getrandom = "0.4.3"
 serde_json = "1.0.151"
 tungstenite = { version = "0.30.0", default-features = false, features = ["rustls-tls-webpki-roots", "handshake"] }
 ureq = { version = "3.3.0", default-features = false, features = ["rustls", "json"] }

secret-rotator/src/bao.rs

@@ -7,9 +7,25 @@
 
 use std::error::Error;
 use std::fmt;
+use std::time::Duration;
 
 const SA_TOKEN: &str = "/var/run/secrets/kubernetes.io/serviceaccount/token";
 
+/// Every HTTP call gets a wall-clock cap. The Python this replaces passed
+/// timeout=30 on every request; ureq 3's default agent has NO timeout, so a
+/// hung endpoint would otherwise stall the Job silently until
+/// activeDeadlineSeconds fires with no useful error. The cluster demonstrably
+/// has hanging paths (the ingress-LB hairpin), so this is not hypothetical.
+const HTTP_TIMEOUT: Duration = Duration::from_secs(30);
+
+/// Shared agent with the global timeout applied, used for every OpenBao call.
+pub fn agent() -> ureq::Agent {
+    ureq::Agent::config_builder()
+        .timeout_global(Some(HTTP_TIMEOUT))
+        .build()
+        .into()
+}
+
 #[derive(Debug)]
 pub struct BaoError(String);
 
@@ -27,6 +43,7 @@ fn err<T>(msg: impl Into<String>) -> Result<T, Box<dyn Error>> {
 pub struct Bao {
     addr: String,
     token: String,
+    agent: ureq::Agent,
 }
 
 impl Bao {
@@ -34,19 +51,25 @@ impl Bao {
     pub fn login(addr: &str, role: &str) -> Result<Self, Box<dyn Error>> {
         let jwt = std::fs::read_to_string(SA_TOKEN)
             .map_err(|e| BaoError(format!("reading {SA_TOKEN}: {e}")))?;
+        let agent = agent();
         let body = serde_json::json!({ "role": role, "jwt": jwt.trim() });
-        let mut resp = ureq::post(format!("{addr}/v1/auth/kubernetes/login"))
+        let mut resp = agent
+            .post(format!("{addr}/v1/auth/kubernetes/login"))
             .send_json(&body)?;
         let v: serde_json::Value = resp.body_mut().read_json()?;
         match v["auth"]["client_token"].as_str() {
-            Some(t) => Ok(Bao { addr: addr.trim_end_matches('/').to_string(), token: t.to_string() }),
+            Some(t) => Ok(Bao {
+                addr: addr.trim_end_matches('/').to_string(),
+                token: t.to_string(),
+                agent,
+            }),
             None => err(format!("login as role '{role}' returned no client_token")),
         }
     }
 
     /// Read one field from a KV v2 path.
     pub fn read_field(&self, path: &str, field: &str) -> Result<String, Box<dyn Error>> {
-        let mut resp = ureq::get(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
+        let mut resp = self.agent.get(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
             .header("X-Vault-Token", &self.token)
             .call()?;
         let v: serde_json::Value = resp.body_mut().read_json()?;
@@ -74,7 +97,7 @@ impl Bao {
     /// incident. Rotation only ever replaces existing fields, so absence is an
     /// error rather than something to paper over.
     pub fn assert_fields_exist(&self, path: &str, fields: &[&str]) -> Result<(), Box<dyn Error>> {
-        let mut resp = ureq::get(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
+        let mut resp = self.agent.get(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
             .header("X-Vault-Token", &self.token)
             .call()?;
         let v: serde_json::Value = resp.body_mut().read_json()?;
@@ -105,7 +128,7 @@ impl Bao {
             data.insert((*k).to_string(), serde_json::Value::String((*v).to_string()));
         }
         let body = serde_json::json!({ "data": data });
-        let resp = ureq::patch(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
+        let resp = self.agent.patch(format!("{}/v1/{}/data/{}", self.addr, mount(path), sub(path)))
             .header("X-Vault-Token", &self.token)
             .header("Content-Type", "application/merge-patch+json")
             .send_json(&body)?;

secret-rotator/src/ha.rs

@@ -12,10 +12,26 @@
 //! `long_lived_access_token`, which is why revocation goes through
 //! auth/delete_refresh_token.
 
+use base64::engine::general_purpose::URL_SAFE_NO_PAD;
+use base64::Engine as _;
 use std::error::Error;
 use std::fmt;
+use std::time::Duration;
+use tungstenite::stream::MaybeTlsStream;
 use tungstenite::{connect, Message};
 
+/// Apply a read timeout to the TcpStream under tungstenite's (possibly TLS-
+/// wrapped) socket, whether the connection is plain ws:// or wss://.
+fn set_read_timeout(s: &MaybeTlsStream<std::net::TcpStream>, d: Duration) -> Result<(), Box<dyn Error>> {
+    let tcp = match s {
+        MaybeTlsStream::Plain(t) => t,
+        MaybeTlsStream::Rustls(t) => t.get_ref(),
+        _ => return err("unexpected websocket stream kind"),
+    };
+    tcp.set_read_timeout(Some(d))?;
+    Ok(())
+}
+
 #[derive(Debug)]
 pub struct HaError(String);
 impl fmt::Display for HaError {
@@ -50,7 +66,8 @@ pub struct Ha {
 /// against auth/refresh_tokens before anything is revoked.
 pub fn token_refresh_id(jwt: &str) -> Result<String, Box<dyn Error>> {
     let payload = jwt.split('.').nth(1).ok_or_else(|| HaError("token is not a JWT".into()))?;
-    let raw = b64url_decode(payload)?;
+    // JWT payloads are base64url and unpadded, hence URL_SAFE_NO_PAD.
+    let raw = URL_SAFE_NO_PAD.decode(payload)?;
     let v: serde_json::Value = serde_json::from_slice(&raw)?;
     match v["iss"].as_str() {
         Some(s) if !s.is_empty() => Ok(s.to_string()),
@@ -58,31 +75,15 @@ pub fn token_refresh_id(jwt: &str) -> Result<String, Box<dyn Error>> {
     }
 }
 
-fn b64url_decode(s: &str) -> Result<Vec<u8>, Box<dyn Error>> {
-    const A: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-_";
-    let mut acc: u32 = 0;
-    let mut bits = 0u32;
-    let mut out = Vec::new();
-    for c in s.bytes() {
-        if c == b'=' {
-            break;
-        }
-        let v = A.iter().position(|&a| a == c).ok_or_else(|| HaError(format!("bad base64url byte {c:?}")))?;
-        acc = (acc << 6) | v as u32;
-        bits += 6;
-        if bits >= 8 {
-            bits -= 8;
-            out.push((acc >> bits) as u8);
-        }
-    }
-    Ok(out)
-}
-
 impl Ha {
     /// Connect and authenticate. `url` is ws://host/api/websocket in-cluster,
     /// or wss:// externally.
     pub fn connect(url: &str, token: &str) -> Result<Self, Box<dyn Error>> {
         let (sock, _) = connect(url)?;
+        // tungstenite has no read timeout of its own, so a silent HA (or a
+        // hairpin-blocked path) would block sock.read() forever. Set one on the
+        // underlying TcpStream — matching the timeout=30 the Python used.
+        set_read_timeout(sock.get_ref(), Duration::from_secs(30))?;
         let mut ha = Ha { sock, next_id: 1 };
         match ha.recv()?["type"].as_str() {
             Some("auth_required") => {}
@@ -182,13 +183,6 @@ mod tests {
         assert_eq!(token_refresh_id(&jwt(p)).unwrap(), "abc123");
     }
 
-    #[test]
-    fn decodes_unpadded_base64url() {
-        assert_eq!(b64url_decode("YQ").unwrap(), b"a");
-        assert_eq!(b64url_decode("YWI").unwrap(), b"ab");
-        assert_eq!(b64url_decode("YWJj").unwrap(), b"abc");
-    }
-
     #[test]
     fn rejects_non_jwt() {
         assert!(token_refresh_id("not-a-jwt").is_err());

secret-rotator/src/main.rs

@@ -62,6 +62,13 @@ fn rotate_random() -> Result<String, Box<dyn Error>> {
     let bao_role = env("BAO_ROLE")?;
     let bao_path = env("BAO_PATH")?;
     let bytes: usize = env_or("RANDOM_BYTES", "32").parse()?;
+    // Floor on entropy. Without it RANDOM_BYTES=0 (or any tiny value) would
+    // generate a near-empty secret, patch it in, and report success — the same
+    // silent-misconfig class assert_fields_exist guards against. Every real
+    // rotator uses 32 or 50; 16 bytes (128 bits) is the lowest defensible.
+    if bytes < 16 {
+        return Err(format!("RANDOM_BYTES={bytes} is too low; refusing to generate a weak secret (min 16)").into());
+    }
     let fields: Vec<String> = env("BAO_FIELDS")?
         .split(',')
         .map(|f| f.trim().to_string())
@@ -139,7 +146,23 @@ fn rotate_ha_token() -> Result<String, Box<dyn Error>> {
     };
 
     let fresh = ha.mint(&client_name, lifespan)?;
-    bao.patch_field(&bao_path, &field, &fresh)?;
+
+    // If the store fails now, the freshly minted token is valid for its full
+    // lifespan, referenced nowhere, and will never be revoked automatically
+    // (revocation only ever targets the OLD stored token's id). Log the
+    // orphan's refresh id — recoverable from its own JWT — so it can be
+    // revoked by hand. Do NOT log the token value.
+    if let Err(e) = bao.patch_field(&bao_path, &field, &fresh) {
+        let orphan = ha::token_refresh_id(&fresh)
+            .unwrap_or_else(|_| "<unknown: could not decode new token>".to_string());
+        eprintln!(
+            "CRITICAL: minted a new '{client_name}' token but FAILED to store it. \
+             The old token is still in OpenBao and still works, so the consumer is fine, \
+             but the new token is orphaned and valid for {lifespan} days. \
+             Revoke it manually — its refresh-token id is {orphan}."
+        );
+        return Err(e);
+    }
 
     // Only now is the old token expendable — the replacement is stored.
     match ha.revoke(&old_id) {

secret-rotator/src/secretgen.rs

@@ -10,54 +10,44 @@
 //! these verbatim — a Django SECRET_KEY, a Woodpecker agent secret — and none
 //! filter the character set, so switching to base64url or dropping padding
 //! would silently produce a different-looking credential for no reason.
+//!
+//! `getrandom` and `base64` are both already in the dependency tree (via
+//! ring/rustls and ureq), so using them adds nothing to the build and avoids
+//! hand-rolled encoding sitting next to key material.
 
+use base64::engine::general_purpose::STANDARD;
+use base64::Engine as _;
 use std::error::Error;
-use std::io::Read;
-
-const STD: &[u8] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";
 
 /// N cryptographically-random bytes, standard-base64 encoded with padding.
 pub fn random_secret(bytes: usize) -> Result<String, Box<dyn Error>> {
     let mut buf = vec![0u8; bytes];
-    // /dev/urandom directly: no rand crate, and nothing to keep patched.
-    std::fs::File::open("/dev/urandom")?.read_exact(&mut buf)?;
-    Ok(b64_encode(&buf))
-}
-
-fn b64_encode(input: &[u8]) -> String {
-    let mut out = String::with_capacity(input.len().div_ceil(3) * 4);
-    for chunk in input.chunks(3) {
-        let b = [chunk[0], *chunk.get(1).unwrap_or(&0), *chunk.get(2).unwrap_or(&0)];
-        let n = ((b[0] as u32) << 16) | ((b[1] as u32) << 8) | b[2] as u32;
-        out.push(STD[(n >> 18) as usize & 63] as char);
-        out.push(STD[(n >> 12) as usize & 63] as char);
-        out.push(if chunk.len() > 1 { STD[(n >> 6) as usize & 63] as char } else { '=' });
-        out.push(if chunk.len() > 2 { STD[n as usize & 63] as char } else { '=' });
-    }
-    out
+    getrandom::fill(&mut buf)?;
+    Ok(STANDARD.encode(&buf))
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
 
+    /// Guards the engine choice rather than base64 itself: STANDARD must stay
+    /// STANDARD. These are the RFC 4648 vectors — what coreutils `base64`
+    /// emits, which is what the shell one-liner used.
     #[test]
-    fn matches_known_base64_vectors() {
-        // RFC 4648 test vectors — same output as coreutils `base64`.
-        assert_eq!(b64_encode(b""), "");
-        assert_eq!(b64_encode(b"f"), "Zg==");
-        assert_eq!(b64_encode(b"fo"), "Zm8=");
-        assert_eq!(b64_encode(b"foo"), "Zm9v");
-        assert_eq!(b64_encode(b"foob"), "Zm9vYg==");
-        assert_eq!(b64_encode(b"fooba"), "Zm9vYmE=");
-        assert_eq!(b64_encode(b"foobar"), "Zm9vYmFy");
+    fn matches_coreutils_base64_vectors() {
+        assert_eq!(STANDARD.encode(b""), "");
+        assert_eq!(STANDARD.encode(b"f"), "Zg==");
+        assert_eq!(STANDARD.encode(b"fo"), "Zm8=");
+        assert_eq!(STANDARD.encode(b"foo"), "Zm9v");
+        assert_eq!(STANDARD.encode(b"foob"), "Zm9vYg==");
+        assert_eq!(STANDARD.encode(b"fooba"), "Zm9vYmE=");
+        assert_eq!(STANDARD.encode(b"foobar"), "Zm9vYmFy");
     }
 
     #[test]
     fn uses_standard_alphabet_not_urlsafe() {
-        // 0xFB 0xFF encodes to "+/" in the standard alphabet and "-_" in
-        // URL-safe. The shell one-liner produced standard, so must we.
-        assert_eq!(b64_encode(&[0xfb, 0xff, 0xbf]), "+/+/");
+        // 0xFB 0xFF 0xBF is "+/+/" in the standard alphabet, "-_-_" in URL-safe.
+        assert_eq!(STANDARD.encode([0xfb, 0xff, 0xbf]), "+/+/");
     }
 
     #[test]
@@ -69,9 +59,7 @@ mod tests {
 
     #[test]
     fn values_are_not_repeated() {
-        let a = random_secret(32).unwrap();
-        let b = random_secret(32).unwrap();
-        assert_ne!(a, b);
+        assert_ne!(random_secret(32).unwrap(), random_secret(32).unwrap());
     }
 
     #[test]