Bug 1700037: Check cert issuer directly #44
Conversation
openshift-ci-robot
added
the
size/S
mrogers950
force-pushed
the
issuer_check
branch
from
bba2da8
to
e8c71b4
Compare
openshift-ci-robot
added
size/L
|
Updated tests. |
|
/retest |
|
/test e2e-aws |
| func (sc *serviceServingCertController) issuedByCurrentCA(secret *corev1.Secret) bool { | ||
| certs, err := cert.ParseCertsPEM(secret.Data[corev1.TLSCertKey]) | ||
| if err != nil { | ||
| return false |
There was a problem hiding this comment.
Please log the error, if even only to debug
| return false | ||
| } | ||
|
|
||
| if certs[0] == nil { |
There was a problem hiding this comment.
The ParseCertsPEM() is fairly weirdly written. Could you add a check len() of returned certs as well in case its implementation changes in the future?
I went through the implementation down to x509.parseCertificate() and I think certs[0] can never be nil if err != nil but I might be wrong.
| GKM7HG83Wj2hA+DWdy9ZJAdBLISB | ||
| -----END CERTIFICATE----- | ||
| ` | ||
| const testCertWrong = ` |
There was a problem hiding this comment.
Maybe rather call it testCertUnknownIssuer
|
This problem makes me wonder - we are currently still relying on the error annotations in service objects to skip error handling when we've done that enough time already. However, in this case, the annotations could also be removed by a different operator owning the service, which may still possibly cause update looping. Should we handle such cases as well? Maybe not set the annotation at all and just handle the error all of the time? I'm fine with merging this after the comments are fixed, but the above is something we should probably still bear in mind. |
I'd have to look into how the controllers are designed here and in library-go, but usually you use a retry counter on the work queue to deal with retry limits. This makes me wonder if it would be better to use a custom resource (or the CSR resource?) where we can safely record errors in the status, rather than annotations. |
This is true about the error annotations that they could also be stomped on. It would probably be better to move this error accounting to a section in the operator status. |
+1 and follow-up PR/discussion for the error accounting. this will unblock monitoring team. |
Since we don't own the annotated service, the signed-by annotation might be overwritten by the owner, leading to continuous cert regeneration. Perform the issuer check by parsing the cert instead of relying on the signed-by annotation.
mrogers950
force-pushed
the
issuer_check
branch
from
e8c71b4
to
42ea235
Compare
| @@ -210,6 +211,24 @@ func (sc *serviceServingCertController) requiresCertGeneration(service *corev1.S | |||
| return true | |||
| } | |||
|
|
|||
| // Returns true if the secret certificate has the same issuer common name as the current CA, false | |||
| // if not or if there is a parsing error. | |||
| func (sc *serviceServingCertController) issuedByCurrentCA(secret *corev1.Secret) bool { | |||
There was a problem hiding this comment.
nit: can be a pure function (remove struct receiver)
|
/lgtm |
Since we don't own the annotated service, the signed-by annotation might be overwritten by the owner, leading to continuous cert regeneration. Perform the issuer check by parsing the cert instead of relying on the signed-by annotation.
https://bugzilla.redhat.com/show_bug.cgi?id=1700037