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.

Sign up for GitHub

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

Jump to bottom

crypto: fix webcrypto private/secret import with empty usages #47877

Merged
merged 2 commits into from May 7, 2023
Merged

crypto: fix webcrypto private/secret import with empty usages #47877

merged 2 commits into from May 7, 2023

Conversation

panva
Copy link
Member

@panva panva commented May 5, 2023

private and secret type keys are not allowed to have empty usages

Refs: #47864

cc @tniessen @fhanau

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels May 5, 2023
@panva
Copy link
Member Author

panva commented May 5, 2023

How do we deal with the semverness of this? The keys were unusable before, they are non-importable now. It is unlikely any code depended on import of keys that weren't usable for any operation but I'm keen to get more opinions.

@panva panva added confirmed-bug Issues with confirmed bugs. webcrypto labels May 5, 2023
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 5, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@panva
Copy link
Member Author

panva commented May 6, 2023

How do we deal with the semverness of this? The keys were unusable before, they are non-importable now. It is unlikely any code depended on import of keys that weren't usable for any operation but I'm keen to get more opinions.

Furthermore if any interoperable code uses webcrypto (kinda its whole point) it already can't depend on this as other implementations properly reject.

I'm proposing to ship this is as a fix not as a breaking change.

@tniessen
Copy link
Member

tniessen commented May 6, 2023

I'd expect near-zero breakage, so spec-compliance outweighs that concern.

@panva
Copy link
Member Author

panva commented May 6, 2023

In that case this is ready for reviews.

tniessen
tniessen approved these changes May 6, 2023
View reviewed changes
lib/internal/crypto/webcrypto.js
}

throw lazyDOMException('Unrecognized algorithm name', 'NotSupportedError');
if ((result.type === 'secret' || result.type === 'private') && result.usages.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ((result.type === 'secret' || result.type === 'private') && result.usages.length === 0) {
if (result.type !== 'public' && result.usages.length === 0) {

@panva panva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 6, 2023
@panva panva added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023
@debadree25 debadree25 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 7, 2023
@nodejs-github-bot nodejs-github-bot merged commit 71eda57 into nodejs:main May 7, 2023
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 71eda57

@panva panva deleted the webcrypto-usages branch May 7, 2023 11:31
targos pushed a commit that referenced this pull request May 12, 2023
@panva @targos
crypto: fix webcrypto private/secret import with empty usages
09fb74a 
Refs: #47864
PR-URL: #47877
Refs: #47864
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request May 15, 2023
Merged
panva added a commit to panva/node that referenced this pull request May 15, 2023
@panva
crypto: fix webcrypto private/secret import with empty usages
ca5b942 
Refs: nodejs#47864
PR-URL: nodejs#47877
Refs: nodejs#47864
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#47336
@panva panva mentioned this pull request May 15, 2023
Closed
@panva panva added lts-watch-v16.x lts-watch-v18.x PRs that may need to be released in v18.x. labels May 20, 2023
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
@panva @danielleadams
crypto: fix webcrypto private/secret import with empty usages
70cca8e 
Refs: #47864
PR-URL: #47877
Refs: #47864
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
MoLow pushed a commit to MoLow/node that referenced this pull request Jul 6, 2023
@panva @MoLow
crypto: fix webcrypto private/secret import with empty usages
de1338d 
Refs: nodejs#47864
PR-URL: nodejs#47877
Refs: nodejs#47864
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Jul 10, 2023
Merged
@panva panva removed lts-watch-v18.x PRs that may need to be released in v18.x. lts-watch-v16.x labels Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@tniessen tniessen tniessen approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. confirmed-bug Issues with confirmed bugs. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. webcrypto
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@panva @nodejs-github-bot @tniessen @jasnell @debadree25