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

x509-cert: don't bind builder with signer early #1161

Merged
merged 2 commits into from
Jan 20, 2024

Conversation

baloo
Copy link
Member

@baloo baloo commented Jul 22, 2023

This is mostly a draft after discussion in #1160

cc @bkstein @tarcieri @jstayton

@tarcieri
Copy link
Member

Looks good at first glance

@bkstein
Copy link
Contributor

bkstein commented Jul 24, 2023

I think, this will improve the situation. However, monomorphization will still take place.

@tarcieri
Copy link
Member

The signing API isn't dynamic (yet) so we can't yet avoid monomorphization around the signing operation. It would need fully dynamic counterparts to everything in signature including signature::Keypair.

@baloo
Copy link
Member Author

baloo commented Jul 27, 2023

This is a breaking change. I don't think we mean to introduce a 0.3 of x509-cert yet. I'm happy with the current state, but I'm not in a hurry to get it merged.

@tarcieri
Copy link
Member

Yeah, that's fine. We can keep it open until the next breaking release cycle.

@tarcieri tarcieri mentioned this pull request Aug 6, 2023
@baloo baloo added the breaking Change to be merged with next release cycle label Nov 24, 2023
@baloo baloo force-pushed the baloo/x509-cert/builder-dynamic-signer branch 4 times, most recently from 41e0d7b to 25a4391 Compare December 13, 2023 17:12
fn finalize<S>(
&mut self,
_signer: &S,
) -> core::result::Result<Vec<u8>, x509_cert::builder::Error>
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be independent of x509_cert?

Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Member Author

Choose a reason for hiding this comment

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

That was just a rebase from this PR on a fresh master.
But I needed to change the signature of finalize because I attach the public key of the signer "late".
Getting the public from the keypair made it so that I could not get away with just a der::Result.

This is still a breaking change though.

cms/src/builder.rs Show resolved Hide resolved
@baloo baloo force-pushed the baloo/x509-cert/builder-dynamic-signer branch from 25a4391 to a561c25 Compare January 2, 2024 22:49
@baloo baloo force-pushed the baloo/x509-cert/builder-dynamic-signer branch from a561c25 to bbe0523 Compare January 4, 2024 21:02
@baloo baloo marked this pull request as ready for review January 4, 2024 21:06
@baloo baloo force-pushed the baloo/x509-cert/builder-dynamic-signer branch 2 times, most recently from 5a3ce21 to 183bbbf Compare January 20, 2024 00:17
This is mostly a draft after discussion in RustCrypto#1160

Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
Signed-off-by: Arthur Gautier <baloo@superbaloo.net>
@baloo baloo force-pushed the baloo/x509-cert/builder-dynamic-signer branch from 183bbbf to ff582c3 Compare January 20, 2024 04:35
@baloo baloo merged commit 345ac96 into RustCrypto:master Jan 20, 2024
53 checks passed
@baloo baloo deleted the baloo/x509-cert/builder-dynamic-signer branch January 20, 2024 04:55
@a1ien
Copy link

a1ien commented Mar 19, 2024

Could finalize possibly accept public_key or perhaps EncodePublicKey instead of signer? This adjustment would enable the implementation of signing using external crates or hardware tokens.
For instance, with a hardware token, the workflow could proceed as follows:

Obtain the public key.
Call `let tbs = builder.finalize(public_key);`
Utilize an API to sign tbs, resulting in signature.
Call `builder.assemble(signature, algo);`

One of the shortcomings of the current API is its assumption that the private key is always available.

By adopting this approach, only the build method would necessitate access to the private key (or signer). Users could directly invoke finalize and assemble.

@baloo
Copy link
Member Author

baloo commented Mar 20, 2024

Here is an example of the use of this API (before this PR merged):
https://github.com/parallaxsecond/rust-cryptoki/pull/192/files#diff-252eb4db79d32a6b9d23737972d71f2fdf640f29003df3b6f84456df92748bb9

Here is an example of the use of this API (after this PR merged):
https://github.com/iqlusioninc/yubikey.rs/pull/554/files#diff-cdf9b5cf6cfb8f48487829e8ca241f6bbdf86b075fea46aa4f660931053dde9f

Both are use-cases where the signer doesn't have direct access to the private material (PKCS11 for use on HSMs, YubiKey in the PIV applet).

For YubiHSMs there are also various implementations of signers:

I'd like to come around and make a compatibility crate for TPMs at some point (in https://github.com/parallaxsecond/rust-tss-esapi) too.

I'd love help on any of those :) But I think that should address your concerns about hardware tokens.

@a1ien
Copy link

a1ien commented Mar 20, 2024

Yeah thanks. This looks really awesome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Change to be merged with next release cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants