Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assert,crypto: make KeyObject and CryptoKey testable for equality #50897

Closed
wants to merge 2 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Nov 24, 2023

This makes CryptoKey and KeyObject instances testable using assert.deepStrictEqual and assert.deepEqual.

The state before was that

  • deepEqual did not check extractable, algorithm, usages
  • neither deepEqual or deepStrict equal compared the key material, so two different keys with the other properties matching evaluated as equal

@panva panva added crypto Issues and PRs related to the crypto subsystem. assert Issues and PRs related to the assert subsystem. webcrypto labels Nov 24, 2023
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Nov 24, 2023
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 24, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva panva added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Nov 25, 2023
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

@panva panva removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 25, 2023
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva
Copy link
Member Author

panva commented Nov 25, 2023

cc @nodejs/crypto

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the review wanted PRs that need reviews. label Nov 30, 2023
@panva panva requested a review from aduh95 December 6, 2023 09:55
@panva
Copy link
Member Author

panva commented Dec 17, 2023

cc @nodejs/crypto

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the use case makes sense, and I'm not able to able to suggest a better implementation, so let's roll with this.

Comment on lines +1230 to +1231
const a = crypto.createSecretKey(Buffer.alloc(1, 0));
const b = crypto.createSecretKey(Buffer.alloc(1, 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I find Buffer.from easier to understand (I had to go read the docs to get what the second argument of Buffer.alloc was for)

Suggested change
const a = crypto.createSecretKey(Buffer.alloc(1, 0));
const b = crypto.createSecretKey(Buffer.alloc(1, 1));
const a = crypto.createSecretKey(Buffer.from([0]));
const b = crypto.createSecretKey(Buffer.from([1]));

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/50897
✔  Done loading data for nodejs/node/pull/50897
----------------------------------- PR info ------------------------------------
Title      assert,crypto: make KeyObject and CryptoKey testable for equality (#50897)
Author     Filip Skokan  (@panva)
Branch     panva:crypto-objects-equal -> nodejs:main
Labels     crypto, assert, util, needs-ci, review wanted, webcrypto, commit-queue-rebase
Commits    2
 - crypto: update CryptoKey symbol properties
 - assert,crypto: make KeyObject and CryptoKey testable for equality
Committers 1
 - Filip Skokan 
PR-URL: https://github.com/nodejs/node/pull/50897
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50897
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 24 Nov 2023 16:50:43 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50897#pullrequestreview-1785408839
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-11-25T19:19:00Z: https://ci.nodejs.org/job/node-test-pull-request/55920/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - crypto: update CryptoKey symbol properties
   ⚠  - assert,crypto: make KeyObject and CryptoKey testable for equality
- Querying data for job/node-test-pull-request/55920/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7237974363

@panva panva added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 17, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 154afbe...0afe731

nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2023
nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2023
@panva panva deleted the crypto-objects-equal branch December 17, 2023 13:16
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@richardlau richardlau mentioned this pull request Mar 25, 2024
@chharvey
Copy link
Contributor

chharvey commented Jun 9, 2024

Not sure if this is the right place to comment but I noticed a regression from v18 to v20, and this PR seems vaguely related.

const a = ['a'];
assert.deepStrictEqual(new Set([a, ['b']]), new Set([a, ['b']]));
  • v18.20.2: pass — no error as expected
  • v20.0.0: fail — unexpected error:

AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal:

Set(2) {
  [
    'a'
  ],
  [
    'b'
  ]
}

The bug seems to occur only when comparing Set objects, with a mix of elements where some elements are compared by reference (===) and other elements are compared by deep equality.

@aduh95
Copy link
Contributor

aduh95 commented Jun 9, 2024

@chharvey can you open a new issue to track this please? (FWIW I'm able to reproduce)

@chharvey
Copy link
Contributor

@aduh95 thanks for your reply. I think I found the culprit and left a comment: #46593 (comment)

@panva
Copy link
Member Author

panva commented Jun 11, 2024

@chharvey can you open a new issue to track this please?

@chharvey
Copy link
Contributor

@aduh95 @panva I opened this issue: #53423

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assert Issues and PRs related to the assert subsystem. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. review wanted PRs that need reviews. util Issues and PRs related to the built-in util module. webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants