weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Ensure fetched certificate is valid for CSRs public key before issuing

Browse Source 
Signed-off-by: James Munnelly <james@munnelly.eu>
This commit is contained in:
James Munnelly 2020-01-22 16:30:44 +00:00
parent 6d9200f9d3
commit 1f7f23895d
3 changed files with 39 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/controller/certificaterequests/acme/BUILD.bazel
View File

@ -18,6 +18,7 @@ go_library(
"//pkg/issuer:go_default_library",
"//pkg/logs:go_default_library",
"//pkg/util:go_default_library",
"//pkg/util/errors:go_default_library",
"//pkg/util/pki:go_default_library",
"@io_k8s_apimachinery//pkg/api/errors:go_default_library",
"@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",

25
pkg/controller/certificaterequests/acme/acme.go
View File

@ -39,6 +39,7 @@ import (
issuerpkg "github.com/jetstack/cert-manager/pkg/issuer"
logf "github.com/jetstack/cert-manager/pkg/logs"
"github.com/jetstack/cert-manager/pkg/util"
"github.com/jetstack/cert-manager/pkg/util/errors"
"github.com/jetstack/cert-manager/pkg/util/pki"
)
@ -137,7 +138,6 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer
return nil, nil
}
if err != nil {
// We are probably in a network error here so we should backoff and retry
message := fmt.Sprintf("Failed to get order resource %s/%s", expectedOrder.Namespace, expectedOrder.Name)
@ -147,6 +147,15 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer
return nil, err
}
if !metav1.IsControlledBy(order, cr) {
// TODO: improve this behaviour - this issue occurs because someone
// else may create a CertificateRequest with a name that is equal to
// the name of the request we are creating, due to our hash function
// not account for parameters stored on the CSR (i.e. the public key).
// We should improve the way we hash input data or somehow avoid
// relying on deterministic names for Order resources.
return nil, fmt.Errorf("found Order resource not owned by this CertificateRequest, retrying")
}
log = logf.WithRelatedResource(log, order)
@ -163,6 +172,20 @@ func (a *ACME) Sign(ctx context.Context, cr *v1alpha2.CertificateRequest, issuer
// Order valid, return cert. The calling controller will update with ready if its happy with the cert.
if order.Status.State == cmacme.Valid {
x509Cert, err := pki.DecodeX509CertificateBytes(order.Status.Certificate)
if errors.IsInvalidData(err) {
log.Error(err, "failed to decode x509 certificate data on Order resource")
return nil, a.acmeClientV.Orders(order.Namespace).Delete(order.Name, nil)
}
ok, err := pki.PublicKeyMatchesCertificate(csr.PublicKey, x509Cert)
if err != nil {
return nil, err
}
if !ok {
log.Error(err, "failed to decode x509 certificate data on Order resource, recreating...")
return nil, a.acmeClientV.Orders(order.Namespace).Delete(order.Name, nil)
}
log.Info("certificate issued")
return &issuerpkg.IssueResponse{

15
pkg/controller/certificates/sync.go
View File

@ -384,7 +384,10 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt
// Validate the CertificateRequest's CSR is valid
log.Info("validating existing CSR data")
x509CSR, err := pki.DecodeX509CertificateRequestBytes(existingReq.Spec.CSRPEM)
// TODO: handle InvalidData
if errors.IsInvalidData(err) {
log.Info("failed to decode existing CSR on CertificateRequest, deleting resource...")
return c.cmClient.CertmanagerV1alpha2().CertificateRequests(existingReq.Namespace).Delete(existingReq.Name, nil)
}
if err != nil {
return err
}
@ -466,6 +469,16 @@ func (c *certificateRequestManager) processCertificate(ctx context.Context, crt
return err
}
log.Info("checking stored private key is valid for stored x509 certificate on CertificateRequest")
publicKeyMatches, err := pki.PublicKeyMatchesCertificate(privateKey.Public(), x509Cert)
if err != nil {
return err
}
if !publicKeyMatches {
log.Info("private key stored in Secret does not match public key of issued certificate, deleting the old CertificateRequest resource")
return c.cmClient.CertmanagerV1alpha2().CertificateRequests(existingReq.Namespace).Delete(existingReq.Name, nil)
}
// Check if the Certificate requires renewal according to the renewBefore
// specified on the Certificate resource.
log.Info("checking if certificate stored on CertificateRequest is up to date")