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

feat(s2n-quic-dc): DC_STATELESS_RESET_TOKENS frame #2198

Merged
merged 7 commits into from
May 7, 2024

Conversation

WesleyRosenblum
Copy link
Contributor

@WesleyRosenblum WesleyRosenblum commented May 4, 2024

Description of changes:

This change adds a DC_STATELESS_RESET_TOKENS frame containing 1 or more stateless reset tokens for use in s2n-quic-dc.

Call-outs:

  • I added some unsafe code in the creation of a DcStatelessResetToken as I'm internally storing the list of tokens as just a &[u8], but I want the user of DcStatelessResetToken to be operating on a slice of stateless::reset::Tokens. I'm not sure there is a way to do this without unsafe, or maybe just a better way in general.
  • I modified the frames! macro to handle extension frames like this one that have a tag larger than a u8. I couldn't find a way to match tag_macro or extension[extension_tag_macro] so I just have it matching [tag_macro] or extension[extension_tag_macro].

Testing:

Added some tests, and added a new .bin for the new frame. Updating the fuzz testing corpus is TODO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.


//# DC_STATELESS_RESET_TOKENS Frame {
//# Type (i) = 0xdc0000,
//# Stateless Reset Tokens [(128)],
Copy link
Contributor

Choose a reason for hiding this comment

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

Frames need explicit lengths (see STREAM or CRYPTO). Otherwise you'll end up consuming the entire packet payload.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just realized that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about a Count like ACK frames have ACK Range Count (i)? And count * 16 = length

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess length works better with the encode_with_len_prefix and decode_with_len_prefix APIs though

Copy link
Contributor

@camshaft camshaft May 6, 2024

Choose a reason for hiding this comment

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

I like Count, since it's more semantic to what we're communicating. Multiplying by 16 is a single bitshift anyway: https://godbolt.org/z/jbrbz45jx

#[derive(Debug, PartialEq, Eq)]
pub struct DcStatelessResetTokens<'a> {
/// 1 or more 128-bit values
stateless_reset_tokens: &'a [u8],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this to be a slice of stateless_reset::Token. This should be doable if you derive zerocopy traits for that type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is going from &'a [u8] to &'a [[u8; 16]] to &'a [stateless_reset::Token]. Would the zerocopy traits have to be derived also for [stateless_reset::Token]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Happy to bring back the #![forbid(unsafe_code)]. It ended up getting a little funky with having a impl_decode_parameterized macro, but overall nicer

quic/s2n-quic-core/src/frame/dc_stateless_reset_tokens.rs Outdated Show resolved Hide resolved
quic/s2n-quic-core/src/frame/mod.rs Show resolved Hide resolved
@WesleyRosenblum WesleyRosenblum merged commit f05dc62 into main May 7, 2024
127 checks passed
@WesleyRosenblum WesleyRosenblum deleted the WesleyRosenblum/dcstateless branch May 7, 2024 16:56
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.

2 participants