Fixes validation when teh 2 signing keys are set

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

pkg/internal/apis/certmanager/validation/certificaterequest.go

@@ -123,13 +123,21 @@ func getCSRKeyUsage(crSpec *cmapi.CertificateRequestSpec, fldPath *field.Path, c
 
 func patchDuplicateKeyUsage(usages []cmapi.KeyUsage) []cmapi.KeyUsage {
 	// usage signing and digital signature are the same key use in x509
-	for i, usage := range usages {
-		if usage == cmapi.UsageSigning {
-			usages[i] = cmapi.UsageDigitalSignature
+	// we should patch this for proper validation
+
+	newUsages := []cmapi.KeyUsage(nil)
+	hasUsageSigning := false
+	for _, usage := range usages {
+		if (usage == cmapi.UsageSigning || usage == cmapi.UsageDigitalSignature) && !hasUsageSigning {
+			newUsages = append(newUsages, cmapi.UsageDigitalSignature)
+			// prevent having 2 UsageDigitalSignature in the slice
+			hasUsageSigning = true
+		} else {
+			newUsages = append(newUsages, usage)
 		}
 	}
 
-	return usages
+	return newUsages
 }
 
 func isUsageEqual(a, b []cmapi.KeyUsage) bool {

pkg/internal/apis/certmanager/validation/certificaterequest_test.go

@@ -0,0 +1,129 @@
+/*
+Copyright 2020 The Jetstack cert-manager contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package validation
+
+import (
+	"bytes"
+	"encoding/pem"
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1"
+	cminternal "github.com/jetstack/cert-manager/pkg/internal/apis/certmanager"
+	"github.com/jetstack/cert-manager/pkg/util/pki"
+	utilpki "github.com/jetstack/cert-manager/pkg/util/pki"
+	"github.com/jetstack/cert-manager/test/unit/gen"
+)
+
+func TestValidateCertificateRequestSpec(t *testing.T) {
+	fldPath := field.NewPath("test")
+
+	tests := []struct {
+		name   string
+		crSpec *cminternal.CertificateRequestSpec
+		want   field.ErrorList
+	}{
+		{
+			name: "Test csr with no usages",
+			crSpec: &cminternal.CertificateRequestSpec{
+				Request:   mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"))),
+				IssuerRef: validIssuerRef,
+				Usages:    nil,
+			},
+			want: []*field.Error{},
+		},
+		{
+			name: "Test csr with double signature usages",
+			crSpec: &cminternal.CertificateRequestSpec{
+				Request:   mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageSigning, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment))),
+				IssuerRef: validIssuerRef,
+				Usages:    []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment},
+			},
+			want: []*field.Error{},
+		},
+		{
+			name: "Test csr with double extended usages",
+			crSpec: &cminternal.CertificateRequestSpec{
+				Request:   mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))),
+				IssuerRef: validIssuerRef,
+				Usages:    []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth},
+			},
+			want: []*field.Error{},
+		},
+		{
+			name: "Error on csr not having all usages",
+			crSpec: &cminternal.CertificateRequestSpec{
+				Request:   mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth))),
+				IssuerRef: validIssuerRef,
+				Usages:    []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment, cminternal.UsageServerAuth, cminternal.UsageClientAuth},
+			},
+			want: []*field.Error{
+				field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[3] != []certmanager.KeyUsage[4]]"),
+			},
+		},
+		{
+			name: "Error on cr not having all usages",
+			crSpec: &cminternal.CertificateRequestSpec{
+				Request:   mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageServerAuth, cmapi.UsageClientAuth))),
+				IssuerRef: validIssuerRef,
+				Usages:    []cminternal.KeyUsage{cminternal.UsageSigning, cminternal.UsageKeyEncipherment},
+			},
+			want: []*field.Error{
+				field.Invalid(fldPath.Child("request"), nil, "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[4] != []certmanager.KeyUsage[2]]"),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := ValidateCertificateRequestSpec(tt.crSpec, field.NewPath("test"), true)
+			for i := range got {
+				// filter out the value so it does not print the full CSR in tests
+				got[i].BadValue = nil
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("ValidateCertificateRequestSpec() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+func mustGenerateCSR(t *testing.T, crt *cmapi.Certificate) []byte {
+	// Create a new private key
+	pk, err := utilpki.GenerateRSAPrivateKey(2048)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	x509CSR, err := pki.GenerateCSR(crt)
+	if err != nil {
+		t.Fatal(err)
+	}
+	csrDER, err := pki.EncodeCSR(x509CSR, pk)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	csrPEM := bytes.NewBuffer([]byte{})
+	err = pem.Encode(csrPEM, &pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csrDER})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return csrPEM.Bytes()
+}

pkg/util/pki/csr_test.go

@@ -444,6 +444,16 @@ func TestGenerateCSR(t *testing.T) {
 				ExtraExtensions:    ipsecExtraExtensions,
 			},
 		},
+		{
+			name: "Generate CSR from certificate with double signing key usages",
+			crt:  &cmapi.Certificate{Spec: cmapi.CertificateSpec{CommonName: "example.org", Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageSigning}}},
+			want: &x509.CertificateRequest{Version: 3,
+				SignatureAlgorithm: x509.SHA256WithRSA,
+				PublicKeyAlgorithm: x509.RSA,
+				Subject:            pkix.Name{CommonName: "example.org"},
+				ExtraExtensions:    defaultExtraExtensions,
+			},
+		},
 		{
 			name:    "Error on generating CSR from certificate with no subject",
 			crt:     &cmapi.Certificate{Spec: cmapi.CertificateSpec{}},