WIP: adds a `signature::Signer` interface #537

baloo · 2024-08-03T23:16:03Z

This brings an implementation of a signature::Signer for keys stored on the TPM.

This is intend to make for easier re-use of this crate and to allow to:

Create certificates https://docs.rs/x509-cert/latest/x509_cert/builder/struct.CertificateBuilder.html
Create CSRs https://docs.rs/x509-cert/latest/x509_cert/builder/struct.RequestBuilder.html
Sign documents with S/MIME https://docs.rs/cms/latest/cms/builder/index.html

Here is an implementation of an SSH agent making use of that infrastructure: wiktor-k/ssh-agent-lib#87

Superhepper

Sorry for reviewing a WIP though I found it a little bit interesting so I couldn't help my self. Feel free to disregard anything I have commented on.

tss-esapi/src/abstraction/public.rs

tss-esapi/src/abstraction/transient/signer.rs

tss-esapi/src/interface_types/algorithm.rs

tss-esapi/src/structures/signatures.rs

tss-esapi/tests/integration_tests/abstraction_tests/public_tests.rs

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo · 2024-08-04T20:38:23Z

Sorry for reviewing a WIP though I found it a little bit interesting so I couldn't help my self. Feel free to disregard anything I have commented on.

Oh, no, thanks for reviewing it!
I sent it a bit early as this is work I've been putting off for almost a year now. I'm doing it mostly to get a sense whether this is a direction the project could go or not.

ionut-arm

Thank you for bringing the patch! The overall design looks good to me.

ionut-arm · 2024-08-05T22:42:18Z

tss-esapi/src/abstraction/signatures.rs

+// Note: this does not implement `TryFrom<RsaSignature>` because `RsaSignature` does not carry the
+// information whether the signatures was generated using PKCS#1v1.5 or PSS.


The fields of RsaSignature are private, so we can extend it to capture this detail as well. It's a deviation from the TPM spec, but I don't necessarily see a problem with it. Thoughts?

cc @Superhepper

No it is not really a problem as long as it does not causes any ambiguities in the conversions between TPMS_SIGNATURE_RSA and RsaSignature.

I think it's fine like that, the comment was more for a future self why we would not have such a TryFrom

ionut-arm · 2024-08-05T22:46:56Z

tss-esapi/src/abstraction/transient/signer.rs

+use signature::{DigestSigner, Error as SigError, KeypairRef};
+
+#[derive(Debug)]
+pub struct Ecdsa<'ctx, C>


(I know this is a draft: ) Would be good to have some docs on these structs to make it clear what they're meant for.

I tried to add some, and add a code sample in the doc as well.

ionut-arm · 2024-08-05T22:58:11Z

tss-esapi/src/abstraction/transient/signer.rs

+    /// Key parameters for this curve
+    pub fn key_params<D>() -> KeyParams


I'm a bit mystified by the purpose of this function. Also by the use of "this curve" in the doc for it, given that Ecdsa is presumably not the description of a curve (?)

C describes the curve (could be NistP256, NistP384, NistP521, ...) those are all supported here. When using the signer (through the Ecdsa struct) you would specify which curve you're using.

This would pick the correct parameters to specify to the TPM when signing, the size of the object that comes back from signature, how to verify them, ...

This function just creates the TPM parameters related to this curve and the selected digest.

Sure, I think I was mostly confused by the existence of two nearly-identical methods before, and that the struct they're tacked to doesn't represent (just) a specific curve.

I did bring back the two nearly-identical methods btw.

baloo · 2024-08-07T03:33:54Z

tss-esapi/tests/integration_tests/abstraction_tests/transient_key_context_tests.rs

+    let builder = RequestBuilder::new(subject, &signer).expect("Create certificate request");
+
+    let cert_req = builder
+        .build::<p256::ecdsa::DerSignature>()
+        .expect("Sign a CSR");


I added proper examples of what this interface is used for.

baloo · 2024-08-07T03:35:07Z

tss-esapi/tests/integration_tests/abstraction_tests/transient_key_context_tests.rs

+// I can't find where in the TPM spec this would be an illegal pair, we'll ignore for now.
+#[ignore]
+#[test]
+fn sign_p256_sha3_256() {


This one is a bit puzzling to me. I can't find where in the spec the the P-256/SHA3-256 pair would be illegal. If anyone has an idea, I'd love to know.

Could it be a limitation of the TPM you're using? From the Structures spec, table 8, it seems SHA3-256 is classified as Assigned rather than TCG standard. I assume this might mean that manufacturers are not required to implement it (?)

I'm using swtpm (0.8.2) with libtpms (0.9.6) I expected it to play nicely. I haven't took time to look in libtpms yet.

Hmm, from this it seems libtpms can block SHA3 at compile-time, perhaps that's what is going on here.

yeah, turns out it's more complicated than that.
stefanberger/libtpms#206

No sha3 support for now.

wiktor-k · 2024-08-07T13:07:32Z

tss-esapi/src/abstraction/hashing.rs

+    const TPM_DIGEST: HashingAlgorithm;
+}
+
+#[cfg(feature = "sha1")]


