ts_noise: factor out Noise implementation

[danderson]

To enable both ts_tunnel and ts_control_noise to use the same underlying crypto primitives when possible.

Fixes #137

[danderson]

I think this is ready. There's probably some cleanups possible in how ts_tunnel and ts_control_noise use the common noise impls, but this was the minimally-ish invasive change needed to make the refactor happen. All tests pass of course, and also verified that examples/tcp_echo still works from a tailscale-go peer, which gives a proof that both the control protocol and wireguard work sufficiently to enable communication.

[npry]

looks good assuming the copy-paste typo on codec send/receive is resolved, everything else is non-blocking

[npry]

Nonblocking/discussion: you could eliminate the internal length checking and panicking by explicitly making the length assertion the caller's responsibility (here and in the other handshake types):

Suggested change

	/// Finalize the handshake and generate a response.
	///
	/// The response is written to `packet`, which must be exactly
	/// [`ReceivedHandshake::RESP_SIZE`] bytes.
	///
	/// # Panics
	///
	/// If `packet` is the wrong size.
	#[inline]
	pub fn finish(self, packet: &mut [u8]) -> Session {
	/// Finalize the handshake and generate a response
	#[inline]
	pub fn finish(self, packet: &mut [u8; Self::RESP_SIZE]) -> Session {

It makes it more annoying to call, but also makes the panic condition more visible to the caller (which notionally it has to check anyway), so it feels arguably worthwhile here

[danderson]

Hmm. So the issue I have with this is that if the caller has a &[u8], converting that to a fixed size array is (afaik) a headache unless you make a copy, or use something like zerocopy to safely transmute the slice into something more structured.

In ts_tunnel we use zerocopy extensively, so there it would probably be fine, but I'm not sure how that fits with ts_control_noise.

In principle, I'd like to do this, but I'm going to defer to a followup PR just because it's one of those things where plumbing the change through the code may spiral out of control and unravel, so I want the damage contained 😂

[npry]

Nonblocking on this PR, but would be nice in the future to have proptests comparing against a third-party implementation

[danderson]

I considered doing this, but the third-party implementations have awkward APIs (due to trying to handle all handshake patterns in a single API), which made this quite painful. So I settled on using a 3p library to generate test vectors and burned them in here.

I'll have another go at an interop test in a followup.

[nrc]

One nit, one question (non-blocking) inline. Also there's quite a few [u8; 32]s in various places, I wonder if it is worth wrapping these in a struct to distinguish between keys and hashes, and maybe different kinds of keys?

[nrc]

Could this split be handled by having Message have it's payloads by value rather than reference and then using &Message or &mut Message rather than needing two enums?

[npry]

I thought this too, but I tried it and it's a layout issue — the enum discriminator needs to physically live somewhere, so you can't cast ref-to-variant into enum or vice versa

[danderson]

The payloads are zerocopy transmutations of a byte slice, so they can't own the values without copying, and that's what I'm trying to avoid. It's possible Message could take ownership of the underlying bytes and then have accessors for the transmuted view, but that's a larger refactor than I want to take on in this change.

I do want to try and get rid of this Message/MessageMut split though, it's unpleasant and strongly suggests the code structure isn't quite right.

[dylan-tailscale]

just did a skim, but lgtm