New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add SecretPublicKeysDiffersFromCurrentCertificateRequest check #6168
Changes from all commits
22440e8
d310d85
82499eb
19377b4
229f99c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,6 +166,35 @@ func SecretIssuerAnnotationsNotUpToDate(input Input) (string, string, bool) { | |
return "", "", false | ||
} | ||
|
||
// SecretPublicKeyDiffersFromCurrentCertificateRequest checks that the current CertificateRequest | ||
// contains a CSR that is signed by the key stored in the Secret. A failure is often caused by the | ||
// Secret being changed outside of the control of cert-manager, causing the current CertificateRequest | ||
// to no longer match what is stored in the Secret. | ||
func SecretPublicKeyDiffersFromCurrentCertificateRequest(input Input) (string, string, bool) { | ||
if input.CurrentRevisionRequest == nil { | ||
return "", "", false | ||
} | ||
pk, err := pki.DecodePrivateKeyBytes(input.Secret.Data[corev1.TLSPrivateKeyKey]) | ||
if err != nil { | ||
return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true | ||
} | ||
|
||
csr, err := pki.DecodeX509CertificateRequestBytes(input.CurrentRevisionRequest.Spec.Request) | ||
if err != nil { | ||
return InvalidCertificateRequest, fmt.Sprintf("Failed to decode current CertificateRequest: %v", err), true | ||
} | ||
|
||
equal, err := pki.PublicKeysEqual(csr.PublicKey, pk.Public()) | ||
if err != nil { | ||
return InvalidCertificateRequest, fmt.Sprintf("CertificateRequest's public key is invalid: %v", err), true | ||
} | ||
Comment on lines
+187
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is a decoding error which not happen under normal circumstances. If you disagree, please add a comment to explain why this return value is appropriate in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the CSR public key would be corrupt. it would be eg. a |
||
if !equal { | ||
return SecretMismatch, "Secret contains a private key that does not match the current CertificateRequest", true | ||
} | ||
|
||
return "", "", false | ||
} | ||
|
||
func CurrentCertificateRequestNotValidForSpec(input Input) (string, string, bool) { | ||
if input.CurrentRevisionRequest == nil { | ||
// Fallback to comparing the Certificate spec with the issued certificate. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in person yesterday and we agree that the error messages returned from this function should only say what is wrong. They should not say what will happen. That is the job of the code that calls this function.
Other policy check functions also make this mistake, so we should correct all of them in a separate PR.