PKCS#7 signing & verification - Certificate extension policies

[nitneuqr]

As per #12267 and #12104, I'm opening this draft implementation of verification of PKCS#7 (S/MIME) certificates. Tests are not passing yet; still need to build an appropriate certificate for this.

The constraints I've gathered so far:

• Certificates used as end entities (i.e., the cert used to sign a PKCS#7/SMIME message) should not have ca=true in their basic constraints extension.
• EKU_CLIENT_AUTH_OID is not required
• EKU_EMAIL_PROTECTION_OID is required

@alex @woodruffw @prauscher feel free to add some more constraints, I'm not particularly familiar with CA verification.

[nitneuqr]

@alex wondering if there are best practices for certificates: better store it in vectors or dynamically create one during testing with the x509.CertificateBuilder?

[alex]

Depends a bit on the use case. If you're verifying that we handle that certificate itself, it's better in vectors. If you're verifying something else either can work.

[nitneuqr]

OK, I'll go with storing it in vectors then. Mostly doing it for coverage.

[prauscher]

My excerpt from https://www.rfc-editor.org/rfc/rfc8550#section-4.4 would be:

• SHOULD NOT contain basicConstraints (per RFC 5280)
• if key Usage extension is present, MUST allow digitalSignature and/or nonRepudiation (if extensions is not present, this MUST be ignored)
• subjectAltName (if used) MUST be encoded using rfc822Name (for ascii-addresses) or otherName (for non-ascii-addresses)
• if extendedKeyUsage is present, it MUST allow id-kp-emailProtection or anyExtendedKeyUsage

Section 4.4.4 also discusses some limitations regarding certificates which may only be used for signing, but not for encryption - so maybe two separate policies (which would be quite similar) would be required?

[nitneuqr]

Hey, just pushed a first version of the constraints for certificates used as EE for email signing & verification.

Section 4.4.4 also discusses some limitations regarding certificates which may only be used for signing, but not for encryption - so maybe two separate policies (which would be quite similar) would be required?

I'd go for constraints linked to signing & verification for now, and address encryption / decryption later. I'm not sure openssl smime -decrypt checks anything in the certificates used, whereas it is the case for openssl smime -verify.

For the constraints, I have no experience with them, I'll implement what seems right w.r.t. RFC 😄

Also, I've left empty the CA constraints, I'm not sure what to fill as of now.

[prauscher]

Not sure about the CA constraints either, from our company-certificate they include EMAIL_PROTECTION in EKU, but not sure if this is required. From this stackexchange-post I read that its application to ca-certificates is application-specific and not required by RFC. So for this use-case ignoring it in the CA might be the best thing (as long as other verifiers in cryptography do not have preference here).

From my view (but without deep knowledge about cryptography-inner-works, so feel free to correct me) I change a few parts:

• according to https://docs.python.org/3/reference/simple_stmts.html#grammar-token-python-grammar-assert_stmt assert is ignored if __debug__ is False - should it be used for verification or am I missing something?
• _validate_eku should probably allow for ANY_EXTENDED_KEY_USAGE too
• Shouldn't ee_policy treat ExtendedKeyUsage as may_be_present in accordance to RFC 8550?
• Shouldn't ee_policy include a may_be_presend check for a CRITICAL keyUsage, with an implementation of something like assert ku.digital_signature or ku.content_commitment # content_commitment used to be named non repudiation?

[nitneuqr]

Totally agree with you @prauscher, for CA I'd either leave it as a .permit_all() or a .webpki_defaults_ca(). Any opinion on this is welcome 😄

For the changes, feel free to initiate a review, this is mostly draft as it blocks #12267. Good catches for the fixes, modifying the code now.

And I totally agree with the assert part, I've mostly copied what was done in other tests since there is not documentation on using validator callback in ExtensionPolicies yet. I'll change to raising a ValueError for now.

[nitneuqr]

As per [RFC5280], certificates MUST contain a basicConstraints extension in CA certificates and SHOULD NOT contain that extension in end-entity certificates.

Since this is specified in https://www.rfc-editor.org/rfc/rfc8550#section-4.4, I'll go with .webpki_defaults_ca() which seems to be exactly what we need.

[nitneuqr]

First suggestions integrated, missing:

• Testing with inappropriate certificates, for coverage
• Handling the SubjectAlternativeName constraint defined here

[prauscher]

For the subjectAlternativeName constraint we probably need to receive the mail address as a parameter of pkcs7_x509_extension_policies. After reading https://www.rfc-editor.org/rfc/rfc5280#section-4.2.1.6 I'd suggest something along the following:

def pkcs7_x509_extension_policies(mail_address: str):
    [...]
    def _validate_subject_alternative_name(policy, cert, san):
        if san is None:
            raise ValueError("Subject Alternative Name is required")

        if mail_address.split("@")[0].isascii():
            if mail_address.lower() not in [name.lower() for name in san.get_values_for_type(x509.RFC822Name)]:
                raise ValueError(f"Subject Alternative Name does not contain RFC822Name {mail_address}")

        else:
            if mail_address.lower() not in [name.value.lower() for name in san.get_values_for_type(x509.OtherName) if name.type_id == x509.ObjectIdentifier("1.3.6.1.5.5.7.8.9")]:
                raise ValueError(f"Subject Alternative Name does not contain OtherName {mail_address}")

[nitneuqr]

Thanks for the code! I'm unsure about specifying all the time an email address in the extension policy. To me, it seems a bit too restrictive (?). Moreover, it seems that there are things checked already in default EE policies in cryptography:

• https://github.com/pyca/cryptography/blob/main/src/rust/cryptography-x509-verification/src/policy/extension.rs#L125-L133
• https://github.com/pyca/cryptography/blob/main/src/rust/cryptography-x509-verification/src/policy/extension.rs#L262-L279

Due to this, I'll pass to .webpki_defaults_ee().

To me, what we need to check from RFC 8550 is:

• Check all the GeneralNames in SubjectAlternativeName
• For those which are email addresses (contains @?), verify that the general parts stored in rfc822Name are ascii, and those stored in otherName are non-ascii

Also, checked RFC 5750 for S/MIME 3.2 (which we are currently supporting), and the differences are minor (such as the ASCII / non-ASCII differentiation). I suggest we go with RFC 8550 for that matter.

Will push a nearly final version in the following hours 😄

[prauscher]

@nitneuqr Is there anything I could do to support in this case?

[nitneuqr]

Hey; coming back on this subject after a few months on a big project. Thanks for the heads up! Will check what's wrong with my current build, rebase to master and adapt accordingly. Should be done in a few weeks.

[nitneuqr]

It seems I'm missing on coverage as of now. You can check this run for more details.

@prauscher if you have any idea on how make the tests cover the missing lines, I'd love to hear about it!

[prauscher]

Thanks for your work, I'll see what I get. From my point of view, there are 4 cases left to cover, all in pkcs7.py:

1. 88->exit: A certificate with a compliant KeyUsage (with digital signature or content commitment set - by the way, shouldn't this line be more like if not (ku.digital_signature or ku.content_commitment):, mind the parantheses)
2. 111: A certificate with an RFC822-Subject Alt Name with non-ascii-characters in the local part
3. 120: A certificate with an OtherName-Subject Alt Name where the local part is ascii
4. 131->exit: A certificate with extended key usage where neither emailProtection nor anyExtendedKeyUsage is given

I could probably build some test cases tomorrow and post them here?

[nitneuqr]

Thanks to your explanations I understood what the what the -> exit meant in the cases 1 and 4. I'll fix this asap 😄 Also, I've already built certificates to test cases 2 and 3. I've not managed to build them using cryptography yet, so openssl it will be.

I'll push what I've done on Friday evening (Paris time). It should cover all the remaining test cases. I'd love a review at that moment. Therefore you probably don't need to build the cases on tomorrow 😄

And good catch for the parentheses, will also take a look!

[nitneuqr]

@alex @prauscher should be good now! Waiting for a review on your side to integrate when needed :)

[nitneuqr]

@prauscher I've taken into account your comments, should be good! @alex waiting for your review to integrate it :)

[nitneuqr]

@alex @reaperhulk any news on the subject? :)

