Add a PKCS#10 (certification requests) implementation

[npmccallum]

The following are outstanding issues and/or discussion points.

• der crate fails to emit implicit context-specific tags on SET #322

• x509::Extension unable to reproduce DER #321

• I'm not sure about structure naming. I've chosen CertReq. But I think the style of the repo might be the longer CertificationRequest. But this gets pretty long for CertificationRequestDocument. Also, *Info seems to be shortened by this project. So some guidance on the naming would be helpful.

• I'm only testing this with RSA keys. However, I'm just re-using spki::SubjectPublicKeyInfo. So I think this might be okay.

• pkcs10::CertReq is incredibly similar to x509::Certificate. These two could be genericized and then they could share an implementation of signature validation. However, I don't know the right crate to put them in. This could also be a future enhancement.

[npmccallum]

@tarcieri I think this is ready for review now. It contains a commit to work around the derive problem. But this can be reverted once the der crate has an appropriate fix. I also split off #347 and #348 which can be reviewed separately.

[npmccallum]

@tarcieri I have removed the workaround since #322 is now fixed. All tests now pass using derive.

[tarcieri]

Cool, will try to review this weekend

[carl-wallace]

The changes look good. One comment, I would probably favor keeping definitions from an ASN.1 module in one file and use the names from that ASN.1 module for the structs (just to reduce a seam). That's mostly personal preference for a module with only a few definitions like this one though.

[tarcieri]

Suggested change

	#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Sequence)]
	#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Sequence, ValueOrd)]

[npmccallum]

Attempting to do this produces the following error:

error[E0599]: the method `der_cmp` exists for struct `SetOfVec<der::Any<'a>>`, but its trait bounds were not satisfied
   --> pkcs10/src/attribute.rs:14:66
    |
14  |   #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Sequence, ValueOrd)]
    |                                                                    ^^^^^^^^ method cannot be called on `SetOfVec<der::Any<'a>>` due to unsatisfied trait bounds
    |
   ::: /Users/npmccallum/Projects/formats/der/src/asn1/set_of.rs:151:1
    |