Hmm... how does this work if I don't see the feature declared in Cargo.toml? 🤔

yeaaah, I learned this one recently too lol.
https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies

By default, this optional dependency implicitly defines a feature that looks like this: [features] gif = ["dep:gif"]

I'm abusing that mechanism here.

I actually need to split the features in the Cargo.toml to look more like:

rustcrypto = ["ecdsa", "elliptic-curve", "signature", "x509-cert"] rustcrypto-full = ["rustcrypto", "p192", "p224", "p256", "p384", "p521", "rsa", "sha1", "sha2", "sha3", "sm2", "sm3"]

That way you can bring in rustcrypto + "p256" + "sha2" if you only care about those.

While I'm at it: I don't know how to name that feature, is rustcrypto good enough?

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo force-pushed the baloo/rust-crypto/signer-interface branch from 85783cc to 6525f15 Compare August 3, 2024 23:17

baloo mentioned this pull request Aug 3, 2024

WIP: examples: adds an agent storing keys on TPMs wiktor-k/ssh-agent-lib#87

Draft

1 task

baloo marked this pull request as draft August 3, 2024 23:20

baloo force-pushed the baloo/rust-crypto/signer-interface branch 2 times, most recently from 336466d to 8145f8e Compare August 4, 2024 03:35

Superhepper reviewed Aug 4, 2024

View reviewed changes

baloo force-pushed the baloo/rust-crypto/signer-interface branch 2 times, most recently from 7331b83 to 18ee544 Compare August 4, 2024 13:51

baloo added 2 commits August 4, 2024 06:54

tss-esapi: move public keys to rustcrypto ecosystem

05fbedf

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

bridge digest crate with tpm digests

1ff6a4e

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo force-pushed the baloo/rust-crypto/signer-interface branch from 18ee544 to 55ea1cc Compare August 4, 2024 13:56

adds a AssociatedHashingAlgorithm to bridge to rustcrypto

412d14a

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo force-pushed the baloo/rust-crypto/signer-interface branch 2 times, most recently from dda6694 to df740fd Compare August 4, 2024 14:17

baloo added 2 commits August 4, 2024 09:27

Convert signature to ecdsa::Signature

ac7f227

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

Convert signature to rsa signatures

bbcb4cc

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo force-pushed the baloo/rust-crypto/signer-interface branch from df740fd to eaf82d5 Compare August 4, 2024 16:28

ionut-arm reviewed Aug 5, 2024

View reviewed changes

baloo force-pushed the baloo/rust-crypto/signer-interface branch 3 times, most recently from fe52b1c to fa8ff3a Compare August 6, 2024 23:16

baloo mentioned this pull request Aug 6, 2024

signature_derive: adds support for lifetimes RustCrypto/traits#1641

Closed

baloo force-pushed the baloo/rust-crypto/signer-interface branch 4 times, most recently from 7bcbb0c to b79e193 Compare August 7, 2024 03:33

baloo commented Aug 7, 2024

View reviewed changes

baloo force-pushed the baloo/rust-crypto/signer-interface branch from b79e193 to 9f760ad Compare August 7, 2024 05:29

wiktor-k reviewed Aug 7, 2024

View reviewed changes

adds a signature::Signer implementation for ecc keys

edf67ed

Signed-off-by: Arthur Gautier <arthur.gautier@arista.com>

baloo force-pushed the baloo/rust-crypto/signer-interface branch from 9f760ad to edf67ed Compare August 7, 2024 16:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: adds a `signature::Signer` interface #537

WIP: adds a `signature::Signer` interface #537

baloo commented Aug 3, 2024

Superhepper left a comment

baloo commented Aug 4, 2024

ionut-arm left a comment

ionut-arm Aug 5, 2024

Superhepper Aug 6, 2024

baloo Aug 6, 2024

ionut-arm Aug 5, 2024

baloo Aug 6, 2024

ionut-arm Aug 5, 2024

baloo Aug 6, 2024

ionut-arm Aug 7, 2024

baloo Aug 7, 2024

baloo Aug 7, 2024

baloo Aug 7, 2024

ionut-arm Aug 7, 2024

baloo Aug 7, 2024

ionut-arm Aug 7, 2024

baloo Aug 7, 2024

wiktor-k Aug 7, 2024

baloo Aug 7, 2024 •

edited

Loading

		// Note: this does not implement `TryFrom<RsaSignature>` because `RsaSignature` does not carry the
		// information whether the signatures was generated using PKCS#1v1.5 or PSS.

		/// Key parameters for this curve
		pub fn key_params<D>() -> KeyParams

WIP: adds a signature::Signer interface #537

Are you sure you want to change the base?

WIP: adds a signature::Signer interface #537

Conversation

baloo commented Aug 3, 2024

Superhepper left a comment

Choose a reason for hiding this comment

baloo commented Aug 4, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baloo Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

WIP: adds a `signature::Signer` interface #537

WIP: adds a `signature::Signer` interface #537

baloo Aug 7, 2024 •

edited

Loading