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

New section on SNI parsing and appendix on Google QUIC #124

Merged
merged 14 commits into from Oct 12, 2020
Merged

Conversation

mirjak
Copy link
Contributor

@mirjak mirjak commented Sep 7, 2020

This PR adds a new section on extracting the SNI. I also copied (most of) the pseudo code from Davids document into the appendix. We need to decide if we want to keep this and if so, potentially add some more explanation there.

Also we could further strengthen the point out that both the 5-tuple and conn ID can change (as in David's doc). There is already text directly following the new SNI section on "flow association". If we want stronger text there, maybe we can capture that in a separate issue though.

This PR adds a new section on extracting the SNI. I also copied (most of) the pseudo code from Davids document into the appendix. We need to decide if we want to keep this and if so, potentially add some more explanation there.

Also we could further strengthen the point out that both the 5-tuple and conn ID can change (as in David's doc). There is already text directly following the new SNI section on "flow association". If we want stronger text there, maybe we can capture that in a separate issue though.
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

I don't think that you should include the entirety of David's code. There are ways in which that will contribute to ossification. Indeed, I don't think that it is helpful to list draft versions here either.

draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved
draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved
Copy link
Contributor

@britram britram left a comment

Choose a reason for hiding this comment

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

thanks for this! one major comment agreeing with @martinthomson: the appendix should drop some details of parsing Google QUIC, especially crypto frame extraction. otherwise a few nits.

draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved
draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved

# Appendix

## Handling of Google QUIC {#sec-google-version}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @martinthomson here: beyond this section (mentioning the version numbers) and its first subsection (explaining how to differentiate Google and IETF QUIC), I'd scrub references to Google QUIC from the rest of the pseudocode, and split the appendix in two (first appendix: recognizing google quic; second appendix: initial salt (IETF only) and crypto frame extraction (IETF only)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I removed the salt section entirely (because I think that value should be in the tls draft and only there) and cut down the crypto frame part to IETF QUIC only.


This sections contains algorithms that allows parsing versions from both
Google QUIC and IETF QUIC. These mechanisms will become
irrelevant when IETF QUIC is fully deployed and Google QUIC is deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add something here noting that the rest of this document says nothing about Google QUIC.

draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved
draft-ietf-quic-manageability.md Outdated Show resolved Hide resolved
@britram
Copy link
Contributor

britram commented Oct 12, 2020

fixes #75

@mirjak mirjak merged commit 2ea9437 into master Oct 12, 2020
@britram britram deleted the mirjak-patch-3 branch April 19, 2021 08:34
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

3 participants