Conversation
|
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. |
fd4cb64 to
64ee5b1
Compare
|
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
left a comment
There was a problem hiding this comment.
1200 lines of code is a lot. I added a bunch of notes on how to pare that down.
rcgen/src/certificate.rs
Outdated
| /// ```rust | ||
| /// use rcgen::{CertificatePolicies, PolicyInformation, Error}; |
rcgen/src/certificate.rs
Outdated
| /// When a CA does not wish to limit the set of policies | ||
| /// for certification paths that include this certificate, | ||
| /// it MAY assert the special policy anyPolicy, with a | ||
| /// value of { 2 5 29 32 0 }. | ||
| pub fn any_policy() -> Self { | ||
| Self::new_oid_only(vec![2, 5, 29, 32, 0]) | ||
| } | ||
|
|
||
| /// Certificate issued in compliance with the Extended Validation Guidelines | ||
| pub fn extended_validation() -> Self { | ||
| Self::new_oid_only(vec![2, 23, 140, 1, 1]) | ||
| } | ||
|
|
||
| /// Certificate issued in compliance with the TLS Baseline Requirements – No entity identity asserted | ||
| pub fn domain_validated() -> Self { | ||
| Self::new_oid_only(vec![2, 23, 140, 1, 2, 1]) | ||
| } | ||
|
|
||
| /// Certificate issued in compliance with the TLS Baseline Requirements – Organization identity asserted | ||
| pub fn organization_validated() -> Self { | ||
| Self::new_oid_only(vec![2, 23, 140, 1, 2, 2]) | ||
| } | ||
|
|
||
| /// Certificate issued in compliance with the TLS Baseline Requirements – Individual identity asserted | ||
| pub fn individual_validated() -> Self { | ||
| Self::new_oid_only(vec![2, 23, 140, 1, 2, 3]) | ||
| } | ||
|
|
||
| /// > The CPS Pointer qualifier contains a pointer to a Certification | ||
| /// > Practice Statement (CPS) published by the CA. The pointer is in the | ||
| /// > form of a URI. Processing requirements for this qualifier are a | ||
| /// > local matter. No action is mandated by this specification regardless | ||
| /// > of the criticality value asserted for the extension. | ||
| pub fn cps_uri(cps_uris: Vec<string::Ia5String>) -> Self { | ||
| Self::new_oid_qualifiers( | ||
| // Didn't find this one in RFC5280 and took it from PKIOverheid (Dutch Government PKI) using Firefox certificate inspection | ||
| // Is it plausible, that this is matching the PolicyQualifierInfo OID? |
There was a problem hiding this comment.
Let's avoid adding any particular policies for this PR.
|
For whatever it's worth I remain not keen on supporting certificate policies and don't plan to try to review this work. |
86d4652 to
275c30c
Compare
|
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.
|
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 :-)
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. |
|
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. |
275c30c to
2d28ee7
Compare
2d28ee7 to
2992e38
Compare
InhibitAnyPolicy is used to limit the use of the special policy anypolicy that circumvents validation. Policies must be validated for example when deciding the level of trust to put into a certificate. Common policies include 2.23.140.1.2 which denotes the validation procedure. 2.23.140.1.2.1 for example is "Domain Validation" See also https://oid-base.com/get/2.23.140.1.2.1 or inspect a LetsEncrypt certificate
2992e38 to
a80f896
Compare
|
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.
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. |
Thanks for offering to review the PR @djc
Adds the following extensions
This PR handles two of the extensions listed/requested in #370
Compatibility check screenshots
Requested Screenshots
OpenSSL (WSL)
cd certsopenssl x509 --in cert.pem --text --nooutWindows "Krypto-Shellerweiterungen"
Ausstellerklärung:
Unfortunately the window can't be resized
Browsers
Minimal webserver
Firefox
Chromium (Edge)
ASN.1 JavaScript decoder
Decode of a generated cert by
cargo run --example certificate_policies