crypto: pass all WebCryptoAPI WPTs #43656

panva · 2022-07-02T16:57:10Z

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-github-bot · 2022-07-02T16:57:14Z

Review requested:

@nodejs/crypto

bnoordhuis

Some drive-by comments. I'm surprised WPT expects zero-sized keys to do something meaningful.

src/crypto/crypto_hkdf.cc

panva · 2022-07-03T18:05:33Z

@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.

src/crypto/crypto_hkdf.cc

panva · 2022-07-12T08:17:26Z

@nodejs/testing please take a look at my previous question #43656 (comment)

Trott · 2022-07-12T15:51:20Z

@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.

panva · 2022-07-12T16:06:37Z

@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.

panva · 2022-07-13T09:13:55Z

@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

panva · 2022-07-20T08:06:21Z

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

nodejs-github-bot · 2022-07-22T21:12:06Z

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

Trott · 2022-07-22T21:18:15Z

@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.

nodejs-github-bot · 2022-07-22T22:06:15Z

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

tniessen

This is really great work.

test/parallel/test-crypto-key-objects.js

tniessen

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

I really don't like supporting things that OpenSSL does not support, but at least it's somewhat simple here.

tniessen · 2022-07-23T11:51:25Z

src/crypto/crypto_keys.cc

@@ -871,7 +871,6 @@ void KeyObjectData::MemoryInfo(MemoryTracker* tracker) const {
 }

 std::shared_ptr<KeyObjectData> KeyObjectData::CreateSecret(ByteSource key) {
-  CHECK(key);


We might need to verify that no other uses of KeyObjectData (and related structures) assume that the stored pointer is not nullptr. Otherwise, we might end up with aborts/crashes if someone constructs 0-length keys and uses them with other APIs.

Why is a zero length buffer key data treated as a nullptr? I would like to keep the check here but I don't understand the C++ keyobject implementation enough.

It might be because malloc(len) for len = 0 is allowed to return a nullptr. The standard only requires the returned pointer to be valid for len bytes, which nullptr fulfills when len = 0.

Do you have a suggestion that would allow us to keep this check?

src/crypto/crypto_hkdf.cc

tniessen · 2022-07-23T13:05:18Z

src/crypto/crypto_hkdf.cc

+    if (!EVP_PKEY_CTX_hkdf_mode(ctx.get(),
+                                EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND) ||
+        !EVP_PKEY_CTX_set1_hkdf_salt(
+            ctx.get(), params.salt.data<unsigned char>(), params.salt.size()) ||


Does a non-zero-length key work with a 0-length salt?

https://datatracker.ietf.org/doc/html/rfc5869#section-2.2

I know it's allowed, I am just wondering if OpenSSL implements it that way. The RFC's statement if not provided could be interpreted as "if the user does not call EVP_PKEY_CTX_set1_hkdf_salt".

Now I am confused as to whether an empty salt is the same as passing no salt in our current implementation.

Our implementation does not allow to omit passing salt, we require the argument, albeit we allow it to be zero-length.

Does a non-zero-length key work with a 0-length salt?

that's also covered by a passing wpt btw

src/crypto/crypto_hkdf.cc

panva · 2022-07-23T13:55:06Z

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

nodejs-github-bot · 2022-07-23T19:10:56Z

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

doc/api/crypto.md

lib/internal/crypto/keys.js

tniessen · 2022-07-23T19:40:45Z

src/crypto/crypto_hkdf.cc

+                                    params.key->GetSymmetricKeySize())) {
+      return false;
+    }
+  } else {


Could you add a brief comment to the beginning of the else branch to explain why it exists for future readers?

If we accept that this branch must exist (until we fully drop OpenSSL 1.1.1), then we should probably either

remove the other branch that uses EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND and pass the key to HMAC in this new branch (that should be equivalent as far as I can tell), or

add a TODO comment saying to remove this branch once we have dropped OpenSSL 1.1.1.

The first option would give us improved coverage because all HMAC operations would go through it.

Could you add a brief comment to the beginning of the else branch to explain why it exists for future readers?

Sure.

If we accept that this branch must exist (until we fully drop OpenSSL 1.1.1), then we should probably either

remove the other branch that uses EVP_PKEY_HKDEF_MODE_EXTRACT_AND_EXPAND and pass the key to HMAC in this new branch (that should be equivalent as far as I can tell)

I think that can be done as a follow-up, I am not up for such challenge myself.

add a TODO comment saying to remove this branch once we have dropped OpenSSL 1.1.1.

You mean to change the implementation to use EVP_KDF-HKDF when 1.1.1 is dropped?

tniessen · 2022-07-23T19:46:03Z

test/common/wpt.js

@@ -459,6 +459,12 @@ class WPTRunner {
        );
        this.inProgress.delete(testFileName);
      });