151 | / pub struct SetOfVec<T>
152 | | where
153 | |     T: Clone + DerOrd,
154 | | {
155 | |     inner: Vec<T>,
156 | | }
    | | -
    | | |
    | |_doesn't satisfy `SetOfVec<der::Any<'_>>: DerOrd`
    |   doesn't satisfy `SetOfVec<der::Any<'_>>: ValueOrd`
    |
    = note: the following trait bounds were not satisfied:
            `SetOfVec<der::Any<'_>>: ValueOrd`
            which is required by `SetOfVec<der::Any<'_>>: DerOrd`
    = note: this error originates in the derive macro `ValueOrd` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0599`.
error: could not compile `pkcs10` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

[tarcieri]

Aah, the problem here is the der::asn1::SetOfVec type itself is missing a ValueOrd impl. Once that's added it will work.

[npmccallum]

@tarcieri I just tried that. The derive macro doesn't seem to work. And I'm not sure what this trait actually does. So implementing it manually is a bit confusing.

[npmccallum]

@tarcieri The latest push has a manual impl for this. I hope I've done it right. Please take a look.

[tarcieri]

#362 adds the necessary impls to the der crate

[npmccallum]

@tarcieri Rebased.

[tarcieri]

As a bit of a sidebar, it'd be nice if we could find somewhere more central to define Attribute, possibly in the spki crate.

It's fine to put it here for now though.

[npmccallum]

@tarcieri I agree. I was hoping to get this merged and then have a conversation about how to structure the type definitions.

Basically, we have the attribute types, including Attribute, Attributes, AttributeType, AttributeValue, and AttributeTypeAndValue (the last one is currently in the x509 crate). We also have the X.501 name types along with common name definitions (currently in the x509 crate). We also have the Extension type along with common extensions (in the x509 crate). We also have the spki crate with its types.

All of these types are shared across different document types and could live in a shared crate (x509-core?).

[carl-wallace]

They are not all the same so will need to be careful. I agree we could collapse where possible (but might want to leave a pointer in module that is losing a definition).

[npmccallum]

@carl-wallace They are actually the same. The history of these types is that early definitions used advanced features of ASN.1 to constrain the types. But these features turned out to be too difficult to implement in the actual ASN.1 parsers. So over time they relaxed the constraints. We actually implement the later, unconstrained, version as defined by RFC 5280:

Attribute               ::= SEQUENCE {
      type             AttributeType,
      values    SET OF AttributeValue }
            -- at least one value is required

AttributeType           ::= OBJECT IDENTIFIER

AttributeValue          ::= ANY -- DEFINED BY AttributeType

AttributeTypeAndValue   ::= SEQUENCE {
        type    AttributeType,
        value   AttributeValue }

This is the version that should be implemented everywhere.

[carl-wallace]

You have the history backwards, they were originally defined without the constraints (in 1988 ASN.1 syntax) then later updated (to the 2002 syntax mostly), however, the bits on the wire did not change in that exchange.

My point is different but your snip illustrates it. The Attribute definition in PCKS10 features a values field with a SET OF type. The AttributeTypeAndValue definition features a value field that is not a SET OF (or at least is not necessarily a SET OF). These are different types. Sometimes these definitions are repeated in modules vs. importing, leaving a mess.

If your point is that we could likely collapse all references down to the definitions above, I probably agree with you but would want to check source modules before each referencing. I mostly favor organizing by ASN.1 module so that the imports flow in the same way as the specs (to reduce confusion). It's probably too late in the game for that though. It's not a big deal either way, we just need to make sure the types are as they need to be.

[npmccallum]

@carl-wallace I don't think anyone here is confusing the Attribute type with the AttributeTypeAndValue type.

[tarcieri]

@npmccallum you'll want to rebase to pick up #359 at the very least, which will break the build given the current code

[npmccallum]

The changes look good. One comment, I would probably favor keeping definitions from an ASN.1 module in one file and use the names from that ASN.1 module for the structs (just to reduce a seam). That's mostly personal preference for a module with only a few definitions like this one though.

I'm not sure what you mean here. Could you give me a concrete example?

[carl-wallace]

RE: concrete example regarding ASN.1 modules, for the emerging CMS support, I collected all definitions from the RFC5652 module into one file (cryptographic_message_syntax2004.rs). I did the same for the RFC5914 support (trust_anchor_format.rs).

[npmccallum]

@carl-wallace I think it is a really bad idea to structure the code to match the structure of the RFC. Implementations have different constraints than standardization docs. And ASN.1 structuring makes for bad code in almost any language.

[npmccallum]

@carl-wallace I added a comment about the differing versions of the Attribute type to the code. This should help clarify this ambiguity for future readers.

[carl-wallace]

FWIW, asn1 compilers emit code as I described and it leaves no seam when orienting oneself to struct definitions once the specs are understood. We have different opinions on structuring and we can leave it there (though I don't have the foggiest idea why grouping like definitions would be bad in any language).

[npmccallum]

FWIW, asn1 compilers emit code as I described and it leaves no seam when orienting oneself to struct definitions once the specs are understood. We have different opinions on structuring and we can leave it there (though I don't have the foggiest idea why grouping like definitions would be bad in any language).

I'm not suggesting that we can't appropriately structure (or group) the definitions. Only that the code shouldn't slavishly follow the grouping found in the RFC.

ASN.1 compilers differ widely in functionality. Many of them cannot even successfully compile all the features used in a given ASN.1 module. Likewise, these are old specifications with widely divergent quality. Many of the early specifications were deemed unimplementable by later committees. Many specifications contain ASN.1 types that are never actually used anywhere; or are redefined in later specifications.

But we don't have an ASN.1 compiler. We are manually implementing these types. Putting all the type definitions into a single file creates a huge mess of a file. This is even worse when you have to mix trait implementations in the same file. Splitting the trait implementations into separate files from their structure definitions likewise has its own drawbacks.

In short, I think a practical implementation defines only the latest version of types in their own files, together with their trait implementations. Likewise, it doesn't attempt to implement everything but implements only the minimum required for basic functionality. This follows the "release early, release often" mantra.

[carl-wallace]

To your enumeration of our imperfect world I'd add "splitting files into one structure per file has its own drawbacks". We do our best and carry on.

[tarcieri]

Going to go ahead and merge this so we have the initial crate structure and boilerplate to submit PRs against.

The remaining open issues in the description can be addressed as separate PRs.