Conversation
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
from
5243530 to
7192b84
Compare
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Hi there, thanks for the issue/PR.
I haven't sat down to form a strong opinion on the core subject, but the specific spot in 5280 you cite and the linked cert-manager issue don't strike me as strong motivation. I think I should page in more context and revisit.
With respect to the diff: it's fairly invasive for an issue that at first glance feels uncommon/niche. Certainly the semver break would be important to avoid unless critical.
WDYT about getting CI to pass and reworking to be semver compatible as a first step?
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
from
7192b84 to
b327923
Compare
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
from
b327923 to
062af97
Compare
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
2 times, most recently
from
e12523c to
0ecfc1b
Compare
There can be multiple of the same entry, e.g. DC (domain component) entries in an issuer.
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
from
2e18fbb to
8e0466f
Compare
da-kami
force-pushed
the
multiple-domain-components-in-issuer
branch
from
8e0466f to
dd62f8c
Compare
cpu
left a comment
cpu
left a comment
There was a problem hiding this comment.
Sorry for the delay in taking a look at this. I've wanted to do it with some care and its harder to find time for that vs simpler PRs. I'm afraid as of now I don't feel comfortable +1ing this branch.
Firstly, unless I'm mistaken it still seems to be a semver breaking change and I don't think the necessity of the feature justifies that.
Secondly, I think while it's true that some distinguished name types should allow multiple value I don't think it's a universal property. In particular I'm nervous that this will encourage users to try things like adding two common names to the certificate subject instead of using the subject alternative names extension. You haven't offered a strong citation to a standards document that supports the idea that all DN types should allow multiple values and so I think this wariness is justified.
I do agree that for domain components in particular it should be possible to encode multiple values, but I think the API as designed here is too broad a change. For comparsion, see how Go's x509 package's pkix.Name.ToRdnSequence() only treats a specific set of dn types as multi-value (and as expected, OrganizationalUnit is one of them).
Could we think up a more complex API that works similar to Go's that would be less likely to cause misuse? Probably. Is it worth the effort? I'm not so sure.
| /// Replaces the *first occurrence* of a type with a new value. | ||
| /// This is a convenience function to avoid duplicating values. | ||
| /// | ||
| /// If there are multiple occurrences of a type there is currently no way of changing them besides iterating over the types and values of an existing instance and creating a new instance. |
There was a problem hiding this comment.
| /// If there are multiple occurrences of a type there is currently no way of changing them besides iterating over the types and values of an existing instance and creating a new instance. | |
| /// If there are multiple occurrences of a type that you wish to replace you must iterate over the types and values of an existing instance and create a new instance. |
| #[test] | ||
| #[cfg(feature = "x509-parser")] | ||
| fn test_parse_certificate_with_multiple_domain_components() { |
There was a problem hiding this comment.
I think this test should be moved to tests/generic.rs since it only relies on x509-parser.
While the static test certificate was generated with the openssl command line tool, it's not using the Rust openssl crate dependency like the other tests in this file.
| let domain_component_values = param.distinguished_name.get(&DnType::CustomDnType(vec![ | ||
| 0, 9, 2342, 19200300, 100, 1, 25, | ||
| ])); |
There was a problem hiding this comment.
Since you have x509-parser for this test you can avoid the hardcoded bare OID with a feature-guarded import of x509_parser::oid_registry::OID_DOMAIN_COMPONENT:
let domain_component_values = param.distinguished_name.get(&DnType::CustomDnType(
OID_DOMAIN_COMPONENT.iter().unwrap().collect(),
));| // Domain Component (DC) | ||
| let dc_type = DnType::CustomDnType(vec![0, 9, 2342, 19200300, 100, 1, 25]); |
There was a problem hiding this comment.
It seems unimportant for the test added in this file that the DnType be a Custom one for DC. I think you could avoid some duplication by using a predefined DnType. WDYT?
| /// Obtains the attribute value for the given attribute type | ||
| pub fn get(&self, ty: &DnType) -> Option<&DnValue> { | ||
| self.entries.get(ty) | ||
| pub fn get(&self, ty: &DnType) -> Vec<&DnValue> { |
There was a problem hiding this comment.
Isn't this a breaking API change? That conclusion also seems to be supported by the necessity to change the example code on push().
"worth the effort" implies a benefit/cost trade-off. I tend to feel that there is a decent amount of value in designing/maintaining careful, misuse-resistant APIs for libraries like this even if the amount of usage is fairly small, because the API itself can help more people guide towards doing the right thing. Realistically, the alternative is that someone faced with this use case will throw together their certs with a less careful API which overall will decrease the quality of certificates in the ecosystem. (There might be a corollary to Postel's law in here, but I'm not sure I have a pithy formulation of it.) So that brings us to the cost -- someone has to bear the cost of designing and reviewing such an API, and it's not completely obvious to me who might be interested in and capable of designing it right in this case. |
I agree with that in the abstract, but in practice I think there's a fixed amount of time anyone can spend on this project and so we should direct it towards improvements that offer the most bang for the buck. Putting that aside, is it fair to say we're in agreement the proposed updated API is not a careful, misuse-resistant API, or is there some disagreement there?
True, but there are many projects that would allow someone to do this outside of rcgen. My principle concern is not making the outcome impossible, but ensuring that any change we make to support niche use-cases doesn't encourage misuse in the more common circumstances.
Indeed, and I think I'm unlikely to want to spend considerable time on this myself. The lag in review is also good evidence I can't prioritize this in practice. Maybe we should take a step back and learn more about the use-case that is driving this feature request? In #301 the problem was described as one of extant certs with multiple-DCs not parsing correctly, but here we have an API to produce such certificates. I feel like I'm missing some details on how we went from A to B. |
Yes, I agree with that.
That's definitely fair. @da-kami can you give us more details about what you are trying to achieve? |
|
I'm fine with making the API more complex, especially I prefer explicit complexity over implicit complexity (which is C style: you need to use the "simple" API correctly and respect the implicit invariants). Ideally there would still be a "simple" API for the 90% of users who don't need multiple domain components. |
|
Since we haven't heard back from the original author and there seems to be consensus this isn't the right solution I would be in favour of closing it for now. 👍 / 👎? |
|
Unfortunately this is exactly what I need for fetching signed certificates from SAP PKI service. There are 2 "OU" components required. |
|
@thhapke Are you or your organization willing to sponsor development of an acceptable fix? |
|
I am checking how lasting the multiple-domain approach is going to be. Personally I am on your line to think it is rather exotic . If there are strong reasons and if we like to use it then I am coming back. Thanks |
Sorry, I'm not sure I'm parsing this question correctly. AFAIK nobody is working on a fix for this issue. I think doing so would involve a redesign of the way RDNs are handled and I'm personally not interested in taking on that work as a volunteer at this time. |
|
Yep I totally got it. I just wanted to mention that there is another use case in addition to the original one and I fully understood that this was not a commitment. |
Hey @cpu , sorry for only cycling back to this after significant time. I'm not sure how much further I can argue in favor of certificates that have multiple domain components. We have customers that do that, and it's hard to convince them otherwise. So for now I'm forced to use a fork. If we assume that it would be a more complete implementation, would you accept a change? On top of that, when it comes to changing this, would you rather have:
Thanks |
|
We have a PR to address this (if it doesn't, please let us know) that will likely get merged soon: If you can contribute test cases to address the feedback in that PR, that would be great. |
I'll get back to this tomorrow and see to add tests. |
5 participants
See: #300
I did not invest a big amount of time into changing the interface; please let me know if this PR is welcome otherwise I'm happy to close it. It would be great if #300 can be overcome at some point.