Derive Sequence for more types

[npmccallum]

This reduces the amount of custom code that is written.

Signed-off-by: Nathaniel McCallum [email protected]

[tarcieri]

The spki crate is a deep, hard dependency of a number of different crates which don’t currently have any proc macro dependencies.

In cases like that, pulling in custom derive has a somewhat cost in terms of compile times of all of the proc macro dependencies, e.g. syn.

I’d prefer to keep spki proc macro-free at least, and probably pkcs1 as well (pkcs8 is another I’d prefer not to change).

pkcs5 is a little more debatable, but also effectively “done” already.

The others are fine.

[carl-wallace]

Did I miss a change to derive? I am wondering what enables proper function with these manual mods gone. I see 349. Will test that out where those structs are inside context specific. Cert parsing doesn't hit that.

A quick test shows this change breaks that. I'd ask that we table this until the outstanding changes land then delete what manual changes can be deleted (as was always the intent as macros changed). There are test cases in the outstanding bits that out help with this. Specifically, the issues look to be arising from tagged sequences (the Decodable vs DecodeValue issue noted before).

Specifically, when compiling a line like this:

///  nameConstr       \[3\] NameConstraints OPTIONAL,

With a definition like this:

    /// nameConstr       \[3\] NameConstraints OPTIONAL,
    #[asn1(context_specific = "3", tag_mode = "IMPLICIT")]
    pub name_constr: Option<NameConstraints<'a>>,

There is this error:

error[E0277]: the trait bound `Option<pkix_extensions::NameConstraints<'_>>: DecodeValue<'_>` is not satisfied
   --> x509/src/trust_anchor_format.rs:147:39
    |
147 | #[derive(Clone, Debug, Eq, PartialEq, Sequence)]
    |                                       ^^^^^^^^ the trait `DecodeValue<'_>` is not implemented for `Option<pkix_extensions::NameConstraints<'_>>`
    |
note: required by a bound in `der::asn1::ContextSpecific::<T>::decode_implicit`
   --> /Users/cwallace/devel/rust/formats-clean/der/src/asn1/context_specific.rs:71:12
    |
71  |         T: DecodeValue<'a> + Tagged,
    |            ^^^^^^^^^^^^^^^ required by this bound in `der::asn1::ContextSpecific::<T>::decode_implicit`
    = note: this error originates in the derive macro `Sequence` (in Nightly builds, run with -Z macro-backtrace for more info)

It looks like there is still some work to be done on context specific support that the manual edits were working around.

[npmccallum]

@carl-wallace How does a developer run the aforementioned tests? And why aren't they in the CI?

[carl-wallace]

The bulk of the tests are in the certval library (and corresponding pittv3 tool) that let you build and validate certs live, enabling pretty thorough exercise of the encoders and decoders. The PR I submitted earlier this week (then chunked up a day or two ago) needs to clear for those first before certval and pittv3 are pushed. The quick test I did just now was to pull over these changes to the version used by certval/pittv3.

[npmccallum]

@carl-wallace Could you add those tests to this repo so that we can find breakage early?

[carl-wallace]

No, as the encoder/decoder that generates this is in the PR in front of it (and the bulk of the testing is driven by building and validating paths). Why is it vital to delete the manual impls now before those pieces land? That seems like the easiest path.

[npmccallum]

@carl-wallace I don't know which PR you're referring to. Could you provide a link?

[carl-wallace]

There was a large PR that you reviewed a few days ago that you suggested be chunked up. I chunked it up (but then pulled one piece back until these clear). One chunk has been processed. Two are currently outstanding: 356 and 357. Once those are done, a minor housekeeping PR will be pushed (to make cargo doc happier) then repos for certval/pittv3 can be set up (there's a yubikey mod that will follow those as well).

[npmccallum]

@carl-wallace Please note that if you add a hash (#) before the PR numbers, you get links directly to them. This aids readers of your comments. For example, the PRs you are referring to are #356 and #357.

[carl-wallace]

FYI, there are some tests in #356 that should highlight breakage if you introduce your changes there. See the tests for trust_anchor_format. I did not try that specifically but what I did earlier is a close cousin to that action. Note, I think we will hit similar edges as CMS support emerges (I did not go enumerate where I think the issues will be but I think there will be similar cases).

[npmccallum]

@carl-wallace What I'm proposing is that if we can discover and fix these issues in the derive code, it makes all this later development easier. That means that each ASN.1 type that is added should have a test which governs it (either explicitly or implicitly). Otherwise, we have no way to know when things are broken or not.

[carl-wallace]

Is there macro change to enable the tests in #356 to pass? If not, please hold off on removing manual impls from x509 until the macros support tagging those structures.