Skip to content

Conversation

@panva
Copy link
Member

@panva panva commented Mar 29, 2024

Resolves a TODO from @jasnell to validate RSA-PSS saltLength in SubtleCrypto.sign and SubtleCrypto.verify

fixes: #52188

@panva panva added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. webcrypto labels Mar 29, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@panva panva requested review from jasnell and tniessen March 29, 2024 15:25
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

const type = mode === kSignJobModeSign ? 'private' : 'public';

if (key.type !== type)
throw lazyDOMException(`Key must be a ${type} key`, 'InvalidAccessError');
Copy link
Member

Choose a reason for hiding this comment

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

The error is not sync anymore. Does this make this semver-major change?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not. The entire API surface of webcrypto is async functions.

case 'SHA-1': return 20;
case 'SHA-256': return 32;
case 'SHA-384': return 48;
case 'SHA-512': return 64;
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a default with unreachable (like throw error)

Copy link
Member Author

@panva panva Mar 30, 2024

Choose a reason for hiding this comment

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

It's done the same way as the method above. I don't feel it's necessary. Used hashes are verified far before this is ever invoked.

@panva panva requested a review from anonrig April 2, 2024 13:09
@panva panva added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit 2241e8c into nodejs:main Apr 3, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 2241e8c

@panva panva deleted the webcrypto-validate-saltLength branch April 3, 2024 07:45
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
fixes: #52188
PR-URL: #52262
Fixes: #52188
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 3, 2024
fixes: #52188
PR-URL: #52262
Fixes: #52188
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. webcrypto

Projects

None yet

Development

Successfully merging this pull request may close these issues.

node:crypto RSA-PSS seems to be broken

5 participants