FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

crypto: fix JWK RSA-PSS SubtleCrypto.exportKey by panva · Pull Request #39828 · nodejs/node · GitHub

/ node Public

crypto: fix JWK RSA-PSS SubtleCrypto.exportKey - #39828

Closed
panva wants to merge 3 commits into
nodejs:masterfrom
panva:fix-webcrypto-rsapss-export
ConversationCommits (3)ChecksFiles changed
Closed

crypto: fix JWK RSA-PSS SubtleCrypto.exportKey#39828
panva wants to merge 3 commits into
nodejs:masterfrom
panva:fix-webcrypto-rsapss-export

Conversation

panva commented Aug 21, 2021

Copy link
Copy Markdown
Member

Allows JWK export from WebCryptoAPI whilst keeping the restriction on KeyObject.prototype.export()

Refs #39805

panva added crypto Issues and PRs related to the crypto subsystem. webcrypto labels Aug 21, 2021
panva requested a review from jasnell August 21, 2021 10:45
nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Aug 21, 2021

panva commented Aug 21, 2021

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

panva added request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. labels Aug 21, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2021

nodejs-github-bot commented Aug 21, 2021

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Aug 21, 2021

Copy link
Copy Markdown
Collaborator

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

jasnell reviewed Aug 21, 2021
View reviewed changes
Comment thread src/crypto/crypto_keys.cc

tniessen commented Aug 21, 2021

Copy link
Copy Markdown
Member

Does this intentionally drop RSA-PSS parameters? That seems to be in violation of the Web Crypto API spec (RSA-PSS "Export Key" operation), and it also seems wrong to remove parameters from a key. If crypto does that (#39805 (comment)), then that's probably a bug in crypto.

tniessen requested changes Aug 21, 2021
View reviewed changes

tniessen left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Making my concerns explicit.

panva commented Aug 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Does this intentionally drop RSA-PSS parameters? That seems to be in violation of the Web Crypto API spec (RSA-PSS "Export Key" operation), and it also seems wrong to remove parameters from a key. If crypto does that (#39805 (comment)), then that's probably a bug in crypto.

@tniessen You've got it backwards - this explicitly allows the JWK export only in Web Crypto API because the CryptoKey instance is already tied to a single Algorithm that represents the digest and use/value of RSA-PSS parameters. WebCryptoAPI exports the "alg" in addition to the key material here in accordance with the Web Cryptography API specification RSA-PSS > Export Key > JWK. The WebCrypto API does not drop anything.

However, because KeyObject does not have that strong tie between its instance and digest and RSA-PSS parameters (and it therefore does not export any "alg" value) we continue to not allow rsa-pss key export as JWK from KeyObject.prototype.export(). This was not allowed ever since its first added in #37081 and it is why this fix is only applied to WebCrypto where it is as per its spec accompanied by an "alg".

panva requested a review from tniessen August 21, 2021 15:01

panva commented Aug 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

This is actually fixing a regression introduced in #39319

tniessen commented Aug 21, 2021

Copy link
Copy Markdown
Member

You've got it backwards - this explicitly allows the JWK export only in Web Crypto API because the CryptoKey instance is already tied to a single Algorithm that represents the digest and use/value of RSA-PSS parameters.

Ah, so the import function ensures that the alg matches the PSS params when importing non-JWK / node.KeyObject? So a PKCS#8 → JWK conversion through Web Crypto always preserves the correct params that were included with the PKCS#8 structure?

tniessen reviewed Aug 21, 2021
View reviewed changes
Comment thread test/parallel/test-webcrypto-export-import-rsa.js Outdated
{ name: 'RSA-PSS', hash: 'SHA-256' },
true,
['verify']);
await subtle.exportKey('jwk', key);

tniessen Aug 21, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Could you add assertions about the returned JWK then? In particular, that it preserves the hash function?

panva Aug 21, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

done.

panva commented Aug 21, 2021 •
edited
Loading

Copy link
Copy Markdown
Member Author

Ah, so the import function ensures that the alg matches the PSS params when importing non-JWK / node.KeyObject? So a PKCS#8 → JWK conversion through Web Crypto always preserves the correct params that were included with the PKCS#8 structure?

Since we have no way of checking the PSS Params (as you know) it does not. You can end up with CryptoKey that does have a different algorithm than the underlying key material. Such key will however fail to be used with both sign and verify. I would say that is a different bug in the Web Crypto API implementation that we should think about how to fix - i.e. how to add RSA-PSS params to keyObject.asymmetricKeyDetails() so that these import checks can be performed.

tniessen approved these changes Aug 21, 2021
View reviewed changes

tniessen left a comment

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I trust your judgement on this, @panva. If this is a regression fix and only affects WebCrypto, not Node.js crypto, and is spec compliant, it should be fine.

panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 21, 2021
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2021

This comment has been minimized.

Sign in to view

nodejs-github-bot commented Aug 21, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Collaborator

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

jasnell pushed a commit that referenced this pull request Aug 25, 2021
crypto: fix JWK RSA-PSS SubtleCrypto.exportKey
4441c3e 
PR-URL: #39828
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell commented Aug 25, 2021

Copy link
Copy Markdown
Member

Landed in 4441c3e

jasnell closed this Aug 25, 2021
targos pushed a commit that referenced this pull request Sep 6, 2021
crypto: fix JWK RSA-PSS SubtleCrypto.exportKey
20cf470 
PR-URL: #39828
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos mentioned this pull request Sep 6, 2021
Merged
codebytere added a commit to electron/electron that referenced this pull request Sep 8, 2021
crypto: fix JWK RSA-PSS SubtleCrypto.exportKey
6030bb7 
nodejs/node#39828
codebytere added a commit to electron/electron that referenced this pull request Sep 8, 2021
crypto: fix JWK RSA-PSS SubtleCrypto.exportKey
adf7f40 
nodejs/node#39828
panva mentioned this pull request Dec 16, 2021
Closed
panva deleted the fix-webcrypto-rsapss-export branch October 13, 2022 09:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
tniessen tniessen approved these changes

Assignees

jasnell

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. webcrypto

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL