Skip to content

ts_keys: require more ceremony to serialize private keys - #316

Merged
danderson merged 2 commits into
mainfrom
push-uokuozswsznx
Sep 10, 2026
Merged

danderson merged 2 commits into
mainfrom
push-uokuozswsznx

Conversation

@danderson

@danderson danderson commented Jul 29, 2026

Copy link
Copy Markdown
Member

I've split the change into two commits for ease of review, but the build is broken in between the commits so they'll be squashed before merging:

The first commit is just the changes to ts_keys and its API. Notable API changes:

  • Private key types can no longer be constructed from raw bytes, converted to bytes, or serialized in other formats (e.g. hex encoded). Callers must .export() to create an Export<T> that allows those operations. Parsing private keys from hex strings and deserialization through serde are still allowed without going through the Export wrapper, since they convert from typed representations.
  • Private and pair types are Clone, but not Copy. This is to gently nudge towards passing keys by reference rather than by value. I want to do this to the pubkey types as well, but that change has wide-ranging API consequences and so I decided to keep it separate (and ts_keys: remove Copy from PublicKey #315 tracks it for later).
  • Conversion to crypto_box and x25519_dalek types uses explicit to_{crypto_box,x25519_dalek} methods, rather than From impls. This is partly because removing Copy made the into conversions awkward at call sites, but also because the conversion erases the key's tailscale type, which increases the risk of mistakenly using the wrong key type. So, I wanted those conversions to be a bit more explicit than a .into().
  • Private and pair types no longer implement Default (which required generating a random key, which is a weird default). The pubkey type still implements default, because ts_control_serde has an unpleasantly strong assumption that all maprequest/response fields implement Default, and I didn't want to untangle it right now.

The second commit is the mechanical changes required to get the codebase to build again. The most notable change is that I removed support for the "old" state file format (which incorrectly persisted the disco key to disk), rather than bend over backwards to keep it working. I believe we stopped using that format either right before v0.1 or right after, either way the number of un-migrated state files in the wild should be zero.

The main thing I don't love about this change is that public keys still feel too easy to convert into raw untyped forms and back. That's a little less scary than with private keys, but could still make it easier than ideal to end up with key type confusion issues. However, making the pubkey types support zerocopy makes downstream crates nicer because you can just stick a pubkey type into a wire packet struct and never have to touch the raw untyped bytes yourself. So, I decided that making that possible (which necessarily exposes methods for conversion to/from raw bytes) was worth the mild peril.

@danderson
danderson force-pushed the push-uokuozswsznx branch from ade80f7 to 130184e Compare July 29, 2026 21:20
@danderson danderson changed the title WIP: other key refactor attempt ts_keys: require more ceremony to serialize private keys Sep 8, 2026
@danderson
danderson marked this pull request as ready for review September 9, 2026 00:03

@npry npry left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of .into() on the foreign types feels surprising, but makes sense -- not sure there's a better answer.

Otherwise, lgtm!

Comment thread ts_keys/src/keystate.rs Outdated
Comment thread ts_keys/src/util.rs Outdated
Fixes #305

Signed-off-by: David Anderson <danderson@tailscale.com>
Change-Id: I5bf5b07370c2bf3e8d09eb4f5d3bdcbe6a6a6964
Fixes #305

Signed-off-by: David Anderson <danderson@tailscale.com>
Change-Id: I5bf5b07370c2bf3e8d09eb4f5d3bdcbe6a6a6964
@danderson
danderson enabled auto-merge (rebase) September 10, 2026 22:35
@danderson
danderson merged commit 4ca02ae into main Sep 10, 2026
23 checks passed
@danderson
danderson deleted the push-uokuozswsznx branch September 10, 2026 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants