Skip to content

Commit

Permalink
Merge pull request #6182 from inteon/stricter_certificaterequest_csr_…
Browse files Browse the repository at this point in the history 
…webhook_validation

BUGFIX: Stricter CertificateRequest CSR webhook validation
  • Loading branch information
@jetstack-bot
jetstack-bot committed Jun 29, 2023
2 parents 7482de8 + 2f56c3c commit e66a92a
Show file tree
Hide file tree
Showing 5 changed files with 215 additions and 12 deletions.
22 changes: 17 additions & 5 deletions internal/apis/certmanager/validation/certificaterequest.go
View file
Expand Up @@ -30,9 +30,11 @@ import (

cmapi "github.com/cert-manager/cert-manager/internal/apis/certmanager"
cmmeta "github.com/cert-manager/cert-manager/internal/apis/meta"
"github.com/cert-manager/cert-manager/internal/webhook/feature"
"github.com/cert-manager/cert-manager/pkg/apis/acme"
"github.com/cert-manager/cert-manager/pkg/apis/certmanager"
"github.com/cert-manager/cert-manager/pkg/util"
utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
"github.com/cert-manager/cert-manager/pkg/util/pki"
)

Expand Down Expand Up @@ -95,16 +97,26 @@ func ValidateCertificateRequestSpec(crSpec *cmapi.CertificateRequestSpec, fldPat
if err != nil {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("failed to decode csr: %s", err)))
} else {
// only compare usages if set on CR and in the CSR
if len(crSpec.Usages) > 0 && len(csr.Extensions) > 0 && !reflect.DeepEqual(crSpec.Usages, defaultInternalKeyUsages) {
// in case DontAllowInsecureCSRUsageDefinition is disabled: only compare usages if set on the CR
// otherwise: always compare usages
if utilfeature.DefaultMutableFeatureGate.Enabled(feature.DontAllowInsecureCSRUsageDefinition) || len(crSpec.Usages) > 0 {
// set capacity to length to obtain a "copy-on-append" slice
crUsages := crSpec.Usages[:len(crSpec.Usages):len(crSpec.Usages)]
if len(crUsages) == 0 {
crUsages = defaultInternalKeyUsages[:len(defaultInternalKeyUsages):len(defaultInternalKeyUsages)]
}

if crSpec.IsCA {
crSpec.Usages = ensureCertSignIsSet(crSpec.Usages)
crUsages = ensureCertSignIsSet(crUsages)
}

csrUsages, err := getCSRKeyUsage(crSpec, fldPath, csr, el)
if len(err) > 0 {
el = append(el, err...)
} else if len(csrUsages) > 0 && !isUsageEqual(csrUsages, crSpec.Usages) && !isUsageEqual(csrUsages, defaultInternalKeyUsages) {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("csr key usages do not match specified usages, these should match if both are set: %s", pretty.Diff(patchDuplicateKeyUsage(csrUsages), patchDuplicateKeyUsage(crSpec.Usages)))))
}

if len(csrUsages) > 0 && !isUsageEqual(csrUsages, crUsages) {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("csr key usages do not match specified usages, these should match if both are set: %s", pretty.Diff(patchDuplicateKeyUsage(csrUsages), patchDuplicateKeyUsage(crUsages)))))
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions internal/webhook/feature/features.go
View file
Expand Up @@ -47,6 +47,13 @@ const (
// This feature gate must be used together with LiteralCertificateSubject webhook feature gate.
// See https://github.com/cert-manager/cert-manager/issues/3203 and https://github.com/cert-manager/cert-manager/issues/4424 for context.
LiteralCertificateSubject featuregate.Feature = "LiteralCertificateSubject"

// Owner (responsible for graduating feature through to GA): @inteon
// GA: v1.13
// DontAllowInsecureCSRUsageDefinition will prevent the webhook from allowing
// CertificateRequest's usages to be only defined in the CSR, while leaving
// the usages field empty.
DontAllowInsecureCSRUsageDefinition featuregate.Feature = "DontAllowInsecureCSRUsageDefinition"
)

