Build SignedData CMS (and PKCS #7)

[bkstein]

This branch adds a builder for SignedData PKCS #7 messages.

[tarcieri]

cc @carl-wallace

See also #1045 where we are discussing the future of the pkcs7 crate

[baloo]

Just wondering, would it make sense to reuse the Builder trait from x509-cert, it's a dependency already.

[bkstein]

This would also hold for CertificateBuilder, right?
As for the PKCS #7 builder: how would we handle multiple signers? My current solution is to add a SignerInfo including the signature with each call of SignedDataBuilder::sign(). Idea: I could possibly rename sign() to add_signer() and just add the SignerInfo without the signature. The signatures would then be calculated when building the message with SignedDataBuilder::build().

[baloo]

Yeah, I guess you would need an owned structure to be able to use it.

[bkstein]

@baloo I'm currently moving my implementations to the cms crate, which uses owned types. It looks like I will be able to reuse your Builder trait for the SignerInfo objects. 🙂

[bkstein]

cc @carl-wallace

See also #1045 where we are discussing the future of the pkcs7 crate

Yes, I will definitely check the cms crate. But first I'd like to solve my lifetime problem.

[tarcieri]

@bkstein the ContentInfo type in the cms crate is owned and doesn't have a lifetime, which may make it easier to work with:

https://docs.rs/cms/0.2.1/cms/content_info/struct.ContentInfo.html

[bkstein]

@bkstein the ContentInfo type in the cms crate is owned and doesn't have a lifetime, which may make it easier to work with:

https://docs.rs/cms/0.2.1/cms/content_info/struct.ContentInfo.html

I had a look into the cms crate and I wonder, why the structs own their content while the pkcs7structs borrow everything. Is there a special reason for this?
However, I think that building cmss types will be easier. I will try this tomorrow.

[tarcieri]

@bkstein some past discussion here: #765

We ended up moving many of the crates to use all owned types. This makes things like builders easier (as you are experiencing), but also makes one-pass deserialization from PEM possible.

The pkcs7 crate predates that decision. We should maybe consider retiring it and moving everything to cms.

[bkstein]

That sounds reasonable to me, too. The only difference between CMS and PKCS #7 I could find ist the coding of encapsulated content (RFC 5652, 5.2.1) and that should be manageable.

[tarcieri]

I've officially deprecated the pkcs7 crate: #1062

@bkstein do you think you could update this PR to add this same functionality to cms instead?

[bkstein]

Yes, I worked exactly on this during the last week. But I'm not ready, yet.

[bkstein]

I moved the code from crate pkcs7 (which is deprecated) to cms. I used Builder from x509-cert to create the SignerInfo. Builder should probably be moved to a more central location in the future. I also moved der::asn1::set_of::der_sort() to make it public for some other types. The implementation is not yet usable for creating PKCS #7 messages, as enveloped_data must also be created. This will be next.

[tarcieri]

I also moved der::asn1::set_of::der_sort() to make it public for some other types.

Unless I'm missing something, all you're doing is calling it before SetOfVec::try_from.

That's unnecessary, as SetOfVec::try_from will call it for you: https://github.com/RustCrypto/formats/blob/8f480f5/der/src/asn1/set_of.rs#L389

[bkstein]

I also moved der::asn1::set_of::der_sort() to make it public for some other types.

Unless I'm missing something, all you're doing is calling it before SetOfVec::try_from.

That's unnecessary, as SetOfVec::try_from will call it for you: https://github.com/RustCrypto/formats/blob/8f480f5/der/src/asn1/set_of.rs#L389

I added try_from to some types, that are not type aliases of SetOfVec, but aggregate SetOfVec. See e.g. RelativeDistinguishedName. My intention was to re-use der_sort() there, but I forgot to replace the duplicated code. I will fix this tomorrow, if moving der_sort() is OK for you.

[tarcieri]

See e.g. RelativeDistinguishedName

That still looks like a newtype of SetOfVec, which you can build using impl TryFrom<Vec<AttributeTypeAndValue>> for SetOfVec<AttributeTypeAndValue>.

I still don't see why you need der_sort in the public API.

[baloo]

I believe changes in pkcs7 need to be dropped (deprecated).

[bkstein]

Right, I will check pkcs7 for remainders of my changes.

[baloo]

looks great! Thank you for doing that!

[bkstein]

@baloo To make sure, I understand correctly: I make a PR #xyz with the x509-cert change, revert this change in the current PR #1051 and after you merged PR #xyz, you or I will patch this into the cms-and const-oid-only PR #1051?

[bkstein]

@baloo Split PR #1092 was merged by tarcieri. I updated this branch to the new master.

[tarcieri]

@bkstein can you split out the const-oid changes? That will make them a lot easier to cherry-pick in a backport

[bkstein]

Sure, I already expected this 🙂.

[baloo]

We also merged #1071 this weekend which bring back the minimal-versions checks, this is to ensure we specify the minimum required version of each dependency.
This will keep failing until we release all the dependencies you need and we bump the cms/Cargo.toml with those versions here.

[bkstein]

Just realized, that resolving does not automatically accept the suggested changes. 🙄

[baloo]

we'll still need you to send the const-oid changes in a new PR. Once merged we'll backport that to const-oid 0.9 release you can use here.

Sorry for the extra work.

[bkstein]

we'll still need you to send the const-oid changes in a new PR. Once merged we'll backport that to const-oid 0.9 release you can use here.

Sorry for the extra work.

I checked the const-oid changes and found out, that all OIDs are now available with RFC 5911. Thus, we don't need a new const-oid. I will check OID usage in my branch, change the const_oid::db::rfc5652::... to the corresponding const_oid::db::rfc5911::... and remove rfc5652.txt.

[bkstein]

@tarcieri@baloo For the above noted reasons, I reverted the changes in const-oid, so we don't need a new version here.

[baloo]

(lastly, I think we'll want you to squash those commits if you could)

[tarcieri]

@baloo we use squash-and-merge so we don't depend on developers to squash their own PRs

[carl-wallace]

Sorry I missed this request somehow last month (was very busy then and just missed it). I reviewed now in prep of reviewing the EnvelopedData changes and had a couple of comments. None of this is essential (though fixing the test case probably ought be done).

SignerInfoBuilder.assemble

• Ought the signer be interrogated for the SignerIdentifier (as it is for signature algorithm)? Same could apply to yielding a certificate (or certification path) that is consistent with the signer as well. This was not appropriate for a certificate builder but given this is a little higher level it may be worth another trait.
• Should there be a consistency check here re: digest algorithm and signature algorithm?

SignedDataBuilder.build

• Same question re: interrogating SignerInfos for digest algorithms and for consistency check where external is used.

Test comments

• The test_build_signed_data uses SHA256 and SHA512 in two different SignerInfo structures but only lists SHA256 in the SignedData.digestAlgorithms field. Section 5.3 indicates hash algorithms in SignerInfo.digestAlgorithm SHOULD be in the SignedData.digestAlgorithms field.
• SHA512 is inconsistent with ecdsaWithSHA256 as well (which may suggest the consistency check mentioned above is needed).
• May be worth including a certs-only test (which worked fine).

Questions

• Why is the signing time attribute required? This seems like something that ought be optional. RFC6019 defines an alternative signing time, for example. Some protocols (like SCEP) use a nonce. A brief survey of things that reference RFC5652 doesn't show much in the way of requiring signingTime (RFC8551 and RFC8995 intimate it is required but there's no MUST).

Nits

• The word signed is presented in quotes in documentation comments, but unsigned is not. Suggest dropping the quotes.

[bkstein]

@carl-wallace Thanks for reviewing this.

I have some questions/remarks.

SignedDataBuilder.build/assemble

Could you give an example, how you would interrogate the info?

Test comments

• If I understand CMS correctly, digest algorithm and signature algorithm are independent. The digest algorithm is used to hash the message content. The hash value is then signed together with other signed attributes. Please correct me, if I'm wrong.
• A "Degenerate certificates-only CMS SignedData" test is a very good idea. 🙂

Questions

• As for the signing time attribute: I think you are right and it is not required. We should probably leave it out. Also to remove unnecessary dependencies. The builder is currently not no_std and I have to admit, that I didn't have that on the agenda. Understanding RFC 5652 was hard enough.

Nits

• The quotes definitely MUST be dropped.

[carl-wallace]

Re: interrogation, adding a trait bound to S is what I was thinking. At present it is Keypair + DynSignatureAlgorithmIdentifier. Adding some trait that returns a SignerIndentifier here (or RecipientIdentifier for EnvelopedData) might close a gap. Where S is a P12 or a hardware module, this info ought be available. I've no surveyed where this would go but it seems doable.

Re: hash algorithm and signature algorithm, you're right they can move independently, but accumulating the full list in the SignedData later is required (as a SHOULD). The consistency check suggestion is likely too much, just ignore that.

Here's a certs only test case:

#[test]
fn test_build_certs_only_signed_data() {
// Make some content
let content = EncapsulatedContentInfo {
econtent_type: const_oid::db::rfc5911::ID_DATA,
econtent: None,
};
let certificate_buf = include_bytes!("examples/ValidCertificatePathTest1EE.pem");
let certificate = x509_cert::Certificate::from_pem(certificate_buf).unwrap();
let mut builder = SignedDataBuilder::new(&content);
let signed_data_pkcs7 = builder
.add_certificate(CertificateChoices::Certificate(certificate))
.expect("error adding certificate")
.build()
.expect("building signed data failed");
let signed_data_pkcs7_der = signed_data_pkcs7
.to_der()
.expect("conversion of signed data to DER failed.");
println!(
"{}",
pem_rfc7468::encode_string("PKCS7", LineEnding::LF, &signed_data_pkcs7_der)
.expect("PEM encoding of signed data DER failed")
);
}