GeezerVison

Review requested:

• @nodejs/crypto

@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 #43656 (comment)

@panva Is the issue with the timeout set in tools/test.py or is this some other timeout?

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.py or is this some other timeout?

node/tools/test.py

Lines 945 to 949 in 3aec7da

	def GetTimeout(self, mode, section=''):
	timeout = self.timeout * TIMEOUT_SCALEFACTOR[ARCH_GUESS or 'ia32'][mode]
	if section == 'pummel' or section == 'benchmark':
	timeout = timeout * 6
	return timeout

If that's the timeout you are referring to, you can perhaps do something similar to what was done for pummel and benchmark tests.

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 Similar to FreeBSD before, the node-test-commit-arm-debug build times out after 5 minutes.

CI: https://ci.nodejs.org/job/node-test-pull-request/45600/

@Trott Similar to FreeBSD before, the node-test-commit-arm-debug build times out after 5 minutes.

@panva I've increased the timeout to 12 minutes. nodejs/build#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.

CI: https://ci.nodejs.org/job/node-test-pull-request/45601/

❗ 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 createSecretKey in 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.

CI: https://ci.nodejs.org/job/node-test-pull-request/45620/

Now on the matter of eventually landing this. I propose to commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. to land the PR in several commits. Also semver-minor PRs that contain new features and should be released in the next minor version. 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 - crypto: fix webcrypto generateKey() AES key length validation error #44170
• crypto: fix webcrypto generateKey() and importKey() EC key namedCurve validation error - crypto: fix webcrypto EC key namedCurve validation errors #44172
• crypto: fix webcrypto HKDF deriveBits() null length validation error - crypto: fix webcrypto deriveBits validations #44173
• crypto: fix webcrypto PBKDF2 deriveBits() null length validation error - crypto: fix webcrypto deriveBits validations #44173
• crypto: fix webcrypto PBKDF2 deriveBits() 0 iterations validation error - crypto: fix webcrypto deriveBits validations #44173
• crypto: fix webcrypto operation errors to be OperationError - crypto: fix webcrypto operation errors to be OperationError #44171
• semver-minor PRs that contain new features and should be released in the next minor version. crypto: allow zero-length secret KeyObject - crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2) #44201
• semver-minor PRs that contain new features and should be released in the next minor version. crypto: allow zero-length IKM in HKDF - crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2) #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.