ts_keys: rework to use generic types with marker traits by danderson · Pull Request #306 · tailscale/tailscale-rs

danderson · 2026-07-25T00:39:26Z

Change-Id: If90c2f9fc4061dfd9cfe0a9e2c97e2596a6a6964

Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: If90c2f9fc4061dfd9cfe0a9e2c97e2596a6a6964

danderson · 2026-07-29T00:03:17Z

Unpleasant change size, but it's almost entirely bubbling out the type change to the entire codebase. The meat is in the ts_keys files.

nrc

Looks good! A bunch of mostly minor things inline. I think the only blocking one is some module-level docs to explain Public/Private/Export/Pair and the traits.

nrc · 2026-07-29T01:30:23Z

@@ -1,18 +1,27 @@
 #![doc = include_str!("../README.md")]
 #![no_std]


I think this crate would benefit from module-level docs explaining how and why everything works.

nrc · 2026-07-29T01:52:08Z

-pub(crate) use create_x25519_private_key_type;
-pub(crate) use create_x25519_public_key_type;
+pub(crate) use x25519_pair;
+pub(crate) use x25519_public;


Given how small this file is, I'd personally just inline it into lib.rs

I might even just write out the impls rather than use macros at all. Not sure.

Reasonable. Inlined the macro for now, I'm not sure how I feel about writing out the repetitive boilerplate by hand but I'll defer that until the followup where I try to figure out how to make the key storage also generic.

nrc · 2026-07-29T01:54:08Z

-        pub struct $key_name(
-            [u8; $key_name::KEY_LEN_BYTES]
-        );
+        #[derive(Debug, Copy, Clone, Eq, PartialEq, Default, Hash, PartialOrd, Ord)]


Do we need all these derives? Since the type is only used as a phantom type, I'd assume it doesn't need any of these?

It's necessary because this marker appears as a type parameter on structs, and the std derive macros add type parameter trait bounds on their impls. So if we want e.g. PublicKey to implement all these traits without having to impl each one by hand to avoid the bound, the markers also need these derives.

It would be possible to just type out the obvious implementation of each trait instead to avoid the bonus bound, but it seemed harmless enough to derive on these marker traits and let the default derivations happen.

nrc · 2026-07-29T02:06:07Z

+
+/// The public half of an asymmetric keypair.
+#[derive(
+    Copy,


32 bytes seems pretty big to be Copy, is it very convenient to do so?

It's quite convenient, but possibly not sufficiently so. In my past work to remove Copy from the private types the fallout in amount of fixup elsewhere in the code was large, so I'm going to defer to a followup to not make this change any bigger. Filed #315 so that I don't forget.

npry

I have some hesitation here: the way the marker types are embedded into the key types feels odd. Maybe i just haven't sat with it enough, but my instinct would be to use a trait to unify the X25519{Public,Private}Key behavior, rather than having them be essentially marker traits in what feels to me like a strange position (gating the behavior of a struct embedding the marker as PhantomData).

As I read it, the functional purpose of having Public<K> is to centralize the implementation of common behavior, but (correct me if wrong) I don't think we have a usecase in mind for abstracting over K outside of this crate. Essentially, we want people to treat Public<DiscoKey> as a full unique type and see the parametricity as an implementation detail. So it's kind of immaterial in that sense if the type name is Public<DiscoKey> or DiscoPublicKey, it's just that the former lets us centralize the implementation and avoid the verbose macros.

If that's all true, (as a point of comparison/for discussion) my intuition would be to keep the old naming scheme and use traits to provide the common functionality -- something like:

mod internal {
    pub trait KeyInfo {
        const KEY_PREFIX: &'static str;
        const KEY_HEX_STR_LEN: usize;

        fn get_key(&self) -> &[u8; 32];
        fn from_key(k: [u8; 32]) -> Self;
    }

    // could be split like you have now or use a single keyinfo with marker traits like this
    pub trait Public {}
    pub trait Private {}
}

pub trait PublicKey: internal::KeyInfo + internal::Public {
    fn random() -> Self { Self::from_key(...) }
    fn as_bytes() -> &[u8; 32] { self.get_key() }
    // ...
}

impl<T> PublicKey for T where T: internal::KeyInfo + internal::Public {}

And then do small macros just to declare the key types and minimal internal trait (which will give you the full functionality through the blanket impl):

macro_rules! x25519_public_key {
    ($name:ident, $prefix:literal) => {
        pub struct $name([u8; 32]);

        impl internal::KeyInfo for $name {
            const KEY_PREFIX: &'static str = $prefix;

            fn get_key(&self) -> &[u8; 32] { &self.0 }
            fn from_key(k: [u8; 32]) -> Self { Self(k) }
        }
        
        impl internal::Public for KeyInfo {}
    }
}

And have Export wrap the actual key type:

pub struct Export<T>(T);

impl<T> Serialize for Export<T> where T: KeyInfo {
     // ...
}

impl<T> Deserialize for Export<T> where T: KeyInfo {
     // ...
}

This approach has downsides, I think probably you wouldn't be able to provide From, AsRef, FromStr instances as blankets (e.g. impl<T> From<[u8; 32]> for T where T: KeyInfo), which would mean more macros, which at least somewhat defeats the original point.

I don't mean this as "no I don't like it, do it this other way" — I'd be good with landing as-is — but it does feel a bit odd/unidiomatic to me atm, and this^ shape as a point of comparison feels less so; wanted to discuss

npry · 2026-07-29T15:24:15Z

-/// A Tailscale cryptographic key.
-#[derive(Debug, Default)]
-#[repr(transparent)]
-pub struct key(pub [u8; 32]);


This was part of the cbindgenned header — it generated a typedef which I think was useful for communicating intent into C:

/** * A Tailscale cryptographic key. */ typedef uint8_t ts_key[32];

I'm not opposed to changing it (e.g. making it more specific about what kind of key it is) or removing it if you do feel it's superfluous, but I have the feeling this API change may not have been super intentional/just part of the overall refactor -- in that case, I would push back against removal

npry · 2026-07-29T15:34:08Z

+    pub peer_key: [u8; 32],

    /// Our private key. Can be hex or base64.
    #[clap(long, value_parser = parse_key)]
-    pub private_key: chacha20poly1305::Key,
+    pub private_key: [u8; 32],


Why not have these as ts_keys::Export?

npry · 2026-07-29T15:41:20Z

