From e612ebfdff22e4bd27ad8345f7c82f074bfedf26 Mon Sep 17 00:00:00 2001 From: wojtekt Date: Tue, 26 Nov 2019 13:29:26 +0100 Subject: [PATCH] Immutable field and validation Upstream-Status: Backport [https://github.com/kubernetes/kubernetes/commit/e612ebfdff22e4bd27ad8345f7c82f074bfedf26] CVE: CVE-2021-25735 #Dependency Patch1 Signed-off-by: Vijay Anusuri --- pkg/apis/core/types.go | 12 + pkg/apis/core/validation/validation.go | 24 +- pkg/apis/core/validation/validation_test.go | 209 ++++++++++++++++-- pkg/features/kube_features.go | 7 + pkg/registry/core/configmap/strategy.go | 28 ++- pkg/registry/core/secret/strategy.go | 17 ++ staging/src/k8s.io/api/core/v1/types.go | 16 ++ .../k8sdeps/transformer/hash/hash.go | 20 +- .../k8sdeps/transformer/hash/hash_test.go | 4 +- .../src/k8s.io/kubectl/pkg/util/hash/hash.go | 20 +- .../k8s.io/kubectl/pkg/util/hash/hash_test.go | 4 +- 11 files changed, 322 insertions(+), 39 deletions(-) diff --git a/pkg/apis/core/types.go b/pkg/apis/core/types.go index 74d22ae973e87..c5ada193effc4 100644 --- a/src/import/pkg/apis/core/types.go +++ b/src/import/pkg/apis/core/types.go @@ -4735,6 +4735,12 @@ type Secret struct { // +optional metav1.ObjectMeta + // Immutable field, if set, ensures that data stored in the Secret cannot + // be updated (only object metadata can be modified). + // This is an alpha field enabled by ImmutableEphemeralVolumes feature gate. + // +optional + Immutable *bool + // Data contains the secret data. Each key must consist of alphanumeric // characters, '-', '_' or '.'. The serialized form of the secret data is a // base64 encoded string, representing the arbitrary (possibly non-string) @@ -4857,6 +4863,12 @@ type ConfigMap struct { // +optional metav1.ObjectMeta + // Immutable field, if set, ensures that data stored in the ConfigMap cannot + // be updated (only object metadata can be modified). + // This is an alpha field enabled by ImmutableEphemeralVolumes feature gate. + // +optional + Immutable *bool + // Data contains the configuration data. // Each key must consist of alphanumeric characters, '-', '_' or '.'. // Values with non-UTF-8 byte sequences must use the BinaryData field. diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 4ad241c745b7d..8e3cfd9d9e423 100644 --- a/src/import/pkg/apis/core/validation/validation.go +++ b/src/import/pkg/apis/core/validation/validation.go @@ -5005,6 +5005,16 @@ func ValidateSecretUpdate(newSecret, oldSecret *core.Secret) field.ErrorList { } allErrs = append(allErrs, ValidateImmutableField(newSecret.Type, oldSecret.Type, field.NewPath("type"))...) + if oldSecret.Immutable != nil && *oldSecret.Immutable { + if !reflect.DeepEqual(newSecret.Immutable, oldSecret.Immutable) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("immutable"), "field is immutable when `immutable` is set")) + } + if !reflect.DeepEqual(newSecret.Data, oldSecret.Data) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("data"), "field is immutable when `immutable` is set")) + } + // We don't validate StringData, as it was already converted back to Data + // before validation is happening. + } allErrs = append(allErrs, ValidateSecret(newSecret)...) return allErrs @@ -5051,8 +5061,20 @@ func ValidateConfigMap(cfg *core.ConfigMap) field.ErrorList { func ValidateConfigMapUpdate(newCfg, oldCfg *core.ConfigMap) field.ErrorList { allErrs := field.ErrorList{} allErrs = append(allErrs, ValidateObjectMetaUpdate(&newCfg.ObjectMeta, &oldCfg.ObjectMeta, field.NewPath("metadata"))...) - allErrs = append(allErrs, ValidateConfigMap(newCfg)...) + if oldCfg.Immutable != nil && *oldCfg.Immutable { + if !reflect.DeepEqual(newCfg.Immutable, oldCfg.Immutable) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("immutable"), "field is immutable when `immutable` is set")) + } + if !reflect.DeepEqual(newCfg.Data, oldCfg.Data) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("data"), "field is immutable when `immutable` is set")) + } + if !reflect.DeepEqual(newCfg.BinaryData, oldCfg.BinaryData) { + allErrs = append(allErrs, field.Forbidden(field.NewPath("binaryData"), "field is immutable when `immutable` is set")) + } + } + + allErrs = append(allErrs, ValidateConfigMap(newCfg)...) return allErrs } diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 8ba68da00fe05..de8c1d49fc196 100644 --- a/src/import/pkg/apis/core/validation/validation_test.go +++ b/src/import/pkg/apis/core/validation/validation_test.go @@ -13117,6 +13117,104 @@ func TestValidateSecret(t *testing.T) { } } +func TestValidateSecretUpdate(t *testing.T) { + validSecret := func() core.Secret { + return core.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "foo", + Namespace: "bar", + ResourceVersion: "20", + }, + Data: map[string][]byte{ + "data-1": []byte("bar"), + }, + } + } + + falseVal := false + trueVal := true + + secret := validSecret() + immutableSecret := validSecret() + immutableSecret.Immutable = &trueVal + mutableSecret := validSecret() + mutableSecret.Immutable = &falseVal + + secretWithData := validSecret() + secretWithData.Data["data-2"] = []byte("baz") + immutableSecretWithData := validSecret() + immutableSecretWithData.Immutable = &trueVal + immutableSecretWithData.Data["data-2"] = []byte("baz") + + secretWithChangedData := validSecret() + secretWithChangedData.Data["data-1"] = []byte("foo") + immutableSecretWithChangedData := validSecret() + immutableSecretWithChangedData.Immutable = &trueVal + immutableSecretWithChangedData.Data["data-1"] = []byte("foo") + + tests := []struct { + name string + oldSecret core.Secret + newSecret core.Secret + valid bool + }{ + { + name: "mark secret immutable", + oldSecret: secret, + newSecret: immutableSecret, + valid: true, + }, + { + name: "revert immutable secret", + oldSecret: immutableSecret, + newSecret: secret, + valid: false, + }, + { + name: "makr immutable secret mutable", + oldSecret: immutableSecret, + newSecret: mutableSecret, + valid: false, + }, + { + name: "add data in secret", + oldSecret: secret, + newSecret: secretWithData, + valid: true, + }, + { + name: "add data in immutable secret", + oldSecret: immutableSecret, + newSecret: immutableSecretWithData, + valid: false, + }, + { + name: "change data in secret", + oldSecret: secret, + newSecret: secretWithChangedData, + valid: true, + }, + { + name: "change data in immutable secret", + oldSecret: immutableSecret, + newSecret: immutableSecretWithChangedData, + valid: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + errs := ValidateSecretUpdate(&tc.newSecret, &tc.oldSecret) + if tc.valid && len(errs) > 0 { + t.Errorf("Unexpected error: %v", errs) + } + if !tc.valid && len(errs) == 0 { + t.Errorf("Unexpected lack of error") + } + }) + } +} + func TestValidateDockerConfigSecret(t *testing.T) { validDockerSecret := func() core.Secret { return core.Secret{ @@ -13731,40 +13829,105 @@ func TestValidateConfigMapUpdate(t *testing.T) { Data: data, } } + validConfigMap := func() core.ConfigMap { + return newConfigMap("1", "validname", "validdns", map[string]string{"key": "value"}) + } - var ( - validConfigMap = newConfigMap("1", "validname", "validns", map[string]string{"key": "value"}) - noVersion = newConfigMap("", "validname", "validns", map[string]string{"key": "value"}) - ) + falseVal := false + trueVal := true + + configMap := validConfigMap() + immutableConfigMap := validConfigMap() + immutableConfigMap.Immutable = &trueVal + mutableConfigMap := validConfigMap() + mutableConfigMap.Immutable = &falseVal + + configMapWithData := validConfigMap() + configMapWithData.Data["key-2"] = "value-2" + immutableConfigMapWithData := validConfigMap() + immutableConfigMapWithData.Immutable = &trueVal + immutableConfigMapWithData.Data["key-2"] = "value-2" + + configMapWithChangedData := validConfigMap() + configMapWithChangedData.Data["key"] = "foo" + immutableConfigMapWithChangedData := validConfigMap() + immutableConfigMapWithChangedData.Immutable = &trueVal + immutableConfigMapWithChangedData.Data["key"] = "foo" + + noVersion := newConfigMap("", "validname", "validns", map[string]string{"key": "value"}) cases := []struct { - name string - newCfg core.ConfigMap - oldCfg core.ConfigMap - isValid bool + name string + newCfg core.ConfigMap + oldCfg core.ConfigMap + valid bool }{ { - name: "valid", - newCfg: validConfigMap, - oldCfg: validConfigMap, - isValid: true, + name: "valid", + newCfg: configMap, + oldCfg: configMap, + valid: true, }, { - name: "invalid", - newCfg: noVersion, - oldCfg: validConfigMap, - isValid: false, + name: "invalid", + newCfg: noVersion, + oldCfg: configMap, + valid: false, + }, + { + name: "mark configmap immutable", + oldCfg: configMap, + newCfg: immutableConfigMap, + valid: true, + }, + { + name: "revert immutable configmap", + oldCfg: immutableConfigMap, + newCfg: configMap, + valid: false, + }, + { + name: "mark immutable configmap mutable", + oldCfg: immutableConfigMap, + newCfg: mutableConfigMap, + valid: false, + }, + { + name: "add data in configmap", + oldCfg: configMap, + newCfg: configMapWithData, + valid: true, + }, + { + name: "add data in immutable configmap", + oldCfg: immutableConfigMap, + newCfg: immutableConfigMapWithData, + valid: false, + }, + { + name: "change data in configmap", + oldCfg: configMap, + newCfg: configMapWithChangedData, + valid: true, + }, + { + name: "change data in immutable configmap", + oldCfg: immutableConfigMap, + newCfg: immutableConfigMapWithChangedData, + valid: false, }, } for _, tc := range cases { - errs := ValidateConfigMapUpdate(&tc.newCfg, &tc.oldCfg) - if tc.isValid && len(errs) > 0 { - t.Errorf("%v: unexpected error: %v", tc.name, errs) - } - if !tc.isValid && len(errs) == 0 { - t.Errorf("%v: unexpected non-error", tc.name) - } + t.Run(tc.name, func(t *testing.T) { + errs := ValidateConfigMapUpdate(&tc.newCfg, &tc.oldCfg) + if tc.valid && len(errs) > 0 { + t.Errorf("Unexpected error: %v", errs) + } + if !tc.valid && len(errs) == 0 { + t.Errorf("Unexpected lack of error") + } + }) } } diff --git a/pkg/features/kube_features.go b/pkg/features/kube_features.go index 309dbb2955663..00da711112d71 100644 --- a/src/import/pkg/features/kube_features.go +++ b/src/import/pkg/features/kube_features.go @@ -548,6 +548,12 @@ const ( // // Enables topology aware service routing ServiceTopology featuregate.Feature = "ServiceTopology" + + // owner: @wojtek-t + // alpha: v1.18 + // + // Enables a feature to make secrets and configmaps data immutable. + ImmutableEphemeralVolumes featuregate.Feature = "ImmutableEphemeralVolumes" ) func init() { @@ -634,6 +640,7 @@ var defaultKubernetesFeatureGates = map[featuregate.Feature]featuregate.FeatureS AllowInsecureBackendProxy: {Default: true, PreRelease: featuregate.Beta}, PodDisruptionBudget: {Default: true, PreRelease: featuregate.Beta}, ServiceTopology: {Default: false, PreRelease: featuregate.Alpha}, + ImmutableEphemeralVolumes: {Default: false, PreRelease: featuregate.Alpha}, // inherited features from generic apiserver, relisted here to get a conflict if it is changed // unintentionally on either side: diff --git a/pkg/registry/core/configmap/strategy.go b/pkg/registry/core/configmap/strategy.go index 4f8bf42e3bd60..d592c181c0c2e 100644 --- a/src/import/pkg/registry/core/configmap/strategy.go +++ b/src/import/pkg/registry/core/configmap/strategy.go @@ -28,9 +28,11 @@ import ( "k8s.io/apiserver/pkg/registry/rest" pkgstorage "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" ) // strategy implements behavior for ConfigMap objects @@ -54,7 +56,8 @@ func (strategy) NamespaceScoped() bool { } func (strategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { - _ = obj.(*api.ConfigMap) + configMap := obj.(*api.ConfigMap) + dropDisabledFields(configMap, nil) } func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { @@ -72,12 +75,9 @@ func (strategy) AllowCreateOnUpdate() bool { } func (strategy) PrepareForUpdate(ctx context.Context, newObj, oldObj runtime.Object) { - _ = oldObj.(*api.ConfigMap) - _ = newObj.(*api.ConfigMap) -} - -func (strategy) AllowUnconditionalUpdate() bool { - return true + oldConfigMap := oldObj.(*api.ConfigMap) + newConfigMap := newObj.(*api.ConfigMap) + dropDisabledFields(newConfigMap, oldConfigMap) } func (strategy) ValidateUpdate(ctx context.Context, newObj, oldObj runtime.Object) field.ErrorList { @@ -86,6 +86,20 @@ func (strategy) ValidateUpdate(ctx context.Context, newObj, oldObj runtime.Objec return validation.ValidateConfigMapUpdate(newCfg, oldCfg) } +func isImmutableInUse(configMap *api.ConfigMap) bool { + return configMap != nil && configMap.Immutable != nil +} + +func dropDisabledFields(configMap *api.ConfigMap, oldConfigMap *api.ConfigMap) { + if !utilfeature.DefaultFeatureGate.Enabled(features.ImmutableEphemeralVolumes) && !isImmutableInUse(oldConfigMap) { + configMap.Immutable = nil + } +} + +func (strategy) AllowUnconditionalUpdate() bool { + return true +} + // GetAttrs returns labels and fields of a given object for filtering purposes. func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) { configMap, ok := obj.(*api.ConfigMap) diff --git a/pkg/registry/core/secret/strategy.go b/pkg/registry/core/secret/strategy.go index 1701805065e6c..0d5908d8975f1 100644 --- a/src/import/pkg/registry/core/secret/strategy.go +++ b/src/import/pkg/registry/core/secret/strategy.go @@ -29,9 +29,11 @@ import ( "k8s.io/apiserver/pkg/registry/rest" pkgstorage "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" + utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/kubernetes/pkg/api/legacyscheme" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/apis/core/validation" + "k8s.io/kubernetes/pkg/features" ) // strategy implements behavior for Secret objects @@ -53,6 +55,8 @@ func (strategy) NamespaceScoped() bool { } func (strategy) PrepareForCreate(ctx context.Context, obj runtime.Object) { + secret := obj.(*api.Secret) + dropDisabledFields(secret, nil) } func (strategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList { @@ -67,12 +71,25 @@ func (strategy) AllowCreateOnUpdate() bool { } func (strategy) PrepareForUpdate(ctx context.Context, obj, old runtime.Object) { + newSecret := obj.(*api.Secret) + oldSecret := old.(*api.Secret) + dropDisabledFields(newSecret, oldSecret) } func (strategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList { return validation.ValidateSecretUpdate(obj.(*api.Secret), old.(*api.Secret)) } +func isImmutableInUse(secret *api.Secret) bool { + return secret != nil && secret.Immutable != nil +} + +func dropDisabledFields(secret *api.Secret, oldSecret *api.Secret) { + if !utilfeature.DefaultFeatureGate.Enabled(features.ImmutableEphemeralVolumes) && !isImmutableInUse(oldSecret) { + secret.Immutable = nil + } +} + func (strategy) AllowUnconditionalUpdate() bool { return true } diff --git a/staging/src/k8s.io/api/core/v1/types.go b/staging/src/k8s.io/api/core/v1/types.go index a78372aeaffa7..1e3aa51730427 100644 --- a/src/import/staging/src/k8s.io/api/core/v1/types.go +++ b/src/import/staging/src/k8s.io/api/core/v1/types.go @@ -5424,6 +5424,14 @@ type Secret struct { // +optional metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` + // Immutable, if set to true, ensures that data stored in the Secret cannot + // be updated (only object metadata can be modified). + // If not set to true, the field can be modified at any time. + // Defaulted to nil. + // This is an alpha field enabled by ImmutableEphemeralVolumes feature gate. + // +optional + Immutable *bool `json:"immutable,omitempty"` + // Data contains the secret data. Each key must consist of alphanumeric // characters, '-', '_' or '.'. The serialized form of the secret data is a // base64 encoded string, representing the arbitrary (possibly non-string) @@ -5557,6 +5565,14 @@ type ConfigMap struct { // +optional metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` + // Immutable, if set to true, ensures that data stored in the ConfigMap cannot + // be updated (only object metadata can be modified). + // If not set to true, the field can be modified at any time. + // Defaulted to nil. + // This is an alpha field enabled by ImmutableEphemeralVolumes feature gate. + // +optional + Immutable *bool `json:"immutable,omitempty"` + // Data contains the configuration data. // Each key must consist of alphanumeric characters, '-', '_' or '.'. // Values with non-UTF-8 byte sequences must use the BinaryData field. diff --git a/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash.go b/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash.go index 17e24ff3e6443..85bf1e731c3fb 100644 --- a/src/import/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash.go +++ b/src/import/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash.go @@ -90,7 +90,14 @@ func SecretHash(sec *v1.Secret) (string, error) { // Data, Kind, and Name are taken into account. func encodeConfigMap(cm *v1.ConfigMap) (string, error) { // json.Marshal sorts the keys in a stable order in the encoding - m := map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data} + m := map[string]interface{}{ + "kind": "ConfigMap", + "name": cm.Name, + "data": cm.Data, + } + if cm.Immutable != nil { + m["immutable"] = *cm.Immutable + } if len(cm.BinaryData) > 0 { m["binaryData"] = cm.BinaryData } @@ -105,7 +112,16 @@ func encodeConfigMap(cm *v1.ConfigMap) (string, error) { // Data, Kind, Name, and Type are taken into account. func encodeSecret(sec *v1.Secret) (string, error) { // json.Marshal sorts the keys in a stable order in the encoding - data, err := json.Marshal(map[string]interface{}{"kind": "Secret", "type": sec.Type, "name": sec.Name, "data": sec.Data}) + m := map[string]interface{}{ + "kind": "Secret", + "type": sec.Type, + "name": sec.Name, + "data": sec.Data, + } + if sec.Immutable != nil { + m["immutable"] = *sec.Immutable + } + data, err := json.Marshal(m) if err != nil { return "", err } diff --git a/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash_test.go b/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash_test.go index 2d336f35a824e..144fe444e4cac 100644 --- a/src/import/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash_test.go +++ b/src/import/staging/src/k8s.io/cli-runtime/pkg/kustomize/k8sdeps/transformer/hash/hash_test.go @@ -178,8 +178,8 @@ not their metadata (e.g. the Data of a ConfigMap, but nothing in ObjectMeta). obj interface{} expect int }{ - {"ConfigMap", v1.ConfigMap{}, 4}, - {"Secret", v1.Secret{}, 5}, + {"ConfigMap", v1.ConfigMap{}, 5}, + {"Secret", v1.Secret{}, 6}, } for _, c := range cases { val := reflect.ValueOf(c.obj) diff --git a/staging/src/k8s.io/kubectl/pkg/util/hash/hash.go b/staging/src/k8s.io/kubectl/pkg/util/hash/hash.go index de0036245d2f1..1b20f384b7098 100644 --- a/src/import/staging/src/k8s.io/kubectl/pkg/util/hash/hash.go +++ b/src/import/staging/src/k8s.io/kubectl/pkg/util/hash/hash.go @@ -56,7 +56,14 @@ func SecretHash(sec *v1.Secret) (string, error) { // Data, Kind, and Name are taken into account. func encodeConfigMap(cm *v1.ConfigMap) (string, error) { // json.Marshal sorts the keys in a stable order in the encoding - m := map[string]interface{}{"kind": "ConfigMap", "name": cm.Name, "data": cm.Data} + m := map[string]interface{}{ + "kind": "ConfigMap", + "name": cm.Name, + "data": cm.Data, + } + if cm.Immutable != nil { + m["immutable"] = *cm.Immutable + } if len(cm.BinaryData) > 0 { m["binaryData"] = cm.BinaryData } @@ -70,8 +77,17 @@ func encodeConfigMap(cm *v1.ConfigMap) (string, error) { // encodeSecret encodes a Secret. // Data, Kind, Name, and Type are taken into account. func encodeSecret(sec *v1.Secret) (string, error) { + m := map[string]interface{}{ + "kind": "Secret", + "type": sec.Type, + "name": sec.Name, + "data": sec.Data, + } + if sec.Immutable != nil { + m["immutable"] = *sec.Immutable + } // json.Marshal sorts the keys in a stable order in the encoding - data, err := json.Marshal(map[string]interface{}{"kind": "Secret", "type": sec.Type, "name": sec.Name, "data": sec.Data}) + data, err := json.Marshal(m) if err != nil { return "", err } diff --git a/staging/src/k8s.io/kubectl/pkg/util/hash/hash_test.go b/staging/src/k8s.io/kubectl/pkg/util/hash/hash_test.go index f527a98a2026c..455459c3b3df5 100644 --- a/src/import/staging/src/k8s.io/kubectl/pkg/util/hash/hash_test.go +++ b/src/import/staging/src/k8s.io/kubectl/pkg/util/hash/hash_test.go @@ -164,8 +164,8 @@ not their metadata (e.g. the Data of a ConfigMap, but nothing in ObjectMeta). obj interface{} expect int }{ - {"ConfigMap", v1.ConfigMap{}, 4}, - {"Secret", v1.Secret{}, 5}, + {"ConfigMap", v1.ConfigMap{}, 5}, + {"Secret", v1.Secret{}, 6}, } for _, c := range cases { val := reflect.ValueOf(c.obj)