Authorization bypass / Policy evaluation desynchronization

PoC (describing the vulnerability path before the fix): - Setup a versioned admission attribute with an initial object state and a CEL policy that depends on the object's fields. - Compute and cache the CEL representation for the initial object (this would be the value used by CEL evaluation during admission). - Mutate the underlying object in the same admission flow (e.g., via a mutating admission step), changing a field that the CEL policy inspects. - Re-evaluate CEL against the object state; due to the pre-fix caching, the CEL value may still correspond to the pre-mutation state, leading to policy decisions based on stale data. - If the policy would have denied the mutated state but allowed the original state, this results in an authorization bypass during admission. Concrete illustrative Go-like pseudocode (conceptual, not guaranteed to compile): // Pre-fix behavior (desync could occur): v := &VersionedAttributes{VersionedObject: &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "alice"}}} cel1, _ := v.GetCELObjectVal() // caches CEL for {name: "alice"} // Mutate during admission pipeline v.UpdateObject(&example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "bob"}}) // mutation occurs cel2, _ := v.GetCELObjectVal() // In pre-fix, would return the same CEL value as cel1 (stale: {name: "alice"}) // Policy evaluates against cel2 and may incorrectly allow the mutated object if policy was designed to reject {name: "bob"} // Post-fix behavior (with LazyObject): v := &VersionedAttributes{VersionedObject: &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "alice"}}} cel1, _ := v.GetCELObjectVal() // caches CEL for {name: "alice"} v.VersionedObject.Set(&example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "bob"}}) // mutation clears cached CEL value cel2, _ := v.GetCELObjectVal() // recalculates CEL from {name: "bob"} // Policy evaluates against cel2 and correctly denies the mutated state

diff --git a/staging/src/k8s.io/apiserver/pkg/admission/conversion.go b/staging/src/k8s.io/apiserver/pkg/admission/conversion.go index ffd2a1212ed00..b1070683b6823 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/conversion.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/conversion.go @@ -31,41 +31,29 @@ import ( type VersionedAttributes struct { // Attributes holds the original admission attributes Attributes - // VersionedOldObject holds Attributes.OldObject (if non-nil), converted to VersionedKind. + // VersionedOldObject holds Attributes.OldObject (if non-nil), converted to VersionedKind and encapsulated in a LazyObject. // It must never be mutated. - VersionedOldObject runtime.Object - // celOldObjectVal lazily caches Attributes.OldObject (if non-nil) as ref.Val representation. This helps in avoiding its repeated construction. - celOldObjectVal ref.Val - // VersionedObject holds Attributes.Object (if non-nil), converted to VersionedKind. + VersionedOldObject LazyObject + // VersionedObject holds Attributes.Object (if non-nil), converted to VersionedKind and encapsulated in a LazyObject. // If mutated, Dirty must be set to true by the mutator. - VersionedObject runtime.Object - // celObjectVal lazily caches Attributes.Object (if non-nil) as ref.Val representation. This helps in avoiding its repeated construction. - celObjectVal ref.Val + VersionedObject LazyObject // VersionedKind holds the fully qualified kind VersionedKind schema.GroupVersionKind - // Dirty indicates VersionedObject has been modified since being converted from Attributes.Object + // Dirty indicates the inner object in VersionedObject has been modified since being converted from Attributes.Object Dirty bool } // UpdateObject updates the VersionedObject and clears the cached CEL representation. -func (v *VersionedAttributes) UpdateObject(obj runtime.Object) error { - v.VersionedObject = obj - v.celObjectVal = nil - return nil -} - -// UpdateOldObject updates the VersionedOldObject and clears the cached CEL representation. -func (v *VersionedAttributes) UpdateOldObject(out runtime.Object) error { - v.VersionedOldObject = out - v.celOldObjectVal = nil - return nil +func (v *VersionedAttributes) UpdateObject(obj runtime.Object) { + v.Dirty = true + v.VersionedObject.Set(obj) } // GetObject overrides the Attributes.GetObject() func (v *VersionedAttributes) GetObject() runtime.Object { - if v.VersionedObject != nil { - return v.VersionedObject + if v.VersionedObject.object != nil { + return v.VersionedObject.object } return v.Attributes.GetObject() } @@ -102,50 +90,60 @@ func NewVersionedAttributes(attr Attributes, gvk schema.GroupVersionKind, o Obje if err != nil { return nil, err } - versionedAttr.VersionedOldObject = out + versionedAttr.VersionedOldObject.Set(out) } if obj := attr.GetObject(); obj != nil { out, err := ConvertToGVK(obj, gvk, o) if err != nil { return nil, err } - versionedAttr.VersionedObject = out + versionedAttr.VersionedObject.Set(out) } return versionedAttr, nil } +// LazyObject encapsulates a versioned runtime.Object and its lazily-evaluated Common Expression Language (CEL) representation. +type LazyObject struct { + object runtime.Object + celVal ref.Val +} -// GetCELObjectVal lazily converts and returns the CEL representation of the object. -func (v *VersionedAttributes) GetCELObjectVal() (ref.Val, error) { - return getCELVal(v.VersionedObject, &v.celObjectVal) +// NewLazyObject returns a new LazyObject wrapping the provided runtime.Object. +func NewLazyObject(obj runtime.Object) LazyObject { + return LazyObject{object: obj} } -// GetCELOldObjectVal lazily converts and returns the CEL representation of the old object. -func (v *VersionedAttributes) GetCELOldObjectVal() (ref.Val, error) { - return getCELVal(v.VersionedOldObject, &v.celOldObjectVal) +// Object returns the underlying runtime.Object. +func (l *LazyObject) Object() runtime.Object { + return l.object } -func getCELVal(obj runtime.Object, cachedVal *ref.Val) (ref.Val, error) { - if *cachedVal != nil { - return *cachedVal, nil +func (l *LazyObject) Set(obj runtime.Object) { + l.object = obj + l.celVal = nil +} + +func (l *LazyObject) CELValue() (ref.Val, error) { + if l.celVal != nil { + return l.celVal, nil } - if obj == nil { + if l.object == nil { return nil, nil } // TODO: Eventually use TypedToVal instead of unstructured object conversion. - unstructuredObj, err := ConvertObjectToUnstructured(obj) + unstructuredObj, err := ConvertObjectToUnstructured(l.object) if err != nil { return nil, err } - *cachedVal = types.DefaultTypeAdapter.NativeToValue(unstructuredObj.Object) - return *cachedVal, nil + l.celVal = types.DefaultTypeAdapter.NativeToValue(unstructuredObj.Object) + return l.celVal, nil } // ConvertVersionedAttributes converts VersionedObject and VersionedOldObject to the specified kind, if needed. // If attr.VersionedKind already matches the requested kind, no conversion is performed. // If conversion is required: -// * attr.VersionedObject is used as the source for the new object if Dirty=true (and is round-tripped through attr.Attributes.Object, clearing Dirty in the process) +// * attr.VersionedObject.Object() is used as the source for the new object if Dirty=true (and is round-tripped through attr.Attributes.Object, clearing Dirty in the process) // * attr.Attributes.Object is used as the source for the new object if Dirty=false // * attr.Attributes.OldObject is used as the source for the old object func ConvertVersionedAttributes(attr *VersionedAttributes, gvk schema.GroupVersionKind, o ObjectInterfaces) error { @@ -160,15 +158,13 @@ func ConvertVersionedAttributes(attr *VersionedAttributes, gvk schema.GroupVersi if err != nil { return err } - if err := attr.UpdateOldObject(out); err != nil { - return err - } + attr.VersionedOldObject.Set(out) } - if attr.VersionedObject != nil { + if attr.VersionedObject.object != nil { // convert the existing versioned object to internal if attr.Dirty { - err := o.GetObjectConvertor().Convert(attr.VersionedObject, attr.Attributes.GetObject(), nil) + err := o.GetObjectConvertor().Convert(attr.VersionedObject.object, attr.Attributes.GetObject(), nil) if err != nil { return err } @@ -179,9 +175,7 @@ func ConvertVersionedAttributes(attr *VersionedAttributes, gvk schema.GroupVersi if err != nil { return err } - if err := attr.UpdateObject(out); err != nil { - return err - } + attr.VersionedObject.Set(out) } // Remember we converted to this version diff --git a/staging/src/k8s.io/apiserver/pkg/admission/conversion_test.go b/staging/src/k8s.io/apiserver/pkg/admission/conversion_test.go index 5d7fbd9b72746..4e56ab9fb0261 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/conversion_test.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/conversion_test.go @@ -194,8 +194,8 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), - VersionedObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}, - VersionedOldObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}), + VersionedOldObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}), VersionedKind: examplev1.SchemeGroupVersion.WithKind("Pod"), Dirty: true, }, @@ -205,8 +205,8 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), - VersionedObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}, - VersionedOldObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}), + VersionedOldObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}), VersionedKind: examplev1.SchemeGroupVersion.WithKind("Pod"), Dirty: true, }, @@ -218,8 +218,8 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), - VersionedObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}, - VersionedOldObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}), + VersionedOldObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}), VersionedKind: gvk("g", "v", "k"), }, GVK: examplev1.SchemeGroupVersion.WithKind("Pod"), @@ -229,8 +229,8 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), // name gets overwritten from converted attributes, type gets set explicitly - VersionedObject: &examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}, - VersionedOldObject: &examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}), + VersionedOldObject: NewLazyObject(&examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}), VersionedKind: examplev1.SchemeGroupVersion.WithKind("Pod"), }, }, @@ -241,8 +241,8 @@ func TestConvertVersionedAttributes(t *testing.T) { u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobj"}}`), u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobj"}}`), ), - VersionedObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobjversioned"}}`), - VersionedOldObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobjversioned"}}`), + VersionedObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobjversioned"}}`)), + VersionedOldObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobjversioned"}}`)), VersionedKind: gvk("g", "v", "k"), // claim a different current version to trigger conversion }, GVK: gvk("mygroup.k8s.io", "v1", "Flunder"), @@ -251,8 +251,8 @@ func TestConvertVersionedAttributes(t *testing.T) { u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobj"}}`), u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobj"}}`), ), - VersionedObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobj"}}`), - VersionedOldObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobj"}}`), + VersionedObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobj"}}`)), + VersionedOldObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobj"}}`)), VersionedKind: gvk("mygroup.k8s.io", "v1", "Flunder"), }, }, @@ -263,8 +263,8 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpod"}}, &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), - VersionedObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}, - VersionedOldObject: &examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}), + VersionedOldObject: NewLazyObject(&examplev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpodversioned"}}), VersionedKind: gvk("g", "v", "k"), // claim a different current version to trigger conversion Dirty: true, }, @@ -275,9 +275,9 @@ func TestConvertVersionedAttributes(t *testing.T) { &example.Pod{ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, ), // new name gets preserved from versioned object, type gets set explicitly - VersionedObject: &examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}, + VersionedObject: NewLazyObject(&examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "newpodversioned"}}), // old name gets overwritten from converted attributes, type gets set explicitly - VersionedOldObject: &examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}, + VersionedOldObject: NewLazyObject(&examplev1.Pod{TypeMeta: metav1.TypeMeta{APIVersion: "example.apiserver.k8s.io/v1", Kind: "Pod"}, ObjectMeta: metav1.ObjectMeta{Name: "oldpod"}}), VersionedKind: examplev1.SchemeGroupVersion.WithKind("Pod"), Dirty: false, }, @@ -289,8 +289,8 @@ func TestConvertVersionedAttributes(t *testing.T) { u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobj"}}`), u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobj"}}`), ), - VersionedObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobjversioned"}}`), - VersionedOldObject: u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobjversioned"}}`), + VersionedObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"newobjversioned"}}`)), + VersionedOldObject: NewLazyObject(u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata":{"name":"oldobjversioned"}}`)), VersionedKind: gvk("g", "v", "k"), // claim a different current version to trigger conversion Dirty: true, }, @@ -301,9 +301,9 @@ func TestConvertVersionedAttributes(t *testing.T) { u(`{"apiVersion": "mygroup.k8s.io/v1","kind": "Flunder","metadata ... [truncated]