Closed MasterKale closed 1 year ago
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).
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.
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.