Skip to content

Conversation

@TheJokr
Copy link
Contributor

@TheJokr TheJokr commented Nov 21, 2025

Due to tokio-quiche's stateless address validation via RETRY packets, we rely on the client to reflect our chosen SCID back to us. We should make sure that we actually generated that SCID by binding the retry token to the SCID.

I didn't want to duplicate the SCID inside the token, so I refactored the code a bit to only include data that is not available from elsewhere. This means we also don't need to include the client IP address in it anymore.

@TheJokr TheJokr requested a review from a team as a code owner November 21, 2025 13:00
Due to tokio-quiche's stateless address validation via RETRY packets, we
rely on the client to reflect our chosen SCID back to us. We should make
sure that we actually generated that SCID by binding the retry token to
the SCID.

I didn't want to duplicate the SCID inside the token, so I refactored
the code a bit to only include data that is not available from
elsewhere. This means we also don't need to include the client IP
address in it anymore.
@LPardue
Copy link
Contributor

LPardue commented Nov 21, 2025

I think this would cause some issues for the potential future use of NEW_TOKEN frame, so needs some consideration. Possibly just a TODO comment so we dont forget but needs a little bit of thought

https://datatracker.ietf.org/doc/html/rfc9000#section-19.7

@TheJokr
Copy link
Contributor Author

TheJokr commented Nov 21, 2025

There are some alternatives to fix the issue, this one is just the first one I thought of without re-reading RFC 9000 in detail.

We could just generate a new SCID when receiving a valid retry token. My understanding is that the RFC explicitly allows this. However, quiche doesn't expose an interface to specify the initial SCID and the retry SCID separately, so I'd have to add that. How do you feel about something like quiche::accept_retry(scid, rdcid, odcid, local, peer, config)?

Alternatively, we could also validate the CID using ConnectionIdGenerator::verify_connection_id. This isn't as robust as the retry token (32 byte HMAC vs <=20 bytes shared with other info). It's also limited by whatever format ConnectionIdGenerator implements, but may provide sufficient guarantees.

What do you think?

debug_assert_eq!(tag.len(), HMAC_TAG_LEN);

token.into_inner()
// Drop the non-payload parts of the HMAC and reuse the storage for the
Copy link
Member

Choose a reason for hiding this comment

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

If we are dropping the non-CID parts of the buffer anyway I think we can just use the Hasher::update() API instead to avoid having to allocate the whole token_buf and then truncating it https://docs.rs/boring/latest/boring/hash/struct.Hasher.html#method.update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's what I wanted to do originally, but boring's Hasher doesn't do HMACs natively. The rust crate only exposes the one-shot API.

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.

3 participants