Input validation

PoC 1: Attempt to create a Pod with a negative terminationGracePeriodSeconds (TGPS) via kubectl. This should be rejected by the API server due to validation that TGPS must be >= 0. kubectl apply -f - << 'YAML' apiVersion: v1 kind: Pod metadata: name: test-neg-tgps spec: terminationGracePeriodSeconds: -1 containers: - name: pause image: busybox command: ["sh", "-c", "sleep 3600"] YAML Expected: Error from server (Invalid): spec.terminationGracePeriodSeconds: Invalid value: -1: must be greater than or equal to 0 PoC 2 (demonstrates storage-level bypass risk): If an attacker can bypass admission validation and mutate etcd directly, they could insert a Pod object with terminationGracePeriodSeconds: -1. After this patch, decoding from storage should clamp to 1 when read back by the API server or kubelet, preventing the presence of an invalid negative value in runtime Pod specs but proving that data could exist in etcd prior to normalization. Steps (conceptual): 1) Identify the Pod key in etcd (e.g., /registry/pods/{namespace}/{name}). 2) Retrieve the current value and modify the internal Pod object so that terminationGracePeriodSeconds = -1. 3) Persist the updated object back to etcd. 4) Retrieve the Pod via kubectl get pod <name> -o json and observe that the decoding/defaulting path normalizes terminationGracePeriodSeconds to 1. Note: This PoC demonstrates potential exposure if etcd is directly writable by an attacker or if there is an alternative path bypassing admission validation. The patch mitigates this by validating TGPS and by ensuring decode-defaulting clamps negative values to 1 during storage-to-object transformation.

diff --git a/pkg/apis/core/v1/conversion.go b/pkg/apis/core/v1/conversion.go index 096bf6a40bfdb..7f3c35de86c4e 100644 --- a/pkg/apis/core/v1/conversion.go +++ b/pkg/apis/core/v1/conversion.go @@ -20,8 +20,6 @@ import ( "fmt" "reflect" - "k8s.io/utils/ptr" - v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/conversion" @@ -292,12 +290,6 @@ func Convert_v1_Pod_To_core_Pod(in *v1.Pod, out *core.Pod, s conversion.Scope) e // drop init container annotations so they don't show up as differences when receiving requests from old clients out.Annotations = dropInitContainerAnnotations(out.Annotations) - - // Forcing the value of TerminationGracePeriodSeconds to 1 if it is negative. - // Just for Pod, not for PodSpec, because we don't want to change the behavior of the PodTemplate. - if in.Spec.TerminationGracePeriodSeconds != nil && *in.Spec.TerminationGracePeriodSeconds < 0 { - out.Spec.TerminationGracePeriodSeconds = ptr.To[int64](1) - } return nil } @@ -309,12 +301,6 @@ func Convert_core_Pod_To_v1_Pod(in *core.Pod, out *v1.Pod, s conversion.Scope) e // drop init container annotations so they don't take effect on legacy kubelets. // remove this once the oldest supported kubelet no longer honors the annotations over the field. out.Annotations = dropInitContainerAnnotations(out.Annotations) - - // Forcing the value of TerminationGracePeriodSeconds to 1 if it is negative. - // Just for Pod, not for PodSpec, because we don't want to change the behavior of the PodTemplate. - if in.Spec.TerminationGracePeriodSeconds != nil && *in.Spec.TerminationGracePeriodSeconds < 0 { - out.Spec.TerminationGracePeriodSeconds = ptr.To[int64](1) - } return nil } diff --git a/pkg/apis/core/v1/conversion_test.go b/pkg/apis/core/v1/conversion_test.go index b76e03348fd52..9c9b4c66a3c33 100644 --- a/pkg/apis/core/v1/conversion_test.go +++ b/pkg/apis/core/v1/conversion_test.go @@ -710,7 +710,9 @@ func TestConvert_v1_Pod_To_core_Pod(t *testing.T) { }, wantOut: &core.Pod{ Spec: core.PodSpec{ - TerminationGracePeriodSeconds: ptr.To[int64](1), + // This is impossible in practice because defaulting will coherce a negative value to 1. + // We test it here to ensure faithful conversion. + TerminationGracePeriodSeconds: ptr.To[int64](-1), }, }, }, @@ -749,7 +751,9 @@ func TestConvert_core_Pod_To_v1_Pod(t *testing.T) { }, wantOut: &v1.Pod{ Spec: v1.PodSpec{ - TerminationGracePeriodSeconds: ptr.To[int64](1), + // This is impossible in practice because defaulting will coherce a negative value to 1. + // We test it here to ensure faithful conversion. + TerminationGracePeriodSeconds: ptr.To[int64](-1), }, }, }, diff --git a/pkg/apis/core/v1/defaults.go b/pkg/apis/core/v1/defaults.go index 7e3cbc6fc3184..f3982e5e0195c 100644 --- a/pkg/apis/core/v1/defaults.go +++ b/pkg/apis/core/v1/defaults.go @@ -162,6 +162,10 @@ func SetDefaults_Service(obj *v1.Service) { } func SetDefaults_Pod(obj *v1.Pod) { + // Enforced on Pod but not on PodSpec. For historical reasons, PodTemplate is not defaulted this way. + if obj.Spec.TerminationGracePeriodSeconds != nil && *obj.Spec.TerminationGracePeriodSeconds < 0 { + obj.Spec.TerminationGracePeriodSeconds = ptr.To[int64](1) + } // If limits are specified, but requests are not, default requests to limits // This is done here rather than a more specific defaulting pass on v1.ResourceRequirements diff --git a/pkg/apis/core/v1/defaults_test.go b/pkg/apis/core/v1/defaults_test.go index 9e82538a3b116..fad4b7e66127c 100644 --- a/pkg/apis/core/v1/defaults_test.go +++ b/pkg/apis/core/v1/defaults_test.go @@ -3430,3 +3430,25 @@ func TestSetDefaultPodStatusPodIPs(t *testing.T) { }) } } + +func TestSetDefaultPodTerminationGracePeriodSeconds(t *testing.T) { + tests := []struct { + name string + grace *int64 + expected *int64 + }{ + {name: "negative clamped to 1", grace: ptr.To[int64](-1), expected: ptr.To[int64](1)}, + {name: "zero preserved", grace: ptr.To[int64](0), expected: ptr.To[int64](0)}, + {name: "positive preserved", grace: ptr.To[int64](42), expected: ptr.To[int64](42)}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + pod := &v1.Pod{Spec: v1.PodSpec{TerminationGracePeriodSeconds: tc.grace}} + obj2 := roundTrip(t, runtime.Object(pod)) + pod2 := obj2.(*v1.Pod) + if !reflect.DeepEqual(pod2.Spec.TerminationGracePeriodSeconds, tc.expected) { + t.Errorf("expected %v, got %v", *tc.expected, *pod2.Spec.TerminationGracePeriodSeconds) + } + }) + } +} diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go index 7cc9a0dae2caf..909d0f2dd00f9 100644 --- a/pkg/apis/core/validation/validation.go +++ b/pkg/apis/core/validation/validation.go @@ -4533,6 +4533,10 @@ func validatePodMetadataAndSpec(pod *core.Pod, opts PodValidationOptions) field. } } + if pod.Spec.TerminationGracePeriodSeconds != nil { + allErrs = append(allErrs, ValidateNonnegativeField(*pod.Spec.TerminationGracePeriodSeconds, specPath.Child("terminationGracePeriodSeconds"))...) + } + allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.Containers, specPath.Child("containers"))...) allErrs = append(allErrs, validateContainersOnlyForPod(pod.Spec.InitContainers, specPath.Child("initContainers"))...) // validateContainersOnlyForPod() is checked for ephemeral containers by validateEphemeralContainers() diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go index 5b87d0347e1f3..804960412982a 100644 --- a/pkg/apis/core/validation/validation_test.go +++ b/pkg/apis/core/validation/validation_test.go @@ -10721,6 +10721,9 @@ func TestValidatePod(t *testing.T) { successCases := map[string]core.Pod{ "basic fields": *podtest.MakePod("123"), + "zero terminationGracePeriodSeconds": *podtest.MakePod("123", + podtest.SetTerminationGracePeriodSeconds(0), + ), "just about everything": *podtest.MakePod("abc.123.do-re-mi", podtest.SetInitContainers(podtest.MakeContainer("ictr")), podtest.SetVolumes(podtest.MakeEmptyVolume(("vol"))), @@ -11501,6 +11504,10 @@ func TestValidatePod(t *testing.T) { expectedError: "spec.containers[0].name", spec: *podtest.MakePod("123", podtest.SetContainers(core.Container{})), }, + "negative terminationGracePeriodSeconds": { + expectedError: "spec.terminationGracePeriodSeconds: Invalid value: -1: must be greater than or equal to 0", + spec: *podtest.MakePod("123", podtest.SetTerminationGracePeriodSeconds(-1)), + }, "bad label": { expectedError: "NoUppercaseOrSpecialCharsLike=Equals", spec: *podtest.MakePod("123", diff --git a/pkg/kubelet/config/common_test.go b/pkg/kubelet/config/common_test.go index 8d60d269cad77..091a78223ce11 100644 --- a/pkg/kubelet/config/common_test.go +++ b/pkg/kubelet/config/common_test.go @@ -419,3 +419,36 @@ func TestStaticPodNameGenerate(t *testing.T) { } } } + +// TestDecodeSinglePodTerminationGracePeriodSeconds ensures +// a negative TerminationGracePeriodSeconds is clamped. +func TestDecodeSinglePodTerminationGracePeriodSeconds(t *testing.T) { + logger, _ := ktesting.NewTestContext(t) + grace := int64(-1) + pod := &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{Name: "test", UID: "12345", Namespace: "mynamespace"}, + Spec: v1.PodSpec{ + RestartPolicy: v1.RestartPolicyAlways, + DNSPolicy: v1.DNSClusterFirst, + TerminationGracePeriodSeconds: &grace, + Containers: []v1.Container{{ + Name: "image", + Image: "test/image", + ImagePullPolicy: "IfNotPresent", + TerminationMessagePath: "/dev/termination-log", + TerminationMessagePolicy: v1.TerminationMessageReadFile, + }}, + }, + } + json, err := runtime.Encode(clientscheme.Codecs.LegacyCodec(v1.SchemeGroupVersion), pod) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + parsed, podOut, err := tryDecodeSinglePod(logger, json, noDefault) + if !parsed || err != nil { + t.Fatalf("parsed=%v, unexpected error: %v", parsed, err) + } + if podOut.Spec.TerminationGracePeriodSeconds == nil || *podOut.Spec.TerminationGracePeriodSeconds != 1 { + t.Errorf("expected negative grace period clamped to 1 by decode defaulting, got %v", podOut.Spec.TerminationGracePeriodSeconds) + } +} diff --git a/pkg/registry/core/pod/storage/storage_test.go b/pkg/registry/core/pod/storage/storage_test.go index cd9918031c33d..68c4a6dfedc4c 100644 --- a/pkg/registry/core/pod/storage/storage_test.go +++ b/pkg/registry/core/pod/storage/storage_test.go @@ -49,6 +49,7 @@ import ( kubefeatures "k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/registry/registrytest" "k8s.io/kubernetes/pkg/securitycontext" + "k8s.io/utils/ptr" ) func newStorage(t *testing.T) (*REST, *BindingREST, *StatusREST, *etcd3testing.EtcdTestServer) { @@ -1306,6 +1307,42 @@ func TestCategories(t *testing.T) { registrytest.AssertCategories(t, storage, expected) } +func TestEtcdTerminationGracePeriodSecondsCompatibility(t *testing.T) { + storage, _, _, server := newStorage(t) + defer server.Terminate(t) + defer storage.Store.DestroyFunc() + ctx := genericregistrytest.NewNamespaceScopeContext(storage.Store, metav1.NamespaceDefault) + + stored := validNewPod() + stored.Spec.TerminationGracePeriodSeconds = ptr.To[int64](-1) + key, _ := storage.KeyFunc(ctx, stored.Name) + if err := storage.Storage.Create(ctx, key, stored, nil, 0, false); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + obj, err := storage.Get(ctx, stored.Name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + served := obj.(*api.Pod) + if served.Spec.TerminationGracePeriodSeconds == nil || *served.Spec.TerminationGracePeriodSeconds != 1 { + t.Fatalf("expected stored negative grace period clamped to 1 by storage-decode defaulting, got %v", served.Spec.TerminationGracePeriodSeconds) + } + + updated := served.DeepCopy() + if _, _, err := storage.Update(ctx, updated.Name, rest.DefaultUpdatedObjectInfo(updated), rest.ValidateAllObjectFunc, rest.ValidateAllObjectUpdateFunc, false, &metav1.UpdateOptions{}); err != nil { + t.Fatalf("unexpected error updating pod: %v", err) + } + obj, err = storage.Get(ctx, stored.Name, &metav1.GetOptions{}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + healed := obj.(*api.Pod) + if healed.Spec.TerminationGracePeriodSeconds == nil || *healed.Spec.TerminationGracePeriodSeconds != 1 { + t.Fatalf("expected grace period of 1 after rewrite, got %v", healed.Spec.TerminationGracePeriodSeconds) + } +} + func TestEtcdPodIPsReadCompatibility(t *testing.T) { storage, _, _, server := newStorage(t) defer server.Terminate(t)