x509-cert: builder updates #1001

lumag · 2023-04-16T10:23:42Z

I started rebasing my code to use CertificateBuilder and I had to make several useful modifications.

Note: It is still not possible to create RSA PSS certificates, since there is no Signer implementation for RSA PSS, only RandomizedSigner (there is no standard detereministic RSA PSS signature process). Locally I have changed the CertificateBuilder to accept RandomizedSigner instead of bare Signer, however this doesn't look like an universal solution too.

lumag · 2023-04-16T10:24:28Z

cc @baloo @tarcieri

lumag · 2023-04-16T11:59:12Z

@baloo another question: would it be useful to accept PK: EncodePublicKey instead of SPKIOwned?

baloo · 2023-04-17T17:00:33Z

x509-cert/src/builder.rs

 {
 /// Creates a new certificate builder
 pub fn new<Signature>(
 profile: Profile,
- version: Version,


good call. Thank you!

baloo · 2023-04-17T17:06:04Z

x509-cert/src/builder.rs

- .to_extension(tbs)?,
- );
+ extensions
+ .push(KeyUsage(KeyUsages::KeyCertSign | KeyUsages::CRLSign).to_extension(tbs)?);


KeyUsages::DigitalSignature is getting dropped, I don't remember why I added it in the first place.

@baloo it is reasoned in the commit message. Please review by the commit.

tarcieri · 2023-04-17T17:17:28Z

It is still not possible to create RSA PSS certificates, since there is no Signer implementation for RSA PSS, only RandomizedSigner (there is no standard detereministic RSA PSS signature process). Locally I have changed the CertificateBuilder to accept RandomizedSigner instead of bare Signer, however this doesn't look like an universal solution too.

A couple potential options here:

Move the signer out of CertificateBuilder::new (which would get rid of the lifetime on the builder), replacing the build method with sign and sign_with_rng methods, which could accept either a Signer or RandomizedSigner
Add a getrandom feature to rsa and use it to gate a Signer impl on pss::SigningKey which uses OsRng for randomness

lumag · 2023-04-17T17:22:35Z

@tarcieri I think the option 1 sounds better. In fact I think there is option 3: switch to SignerMut and try wrapping RadomizedSigner in the SignerMut, but I think it is worse than 1.

tarcieri · 2023-04-17T17:26:01Z

I think we could also potentially do both. Having a Signer impl on PSS seems nice when OsRng is available.

baloo · 2023-04-17T17:32:17Z

@baloo another question: would it be useful to accept PK: EncodePublicKey instead of SPKIOwned?

That would save a round trip to der serialization, and it's more consistent with the signer. Up to you.

baloo · 2023-04-17T17:32:54Z

(the rest of the changes are okay with me, I've pulled the changes over to yubikey.rs to see and it works as expected)
iqlusioninc/yubikey.rs#495

Without this, x509-cert::builder::Error can not be used in the pattern of Box<dyn Error>, which is pretty common. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

…keys Per RFC5280, DigitalSignature 'is asserted when the subject public key is used for verifying digital signatures, other than signatures on certificates (bit 5) and CRLs (bit 6)'. Using CA keys to sign random data would definitely be a bad practice and should be avoided. Thus remove the DigitalSignature keyUsage from these certificates. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

RSA PSS implements DynSignatureAlgorithmIdentifier only for the SigningKey, not for the verifying key. To allow using CertificateBuilder with RSA PSS keys require DynSignatureAlgorithmIdentifier implementation on S rather than on S::VerifyingKey. This also follows the following logic: verifying key can possibly verify several kinds of signatures, while for the signing key we must know exact signature kind and parameters. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Signer (unlike SignerMut) is not expected to be mutable. Don't require mutability of the Signer argument. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

ECDSA keys can not be used for keyEncipherment. Make this keyUsage bit optional. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Add Eq to the list of derived traits to make clippy happy. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

There are no dependencies on the "derive" featue of the signature crate. Drop it now. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

…rsion Follow the rules from RFC 5280 Section 4.1.2.1 to set the certificate's version depending on the presence of the extensions and identifiers. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Remove unused conversion when building RDN fields. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

lumag · 2023-04-18T00:12:46Z

After rereading RFC5280 I found that I was incorrect regarding the TbsCertificate.signature field. Please excuse me.

tarcieri · 2023-04-18T00:13:26Z

@lumag can you rebase?

lumag · 2023-04-18T00:15:03Z

@tarcieri Yes. I was going to push several additional changes, but probably I'd better move them to a separate PR as this one was reviewed by @baloo .

lumag · 2023-04-18T00:16:37Z

done

tarcieri · 2023-04-18T00:17:39Z

FWIW, this should allow RSASSA-PSS with the Signer trait: RustCrypto/RSA#297

lumag · 2023-04-18T00:18:36Z

Yes, I saw that, thank you. I'm going to push a chane adding RadnomizedSigner support, but let's do that in a separate PR.

Added - Certificate builder ([RustCrypto#764]) - Support for `RandomizedSigner` in builder ([RustCrypto#1007]) - Provide parsing profiles ([RustCrypto#987]) - Support for `Time::INFINITY` ([RustCrypto#1024]) - Conversion from `std::net::IpAddr` ([RustCrypto#1035]) - `CertReq` builder ([RustCrypto#1034]) Changed - use `ErrorKind::Value` for overlength serial ([RustCrypto#988]) - Bump `hex-literal` to v0.4.1 ([RustCrypto#999]) - Builder updates ([RustCrypto#1001]) - better debug info when `zlint` isn't installed ([RustCrypto#1018]) - make SKI optional in leaf certificate ([RustCrypto#1028]) - bump rsa from 0.9.0-pre.2 to 0.9.0 ([RustCrypto#1033]) Fixed - fix `KeyUsage` bit tests ([RustCrypto#993]) - extraneous PhantomData in `TbsCertificate` ([RustCrypto#1019])

Added - Certificate builder ([RustCrypto#764]) - Support for `RandomizedSigner` in builder ([RustCrypto#1007]) - Provide parsing profiles ([RustCrypto#987]) - Support for `Time::INFINITY` ([RustCrypto#1024]) - Conversion from `std::net::IpAddr` ([RustCrypto#1035]) - `CertReq` builder ([RustCrypto#1034]) Changed - use `ErrorKind::Value` for overlength serial ([RustCrypto#988]) - Bump `hex-literal` to v0.4.1 ([RustCrypto#999]) - Builder updates ([RustCrypto#1001]) - better debug info when `zlint` isn't installed ([RustCrypto#1018]) - make SKI optional in leaf certificate ([RustCrypto#1028]) - bump rsa from 0.9.0-pre.2 to 0.9.0 ([RustCrypto#1033]) Fixed - fix `KeyUsage` bit tests ([RustCrypto#993]) - extraneous PhantomData in `TbsCertificate` ([RustCrypto#1017])

Added - Certificate builder (RustCrypto#764) - Support for `RandomizedSigner` in builder (RustCrypto#1007) - Provide parsing profiles (RustCrypto#987) - Support for `Time::INFINITY` (RustCrypto#1024) - Conversion from `std::net::IpAddr` (RustCrypto#1035) - `CertReq` builder (RustCrypto#1034) - missing extension implementations (RustCrypto#1050) - notes about `UTCTime` range being 1970-2049 (RustCrypto#1052) Changed - use `ErrorKind::Value` for overlength serial (RustCrypto#988) - Bump `hex-literal` to v0.4.1 (RustCrypto#999) - Builder updates (RustCrypto#1001) - better debug info when `zlint` isn't installed (RustCrypto#1018) - make SKI optional in leaf certificate (RustCrypto#1028) - bump rsa from 0.9.0-pre.2 to 0.9.0 (RustCrypto#1033) - bump rsa from 0.9.1 to 0.9.2 (RustCrypto#1056) Fixed - fix `KeyUsage` bit tests (RustCrypto#993) - extraneous PhantomData in `TbsCertificate` (RustCrypto#1017) - CI flakiness (RustCrypto#1042) - usage of ecdsa signer (RustCrypto#1043)

Added - Certificate builder (RustCrypto#764) - Support for `RandomizedSigner` in builder (RustCrypto#1007) - Provide parsing profiles (RustCrypto#987) - Support for `Time::INFINITY` (RustCrypto#1024) - Conversion from `std::net::IpAddr` (RustCrypto#1035) - `CertReq` builder (RustCrypto#1034) - missing extension implementations (RustCrypto#1050) - notes about `UTCTime` range being 1970-2049 (RustCrypto#1052) - consume the `SignatureBitStringEncoding` trait (RustCrypto#1048) Changed - use `ErrorKind::Value` for overlength serial (RustCrypto#988) - Bump `hex-literal` to v0.4.1 (RustCrypto#999) - Builder updates (RustCrypto#1001) - better debug info when `zlint` isn't installed (RustCrypto#1018) - make SKI optional in leaf certificate (RustCrypto#1028) - bump rsa from 0.9.0-pre.2 to 0.9.0 (RustCrypto#1033) - bump rsa from 0.9.1 to 0.9.2 (RustCrypto#1056) Fixed - fix `KeyUsage` bit tests (RustCrypto#993) - extraneous PhantomData in `TbsCertificate` (RustCrypto#1017) - CI flakiness (RustCrypto#1042) - usage of ecdsa signer (RustCrypto#1043)

Added - Certificate builder (#764) - Support for `RandomizedSigner` in builder (#1007) - Provide parsing profiles (#987) - Support for `Time::INFINITY` (#1024) - Conversion from `std::net::IpAddr` (#1035) - `CertReq` builder (#1034) - missing extension implementations (#1050) - notes about `UTCTime` range being 1970-2049 (#1052) - consume the `SignatureBitStringEncoding` trait (#1048) Changed - use `ErrorKind::Value` for overlength serial (#988) - Bump `hex-literal` to v0.4.1 (#999) - Builder updates (#1001) - better debug info when `zlint` isn't installed (#1018) - make SKI optional in leaf certificate (#1028) - bump rsa from 0.9.0-pre.2 to 0.9.0 (#1033) - bump rsa from 0.9.1 to 0.9.2 (#1056) Fixed - fix `KeyUsage` bit tests (#993) - extraneous PhantomData in `TbsCertificate` (#1017) - CI flakiness (#1042) - usage of ecdsa signer (#1043)

lumag force-pushed the builder-upd branch 3 times, most recently from 3346519 to e96fd0f Compare April 16, 2023 11:30

lumag force-pushed the builder-upd branch 3 times, most recently from 7a94229 to a858334 Compare April 16, 2023 12:38

baloo reviewed Apr 17, 2023

View reviewed changes

This comment was marked as resolved.

Sign in to view

lumag added 9 commits April 18, 2023 03:02

x509-cert: builder: implement std::error::Error for Error

9d733ea

Without this, x509-cert::builder::Error can not be used in the pattern of Box<dyn Error>, which is pretty common. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert: builder: don't require mutability of the signer

26b0c58

Signer (unlike SignerMut) is not expected to be mutable. Don't require mutability of the Signer argument. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert: builder: make keyEncipherment usage optional

da6cd22

ECDSA keys can not be used for keyEncipherment. Make this keyUsage bit optional. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert: test-support/zlint: implement Eq in addition to PartialEq

77536b3

Add Eq to the list of derived traits to make clippy happy. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert: don't require signature's 'derive' feature

6d6160b

There are no dependencies on the "derive" featue of the signature crate. Drop it now. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert: builder: follow rules from RFC5280 to set certificate's ve…

0032002

…rsion Follow the rules from RFC 5280 Section 4.1.2.1 to set the certificate's version depending on the presence of the extensions and identifiers. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

x509-cert/tests/builder: remove extra to_der/from_der conversion

e560912

Remove unused conversion when building RDN fields. Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

lumag force-pushed the builder-upd branch from a858334 to e560912 Compare April 18, 2023 00:16

tarcieri merged commit 8c29e12 into RustCrypto:master Apr 18, 2023

tarcieri changed the title ~~small builder updates~~ x509-cert: builder updates Apr 18, 2023

lumag deleted the builder-upd branch April 18, 2023 00:56

baloo mentioned this pull request May 3, 2023

x509-cert: v0.2.2 #1040

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x509-cert: builder updates #1001

x509-cert: builder updates #1001

lumag commented Apr 16, 2023

lumag commented Apr 16, 2023

lumag commented Apr 16, 2023

baloo Apr 17, 2023

baloo Apr 17, 2023

lumag Apr 17, 2023

tarcieri commented Apr 17, 2023

lumag commented Apr 17, 2023

tarcieri commented Apr 17, 2023

baloo commented Apr 17, 2023

baloo commented Apr 17, 2023 •

edited

Loading

This comment was marked as resolved.

This comment was marked as resolved.

lumag commented Apr 18, 2023

tarcieri commented Apr 18, 2023

lumag commented Apr 18, 2023

lumag commented Apr 18, 2023

tarcieri commented Apr 18, 2023

lumag commented Apr 18, 2023

x509-cert: builder updates #1001

x509-cert: builder updates #1001

Conversation

lumag commented Apr 16, 2023

lumag commented Apr 16, 2023

lumag commented Apr 16, 2023

baloo Apr 17, 2023

Choose a reason for hiding this comment

baloo Apr 17, 2023

Choose a reason for hiding this comment

lumag Apr 17, 2023

Choose a reason for hiding this comment

tarcieri commented Apr 17, 2023

lumag commented Apr 17, 2023

tarcieri commented Apr 17, 2023

baloo commented Apr 17, 2023

baloo commented Apr 17, 2023 • edited Loading

This comment was marked as resolved.

This comment was marked as resolved.

lumag commented Apr 18, 2023

tarcieri commented Apr 18, 2023

lumag commented Apr 18, 2023

lumag commented Apr 18, 2023

tarcieri commented Apr 18, 2023

lumag commented Apr 18, 2023

baloo commented Apr 17, 2023 •

edited

Loading