spki: make `AlgorithmIdentifier` generic around `Params` #769

tarcieri · 2022-11-25T22:10:36Z

NOTE: breaking change.

Previously AlgorithmIdentifier::parameters were always AnyRef. This commit changes them to a generic parameter Params.

An alias AlgorithmIdentifierRef provides a type identical to the original with AnyRef as its parameters, which is used in all of the other crates in this repo.

cc @baloo @lumag

tarcieri · 2022-11-25T22:30:44Z

spki/src/spki.rs

@@ -36,7 +39,7 @@ use der::pem::PemLabel;
 #[derive(Copy, Clone, Debug, Eq, PartialEq)]
 pub struct SubjectPublicKeyInfo<'a> {
    /// X.509 [`AlgorithmIdentifier`] for the public key type
-    pub algorithm: AlgorithmIdentifier<'a>,
+    pub algorithm: AlgorithmIdentifierRef<'a>,


Note that this would be a lot more useful if Params were also added to SubjectPublicKeyInfo, i.e. this were changed to AlgorithmIdentifier<Params>.

It should probably get tackled as part of this PR.

Likewise it should be changed on PrivateKeyInfo in the pkcs8 crate

I think it should be some type bound rather than generic <Params>

After trying to implement generic arguments to SPKI or to Certificate/TBSCertificate, I think that Params might need to be bound with something like Into<AnyRef>, or any other generic enough type, so that there would be no need to add type parameters to these structures. I might be wrong with the SPKI, but handling type parameters for Certificate looks like a complete mess.

I don't follow what you mean by "type bound" if not a generic parameter.

If you need additional bounds you can always specify them. But requiring one would be rather onerous I think, especially as the conversion for e.g. ObjectIdentifier only works for &ObjectIdentifier.

Certificate/TBSCertificate

...those should be using a concrete type?

I'm still playing here. I have the feeling that TBSCertitificate & company should not have generic parameters per se. Otherwise handling them easily becomes a mess. I'm still trying to fit SPKI / SPKIRef properly into my code. Maybe this will flow better once I update RSA crate to return specific SPKI rather than generic SPKIRef.

Yes, they should use a concrete type. The only things I think it makes sense parameterizing are AlgorithmIdentifier, SubjectPublicKeyInfo, and PrivateKeyInfo.

Is there any easy way to perform the Into<AnyRef> or TryInto<AnyRef> conversion? The only way I have at this moment is to encode the AlgorithmIndeifier<Params> into DER format and then reparse it again into AlgorithmIndentifierRef.

tarcieri · 2022-11-25T22:34:05Z

spki/src/algorithm.rs

