Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUGFIX: Stricter CertificateRequest CSR webhook validation #6182

Merged
merged 3 commits into from Jun 29, 2023
Merged

BUGFIX: Stricter CertificateRequest CSR webhook validation #6182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3938c75
improve (Extended)KeyUsage parsing to be more consistent
inteon Jun 26, 2023
6338701
make CertificateRequest webhook validation more strict (the Usages ar…
inteon Jun 26, 2023
2f56c3c
add DontAllowInsecureCSRUsageDefinition feature gate to disable the s…
inteon Jun 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 17 additions & 5 deletions internal/apis/certmanager/validation/certificaterequest.go
View file
Open in desktop
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
Open in desktop
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"
Comment on lines +51 to +56
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: I don't think this flag really matches my understanding of what we want a "feature gate" to be.

My understanding was that a feature gate was something we were concerned might break or need to change in the future.

I don't really see a huge problem here with this just being a regular CLI flag. I suppose a feature gate makes it easier to remove this in the future, but I don't really see a pressing need to remove this option.

It doesn't really matter though!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that we would like to remove this feature flag in an upcoming release if no one complains that this change causes problems for them. That is why I added this as a feature flag.
I think you only want to be less strict in case you have a cert-manager integration that "incorrectly" uses the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the future, I would like to make some other improvements to our validation webhook that might be a bit more difficult to implement with this feature flag (not sure). eg. #6056

)

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
Open in desktop
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
Open in desktop
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
Open in desktop
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