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(extensions/crypto): implement generateKey() & sign() #9614

Merged
merged 166 commits into from
Jul 6, 2021

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Feb 27, 2021

Related #1891
Implements CryptoKey related logic along with the following methods:

generateKey() and sign()

  • RSASSA-PKCS1-v1_5
  • RSA-PSS
  • ECDSA
  • HMAC

Uses ring for ECDSA & HMAC
Uses rsa for RSA-*

@littledivy littledivy changed the title feat(op_crates/crypto): generateKey() & sign() [wip] feat(op_crates/crypto): generateKey() & sign() Feb 27, 2021
@lucacasonato lucacasonato self-requested a review June 30, 2021 21:32
@lucacasonato
Copy link
Member

Unfortunately we cannot test sign until we have verify, because they are both used in the WPT tests.

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.

Happy with this now. Thanks a lot @littledivy, and sorry that this took so long to land.

@bartlomieju could you additionally review?

@bnoordhuis could you review the key generation ops for any egregious security issues?

@@ -18,5 +18,11 @@ deno_core = { version = "0.92.0", path = "../../core" }
deno_web = { version = "0.41.1", path = "../web" }
tokio = { version = "1.7.1", features = ["full"] }
rand = "0.8.3"
ring = "0.16.20"
ring = { version = "0.16.20", features = ["std"] }
rsa = "0.4.0" # TODO: remove "pem" feature when next release is on crates.io
Copy link
Member

Choose a reason for hiding this comment

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

This TODO is outdated

Copy link
Member

Choose a reason for hiding this comment

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

No, I am waiting for RustCrypto/RSA@366ff6e to land in a release.

extensions/crypto/lib.rs Outdated Show resolved Hide resolved
extensions/crypto/Cargo.toml Outdated Show resolved Hide resolved
ring = "0.16.20"
ring = { version = "0.16.20", features = ["std"] }
rsa = "0.4.0" # TODO: remove "pem" feature when next release is on crates.io
sha-1 = "0.9.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

perf nit: we could enable the asm feature of sha-1/sha2. Should we?

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Rust bits LGTM. Thanks for the PR!

extensions/crypto/key.rs Outdated Show resolved Hide resolved
));
}

let mut rng = OsRng;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using different PRNGs is kind of regrettable, although I understand you can't really get around that. I trust both rand and ring to get it right but still...

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.

LGTM too

@lucacasonato lucacasonato merged commit 570309d into denoland:main Jul 6, 2021
@panva
Copy link
Contributor

panva commented Jul 21, 2021

@lucacasonato

To circumvent these issues I suggest we drop support for "non extractable" keys and throw a SecurityError DOMException when you try to create one. The bytes that back a key should be stored on the JS object, and no resources should be involved.

Can you reconsider the way non-extractable keys are handled. Existing libraries that implement higher level functions often work with generating single-use keys generated on the fly to use in lower level functions. These use extractable: false (also because some algorithms only work with extractable: false, e.g. PBKDF2).

This lack of support will mean existing libraries will not be usable under deno, only because of this one thing.

@lucacasonato
Copy link
Member

@panva We are happy to reconsider, but first we need clarification from the spec authors if "non extractable keys" are meant to be a security primitive or not. Deno has no way to implement non extractable keys securely if they are meant to be a security primitive.

I'll do some more research and will reach out to some WebCrypto API authors to figure out the details.

@panva
Copy link
Contributor

panva commented Jul 21, 2021

@panva We are happy to reconsider, but first we need clarification from the spec authors if "non extractable keys" are meant to be a security primitive or not. Deno has no way to implement non extractable keys securely if they are meant to be a security primitive.

I'll do some more research and will reach out to some WebCrypto API authors to figure out the details.

Thank you, that's fair.

I have not encountered a normative language in the spec that would say e.g. that the memory used to hold the non-extractable keying material must be out of reach, it only says the raw keying material may not be exported by the application.

extractable
Reflects the [[extractable]] internal slot, which indicates whether or not the raw keying material may be exported by the application.

@lucacasonato
Copy link
Member

I have opened #11481 to figure this out.

@littledivy littledivy deleted the op_crates/crypto/generateKey branch September 1, 2021 07:02
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.

9 participants