From 5d1b79765cdfba75e89bc8264cd08f9341edddf5 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Mon, 14 Sep 2020 11:05:44 +0200 Subject: [PATCH 1/4] Fixes validation when teh 2 signing keys are set Signed-off-by: Maartje Eyskens --- .../validation/certificaterequest.go | 16 ++- .../validation/certificaterequest_test.go | 129 ++++++++++++++++++ pkg/util/pki/csr_test.go | 10 ++ 3 files changed, 151 insertions(+), 4 deletions(-) create mode 100644 pkg/internal/apis/certmanager/validation/certificaterequest_test.go diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 9ceaa9cf3ed..89d8f7ce426 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/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 { diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go new file mode 100644 index 00000000000..e1d7201e7e3 --- /dev/null +++ b/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() +} diff --git a/pkg/util/pki/csr_test.go b/pkg/util/pki/csr_test.go index 20eb4e0dcdd..78f88ba5146 100644 --- a/pkg/util/pki/csr_test.go +++ b/pkg/util/pki/csr_test.go @@ -437,6 +437,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{}}, From d283077f5fb0700a1b7fd8dff834a98a1fe1a339 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Mon, 14 Sep 2020 11:20:54 +0200 Subject: [PATCH 2/4] Update bazel Signed-off-by: Maartje Eyskens --- pkg/internal/apis/certmanager/validation/BUILD.bazel | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/internal/apis/certmanager/validation/BUILD.bazel b/pkg/internal/apis/certmanager/validation/BUILD.bazel index caf1547da3e..7fef26f56c2 100644 --- a/pkg/internal/apis/certmanager/validation/BUILD.bazel +++ b/pkg/internal/apis/certmanager/validation/BUILD.bazel @@ -34,6 +34,7 @@ go_test( srcs = [ "certificate_for_issuer_test.go", "certificate_test.go", + "certificaterequest_test.go", "issuer_test.go", ], embed = [":go_default_library"], @@ -42,6 +43,7 @@ go_test( "//pkg/internal/apis/acme:go_default_library", "//pkg/internal/apis/certmanager:go_default_library", "//pkg/internal/apis/meta:go_default_library", + "//pkg/util/pki:go_default_library", "//test/unit/gen:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", From 26513a58b0bb94b2474b1794978d01b5b074ffb8 Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Mon, 14 Sep 2020 17:11:20 +0200 Subject: [PATCH 3/4] Add test to test not erroring on reordered values Signed-off-by: Maartje Eyskens --- .../certmanager/validation/certificaterequest_test.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index e1d7201e7e3..c695bc34e2f 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -66,6 +66,15 @@ func TestValidateCertificateRequestSpec(t *testing.T) { }, want: []*field.Error{}, }, + { + name: "Test csr with reordered 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.UsageServerAuth, cminternal.UsageClientAuth, cminternal.UsageKeyEncipherment, cminternal.UsageDigitalSignature}, + }, + want: []*field.Error{}, + }, { name: "Error on csr not having all usages", crSpec: &cminternal.CertificateRequestSpec{ From 772d3ccf77bb4c3b490387ebc8281d8f97dc70bb Mon Sep 17 00:00:00 2001 From: Maartje Eyskens Date: Tue, 15 Sep 2020 10:43:26 +0200 Subject: [PATCH 4/4] Fix edge case with isCA Signed-off-by: Maartje Eyskens --- .../validation/certificaterequest.go | 16 +++++++++++++++ .../validation/certificaterequest_test.go | 20 +++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest.go b/pkg/internal/apis/certmanager/validation/certificaterequest.go index 89d8f7ce426..999d6281b09 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest.go @@ -63,6 +63,9 @@ func ValidateCertificateRequestSpec(crSpec *cmapi.CertificateRequestSpec, fldPat } else { // only compare usages if set on CR and in the CSR if len(crSpec.Usages) > 0 && len(csr.Extensions) > 0 && validateCSRContent && !reflect.DeepEqual(crSpec.Usages, defaultInternalKeyUsages) { + if crSpec.IsCA { + crSpec.Usages = ensureCertSignIsSet(crSpec.Usages) + } csrUsages, err := getCSRKeyUsage(crSpec, fldPath, csr, el) if len(err) > 0 { el = append(el, err...) @@ -156,3 +159,16 @@ func isUsageEqual(a, b []cmapi.KeyUsage) bool { return util.EqualUnsorted(aStrings, bStrings) } + +// ensureCertSignIsSet adds UsageCertSign in case it is not set +// TODO: add a mutating webhook to make sure this is always set +// when isCA is true. +func ensureCertSignIsSet(list []cmapi.KeyUsage) []cmapi.KeyUsage { + for _, usage := range list { + if usage == cmapi.UsageCertSign { + return list + } + } + + return append(list, cmapi.UsageCertSign) +} diff --git a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go index c695bc34e2f..7e888c7a1c1 100644 --- a/pkg/internal/apis/certmanager/validation/certificaterequest_test.go +++ b/pkg/internal/apis/certmanager/validation/certificaterequest_test.go @@ -75,6 +75,26 @@ func TestValidateCertificateRequestSpec(t *testing.T) { }, want: []*field.Error{}, }, + { + name: "Test csr that is CA with usages set", + crSpec: &cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageCertSign), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageCertSign}, + }, + want: []*field.Error{}, + }, + { + name: "Test csr that is CA but no cert sign in usages", + crSpec: &cminternal.CertificateRequestSpec{ + Request: mustGenerateCSR(t, gen.Certificate("test", gen.SetCertificateDNSNames("example.com"), gen.SetCertificateKeyUsages(cmapi.UsageAny, cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth, cmapi.UsageServerAuth), gen.SetCertificateIsCA(true))), + IssuerRef: validIssuerRef, + IsCA: true, + Usages: []cminternal.KeyUsage{cminternal.UsageAny, cminternal.UsageDigitalSignature, cminternal.UsageKeyEncipherment, cminternal.UsageClientAuth, cminternal.UsageServerAuth}, + }, + want: []*field.Error{}, + }, { name: "Error on csr not having all usages", crSpec: &cminternal.CertificateRequestSpec{