diff --git a/Cargo.lock b/Cargo.lock index 20afa93..c2a5d0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -374,6 +374,7 @@ dependencies = [ "tokio-rustls", "tracing", "webpki-roots", + "zeroize", "zstd", ] @@ -1813,6 +1814,7 @@ version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" dependencies = [ + "serde", "zeroize_derive", ] diff --git a/Cargo.toml b/Cargo.toml index a4f0780..e41a2ab 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -63,7 +63,7 @@ hostname = "0.4" # Noise protocol snow = "0.10" x25519-dalek = { version = "2", features = ["static_secrets"] } -zeroize = { version = "1", features = ["derive"] } +zeroize = { version = "1", features = ["derive", "serde"] } rand = "0.10" base64 = "0.22" zstd = "0.13" diff --git a/crates/dictyon/Cargo.toml b/crates/dictyon/Cargo.toml index 224d0aa..b954a11 100644 --- a/crates/dictyon/Cargo.toml +++ b/crates/dictyon/Cargo.toml @@ -25,6 +25,7 @@ zstd = { workspace = true } proptest = { workspace = true } rcgen = "0.13" koinon = { workspace = true } +zeroize = { workspace = true } [lints] workspace = true diff --git a/crates/dictyon/examples/connect.rs b/crates/dictyon/examples/connect.rs index 69fc2a8..68577b2 100644 --- a/crates/dictyon/examples/connect.rs +++ b/crates/dictyon/examples/connect.rs @@ -17,6 +17,7 @@ use koinon::telemetry; use mitos::keys::{DiscoPrivate, MachinePrivate, NodePrivate}; use snafu::{ResultExt, Snafu}; use tracing::{info, warn}; +use zeroize::Zeroizing; const CONTROL_URL: &str = "https://controlplane.tailscale.com"; @@ -89,8 +90,13 @@ async fn main() -> Result<(), ExampleError> { } async fn run() -> Result<(), ExampleError> { + // WHY: `std::env::var` is wrapped in `Zeroizing` in the same statement + // that allocates it, mirroring `AuthInfo::new` (mitos/src/types/mod.rs) + // — no unwrapped copy of the raw secret is ever bound to a separate + // variable, so there is nothing left for a core dump or `/proc//mem` + // read to recover once this value is dropped. let auth_key = match std::env::var("TS_AUTHKEY") { - Ok(value) => Some(value), + Ok(value) => Some(Zeroizing::new(value)), Err(std::env::VarError::NotPresent) => { warn!("TS_AUTHKEY not set - server will require interactive auth"); None @@ -123,7 +129,17 @@ async fn run() -> Result<(), ExampleError> { disco_key, ); - register_node(&mut client, &mut stream, auth_key.as_deref()).await?; + register_node( + &mut client, + &mut stream, + auth_key.as_deref().map(String::as_str), + ) + .await?; + // WHY: auth_key is only needed for registration; stream_map below runs + // until interrupted, so dropping here — rather than letting it live to + // the end of `run` — bounds how long the zeroizing allocation backing + // this reusable, session-independent secret stays resident. + drop(auth_key); stream_map(&mut client, &mut stream).await?; Ok(()) } diff --git a/crates/dictyon/src/control/mod.rs b/crates/dictyon/src/control/mod.rs index 5db2b4a..7e355c8 100644 --- a/crates/dictyon/src/control/mod.rs +++ b/crates/dictyon/src/control/mod.rs @@ -290,13 +290,16 @@ impl ControlClient { /// # Errors /// /// Returns [`ControlError::Json`] if serialization fails. + /// + /// Time: O(n) — dominated by `serde_json::to_vec` over the request, + /// where `n` is the serialized payload size. + /// Space: O(n) — the returned buffer plus the intermediate + /// [`RegisterRequest`]. pub fn build_register_request(&self, auth_key: Option<&str>) -> Result, ControlError> { let req = RegisterRequest { node_key: self.node_key.public_key().to_hex(), old_node_key: String::new(), // kanon:ignore RUST/plain-string-secret -- public key hex, not a secret - auth: auth_key.map(|k| AuthInfo { - auth_key: Some(k.to_string()), - }), + auth: auth_key.map(AuthInfo::new), hostinfo: self.hostinfo(), followup: None, }; diff --git a/crates/mitos/src/types/mod.rs b/crates/mitos/src/types/mod.rs index 03918df..0046aca 100644 --- a/crates/mitos/src/types/mod.rs +++ b/crates/mitos/src/types/mod.rs @@ -9,6 +9,7 @@ use std::fmt; use serde::{Deserialize, Serialize}; +use zeroize::Zeroizing; // --------------------------------------------------------------------------- // Registration @@ -64,11 +65,40 @@ pub struct RegisterRequest { /// the [`RegisterRequest`] that owns it — a log line, an error context, a panic /// payload. The redaction mirrors the one on private key types in /// [`crate::keys`]. +/// +/// WARNING: `auth_key` is `Zeroizing`, not `String`, for the same +/// reason the private key types in [`crate::keys`] zero their backing bytes +/// on drop: a pre-auth key authorizes unattended device enrollment and is +/// reusable across sessions, so a heap allocation left holding it after use +/// is a recoverable credential in a core dump, `/proc//mem`, or swap. +/// `zeroize`'s `serde` feature gives `Zeroizing` the same +/// `Serialize` impl `String` has, so the wire format is unchanged. The +/// field stays crate-private; construct through [`AuthInfo::new`] so the +/// raw key text passes through exactly one allocation, wrapped immediately, +/// with no separate unwrapped copy left behind at the call site. `new` is +/// also the only way to reach a non-empty `Auth` object: a bare +/// `auth_key: None` would serialize as `"Auth":{}` instead of omitting the +/// field entirely, a wire shape no real caller wants. #[derive(Serialize)] pub struct AuthInfo { - /// Pre-auth key value (e.g. `tskey-auth-...`). + /// Pre-auth key value (e.g. `tskey-auth-...`), zeroed on drop. #[serde(rename = "AuthKey", skip_serializing_if = "Option::is_none")] - pub auth_key: Option, + pub(crate) auth_key: Option>, +} + +impl AuthInfo { + /// Wrap a pre-auth key so its backing allocation is zeroed when this + /// value (or the [`RegisterRequest`] that owns it) is dropped. + /// + /// Time: O(n) — copies `auth_key`'s bytes into a fresh allocation, where + /// `n` is `auth_key.len()`. + /// Space: O(n) — one allocation the size of `auth_key`. + #[must_use] + pub fn new(auth_key: &str) -> Self { + Self { + auth_key: Some(Zeroizing::new(auth_key.to_string())), + } + } } impl fmt::Debug for AuthInfo { diff --git a/crates/mitos/src/types/tests.rs b/crates/mitos/src/types/tests.rs index 56e1f7f..64d90e2 100644 --- a/crates/mitos/src/types/tests.rs +++ b/crates/mitos/src/types/tests.rs @@ -8,6 +8,8 @@ reason = "tests use expect() for invariants that must hold" )] +use zeroize::Zeroize; + use super::*; /// WHY(#54): the failure this guards is not a wrong value but a refusal to @@ -42,9 +44,7 @@ fn register_request_serializes_to_json() { let req = RegisterRequest { node_key: "nodekey:abc123".to_string(), old_node_key: String::new(), - auth: Some(AuthInfo { - auth_key: Some("tskey-auth-test".to_string()), - }), + auth: Some(AuthInfo::new("tskey-auth-test")), hostinfo: Hostinfo { backend_log_id: BackendLogId::new("log123"), os: "linux".to_string(), @@ -79,6 +79,14 @@ fn register_request_serializes_to_json() { json.contains("\"dictyon/0.1.0\""), "GoVersion value wrong: {json}" ); + // The AuthKey field must survive the String -> Zeroizing + // wrapper unchanged: exact value, not merely "Auth present" (Zeroizing's + // serde impl delegates to String's, but that delegation is exactly what + // this test exists to hold in place). + assert!( + json.contains("\"AuthKey\":\"tskey-auth-test\""), + "AuthKey value wrong: {json}" + ); // Followup should be omitted when None assert!( @@ -278,9 +286,7 @@ fn map_response_deserializes_peer_removals_by_node_id_and_key() { /// fails if the manual impl is dropped or a derive is reinstated. #[test] fn auth_info_debug_redacts_the_pre_auth_key() { - let info = AuthInfo { - auth_key: Some("tskey-auth-kSeCrEtValue".to_string()), - }; + let info = AuthInfo::new("tskey-auth-kSeCrEtValue"); let rendered = format!("{info:?}"); @@ -301,9 +307,7 @@ fn register_request_debug_does_not_leak_the_nested_pre_auth_key() { let request = RegisterRequest { node_key: "nodekey:abc".to_string(), old_node_key: String::new(), - auth: Some(AuthInfo { - auth_key: Some("tskey-auth-kSeCrEtValue".to_string()), - }), + auth: Some(AuthInfo::new("tskey-auth-kSeCrEtValue")), hostinfo: Hostinfo { backend_log_id: BackendLogId::new("log-id"), os: "linux".to_string(), @@ -336,3 +340,53 @@ fn auth_info_debug_distinguishes_absent_from_redacted() { "an absent key must not claim to be redacted: {absent}" ); } + +/// WHY this is the one place `unsafe` is justified in this crate: a safe +/// accessor cannot distinguish "scrubbed" from "never touched" here, because +/// `String::zeroize` overwrites every byte of the backing buffer and *then* +/// truncates the logical length to zero (`zeroize-1.8.2` `src/lib.rs`, the +/// `Vec` impl) — so `as_bytes`/`as_str` read an empty string either way. +/// Proving the write actually happened means reading the buffer at the +/// address it was always at, which safe `String`/`Vec` APIs will not do once +/// the length is zero. +/// +/// This exercises the exact call `Zeroizing`'s `Drop` makes +/// (`Zeroize::zeroize`), without dropping the wrapper first: dropping would +/// free the allocation, and reading through a dangling pointer afterward +/// would be a second, unrelated unsoundness this test has no need to risk. +#[test] +#[expect( + unsafe_code, + reason = "reads the still-live backing buffer of a String after zeroize() \ + truncates its length to 0, which is the only way to observe \ + that the bytes were actually overwritten rather than merely \ + hidden by the truncation; see the WHY above" +)] +fn scrubbing_the_pre_auth_key_zeroes_its_backing_bytes() { + let info = AuthInfo::new("tskey-auth-kSeCrEtValue"); + let mut key: Zeroizing = info.auth_key.expect("AuthInfo::new always sets Some(..)"); + let ptr = key.as_ptr(); + let len = key.len(); + assert_eq!( + len, + "tskey-auth-kSeCrEtValue".len(), + "sanity: key not yet touched" + ); + + key.zeroize(); + + // SAFETY: `ptr` was obtained from `key.as_ptr()` above and `key` is + // still alive and un-dropped at this point (it is dropped for real at + // the end of this function), so the allocation is still live and has + // not moved or been reallocated -- `zeroize()` mutates in place. `len` + // is the byte count that was valid (and initialized) immediately before + // the call, and `Vec::zeroize` overwrites every one of those bytes + // via `iter_mut().zeroize()` before it truncates the logical length, so + // all `len` bytes at `ptr` are both in-bounds of the live allocation and + // were written through, not merely allocated. + let bytes = unsafe { std::slice::from_raw_parts(ptr, len) }; + assert!( + bytes.iter().all(|&b| b == 0), + "pre-auth key bytes were not zeroed by Zeroize::zeroize", + ); +} diff --git a/crates/mitos/tests/public_api.rs b/crates/mitos/tests/public_api.rs index 5fc7875..ec1df15 100644 --- a/crates/mitos/tests/public_api.rs +++ b/crates/mitos/tests/public_api.rs @@ -83,9 +83,7 @@ fn register_request_omits_none_fields() { let req = RegisterRequest { node_key: "nodekey:abc".to_string(), old_node_key: String::new(), - auth: Some(AuthInfo { - auth_key: Some("tskey-auth-test".to_string()), - }), + auth: Some(AuthInfo::new("tskey-auth-test")), hostinfo: Hostinfo { backend_log_id: BackendLogId::new("log"), os: "linux".to_string(), @@ -103,6 +101,13 @@ fn register_request_omits_none_fields() { json.contains("\"Auth\""), "Some Auth should be present: {json}" ); + // `"Auth"` alone would still pass for a nested AuthKey that were + // missing, empty, or wrong-shaped ("Auth":{} also contains "Auth") — + // pin the exact nested value so this test can actually fail on that. + assert!( + json.contains("\"AuthKey\":\"tskey-auth-test\""), + "AuthKey value should be present under Auth: {json}" + ); } #[test]