dataplane: split out derp and stun traffic - #237
Conversation
09f9b3f to
68737f4
Compare
68737f4 to
595ca80
Compare
danderson
left a comment
danderson
left a comment
There was a problem hiding this comment.
Couple minor thoughts, but looks reasonable to me. The diff weirdness is worrying me a little, but it might just be github having a moment.
| pub type DataplaneFromUnderlay = mpsc::UnboundedReceiver<(PeerId, Vec<PacketMut>)>; | ||
|
|
||
| // TODO: wire in overlay/underlay transport traits | ||
| // NOTE(npry): this used to have unique types for each queue, but the names got confusing due to |
There was a problem hiding this comment.
Hmm, I'll have to see how this feels in code. The naming problem I'm trying to avoid coming from the Go codebase is that we mixed up our directions a ton based on what made local sense, but that made it hard to globally understand the flow of traffic.
I agree with your problem statement that it's very confusing to have to wrangle both general dataflow and also which end of a channel you're holding. I was going to write here that I find something like Rx<FromOverlay> confusing to understand the direction of flow, but as I was writing it, I think the interpretation is unambiguous and conveys both pieces of information we want. Nice!
dylan-tailscale
left a comment
dylan-tailscale
left a comment
There was a problem hiding this comment.
Overall lgtm, two very minor fixes needed.
The new names for the transport "queues" are way better imo, but it still took me a minute to validate that, e.g., a Tx<ToUnderlay> was what I thought it was. I think we can bridge the gap in the ARCHITECTURE.md with just a description/diagram of what's going on between the dataplane/overlay transports/underlay transports and the various combinations of Tx<T>/Rx<T> and {To, From}{Overlay, Underlay} (obv future work)...wdyt?
9215de7 to
d83b705
Compare
a749ba2 to
ba6f86d
Compare
ba6f86d to
d6406d6
Compare
9045394 to
e3a2244
Compare
Currently, receipt of a disco or stun packet causes the dataplane to log errors because it doesn't understand the traffic and tries to treat it as WireGuard. This provides comparable logic to `wgengine/magicsock.go`'s packet identification. It is plumbed into the dataplane in a future commit. Signed-off-by: Nathan Perry <nathan@tailscale.com> Change-Id: I87e84518ed0adbe4ba35358cab0a3dc66a6a6964
The peer id isn't actually known a priori from the underlay, even if some transports can give an indication of what they think it is (such as derp reporting the nodekey). Ultimately the only way the peer identity can be trusted is if it's cryptographically authenticated by data within the packet, which WireGuard derives from the ongoing session (identified by the session id field), and disco gets from the disco pubkey field in its header. STUN packets are unverified but don't require any association to the peer's id. Removing this requirement will simplify the architecture for the UDP direct transport in future commits. Signed-off-by: Nathan Perry <nathan@tailscale.com> Change-Id: Ib3fe81d7f41fbd6e3b637f282b51f4f96a6a6964
Signed-off-by: Nathan Perry <nathan@tailscale.com> Change-Id: Ibf7d20ff1a3ab10e1277e78179285eb46a6a6964
e3a2244 to
0d4013c
Compare
3 participants
stacked on #214
Port packet identification logic from Go (wireguard vs disco vs stun), begin partitioning incoming packet stream by type. This isn't used in
runtimeyet — that integration depends on a bunch of other changes I made here so I'm holding off on PRing that until this round of changes merges.This also eliminates the requirement for underlay transports to supply a peer id, as this is in general unnecessary since packets must cryptographically authenticate themselves anyway, and it will simplify implementation for the UDP transport. Peer ids are still required to route to a peer.