Add validation to ensure Order & Challenge fields are immutable

Signed-off-by: James Munnelly <james@munnelly.eu>

deploy/charts/cert-manager/templates/webhook-validating-webhook.yaml

@@ -36,6 +36,16 @@ webhooks:
           - issuers
           - clusterissuers
           - certificaterequests
+      - apiGroups:
+          - "acme.cert-manager.io"
+        apiVersions:
+          - v1alpha2
+        operations:
+          - CREATE
+          - UPDATE
+        resources:
+          - orders
+          - challenges
     failurePolicy: Fail
     sideEffects: None
     clientConfig:

pkg/apis/acme/v1alpha2/types.go

@@ -38,5 +38,6 @@ const (
 )
 
 const (
-	OrderKind = "Order"
+	OrderKind     = "Order"
+	ChallengeKind = "Challenge"
 )

pkg/internal/apis/acme/BUILD.bazel

@@ -37,6 +37,7 @@ filegroup(
         "//pkg/internal/apis/acme/fuzzer:all-srcs",
         "//pkg/internal/apis/acme/install:all-srcs",
         "//pkg/internal/apis/acme/v1alpha2:all-srcs",
+        "//pkg/internal/apis/acme/validation:all-srcs",
     ],
     tags = ["automanaged"],
     visibility = ["//visibility:public"],

pkg/internal/apis/acme/validation/BUILD.bazel

@@ -0,0 +1,44 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "go_default_library",
+    srcs = [
+        "challenge.go",
+        "order.go",
+    ],
+    importpath = "github.com/jetstack/cert-manager/pkg/internal/apis/acme/validation",
+    visibility = ["//pkg:__subpackages__"],
+    deps = [
+        "//pkg/apis/acme/v1alpha2:go_default_library",
+        "@io_k8s_apimachinery//pkg/runtime:go_default_library",
+        "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library",
+    ],
+)
+
+go_test(
+    name = "go_default_test",
+    srcs = [
+        "challenge_test.go",
+        "order_test.go",
+    ],
+    embed = [":go_default_library"],
+    deps = [
+        "//pkg/apis/acme/v1alpha2:go_default_library",
+        "@io_k8s_apimachinery//pkg/util/validation/field:go_default_library",
+        "@io_k8s_utils//pointer:go_default_library",
+    ],
+)
+
+filegroup(
+    name = "package-srcs",
+    srcs = glob(["**"]),
+    tags = ["automanaged"],
+    visibility = ["//visibility:private"],
+)
+
+filegroup(
+    name = "all-srcs",
+    srcs = [":package-srcs"],
+    tags = ["automanaged"],
+    visibility = ["//visibility:public"],
+)

pkg/internal/apis/acme/validation/challenge.go

@@ -0,0 +1,42 @@
+/*
+Copyright 2019 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 (
+	"reflect"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2"
+)
+
+func ValidateChallengeUpdate(oldObj, newObj runtime.Object) field.ErrorList {
+	old, ok := oldObj.(*cmacme.Challenge)
+	new := newObj.(*cmacme.Challenge)
+	// if oldObj is not set, the Update operation is always valid.
+	if !ok || old == nil {
+		return nil
+	}
+
+	el := field.ErrorList{}
+	if !reflect.DeepEqual(old.Spec, new.Spec) {
+		el = append(el, field.Forbidden(field.NewPath("spec"), "challenge spec is immutable after creation"))
+	}
+
+	return el
+}

pkg/internal/apis/acme/validation/challenge_test.go

@@ -0,0 +1,83 @@
+/*
+Copyright 2019 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 (
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2"
+)
+
+func TestValidateChallengeUpdate(t *testing.T) {
+	scenarios := map[string]struct {
+		old, new *cmacme.Challenge
+		errs     []*field.Error
+	}{
+		"allows setting challenge spec for the first time": {
+			new: &cmacme.Challenge{
+				Spec: cmacme.ChallengeSpec{
+					URL: "testurl",
+				},
+			},
+		},
+		"disallow updating challenge spec": {
+			old: &cmacme.Challenge{
+				Spec: cmacme.ChallengeSpec{
+					URL: "testurl",
+				},
+			},
+			new: &cmacme.Challenge{
+				Spec: cmacme.ChallengeSpec{
+					URL: "newtesturl",
+				},
+			},
+			errs: []*field.Error{
+				field.Forbidden(field.NewPath("spec"), "challenge spec is immutable after creation"),
+			},
+		},
+		"allow updating challenge spec if no changes are made": {
+			old: &cmacme.Challenge{
+				Spec: cmacme.ChallengeSpec{
+					URL: "testurl",
+				},
+			},
+			new: &cmacme.Challenge{
+				Spec: cmacme.ChallengeSpec{
+					URL: "testurl",
+				},
+			},
+		},
+	}
+	for n, s := range scenarios {
+		t.Run(n, func(t *testing.T) {
+			errs := ValidateChallengeUpdate(s.old, s.new)
+			if len(errs) != len(s.errs) {
+				t.Errorf("Expected %v but got %v", s.errs, errs)
+				return
+			}
+			for i, e := range errs {
+				expectedErr := s.errs[i]
+				if !reflect.DeepEqual(e, expectedErr) {
+					t.Errorf("Expected %v but got %v", expectedErr, e)
+				}
+			}
+		})
+	}
+}

pkg/internal/apis/acme/validation/order.go

@@ -0,0 +1,120 @@
+/*
+Copyright 2019 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"
+
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2"
+)
+
+func ValidateOrderUpdate(oldObj, newObj runtime.Object) field.ErrorList {
+	old, ok := oldObj.(*cmacme.Order)
+	new := newObj.(*cmacme.Order)
+	// if oldObj is not set, the Update operation is always valid.
+	if !ok || old == nil {
+		return nil
+	}
+
+	el := field.ErrorList{}
+	el = append(el, ValidateOrderSpecUpdate(old.Spec, new.Spec, field.NewPath("spec"))...)
+	el = append(el, ValidateOrderStatusUpdate(old.Status, new.Status, field.NewPath("status"))...)
+	return el
+}
+
+func ValidateOrderSpecUpdate(old, new cmacme.OrderSpec, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	if len(old.CSR) > 0 && bytes.Compare(old.CSR, new.CSR) != 0 {
+		el = append(el, field.Forbidden(fldPath.Child("csr"), "field is immutable once set"))
+	}
+	return el
+}
+
+func ValidateOrderStatusUpdate(old, new cmacme.OrderStatus, fldPath *field.Path) field.ErrorList {
+	el := field.ErrorList{}
+	// once the order URL has been set, it cannot be changed
+	if old.URL != "" && old.URL != new.URL {
+		el = append(el, field.Forbidden(fldPath.Child("url"), "field is immutable once set"))
+	}
+	// once the FinalizeURL has been set, it cannot be changed
+	if old.FinalizeURL != "" && old.FinalizeURL != new.FinalizeURL {
+		el = append(el, field.Forbidden(fldPath.Child("finalizeURL"), "field is immutable once set"))
+	}
+	// once the Certificate has been issued, it cannot be changed
+	if len(old.Certificate) > 0 && bytes.Compare(old.Certificate, new.Certificate) != 0 {
+		el = append(el, field.Forbidden(fldPath.Child("certificate"), "field is immutable once set"))
+	}
+
+	if len(old.Authorizations) > 0 {
+		fldPath := fldPath.Child("authorizations")
+
+		// once at least one Authorization has been inserted, no more can be added
+		// or deleted from the Order
+		if len(old.Authorizations) != len(new.Authorizations) {
+			el = append(el, field.Forbidden(fldPath, "field is immutable once set"))
+		}
+
+		// here we know that len(old) == len(new), so we proceed to validate
+		// the updates that the user requested on each Authorization.
+		// fields on Authorization's cannot be changed after being set from
+		// their zero value.
+		for i := range old.Authorizations {
+			fldPath := fldPath.Index(i)
+			old := old.Authorizations[i]
+			new := new.Authorizations[i]
+			if old.URL != "" && old.URL != new.URL {
+				el = append(el, field.Forbidden(fldPath.Child("url"), "field is immutable once set"))
+			}
+			if old.Identifier != "" && old.Identifier != new.Identifier {
+				el = append(el, field.Forbidden(fldPath.Child("identifier"), "field is immutable once set"))
+			}
+			// don't allow the value of the Wildcard field to change unless the
+			// old value is nil
+			if old.Wildcard != nil && (new.Wildcard == nil || *old.Wildcard != *new.Wildcard) {
+				el = append(el, field.Forbidden(fldPath.Child("wildcard"), "field is immutable once set"))
+			}
+
+			if len(old.Challenges) > 0 {
+				fldPath := fldPath.Child("challenges")
+				if len(old.Challenges) != len(new.Challenges) {
+					el = append(el, field.Forbidden(fldPath, "field is immutable once set"))
+				}
+
+				for i := range old.Challenges {
+					fldPath := fldPath.Index(i)
+					old := old.Challenges[i]
+					new := new.Challenges[i]
+
+					if old.URL != "" && old.URL != new.URL {
+						el = append(el, field.Forbidden(fldPath.Child("url"), "field is immutable once set"))
+					}
+					if old.Type != "" && old.Type != new.Type {
+						el = append(el, field.Forbidden(fldPath.Child("type"), "field is immutable once set"))
+					}
+					if old.Token != "" && old.Token != new.Token {
+						el = append(el, field.Forbidden(fldPath.Child("token"), "field is immutable once set"))
+					}
+				}
+			}
+		}
+	}
+
+	return el
+}

pkg/internal/apis/acme/validation/order_test.go

@@ -0,0 +1,226 @@
+/*
+Copyright 2019 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 (
+	"k8s.io/utils/pointer"
+	"reflect"
+	"testing"
+
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2"
+)
+
+type testValue string
+
+const (
+	testValueNone      = ""
+	testValueOptionOne = "one"
+	testValueOptionTwo = "two"
+)
+
+// testImmutableOrderField will test that the field at path fldPath does
+// not allow changes after being set, but does allow changes if the old field
+// is not set.
+func testImmutableOrderField(t *testing.T, fldPath *field.Path, setter func(*cmacme.Order, testValue)) {
+	t.Run("should reject updates to "+fldPath.String(), func(t *testing.T) {
+		expectedErrs := []*field.Error{
+			field.Forbidden(fldPath, "field is immutable once set"),
+		}
+		old := &cmacme.Order{}
+		new := &cmacme.Order{}
+		setter(old, testValueOptionOne)
+		setter(new, testValueOptionTwo)
+		errs := ValidateOrderUpdate(old, new)
+		if len(errs) != len(expectedErrs) {
+			t.Errorf("Expected %v but got %v", expectedErrs, errs)
+			return
+		}
+		for i, e := range errs {
+			expectedErr := expectedErrs[i]
+			if !reflect.DeepEqual(e, expectedErr) {
+				t.Errorf("Expected %v but got %v", expectedErr, e)
+			}
+		}
+	})
+	t.Run("should allow updates to "+fldPath.String()+" if not already set", func(t *testing.T) {
+		expectedErrs := []*field.Error{}
+		old := &cmacme.Order{}
+		new := &cmacme.Order{}
+		setter(old, testValueNone)
+		setter(new, testValueOptionOne)
+		errs := ValidateOrderUpdate(old, new)
+		if len(errs) != len(expectedErrs) {
+			t.Errorf("Expected %v but got %v", expectedErrs, errs)
+			return
+		}
+		for i, e := range errs {
+			expectedErr := expectedErrs[i]
+			if !reflect.DeepEqual(e, expectedErr) {
+				t.Errorf("Expected %v but got %v", expectedErr, e)
+			}
+		}
+	})
+}
+
+func TestValidateCertificateUpdate(t *testing.T) {
+	authorizationsFldPath := field.NewPath("status", "authorizations")
+	challengesFldPath := authorizationsFldPath.Index(0).Child("challenges")
+
+	testImmutableOrderField(t, field.NewPath("spec", "csr"), func(o *cmacme.Order, s testValue) {
+		if s == testValueNone {
+			o.Spec.CSR = nil
+		}
+		o.Spec.CSR = []byte(s)
+	})
+	testImmutableOrderField(t, field.NewPath("status", "url"), func(o *cmacme.Order, s testValue) {
+		o.Status.URL = string(s)
+	})
+	testImmutableOrderField(t, field.NewPath("status", "finalizeURL"), func(o *cmacme.Order, s testValue) {
+		o.Status.FinalizeURL = string(s)
+	})
+	testImmutableOrderField(t, field.NewPath("status", "certificate"), func(o *cmacme.Order, s testValue) {
+		if s == testValueNone {
+			o.Status.Certificate = nil
+		}
+		o.Status.Certificate = []byte(s)
+	})
+	testImmutableOrderField(t, authorizationsFldPath, func(o *cmacme.Order, s testValue) {
+		switch s {
+		case testValueNone:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{}
+		case testValueOptionOne:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{},
+			}
+		case testValueOptionTwo:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{},
+				{},
+			}
+		}
+	})
+	testImmutableOrderField(t, authorizationsFldPath.Index(0).Child("url"), func(o *cmacme.Order, s testValue) {
+		o.Status.Authorizations = []cmacme.ACMEAuthorization{
+			{URL: string(s)},
+		}
+	})
+	testImmutableOrderField(t, authorizationsFldPath.Index(0).Child("identifier"), func(o *cmacme.Order, s testValue) {
+		o.Status.Authorizations = []cmacme.ACMEAuthorization{
+			{Identifier: string(s)},
+		}
+	})
+	testImmutableOrderField(t, authorizationsFldPath.Index(0).Child("wildcard"), func(o *cmacme.Order, s testValue) {
+		switch s {
+		case testValueNone:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{Wildcard: nil},
+			}
+		case testValueOptionOne:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{Wildcard: pointer.BoolPtr(false)},
+			}
+		case testValueOptionTwo:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{Wildcard: pointer.BoolPtr(true)},
+			}
+		}
+	})
+	testImmutableOrderField(t, challengesFldPath.Index(0).Child("url"), func(o *cmacme.Order, s testValue) {
+		o.Status.Authorizations = []cmacme.ACMEAuthorization{
+			{
+				Challenges: []cmacme.ACMEChallenge{
+					{URL: string(s)},
+				},
+			},
+		}
+	})
+	testImmutableOrderField(t, challengesFldPath.Index(0).Child("token"), func(o *cmacme.Order, s testValue) {
+		o.Status.Authorizations = []cmacme.ACMEAuthorization{
+			{
+				Challenges: []cmacme.ACMEChallenge{
+					{Token: string(s)},
+				},
+			},
+		}
+	})
+	testImmutableOrderField(t, challengesFldPath.Index(0).Child("type"), func(o *cmacme.Order, s testValue) {
+		o.Status.Authorizations = []cmacme.ACMEAuthorization{
+			{
+				Challenges: []cmacme.ACMEChallenge{
+					{Type: cmacme.ACMEChallengeType(s)},
+				},
+			},
+		}
+	})
+	testImmutableOrderField(t, challengesFldPath, func(o *cmacme.Order, s testValue) {
+		switch s {
+		case testValueNone:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{
+					Challenges: []cmacme.ACMEChallenge{},
+				},
+			}
+		case testValueOptionOne:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{
+					Challenges: []cmacme.ACMEChallenge{
+						{},
+					},
+				},
+			}
+		case testValueOptionTwo:
+			o.Status.Authorizations = []cmacme.ACMEAuthorization{
+				{
+					Challenges: []cmacme.ACMEChallenge{
+						{},
+						{},
+					},
+				},
+			}
+		}
+	})
+
+	scenarios := map[string]struct {
+		old, new *cmacme.Order
+		errs     []*field.Error
+	}{
+		"allows all updates if old is nil": {
+			new: &cmacme.Order{
+				Spec: cmacme.OrderSpec{
+					CSR: []byte("testing"),
+				},
+			},
+		},
+	}
+	for n, s := range scenarios {
+		t.Run(n, func(t *testing.T) {
+			errs := ValidateOrderUpdate(s.old, s.new)
+			if len(errs) != len(s.errs) {
+				t.Errorf("Expected %v but got %v", s.errs, errs)
+				return
+			}
+			for i, e := range errs {
+				expectedErr := s.errs[i]
+				if !reflect.DeepEqual(e, expectedErr) {
+					t.Errorf("Expected %v but got %v", expectedErr, e)
+				}
+			}
+		})
+	}
+}

pkg/webhook/BUILD.bazel

@@ -9,8 +9,10 @@ go_library(
     importpath = "github.com/jetstack/cert-manager/pkg/webhook",
     visibility = ["//visibility:public"],
     deps = [
+        "//pkg/apis/acme/v1alpha2:go_default_library",
         "//pkg/apis/certmanager/v1alpha2:go_default_library",
         "//pkg/internal/apis/acme/install:go_default_library",
+        "//pkg/internal/apis/acme/validation:go_default_library",
         "//pkg/internal/apis/certmanager/install:go_default_library",
         "//pkg/internal/apis/certmanager/validation:go_default_library",
         "//pkg/internal/apis/meta/install:go_default_library",

pkg/webhook/certmanager.go

@@ -19,7 +19,9 @@ package webhook
 import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
+	cmacme "github.com/jetstack/cert-manager/pkg/apis/acme/v1alpha2"
 	cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1alpha2"
+	acmeval "github.com/jetstack/cert-manager/pkg/internal/apis/acme/validation"
 	"github.com/jetstack/cert-manager/pkg/internal/apis/certmanager/validation"
 	"github.com/jetstack/cert-manager/pkg/webhook/handlers"
 )
@@ -29,6 +31,8 @@ var Validators = map[schema.GroupKind]handlers.Validator{
 	gk(cmapi.SchemeGroupVersion, cmapi.CertificateRequestKind): certificateRequestValidator,
 	gk(cmapi.SchemeGroupVersion, cmapi.IssuerKind):             issuerValidator,
 	gk(cmapi.SchemeGroupVersion, cmapi.ClusterIssuerKind):      clusterIssuerValidator,
+	gk(cmacme.SchemeGroupVersion, cmacme.OrderKind):            orderValidator,
+	gk(cmacme.SchemeGroupVersion, cmacme.ChallengeKind):        challengeValidator,
 }
 
 var (
@@ -36,6 +40,8 @@ var (
 	certificateRequestValidator = handlers.ValidatorFunc(&cmapi.CertificateRequest{}, validation.ValidateCertificateRequest, nil)
 	issuerValidator             = handlers.ValidatorFunc(&cmapi.Issuer{}, validation.ValidateIssuer, nil)
 	clusterIssuerValidator      = handlers.ValidatorFunc(&cmapi.ClusterIssuer{}, validation.ValidateClusterIssuer, nil)
+	orderValidator              = handlers.ValidatorFunc(&cmacme.Order{}, nil, acmeval.ValidateOrderUpdate)
+	challengeValidator          = handlers.ValidatorFunc(&cmacme.Challenge{}, nil, acmeval.ValidateChallengeUpdate)
 )
 
 func gk(gv schema.GroupVersion, kind string) schema.GroupKind {