Improve validation of certificates. Fix bug in checking certificate validity

pkg/controller/certificates/sync.go

@@ -111,8 +111,14 @@ func (c *Controller) Sync(ctx context.Context, crt *v1alpha1.Certificate) (err e
 	defer c.scheduleRenewal(crt)
 
 	crtCopy := crt.DeepCopy()
-	expectedCN := pki.CommonNameForCertificate(crtCopy)
-	expectedDNSNames := pki.DNSNamesForCertificate(crtCopy)
+	expectedCN, err := pki.CommonNameForCertificate(crtCopy)
+	if err != nil {
+		return err
+	}
+	expectedDNSNames, err := pki.DNSNamesForCertificate(crtCopy)
+	if err != nil {
+		return err
+	}
 
 	// if the certificate was not found, or the certificate data is invalid, we
 	// should issue a new certificate.

pkg/issuer/acme/prepare.go

@@ -213,25 +213,12 @@ func authorizationsMap(list []v1alpha1.ACMEDomainAuthorization) map[string]v1alp
 	return out
 }
 
-func removeDuplicates(in []string) []string {
-	var found []string
-Outer:
-	for _, i := range in {
-		for _, i2 := range found {
-			if i2 == i {
-				continue Outer
-			}
-		}
-		found = append(found, i)
-	}
-	return found
-}
-
 func (a *Acme) authorizationsToObtain(ctx context.Context, cl *acme.Client, crt *v1alpha1.Certificate) ([]string, error) {
 	authMap := authorizationsMap(crt.Status.ACMEStatus().Authorizations)
-	expectedCN := pki.CommonNameForCertificate(crt)
-	expectedDNSNames := pki.DNSNamesForCertificate(crt)
-	check := removeDuplicates(append(expectedDNSNames, expectedCN))
+	expectedDNSNames, err := pki.DNSNamesForCertificate(crt)
+	if err != nil {
+		return nil, err
+	}
 	toAuthorize := util.StringFilter(func(domain string) (bool, error) {
 		auth, ok := authMap[domain]
 		glog.Infof("Compare %q with %q", auth.Account, a.issuer.GetStatus().ACMEStatus().URI)
@@ -239,7 +226,7 @@ func (a *Acme) authorizationsToObtain(ctx context.Context, cl *acme.Client, crt
 			return false, nil
 		}
 		return checkAuthorization(ctx, cl, auth.URI)
-	}, check...)
+	}, expectedDNSNames...)
 
 	domains := make([]string, len(toAuthorize))
 	for i, v := range toAuthorize {

pkg/util/filter.go

@@ -55,3 +55,17 @@ func StringFilter(fn FilterFn, in ...string) StringFilterWrapper {
 	}
 	return res
 }
+
+func RemoveDuplicates(in []string) []string {
+	dedupMap := make(map[string]struct{})
+	for _, i := range in {
+		dedupMap[i] = struct{}{}
+	}
+	out := make([]string, len(dedupMap))
+	i := 0
+	for s := range dedupMap {
+		out[i] = s
+		i++
+	}
+	return out
+}

pkg/util/pki/csr.go

@@ -3,28 +3,36 @@ package pki
 import (
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"fmt"
 	"github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha1"
+	"github.com/jetstack/cert-manager/pkg/util"
 )
 
 // The provided Certificate *must* specify either a DNS name or a
 // CommonName else this function will panic.
-func CommonNameForCertificate(crt *v1alpha1.Certificate) string {
-	if crt.Spec.CommonName == "" {
-		return crt.Spec.DNSNames[0]
+func CommonNameForCertificate(crt *v1alpha1.Certificate) (string, error) {
+	if crt.Spec.CommonName != "" {
+		return crt.Spec.CommonName, nil
 	}
-	return crt.Spec.CommonName
+	if len(crt.Spec.DNSNames) == 0 {
+		return "", fmt.Errorf("certificate must specify at least one of dnsNames or commonName")
+	}
+	return crt.Spec.DNSNames[0], nil
 }
 
 // The provided Certificate *must* specify either a DNS name or a
 // CommonName else this function will panic.
-func DNSNamesForCertificate(crt *v1alpha1.Certificate) []string {
+func DNSNamesForCertificate(crt *v1alpha1.Certificate) ([]string, error) {
 	if len(crt.Spec.DNSNames) == 0 {
-		return []string{crt.Spec.CommonName}
+		if crt.Spec.CommonName == "" {
+			return nil, fmt.Errorf("certificate must specify at least one of dnsNames or commonName")
+		}
+		return []string{crt.Spec.CommonName}, nil
 	}
 	if crt.Spec.CommonName != "" {
-		return append([]string{crt.Spec.CommonName}, crt.Spec.DNSNames...)
+		return util.RemoveDuplicates(append([]string{crt.Spec.CommonName}, crt.Spec.DNSNames...)), nil
 	}
-	return crt.Spec.DNSNames
+	return crt.Spec.DNSNames, nil
 }
 
 func GenerateCSR(commonName string, altNames ...string) *x509.CertificateRequest {