From b415316e59e8433e710f5d27a53bde1c03093d46 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 29 Jul 2026 11:15:45 -0700 Subject: [PATCH 1/2] ts_keys: require more ceremony to serialize private keys Fixes #305 Signed-off-by: David Anderson Change-Id: I5bf5b07370c2bf3e8d09eb4f5d3bdcbe6a6a6964 --- ts_keys/src/dalek.rs | 23 +++ ts_keys/src/keystate.rs | 47 ++--- ts_keys/src/lib.rs | 135 ++++++++++--- ts_keys/src/macros.rs | 431 ++++++++++++++++------------------------ ts_keys/src/util.rs | 92 +++++++++ 5 files changed, 417 insertions(+), 311 deletions(-) create mode 100644 ts_keys/src/dalek.rs create mode 100644 ts_keys/src/util.rs diff --git a/ts_keys/src/dalek.rs b/ts_keys/src/dalek.rs new file mode 100644 index 00000000..00aef869 --- /dev/null +++ b/ts_keys/src/dalek.rs @@ -0,0 +1,23 @@ +//! Utility type for working with the x25519_dalek crate. + +/// An x25519_dalek private key, and its associated public key. +/// +/// This exists because the x25519_dalek crate doesn't have a pair type, which results in +/// awkward APIs and pubkey recomputations when we have a strongly typed keypair of our own that +/// we want to convert cheaply for crypto ops. +#[derive(Clone)] +pub struct X25519KeyPair { + /// The keypair's public key. + pub public: ::x25519_dalek::PublicKey, + /// The keypair's private key. + pub private: ::x25519_dalek::StaticSecret, +} + +impl X25519KeyPair { + /// Return a new random keypair. + pub fn random() -> Self { + let private = ::x25519_dalek::StaticSecret::random(); + let public = ::x25519_dalek::PublicKey::from(&private); + Self { public, private } + } +} diff --git a/ts_keys/src/keystate.rs b/ts_keys/src/keystate.rs index 37e6cfb7..8d095d63 100644 --- a/ts_keys/src/keystate.rs +++ b/ts_keys/src/keystate.rs @@ -1,9 +1,6 @@ use core::fmt::{Debug, Display, Formatter}; -use crate::{ - DiscoKeyPair, MachineKeyPair, MachinePrivateKey, NetworkLockKeyPair, NetworkLockPrivateKey, - NodeKeyPair, NodePrivateKey, -}; +use crate::{DiscoKeyPair, Export, ExportableKey, MachineKeyPair, NetworkLockKeyPair, NodeKeyPair}; /// The portion of the key state that should be retained between runs of the same device. /// @@ -12,22 +9,22 @@ use crate::{ #[derive(Clone, Debug)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] pub struct PersistState { - /// The [`MachinePrivateKey`] for the hardware this Tailnet peer runs on. - pub machine_key: MachinePrivateKey, + /// The [`crate::MachinePrivateKey`] for the hardware this Tailnet peer runs on. + pub machine_key: Export, - /// The [`NetworkLockPrivateKey`] for this Tailnet peer, for use with Tailnet Lock. - pub network_lock_key: NetworkLockPrivateKey, + /// The [`crate::NetworkLockPrivateKey`] for this Tailnet peer, for use with Tailnet Lock. + pub network_lock_key: Export, - /// The [`NodePrivateKey`] for this Tailnet peer. - pub node_key: NodePrivateKey, + /// The [`crate::NodePrivateKey`] for this Tailnet peer. + pub node_key: Export, } impl From<&NodeState> for PersistState { fn from(value: &NodeState) -> Self { Self { - node_key: value.node_keys.private.clone(), - machine_key: value.machine_keys.private.clone(), - network_lock_key: value.network_lock_keys.private.clone(), + node_key: value.node_keys.export(), + machine_key: value.machine_keys.export(), + network_lock_key: value.network_lock_keys.export(), } } } @@ -41,16 +38,15 @@ impl From for PersistState { impl Default for PersistState { fn default() -> Self { Self { - machine_key: MachinePrivateKey::random(), - network_lock_key: NetworkLockPrivateKey::random(), - node_key: NodePrivateKey::random(), + machine_key: MachineKeyPair::random().export(), + network_lock_key: NetworkLockKeyPair::random().export(), + node_key: NodeKeyPair::random().export(), } } } /// The complete runtime key state for a Tailscale node. -#[derive(Clone, Default)] -#[cfg_attr(feature = "serde", derive(serde::Deserialize))] +#[derive(Clone)] pub struct NodeState { /// The [`DiscoKeyPair`] this Tailnet peer uses for the Disco protocol. /// @@ -87,17 +83,22 @@ impl Display for NodeState { impl NodeState { /// Generate a new [`NodeState`]. All keys get random values. pub fn generate() -> Self { - Default::default() + Self { + machine_keys: MachineKeyPair::random(), + node_keys: NodeKeyPair::random(), + disco_keys: DiscoKeyPair::random(), + network_lock_keys: NetworkLockKeyPair::random(), + } } } impl From<&PersistState> for NodeState { fn from(value: &PersistState) -> Self { Self { - disco_keys: Default::default(), - node_keys: value.node_key.clone().into(), - machine_keys: value.machine_key.clone().into(), - network_lock_keys: value.network_lock_key.clone().into(), + disco_keys: DiscoKeyPair::random(), + node_keys: value.node_key.import(), + machine_keys: value.machine_key.import(), + network_lock_keys: value.network_lock_key.import(), } } } diff --git a/ts_keys/src/lib.rs b/ts_keys/src/lib.rs index 08c4e74f..799aa43a 100644 --- a/ts_keys/src/lib.rs +++ b/ts_keys/src/lib.rs @@ -3,51 +3,125 @@ extern crate alloc; +pub mod dalek; mod keystate; mod macros; +mod util; +use alloc::string::ToString; +use core::{ + fmt, + fmt::{Debug, Display, Formatter}, + str::FromStr, +}; + +pub use dalek::X25519KeyPair; #[doc(inline)] pub use keystate::{NodeState, PersistState}; use macros::{ - _create_x25519_base_key_type, create_x25519_keypair_types, create_x25519_private_key_type, - create_x25519_public_key_type, + create_x25519_keypair_types, create_x25519_private_key_type, create_x25519_public_key_type, }; +#[cfg(feature = "serde")] +use serde::de::Error; + +use crate::util::write_hex; + +mod private { + use core::{fmt::Debug, str::FromStr}; -/// Errors that may occur when parsing a string into a key type. -#[derive(Debug, Copy, Clone, PartialEq, Eq, thiserror::Error)] -pub enum ParseError { - /// Key string was formatted incorrectly. - #[error("key string was formatted incorrectly")] - InvalidFormat, + use crate::util::ParseError; + + pub trait SealedExportable: Clone + Debug + FromStr { + const KEY_PREFIX: &'static str; + + fn as_bytes(&self) -> &[u8; 32]; + + fn from_bytes(bytes: [u8; 32]) -> Self; + } +} - /// Key was the wrong length. - #[error("key was the wrong length")] - WrongLength, +/// A key that can be exported for serialization. +pub trait ExportableKey: private::SealedExportable { + /// Convert the key to its serializable form. + fn export(&self) -> Export; +} - /// Parsed prefix did not match the key type. - #[error("parsed prefix did not match the key type")] - BadPrefix, +impl ExportableKey for T { + fn export(&self) -> Export { + Export(self.clone()) + } } -/// An x25519_dalek private key, and its associated public key. +/// A wrapped key that can be serialized, but not used for cryptographic operations. +/// +/// This type exists as a guard against accidentally serializing private keys: the regular +/// key type can be used for cryptographic operations, but cannot be serialized. Conversely, +/// this wrapper can be serialized, but not used for cryptography without first converting it +/// back to the underlying key type using [`Export::import`]. /// -/// This exists because the x25519_dalek crate doesn't have a pair type, which results in -/// awkward APIs and pubkey recomputations when we have a strongly typed keypair of our own that -/// we want to convert cheaply for crypto ops. +/// Keys should be converted to exportable form as close as possible to the point of +/// serialization, to make it harder to accidentally serialize the key at an unexpected point. #[derive(Clone)] -pub struct X25519KeyPair { - /// The keypair's public key. - pub public: ::x25519_dalek::PublicKey, - /// The keypair's private key. - pub private: ::x25519_dalek::StaticSecret, +pub struct Export(T); + +impl Export { + /// Remove the export wrapper, returning the underlying key that can be used for + /// cryptographic operations. + pub fn import(&self) -> T { + self.0.clone() + } + + /// Create a key from its raw byte representation. + pub fn from_bytes(bytes: [u8; 32]) -> Self { + Self(T::from_bytes(bytes)) + } + + /// Return the raw byte representation of the key. + pub fn as_bytes(&self) -> &[u8; 32] { + self.0.as_bytes() + } +} + +impl Debug for Export { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + // The debug impl is redacted even for exportable keys. It's never correct to dump a private + // key into logs. + Debug::fmt(&self.0, f) + } +} + +impl Display for Export { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write_hex(self.0.as_bytes(), T::KEY_PREFIX, f) + } } -impl X25519KeyPair { - /// Return a new random keypair. - pub fn random() -> Self { - let private = ::x25519_dalek::StaticSecret::random(); - let public = ::x25519_dalek::PublicKey::from(&private); - Self { public, private } +impl FromStr for Export { + type Err = ::Err; + + fn from_str(s: &str) -> Result { + ::from_str(s).map(Export) + } +} + +#[cfg(feature = "serde")] +impl<'de, T: ExportableKey> serde::Deserialize<'de> for Export { + fn deserialize(deserializer: D) -> Result + where + D: ::serde::Deserializer<'de>, + { + let s = <&str>::deserialize(deserializer)?; + T::from_str(s).map_err(D::Error::custom).map(Self) + } +} + +#[cfg(feature = "serde")] +impl ::serde::Serialize for Export { + fn serialize(&self, serializer: S) -> Result + where + S: ::serde::Serializer, + { + serializer.serialize_str(&self.to_string()) } } @@ -59,13 +133,12 @@ create_x25519_public_key_type!( "chalpub" ); -// The client never handles DERP server private keys, so we only create a public key type rather -// than public/private/keypair types. create_x25519_public_key_type!( /// The X25519 public key of a DERP server. DerpServerPublicKey, "derp" ); + create_x25519_keypair_types!( /// The X25519 public key a Tailscale node uses for the Disco protocol. DiscoPublicKey, diff --git a/ts_keys/src/macros.rs b/ts_keys/src/macros.rs index 01c68166..1a0c7736 100644 --- a/ts_keys/src/macros.rs +++ b/ts_keys/src/macros.rs @@ -1,364 +1,281 @@ /// Generates a struct that implements all the fields/methods needed by both public and private /// X25519 keys. Used by `create_x25519_{public_key, private_key, keypair}_type{s}` macros, not /// intended to be used by itself. -macro_rules! _create_x25519_base_key_type { - ($(#[$attr:meta])* $key_name:ident, $key_prefix:literal) => { +macro_rules! create_x25519_public_key_type { + ($(#[$attr:meta])* $public_name:ident, $key_prefix:literal) => { $(#[$attr])* - #[derive(Clone, Eq, PartialEq, ::zerocopy::FromBytes, ::zerocopy::Immutable, ::zerocopy::IntoBytes, ::zerocopy::KnownLayout)] - pub struct $key_name( - [u8; $key_name::KEY_LEN_BYTES] - ); - - impl $key_name { - /// The length of this key type, in bytes. - pub const KEY_LEN_BYTES: usize = 32; - /// The length of a hexidecimal string representation of this key, excluding the - /// prefix and colon. - pub const KEY_LEN_HEX_STR: usize = $key_name::KEY_LEN_BYTES * 2; - /// The length of a hexidecimal string representation of this key, including the - /// prefix and colon. - pub const KEY_LEN_FULL_STR: usize = $key_name::KEY_LEN_HEX_STR + $key_name::KEY_PREFIX.len() + 1; - /// The prefix placed in front of string representations of this key type, such - /// as "$key_prefix:abcd..." - pub const KEY_PREFIX: &'static str = $key_prefix; - - /// Return this key as a `u8` byte array. - pub fn to_bytes(&self) -> [u8; $key_name::KEY_LEN_BYTES] { - self.0 + #[derive( + Clone, + Eq, + PartialEq, + Ord, + PartialOrd, + Hash, + ::zerocopy::Immutable, + ::zerocopy::FromBytes, + ::zerocopy::IntoBytes, + ::zerocopy::KnownLayout, + ::zerocopy::Unaligned, + )] + #[repr(C)] + pub struct $public_name([u8; 32]); + + impl $public_name { + /// Create a new random public key. + /// + /// The corresponding private key is discarded, so this is only useful for tests + /// that need a public key that will never be used for communication. + pub fn random() -> Self { + Self($crate::util::random_x25519_public()) } - fn write_key_str(&self, out: &mut impl ::alloc::fmt::Write) -> ::core::fmt::Result { - ::core::write!(out, "{}:", Self::KEY_PREFIX)?; - for b in self.0.iter() { - ::core::write!(out, "{b:02x}")?; - } - Ok(()) + /// Convert the key to a `crypto_box` [`::crypto_box::PublicKey`], to perform cryptographic operations. + /// + /// The `crypto_box` type does not preserve the key's Tailscale purpose. You should convert + /// keys to the `crypto_box` type as close as possible to the cryptographic operations. + pub fn to_crypto_box(&self) -> ::crypto_box::PublicKey { + self.0.into() } - /// Return this key as a hex-encoded string. + /// Convert the key to an `x25519_dalek` [`::x25519_dalek::PublicKey`], to perform cryptographic operations. /// - /// This method exists instead of a Display/ToString impl because the latter makes it - /// very easy to inadvertently print secret key material indirectly, through derived - /// impls that delegate to the standard traits. + /// The `x25519_dalek` type does not preserve the key's Tailscale purpose. You should + /// convert keys to the `x25519_dalek` type as close as possible to the cryptographic operations. + pub fn to_x25519_dalek(&self) -> ::x25519_dalek::PublicKey { + self.0.into() + } + + /// Create a key from a raw byte array. /// - /// This method exists for situations where a key truly does need to be formatted as - /// a hex string, e.g. for credential storage. - pub fn to_key_str(&self) -> ::alloc::string::String { - let mut ret = ::alloc::string::String::with_capacity(Self::KEY_LEN_FULL_STR); - self.write_key_str(&mut ret).unwrap(); - ret + /// Use this sparingly, as it makes it easy to convert bytes into the wrong type of key. + /// For serialization, prefer embedding the key type directly into a struct, and relying + /// on zerocopy or serde serialization. + pub fn from_bytes(b: [u8; 32]) -> Self { + Self(b) } } - impl ::core::str::FromStr for $key_name { - type Err = $crate::ParseError; - - fn from_str(s: &str) -> Result { - if s.len() != $key_name::KEY_LEN_FULL_STR { - return Err($crate::ParseError::WrongLength); - } + // TODO: get rid of this default impl. Primary user is ts_control_serde's MapRequest/MapResponse. + impl ::core::default::Default for $public_name { + fn default() -> Self { + Self::random() + } + } - let mut parts = s.split(':'); - let Some(prefix) = parts.next() else { - return Err($crate::ParseError::InvalidFormat); - }; - if prefix != $key_name::KEY_PREFIX { - return Err($crate::ParseError::BadPrefix); - } + // TODO: get rid of the copy trait. It encourages too much silent copying for a type of marginal size. + impl ::core::marker::Copy for $public_name {} - let Some(hex_str) = parts.next() else { - return Err($crate::ParseError::WrongLength); - }; - if hex_str.len() != $key_name::KEY_LEN_HEX_STR { - return Err($crate::ParseError::WrongLength); - } + impl $crate::private::SealedExportable for $public_name { + const KEY_PREFIX: &'static str = $key_prefix; - // s.split(':') should only return 2 parts: the prefix and the hex string. If - // the string contained additional colons, it's malformed and not a valid key - // string. - if parts.next().is_some() { - return Err($crate::ParseError::InvalidFormat) - } + fn as_bytes(&self) -> &[u8; 32] { + &self.0 + } - let mut key = $key_name([0u8; $key_name::KEY_LEN_BYTES]); - for i in (0..$key_name::KEY_LEN_HEX_STR).step_by(2) { - let slice = hex_str.get(i..i + 2).unwrap(); - let keyidx = i / 2; - let x = u8::from_str_radix(slice, 16).map_err(|_| $crate::ParseError::InvalidFormat)?; - key.0[keyidx] = x; - } - Ok(key) + fn from_bytes(bytes: [u8; 32]) -> Self { + $public_name(bytes) } } - impl From<[u8; $key_name::KEY_LEN_BYTES]> for $key_name { - fn from(v: [u8; $key_name::KEY_LEN_BYTES]) -> Self { - $key_name(v) + impl ::core::str::FromStr for $public_name { + type Err = $crate::util::ParseError; + + fn from_str(s: &str) -> Result { + $crate::util::parse_hex(s, $key_prefix).map($public_name) } } - impl From<$key_name> for [u8; $key_name::KEY_LEN_BYTES] { - fn from(v: $key_name) -> [u8; $key_name::KEY_LEN_BYTES] { - v.0 + impl ::core::fmt::Debug for $public_name { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + $crate::util::write_hex(&self.0, $key_prefix, f) } } - impl From<&$key_name> for [u8; $key_name::KEY_LEN_BYTES] { - fn from(v: &$key_name) -> [u8; $key_name::KEY_LEN_BYTES] { - v.0 + impl ::core::fmt::Display for $public_name { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + $crate::util::write_hex(&self.0, $key_prefix, f) } } #[cfg(feature = "serde")] - impl<'de> ::serde::Deserialize<'de> for $key_name { - fn deserialize(deserializer: D) -> ::core::result::Result<$key_name, D::Error> where D: ::serde::Deserializer<'de> { - use ::core::str::FromStr; - - struct KeyVisitor; - - impl<'a> ::serde::de::Visitor<'a> for KeyVisitor { - type Value = $key_name; - - fn expecting(&self, formatter: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { - ::core::write!( - formatter, - "a {}-character string with the prefix '{}:' followed by {} hex characters", - $key_name::KEY_LEN_FULL_STR, $key_name::KEY_PREFIX, $key_name::KEY_LEN_HEX_STR - ) - } - - fn visit_str(self, value: &str) -> ::core::result::Result where E: ::serde::de::Error { - $key_name::from_str(value).map_err(|e| ::serde::de::Error::custom(e)) - } - } - - deserializer.deserialize_str(KeyVisitor) + impl<'de> ::serde::Deserialize<'de> for $public_name { + fn deserialize(deserializer: D) -> ::core::result::Result<$public_name, D::Error> where D: ::serde::Deserializer<'de> { + let s = <&str>::deserialize(deserializer)?; + $crate::util::parse_hex(s, $key_prefix).map_err(::serde::de::Error::custom).map($public_name) } } #[cfg(feature = "serde")] - impl ::serde::Serialize for $key_name { + impl ::serde::Serialize for $public_name { fn serialize(&self, serializer: S) -> ::core::result::Result where S: ::serde::Serializer { - serializer.serialize_str(&self.to_key_str()) + serializer.serialize_str(&$crate::util::to_hex_string(&self.0, $key_prefix)) } } } } -/// Generates a struct that implements all the fields/methods needed by X25519 public keys. -macro_rules! create_x25519_public_key_type { - ($(#[$attr:meta])* $public_name:ident, $key_prefix:literal) => { - _create_x25519_base_key_type!($(#[$attr])* #[derive(Copy, Default, Hash, PartialOrd, Ord)] $public_name, $key_prefix); - - impl From<$public_name> for ::x25519_dalek::PublicKey { - fn from(v: $public_name) -> Self { - v.0.into() - } - } - - impl From<$public_name> for ::crypto_box::PublicKey { - fn from(v: $public_name) -> Self { - v.0.into() - } - } +macro_rules! create_x25519_private_key_type { + ($(#[$attr:meta])* $private_name:ident, $key_prefix:literal, $public_name:ident, $pair_name:ident) => { + $(#[$attr])* + #[doc = concat!("If you need both the public and private components of the key, use ", stringify!($pair_name), " to avoid repeated recomputation of the public key.")] + #[derive(Clone, Eq, PartialEq, ::zeroize::ZeroizeOnDrop)] + pub struct $private_name([u8; 32]); - impl From<&$public_name> for ::x25519_dalek::PublicKey { - fn from(v: &$public_name) -> Self { - v.0.into() + impl $private_name { + /// Create a new random private key. + pub fn random() -> Self { + Self($crate::util::random_x25519_private()) } - } - impl From<&$public_name> for ::crypto_box::PublicKey { - fn from(v: &$public_name) -> Self { - v.0.into() + /// Compute the public key counterpart of this private key. + #[doc = concat!("Consider instead converting the private key to a ", stringify!($pair_name), ", which caches the public key computation.")] + pub fn public_key(&self) -> $public_name { + let private = self.to_x25519_dalek(); + let public = ::x25519_dalek::PublicKey::from(&private); + $public_name(public.to_bytes()) } - } - impl ::core::fmt::Debug for $public_name { - fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { - ::core::write!(f, "{self}") + /// Convert the key to a `crypto_box` [`::crypto_box::SecretKey`], to perform cryptographic operations. + /// + /// The `crypto_box` type does not preserve the key's Tailscale purpose. You should convert + /// keys to the `crypto_box` type as close as possible to the cryptographic operations. + pub fn to_crypto_box(&self) -> ::crypto_box::SecretKey { + self.0.into() } - } - impl ::core::fmt::Display for $public_name { - fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { - self.write_key_str(f) + /// Convert the key to an `x25519_dalek` [`::x25519_dalek::StaticSecret`], to perform cryptographic operations. + /// + /// The `x25519_dalek` type does not preserve the key's Tailscale purpose. You should + /// convert keys to the `x25519_dalek` type as close as possible to the cryptographic operations. + pub fn to_x25519_dalek(&self) -> ::x25519_dalek::StaticSecret { + self.0.into() } } - } -} -/// Generates a struct that implements all the fields/methods needed by X25519 private keys. -macro_rules! create_x25519_private_key_type { - ($(#[$attr:meta])* $private_name:ident, $public_name:ident, $key_prefix:literal) => { - _create_x25519_base_key_type!($(#[$attr])* #[derive(::zeroize::ZeroizeOnDrop)] $private_name, $key_prefix); + impl $crate::private::SealedExportable for $private_name { + const KEY_PREFIX: &'static str = $key_prefix; - impl $private_name { - /// Generate a new X25519 private key. - pub fn random() -> Self { - $private_name(::x25519_dalek::StaticSecret::random().to_bytes()) + fn as_bytes(&self) -> &[u8; 32] { + &self.0 } - /// Calculate the corresponding public key for this private key. - pub fn public_key(&self) -> $public_name { - ::crypto_box::SecretKey::from(self).public_key().to_bytes().into() + fn from_bytes(bytes: [u8; 32]) -> Self { + $private_name(bytes) } } - impl From<$private_name> for ::x25519_dalek::StaticSecret { - fn from(v: $private_name) -> Self { - v.0.into() - } - } + impl ::core::str::FromStr for $private_name { + type Err = $crate::util::ParseError; - impl From<&$private_name> for ::x25519_dalek::StaticSecret { - fn from(v: &$private_name) -> Self { - v.0.into() + fn from_str(s: &str) -> Result { + $crate::util::parse_hex(s, $key_prefix).map($private_name) } } - impl From<$private_name> for ::crypto_box::SecretKey { - fn from(v: $private_name) -> Self { - v.0.into() + impl ::core::fmt::Debug for $private_name { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + ::core::write!(f, "{}:[redacted]", $key_prefix) } } - impl From<&$private_name> for ::crypto_box::SecretKey { - fn from(v: &$private_name) -> Self { - v.0.into() + impl ::core::fmt::Display for $private_name { + fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result { + ::core::write!(f, "{}:[redacted]", $key_prefix) } } - impl ::core::fmt::Debug for $private_name { - fn fmt(&self, f: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { - ::core::write!(f, "[redacted]") + #[cfg(feature = "serde")] + impl<'de> ::serde::Deserialize<'de> for $private_name { + fn deserialize(deserializer: D) -> ::core::result::Result<$private_name, D::Error> where D: ::serde::Deserializer<'de> { + let s = <&str>::deserialize(deserializer)?; + $crate::util::parse_hex(s, $key_prefix).map_err(::serde::de::Error::custom).map($private_name) } } } } -/// Generates the public key, private key, and key pair structs with all the fields/methods needed -/// to work with X25519 keys. macro_rules! create_x25519_keypair_types { - ($(#[$public_attr:meta])* $public_name:ident, $public_prefix:literal, $(#[$private_attr:meta])* $private_name:ident, $private_prefix:literal, $(#[$pair_attr:meta])* $keypair_name:ident) => { - create_x25519_public_key_type! { $(#[$public_attr])* $public_name, $public_prefix } - create_x25519_private_key_type! { $(#[$private_attr])* $private_name, $public_name, $private_prefix } - - impl From<$private_name> for $public_name { - fn from(v: $private_name) -> Self { - let private = ::x25519_dalek::StaticSecret::from(v.0); - let public = ::x25519_dalek::PublicKey::from(&private); - $public_name(public.to_bytes()) - } - } + ( + $(#[$public_attr:meta])* + $public_name:ident, + $public_prefix:literal, + $(#[$private_attr:meta])* + $private_name:ident, + $private_prefix:literal, + $(#[$pair_attr:meta])* + $pair_name:ident + ) => { + create_x25519_public_key_type!($(#[$public_attr])* $public_name, $public_prefix); + create_x25519_private_key_type!($(#[$private_attr])* $private_name, $private_prefix, $public_name, $pair_name); $(#[$pair_attr])* - #[cfg_attr(feature = "serde", derive(::serde::Deserialize, ::serde::Serialize))] - #[derive(Clone, Debug, Eq, PartialEq, ::zerocopy::FromBytes, ::zerocopy::Immutable, ::zerocopy::IntoBytes, ::zerocopy::KnownLayout)] - pub struct $keypair_name { - /// This keypair's public key. + #[derive(Clone, Debug, Eq, PartialEq)] + pub struct $pair_name { + /// The public half of the key pair. pub public: $public_name, - /// This keypair's private key. + /// The private half of the key pair. pub private: $private_name, } - impl $keypair_name { - /// Generate a new X25519 public/private key pair. - pub fn new() -> Self { - let private = $private_name::random(); - let public = private.public_key(); - Self { - private, - public, - } - } - } - - impl Default for $keypair_name { - fn default() -> Self { - Self::new() - } - } - - impl From<$private_name> for $keypair_name { - fn from(private: $private_name) -> Self { - let public = private.public_key(); - Self { - private, - public, - } + impl $pair_name { + /// Create a new random keypair. + pub fn random() -> Self { + $private_name::random().into() } - } - impl From<$keypair_name> for X25519KeyPair { - fn from(v: $keypair_name) -> Self { - X25519KeyPair{ - public: v.public.into(), - private: v.private.into(), - } - } - } - - impl From<&$keypair_name> for X25519KeyPair { - fn from(v: &$keypair_name) -> Self { - X25519KeyPair{ - public: (&v.public).into(), - private: (&v.private).into(), + /// Convert the key to an `x25519_dalek` keypair, to perform cryptographic operations. + /// + /// The `x25519_dalek` type does not preserve the key's Tailscale purpose. You should + /// convert keys to the `x25519_dalek` type as close as possible to the cryptographic operations. + pub fn to_x25519_dalek(&self) -> $crate::dalek::X25519KeyPair { + $crate::dalek::X25519KeyPair{ + public: self.public.to_x25519_dalek(), + private: self.private.to_x25519_dalek(), } } } - impl From<$keypair_name> for ::x25519_dalek::PublicKey { - fn from(v: $keypair_name) -> Self { - v.public.into() - } - } + impl $crate::private::SealedExportable for $pair_name { + const KEY_PREFIX: &'static str = $private_prefix; - impl From<&$keypair_name> for ::x25519_dalek::PublicKey { - fn from(v: &$keypair_name) -> Self { - v.public.into() + fn as_bytes(&self) -> &[u8; 32] { + self.private.as_bytes() } - } - impl From<$keypair_name> for ::crypto_box::PublicKey { - fn from(v: $keypair_name) -> Self { - v.public.into() + fn from_bytes(bytes: [u8; 32]) -> Self { + $private_name(bytes).into() } } - impl From<&$keypair_name> for ::crypto_box::PublicKey { - fn from(v: &$keypair_name) -> Self { - v.public.into() - } - } + impl ::core::str::FromStr for $pair_name { + type Err = $crate::util::ParseError; - impl From<$keypair_name> for ::x25519_dalek::StaticSecret { - fn from(v: $keypair_name) -> Self { - v.private.into() + fn from_str(s: &str) -> Result { + $private_name::from_str(s).map(Self::from) } } - impl From<&$keypair_name> for ::x25519_dalek::StaticSecret { - fn from(v: &$keypair_name) -> Self { - (&v.private).into() + impl From<$private_name> for $pair_name { + fn from(private: $private_name) -> Self { + let public = private.public_key(); + Self { public, private } } } - impl From<$keypair_name> for ::crypto_box::SecretKey { - fn from(v: $keypair_name) -> Self { - v.private.into() + impl AsRef<$public_name> for $pair_name { + fn as_ref(&self) -> &$public_name { + &self.public } } - impl From<&$keypair_name> for ::crypto_box::SecretKey { - fn from(v: &$keypair_name) -> Self { - (&v.private).into() + impl AsRef<$private_name> for $pair_name { + fn as_ref(&self) -> &$private_name { + &self.private } } - } + }; } -pub(crate) use _create_x25519_base_key_type; pub(crate) use create_x25519_keypair_types; pub(crate) use create_x25519_private_key_type; pub(crate) use create_x25519_public_key_type; diff --git a/ts_keys/src/util.rs b/ts_keys/src/util.rs new file mode 100644 index 00000000..bda0fd30 --- /dev/null +++ b/ts_keys/src/util.rs @@ -0,0 +1,92 @@ +use alloc::string::String; +use core::{fmt, fmt::Write}; + +/// Errors that may occur when parsing a string into a key type. +#[derive(Debug, Copy, Clone, PartialEq, Eq, thiserror::Error)] +pub enum ParseError { + /// Key string was formatted incorrectly. + #[error("key string was formatted incorrectly")] + InvalidFormat, + + /// Key was the wrong length. + #[error("key was the wrong length")] + WrongLength, + + /// Parsed prefix did not match the key type. + #[error("parsed prefix did not match the key type")] + BadPrefix, +} + +const X25519_LEN_HEX_STR: usize = 64; + +pub const fn key_hex_str_len(prefix: &'static str) -> usize { + prefix.len() + 1 + X25519_LEN_HEX_STR +} + +/// Parse a hex-formatted key into an untyped byte array. +/// +/// Keys are in the format `:<64 hex digits>`. The key string's +/// prefix must match the provided `want_prefix`. +pub fn parse_hex(s: &str, want_prefix: &'static str) -> Result<[u8; 32], ParseError> { + if s.len() != key_hex_str_len(want_prefix) { + return Err(ParseError::WrongLength); + } + + let mut parts = s.split(':'); + let Some(prefix) = parts.next() else { + return Err(ParseError::InvalidFormat); + }; + if prefix != want_prefix { + return Err(ParseError::BadPrefix); + } + + let Some(hex_str) = parts.next() else { + return Err(ParseError::WrongLength); + }; + if hex_str.len() != X25519_LEN_HEX_STR { + return Err(ParseError::WrongLength); + } + + // s.split(':') should only return 2 parts: the prefix and the hex string. If + // the string contained additional colons, it's malformed and not a valid key + // string. + if parts.next().is_some() { + return Err(ParseError::InvalidFormat); + } + + let mut key = [0u8; 32]; + for i in (0..X25519_LEN_HEX_STR).step_by(2) { + let slice = hex_str.get(i..i + 2).unwrap(); + let keyidx = i / 2; + let x = u8::from_str_radix(slice, 16).map_err(|_| ParseError::InvalidFormat)?; + key[keyidx] = x; + } + Ok(key) +} + +/// Write an untyped byte array representing a key to `out`, in tailscale's conventional hex +/// encoding. +/// +/// Keys are in the format `:<64 hex digits>`. +pub fn write_hex(key: &[u8; 32], prefix: &'static str, out: &mut impl Write) -> fmt::Result { + write!(out, "{}:", prefix)?; + for b in key.iter() { + write!(out, "{b:02x}")?; + } + Ok(()) +} + +pub fn to_hex_string(key: &[u8; 32], prefix: &'static str) -> String { + let mut ret = String::with_capacity(key_hex_str_len(prefix)); + write_hex(key, prefix, &mut ret).unwrap(); + ret +} + +pub fn random_x25519_private() -> [u8; 32] { + x25519_dalek::StaticSecret::random().to_bytes() +} + +pub fn random_x25519_public() -> [u8; 32] { + let private = x25519_dalek::StaticSecret::random(); + x25519_dalek::PublicKey::from(&private).to_bytes() +} From e4e4a875f632915d07501c19e0b9e30a7ef4b276 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 29 Jul 2026 11:15:45 -0700 Subject: [PATCH 2/2] ts_keys: require more ceremony to serialize private keys Fixes #305 Signed-off-by: David Anderson Change-Id: I5bf5b07370c2bf3e8d09eb4f5d3bdcbe6a6a6964 --- CHANGELOG.md | 9 +++ Cargo.lock | 2 + src/config.rs | 79 ++---------------------- src/lib.rs | 2 +- ts_cli_util/src/lib.rs | 2 +- ts_control_noise/src/handshake.rs | 9 ++- ts_derp/examples/listen.rs | 2 +- ts_derp/examples/ping.rs | 2 +- ts_derp/src/client.rs | 10 ++- ts_devtools/src/bin/derp_ping.rs | 4 +- ts_disco_protocol/src/lib.rs | 4 +- ts_disco_protocol/src/packet.rs | 21 ++++--- ts_elixir/native/ts_elixir/src/config.rs | 22 +++---- ts_ffi/src/keys.rs | 58 +++-------------- ts_python/Cargo.toml | 1 + ts_python/src/key_state.rs | 31 +++++----- ts_python/src/node_info.rs | 8 ++- ts_runtime/src/peer_tracker/peer_db.rs | 17 +++-- ts_transport/Cargo.toml | 1 + ts_transport/src/endpoint.rs | 3 +- ts_tunnel/examples/handshake.rs | 20 +++--- ts_tunnel/src/endpoint.rs | 4 +- ts_tunnel/src/handshake.rs | 19 +++--- ts_tunnel/src/macs.rs | 4 +- ts_tunnel/src/session.rs | 4 +- 25 files changed, 125 insertions(+), 213 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96744688..3ed10b1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,15 @@ Record breaking or significant changes here. All dates are UTC. Put changes for the upcoming release here! +- **Breaking** (Rust API): changed the key types of `tailscale::keys::PersistState` to new "exportable" keys. This + should require no changes to code if you're just serializing/deserializing `PersistStates` without touching the + contents. +- **Breaking** (Rust API): removed support for the "old" stored key format. State files produced by v0.1 and that + have not been loaded by a more recent version (up to v0.5 inclusive) will no longer load. +- **Breaking** (ts_keys): removed a number of methods and trait impls from key types, to make it harder to accidentally + leak private keys through serialization and type-erasing conversions. In code that requires the ability to serialize + private keys (e.g. state storage), a new `Export` wrapper type and corresponding `export()` methods on private and + pair types allow code to explicitly opt into serializability. - Changed (Rust API, lang bindings): the `TS_RS_EXPERIMENT` environment variable is no longer required to use the library. The library logs a warning during initialization, as a reminder that it's still work-in-progress software. - Updated MSRV to 1.97. diff --git a/Cargo.lock b/Cargo.lock index 9b3674a7..d105c5f7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6304,6 +6304,7 @@ dependencies = [ "pyo3-async-runtimes", "tailscale", "tracing-subscriber", + "zerocopy", ] [[package]] @@ -6383,6 +6384,7 @@ dependencies = [ "ts_hexdump", "ts_keys", "ts_packet", + "zerocopy", ] [[package]] diff --git a/src/config.rs b/src/config.rs index 493941d8..6a820def 100644 --- a/src/config.rs +++ b/src/config.rs @@ -2,11 +2,8 @@ use std::path::Path; -use serde::Serializer; use ts_keys::PersistState; -use crate::keys::NodeState; - const CONTROL_URL_VAR: &str = "TS_CONTROL_URL"; const HOSTNAME_VAR: &str = "TS_HOSTNAME"; const AUTHKEY_VAR: &str = "TS_AUTH_KEY"; @@ -104,68 +101,15 @@ pub async fn load_key_file( tracing::trace!(key_file = %p.display(), "loading key file"); - let key_file = load_or_init::( - &p, - Default::default, - |x| match x { - #[allow(deprecated)] - KeyFile::Old(old) => Some(KeyFile::New(KeyFileNew { - key_state: PersistState::from(&old.key_state), - })), - _ => None, - }, - bad_format, - ) - .await?; - Ok(key_file.key_state()) -} - -#[derive(serde::Deserialize)] -#[serde(untagged)] -enum KeyFile { - #[deprecated] - Old(KeyFileOld), - New(KeyFileNew), -} - -impl KeyFile { - #[allow(deprecated)] - pub fn key_state(&self) -> PersistState { - match self { - Self::Old(old) => (&old.key_state).into(), - Self::New(new) => new.key_state.clone(), - } - } -} - -impl Default for KeyFile { - fn default() -> Self { - KeyFile::New(KeyFileNew::default()) - } -} - -impl serde::Serialize for KeyFile { - fn serialize(&self, serializer: S) -> Result - where - S: Serializer, - { - KeyFileNew { - key_state: self.key_state(), - } - .serialize(serializer) - } + let key_file = load_or_init::(&p, Default::default, bad_format).await?; + Ok(key_file.key_state) } #[derive(serde::Deserialize, serde::Serialize, Default)] -struct KeyFileNew { +struct KeyFile { key_state: PersistState, } -#[derive(serde::Deserialize)] -struct KeyFileOld { - key_state: NodeState, -} - impl From<&Config> for ts_control::Config { fn from(value: &Config) -> ts_control::Config { ts_control::Config { @@ -210,7 +154,6 @@ pub enum BadFormatBehavior { async fn load_or_init( path: impl AsRef, default: impl FnOnce() -> KeyState, - migrate: impl FnOnce(&KeyState) -> Option, bad_format_behavior: BadFormatBehavior, ) -> Result where @@ -227,21 +170,7 @@ where match tokio::fs::read(path).await { Ok(contents) => match serde_json::from_slice::(&contents) { - Ok(state) => { - if let Some(migrated) = migrate(&state) { - match try_write(path, &migrated).await { - Ok(_) => { - tracing::info!("migrated key file to new disco-less format"); - return Ok(migrated); - } - Err(e) => { - tracing::error!(error = %e, "unable to migrate key file"); - } - } - } - - return Ok(state); - } + Ok(state) => return Ok(state), Err(e) => match bad_format_behavior { BadFormatBehavior::Error => { tracing::error!(error = %e, "parsing key file"); diff --git a/src/lib.rs b/src/lib.rs index b4cc7f3a..94935a8b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -369,7 +369,7 @@ pub mod netstack { pub mod keys { #[doc(inline)] pub use ts_keys::{ - DiscoKeyPair, DiscoPrivateKey, DiscoPublicKey, MachineKeyPair, MachinePrivateKey, + DiscoKeyPair, DiscoPrivateKey, DiscoPublicKey, Export, MachineKeyPair, MachinePrivateKey, MachinePublicKey, NetworkLockKeyPair, NetworkLockPrivateKey, NetworkLockPublicKey, NodeKeyPair, NodePrivateKey, NodePublicKey, NodeState, PersistState, }; diff --git a/ts_cli_util/src/lib.rs b/ts_cli_util/src/lib.rs index 0d26c597..8757cceb 100644 --- a/ts_cli_util/src/lib.rs +++ b/ts_cli_util/src/lib.rs @@ -69,7 +69,7 @@ impl CommonArgs { let conn = ts_control::connect( &config.control_server_url, - &config.key_state.machine_key.clone().into(), + &config.key_state.machine_key.import(), ) .await?; diff --git a/ts_control_noise/src/handshake.rs b/ts_control_noise/src/handshake.rs index e7ffe413..6d180224 100644 --- a/ts_control_noise/src/handshake.rs +++ b/ts_control_noise/src/handshake.rs @@ -37,8 +37,8 @@ impl Handshake { ) -> (Self, String) { let mut ciphertext = [0; SentHandshake::INIT_SIZE]; let state = SentHandshake::new( - node_machine_key.into(), - control_public_key.into(), + node_machine_key.to_x25519_dalek(), + control_public_key.to_x25519_dalek(), prologue.as_bytes(), &mut ciphertext, ); @@ -78,7 +78,10 @@ impl Handshake { return Err(Error::BadFormat); } - let session = match self.state.try_finish(&mut packet, node_machine_key.into()) { + let session = match self + .state + .try_finish(&mut packet, node_machine_key.to_x25519_dalek()) + { Ok(session) => session, Err(state) => { self.state = state; diff --git a/ts_derp/examples/listen.rs b/ts_derp/examples/listen.rs index f88a0d40..551eac02 100644 --- a/ts_derp/examples/listen.rs +++ b/ts_derp/examples/listen.rs @@ -13,7 +13,7 @@ async fn main() -> ts_cli_util::Result<()> { let derp_map = common::load_derp_map().await; let region = derp_map.get(&common::REGION_1).unwrap(); - let keypair = NodeKeyPair::new(); + let keypair = NodeKeyPair::random(); let client = ts_derp::Client::connect(region, &keypair).await?; tracing::info!("derp handshake done"); diff --git a/ts_derp/examples/ping.rs b/ts_derp/examples/ping.rs index a52be949..1cfc7a3a 100644 --- a/ts_derp/examples/ping.rs +++ b/ts_derp/examples/ping.rs @@ -14,7 +14,7 @@ async fn main() -> ts_cli_util::Result<()> { let derp_map = common::load_derp_map().await; let region = derp_map.get(&common::REGION_1).unwrap(); - let keypair = NodeKeyPair::new(); + let keypair = NodeKeyPair::random(); let client = ts_derp::Client::connect(region, &keypair).await?; tracing::info!("derp handshake done"); diff --git a/ts_derp/src/client.rs b/ts_derp/src/client.rs index b6e751a9..726ddb6d 100644 --- a/ts_derp/src/client.rs +++ b/ts_derp/src/client.rs @@ -236,7 +236,10 @@ fn make_clientinfo( node_keypair: &NodeKeyPair, server_key: &ts_keys::DerpServerPublicKey, ) -> Result<(ClientInfo, Vec), Error> { - let cbox = crypto_box::SalsaBox::new(&server_key.into(), &node_keypair.into()); + let cbox = crypto_box::SalsaBox::new( + &server_key.to_crypto_box(), + &node_keypair.private.to_crypto_box(), + ); let nonce = crypto_box::SalsaBox::generate_nonce(&mut OsRng); let json = serde_json::to_vec(&frame::ClientInfoPayload { @@ -266,7 +269,10 @@ fn decrypt_server_info( ) -> Result { let mut payload = PacketMut::from(payload); - let mut cbox = crypto_box::SalsaBox::new(&sk.key.into(), &node_keypair.into()); + let mut cbox = crypto_box::SalsaBox::new( + &sk.key.to_crypto_box(), + &node_keypair.private.to_crypto_box(), + ); cbox.decrypt_in_place(&server_info.nonce.into(), &[], &mut payload) .map_err(|e| frame::Error::DecryptionFailed(format!("err: {e}")))?; diff --git a/ts_devtools/src/bin/derp_ping.rs b/ts_devtools/src/bin/derp_ping.rs index 33ef77a9..4e4b99ec 100644 --- a/ts_devtools/src/bin/derp_ping.rs +++ b/ts_devtools/src/bin/derp_ping.rs @@ -40,11 +40,11 @@ async fn main() -> ts_cli_util::Result<()> { let peer = args .send_to_self - .then_some(config.key_state.node_key.public_key()) + .then_some(config.key_state.node_key.import().public) .or(args.peer); tracing::info!(?region_id, "starting derp transport"); - let derp = ts_derp::Client::connect(&derp_servers, &config.key_state.node_key.into()).await?; + let derp = ts_derp::Client::connect(&derp_servers, &config.key_state.node_key.import()).await?; let derp = Arc::new(derp); if let Some(peer) = peer { diff --git a/ts_disco_protocol/src/lib.rs b/ts_disco_protocol/src/lib.rs index 1bbbbc36..55f37350 100644 --- a/ts_disco_protocol/src/lib.rs +++ b/ts_disco_protocol/src/lib.rs @@ -40,7 +40,7 @@ mod test { net::{Ipv6Addr, SocketAddrV6}, }; - use ts_keys::{DiscoPrivateKey, NodePrivateKey}; + use ts_keys::{DiscoPrivateKey, DiscoPublicKey, NodePrivateKey}; use zerocopy::IntoBytes; use super::*; @@ -55,7 +55,7 @@ mod test { fn roundtrip_header() { let mut rng = rand::rng(); - let header = Header::new(rand_array(&mut rng).into(), rand_array(&mut rng)); + let header = Header::new(DiscoPublicKey::random(), rand_array(&mut rng)); header.validate().unwrap(); let mut out = alloc::vec::Vec::new(); diff --git a/ts_disco_protocol/src/packet.rs b/ts_disco_protocol/src/packet.rs index d22a6e53..68f3a6f5 100644 --- a/ts_disco_protocol/src/packet.rs +++ b/ts_disco_protocol/src/packet.rs @@ -191,7 +191,7 @@ impl Packet { receiver: &DiscoPublicKey, nonce: [u8; Header::NONCE_LEN], ) -> Result<&mut Packet<Encrypted>, Error> { - let bx = crypto_box::SalsaBox::new(&receiver.into(), &secret.into()); + let bx = crypto_box::SalsaBox::new(&receiver.to_crypto_box(), &secret.to_crypto_box()); self.header = Header::new(secret.public_key(), nonce); @@ -332,14 +332,17 @@ impl Packet<Encrypted> { &mut self, secret: &DiscoPrivateKey, ) -> Result<&mut Packet<Plaintext>, Error> { - crypto_box::SalsaBox::new(&self.header.sender_pub.into(), &secret.into()) - .decrypt_in_place_detached( - &self.header.nonce.into(), - &[], - &mut self.payload.payload, - Tag::from_slice(&self.payload.tag), - ) - .map_err(|_e| Error::CryptoFailed)?; + crypto_box::SalsaBox::new( + &self.header.sender_pub.to_crypto_box(), + &secret.to_crypto_box(), + ) + .decrypt_in_place_detached( + &self.header.nonce.into(), + &[], + &mut self.payload.payload, + Tag::from_slice(&self.payload.tag), + ) + .map_err(|_e| Error::CryptoFailed)?; let bs = self.as_mut_bytes(); let ret = Packet::mut_from_bytes(bs)?; diff --git a/ts_elixir/native/ts_elixir/src/config.rs b/ts_elixir/native/ts_elixir/src/config.rs index 6085cbda..4dc2625c 100644 --- a/ts_elixir/native/ts_elixir/src/config.rs +++ b/ts_elixir/native/ts_elixir/src/config.rs @@ -1,6 +1,7 @@ use std::collections::HashMap; use rustler::{Atom, NifResult, Term}; +use tailscale::keys::Export; mod atoms { rustler::atoms! { @@ -71,9 +72,9 @@ pub struct Keystate { impl From<tailscale::keys::PersistState> for Keystate { fn from(value: tailscale::keys::PersistState) -> Self { Self { - machine: value.machine_key.to_bytes().into(), - node: value.node_key.to_bytes().into(), - network_lock: value.network_lock_key.to_bytes().into(), + machine: value.machine_key.as_bytes().into(), + node: value.node_key.as_bytes().into(), + network_lock: value.network_lock_key.as_bytes().into(), } } } @@ -82,17 +83,14 @@ impl TryFrom<Keystate> for tailscale::keys::PersistState { type Error = (); fn try_from(value: Keystate) -> Result<Self, ()> { - fn key<T>(v: Vec<u8>) -> Result<T, ()> - where - T: From<[u8; 32]>, - { - Ok(<[u8; 32]>::try_from(v).map_err(|_| ())?.into()) - } + let machine_key = Export::from_bytes(value.machine.try_into().map_err(|_| ())?); + let node_key = Export::from_bytes(value.node.try_into().map_err(|_| ())?); + let network_lock_key = Export::from_bytes(value.network_lock.try_into().map_err(|_| ())?); Ok(Self { - machine_key: key(value.machine)?, - node_key: key(value.node)?, - network_lock_key: key(value.network_lock)?, + machine_key, + node_key, + network_lock_key, }) } } diff --git a/ts_ffi/src/keys.rs b/ts_ffi/src/keys.rs index 9c87a30e..1ff72ef2 100644 --- a/ts_ffi/src/keys.rs +++ b/ts_ffi/src/keys.rs @@ -3,6 +3,8 @@ use std::{ ffi::{CStr, c_char}, }; +use ts_keys::Export; + use crate::TOKIO_RUNTIME; /// A Tailscale cryptographic key. @@ -10,50 +12,6 @@ use crate::TOKIO_RUNTIME; #[repr(transparent)] pub struct key(pub [u8; 32]); -macro_rules! impl_to_from { - ($key:ty, $($keys:ty),+) => { - impl_to_from!($key); - impl_to_from!($($keys),+); - }; - - ($key:ty$(,)?) => { - impl From<key> for $key { - fn from(value: key) -> Self { - value.0.into() - } - } - - impl From<&key> for $key { - fn from(value: &key) -> Self { - value.0.into() - } - } - - impl From<$key> for key { - fn from(value: $key) -> Self { - key(value.into()) - } - } - - impl From<&$key> for key { - fn from(value: &$key) -> Self { - key(value.into()) - } - } - }; -} - -impl_to_from!( - ts_keys::NodePublicKey, - ts_keys::NodePrivateKey, - ts_keys::DiscoPublicKey, - ts_keys::DiscoPrivateKey, - ts_keys::MachinePrivateKey, - ts_keys::MachinePublicKey, - ts_keys::NetworkLockPrivateKey, - ts_keys::NetworkLockPublicKey -); - /// Tailscale key state for running a device. #[derive(Default)] #[repr(C)] @@ -75,9 +33,9 @@ impl From<persisted_key_state> for ts_keys::PersistState { impl From<&persisted_key_state> for ts_keys::PersistState { fn from(value: &persisted_key_state) -> Self { ts_keys::PersistState { - machine_key: (&value.machine_private_key).into(), - network_lock_key: (&value.network_lock_private_key).into(), - node_key: (&value.node_private_key).into(), + machine_key: Export::from_bytes(value.machine_private_key.0), + network_lock_key: Export::from_bytes(value.network_lock_private_key.0), + node_key: Export::from_bytes(value.node_private_key.0), } } } @@ -85,9 +43,9 @@ impl From<&persisted_key_state> for ts_keys::PersistState { impl From<ts_keys::PersistState> for persisted_key_state { fn from(value: ts_keys::PersistState) -> Self { Self { - machine_private_key: value.machine_key.into(), - network_lock_private_key: value.network_lock_key.into(), - node_private_key: value.node_key.into(), + machine_private_key: key(*value.machine_key.as_bytes()), + network_lock_private_key: key(*value.network_lock_key.as_bytes()), + node_private_key: key(*value.node_key.as_bytes()), } } } diff --git a/ts_python/Cargo.toml b/ts_python/Cargo.toml index 93bba6d6..4881e3ae 100644 --- a/ts_python/Cargo.toml +++ b/ts_python/Cargo.toml @@ -14,6 +14,7 @@ rust-version.workspace = true tailscale.workspace = true hex.workspace = true +zerocopy.workspace = true pyo3 = { version = "0.29", features = ["bytes", "abi3-py312", "experimental-inspect"] } pyo3-async-runtimes = { version = "0.29", features = ["tokio-runtime", "attributes"] } tracing-subscriber = { version = "0.3", features = ["env-filter"] } diff --git a/ts_python/src/key_state.rs b/ts_python/src/key_state.rs index d4d66110..9bfc78b7 100644 --- a/ts_python/src/key_state.rs +++ b/ts_python/src/key_state.rs @@ -1,3 +1,6 @@ +use ts::keys::Export; +use zerocopy::IntoBytes; + /// Tailscale keys. #[derive(Debug, Clone, PartialEq, Eq)] #[pyo3::pyclass(frozen, get_all, from_py_object, module = "tailscale")] @@ -41,9 +44,9 @@ impl Keystate { Ok(state) => { format!( "tailscale.Keystate(machine={}, node={}, network_lock={})", - hex::encode(state.machine_key.public_key().to_bytes()), - hex::encode(state.node_key.public_key().to_bytes()), - hex::encode(state.network_lock_key.public_key().to_bytes()), + hex::encode(state.machine_key.import().public.as_bytes()), + hex::encode(state.node_key.import().public.as_bytes()), + hex::encode(state.network_lock_key.import().public.as_bytes()), ) } Err(_) => "tailscale.Keystate(<invalid>)".to_owned(), @@ -54,9 +57,9 @@ impl Keystate { impl From<tailscale::keys::PersistState> for Keystate { fn from(value: tailscale::keys::PersistState) -> Self { Self { - machine: value.machine_key.to_bytes().into(), - node: value.node_key.to_bytes().into(), - network_lock: value.network_lock_key.to_bytes().into(), + machine: value.machine_key.as_bytes().into(), + node: value.node_key.as_bytes().into(), + network_lock: value.network_lock_key.as_bytes().into(), } } } @@ -65,17 +68,15 @@ impl TryFrom<&Keystate> for tailscale::keys::PersistState { type Error = (); fn try_from(value: &Keystate) -> Result<Self, ()> { - fn key<T>(v: &[u8]) -> Result<T, ()> - where - T: From<[u8; 32]>, - { - Ok(<[u8; 32]>::try_from(v).map_err(|_| ())?.into()) - } + let machine_key = Export::from_bytes(value.machine.clone().try_into().map_err(|_| ())?); + let node_key = Export::from_bytes(value.node.clone().try_into().map_err(|_| ())?); + let network_lock_key = + Export::from_bytes(value.network_lock.clone().try_into().map_err(|_| ())?); Ok(Self { - machine_key: key(&value.machine)?, - node_key: key(&value.node)?, - network_lock_key: key(&value.network_lock)?, + machine_key, + node_key, + network_lock_key, }) } } diff --git a/ts_python/src/node_info.rs b/ts_python/src/node_info.rs index 522fd332..88a929e3 100644 --- a/ts_python/src/node_info.rs +++ b/ts_python/src/node_info.rs @@ -1,5 +1,7 @@ use std::net::IpAddr; +use zerocopy::IntoBytes; + /// Info about a tailscale node. /// /// Turned into a `dict` on the Python side via `IntoPyObject`. @@ -35,7 +37,7 @@ impl From<&tailscale::NodeInfo> for NodeInfo { id: value.id, stable_id: value.stable_id.0.clone(), tags: value.tags.clone(), - node_key: value.node_key.to_bytes().to_vec(), + node_key: value.node_key.as_bytes().to_vec(), hostname: value.hostname.clone(), tailnet: value.tailnet.clone(), @@ -45,8 +47,8 @@ impl From<&tailscale::NodeInfo> for NodeInfo { ], derp_region: value.derp_region.map(|x| x.0.get()), - machine_key: value.machine_key.as_ref().map(|x| x.to_bytes().to_vec()), - disco_key: value.disco_key.as_ref().map(|x| x.to_bytes().to_vec()), + machine_key: value.machine_key.as_ref().map(|x| x.as_bytes().to_vec()), + disco_key: value.disco_key.as_ref().map(|x| x.as_bytes().to_vec()), underlay_addresses: value .underlay_addresses .iter() diff --git a/ts_runtime/src/peer_tracker/peer_db.rs b/ts_runtime/src/peer_tracker/peer_db.rs index c3b6d04c..71eafa6d 100644 --- a/ts_runtime/src/peer_tracker/peer_db.rs +++ b/ts_runtime/src/peer_tracker/peer_db.rs @@ -473,6 +473,7 @@ mod test { }; use ts_capabilityversion::CapabilityVersion; use ts_control::{NodeLastSeen, NodeStatus, TailnetAddress}; + use ts_keys::MachinePublicKey; use super::*; @@ -514,13 +515,9 @@ mod test { ipv4: rand_ipv4(&mut rng).into(), ipv6: rand_ipv6(&mut rng).into(), }, - node_key: rng.random::<[u8; 32]>().into(), - disco_key: rng - .random::<bool>() - .then_some(rng.random::<[u8; 32]>().into()), - machine_key: rng - .random::<bool>() - .then_some(rng.random::<[u8; 32]>().into()), + node_key: NodePublicKey::random(), + disco_key: rng.random::<bool>().then_some(DiscoPublicKey::random()), + machine_key: rng.random::<bool>().then_some(MachinePublicKey::random()), id: rng.random(), accepted_routes: (0..rng.random_range(0..32)) .map(|_| rand_route(&mut rng)) @@ -836,9 +833,9 @@ mod test { tailnet: has_tailnet.then_some(tailnet), status, - node_key: node_key.into(), - disco_key: disco_key.map(Into::into), - machine_key: machine_key.map(Into::into), + node_key: NodePublicKey::from_bytes(node_key), + disco_key: disco_key.map(DiscoPublicKey::from_bytes), + machine_key: machine_key.map(MachinePublicKey::from_bytes), tailnet_lock_key_signature: tailnet_lock_key_signature.map(Into::into), node_key_expiry: None, diff --git a/ts_transport/Cargo.toml b/ts_transport/Cargo.toml index 05dff514..e935df56 100644 --- a/ts_transport/Cargo.toml +++ b/ts_transport/Cargo.toml @@ -19,6 +19,7 @@ ts_keys.workspace = true smallbox = { version = "0.8", default-features = false } dyn-hash = "1.0" dyn-eq = "0.1" +zerocopy.workspace = true [lints] workspace = true diff --git a/ts_transport/src/endpoint.rs b/ts_transport/src/endpoint.rs index 847d0dfc..ce0be2dd 100644 --- a/ts_transport/src/endpoint.rs +++ b/ts_transport/src/endpoint.rs @@ -19,6 +19,7 @@ use core::{ use dyn_eq::DynEq; use dyn_hash::DynHash; +use zerocopy::IntoBytes; mod private { pub trait Sealed {} @@ -223,7 +224,7 @@ impl Debug for DerpEndpoint { write!( f, "derp:{:02x}", - ts_hexdump::IterFmt::contiguous(&self.0.to_bytes()) + ts_hexdump::IterFmt::contiguous(self.0.as_bytes()) ) } } diff --git a/ts_tunnel/examples/handshake.rs b/ts_tunnel/examples/handshake.rs index 28fc2edf..96fb86c3 100644 --- a/ts_tunnel/examples/handshake.rs +++ b/ts_tunnel/examples/handshake.rs @@ -13,11 +13,11 @@ use tokio::{ select, time::{interval_at, sleep_until}, }; -use ts_keys::{NodePrivateKey, NodePublicKey}; +use ts_keys::{Export, NodeKeyPair, NodePublicKey}; use ts_packet::PacketMut; use ts_time::TimeRange; use ts_tunnel::Endpoint; -use zerocopy::{FromBytes, IntoBytes, TryFromBytes}; +use zerocopy::{IntoBytes, TryFromBytes}; // Minimal example config: // // [Interface] @@ -42,18 +42,18 @@ pub struct Args { /// Public key of the endpoint. Can be hex or base64. #[clap(long, value_parser = parse_key)] - pub peer_key: chacha20poly1305::Key, + pub peer_key: Export<NodeKeyPair>, /// Our private key. Can be hex or base64. #[clap(long, value_parser = parse_key)] - pub private_key: chacha20poly1305::Key, + pub private_key: NodeKeyPair, } type BoxResult<T> = Result<T, Box<dyn core::error::Error + Send + Sync>>; -/// Parse a [`chacha20poly1305::Key`] from a string, trying hex (Tailscale-typical) and +/// Parse a [`Export<NodeKeyPair>`] from a string, trying hex (Tailscale-typical) and /// base64 (WireGuard-typical) formats in succession. -pub fn parse_key(s: &str) -> BoxResult<chacha20poly1305::Key> { +pub fn parse_key(s: &str) -> BoxResult<NodeKeyPair> { let s = s.trim().as_bytes(); let key_bytes = @@ -63,7 +63,7 @@ pub fn parse_key(s: &str) -> BoxResult<chacha20poly1305::Key> { .try_into() .map_err(|_v: Vec<u8>| "invalid key len")?; - Ok(key_bytes.into()) + Ok(Export::from_bytes(key_bytes).import()) } #[tokio::main] @@ -72,17 +72,15 @@ async fn main() -> BoxResult<()> { let args = Args::parse(); - let privkey = NodePrivateKey::read_from_bytes(args.private_key.as_bytes()) - .map_err(|_| "failed reading private key")?; eprintln!( "my pubkey: {}", - base64::engine::general_purpose::STANDARD.encode(privkey.public_key().as_bytes()) + base64::engine::general_purpose::STANDARD.encode(args.private_key.public.as_bytes()) ); let peer_key = *NodePublicKey::try_ref_from_bytes(args.peer_key.as_bytes()) .map_err(|_| "failed reading public key")?; - let mut ep = Endpoint::new(privkey.into()); + let mut ep = Endpoint::new(args.private_key); let peer_id = ts_tunnel::PeerId(1); diff --git a/ts_tunnel/src/endpoint.rs b/ts_tunnel/src/endpoint.rs index 0a8ffa3f..d32241d1 100644 --- a/ts_tunnel/src/endpoint.rs +++ b/ts_tunnel/src/endpoint.rs @@ -686,8 +686,8 @@ mod tests { impl EndpointPair { pub fn new() -> Self { - let key_a = NodeKeyPair::new(); - let key_b = NodeKeyPair::new(); + let key_a = NodeKeyPair::random(); + let key_b = NodeKeyPair::random(); let mut a = Endpoint::new(key_a.clone()); let mut b = Endpoint::new(key_b.clone()); let psk = rand::random(); diff --git a/ts_tunnel/src/handshake.rs b/ts_tunnel/src/handshake.rs index 0dfc4870..52ae7894 100644 --- a/ts_tunnel/src/handshake.rs +++ b/ts_tunnel/src/handshake.rs @@ -48,8 +48,11 @@ impl ReceivedHandshake { return None; }; - let (noise, timestamp) = - ikpsk2::ReceivedHandshake::new::<TAI64N>(&mut init.noise, PROLOGUE, my_static.into())?; + let (noise, timestamp) = ikpsk2::ReceivedHandshake::new::<TAI64N>( + &mut init.noise, + PROLOGUE, + my_static.to_x25519_dalek(), + )?; Some(Self { responder_to_initiator_id: init.sender_id, @@ -60,7 +63,7 @@ impl ReceivedHandshake { /// Return the peer's static public key. pub fn peer_static(&self) -> NodePublicKey { - self.noise.peer_static_pub.to_bytes().into() + NodePublicKey::from_bytes(self.noise.peer_static_pub.to_bytes()) } } @@ -240,8 +243,8 @@ impl Handshake { }; let noise = ikpsk2::SentHandshake::new( - (&endpoint.my_key).into(), - peer.key.into(), + endpoint.my_key.to_x25519_dalek(), + peer.key.to_x25519_dalek(), PROLOGUE, endpoint.timestamps.now(), pkt.noise.as_mut_bytes(), @@ -284,7 +287,7 @@ impl Handshake { let session_keys = match sent_handshake.noise.try_finish( &mut packet.noise, - (&endpoint.my_key).into(), + endpoint.my_key.to_x25519_dalek(), &peer.psk, ) { Ok(session_keys) => session_keys, @@ -424,7 +427,7 @@ mod tests { #[test] fn test_handshake() { - let (a_static, b_static) = (NodeKeyPair::new(), NodeKeyPair::new()); + let (a_static, b_static) = (NodeKeyPair::random(), NodeKeyPair::random()); let psk = rand::random(); // Peer A sends a handshake initiation... @@ -470,7 +473,7 @@ mod tests { // Regression test for https://github.com/tailscale/tailscale-rs/issues/334 #[test] fn test_invalid_response_ignored() { - let (a_static, b_static) = (NodeKeyPair::new(), NodeKeyPair::new()); + let (a_static, b_static) = (NodeKeyPair::random(), NodeKeyPair::random()); let psk = rand::random(); // A sends a handshake diff --git a/ts_tunnel/src/macs.rs b/ts_tunnel/src/macs.rs index ccf60ff0..e55bb7b4 100644 --- a/ts_tunnel/src/macs.rs +++ b/ts_tunnel/src/macs.rs @@ -34,13 +34,13 @@ struct Mac2Trailer { fn mac1_key(key: &NodePublicKey) -> [u8; 32] { let mut h = Blake2s256::new_with_prefix(MAC1_LABEL); - h.update(key.to_bytes()); + h.update(key.as_bytes()); h.finalize().into() } fn mac2_key(key: &NodePublicKey) -> [u8; 32] { let mut h = Blake2s256::new_with_prefix(MAC2_LABEL); - h.update(key.to_bytes()); + h.update(key.as_bytes()); h.finalize().into() } diff --git a/ts_tunnel/src/session.rs b/ts_tunnel/src/session.rs index e7aeeb81..28745cf9 100644 --- a/ts_tunnel/src/session.rs +++ b/ts_tunnel/src/session.rs @@ -517,7 +517,7 @@ mod tests { let responder_session = ids.allocate_session(responder_cfg.id); let responder_session_id = responder_session.id(); let now = Instant::now(); - let mut endpoint = EndpointState::from(NodeKeyPair::new()); + let mut endpoint = EndpointState::from(NodeKeyPair::random()); // NOTE: this would be catastrophically insecure in non-test code, because it reuses the // same key in both directions, which leads to catastrophic nonce reuse. It's okay here // because (a) it's a test and (b) we only ever transmit in one direction. @@ -571,7 +571,7 @@ mod tests { fn test_session_timers() { let k: [u8; 32] = rand::random(); let mut ids = IdMap::default(); - let mut endpoint = EndpointState::from(NodeKeyPair::new()); + let mut endpoint = EndpointState::from(NodeKeyPair::random()); let recv_cfg = PeerConfig::new(PeerId(1), NodePublicKey::default(), Psk::default()); let recv_session = ids.allocate_session(recv_cfg.id); let recv_session_id = recv_session.id();