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 <ashley.davis@jetstack.io>

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",
     ],
 )
 

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 {

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 {