Skip to content

Commit 9932dbe

Browse files
tkashemAbu Kashem
authored and
Abu Kashem
committed
add access control check for unsafe delete
add access control check to ensure that the user has permission to do 'unsafe-delete-ignore-read-error' on the resource being deleted
1 parent 367a265 commit 9932dbe

File tree

3 files changed

+447
-17
lines changed

3 files changed

+447
-17
lines changed

Diff for: staging/src/k8s.io/apiserver/pkg/endpoints/handlers/delete.go

+91-2
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,14 @@ import (
3030
metainternalversionvalidation "k8s.io/apimachinery/pkg/apis/meta/internalversion/validation"
3131
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
3232
"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
33+
"k8s.io/apimachinery/pkg/fields"
34+
"k8s.io/apimachinery/pkg/labels"
3335
"k8s.io/apimachinery/pkg/runtime"
3436
"k8s.io/apimachinery/pkg/runtime/schema"
3537
"k8s.io/apimachinery/pkg/util/validation/field"
3638
"k8s.io/apiserver/pkg/admission"
3739
"k8s.io/apiserver/pkg/audit"
40+
"k8s.io/apiserver/pkg/authorization/authorizer"
3841
"k8s.io/apiserver/pkg/endpoints/handlers/finisher"
3942
requestmetrics "k8s.io/apiserver/pkg/endpoints/handlers/metrics"
4043
"k8s.io/apiserver/pkg/endpoints/handlers/negotiation"
@@ -45,6 +48,8 @@ import (
4548
"k8s.io/apiserver/pkg/util/dryrun"
4649
utilfeature "k8s.io/apiserver/pkg/util/feature"
4750
"k8s.io/component-base/tracing"
51+
52+
"k8s.io/klog/v2"
4853
"k8s.io/utils/ptr"
4954
)
5055

@@ -128,6 +133,9 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc
128133
}
129134
options.TypeMeta.SetGroupVersionKind(metav1.SchemeGroupVersion.WithKind("DeleteOptions"))
130135

136+
userInfo, _ := request.UserFrom(ctx)
137+
staticAdmissionAttrs := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Delete, options, dryrun.IsDryRun(options.DryRun), userInfo)
138+
131139
if utilfeature.DefaultFeatureGate.Enabled(features.AllowUnsafeMalformedObjectDeletion) {
132140
if options != nil && ptr.Deref(options.IgnoreStoreReadErrorWithClusterBreakingPotential, false) {
133141
// let's make sure that the audit will reflect that this delete request
@@ -140,14 +148,21 @@ func DeleteResource(r rest.GracefulDeleter, allowsOptions bool, scope *RequestSc
140148
scope.err(errors.NewInternalError(fmt.Errorf("no unsafe deleter provided, can not honor ignoreStoreReadErrorWithClusterBreakingPotential")), w, req)
141149
return
142150
}
151+
if scope.Authorizer == nil {
152+
scope.err(errors.NewInternalError(fmt.Errorf("no authorizer provided, unable to authorize unsafe delete")), w, req)
153+
return
154+
}
155+
if err := authorizeUnsafeDelete(ctx, staticAdmissionAttrs, scope.Authorizer); err != nil {
156+
scope.err(err, w, req)
157+
return
158+
}
159+
143160
r = p.GetCorruptObjDeleter()
144161
}
145162
}
146163

