ts_keys: require more ceremony to serialize private keys

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.

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::<KeyFile>(
-        &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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        KeyFileNew {
-            key_state: self.key_state(),
-        }
-        .serialize(serializer)
-    }
+    let key_file = load_or_init::<KeyFile>(&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<KeyState>(
     path: impl AsRef<Path>,
     default: impl FnOnce() -> KeyState,
-    migrate: impl FnOnce(&KeyState) -> Option<KeyState>,
     bad_format_behavior: BadFormatBehavior,
 ) -> Result<KeyState, crate::Error>
 where
@@ -227,21 +170,7 @@ where
 
     match tokio::fs::read(path).await {
         Ok(contents) => match serde_json::from_slice::<KeyState>(&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");

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,
     };

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?;
 

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;

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");

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");

ts_derp/src/client.rs

@@ -236,7 +236,10 @@ fn make_clientinfo(
     node_keypair: &NodeKeyPair,
     server_key: &ts_keys::DerpServerPublicKey,
 ) -> Result<(ClientInfo, Vec<u8>), 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<frame::ServerInfoPayload, Error> {
     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}")))?;
 

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 {

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();

ts_disco_protocol/src/packet.rs

@@ -191,7 +191,7 @@ impl Packet<Plaintext> {
         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)?;

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,
         })
     }
 }