ts_tunnel: fix the triggers for session rotation#286
Open
danderson wants to merge 1 commit into
Open
Conversation
npry
reviewed
Jul 17, 2026
npry
left a comment
npry
left a comment
Collaborator
There was a problem hiding this comment.
only blocking question is the polarity on codec, otherwise lgtm
Comment on lines
+60
to
+61
| tx: Codec::<Tx>::from(session.initiator_to_responder), | ||
| rx: Codec::<Rx>::from(session.responder_to_initiator), |
Collaborator
There was a problem hiding this comment.
Shouldn't this check the session polarity to resolve which one is send vs receive from our perspective?
Member
Author
There was a problem hiding this comment.
Yeah, fair enough. I skipped it because in our current code we're always the initiator, but that's not necessarily true in future and could result in catastrophic key reuse in that case. Fixed.
danderson
force-pushed
the
push-vklkwnswztqz
branch
2 times, most recently
from
July 22, 2026 18:42
3b9569b to
a4de20c
Compare
npry
approved these changes
Jul 23, 2026
danderson
force-pushed
the
push-vklkwnswztqz
branch
2 times, most recently
from
July 24, 2026 19:45
30a457e to
ce31ede
Compare
Previously, session rotation triggered only on sending by either peer. This does not conform to the spec (section 6.2): only the initiator should trigger session rotation, and it should do so on both send and receive. Fixes #283 Signed-off-by: David Anderson <danderson@tailscale.com> Change-Id: I4fef3c7306905957932220aca8bcab1f6a6a6964
danderson
force-pushed
the
push-vklkwnswztqz
branch
from
July 24, 2026 19:53
ce31ede to
a120aab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, session rotation triggered only on sending by either peer. This does not conform to the spec (section 6.2): only the initiator should trigger session rotation, and it should do so on both send and receive.
Fixes #283
Change-Id: I4fef3c7306905957932220aca8bcab1f6a6a6964
This change is a bit larger than the minimum surgical change: now that the final consumer of an established session needs to know whether or not it's the initiator (previously it just needed to know what its send/recv keys were), I adjusted the ts_noise APIs to return the session keys in a role-agnostic way (named
initiator_to_responderrather thansendorrecv) along with the local endpoint's role, and let the caller decide how to use each key based on that.For the control protocol this is trivial since the client is always the initiator, but for ts_tunnel BidiSession's constructor is now where the keys are assigned their send/recv purpose, depending on the local role. I found that this ended up being easier to follow through the layers of handshakes and helpers, because it means you don't have to look around at the local context to figure out which traffic direction
keys.sendis supposed to be.This PR has no regression tests for this issue, because as I implemented those I got side-tracked into refactoring the tests to be more readable. That is #284 (currently draft), which I'll polish up and send out as a followup to this PR. The draft PR does include tests for key rotation, both from the initiator and responder's perspective, and they confirm that with this change, rotation triggers at the right time and place.