147164
span.AddEvent("About to delete object from database")
148165
wasDeleted := true
149-
userInfo, _ := request.UserFrom(ctx)
150-
staticAdmissionAttrs := admission.NewAttributesRecord(nil, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Delete, options, dryrun.IsDryRun(options.DryRun), userInfo)
151166
result, err := finisher.FinishRequest(ctx, func() (runtime.Object, error) {
152167
obj, deleted, err := r.Delete(ctx, name, rest.AdmissionToValidateObjectDeleteFunc(admit, staticAdmissionAttrs, scope), options)
153168
wasDeleted = deleted
@@ -331,3 +346,77 @@ func DeleteCollection(r rest.CollectionDeleter, checkBody bool, scope *RequestSc
331346
transformResponseObject(ctx, scope, req, w, http.StatusOK, outputMediaType, result)
332347
}
333348
}
349+
350+
// authorizeUnsafeDelete ensures that the user has permission to do
351+
// 'unsafe-delete-ignore-read-errors' on the resource being deleted when
352+
// ignoreStoreReadErrorWithClusterBreakingPotential is enabled
353+
func authorizeUnsafeDelete(ctx context.Context, attr admission.Attributes, authz authorizer.Authorizer) (err error) {
354+
if attr.GetOperation() != admission.Delete || attr.GetOperationOptions() == nil {
355+
return nil
356+
}
357+
options, ok := attr.GetOperationOptions().(*metav1.DeleteOptions)
358+
if !ok {
359+
return errors.NewInternalError(fmt.Errorf("expected an option of type: %T, but got: %T", &metav1.DeleteOptions{}, attr.GetOperationOptions()))
360+
}
361+
if !ptr.Deref(options.IgnoreStoreReadErrorWithClusterBreakingPotential, false) {
362+
return nil
363+
}
364+
365+
requestInfo, found := request.RequestInfoFrom(ctx)
366+
if !found {
367+
return admission.NewForbidden(attr, fmt.Errorf("no RequestInfo found in the context"))
368+
}
369+
if !requestInfo.IsResourceRequest || len(attr.GetSubresource()) > 0 {
370+
return admission.NewForbidden(attr, fmt.Errorf("ignoreStoreReadErrorWithClusterBreakingPotential delete option is not allowed on a subresource or non-resource request"))
371+
}
372+
373+
// if we are here, IgnoreStoreReadErrorWithClusterBreakingPotential
374+
// is set to true in the delete options, the user must have permission
375+
// to do 'unsafe-delete-ignore-read-errors' on the given resource.
376+
record := authorizer.AttributesRecord{
377+
User: attr.GetUserInfo(),
378+
Verb: "unsafe-delete-ignore-read-errors",
379+
Namespace: attr.GetNamespace(),
380+
Name: attr.GetName(),
381+
APIGroup: attr.GetResource().Group,
382+
APIVersion: attr.GetResource().Version,
383+
Resource: attr.GetResource().Resource,
384+
ResourceRequest: true,
385+
}
386+
// TODO: can't use ResourceAttributesFrom from k8s.io/kubernetes/pkg/registry/authorization/util
387+
// due to prevent staging --> k8s.io/kubernetes dep issue
388+
if utilfeature.DefaultFeatureGate.Enabled(features.AuthorizeWithSelectors) {
389+
if len(requestInfo.FieldSelector) > 0 {
390+
fieldSelector, err := fields.ParseSelector(requestInfo.FieldSelector)
391+
if err != nil {
392+
record.FieldSelectorRequirements, record.FieldSelectorParsingErr = nil, err
393+
} else {
394+
if requirements := fieldSelector.Requirements(); len(requirements) > 0 {
395+
record.FieldSelectorRequirements, record.FieldSelectorParsingErr = fieldSelector.Requirements(), nil
396+
}
397+
}
398+
}
399+
if len(requestInfo.LabelSelector) > 0 {
400+
labelSelector, err := labels.Parse(requestInfo.LabelSelector)
401+
if err != nil {
402+
record.LabelSelectorRequirements, record.LabelSelectorParsingErr = nil, err
403+
} else {
404+
if requirements, _ /*selectable*/ := labelSelector.Requirements(); len(requirements) > 0 {
405+
record.LabelSelectorRequirements, record.LabelSelectorParsingErr = requirements, nil
406+
}
407+
}
408+
}
409+
}
410+
411+
decision, reason, err := authz.Authorize(ctx, record)
412+
if err != nil {
413+
err = fmt.Errorf("error while checking permission for %q, %w", record.Verb, err)
414+
klog.FromContext(ctx).V(1).Error(err, "failed to authorize")
415+
return admission.NewForbidden(attr, err)
416+
}
417+
if decision == authorizer.DecisionAllow {
418+
return nil
419+
}
420+
421+
return admission.NewForbidden(attr, fmt.Errorf("not permitted to do %q, reason: %s", record.Verb, reason))
422+
}

0 commit comments

Comments
 (0)