Feature: Certificate policies

[Gabgobie]

Thanks for offering to review the PR @djc

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

Compatibility check screenshots

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]

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.

[djc]

1200 lines of code is a lot. I added a bunch of notes on how to pare that down.

[djc]

Also too much.

[cpu]

For whatever it's worth I remain not keen on supporting certificate policies and don't plan to try to review this work.

[Gabgobie]

My apologies. I just took a look at the PR from a private window and noticed that my replies were never published. PR inexperience...

I thought you were just busy! I'll look into what it takes to publish a response.

---

@cpu Should I take your comment as active opposition to this PR? I'll be happy to make more changes but it would have been nice if you were clear about that beforehand. I interpreted your previous comment as indifference/approval. I also found more use-cases for policies aside from user notices, some of which I posted in the original issue.

I'm not strictly opposed to support if you can write a clean PR with appropriate test coverage without a lot of guidance1 but I'm having a hard time seeing this as a genuine use case that will benefit a large enough number of people to be worth the ongoing maintenance.

[cpu]

@cpu Should I take your comment as active opposition to this PR?

No, I wouldn't characterize it as active opposition in the sense that I would be in favour of blocking a PR that other maintainers wanted to approve or something like that. If you can get approvals, I won't stand in the way :-)

it would have been nice if you were clear about that beforehand. I interpreted your previous comment as indifference/approval.

My previous comment also emphasized a desire for a clean PR with appropriate test coverage. I haven't looked at the substance of the diff, but the commit history is pretty messy in the current state. Apologies if I misrepresented my interest/availability for reviewing the work in general, but I think you still have a path forward with djc/est31.

[Gabgobie]

Thank you for clarifying. I am trying to do as much on my own as possible but this being my first larger PR I am not sure about the design decisions you - as in all maintainers of this project - would prefer. Maybe it would be better if I just make a decision over trying to add suggestions to pick from.

I wasn't aware that a clean history is valuable since it will be squashed on merge anyways. I will look into cleaning that up as well.

I'll also give reviewing my own changes another try. Not having looked at it for ~3 weeks I should have a fresh pair of eyes.

[Gabgobie]

If the checks pass, which I'd expect after having them run locally, I'll look into ergonomics when consuming my fork and mark this ready for review afterwards.

#406 (comment)

Let's avoid adding any particular policies for this PR.

Would you like a separate PR or commit with constructors for commonly used/standardized policies or should I implement them on the consumer side? Without these constructors, the consumer needs to know OIDs and in some cases encode their own DER. rcgen generally abstracts this low-level stuff so this would feel out of place to me.

[Gabgobie]

I somehow just now noticed that - aside from testing - from_x509 is useless anyways, see also:

• add Certificate.from_pem and Certificate.from_der #387 (comment)
• CertifiedIssuer::from_ca_cert_{pem,der}? #375 (comment)
• add function to return a Certificate from DER format #293 (comment) (before this restriction was added)

Should I remove this getter and the pub on critical entirely?

[Gabgobie]

Cleaned the history, rebased and tried reviewing the proposed changes again. I think it's ready for another review at your leisure :D