node
node copied to clipboard
nodejs
crypto: pass all WebCryptoAPI WPTs
I'm not expecting to land this PR as-is but i'm seeking feedback from @nodejs/crypto namely @tniessen and @jasnell on the end state which this currently reflects.
Overall the question is - is passing all relevant WebCryptoAPI WPT worth the ❗ items?
- update WebCryptoAPI WPT
- OperationError for generateKey() AES key length validation
- NotSupportedError for generateKey() and importKey() EC key namedCurve validation
- OperationError for HKDF deriveBits() length being null
- ❗ support for zero-length secret KeyObject
- OperationError for PBKDF2 deriveBits() iterations being 0
- OperationError for PBKDF2 deriveBits() length being null
- OperationError for when async crypto jobs fail for operation specific reasons
- ❗ workaround for openssl EVP_PKEY_derive HKDF not playing nice with zero-length IKM
- WPT global.location polyfill so that wpt/common/subset-tests.js execute
The C++ linter fails due to variable-length array use and I'd like suggestions for the C++ changes anyway.
I would still break this down into more individual commits. Or PRs if that's what the ask will be.
@nodejs/testing how can I increase the test timeout for test.wpt/test-webcrypto? The new vectors and the fact we execute them all seems to have pushed it over the 2m edge on some configurations as well as github CI.
@nodejs/testing please take a look at my previous question https://github.com/nodejs/node/pull/43656#issuecomment-1173145903
@panva Is the issue with the timeout set in tools/test.py or is this some other timeout?
https://github.com/nodejs/node/blob/3aec7da5c6fcbedffdb034a9b7f6b60958b88d93/tools/test.py#L945-L949
If that's the timeout you are referring to, you can perhaps do something similar to what was done for pummel and benchmark tests.
@panva Is the issue with the timeout set in
tools/test.pyor is this some other timeout?https://github.com/nodejs/node/blob/3aec7da5c6fcbedffdb034a9b7f6b60958b88d93/tools/test.py#L945-L949
If that's the timeout you are referring to, you can perhaps do something similar to what was done for
pummelandbenchmarktests.
Thank you @Trott, I've just pushed an inclusion of wpt in the same condition to see if it'll help or if it times out for some other reason.
@Trott the timeout change has definitely helped on most configurations, and github CI, but node-test-commit-freebsd failed the same way two times in a row, the webcrypto job is not even logged
@Trott the timeout change has definitely helped on most configurations, and github CI, but node-test-commit-freebsd failed the same way two times in a row, the webcrypto job is not even logged
12:16:59 ok 3544 wpt/test-wasm-webapi
12:17:00 ---
12:17:00 duration_ms: 0.979
12:17:00 ...
12:17:00 Build timed out (after 6 minutes). Marking the build as failed.
12:23:00 Build was aborted
I wonder if the "Build timed out (after 6 minutes)" part is a Jenkins thing or something. grep -r 'Build timed out' . on the code base didn't find anything. @nodejs/build
I wonder if the "Build timed out (after 6 minutes)" part is a Jenkins thing or something.
grep -r 'Build timed out' .on the code base didn't find anything. @nodejs/build
Found it in the Jenkins config:
Found it in the Jenkins config:
@nodejs/build I see that node-test-commit-linux has a timeout 20 times larger than FreeBSD (7200 compared to 360). I'm going to take the liberty of bumping FreeBSD up to 720 to see if it fixes this issue. Is there anywhere that we document these changes?
🤞 I've changed the wpt runner to execute wpt files one by one rather than all at the same time, freebsd already passed, might even go without the test.py timeout increase.
Proven wrong: https://ci.nodejs.org/job/node-test-commit-arm-debug/3077/nodes=ubuntu1804-arm64/#showFailuresLink
Scheduled restart of the build (up to 1 times).
Build timed out (after 5 minutes). Marking the build as aborted.
Build was aborted
Performing Post build task...
Match found for : : True
Logical operation result is TRUE
Running script : #/bin/bash -x
mkdir -p out/junit
tap2junit -i test.tap -o out/junit/test.xml
tap2junit -i cctest.tap -o out/junit/cctest.xml
not ok 3559 wpt/test-webcrypto
---
duration_ms: 300.344
severity: crashed
exitcode: -15
stack: |-
@Trott Similar to FreeBSD before, the node-test-commit-arm-debug build times out after 5 minutes.
@Trott Similar to FreeBSD before, the
node-test-commit-arm-debugbuild times out after 5 minutes.
@panva I've increased the timeout to 12 minutes. https://github.com/nodejs/build/issues/3001
I didn't do it before resuming the build 5 minutes ago, though, so if that build fails, use Resume Build on it and it should run again with the 12-minute limit instead.
❗ support for zero-length secret KeyObject
I am not happy about this, but I also don't see a way around it.
❗ workaround for openssl EVP_PKEY_derive HKDF not playing nice with zero-length IKM
It might be worth considering allowing this only through Web Crypto, not through node crypto. On the other hand, it seems to go through
createSecretKeyin both cases, so that might not be pretty.
My thought process was exactly the same. I know we're missing 0-length tests for every single operation we accept a SecretKeyObject in.
I really don't like supporting things that OpenSSL does not support, but at least it's somewhat simple here.
OpenSSL does support it, but this fix did not get extended to the particular API we use. I would happily use the new API but it's only available in 3 and we still have to support 1.
Now on the matter of eventually landing this. I propose to https://github.com/nodejs/node/labels/commit-queue-rebase to land the PR in several commits. Also https://github.com/nodejs/node/labels/semver-minor and leave a note to releasers that only some of the commits are minor, the rest are patch.
- crypto: fix webcrypto generateKey() AES key length validation error - #44170
- crypto: fix webcrypto generateKey() and importKey() EC key namedCurve validation error - #44172
- crypto: fix webcrypto HKDF deriveBits() null length validation error - #44173
- crypto: fix webcrypto PBKDF2 deriveBits() null length validation error - #44173
- crypto: fix webcrypto PBKDF2 deriveBits() 0 iterations validation error - #44173
- crypto: fix webcrypto operation errors to be OperationError - #44171
- https://github.com/nodejs/node/labels/semver-minor crypto: allow zero-length secret KeyObject - #44201
- https://github.com/nodejs/node/labels/semver-minor crypto: allow zero-length IKM in HKDF - #44201
- test,crypto: update WebCryptoAPI WPT - TBD
WDYT?
I will pick this up again and break down to individual PRs after #43455 when every new WPT fixing PR will visibly show how many tests it fixes.
Split up into #44170, #44171, #44172, #44173, and #44201. Pulling updated WPT will be done after they land to avoid unnecessary conflicts.