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 {