Skip to content

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

Merged
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
panva:webcrypto-usages
May 7, 2023
Merged

crypto: fix webcrypto private/secret import with empty usages#47877
nodejs-github-bot merged 2 commits into
nodejs:mainfrom
panva:webcrypto-usages

Conversation

@panva

@panvapanva commented May 5, 2023

Copy link
Copy Markdown
Member

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

Refs: #47864

cc @tniessen@fhanau

@nodejs-github-bot

nodejs-github-bot commented May 5, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-botnodejs-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

panva commented May 5, 2023

Copy link
Copy Markdown
MemberAuthor

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.

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

nodejs-github-bot commented May 5, 2023

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented May 5, 2023

Copy link
Copy Markdown
Collaborator

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

@panva

panva commented May 6, 2023

Copy link
Copy Markdown
MemberAuthor

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

tniessen commented May 6, 2023

Copy link
Copy Markdown
Member

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

@panva

panva commented May 6, 2023

Copy link
Copy Markdown
MemberAuthor

In that case this is ready for reviews.

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

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

@tniessentniessenMay 6, 2023

Copy link
Copy Markdown
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){

@panvapanva added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 6, 2023
@panvapanva added the commit-queue Add this label to land a pull request using GitHub Actions. label May 6, 2023
@debadree25debadree25 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-botnodejs-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:mainMay 7, 2023
@nodejs-github-bot

nodejs-github-bot commented May 7, 2023

Copy link
Copy Markdown
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>
@targostargos 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
@panvapanva mentioned this pull request May 15, 2023
Closed
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>
@danielleadamsdanielleadams mentioned this pull request Jul 10, 2023
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@tniessentniessentniessen approved these changes

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.confirmed-bugIssues with confirmed bugs.cryptoIssues and PRs related to the crypto subsystem.needs-ciPRs that need a full CI run.webcrypto

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@panva@nodejs-github-bot@tniessen@jasnell@debadree25