Feature: Certificate policies

[Gabgobie]

Thanks for offering to review the PR @djc

I expect you will want me to make some changes but wanted to talk about them before blindly trying to anticipate everything.

In #355 was some good justification for avoiding trait impls. Unfortunately I didn't see this until after I was almost done. I assume this concerns both x509_parser and yasna. Would you like me to

1. Remove all trait impls
2. Remove trait impls from public structs
3. Leave them

There are some cases where I felt that there wasn't a fitting error case. I added an Other case as a temporary measure. They could probably be collectively classified as UserError(String) or are entirely unnecessary if it is not the libraries responsibility to enforce valid usage. Once example for these cases would be that any given Certificate Policy OID may only appear once per certificate.

I also added comments about my thoughts and doubts during dev.

You requested some basic tests but I don't really know how I would do that from within the library. Serializing and deserializing the structs I just added doesn't really guarantee conformity and compatibility with external tools that are going to work with the output. So far my test was the newly added example and inspection of the output with openssl.

Adds the following extensions

This PR handles two of the extensions listed/requested in #370

• 4.2.1.4. Certificate Policies
• 4.2.1.14. Inhibit anyPolicy

In the following I just pasted the notes I took while I was trying to implement the extensions.

Contributor notes

This is a temporary place to collect questions and notes until I open the PR. This file should not become part of the repo.

From<x509_parser::extension::Ext> Trait impls

#355 contains valid reasoning for not implementing From<x509_parser::extensions::ExtensionCounterpart<'_>>. That was how I implemented the parser feature until now. Should this be removed in favor of from_x509 functions in plain impl blocks?

yasna::DEREncodable Trait

The string definitions are missing this trait impl. I assume this is for the same reason as the x509_parser traits - to avoid exposing them in the public API until they are considered stable?

x509_parser::asn1_rs::Oid<'_> to Vec<u64>

The integer type used is confusing at times.
I'd expect we usually need u8 but we use u64 in many places so I stuck with that.
Adding to that, the parser is using u8 internally but its macro oid!() takes u64 and returns u64 on the fallible iterator.

What is the correct way to convert?

What is or is not our responsiblity?

Is producing conforming or even recommended extensions the libraries or the library consumer's responsibility?

A certificate policy OID MUST NOT appear more than once in a certificate policies extension.

If we implement the policies as a HashSet instead of a Vec and define the hash to only consider the OID, uniqueness could be enforced on our side.

An explicitText field includes the textual statement directly in the certificate. The explicitText field is a string with a maximum size of 200 characters. Conforming CAs SHOULD use the UTF8String encoding for explicitText, but MAY use IA5String. Conforming CAs MUST NOT encode explicitText as VisibleString or BMPString. The explicitText string SHOULD NOT include any control characters (e.g., U+0000 to U+001F and U+007F to U+009F). When the UTF8String encoding is used, all character sequences SHOULD be normalized according to Unicode normalization form C (NFC) [NFC].

Do we need to ensure these terms are met or is it the consumer's responsibility? Should we provide access to all string types or only UTF8String?

PR Scope

Should 4.2.1.14. Inhibit anyPolicy be part of this PR?
It seems sensible to add support for this in parallel with 4.2.1.4. Certificate Policies

Error handling

Sometimes there are branches that should be unreachable. Currently I implemented multiple checks so even branches that should be unreachable are handled. Is that the way to go, should we use unreachable!() or is there a better way?

Internal representation

Should we stay close to the ASN.1 definitions or can/should we diverge as long as the (de-)serialization works? Sometimes RFC5280 asserts that entries must be unique in text but uses a SEQUENCE OF in the ASN.1 definition. I feel like using a HashSet would be the logical choice for unique entries when the order carries no meaning. An example for this would be CertificatePolicies::policy_information

How should CertificatePolicies be built?

I read https://users.rust-lang.org/t/builder-pattern-in-rust-self-vs-mut-self-and-method-vs-associated-function/72892 to try and get an idea of builder design so we can enforce validity.

Other

Other things I noticed that are not directly related to the PR

ExtendedKeyUsagePurpose

Other is not handeled on the parser side.

Requested Screenshots

OpenSSL (WSL)

• cd certs
• openssl x509 --in cert.pem --text --noout

Windows "Krypto-Shellerweiterungen"

Ausstellerklärung:

Unfortunately the window can't be resized

Browsers

Minimal webserver

from uvicorn import run
from fastapi import FastAPI

run(
    FastAPI(),
    host="127.0.0.1",
    port=443,
    ssl_certfile="certs/cert.pem",
    ssl_keyfile="certs/key.pem",
)

Firefox

Chromium (Edge)

ASN.1 JavaScript decoder

Decode of a generated cert by cargo run --example certificate_policies

[Gabgobie]

I'll take another look at test coverage and the pipeline checks in the coming days but wanted to get this out here since the output seems to at least be valid.

[Gabgobie]

I think ci / Validate external types appearing in public API should have failed but didn't. TryFrom<x509_parser::extensions::PolicyInformation<'_>> for PolicyInformation is implemented but not approved.

I'll remove the impl and replace the try_from with from_x509

[Gabgobie]

Suppose this is about as ready for review as it can get. There are still changes to be made but I'll need to know which direction they should go in.