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

Fix MSB 1 leading to negative modulus in bun #155

Merged
merged 2 commits into from
Feb 12, 2024

Conversation

ottokruse
Copy link
Contributor

@ottokruse ottokruse commented Feb 9, 2024

Issue #, if available: #154

Description of changes: Fixes a bug in the DER encoder of aws-jwt-verify that does not add a leading 0-byte if the MSB (most significant bit) of the public key modulus is 1. In NodeJS this doesn't throw, because OpenSSL is friendly enough (?) to still parse the modulus as a positive number, but in bun it leads to the modulus being parsed as a negative number as @cirospaciari saw. I think that's actually right, because the number is supposed to be encoded in 2's complement per DER spec (https://www.itu.int/ITU-T/studygroups/com17/languages/X.690-0207.pdf#%5B%7B%22num%22%3A41%2C%22gen%22%3A0%7D%2C%7B%22name%22%3A%22FitH%22%7D%2C799%5D).

(Also, had to fix a unit test that suddenly failed on me in Node v20, as the message changed slightly)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@ottokruse
Copy link
Contributor Author

ottokruse commented Feb 9, 2024

Should also add a unit test to catch the error condition (MSB 1)

Update: DONE

@ottokruse
Copy link
Contributor Author

Note in modern Node (and presume in Bun) you don't need a DER encoder as you can use JWKs natively with crypto. But officially we still support Node14 that can't do that so for now I will leave the DER encoder in place.

@ottokruse ottokruse merged commit 34198c4 into awslabs:main Feb 12, 2024
1 check passed
@ottokruse ottokruse deleted the fix/msb-one branch February 12, 2024 07:03
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

2 participants