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 JWK RSA-PSS SubtleCrypto.exportKey #39828

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

panva
Copy link
Member

@panva panva commented Aug 21, 2021

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

Refs #39805

@panva panva added crypto Issues and PRs related to the crypto subsystem. webcrypto labels Aug 21, 2021
@panva panva requested a review from jasnell August 21, 2021 10:45
@nodejs-github-bot 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
Copy link
Member Author

panva commented Aug 21, 2021

cc @nodejs/crypto

@panva 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 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
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@tniessen
Copy link
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
tniessen requested changes Aug 21, 2021
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

Making my concerns explicit.

@panva
Copy link
Member Author

panva commented Aug 21, 2021 •
edited
Loading

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 panva requested a review from tniessen August 21, 2021 15:01
@panva
Copy link
Member Author

panva commented Aug 21, 2021 •
edited
Loading

This is actually fixing a regression introduced in #39319

@tniessen
Copy link
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
tniessen reviewed Aug 21, 2021
View reviewed changes
test/parallel/test-webcrypto-export-import-rsa.js Outdated
{ name: 'RSA-PSS', hash: 'SHA-256' },
true,
['verify']);
await subtle.exportKey('jwk', key);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@panva
Copy link
Member Author

panva commented Aug 21, 2021 •
edited
Loading

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
tniessen approved these changes Aug 21, 2021
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

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 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 Aug 21, 2021
@github-actions 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

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

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

jasnell pushed a commit that referenced this pull request Aug 25, 2021
@panva @jasnell
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
Copy link
Member

jasnell commented Aug 25, 2021

Landed in 4441c3e

@jasnell jasnell closed this Aug 25, 2021
targos pushed a commit that referenced this pull request Sep 6, 2021
@panva @targos
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 targos mentioned this pull request Sep 6, 2021
Merged
codebytere added a commit to electron/electron that referenced this pull request Sep 8, 2021
@codebytere
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
@codebytere
crypto: fix JWK RSA-PSS SubtleCrypto.exportKey
adf7f40 
nodejs/node#39828
@panva panva mentioned this pull request Dec 16, 2021
Closed
@panva panva deleted the fix-webcrypto-rsapss-export branch October 13, 2022 09:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@jasnell jasnell jasnell approved these changes

Assignees

@jasnell jasnell

Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@panva @nodejs-github-bot @tniessen @jasnell