feat(ext/crypto): implement importKey and deriveBits for Pbkdf2 #11642
Conversation
littledivy
changed the title
This comment has been minimized.
This comment has been minimized.
ext/crypto/00_crypto.js
Outdated
| switch (normalizedAlgorithm.name) { | ||
| case "PBKDF2": { | ||
| // 1. | ||
| if (length == null || length == 0 || length % 8 !== 0) { |
There was a problem hiding this comment.
Where does it say that length may not be 0?
There was a problem hiding this comment.
Not in the spec but WPT has a test for it https://github.com/web-platform-tests/wpt/blob/561fc4c5463673e5291894705c7a6aeb88e78d99/WebCryptoAPI/derive_bits_keys/pbkdf2.js#L118
There was a problem hiding this comment.
Can you open a spec issue?
There was a problem hiding this comment.
Sure - w3c/webcrypto#274
There was a problem hiding this comment.
Looks great from my side, other than the minor issues. @bnoordhuis please review the Rust side code too.
| switch (normalizedAlgorithm.name) { | ||
| case "PBKDF2": { | ||
| // 1. | ||
| if (length == null || length == 0 || length % 8 !== 0) { |
There was a problem hiding this comment.
Can you check if there's e.g. a WPT test that tests non-numeric inputs?
The logic is correct because e.g. "bad" % 8 evaluates to NaN and NaN !== 0 but I suspect that if it somehow managed to slip through to Rust land, it'd get coerced to zero.
There was a problem hiding this comment.
@bnoordhuis All the input types are always valid. They are validated by various webidl converters at the top of this function.
There was a problem hiding this comment.
Only null as non-numeric length is tested in WPT.
Wouldn't the WebIDL converter (unsigned long) also complain? Edit: Whoops Github doesn't update in realtime
There was a problem hiding this comment.
Yes, it will. Strings will already fail parsing there, so will never reach this statement.
Introduces
deriveBitsin SubtleCryptoTowards #11690