home/cert-manager
2
0
Fork 0
mirror of https://github.com/wahyd4/cert-manager.git synced 2026-08-09 05:06:38 +10:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #3279 from meyskens/fix-double-signing-validation

Browse Source 
Fix double "signing" KU validation
This commit is contained in:
jetstack-bot
2020-09-22 10:27:51 +01:00
committed by GitHub
parent 4c32570b0d 6930ee1753
commit 4a13dd72f3
4 changed files with 198 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/internal/apis/certmanager/validation/BUILD.bazel
+2
View File
@@ -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",
pkg/internal/apis/certmanager/validation/certificaterequest.go
+28 -4
View File
@@ -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...)
@@ -123,13 +126,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 {
@@ -148,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)
}
pkg/internal/apis/certmanager/validation/certificaterequest_test.go
+158
View File
@@ -0,0 +1,158 @@
/*
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: "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: "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{
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
+10
View File
@@ -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{}},