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

Replace jsrsasign #315

Merged
merged 8 commits into from
Dec 19, 2022
Merged

Replace jsrsasign #315

merged 8 commits into from
Dec 19, 2022

Conversation

MasterKale
Copy link
Owner

In my never-ending quest for third-party dependency minimalism, I discovered I could replace the last remaining use of jsrsasign (for verification of FIDO MDS JWTs) with some of the WebCrypto logic I'd added in #299.

The JWT verification logic I'm replacing jsrsasign with isn't as featureful (it assumes an EC2 leaf cert public key and "alg": "ES256" in the JWT header because that's what FIDO MDS currently uses), but it can easily be expanded upon later if this becomes an issue (perhaps FIDO MDS switches things up, or someone implements a custom MDS service that uses different public keys and alg.) I hopefully left enough notes for my future self to implement support for more JWS algs if that time ever comes.

@erenes
Copy link

erenes commented Dec 18, 2022

Firstly, take my comments with a load of salt, this is not my language nor my domain. But open source thrives when more people take a look at the code, and when I saw you said you rolled your own crypto I couldn't not take a look ;-)

I am glad to see that you're mostly not actually rolling your own crypto :D

Initially I was worried that the JWT is user input that could be maliciously malformed, but since the changes are server-side and the JWT is downloaded from an MDS server (which is only configurable for the developer?), this seems like an irrelevant attack vector.

So my only remark is that your unit test only proves the positive case, not the negative case. False positives are still possible based on the unit test (not on a code review).

@MasterKale
Copy link
Owner Author

So my only remark is that your unit test only proves the positive case, not the negative case. False positives are still possible based on the unit test (not on a code review).

@erenes Thank you for stopping by and keeping me honest. I've gone ahead and added a couple more tests to verify expected errors when working on bad values. It's tougher generating bad values for this JWT stuff in particular, and I didn't want to bloat up the tests with many more megabytes of JWTs (the MDS blob in the test is over 2MB of plaintext...), so I'm going to stop with these three tests for now. I should probably do another test coverage report and make a push to shore up some testing, especially with all the recent changes.

@MasterKale MasterKale merged commit 495c088 into master Dec 19, 2022
@MasterKale MasterKale deleted the replace-jsrsasign branch December 19, 2022 06:12
@MasterKale MasterKale added the package:server @simplewebauthn/server label Dec 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:server @simplewebauthn/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants