Merge pull request #6199 from inteon/add_validation_to_pki

Add validation to pki CertificateTemplate functions
cert-manager · Jul 7, 2023 · 843deed · 843deed
2 parents 914944c + dcf3c99
commit 843deed
Show file tree

Hide file tree

Showing 24 changed files with 209 additions and 60 deletions.
diff --git a/LICENSES b/LICENSES
@@ -122,6 +122,7 @@ go.uber.org/atomic,https://github.com/uber-go/atomic/blob/v1.9.0/LICENSE.txt,MIT
 go.uber.org/multierr,https://github.com/uber-go/multierr/blob/v1.6.0/LICENSE.txt,MIT
 go.uber.org/zap,https://github.com/uber-go/zap/blob/v1.24.0/LICENSE.txt,MIT
 golang.org/x/crypto,https://cs.opensource.google/go/x/crypto/+/v0.6.0:LICENSE,BSD-3-Clause
+golang.org/x/exp,https://cs.opensource.google/go/x/exp/+/2e198f4a:LICENSE,BSD-3-Clause
 golang.org/x/net,https://cs.opensource.google/go/x/net/+/v0.10.0:LICENSE,BSD-3-Clause
 golang.org/x/oauth2,https://cs.opensource.google/go/x/oauth2/+/v0.5.0:LICENSE,BSD-3-Clause
 golang.org/x/sync,https://cs.opensource.google/go/x/sync/+/v0.2.0:LICENSE,BSD-3-Clause

diff --git a/cmd/controller/LICENSES b/cmd/controller/LICENSES
@@ -111,6 +111,7 @@ go.uber.org/atomic,https://github.com/uber-go/atomic/blob/v1.9.0/LICENSE.txt,MIT
 go.uber.org/multierr,https://github.com/uber-go/multierr/blob/v1.6.0/LICENSE.txt,MIT
 go.uber.org/zap,https://github.com/uber-go/zap/blob/v1.24.0/LICENSE.txt,MIT
 golang.org/x/crypto,https://cs.opensource.google/go/x/crypto/+/v0.6.0:LICENSE,BSD-3-Clause
+golang.org/x/exp,https://cs.opensource.google/go/x/exp/+/2e198f4a:LICENSE,BSD-3-Clause
 golang.org/x/net,https://cs.opensource.google/go/x/net/+/v0.10.0:LICENSE,BSD-3-Clause
 golang.org/x/oauth2,https://cs.opensource.google/go/x/oauth2/+/v0.5.0:LICENSE,BSD-3-Clause
 golang.org/x/sync/errgroup,https://cs.opensource.google/go/x/sync/+/v0.2.0:LICENSE,BSD-3-Clause

diff --git a/cmd/controller/go.mod b/cmd/controller/go.mod
@@ -121,6 +121,7 @@ require (
 	go.uber.org/multierr v1.6.0 // indirect
 	go.uber.org/zap v1.24.0 // indirect
 	golang.org/x/crypto v0.6.0 // indirect
+	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect
 	golang.org/x/mod v0.10.0 // indirect
 	golang.org/x/net v0.10.0 // indirect
 	golang.org/x/oauth2 v0.5.0 // indirect

diff --git a/cmd/controller/go.sum b/cmd/controller/go.sum
@@ -578,6 +578,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 h1:k/i9J1pBpvlfR+9QsetwPyERsqu1GIbi967PQMq3Ivc=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=

diff --git a/cmd/ctl/LICENSES b/cmd/ctl/LICENSES
@@ -110,6 +110,7 @@ go.uber.org/atomic,https://github.com/uber-go/atomic/blob/v1.9.0/LICENSE.txt,MIT
 go.uber.org/multierr,https://github.com/uber-go/multierr/blob/v1.6.0/LICENSE.txt,MIT
 go.uber.org/zap,https://github.com/uber-go/zap/blob/v1.24.0/LICENSE.txt,MIT
 golang.org/x/crypto,https://cs.opensource.google/go/x/crypto/+/v0.6.0:LICENSE,BSD-3-Clause
+golang.org/x/exp,https://cs.opensource.google/go/x/exp/+/2e198f4a:LICENSE,BSD-3-Clause
 golang.org/x/net,https://cs.opensource.google/go/x/net/+/v0.10.0:LICENSE,BSD-3-Clause
 golang.org/x/oauth2,https://cs.opensource.google/go/x/oauth2/+/v0.5.0:LICENSE,BSD-3-Clause
 golang.org/x/sync,https://cs.opensource.google/go/x/sync/+/v0.2.0:LICENSE,BSD-3-Clause

diff --git a/cmd/webhook/LICENSES b/cmd/webhook/LICENSES
@@ -54,6 +54,7 @@ go.uber.org/atomic,https://github.com/uber-go/atomic/blob/v1.9.0/LICENSE.txt,MIT
 go.uber.org/multierr,https://github.com/uber-go/multierr/blob/v1.6.0/LICENSE.txt,MIT
 go.uber.org/zap,https://github.com/uber-go/zap/blob/v1.24.0/LICENSE.txt,MIT
 golang.org/x/crypto/md4,https://cs.opensource.google/go/x/crypto/+/v0.6.0:LICENSE,BSD-3-Clause
+golang.org/x/exp,https://cs.opensource.google/go/x/exp/+/2e198f4a:LICENSE,BSD-3-Clause
 golang.org/x/net,https://cs.opensource.google/go/x/net/+/v0.10.0:LICENSE,BSD-3-Clause
 golang.org/x/oauth2,https://cs.opensource.google/go/x/oauth2/+/v0.5.0:LICENSE,BSD-3-Clause
 golang.org/x/sync/errgroup,https://cs.opensource.google/go/x/sync/+/v0.2.0:LICENSE,BSD-3-Clause

diff --git a/cmd/webhook/go.mod b/cmd/webhook/go.mod
@@ -64,6 +64,7 @@ require (
 	go.uber.org/multierr v1.6.0 // indirect
 	go.uber.org/zap v1.24.0 // indirect
 	golang.org/x/crypto v0.6.0 // indirect
+	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 // indirect
 	golang.org/x/net v0.10.0 // indirect
 	golang.org/x/oauth2 v0.5.0 // indirect
 	golang.org/x/sync v0.2.0 // indirect

diff --git a/cmd/webhook/go.sum b/cmd/webhook/go.sum
@@ -309,6 +309,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 h1:k/i9J1pBpvlfR+9QsetwPyERsqu1GIbi967PQMq3Ivc=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=

diff --git a/go.mod b/go.mod
@@ -36,6 +36,7 @@ require (
 	github.com/spf13/pflag v1.0.5
 	github.com/stretchr/testify v1.8.2
 	golang.org/x/crypto v0.6.0
+	golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1
 	golang.org/x/oauth2 v0.5.0
 	golang.org/x/sync v0.2.0
 	gomodules.xyz/jsonpatch/v2 v2.3.0

diff --git a/go.sum b/go.sum
@@ -619,6 +619,8 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0
 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4=
 golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM=
 golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1 h1:k/i9J1pBpvlfR+9QsetwPyERsqu1GIbi967PQMq3Ivc=
+golang.org/x/exp v0.0.0-20230522175609-2e198f4a06a1/go.mod h1:V1LtkGg67GoY2N1AnLN78QLrzxkLyJw7RJb1gzOOz9w=
 golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js=
 golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0=
 golang.org/x/lint v0.0.0-20181026193005-c67002cb31c3/go.mod h1:UVdnD1Gm6xHRNCYTkRU2/jEulfH38KcIWyp/GAMgvoE=

diff --git a/internal/controller/feature/features.go b/internal/controller/feature/features.go
@@ -85,6 +85,13 @@ const (
 	// they know cert-manager will need access to to speed up issuance.
 	// See https://github.com/cert-manager/cert-manager/blob/master/design/20221205-memory-management.md
 	SecretsFilteredCaching featuregate.Feature = "SecretsFilteredCaching"
+
+	// 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() {
@@ -95,6 +102,8 @@ 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{
+	DontAllowInsecureCSRUsageDefinition: {Default: true, PreRelease: featuregate.GA},
+
 	ValidateCAA: {Default: false, PreRelease: featuregate.Alpha},
 	ExperimentalCertificateSigningRequestControllers: {Default: false, PreRelease: featuregate.Alpha},
 	ExperimentalGatewayAPISupport:                    {Default: false, PreRelease: featuregate.Alpha},

diff --git a/pkg/controller/certificaterequests/acme/acme_test.go b/pkg/controller/certificaterequests/acme/acme_test.go
@@ -172,8 +172,8 @@ func TestSign(t *testing.T) {
 	template2, err := pki.CertificateTemplateFromCSRPEM(
 		generateCSR(t, sk2, "example.com", "example.com", "foo.com"),
 		pki.CertificateTemplateOverrideDuration(time.Hour),
-		pki.CertificateTemplateOverrideBasicConstraints(false, nil),
-		pki.CertificateTemplateOverrideKeyUsages(0, nil),
+		pki.CertificateTemplateValidateAndOverrideBasicConstraints(false, nil),
+		pki.CertificateTemplateValidateAndOverrideKeyUsages(0, nil),
 	)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/controller/certificaterequests/ca/ca.go b/pkg/controller/certificaterequests/ca/ca.go
@@ -24,6 +24,7 @@ import (
 
 	k8sErrors "k8s.io/apimachinery/pkg/api/errors"
 
+	"github.com/cert-manager/cert-manager/internal/controller/feature"
 	internalinformers "github.com/cert-manager/cert-manager/internal/informers"
 	apiutil "github.com/cert-manager/cert-manager/pkg/api/util"
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -33,6 +34,7 @@ import (
 	issuerpkg "github.com/cert-manager/cert-manager/pkg/issuer"
 	logf "github.com/cert-manager/cert-manager/pkg/logs"
 	cmerrors "github.com/cert-manager/cert-manager/pkg/util/errors"
+	utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
 	"github.com/cert-manager/cert-manager/pkg/util/kube"
 	"github.com/cert-manager/cert-manager/pkg/util/pki"
 )
@@ -66,11 +68,17 @@ func init() {
 
 func NewCA(ctx *controllerpkg.Context) certificaterequests.Issuer {
 	return &CA{
-		issuerOptions:     ctx.IssuerOptions,
-		secretsLister:     ctx.KubeSharedInformerFactory.Secrets().Lister(),
-		reporter:          crutil.NewReporter(ctx.Clock, ctx.Recorder),
-		templateGenerator: pki.CertificateTemplateFromCertificateRequest,
-		signingFn:         pki.SignCSRTemplate,
+		issuerOptions: ctx.IssuerOptions,
+		secretsLister: ctx.KubeSharedInformerFactory.Secrets().Lister(),
+		reporter:      crutil.NewReporter(ctx.Clock, ctx.Recorder),
+		templateGenerator: func(cr *cmapi.CertificateRequest) (*x509.Certificate, error) {
+			if !utilfeature.DefaultMutableFeatureGate.Enabled(feature.DontAllowInsecureCSRUsageDefinition) {
+				return pki.DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition(cr)
+			}
+
+			return pki.CertificateTemplateFromCertificateRequest(cr)
+		},
+		signingFn: pki.SignCSRTemplate,
 	}
 }
 

diff --git a/pkg/controller/certificaterequests/selfsigned/selfsigned.go b/pkg/controller/certificaterequests/selfsigned/selfsigned.go
@@ -29,6 +29,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/client-go/util/workqueue"
 
+	"github.com/cert-manager/cert-manager/internal/controller/feature"
 	internalinformers "github.com/cert-manager/cert-manager/internal/informers"
 	apiutil "github.com/cert-manager/cert-manager/pkg/api/util"
 	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -38,6 +39,7 @@ import (
 	"github.com/cert-manager/cert-manager/pkg/issuer"
 	logf "github.com/cert-manager/cert-manager/pkg/logs"
 	cmerrors "github.com/cert-manager/cert-manager/pkg/util/errors"
+	utilfeature "github.com/cert-manager/cert-manager/pkg/util/feature"
 	"github.com/cert-manager/cert-manager/pkg/util/kube"
 	"github.com/cert-manager/cert-manager/pkg/util/pki"
 	"github.com/go-logr/logr"
@@ -147,7 +149,12 @@ func (s *SelfSigned) Sign(ctx context.Context, cr *cmapi.CertificateRequest, iss
 		return nil, err
 	}
 
-	template, err := pki.CertificateTemplateFromCertificateRequest(cr)
+	var template *x509.Certificate
+	if !utilfeature.DefaultMutableFeatureGate.Enabled(feature.DontAllowInsecureCSRUsageDefinition) {
+		template, err = pki.DeprecatedCertificateTemplateFromCertificateRequestAndAllowInsecureCSRUsageDefinition(cr)
+	} else {
+		template, err = pki.CertificateTemplateFromCertificateRequest(cr)
+	}
 	if err != nil {
 		message := "Error generating certificate template"
 		s.reporter.Failed(cr, err, "ErrorGenerating", message)

diff --git a/pkg/controller/certificatesigningrequests/venafi/venafi_test.go b/pkg/controller/certificatesigningrequests/venafi/venafi_test.go
@@ -72,8 +72,8 @@ func TestProcessItem(t *testing.T) {
 	rootTmpl, err := pki.CertificateTemplateFromCSRPEM(
 		rootCSRPEM,
 		pki.CertificateTemplateOverrideDuration(time.Hour),
-		pki.CertificateTemplateOverrideBasicConstraints(true, nil),
-		pki.CertificateTemplateOverrideKeyUsages(0, nil),
+		pki.CertificateTemplateValidateAndOverrideBasicConstraints(true, nil),
+		pki.CertificateTemplateValidateAndOverrideKeyUsages(0, nil),
 	)
 	if err != nil {
 		t.Fatal(err)
@@ -92,8 +92,8 @@ func TestProcessItem(t *testing.T) {
 	leafTmpl, err := pki.CertificateTemplateFromCSRPEM(
 		leafCSRPEM,
 		pki.CertificateTemplateOverrideDuration(time.Hour),
-		pki.CertificateTemplateOverrideBasicConstraints(false, nil),
-		pki.CertificateTemplateOverrideKeyUsages(0, nil),
+		pki.CertificateTemplateValidateAndOverrideBasicConstraints(false, nil),
+		pki.CertificateTemplateValidateAndOverrideKeyUsages(0, nil),
 	)
 	if err != nil {
 		t.Fatal(err)

diff --git a/pkg/issuer/venafi/client/request.go b/pkg/issuer/venafi/client/request.go
@@ -83,12 +83,7 @@ func (v *Venafi) buildVReq(csrPEM []byte, duration time.Duration, customFields [
 		return nil, err
 	}
 
-	tmpl, err := pki.CertificateTemplateFromCSRPEM(
-		csrPEM,
-		pki.CertificateTemplateOverrideDuration(duration),
-		pki.CertificateTemplateOverrideBasicConstraints(false, nil),
-		pki.CertificateTemplateOverrideKeyUsages(0, nil),
-	)
+	tmpl, err := pki.CertificateTemplateFromCSRPEM(csrPEM)
 	if err != nil {
 		return nil, err
 	}