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.

Sign up for GitHub

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

Jump to bottom

lib, qlog: Track and Log Unknown Transport Parameters #1861

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

lib, qlog: Track and Log Unknown Transport Parameters #1861

wants to merge 1 commit into from
+255 −13

Conversation

hawkinsw
Copy link
Contributor

@hawkinsw hawkinsw commented Oct 16, 2024 •
edited
Loading

Track (and make available to qlog for logging) transport parameters and their values that this implementation does not specifically recognize.

@hawkinsw hawkinsw requested a review from a team as a code owner October 16, 2024 14:09
@hawkinsw
Copy link
Contributor Author

hawkinsw commented Oct 16, 2024 •
edited
Loading

@LPardue Let me know what you think about the direction of this PR. If you think that it is on the right track, I will gladly do whatever you say so that it meets the high standards of the code that you have already written in this library.

In re: quicwg/qlog#436 and (potentially) quicwg/qlog#176.

@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from 13da4a9 to 7e6b807 Compare October 16, 2024 14:12
LPardue
LPardue reviewed Oct 16, 2024
View reviewed changes
quiche/src/lib.rs Outdated
_ => (),
// Track unknown transport parameters specially.
unknown_tp_id => {
tp.unknown_params.push(UnknownTransportParameter {
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want to put an upper bound on the number of these that we would store, because I don't think there's limit in TLS (other than the full length of the QUIC crypto stream).

I don't know what the bounds should be right now, but as inspiration we have something alongthe same lines for SETTINGS -

quiche/quiche/src/h3/frame.rs

Line 50 in 9ea2152

const MAX_SETTINGS_PAYLOAD_SIZE: usize = 256;
. I don't think it's 100% the same but might be a useful reference point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if you like the latest revision. I decided against using smallvec because I don't think that users will see unknown parameters often enough to warrant pre-allocating space for any entries.

LPardue
LPardue reviewed Oct 16, 2024
View reviewed changes
quiche/src/lib.rs Outdated
@@ -8952,6 +8989,7 @@ mod tests {
initial_source_connection_id: Some(b"woot woot".to_vec().into()),
retry_source_connection_id: Some(b"retry".to_vec().into()),
max_datagram_frame_size: Some(32),
unknown_params: vec![],
Copy link
Contributor

Choose a reason for hiding this comment

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

Its sad we can't test this, but that's because we don't allow apps to send unknown values via the quiche API. I was partly suprised to see we don't send grease TPS (https://datatracker.ietf.org/doc/html/rfc9000#name-reserved-transport-paramete) so maybe that's something we could consider. Although maybe doing it separately might be wise (and the tests could be updated if/when that happens)

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 believe that I have found a way to do at least a little testing on the new feature. Let me know what you think!

@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch 2 times, most recently from 1e51a7a to 8e69291 Compare October 19, 2024 05:05
@hawkinsw hawkinsw requested a review from LPardue October 19, 2024 05:05
@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from 8e69291 to 82ec65f Compare October 19, 2024 05:11
LPardue
LPardue requested changes Oct 20, 2024
View reviewed changes
Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

Overall looks like its headed in the right direction. Left a few comments to help tighten up the defensiveness of the code.

quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch 2 times, most recently from 90c52bc to 1394996 Compare October 21, 2024 22:06
@hawkinsw hawkinsw requested a review from LPardue October 21, 2024 22:07
@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from 1394996 to 746c9cb Compare October 23, 2024 12:46
LPardue
LPardue reviewed Oct 26, 2024
View reviewed changes
Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

Looking good, just a few minor things to harden it up a bit more.

qlog/src/events/quic.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
qlog/src/events/quic.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from 746c9cb to c17adb1 Compare October 28, 2024 20:43
@hawkinsw hawkinsw requested a review from LPardue October 28, 2024 20:44
@hawkinsw
Copy link
Contributor Author

Sorry it took me so long, @LPardue ! If this version looks okay, I will start in on determining how to handle FFI!

@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from c17adb1 to 708c724 Compare October 28, 2024 20:46
LPardue
LPardue reviewed Nov 18, 2024
View reviewed changes
Copy link
Contributor

@LPardue LPardue left a comment

Choose a reason for hiding this comment

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

a few little nits but otherwise very close

quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
quiche/src/lib.rs Outdated Show resolved Hide resolved
@hawkinsw
lib, qlog: Track and Log Unknown Transport Parameters
272230d 
Track (and make available to qlog for logging) transport parameters and
their values that this implementation does not specifically recognize.
@hawkinsw hawkinsw force-pushed the unknown_transport_parameters branch from 708c724 to 272230d Compare November 19, 2024 01:09
@hawkinsw hawkinsw requested a review from LPardue November 19, 2024 01:11
@LPardue
Copy link
Contributor

LPardue commented Nov 25, 2024

LGTM thanks @hawkinsw .

I'd like @ghedo to take a final look over before merging it though

@LPardue LPardue mentioned this pull request Nov 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LPardue LPardue LPardue approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hawkinsw @LPardue