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.
| /// ```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. |
3fca22b to
b92ec85
Compare
cargo fmt --all -- --config-path .rustfmt.unstable.toml
cargo doc --features ring,pem,x509-parser --document-private-items
…for InhibitAnyPolicy
…cting custom policies
Ran every failed command locally
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. |
Thanks for offering to review the PR @djc
Adds the following extensions
This PR handles two of the extensions listed/requested in #370
Questions
What is or is not our responsiblity? Is producing conforming or even recommended extensions the library's or the library consumer's responsibility?
Examples for directives we could enforce
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?
Internal representation (related to the question about responsibilities)
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.#265 has raised the same question about
key_usagesand seems to plan making this change for the next major release. As raised over there, this would introduce a non-deterministic order.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_policiesTODO
Taking another look at the
Errorvariants, there seem to be highly specific errors. I'll continue with this style by adding specific instead of generic errors as well.As part of my own review I tried consuming the new extension and am not happy with the current handling. I'll be changing the instantiation process to be more ergonomic.