+#[cfg(feature = "serde")]
+struct KeyVisitor(&'static str);
+
+#[cfg(feature = "serde")]
+impl<'de> Visitor<'de> for KeyVisitor {
+    type Value = [u8; 32];
+
+    fn expecting(&self, formatter: &mut Formatter) -> fmt::Result {
+        write!(
+            formatter,
+            "a string with the prefix '{}:' followed by {} hex characters",
+            self.0, X25519_LEN_HEX_STR
+        )
+    }
+
+    fn visit_str<E>(self, v: &str) -> Result<Self::Value, E>
+    where
+        E: de::Error,
+    {
+        parse_hex(v, self.0).map_err(|e| ::serde::de::Error::custom(e))
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de, T: X25519Public> ::serde::Deserialize<'de> for Public<T> {
+    fn deserialize<D: ::serde::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
+        d.deserialize_str(KeyVisitor(T::PUBLIC_KEY_PREFIX))
+            .map(|key| Public {
+                key,
+                _marker: PhantomData,
+            })
+    }
+}


why the custom visitor type? for the expecting? if not, you could get away with:

let s = <&str>::deserialize(d)?; parse_hex(s, T::PUBLIC_KEY_PREFIX).map_err(serde::de::Error::custom)

npry · 2026-07-29T15:44:25Z

+    where
+        S: ::serde::Serializer,
+    {
+        serializer.serialize_str(self.to_string().as_ref())


nit: you could get away without allocating here, e.g. by hex::encode_to_slice (or write! manually) into a [u8; T::PUBLIC_KEY_HEX_STR_LEN] and then calling serialize_str on that. nbd/i count as a nit because serde is probably allocating anyway

npry · 2026-07-29T15:55:04Z

+/// A public key that can perform X25519 operations.
+pub trait X25519Public {
+    /// The descriptive prefix of the key when serialized to a string.
+    const PUBLIC_KEY_PREFIX: &'static str;
+
+    /// The length of the key in its serialized form.
+    const PUBLIC_KEY_HEX_STR_LEN: usize = Self::PUBLIC_KEY_PREFIX.len() + 1 + X25519_LEN_HEX_STR;
+}
+
+/// A private key that can perform X25519 operations.
+pub trait X25519Private: X25519Public {
+    /// The descriptive prefix of the key when serialized to a string.
+    const PRIVATE_KEY_PREFIX: &'static str;
+
+    /// The length of the key in its serialized form.
+    const PRIVATE_KEY_HEX_STR_LEN: usize = Self::PRIVATE_KEY_PREFIX.len() + 1 + X25519_LEN_HEX_STR;
+}


Hmm, these are a bit confusing naming-wise. I would expect these to specify behavior for a key, i.e. a type that implements X25519Public is a public key, but that's not what they do, they're meant to carry information for the marker and don't specify any behavior. I.e. Public<K> doesn't implement X25519Public. It's a strange inverted relationship, they actually mean very little about the type they're implemented on and exist to provide information to an enclosing struct. The type parameter on all the wrapper types means nothing about the function of the key and is only there for naming and to provide a prefix. It feels kind of unidiomatic, but I follow the design and I'm not sure what else to do about it, other than maybe to suffix Info or Marker or something? Fine if there isn't an answer, but wanted to flag

Separately, personal preference: I wouldn't include the supertrait bound on Private, since I don't think it serves a purpose (you can write X25519Private + X25519Public at usage sites where you need both)

Separately, personal preference: I wouldn't include the supertrait bound on Private, since I don't think it serves a purpose (you can write X25519Private + X25519Public at usage sites where you need both)

It made sense to me, but now that I try and write out the reasoning, I wonder if there is a use case for having the private key metadata without the public key metadata? That seems reasonable to me, but I don't really know (my mental model for these traits is that they represent the metadata that describes a class of keys)

npry · 2026-07-29T16:00:47Z

+    {
+        let mut s = String::with_capacity(T::PRIVATE_KEY_HEX_STR_LEN);
+        write_hex(self.key, T::PRIVATE_KEY_PREFIX, &mut s).unwrap();
+        ser.serialize_str(s.as_ref())
+    }


nit: why different from the other Serialize impl?

npry · 2026-07-29T16:08:53Z

+    pub fn as_bytes(&self) -> [u8; 32] {
+        self.key
+    }


as_ conventionally implies a ref (where to_ is converting to an owned type without consuming self, and consuming self is into_) -- I would have this return &[u8; 32] or use to_. Ditto on the other methods named this way

Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: If44a597538744bd05557c08675e0feb86a6a6964

danderson · 2026-07-29T18:08:17Z

Fair points on the generic types. There is one place in the codebase so far where the generic nature helps (in ts_noise, which wants to do crypto with any kind of x25519 key). Elsewhere, I think the main thing it does is convey that these types all share a common API shape and set of behaviors, which isn't as obvious when they're looseleaf types with shared implementation bits. But maybe that's not worthwhile enough to convey?

The other aspect as you say is that, absent this structure, a bunch of the impls we want won't be implementable again due to orphan rules, and so we'll be back to the giant macros I was hoping to get away from. But perhaps that's not worth optimizing for. Development that has to touch the macro body sucks because the error reporting for them sucks, but maybe I'm having recency bias from having to touch them repeatedly recently.

Hmmmm. I'll hold on merging this and prototype the shape you suggest, see how it feels and how that feels.

danderson requested review from dylan-tailscale, npry and nrc as code owners July 25, 2026 00:39

An error occurred while trying to automatically change base from push-ulpvuupqmtwq to push-rpnxoxqnpsrp July 28, 2026 16:51

danderson force-pushed the push-kqznxkqknvzt branch from 7832c16 to 35381cc Compare July 28, 2026 23:28

ts_keys: rework to use generic types with marker traits

e4ef90f

Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: If90c2f9fc4061dfd9cfe0a9e2c97e2596a6a6964

danderson force-pushed the push-kqznxkqknvzt branch from 35381cc to e4ef90f Compare July 28, 2026 23:51

danderson changed the base branch from push-ulpvuupqmtwq to main July 29, 2026 00:02

danderson changed the title ~~WIP: rework ts_keys to use generic types with marker traits~~ ts_keys: rework to use generic types with marker traits Jul 29, 2026

nrc reviewed Jul 29, 2026

View reviewed changes

danderson mentioned this pull request Jul 29, 2026

ts_keys: remove Copy from PublicKey #315

Open

npry reviewed Jul 29, 2026

View reviewed changes

FIXUP: review feedback

c3768ff

Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: If44a597538744bd05557c08675e0feb86a6a6964

danderson mentioned this pull request Jul 29, 2026

WIP: other key refactor attempt #316

Draft

		@@ -1,18 +1,27 @@
		#![doc = include_str!("../README.md")]
		#![no_std]

Uh oh!

Conversation

danderson commented Jul 25, 2026

Uh oh!

danderson commented Jul 29, 2026

Uh oh!

nrc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npry left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

npry Jul 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danderson commented Jul 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

npry left a comment •

edited

Loading

npry Jul 29, 2026 •

edited

Loading