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.

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

feat(ext/crypto): implement importKey (jwk) for RSA-PSS , RSA-OAEP and RSA-PKCS1-v1_5 #12022

Closed
wants to merge 51 commits into from
Closed

feat(ext/crypto): implement importKey (jwk) for RSA-PSS , RSA-OAEP and RSA-PKCS1-v1_5 #12022

wants to merge 51 commits into from

Conversation

cryptographix
Copy link
Contributor

@cryptographix cryptographix commented Sep 13, 2021

towards #11690, implementing:

  • importKey(jwk) for RSA-PSS, RSASSA-PKCS1-v1_5, RSA-OAEP
  • exportKey(jwk) for same algorithms.

@littledivy
Copy link
Member

@seanwykes pkcs#8 for all RSA algorithms is being done in #11891

ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
@cryptographix cryptographix changed the title feat(ext/crypto): implement importKey for RSA-PSS (jwk/pkcs8/spki) feat(ext/crypto): implement importKey for RSA-PSS (jwk) Sep 13, 2021
ext/crypto/lib.rs Outdated Show resolved Hide resolved
ext/crypto/lib.rs Outdated Show resolved Hide resolved
@cryptographix cryptographix changed the title feat(ext/crypto): implement importKey for RSA-PSS (jwk) feat(ext/crypto): implement importKey (jwk) for RSA-PSS , RSA-OAEP and RSA-PKCS1-v1_5 Sep 15, 2021
ext/crypto/lib.rs Outdated Show resolved Hide resolved
ext/crypto/lib.rs Outdated Show resolved Hide resolved
ext/crypto/lib.rs Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
ext/crypto/00_crypto.js Outdated Show resolved Hide resolved
@cryptographix
Copy link
Contributor Author

@seanwykes could you please rebase the PR? I would love to land it for 1.17 in two weeks.
@bartlomieju - rebased and cleaned up.
@littledivy and @lucacasonato - ready for code review.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@seanwykes thank you for the PR, it looks mostly good to me. I want to get review from @bnoordhuis and @lucacasonato before landing.

As a heads up it looks to me that there are gonna be conflicts between this PR and #12921. I believe this one should be landed first.

@cryptographix
Copy link
Contributor Author

As a heads up it looks to me that there are gonna be conflicts between this PR and #12921. I believe this one should be landed first.

Yep, already had a look at #12921 and did what I could to minimize conflicts (such as using r#type to indicate keyType, copied an impl for read_rsa_public_key(), etc). Hopefully should be relatively painless.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

I have a feeling #12921 will land first (probably later today), but I am happy to rebase this PR if that causes trouble :-)

ext/crypto/00_crypto.js Show resolved Hide resolved
@cryptographix
Copy link
Contributor Author

@lucacasonato great work on refactors!

PLMK how you want to proceed with this PR. I think the base refactors have all landed now, so are you still going to rebase this?

Or should I rework the JWK import on top of main, and submit a new PR? Actually, I am thinking of 3 PRs

  • JWK RSA Import
  • Refactor exportKey to return the Import enum pkcs8/spki/JWK,raw,
  • JWK RSA export.

Then I can finally rework #13013, hopefully in time for 1.17.

Regards.

@lucacasonato
Copy link
Member

@seanwykes Yes, I will rebase and finish this up today. I was just waiting on #13052 to land.

@lucacasonato
Copy link
Member

And yes, I'll go and pick this PR some more into smaller chunks (first JWK importing). I'll add you as a co-author to all PRs I'll open :-)

@lucacasonato
Copy link
Member

This PR has now been superseded by #13081 and #13071. Thanks @seanwykes!

@cryptographix cryptographix deleted the crypto-rsa-pss-import branch December 14, 2021 16:32
@cryptographix
Copy link
Contributor Author

@lucacasonato what are your plans for #13013? Can you rebase or should I rework it for the shiny new refactored base?

@lucacasonato
Copy link
Member

@seanwykes Up to you: if you want to do it yourself, please split it out into two PRs (one for import, one for export). Otherwise I am also happy to do it 🙂

@cryptographix
Copy link
Contributor Author

@seanwykes Up to you: if you want to do it yourself, please split it out into two PRs (one for import, one for export). Otherwise I am also happy to do it 🙂

I'd like to try, I'm starting to see how thinks tick at deno. I'll split it up as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants