From d702a2147af344c03efaa13fa280290d908bfdec Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 20:33:53 -0500 Subject: [PATCH 1/4] fix(mitos): zeroize AuthInfo::auth_key on drop Pre-auth keys are reusable, unattended-enrollment credentials, and a plain Option leaves the heap allocation holding one behind after use -- recoverable from a core dump, /proc//mem, or swap. Wrap the field in zeroize::Zeroizing (serde feature enabled so the wire format is unchanged) and route every construction through AuthInfo::new, which puts the key text into exactly one allocation, wrapped immediately, so no separate unwrapped copy is left at the call site. The field goes pub(crate): a direct auth_key: None literal would also produce a wire shape (an empty Auth object) no real caller wants. --- Cargo.toml | 2 +- crates/dictyon/src/control/mod.rs | 9 +++-- crates/mitos/src/types/mod.rs | 34 +++++++++++++++- crates/mitos/src/types/tests.rs | 64 ++++++++++++++++++++++++++----- crates/mitos/tests/public_api.rs | 4 +- 5 files changed, 95 insertions(+), 18 deletions(-) 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/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..ca3d3a8 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(), @@ -278,9 +278,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 +299,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 +332,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..b3625a6 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(), From 381a5c07bddd3faf7899fdfc17670dd5463f9906 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 20:39:42 -0500 Subject: [PATCH 2/4] fix(mitos): commit the zeroize serde-feature lockfile edge Cargo.toml enabled zeroize's serde feature but Cargo.lock still listed zeroize with no serde dependency edge, so a --locked build would have diverged from what cargo actually resolves. --- Cargo.lock | 1 + 1 file changed, 1 insertion(+) diff --git a/Cargo.lock b/Cargo.lock index 20afa93..ce91341 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1813,6 +1813,7 @@ version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" dependencies = [ + "serde", "zeroize_derive", ] From 0b7102828f99982cdf254bb720e6e7332553eb68 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 21:27:02 -0500 Subject: [PATCH 3/4] fix(dictyon): zeroize TS_AUTHKEY at the point it leaves the environment AuthInfo::auth_key was wrapped in Zeroizing, but the crate's own shipped reference implementation (examples/connect.rs) read TS_AUTHKEY into a plain Option that outlived registration for the rest of the process, including the unbounded map-stream loop. That raw allocation was never scrubbed -- the fix protected a copy, not the secret's origin. Wrap the env-var read in Zeroizing in the same statement that allocates it (mirroring AuthInfo::new: one allocation, wrapped before any other binding can hold an unwrapped copy), and drop it explicitly once registration no longer needs it rather than letting it live to the end of run(). --- Cargo.lock | 1 + crates/dictyon/Cargo.toml | 1 + crates/dictyon/examples/connect.rs | 20 ++++++++++++++++++-- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ce91341..c2a5d0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -374,6 +374,7 @@ dependencies = [ "tokio-rustls", "tracing", "webpki-roots", + "zeroize", "zstd", ] 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(()) } From d41e244b78eb150b27f2ae36b877f60a9a13da56 Mon Sep 17 00:00:00 2001 From: forkwright Date: Sat, 15 Aug 2026 21:28:11 -0500 Subject: [PATCH 4/4] test(mitos): pin the AuthKey wire value in the two tests PR body cites --- crates/mitos/src/types/tests.rs | 8 ++++++++ crates/mitos/tests/public_api.rs | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/crates/mitos/src/types/tests.rs b/crates/mitos/src/types/tests.rs index ca3d3a8..64d90e2 100644 --- a/crates/mitos/src/types/tests.rs +++ b/crates/mitos/src/types/tests.rs @@ -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!( diff --git a/crates/mitos/tests/public_api.rs b/crates/mitos/tests/public_api.rs index b3625a6..ec1df15 100644 --- a/crates/mitos/tests/public_api.rs +++ b/crates/mitos/tests/public_api.rs @@ -101,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]