CMS EnvelopedData

[bkstein]

This PR brings a builder for CMS EnvelopedData and a RecipientInfo trait. Only KeyTransRecipientInfo with RSA was implemented for key encryption, but the code is prepared for beeing extended to all other RecipientInfos and arbitrary encryption algorithms. Volunteers are welcome 🙂.

Open issues:

• Encryption is limited to what is supported by the builder(s). This should be more flexible, e.g. by requiring the user to implement the encrypt() function.

[tarcieri]

@bkstein being able to pass in an RNG is nice for no_std targets (or more specifically targets where getrandom doesn't build)

One option would be to add a fn build_with_rng and gate a provided fn build which uses OsRng on a getrandom feature.

[bkstein]

@tarcieri An fn build_with_rng should be possible. We already have a corresponding function for the x509 builder, so that would fit nicely.

[bkstein]

@tarcireri As for the failing test: I use impl TryFrom<String> for PrintableString which was implemented by turbocool3er last month, but is not released, yet. How do I handle this gracefully?

[tarcieri]

I can cut a release, then you can bump the minor version requirement

[carl-wallace]

This looks like a good start. Two thoughts as it moves forward. As with my suggestion (earlier today) re: signer identifiers, it may be nice to interrogate the key encryption info for the recipient identifier. Similarly, it may be good to define a few traits so algorithm and padding details don't live in the builder object. This will likely matter more as key agreement and KEM support is added but at present OAEP would require a new builder implementation.

[bkstein]

I can cut a release, then you can bump the minor version requirement

@tarcieri I'm not sure, if you saw my 👍? A new release were nice. However, I'm working on the rng feature and I hope, I can push it by the end of this week.

[bkstein]

... As with my suggestion (earlier today) re: signer identifiers, it may be nice to interrogate the key encryption info for the recipient identifier. Similarly, it may be good to define a few traits so algorithm and padding details don't live in the builder object. This will likely matter more as key agreement and KEM support is added but at present OAEP would require a new builder implementation.

@carl-wallace you address a point, that I would like to improve, too. I also don't like my hardcoded implementation of the encryption. I searched for a way how we could leave the choice for encryption algorithms to the user of this crate. My first idea was to pass an encryptor, but there is no common trait for that. What, if the builder accepted a

pub trait Encryptor {
    fn encrypt(data: &[u8]) -> Result<Vec<u8>>;
}

which must be implemented by the user? This would keep all encryption details outside the builder.

[carl-wallace]

That could work. I wonder if a companion to the signature crate is really what's needed though (maybe as a longer term goal).

[tarcieri]

We do have traits like this in the rsa crate:

• https://docs.rs/rsa/latest/rsa/traits/trait.RandomizedEncryptor.html
• https://docs.rs/rsa/latest/rsa/traits/trait.RandomizedDecryptor.html
• https://docs.rs/rsa/latest/rsa/traits/trait.Decryptor.html

[tarcieri]

@bkstein der v0.7.7 released

[bkstein]

I made rng an input parameter for key and content encryption. This will probably become obsolete anyway, when and if encryption details are moved out of the builders.
I also finished the build_pkcs7_scep_pkcsreq test, which is quite long now for a test. But it demonstrates, what has to be done to generate and read a SCEP pkiMessage.

[tarcieri]

@bkstein if you think the PR is ready for review, can you remove draft?

[bkstein]

@tarcieri I'd like to test the implementation in our project. Just to make sure, I didn't miss something. This shouldn't take too long. I'll remove the draft then.

[carl-wallace]

FWIW, I should be able to give the EnvelopedData some exercise on a project over the next week or two (as part of SCEP execution).

@bkstein I am curious if you have tried SignedData with a SignerInfo that used SKID for identifying the signer (the openssl test cases look to use issuer/serial). In some interop work today I ran into an issue and it looks like

    #[asn1(context_specific = "0", tag_mode = "EXPLICIT")]
    SubjectKeyIdentifier(SubjectKeyIdentifier),

should have been

    #[asn1(context_specific = "0", tag_mode = "IMPLICIT")]
    SubjectKeyIdentifier(SubjectKeyIdentifier),

[bkstein]

@carl-wallace I'm also integrating the PR in our SCEP application to see, if it really works. But I think, you found an error: according to the ASN.1 specification in RFC 5652 § 12.1 all SubjectKeyIdentifiers should be IMPLICIT (by default):

12.1.  CMS ASN.1 Module

   CryptographicMessageSyntax2004
     { iso(1) member-body(2) us(840) rsadsi(113549)
       pkcs(1) pkcs-9(9) smime(16) modules(0) cms-2004(24) }

   DEFINITIONS IMPLICIT TAGS ::=
   BEGIN
   ...

[carl-wallace]

Re: SCEP, I had success this morning but had to make one change relative to the test code. The message type attribute should be a PrintableString (see table 1 in section 3.2.1 in RFC8894). The code that worked for me was:

    let mut message_type_value: SetOfVec<AttributeValue> = Default::default();
    let id = PrintableString::try_from(String::from("19")).unwrap();
    message_type_value
        .insert(Any::from(&id))
        .unwrap();
    let message_type = Attribute {
        oid: RFC8894_ID_MESSAGE_TYPE,
        values: message_type_value,
    };

This change was in addition to the change noted earlier relative to key identifier variant of SignerIdentifier. Oddly, RecipientIdentifier was fine.

This was part of an implementation of a client for Apple's Over-the-Air profile delivery and configuration protocol to provision a yubikey (with a custom attribute included in the SCEP request to convey an attestation to the CA). I exercised both the SignedData and EnvelopedData builders (as well as some of the untested code in the yubikey crate along with one or two additions there). Those worked well. I have some cleanup to do before contemplating PRs.

[baloo]

Just a heads up, I might want to publish a release of CMS by the end of this week (to pull the SignedData builder more easily (thank you for doing that!!)).
Let me know if can do anything to help with the review of this one and get it in the same train too :)

