selfsigned: warn when certs are issued with empty issuer DNs #3760
selfsigned: warn when certs are issued with empty issuer DNs #3760jetstack-bot merged 4 commits intocert-manager:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
there are clearly some missing pieces here based on the tests! I'd missed the e2es |
d47321e to
5577570
Compare
|
/retest |
|
/assign |
JoshVanL
left a comment
There was a problem hiding this comment.
I think the code itself looks good, but am curious as to why this is needed in the first place. In the case of selfsigned issuers, I am strongly in favour of users being able to request exactly what they asked for. If a DN is required for whatever application the user is using, then it makes sense that the user request it.
It also doesn't make sense to me that the subject would default to cert-manager, when the application using is probably not cert-manager.
/unassign
/assign @SgtCoDFish
I understand the desire to allow maximum control and I generally agree; I think my caveat would be that users should be able to request exactly what they ask for as long as it's in spec. Otherwise, inevitably, you end up getting bitten by the cert being invalid somewhere down the road, as happened in #3634 My concern is the following:
This is similar to what can happen with non-critical x509 extensions, which I've seen cause pretty significant outages in the past as newer versions of Chrome started to understand and enforce those non-critical extensions. It's a ticking time-bomb. More generally, I think my position is that cert-manager should not, by default, allow the issuance of a cert for TLS which violates the spec, and to treat anywhere that it's possible to generate an invalid cert as a bug. Maybe cert-manager could have a flag like |
Yeah I understand this - we could just pull a DNS name out of the SAN extension. My thought process was:
|
pkg/controller/certificaterequests/selfsigned/selfsigned_test.go
Outdated
Show resolved
Hide resolved
|
Will take a look today! /assign |
|
I think we would also need to update https://cert-manager.io/docs/configuration/selfsigned with the change made to the self signed issuer API behavior, and explain that if a certificate is created without a commonName, then a default DN Regarding the release-note: since we changed the API, I think we also need to be more specific with regards to the message. Suggestion in cert-manager.io:
Suggestion for the release-notes: |
|
Done with my review! |
it's conceivable that in the future we could have Ed25519 certs, which would also have a key size of 256 but would be a new named entry here Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
this behaviour seems to be handled by translateIngressAnnotations Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
ddd6f3a to
a8d98da
Compare
|
/retest |
I'd strongly lean in favor of Josh's comment here - we previously used to default other fields (like 'Organization') in a similar way, and ended out having to remove it. There are many ways you can violate RFCs with x509, and whilst I think we should help guide users to create valid certificates, we shouldn't do so by affixing things like this IMO. Could we instead add some clear guidance into the documentation, and potentially also fire an Event in the selfsigned certificaterequest controller to note that it has signed a certificate that does not specify a DN and may be incompatible with some clients (and link to the relevant part of the RFC?) |
This does seem reasonable, and I'd be happy to be outvoted and take that approach. That said, I still view outputting a warning Event as being inferior to just preventing the issue entirely; I think what's missing for me is the reasoning:
why did it have to be removed? was it being added to address a similar issue to what we have here?
again, I'd be really interested to know why you hold this belief. For context, I'm in a situation ATM which I'm sure we've all been in at some point: this change seemed super clear and obvious to me and I was really surprised to see any pushback on it at all. (That of course doesn't mean I'm right or that my approach in this PR is right!) The fact that I was so surprised hints to me that I'm maybe missing some part of the bigger picture - maybe I'm over-focusing on the scenario I described here (which is a situation I've seen cause huge problems before) and I'm missing something as a result. |
I guess my lack of X.509 knowledge prevents me from understanding the full implications of the "defaulting" that I thought I a good idea in this PR. Could we have more information about the removal of |
a8d98da to
86e46e8
Compare
86e46e8 to
1298973
Compare
JoshVanL
left a comment
There was a problem hiding this comment.
Couple things from me
/assign @SgtCoDFish
|
|
||
| func runTest(t *testing.T, test testT) { | ||
| test.builder.T = t | ||
| //test.builder.Recorder = new(testpkg.FakeRecorder) |
There was a problem hiding this comment.
Still needs to be removed 😅
There was a problem hiding this comment.
I'm an expert at finding ways to shoot myself in the foot with git 👍
pkg/controller/certificaterequests/selfsigned/selfsigned_test.go
Outdated
Show resolved
Hide resolved
1298973 to
87e71eb
Compare
|
The latest comments should now be addressed 👍 |
|
|
||
| func runTest(t *testing.T, test testT) { | ||
| test.builder.T = t | ||
| //test.builder.Recorder = new(testpkg.FakeRecorder) |
There was a problem hiding this comment.
Still needs to be removed 😅
87e71eb to
5e31fa3
Compare
as raised in#3634 - RFC 5280 states that the issuer field cannot be empty, but this could easily happen with selfsigned certs which had an empty subject (as the issuer matches the subject when the cert is self signed) this commit detects when a cert would be issued selfsigned with an empty subject DN and emits a warning event, allowing cluster operators to detect the warning and potentially either re-issue to generate a compliant cert, or else accept the risk. Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
|
/lgtm |
What this PR does / why we need it: RFC 5280 states that the issuer field cannot be empty, but this is effectively the default for selfsigned certs where the user doesn't specify any part of the subject DN (and with selfsigned certs, the subject becomes the issuer).
There are also some smaller tweaks to comments I discovered as part of this work.
/kind bug
Which issue this PR fixes: fixes #3634 (sort of)
Release note: