Compatiblity layer with RustCrypto #192

baloo · 2023-11-23T04:41:23Z

This is another take on #158

The purpose is to bring a compatibility layer with rustcrypto and use the tools developed there (x509 signing, pkcs7/CMS, ocsp, ...) with a pkcs#11-backed HSM

cryptoki-rustcrypto/src/ecdsa.rs

ionut-arm · 2023-11-25T16:32:58Z

Amazing amount of work 🤯 thank you for the patch, I'll do a deep-dive when I get a bit of time.

One thing I'm wondering is if it makes more sense to have the extra functionality as a new crate, or as a module in the existing crate. We have something similar in tss-esapi with the abstractions module - not really sure which way is better, tbh, they both have advantages.

EDIT: if we keep it as a separate crate maybe we can frame it as a generic abstractions crate as well - if we want to implement other nice-to-have functionality in the future, we can just chuck it in the same one, as a separate mod.

baloo · 2023-11-25T20:41:08Z

I don't really have an opinion whether to make it a module (behind a feature flag) or an additional crate. What I know is that this is going to end up pulling a chunky part of rust-crypto and that dependency management is going to be tedious.
I tend to believe this is going to be slightly easier with an external crate, but I'm fine either way.

I do intend to bring a similar abstraction over to tss-esapi.

For the background story of this effort, nixos folks (this includes @RaitoBezarius who chimed in earlier) are doing secureboot work, and intend to use rust to sign build artifacts (with both HSM and TPM, depending on the use-case).
We have a PE signer (https://github.com/RaitoBezarius/goblin-signing) that consumes a signatures::Signer.
I (/me pulls the professional hat) am deploying a similar effort at work (secureboot efforts as well as PKI work).

I do intend to continue supporting this in the foreseeable future.

cryptoki-rustcrypto/src/ecdsa.rs

cryptoki-rustcrypto/tests/ecdsa.rs

RaitoBezarius · 2024-01-02T01:06:31Z

Happy new year! Anything we can do on our side to make this PR gets in a nice state for merge? :)

ionut-arm

Thank you for the patch! I've read throug the non-test code and it seems like a great step forward to me. Left a few comments below, but overall I'd be happy to merge.

Regarding the massive delay - if either of you would be willing to join us on the Slack channel, please ping me directly there and I'll make time to look at changes/issues/etc. Unfortunately it's all too easy for things to slip between cracks 😞

cryptoki-rustcrypto/src/lib.rs

cryptoki-rustcrypto/tests/15b05c4865410c6b3ff76a4e8f3d87276756bd0c.der

cryptoki-rustcrypto/src/ecdsa.rs

ionut-arm · 2024-02-19T16:29:35Z

cryptoki-rustcrypto/src/rng.rs

+/// [`Rng`] is a PKCS#11-backed CSPRNG.
+pub struct Rng<S: SessionLike>(S);
+
+// TODO(baloo): check for CKF_RNG bit flag (CK_TOKEN_INFO struct -> flags)


Was this resolved, or will it result in a new issue once this is merged?

No, I'll need to make an API change to session to get back the Pkcs11 object and get_token_info on it.

ionut-arm · 2024-02-19T16:32:13Z

cryptoki-rustcrypto/src/rng.rs

+    fn fill_bytes(&mut self, dest: &mut [u8]) {
+        self.try_fill_bytes(dest)
+            .expect("Cryptoki provider failed to generate random");
+    }


I take it there's no way to not implement this and have the user always deal with the potential error. That's quite unfortunate. Maybe a comment on this method would be good to warn of a potential panic?

Yep. I think the "panic warninig" is part of the documentation of the interface:

This method should guarantee that dest is entirely filled with new data, and may panic if this is impossible (e.g. reading past the end of a file that is being used as the source of randomness).

Source: https://docs.rs/rand_core/latest/rand_core/trait.RngCore.html#tymethod.fill_bytes

I added a documentation for that panic on our Rng object.

ionut-arm · 2024-02-19T16:34:28Z

cryptoki-rustcrypto/src/rsa/mod.rs

