Noise protocol its not concurrent
Two goroutines, one cipher state, and a non concurrent safe library.
If you read the 2 day debug session post, you know how that one ended. The relay was an amd64 binary running under qemu on my Mac, the crypto came out wrong sometimes, I fixed the Dockerfile and wrote this commit message:
bugfix: relay ran emulated → flynn/noise crypto intermittently produced wrong AEAD output
I never had so much pain from a stupid error like this
A while after that, I found the relay was corrupting its own encrypted stream, which could've been the actual real cause of the wrong AEAD output that I blamed on emulation.
# Two goroutines, one cipher state
After the Noise handshake, the relay keeps a session open to each device, and two things write to it. A goroutine pushes new messages down (Deliver frames), and the read loop answers the client's heartbeats. Both encrypt with the same outbound cipher state:
// deliver goroutine
encrypted, err := cs2.Encrypt(nil, nil, frame)
// ...
writeNoiseMsg(conn, encrypted)
// read loop, on a Heartbeat
hb, err := cs2.Encrypt(nil, nil, Frame(MsgTypeHeartbeat, nil))
// ...
writeNoiseMsg(conn, hb)
Noise transport messages use a counter as the nonce. Every Encrypt bumps it, and the other side expects frames to arrive in exactly that order. flynn/noise's CipherState isn't safe for concurrent use (did not know that), so with two goroutines calling it at once you can encrypt frame n and n+1 and put n+1 on the wire first. Then the client can't decrypt and the session is dead.
And as a bonus, writeNoiseMsg wrote the 2-byte length and the body as two separate Write calls. Which means two writers could also interleave frames on the wire. It's the same two-Write framing I stared at in theory 1 of the Docker post (I also wrote a whole post about framing before that).
I don't think the Docker post was wrong. That failure was on msg1 of the handshake, and this cipher state doesn't even exist until the handshake is done, so the qemu thing was real. But this race was sitting there since the beginning of the receive session
# One writer
The fix is as simple as a sessionWriter now being the only thing that owns the outbound cipher state and the write side of the connection. Everything else puts frames on its queue. Nonce order is call order, and length plus body go out in one Write.
The queue holds 16 frames, so it needs a policy for when it's full. Deliver frames block (anything not written never gets acked, so it stays in the inbox). Heartbeat echoes get dropped, because the read loop also handles acks and shouldn't stall behind a slow connection.
That dropped echo leaks into the client. If the relay can drop echoes under load, the client can't use "I got my echo back" to decide the connection is alive. So any inbound frame now counts as liveness, and the client severs the session if nothing arrives in 75 seconds.
This is far from perfect, but at least a way to move forward.
# The Rust side had a cousin
The Rust client sits on my own noise crate, and it had the same class of bug. Two threads sending on one session could encrypt with nonce n and n+1 and write n+1 first. Two threads receiving could each grab part of the same frame. Both had failing tests, for XX and NK. Now send holds a lock from encrypt through write, receive holds one from the first header byte through decrypt.