func init() {
Expand All @@ -61,6 +68,8 @@ func init() {
//
// Where utilfeature is github.com/cert-manager/cert-manager/pkg/util/feature.
var webhookFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
DontAllowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.GA},

AdditionalCertificateOutputFormats: {Default: false, PreRelease: featuregate.Alpha},
LiteralCertificateSubject: {Default: false, PreRelease: featuregate.Alpha},
}
8 changes: 8 additions & 0 deletions pkg/api/util/kube.go
View file
Expand Up @@ -91,6 +91,10 @@ func KubeExtKeyUsageStrings(usage []x509.ExtKeyUsage) []certificatesv1.KeyUsage

// kubeKeyUsageString returns the cmapi.KeyUsage and "unknown" if not found
func kubeKeyUsageString(usage x509.KeyUsage) certificatesv1.KeyUsage {
if usage == x509.KeyUsageDigitalSignature {
return certificatesv1.UsageDigitalSignature // we have two keys that map to KeyUsageDigitalSignature in our map, we should be consistent when parsing
}

for k, v := range keyUsagesKube {
if usage == v {
return k
Expand All @@ -102,6 +106,10 @@ func kubeKeyUsageString(usage x509.KeyUsage) certificatesv1.KeyUsage {

// kubeExtKeyUsageString returns the cmapi.ExtKeyUsage and "unknown" if not found
func kubeExtKeyUsageString(usage x509.ExtKeyUsage) certificatesv1.KeyUsage {
if usage == x509.ExtKeyUsageEmailProtection {
return certificatesv1.UsageEmailProtection // we have two keys that map to ExtKeyUsageEmailProtection in our map, we should be consistent when parsing
}

for k, v := range extKeyUsagesKube {
if usage == v {
return k
Expand Down
11 changes: 8 additions & 3 deletions pkg/api/util/usages.go
View file
Expand Up @@ -90,10 +90,11 @@ func ExtKeyUsageStrings(usage []x509.ExtKeyUsage) []cmapi.KeyUsage {

// keyUsageString returns the cmapi.KeyUsage and "unknown" if not found
func keyUsageString(usage x509.KeyUsage) cmapi.KeyUsage {
if usage == x509.KeyUsageDigitalSignature {
return cmapi.UsageDigitalSignature // we have two keys that map to KeyUsageDigitalSignature in our map, we should be consistent when parsing
}

for k, v := range keyUsages {
if usage == x509.KeyUsageDigitalSignature {
return cmapi.UsageDigitalSignature // we have KeyUsageDigitalSignature twice in our array, we should be consistent when parsing
}
if usage == v {
return k
}
Expand All @@ -104,6 +105,10 @@ func keyUsageString(usage x509.KeyUsage) cmapi.KeyUsage {

// extKeyUsageString returns the cmapi.ExtKeyUsage and "unknown" if not found
func extKeyUsageString(usage x509.ExtKeyUsage) cmapi.KeyUsage {
if usage == x509.ExtKeyUsageEmailProtection {
return cmapi.UsageEmailProtection // we have two keys that map to ExtKeyUsageEmailProtection in our map, we should be consistent when parsing
}

for k, v := range extKeyUsages {
if usage == v {
return k
Expand Down
177 changes: 173 additions & 4 deletions test/integration/validation/certificaterequest_test.go
View file
Expand Up @@ -26,12 +26,14 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/pointer"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/cert-manager/cert-manager/integration-tests/framework"
"github.com/cert-manager/cert-manager/pkg/api"
cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
"github.com/cert-manager/cert-manager/pkg/util/pki"
)

Expand Down Expand Up @@ -100,7 +102,8 @@ func TestValidationCertificateRequests(t *testing.T) {
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: false,
expectError: true,
errorSuffix: "csr key usages do not match specified usages, these should match if both are set: [[]certmanager.KeyUsage[3] != []certmanager.KeyUsage[2]]",
},
"No errors on valid certificaterequest with special usages only set in spec": {
input: &cmapi.CertificateRequest{
Expand All @@ -111,8 +114,9 @@ func TestValidationCertificateRequests(t *testing.T) {
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
EncodeUsagesInRequest: pointer.Bool(false),
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
Expand Down Expand Up @@ -142,6 +146,73 @@ func TestValidationCertificateRequests(t *testing.T) {
errorSuffix: "csr key usages do not match specified usages, these should match if both are set: [[2]: \"client auth\" != \"code signing\"]",
},
"Shouldn't error when setting user info, since this will be overwritten by the mutating webhook": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
EncodeUsagesInRequest: pointer.Bool(false),
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
IssuerRef: cmmeta.ObjectReference{Name: "test"},
Username: "user-1",
Groups: []string{"group-1", "group-2"},
},
},
expectError: false,
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
cert := test.input.(*cmapi.CertificateRequest)
cert.SetGroupVersionKind(certGVK)

ctx, cancel := context.WithTimeout(context.Background(), time.Second*40)
defer cancel()

// The default is true, but we set it here to make sure it was not changed by other tests
utilfeature.DefaultMutableFeatureGate.Set("DontAllowInsecureCSRUsageDefinition=true")

config, stop := framework.RunControlPlane(t, ctx)
defer stop()

framework.WaitForOpenAPIResourcesToBeLoaded(t, ctx, config, certGVK)

// create the object to get any errors back from the webhook
cl, err := client.New(config, client.Options{Scheme: api.Scheme})
if err != nil {
t.Fatal(err)
}

err = cl.Create(ctx, cert)
if test.expectError != (err != nil) {
t.Errorf("unexpected error, exp=%t got=%v",
test.expectError, err)
}
if test.expectError && !strings.HasSuffix(err.Error(), test.errorSuffix) {
t.Errorf("unexpected error suffix, exp=%s got=%s",
test.errorSuffix, err)
}
})
}
}

// TestValidationCertificateRequests_DontAllowInsecureCSRUsageDefinition_false makes sure that the
// validation webhook keeps working as before when the DontAllowInsecureCSRUsageDefinition feature
// gate is disabled.
func TestValidationCertificateRequests_DontAllowInsecureCSRUsageDefinition_false(t *testing.T) {
tests := map[string]struct {
input runtime.Object
errorSuffix string // is a suffix as the API server sends the whole value back in the error
expectError bool
}{
"No errors on valid certificaterequest with no usages set": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Expand All @@ -151,7 +222,103 @@ func TestValidationCertificateRequests(t *testing.T) {
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
},
}),
Usages: []cmapi.KeyUsage{},
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: false,
},
"No errors on valid certificaterequest with special usages set": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: false,
},
"No errors on valid certificaterequest with special usages set only in CSR": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
},
}),
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: false,
},
"No errors on valid certificaterequest with special usages only set in spec": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
EncodeUsagesInRequest: pointer.Bool(false),
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: false,
},
"Errors on certificaterequest with mismatch of usages": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageCodeSigning},
IssuerRef: cmmeta.ObjectReference{Name: "test"},
},
},
expectError: true,
errorSuffix: "csr key usages do not match specified usages, these should match if both are set: [[2]: \"client auth\" != \"code signing\"]",
},
"Shouldn't error when setting user info, since this will be overwritten by the mutating webhook": {
input: &cmapi.CertificateRequest{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
Usages: []cmapi.KeyUsage{},
EncodeUsagesInRequest: pointer.Bool(false),
},
}),
Usages: []cmapi.KeyUsage{cmapi.UsageDigitalSignature, cmapi.UsageKeyEncipherment, cmapi.UsageClientAuth},
Expand All @@ -171,6 +338,8 @@ func TestValidationCertificateRequests(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), time.Second*40)
defer cancel()

utilfeature.DefaultMutableFeatureGate.Set("DontAllowInsecureCSRUsageDefinition=false")

config, stop := framework.RunControlPlane(t, ctx)
defer stop()

Expand Down

0 comments on commit e66a92a

Please sign in to comment.