[alex]

Sorry we've been slow to review. I think the best next step would be to split the test vectors out into their own PR, we generally prefer to review those standalone.

[prauscher]

@alex Not sure if I understood your comment correctly, but #13371 and #13372 are now two separate PRs. Surely, #13372 fails tests now as #13371 must be merged prior. Please feel free to close both If I may have misunderstood your comment.

[briantist]

@prauscher I have not contributed to this project and I also found that comment confusing. After looking a bit more closely, I think the ask was specifically about test vectors as described in the docs/development/test-vectors.rst file.

So I think just the addition of the files under vectors/ and the change to the file above would be submitted separately (first), and once reviewed and merged, the code and tests themselves (together) would be their own PR, which I think could have just used this PR with the vector changes removed (since they would be part of the source branch).

I could definitely be wrong so hopefully @alex will clarify.

[prauscher]

Thanks @briantist - this seems way more likely. I drafted #13376 for this purpose and will close #13371 and #13372 soon. After the merge of #13376 this PR can be rebased and the test vectors should disappear from it.

[prauscher]

@nitneuqr while renaming the test vectors in discussion with @alex there are some changes required to this PR. Can you please apply the attached patch file and push results here? Once #13376 is merged this PR should be rebased and all changes to the vectors should be removed from this PR. 12465-vectors.patch

[nitneuqr]

@nitneuqr while renaming the test vectors in discussion with @alex there are some changes required to this PR. Can you please apply the attached patch file and push results here? Once #13376 is merged this PR should be rebased and all changes to the vectors should be removed from this PR. 12465-vectors.patch

Hey @prauscher , thanks for the help on the subject! It's been a rough couple of weeks for me, but I'm getting back on tracks!

I should be able to rebase the changes from the newly integrated PR early this week. You can close #13404, I'd rather we integrate this PR :)

[nitneuqr]

@alex @briantist should be good for review 😄

[prauscher]

@alex what is your opinion regarding this PR? Anything you'd like changed? :)

[alex]

I'm hoping to be able to make time to reivew this next week. Apologies for the delay.

[prauscher]

@alex sorry to bother, but is there anything we can help you with for the review phase?