From 5e31fa37ff22a7a0e2ffb625f6000c3d6c6c025a Mon Sep 17 00:00:00 2001 From: Ashley Davis Date: Wed, 10 Mar 2021 15:51:34 +0000 Subject: [PATCH] selfsigned: warn when certs have empty issuer DNs 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 --- .../selfsigned/BUILD.bazel | 2 + .../selfsigned/selfsigned.go | 14 ++++ .../selfsigned/selfsigned_test.go | 70 +++++++++++++++++-- 3 files changed, 82 insertions(+), 4 deletions(-) diff --git a/pkg/controller/certificaterequests/selfsigned/BUILD.bazel b/pkg/controller/certificaterequests/selfsigned/BUILD.bazel index f01e5079d..24560fe24 100644 --- a/pkg/controller/certificaterequests/selfsigned/BUILD.bazel +++ b/pkg/controller/certificaterequests/selfsigned/BUILD.bazel @@ -16,8 +16,10 @@ go_library( "//pkg/util/errors:go_default_library", "//pkg/util/kube:go_default_library", "//pkg/util/pki:go_default_library", + "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/api/errors:go_default_library", "@io_k8s_client_go//listers/core/v1:go_default_library", + "@io_k8s_client_go//tools/record:go_default_library", ], ) diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned.go b/pkg/controller/certificaterequests/selfsigned/selfsigned.go index 44fce80fe..2fed19b50 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned.go @@ -23,8 +23,10 @@ import ( "errors" "fmt" + corev1 "k8s.io/api/core/v1" k8sErrors "k8s.io/apimachinery/pkg/api/errors" corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/record" apiutil "github.com/jetstack/cert-manager/pkg/api/util" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" @@ -40,6 +42,7 @@ import ( const ( CRControllerName = "certificaterequests-issuer-selfsigned" + emptyDNMessage = "Certificate will be issued with an empty Issuer DN, which contravenes RFC 5280 and could break some strict clients" ) type signingFn func(*x509.Certificate, *x509.Certificate, crypto.PublicKey, interface{}) ([]byte, *x509.Certificate, error) @@ -49,6 +52,7 @@ type SelfSigned struct { secretsLister corelisters.SecretLister reporter *crutil.Reporter + recorder record.EventRecorder // Used for testing to get reproducible resulting certificates signingFn signingFn @@ -68,6 +72,7 @@ func NewSelfSigned(ctx *controllerpkg.Context) *SelfSigned { issuerOptions: ctx.IssuerOptions, secretsLister: ctx.KubeSharedInformerFactory.Core().V1().Secrets().Lister(), reporter: crutil.NewReporter(ctx.Clock, ctx.Recorder), + recorder: ctx.Recorder, signingFn: pki.SignCertificate, } } @@ -127,6 +132,15 @@ func (s *SelfSigned) Sign(ctx context.Context, cr *cmapi.CertificateRequest, iss template.CRLDistributionPoints = issuerObj.GetSpec().SelfSigned.CRLDistributionPoints + if template.Subject.String() == "" { + // RFC 5280 (https://tools.ietf.org/html/rfc5280#section-4.1.2.4) says that: + // "The issuer field MUST contain a non-empty distinguished name (DN)." + // Since we're creating a self-signed cert, the issuer will match whatever is + // in the template's subject DN. + log.V(logf.DebugLevel).Info("issued cert will have an empty issuer DN, which contravenes RFC 5280. emitting warning event") + s.recorder.Event(cr, corev1.EventTypeWarning, "BadConfig", emptyDNMessage) + } + // extract the public component of the key publickey, err := pki.PublicKeyForPrivateKey(privatekey) if err != nil { diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go index 2eeea36aa..18bd36241 100644 --- a/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go +++ b/pkg/controller/certificaterequests/selfsigned/selfsigned_test.go @@ -25,6 +25,7 @@ import ( "encoding/asn1" "encoding/pem" "errors" + "fmt" "testing" "time" @@ -51,9 +52,9 @@ var ( fixedClock = fakeclock.NewFakeClock(fixedClockStart) ) -func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgorithm) []byte { +func generateCSR(t *testing.T, secretKey crypto.Signer, alg x509.SignatureAlgorithm, commonName string) []byte { asn1Subj, _ := asn1.Marshal(pkix.Name{ - CommonName: "test", + CommonName: commonName, }.ToRDNSequence()) template := x509.CertificateRequest{ RawSubject: asn1Subj, @@ -106,7 +107,7 @@ func TestSign(t *testing.T) { corev1.TLSPrivateKeyKey: []byte("this is a bad key"), }, } - csrRSAPEM := generateCSR(t, skRSA, x509.SHA256WithRSA) + csrRSAPEM := generateCSR(t, skRSA, x509.SHA256WithRSA, "test-rsa") skEC, err := pki.GenerateECPrivateKey(256) if err != nil { @@ -127,7 +128,9 @@ func TestSign(t *testing.T) { corev1.TLSPrivateKeyKey: skECPEM, }, } - csrECPEM := generateCSR(t, skEC, x509.ECDSAWithSHA256) + csrECPEM := generateCSR(t, skEC, x509.ECDSAWithSHA256, "test-ec") + + csrEmptyCertPEM := generateCSR(t, skEC, x509.ECDSAWithSHA256, "") baseCRNotApproved := gen.CertificateRequest("test-cr", gen.SetCertificateRequestAnnotations( @@ -163,6 +166,9 @@ func TestSign(t *testing.T) { ecCR := gen.CertificateRequestFrom(baseCR, gen.SetCertificateRequestCSR(csrECPEM), ) + emptyCR := gen.CertificateRequestFrom(baseCR, + gen.SetCertificateRequestCSR(csrEmptyCertPEM), + ) templateRSA, err := pki.GenerateTemplateFromCertificateRequest(baseCR) if err != nil { @@ -186,6 +192,18 @@ func TestSign(t *testing.T) { t.FailNow() } + templateEmptyCert, err := pki.GenerateTemplateFromCertificateRequest(emptyCR) + if err != nil { + t.Error(err) + t.FailNow() + } + + emptyCertPEM, _, err := pki.SignCertificate(templateEmptyCert, templateEmptyCert, skEC.Public(), skEC) + if err != nil { + t.Error(err) + t.FailNow() + } + tests := map[string]testT{ "a CertificateRequest without an approved condition should do nothing": { certificateRequest: baseCRNotApproved.DeepCopy(), @@ -519,6 +537,50 @@ func TestSign(t *testing.T) { }, }, }, + "should sign a cert with no subject DN and create a warning event": { + certificateRequest: emptyCR.DeepCopy(), + signingFn: func(c1 *x509.Certificate, c2 *x509.Certificate, pk crypto.PublicKey, sk interface{}) ([]byte, *x509.Certificate, error) { + _, cert, err := pki.SignCertificate(c1, c2, pk, sk) + if err != nil { + return nil, nil, err + } + + if cert.Subject.String() != "" { + return nil, nil, errors.New("invalid test: cert being issued should have an empty DN") + } + + // need to return a known PEM cert as is done in other tets, since the actual issued cert (`cert` above) + // will have a different serial number + expiry + return emptyCertPEM, nil, nil + }, + builder: &testpkg.Builder{ + KubeObjects: []runtime.Object{ecKeySecret}, + CertManagerObjects: []runtime.Object{emptyCR.DeepCopy(), baseIssuer}, + ExpectedEvents: []string{ + fmt.Sprintf("Warning BadConfig %s", emptyDNMessage), + "Normal CertificateIssued Certificate fetched from issuer successfully", + }, + ExpectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificaterequests"), + "status", + gen.DefaultTestNamespace, + gen.CertificateRequestFrom( + emptyCR, + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: cmapi.CertificateRequestReasonIssued, + Message: "Certificate fetched from issuer successfully", + LastTransitionTime: &metaFixedClockStart, + }), + gen.SetCertificateRequestCertificate(emptyCertPEM), + gen.SetCertificateRequestCA(emptyCertPEM), + ), + )), + }, + }, + }, } for name, test := range tests {