Feature/crytpo pbkdf2 - #5380
Conversation
This PR made this race condition apparent, so instead of adding another PR to fix it, I've added a small commit here to fix the issue.
…veKey & deriveBits in pbkdf2
…ion' into feature/crytpo-pbkdf2
oleiade
left a comment
There was a problem hiding this comment.
I've left a couple of nits behind, but overall, this looks good to me 👍🏻
Great work there, and really happy to see this feature move forward 🙇🏻
| byteSalt, err := common.ToBytes(salt.Export()) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
This is a nit
I'm not sure where they got it from (I couldn't get a hold of that specific requirement in the specification), but the mozilla web api documentation for the PBKDF2 Params struct indicates it should be at least 16 bytes long, which makes sense to me: https://developer.mozilla.org/en-US/docs/Web/API/Pbkdf2Params#salt.
I remember the IV for encryption/decryption with AES should be 16 bytes though, maybe that's linked.
Even though there is no specific test for it in WPT, I think it would make sense to add that guardrail nonetheless, and add a unit test of our own, and document that behavior?
byteSalt, err := common.ToBytes(salt.Export())
if err != nil {
return nil, err
}
if len(byteSalt) < 16 {
return nil, NewError(OperationError, "salt must be at least 16 bytes") // Similar error as when the iv is not 16 bytes in AES
}There was a problem hiding this comment.
@oleiade some WPT pbkdf2 tests use a 4 byte salt. I can update the tests to make them at min 16, but wanted to run it by you first if you wanted to me to change the actual test cases.
|
@rahulmedicharla I wouldn't, we really want to keep the WPT tests as close as possible to the originals 🙇🏻 The Web Platform Tests are currently red though (I did not notice) was it always the case or did it just happen? |
|
@oleiade no that just happened, it's because the WPT tests have valid test cases where the salt length is 4 bytes, so if I add the 16 byte restriction they are failing. If we don't want to change the WPT tests, we may have to revert that change. |
|
@rahulmedicharla I see, then apologies for the useless round-trip. Please revert the 16 bytes restriction. Our top priority should be for the WPT suite to pass. Sorry again for making you lose time. I poked the k6 maintainers team on this PR, and once the tests are green again, I'll make sure we merge it as soon as possible 🙇🏻 |
|
@oleiade, no worries, I reverted the changes so it should pass now. |
AgnesToulet
left a comment
There was a problem hiding this comment.
LGTM! Thanks for your contribution 🚀
|
Thanks a ton for you contribution @rahulmedicharla 🙇🏻 |
Add documentation for the new PBKDF2 key derivation support: - Add Pbkdf2Params documentation with properties (name, hash, salt, iterations) - Add deriveKey method documentation with supported algorithms - Update deriveBits to include PBKDF2 support and add Throws section - Update importKey with PBKDF2 constraints (extractable=false, raw format only) - Update supported algorithms tables to include PBKDF2 This documents the changes for grafana/k6#5380
Add documentation for the new PBKDF2 key derivation support: - Add Pbkdf2Params documentation with properties (name, hash, salt, iterations) - Add deriveKey method documentation with supported algorithms - Update deriveBits to include PBKDF2 support and add Throws section - Update importKey with PBKDF2 constraints (extractable=false, raw format only) - Update supported algorithms tables to include PBKDF2 This documents the changes for grafana/k6#5380
What
This PR implements the PBKDF2 support in the webcrypto module following the WebCrypto Api specification.
Why?
Having support of the PBKDF2 makes our implementation compliant with web crypto API
Checklist
Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #4267