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

Boring SSL update #31

Merged
merged 5 commits into from
Jul 17, 2020
Merged

Boring SSL update #31

merged 5 commits into from
Jul 17, 2020

Conversation

0xTim
Copy link
Member

@0xTim 0xTim commented Jul 16, 2020 •
edited by tanner0101
Loading

Brings in the latest version of BoringSSL (#31).

@0xTim 0xTim requested a review from tanner0101 July 16, 2020 07:05
@0xTim 0xTim mentioned this pull request Jul 16, 2020
Merged
@0xTim
Copy link
Member Author

0xTim commented Jul 16, 2020

@tanner0101 this test is failing on master too - looks like there might actually be a bug. From the brief look at it, it's looks like the payload is correct but the header and signature are different

@tanner0101 tanner0101 added the enhancement New feature or request label Jul 16, 2020
@tanner0101
Copy link
Member

tanner0101 commented Jul 16, 2020 •
edited
Loading

Looks like the JWT header key sorting changed. So a bug in the test technically.

Screenshot from 2020-07-16 12-12-19

I set the header encoder to sort keys but I'm not sure that's the right solution. The keys don't really need to be sorted and that costs cpu time.

Really the deterministic encoding just makes testing easier, so this should probably go behind some sort of flag. Or the tests should be refactored to not care about JSON ordering. Thoughts?

@0xTim
Copy link
Member Author

0xTim commented Jul 17, 2020

Yeah I agree having the header keys sorted in prod code is not the way forward. My preference would be to refactor the tests so that they don't care about JSON ordering - since this is what happens in real life. As a first pass to get this and #32 unblocked we could check both variants in the test. Since there are only two keys there are only two possible values so it's not unreasonable

@tanner0101 tanner0101 added this to the m milestone Jul 17, 2020
@tanner0101 tanner0101 added semver-patch Internal changes only semver-minor Contains new APIs and removed semver-patch Internal changes only labels Jul 17, 2020
@tanner0101 tanner0101 changed the title Boring SSL Update Boring SSL update Jul 17, 2020
@tanner0101 tanner0101 merged commit fa439a7 into master Jul 17, 2020
@tanner0101 tanner0101 deleted the boring-ssl-update branch July 17, 2020 15:15
@tanner0101
Copy link
Member

These changes are now available in 4.0.0-rc.1.6

@tanner0101 tanner0101 removed this from the m milestone Jul 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanner0101 tanner0101 tanner0101 approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request semver-minor Contains new APIs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@0xTim @tanner0101