+impl<'a> AlgorithmIdentifierRef<'a> {
    /// Assert the `algorithm` OID is an expected value.
    pub fn assert_algorithm_oid(&self, expected_oid: ObjectIdentifier) -> Result<ObjectIdentifier> {


I left the inherent methods unchanged for now, however it's now possible to use AlgorithmIdentifier<ObjectIdentifier> which eliminates the need for many of them, since the main thing they provide is type conversions to/from ObjectIdentifier.

The assert_algorithm_oid should be implemented for the generic type, not for the AlgorithmIdentifierRef

tarcieri · 2022-11-25T22:52:44Z

This is a weird failure:

https://github.com/RustCrypto/formats/actions/runs/3551054810/jobs/5964957749

rstest is failing? But Cargo.lock didn't change

lumag · 2022-11-25T23:37:29Z

x509-cert/src/crl.rs

@@ -75,7 +75,7 @@ pub struct RevokedCert<'a> {
 #[allow(missing_docs)]
 pub struct TbsCertList<'a> {
    pub version: Version,
-    pub signature: AlgorithmIdentifier<'a>,
+    pub signature: AlgorithmIdentifierRef<'a>,


This should also allow AlgorithmIdentifier<...> (and in Certificate/CertificateList) too.
I can not come with the good enough argument at this moment.

lumag · 2022-11-26T00:23:05Z

@tarcieri please consider also merging https://pastebin.com/fNszVrjA

I tried generalizing SubjectPublicKeyInfo, but got stuck.

tarcieri · 2022-11-26T02:13:49Z

@tarcieri please consider also merging https://pastebin.com/fNszVrjA

I think some of that looks good but would rather hold off on doing those sorts of updates in follow-up PRs. Maybe you can PR them separately after this is merged?

This PR isn't done yet and it's going to get more complex with changes to SubjectPublicKeyInfo and PrivateKeyInfo. And continuing to use AnyRef as Params should still be fine for now.

baloo · 2022-11-26T06:12:47Z

This is a weird failure:

https://github.com/RustCrypto/formats/actions/runs/3551054810/jobs/5964957749

rstest is failing? But Cargo.lock didn't change

#767 (comment)

This is a regression on nightly.

Temporary fix here:
RustCrypto/actions#20
demo:
https://github.com/RustCrypto/formats/actions/runs/3552592187/jobs/5967598693
#770

lumag · 2022-11-26T09:56:29Z

@tarcieri sure, I can update RsaPssParams afterwards. The last chunk (regarding assert_algorithm_oid) can probably be a part of this PR.
I'm looking forward to seeing how you'd update SPKI/PrivateKeyInfo.

tarcieri · 2022-11-26T18:59:01Z

@lumag b09ff11 makes SubjectPublicKeyInfo generic around Params, following much the same pattern as I used for AlgorithmIdentifier

Unfortunately this isn't enough to eliminate the lifetime and allow a fully owned form. That would require making the public key data generic around e.g. a Bits parameter.

baloo · 2022-11-29T05:53:28Z

I've played a bit with the PR.
If I understand correctly the idea would be to get something like:

patch on `x509-cert/src/certificate.rs`

diff --git a/x509-cert/src/certificate.rs b/x509-cert/src/certificate.rs
index ec607eb..28cc2d6 100644
--- a/x509-cert/src/certificate.rs
+++ b/x509-cert/src/certificate.rs
@@ -8,7 +8,7 @@ use core::cmp::Ordering;
 use const_oid::AssociatedOid;
 use der::asn1::BitString;
 use der::{Decode, Enumerated, Error, ErrorKind, Sequence, ValueOrd};
-use spki::{AlgorithmIdentifierRef, SubjectPublicKeyInfoRef};
+use spki::{AlgorithmIdentifier, SubjectPublicKeyInfoRef};

 #[cfg(feature = "pem")]
 use der::pem::PemLabel;
@@ -73,7 +73,7 @@ impl Default for Version {
 /// [RFC 5280 Section 4.1]: https://datatracker.ietf.org/doc/html/rfc5280#section-4.1
 #[derive(Clone, Debug, Eq, PartialEq, Sequence, ValueOrd)]
 #[allow(missing_docs)]
-pub struct TbsCertificate<'a> {
+pub struct TbsCertificate<'a, S, K> {
     /// The certificate version
     ///
     /// Note that this value defaults to Version 1 per the RFC. However,
@@ -84,11 +84,11 @@ pub struct TbsCertificate<'a> {
     pub version: Version,

     pub serial_number: SerialNumber,
-    pub signature: AlgorithmIdentifierRef<'a>,
+    pub signature: AlgorithmIdentifier<S>,
     pub issuer: Name,
     pub validity: Validity,
     pub subject: Name,
-    pub subject_public_key_info: SubjectPublicKeyInfoRef<'a>,
+    pub subject_public_key_info: SubjectPublicKeyInfo<'a, K>,

     #[asn1(context_specific = "1", tag_mode = "IMPLICIT", optional = "true")]
     pub issuer_unique_id: Option<BitString>,
@@ -100,7 +100,7 @@ pub struct TbsCertificate<'a> {
     pub extensions: Option<crate::ext::Extensions>,
 }

-impl<'a> TbsCertificate<'a> {
+impl<'a, S, K> TbsCertificate<'a, S, K> {
     /// Decodes a single extension
     ///
     /// Returns an error if multiple of these extensions is present. Returns
@@ -147,15 +147,15 @@ impl<'a> TbsCertificate<'a> {
 /// [RFC 5280 Section 4.1]: https://datatracker.ietf.org/doc/html/rfc5280#section-4.1
 #[derive(Clone, Debug, Eq, PartialEq, Sequence, ValueOrd)]
 #[allow(missing_docs)]
-pub struct Certificate<'a> {
-    pub tbs_certificate: TbsCertificate<'a>,
-    pub signature_algorithm: AlgorithmIdentifierRef<'a>,
+pub struct Certificate<'a, S, K> {
+    pub tbs_certificate: TbsCertificate<'a, S, K>,
+    pub signature_algorithm: AlgorithmIdentifier<S>,
     pub signature: BitString,
 }

 #[cfg(feature = "pem")]
 #[cfg_attr(docsrs, doc(cfg(feature = "pem")))]
-impl PemLabel for Certificate<'_> {
+impl PemLabel for Certificate<'_, _, _> {
     const PEM_LABEL: &'static str = "CERTIFICATE";
 }

@@ -170,4 +170,4 @@ impl PemLabel for Certificate<'_> {
 /// ```
 ///
 /// [RFC 6066]: https://datatracker.ietf.org/doc/html/rfc6066#section-10.1
-pub type PkiPath<'a> = Vec<Certificate<'a>>;
+pub type PkiPath<'a, S, K> = Vec<Certificate<'a, S, K>>;

I'm not sure the Vec<Certificate<'a, S, K>> would make much sense as there is no obligation for a verification path to use the same signature or same key algorithm?

tarcieri · 2022-11-29T17:44:03Z

@baloo I don't think it makes sense to parameterize Certificate around generic Params.

Instead they can use owned/allocating Any now, and runtime dispatch based on the provided AlgorithmIdentifier. That moves us one step closer to owned types.

baloo · 2022-11-29T17:55:51Z

Ha, right, I missed the intention of the Params completely.

tarcieri · 2022-11-29T18:34:05Z

I think it allows for a number of cross-cutting concerns, including using a concrete type like ObjectIdentifier when it makes sense.

We're also trying to solve the problem of having AlgorithmIdentifier constants where the Params are e.g. a SEQUENCE.

Previously `AlgorithmIdentifier::parameters` were always `AnyRef`. This commit changes them to a generic parameter `Params`. An alias `AlgorithmIdentifierRef` provides a type identical to the original with `AnyRef` as its `parameters`, which is used in all of the other crates in this repo.

tarcieri marked this pull request as draft November 25, 2022 22:11

tarcieri force-pushed the spki/generic-algorithm-identifier branch from efc5b1a to c98cbee Compare November 25, 2022 22:27

tarcieri commented Nov 25, 2022

View reviewed changes

tarcieri force-pushed the spki/generic-algorithm-identifier branch 2 times, most recently from b5c715d to 630514d Compare November 25, 2022 22:52

lumag reviewed Nov 25, 2022

View reviewed changes

tarcieri force-pushed the spki/generic-algorithm-identifier branch from ffb9092 to b09ff11 Compare November 26, 2022 19:09

baloo mentioned this pull request Nov 28, 2022

x509-cert: wip: owned api #776

Closed

baloo mentioned this pull request Nov 29, 2022

spki: make SubjectPublicKeyInfo own the public key #778

Closed

tarcieri force-pushed the spki/generic-algorithm-identifier branch from b09ff11 to d6741cb Compare December 11, 2022 15:03

tarcieri changed the title ~~[WIP] spki: make AlgorithmIdentifier generic around Params~~ spki: make AlgorithmIdentifier generic around Params Dec 11, 2022

tarcieri marked this pull request as ready for review December 11, 2022 16:08

tarcieri merged commit 855f787 into master Dec 11, 2022

tarcieri deleted the spki/generic-algorithm-identifier branch December 11, 2022 16:09

tarcieri mentioned this pull request Dec 18, 2022

der: Any vs AnyRef: issues with generating AlgorithmIdentifier's parameters on the fly #731

Closed

tarcieri mentioned this pull request Feb 27, 2023

spki v0.7.0 #900

Merged

baloo mentioned this pull request May 23, 2024

der: introduce an AnyLike trait to mark parameters #1415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spki: make `AlgorithmIdentifier` generic around `Params` #769

spki: make `AlgorithmIdentifier` generic around `Params` #769

tarcieri commented Nov 25, 2022

tarcieri Nov 25, 2022

tarcieri Nov 25, 2022

lumag Nov 25, 2022

lumag Nov 26, 2022

tarcieri Nov 26, 2022 •

edited

Loading

lumag Nov 27, 2022

tarcieri Nov 27, 2022

lumag Nov 27, 2022 •

edited

Loading

tarcieri Nov 25, 2022

lumag Nov 25, 2022

tarcieri commented Nov 25, 2022

lumag Nov 25, 2022

lumag commented Nov 26, 2022

tarcieri commented Nov 26, 2022

baloo commented Nov 26, 2022 •

edited

Loading

lumag commented Nov 26, 2022

tarcieri commented Nov 26, 2022 •

edited

Loading

baloo commented Nov 29, 2022

tarcieri commented Nov 29, 2022

baloo commented Nov 29, 2022

tarcieri commented Nov 29, 2022

spki: make AlgorithmIdentifier generic around Params #769

spki: make AlgorithmIdentifier generic around Params #769

Conversation

tarcieri commented Nov 25, 2022

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

tarcieri Nov 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lumag Nov 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarcieri commented Nov 25, 2022

Choose a reason for hiding this comment

lumag commented Nov 26, 2022

tarcieri commented Nov 26, 2022

baloo commented Nov 26, 2022 • edited Loading

lumag commented Nov 26, 2022

tarcieri commented Nov 26, 2022 • edited Loading

baloo commented Nov 29, 2022

tarcieri commented Nov 29, 2022

baloo commented Nov 29, 2022

tarcieri commented Nov 29, 2022

spki: make `AlgorithmIdentifier` generic around `Params` #769

spki: make `AlgorithmIdentifier` generic around `Params` #769

tarcieri Nov 26, 2022 •

edited

Loading

lumag Nov 27, 2022 •

edited

Loading

baloo commented Nov 26, 2022 •

edited

Loading

tarcieri commented Nov 26, 2022 •

edited

Loading