Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

token-related fixes #123

Closed
wants to merge 2 commits into from
Closed

Conversation

marten-seemann
Copy link
Collaborator

Fixes #122.

This PR contains 2 fixes:

  • stateless reset tokens are not tokens
  • PacketHeader already contains a Token, so we don't need another field on the packet_sent and packet_received event

@@ -1817,7 +1813,7 @@ class PacketHeader {

flags?: uint8; // the bit flags of the packet headers (spin bit, key update bit, etc. up to and including the packet number length bits if present) interpreted as a single 8-bit integer

token?:Token; // only if packet_type == initial
token?:Token; // only if packet_type == initial || retry
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is a bit of a difficult one imo.
I'd put retry_token outside, since it's encapsulated in its own packet type, and so it can be considered to be part of the "payload" rather than the header.
This is different from the initial token, which is clearly part of the header.

If we do put the retry token in the PacketHeader struct, imo it would make sense to do the same for the stateless_reset_token as well, since from the same logic, you could say that that is part of the Stateless Reset Packet header as well, rater than its payload.

I think it's probably cleaner to have all three in the PacketHeader then, with token?:Token covering both initial and retry as you proposed here.

@marten-seemann
Copy link
Collaborator Author

Conclusion: remove retry_token from packet_sent and packet_received, allow using the token field on the header for Retry packets.

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.

None yet

2 participants