weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

Merge pull request #6963 from inteon/graduate_DisallowInsecureCSRUsageDefinition

Browse Source 
Graduate 'DisallowInsecureCSRUsageDefinition' to GA (part 2)
This commit is contained in:
cert-manager-prow[bot] 2024-04-26 17:22:35 +00:00 committed by GitHub
commit 410b7a6ffb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 25 additions and 223 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
.golangci.yaml
View File

@ -45,7 +45,7 @@ issues:
text: "SA(1002|1006|4000|4006)"
- linters:
- staticcheck
text: "(NewCertManagerBasicCertificateRequest|DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition)"
text: "(NewCertManagerBasicCertificateRequest)"
linters:
# Explicitly define all enabled linters
disable-all: true

34
internal/apis/certmanager/validation/certificaterequest.go
View File

@ -27,11 +27,9 @@ 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"
cmapiv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
"github.com/cert-manager/cert-manager/pkg/util/pki"
)
@ -109,30 +107,14 @@ func validateCertificateRequestSpecRequest(crSpec *cmapi.CertificateRequestSpec,
return el
}
// If DisallowInsecureCSRUsageDefinition is disabled and usages is empty,
// then we should allow the request to be created without requiring that the
// CSR usages match the default usages, instead we only validate that the
// BasicConstraints are valid.
// TODO: simplify this logic when we remove the feature gate
if !utilfeature.DefaultMutableFeatureGate.Enabled(feature.DisallowInsecureCSRUsageDefinition) && len(crSpec.Usages) == 0 {
_, err = pki.CertificateTemplateFromCSRPEM(
crSpec.Request,
pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil),
)
if err != nil {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error()))
return el
}
} else {
_, err = pki.CertificateTemplateFromCSRPEM(
crSpec.Request,
pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil),
pki.CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage),
)
if err != nil {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error()))
return el
}
_, err = pki.CertificateTemplateFromCSRPEM(
crSpec.Request,
pki.CertificateTemplateValidateAndOverrideBasicConstraints(crSpec.IsCA, nil),
pki.CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage),
)
if err != nil {
el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, err.Error()))
return el
}
return el

3
internal/controller/feature/features.go
View File

@ -114,6 +114,7 @@ const (
// Owner: @inteon
// Beta: v1.13
// GA: v1.15
//
// DisallowInsecureCSRUsageDefinition will prevent the webhook from allowing
// CertificateRequest's usages to be only defined in the CSR, while leaving
@ -145,7 +146,7 @@ func init() {
// To add a new feature, define a key for it above and add it here. The features will be
// available on the cert-manager controller binary.
var defaultCertManagerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{
DisallowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.Beta},
DisallowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.GA},
StableCertificateRequestName: {Default: true, PreRelease: featuregate.Beta},
SecretsFilteredCaching: {Default: true, PreRelease: featuregate.Beta},

1
internal/webhook/feature/features.go
View File

@ -55,6 +55,7 @@ const (
LiteralCertificateSubject featuregate.Feature = "LiteralCertificateSubject"
// Owner: @inteon
// Beta: v1.13
// GA: v1.15
//
// DisallowInsecureCSRUsageDefinition will prevent the webhook from allowing

43
pkg/util/pki/certificatetemplate.go
View File

@ -321,39 +321,22 @@ func CertificateTemplateFromCertificate(crt *v1.Certificate) (*x509.Certificate,
)
}
func makeCertificateTemplateFromCertificateRequestFunc(allowInsecureCSRUsageDefinition bool) func(cr *v1.CertificateRequest) (*x509.Certificate, error) {
return func(cr *v1.CertificateRequest) (*x509.Certificate, error) {
certDuration := apiutil.DefaultCertDuration(cr.Spec.Duration)
keyUsage, extKeyUsage, err := KeyUsagesForCertificateOrCertificateRequest(cr.Spec.Usages, cr.Spec.IsCA)
if err != nil {
return nil, err
}
return CertificateTemplateFromCSRPEM(
cr.Spec.Request,
CertificateTemplateOverrideDuration(certDuration),
CertificateTemplateValidateAndOverrideBasicConstraints(cr.Spec.IsCA, nil), // Override the basic constraints, but make sure they match the constraints in the CSR if present
(func() CertificateTemplateValidatorMutator {
if allowInsecureCSRUsageDefinition && len(cr.Spec.Usages) == 0 {
// If the CertificateRequest does not specify any usages, and the AllowInsecureCSRUsageDefinition
// flag is set, then we allow the usages to be defined solely by the CSR blob, but we still override
// the usages to match the old behavior.
return certificateTemplateOverrideKeyUsages(keyUsage, extKeyUsage)
}
// Override the key usages, but make sure they match the usages in the CSR if present
return CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage)
})(),
)
}
}
// CertificateTemplateFromCertificateRequest will create a x509.Certificate for the given
// CertificateRequest resource
var CertificateTemplateFromCertificateRequest = makeCertificateTemplateFromCertificateRequestFunc(false)
func CertificateTemplateFromCertificateRequest(cr *v1.CertificateRequest) (*x509.Certificate, error) {
certDuration := apiutil.DefaultCertDuration(cr.Spec.Duration)
keyUsage, extKeyUsage, err := KeyUsagesForCertificateOrCertificateRequest(cr.Spec.Usages, cr.Spec.IsCA)
if err != nil {
return nil, err
}
// Deprecated: Use CertificateTemplateFromCertificateRequest instead.
var DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition = makeCertificateTemplateFromCertificateRequestFunc(true)
return CertificateTemplateFromCSRPEM(
cr.Spec.Request,
CertificateTemplateOverrideDuration(certDuration),
CertificateTemplateValidateAndOverrideBasicConstraints(cr.Spec.IsCA, nil), // Override the basic constraints, but make sure they match the constraints in the CSR if present
CertificateTemplateValidateAndOverrideKeyUsages(keyUsage, extKeyUsage), // Override the key usages, but make sure they match the usages in the CSR if present
)
}
// CertificateTemplateFromCertificateSigningRequest will create a x509.Certificate for the given
// CertificateSigningRequest resource

165
test/integration/validation/certificaterequest_test.go
View File

@ -33,7 +33,6 @@ import (
"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"
)
@ -176,170 +175,6 @@ func TestValidationCertificateRequests(t *testing.T) {
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("DisallowInsecureCSRUsageDefinition=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_DisallowInsecureCSRUsageDefinition_false makes sure that the
// validation webhook keeps working as before when the DisallowInsecureCSRUsageDefinition feature
// gate is disabled.
func TestValidationCertificateRequests_DisallowInsecureCSRUsageDefinition_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",
Namespace: "default",
},
Spec: cmapi.CertificateRequestSpec{
Request: mustGenerateCSR(t, &cmapi.Certificate{
Spec: cmapi.CertificateSpec{
DNSNames: []string{"example.com"},
},
}),
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: ptr.To(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: "encoded CSR error: the ExtKeyUsages [ 'client auth' ] do not match the expected ExtKeyUsages [ '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: ptr.To(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()
utilfeature.DefaultMutableFeatureGate.Set("DisallowInsecureCSRUsageDefinition=false")
config, stop := framework.RunControlPlane(t, ctx)
defer stop()