+pub enum Error {
+    #[error("Cryptoki error: {0}")]
+    Cryptoki(#[from] cryptoki::error::Error),
+
+    #[error("Private key missing attribute: {0}")]
+    MissingAttribute(AttributeType),
+
+    #[error("RSA error: {0}")]
+    Rsa(#[from] rsa::Error),
+
+    #[error("Key not found")]
+    MissingKey,
+}


I'm curious - why have a different error struct per module? Seems like it could add some headaches if someone wants to use multiple crypto primitives in the same file, they'd have to import and rename them.

I agree. One Error type should be sufficient. ( ™️ )

(But seriously, it's not such a big crate to need so many Errors).

I go back and forth between "errors should be the most precise" and "dyn Error is enough for everyone". Looks like that day was the former.

cryptoki-rustcrypto/src/ecdsa.rs

wiktor-k

It's a great idea and thanks for working on it... I expect to be in need of this kind of crate in the future so... thanks for your time and work! 🙏

cryptoki-rustcrypto/src/lib.rs

wiktor-k · 2024-02-21T08:57:36Z

cryptoki-rustcrypto/src/rng.rs

+    fn fill_bytes(&mut self, dest: &mut [u8]) {
+        self.try_fill_bytes(dest)
+            .expect("Cryptoki provider failed to generate random");
+    }


Yep. I think the "panic warninig" is part of the documentation of the interface:

This method should guarantee that dest is entirely filled with new data, and may panic if this is impossible (e.g. reading past the end of a file that is being used as the source of randomness).

Source: https://docs.rs/rand_core/latest/rand_core/trait.RngCore.html#tymethod.fill_bytes

wiktor-k · 2024-02-21T08:58:43Z

cryptoki-rustcrypto/src/rsa/mod.rs

+pub enum Error {
+    #[error("Cryptoki error: {0}")]
+    Cryptoki(#[from] cryptoki::error::Error),
+
+    #[error("Private key missing attribute: {0}")]
+    MissingAttribute(AttributeType),
+
+    #[error("RSA error: {0}")]
+    Rsa(#[from] rsa::Error),
+
+    #[error("Key not found")]
+    MissingKey,
+}


I agree. One Error type should be sufficient. ( ™️ )

(But seriously, it's not such a big crate to need so many Errors).

Signed-off-by: Arthur Gautier <[email protected]>

https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf Signed-off-by: Arthur Gautier <[email protected]>

Signed-off-by: Arthur Gautier <[email protected]>

baloo force-pushed the baloo/rustcrypto/init branch 5 times, most recently from 7f68cb7 to 3e33f45 Compare November 23, 2023 09:42

RaitoBezarius mentioned this pull request Nov 23, 2023

feat(session): support Signer trait through a Session #158

Closed

RaitoBezarius reviewed Nov 23, 2023

View reviewed changes

cryptoki-rustcrypto/src/ecdsa.rs Outdated Show resolved Hide resolved

baloo force-pushed the baloo/rustcrypto/init branch 4 times, most recently from a68557d to 230120d Compare November 25, 2023 07:17

baloo force-pushed the baloo/rustcrypto/init branch 2 times, most recently from b7a3961 to b00e7c5 Compare November 26, 2023 22:28

baloo commented Nov 28, 2023

View reviewed changes

cryptoki-rustcrypto/src/ecdsa.rs Outdated Show resolved Hide resolved

baloo commented Nov 28, 2023

View reviewed changes

cryptoki-rustcrypto/tests/ecdsa.rs Outdated Show resolved Hide resolved

baloo force-pushed the baloo/rustcrypto/init branch 2 times, most recently from 066d6fa to 2972f78 Compare December 1, 2023 23:35

baloo force-pushed the baloo/rustcrypto/init branch from f6998fc to bdec32f Compare January 2, 2024 21:27

tgonzalezorlandoarm requested review from tgonzalezorlandoarm and removed request for tgonzalezorlandoarm January 16, 2024 11:47

ionut-arm reviewed Feb 19, 2024

View reviewed changes

wiktor-k mentioned this pull request Feb 21, 2024

signature::Signer support rpgp/rpgp#286

Open

wiktor-k reviewed Feb 21, 2024

View reviewed changes

baloo added 4 commits February 23, 2024 21:25

rustcrypto: initial commit

3c81335

Signed-off-by: Arthur Gautier <[email protected]>

ecdsa support

58bed5d

Signed-off-by: Arthur Gautier <[email protected]>

demo x509 builder usage

eb8f331

Signed-off-by: Arthur Gautier <[email protected]>

use a session ref

00f0158

Signed-off-by: Arthur Gautier <[email protected]>

baloo force-pushed the baloo/rustcrypto/init branch 2 times, most recently from 33aa4e1 to 24d9148 Compare February 24, 2024 05:56

baloo added 10 commits February 23, 2024 22:04

Add helpers to read public keys

a248888

Signed-off-by: Arthur Gautier <[email protected]>

ecdsa: bring support for k256, p224 and p384

0883198

Signed-off-by: Arthur Gautier <[email protected]>

adds x509 certificate support

c25fcf4

Signed-off-by: Arthur Gautier <[email protected]>

provide a cryptoki-backed CSPRNG

5b9b05b

Signed-off-by: Arthur Gautier <[email protected]>

fixup pss

37c82bb

Signed-off-by: Arthur Gautier <[email protected]>

rework rsa signer algorithm

6d4e4cb

Signed-off-by: Arthur Gautier <[email protected]>

cleanup

edbc82a

Signed-off-by: Arthur Gautier <[email protected]>

fixup ecdsa signer implementation

5254651

Signed-off-by: Arthur Gautier <[email protected]>

ecdsa: implement key import

b1de50d

Signed-off-by: Arthur Gautier <[email protected]>

re-export cryptoki

7512207

Signed-off-by: Arthur Gautier <[email protected]>

baloo force-pushed the baloo/rustcrypto/init branch from 24d9148 to 7512207 Compare February 24, 2024 06:04

wiktor-k mentioned this pull request Feb 27, 2024

Add support for private key operations based on hardware security devices rpgp/rpgp#299

Closed

baloo added 4 commits February 27, 2024 20:34

TODO(split that): Add support for SP.800-38F key wrapping

3dd1430

https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-38F.pdf Signed-off-by: Arthur Gautier <[email protected]>

fixup import

28b92e4

Signed-off-by: Arthur Gautier <[email protected]>

add missing methods to SessionLike

64f57a5

Signed-off-by: Arthur Gautier <[email protected]>

bump rustcrypto dependencies to pre-releases

45a1d80

Signed-off-by: Arthur Gautier <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compatiblity layer with RustCrypto #192

Compatiblity layer with RustCrypto #192

baloo commented Nov 23, 2023 •

edited

Loading

ionut-arm commented Nov 25, 2023 •

edited

Loading

baloo commented Nov 25, 2023

RaitoBezarius commented Jan 2, 2024

ionut-arm left a comment

ionut-arm Feb 19, 2024

baloo Feb 24, 2024

ionut-arm Feb 19, 2024

wiktor-k Feb 21, 2024

baloo Feb 24, 2024 •

edited

Loading

ionut-arm Feb 19, 2024

wiktor-k Feb 21, 2024

baloo Feb 24, 2024 •

edited

Loading

wiktor-k left a comment

wiktor-k Feb 21, 2024

wiktor-k Feb 21, 2024

Compatiblity layer with RustCrypto #192

Are you sure you want to change the base?

Compatiblity layer with RustCrypto #192

Conversation

baloo commented Nov 23, 2023 • edited Loading

ionut-arm commented Nov 25, 2023 • edited Loading

baloo commented Nov 25, 2023

RaitoBezarius commented Jan 2, 2024

ionut-arm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

wiktor-k left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo commented Nov 23, 2023 •

edited

Loading

ionut-arm commented Nov 25, 2023 •

edited

Loading

baloo Feb 24, 2024 •

edited

Loading

baloo Feb 24, 2024 •

edited

Loading