+
+      await new Promise((resolve) => {


If you want to reject when the worker emits an error event, you could also use
await events.once(worker, 'exit').

That being said, it looks like the caller never awaits the returned Promise (e.g., in test-webcrypto.js). What is the point in creating the Promise here if the caller does not await it?

The point is to process the queue one by one. This has helped stabilize CI, especially the keygen tests that easily bogged the hosts down minutes at a time.

If you want to reject when the worker emits an error event, you could also use await events.once(worker, 'exit').

Rejecting would skip running the rest of the queue. So maybe this?

await Promise.allSettled([events.once(worker, 'exit')])

or

await events.once(worker, 'exit').catch(() => {})

panva · 2022-07-23T21:22:14Z

Now on the matter of eventually landing this. I propose to commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several 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?

panva · 2022-08-07T09:01:47Z

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.

panva · 2022-08-11T10:37:05Z

Split up into #44170, #44171, #44172, #44173, and #44201. Pulling updated WPT will be done after they land to avoid unnecessary conflicts.

panva added crypto Issues and PRs related to the crypto subsystem. experimental Issues and PRs related to experimental features. webcrypto labels Jul 2, 2022

panva requested review from jasnell and tniessen July 2, 2022 16:57

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 2, 2022

panva force-pushed the webcrypto-wpt branch 3 times, most recently from 716caf7 to 5b47b67 Compare July 2, 2022 17:40

panva added request-ci Add this label to start a Jenkins CI on a PR. labels Jul 2, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 2, 2022

This comment was marked as outdated.

Sign in to view

bnoordhuis reviewed Jul 2, 2022

View reviewed changes

src/crypto/crypto_hkdf.cc Outdated Show resolved Hide resolved

src/crypto/crypto_hkdf.cc Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Jul 4, 2022

CI Reliability 2022-07-04 nodejs/reliability#317

Open

23 tasks

bnoordhuis reviewed Jul 5, 2022

View reviewed changes

src/crypto/crypto_hkdf.cc Outdated Show resolved Hide resolved

src/crypto/crypto_hkdf.cc Outdated Show resolved Hide resolved

github-actions bot mentioned this pull request Jul 8, 2022

CI Reliability 2022-07-08 nodejs/reliability#318

Open

16 tasks

panva added the review wanted PRs that need reviews. label Jul 12, 2022

This comment was marked as outdated.

Sign in to view

github-actions bot mentioned this pull request Jul 13, 2022

CI Reliability 2022-07-13 nodejs/reliability#321

Open

23 tasks

Trott mentioned this pull request Jul 22, 2022

increasing no-activity timeout for node-test-commit-arm-debug to 12 minutes nodejs/build#3001

Closed

tniessen reviewed Jul 22, 2022

View reviewed changes

test/parallel/test-crypto-key-objects.js Show resolved Hide resolved

tniessen reviewed Jul 23, 2022

View reviewed changes

panva added 3 commits July 23, 2022 20:43

apply review feedback

03275d0

update hkdf docs

8c05855

add 0-length key hmac and equals tests

2561171

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 23, 2022

tniessen reviewed Jul 23, 2022

View reviewed changes

panva added 3 commits July 23, 2022 22:59

add yaml change entries

09a6e50

apply review feedback

d51c30c

make lint happy

c8d7b2c

This was referenced Jul 24, 2022

CI Reliability 2022-07-24 nodejs/reliability#330

Open

CI Reliability 2022-07-25 nodejs/reliability#331

Open

CI Reliability 2022-07-26 nodejs/reliability#332

Open

CI Reliability 2022-07-27 nodejs/reliability#333

Open

tniessen mentioned this pull request Aug 2, 2022

test: update webcrypto api wpt tests #44097

Closed

panva marked this pull request as draft August 8, 2022 10:41

panva mentioned this pull request Aug 10, 2022

crypto: allow zero-length secrets and IKM in HKDF (and in webcrypto PBKDF2) #44201

Closed

panva closed this Aug 11, 2022

panva deleted the webcrypto-wpt branch October 13, 2022 09:12

crypto: pass all WebCryptoAPI WPTs #43656

crypto: pass all WebCryptoAPI WPTs #43656

Conversation

panva commented Jul 2, 2022 • edited Loading

nodejs-github-bot commented Jul 2, 2022

This comment was marked as outdated.

bnoordhuis left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

panva commented Jul 3, 2022 • edited Loading

This comment was marked as outdated.

panva commented Jul 12, 2022

Trott commented Jul 12, 2022

panva commented Jul 12, 2022

This comment was marked as outdated.

This comment was marked as outdated.

panva commented Jul 13, 2022

panva commented Jul 20, 2022 • edited Loading

nodejs-github-bot commented Jul 22, 2022

Trott commented Jul 22, 2022

nodejs-github-bot commented Jul 22, 2022

tniessen left a comment

Choose a reason for hiding this comment

tniessen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panva Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panva commented Jul 23, 2022

nodejs-github-bot commented Jul 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panva Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

panva commented Jul 23, 2022 • edited Loading

panva commented Aug 7, 2022

panva commented Aug 11, 2022

panva commented Jul 2, 2022 •

edited

Loading

panva commented Jul 3, 2022 •

edited

Loading

panva commented Jul 20, 2022 •

edited

Loading

panva Jul 23, 2022 •

edited

Loading

panva Jul 23, 2022 •

edited

Loading

panva commented Jul 23, 2022 •

edited

Loading