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): support importing ECSDA and ECDH #13088

Merged
merged 16 commits into from
Dec 16, 2021
Merged

feat(ext/crypto): support importing ECSDA and ECDH #13088

merged 16 commits into from
Dec 16, 2021

Conversation

cryptographix
Copy link
Contributor

@cryptographix cryptographix commented Dec 15, 2021

importKey implemented for ECDSA and ECDH, extracted from #13013 with modifications

  • pkcs8 : curve P-256 only
  • spki : curves P-256 and P-384
  • jwk :
    public key - curves P-256 and P-384
    private key - curve P-256 only

Restrictions on to P-384 curve due to lack of support in p384 crate.

@lucacasonato lucacasonato self-assigned this Dec 15, 2021
@lucacasonato lucacasonato added this to the 1.17.0 milestone Dec 15, 2021
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.

Good start. Thanks @seanwykes.

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
ext/crypto/import_key.rs Outdated Show resolved Hide resolved
ext/crypto/import_key.rs Show resolved Hide resolved
ext/crypto/import_key.rs Outdated Show resolved Hide resolved
ext/crypto/import_key.rs Show resolved Hide resolved
ext/crypto/import_key.rs Outdated Show resolved Hide resolved
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.

Are the version bumps necessary? I would rather wait for them until rsa has gotten a new release, so we don't duplicate so many dependencies.

ext/crypto/Cargo.toml Outdated Show resolved Hide resolved
ext/crypto/Cargo.toml Outdated Show resolved Hide resolved
@cryptographix
Copy link
Contributor Author

Are the version bumps necessary? I would rather wait for them until rsa has gotten a new release, so we don't duplicate so many dependencies.

Not as such .. bumped during attempts to improve p384 coverage (pkcs8 etc). Will back-out the changes ..

@lucacasonato
Copy link
Member

@seanwykes I have unbumped the crates locally - will push that shortly.

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.

There are some WPT regressions it seems that I need to be dealt with before merge. (I can do this)

I will try to get this landed for 1.17.0 tomorrow. If it doesn't make it, I'll try to land it for 1.17.1 in a week from now. Maybe we can have web crypto "done" by the end of the year.

@tarcieri
Copy link

I can take a look at getting the rsa crate bumped to the latest versions of pkcs1/pkcs8

@cryptographix
Copy link
Contributor Author

There are some WPT regressions it seems that I need to be dealt with before merge. (I can do this)

I will try to get this landed for 1.17.0 tomorrow. If it doesn't make it, I'll try to land it for 1.17.1 in a week from now. Maybe we can have web crypto "done" by the end of the year.

@lucacasonato WPT - import_export/ec will only pass the supported cases when #13104 is in, since the tests need exportKey to function. PTAL @ #13104 it should hopefully be in reasonable shape. ;)

@lucacasonato
Copy link
Member

import_export/ec will only pass the supported cases when #13104 is in

I am aware. I meant the cases in WebCryptoAPI/sign_verify (I have no idea yet what is going on there).

@cryptographix
Copy link
Contributor Author

import_export/ec will only pass the supported cases when #13104 is in

I am aware. I meant the cases in WebCryptoAPI/sign_verify (I have no idea yet what is going on there).
@lucacasonato, I think there maybe something strange going on with key-hash combinations. In op_crypto_sign_key ECDSA, the comment states underlying ring::signature crate only supports two algo pairs
// We only support P256-SHA256 & P384-SHA384. These are recommended signature pairs.
// https://briansmith.org/rustdoc/ring/signature/index.html#statics
However, the hash param passed from js is checked (SHA-256 or SHA-384), independent of the curve used.

@lucacasonato
Copy link
Member

Yeah, just found that too. This code needs a general rewrite in the future.

I'll clean up this PR now, and then land it.

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.

LGTM, thanks @seanwykes.

ID_SECP256R1_OID => Some(EcNamedCurve::P256),
// id-secp384r1
ID_SECP384R1_OID => Some(EcNamedCurve::P384),
// id-secp384r1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

// id-secp521r1

@cryptographix
Copy link
Contributor Author

LGTM, thanks @seanwykes.

@lucacasonato - thanks for your help, cleanups and fixes. Learnt a lot about paradigmatic Rust from this exercise.

@lucacasonato lucacasonato merged commit 60faf7a into denoland:main Dec 16, 2021
@cryptographix cryptographix deleted the ec-import branch January 5, 2022 22:33
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.

3 participants