fix: correct JA4 alpn parsing #4721
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goatgoose
approved these changes
Aug 21, 2024
| 0x00, TLS_EXTENSION_ALPN, 0x00, 20, | ||
| 0x00, 18, | ||
| 0, 0x00, 8, 'h', 't', 't', 'p', '/', '1', '.', '1', | ||
| 0, 0x00, 4, 'q', 'u', 'i', 'c'); |
There was a problem hiding this comment.
Probably not worth doing now, and it might be too complex or not be possible for some tests, but I wonder if this could have been caught if these tests modified an s2n_connection and then called s2n_client_hello_send rather than manually constructing the client hello? Then any assumptions about the structure of the client hello wouldn't be in the tests themselves.
maddeleine
approved these changes
Aug 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
I made a VERY silly mistake in the JA4 code: I parsed the alpn extension using the sni extension definition (a two byte length + one byte type instead of just a one byte length). 🤦 I really don't know how that made it past testing and reviews. I caught it when I was adding more pcaps to the pcap tests. All the current test pcaps don't include an alpn, but some of the new pcaps did, and they were all showing up as "00".
Testing:
I added a new known-value unit test. I'll also have a follow-up PR with all the new test pcaps for the pcap tests, which is how I originally found the error.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.