I guess you'd need a const-oid 0.9.4 release with backports of the RFC8894_ID_* objects, correct?

[carl-wallace]

Re: CMS release, please make sure to include the tagging change for SignerIdentifier that's in this PR.

[bkstein]

Re: CMS, we probably should remove setting the signing time attribute first. Or move creation of this attribute into an own method for convenience, if someone should need it. I'm still busy with replacing our rust-openssl implementation with RustCrypto cms. Thus not sure, if I will be able to do it until Friday. But I can try it.

[bkstein]

I guess you'd need a const-oid 0.9.4 release with backports of the RFC8894_ID_* objects, correct?

@baloo Correct. I currently have to use my own defnitions, until the RFC8894_ID_* are available.

[baloo]

Re: CMS release, please make sure to include the tagging change for SignerIdentifier that's in this PR.

Good point, I completely missed that. @bkstein do you want to send a separate PR for 29ac637 (I can do it, but this will strip authorship in the final commit, up to you).

[baloo]

@bkstein https://crates.io/crates/const-oid/0.9.4

[bkstein]

@baloo I created #1148 with removed signing time attribute creation.

[baloo]

Curious, which RFC defines that as u32? all I could find was RFC5280:

MAX indicates that the upper bound is unspecified.
   Implementations are free to choose an upper bound that suits their
   environment.

Is there someone with more than 255 certificates in a chain?!?

@bkstein did you remove the comment? Or am I crazy?

[bkstein]

@baloo I'm sorry for the confusion. Yes, I deleted the comment. I think, you are right. I read about the meaning of MAX after I posted the comment and hoped to get away with it unnoticed. Probably not good style to delete comments, especially, when I mentioned someone. I was mislead by rust-openssl, which defined the pathlen as u32. I'm currently replacing our rust-openssl usage by RustCrypto. That's where I met this type mismatch. But I would also guess, that a u8 will be sufficient.

[bkstein]

Empty EncapsulatedContentInfos are now allowed. This is required for degenerate certificate-only CMS messages.

[tarcieri]

@bkstein needs rustfmt