Authorization bypass / Input validation

diff --git a/pkg/apis/authorization/validation/validation.go b/pkg/apis/authorization/validation/validation.go index 2548cf704e817..0c602b07bdef4 100644 --- a/pkg/apis/authorization/validation/validation.go +++ b/pkg/apis/authorization/validation/validation.go @@ -17,12 +17,16 @@ limitations under the License. package validation import ( + "context" "fmt" apiequality "k8s.io/apimachinery/pkg/api/equality" + "k8s.io/apimachinery/pkg/api/operation" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/registry/rest" authorizationapi "k8s.io/kubernetes/pkg/apis/authorization" ) @@ -31,10 +35,10 @@ import ( func ValidateSubjectAccessReviewSpec(spec authorizationapi.SubjectAccessReviewSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if spec.ResourceAttributes != nil && spec.NonResourceAttributes != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceAttributes"), spec.NonResourceAttributes, `cannot be specified in combination with resourceAttributes`)) + allErrs = append(allErrs, field.Invalid(fldPath, spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`).WithOrigin("union").MarkCoveredByDeclarative()) } if spec.ResourceAttributes == nil && spec.NonResourceAttributes == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceAttributes"), spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`)) + allErrs = append(allErrs, field.Invalid(fldPath, spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`).WithOrigin("union").MarkCoveredByDeclarative()) } if len(spec.User) == 0 && len(spec.Groups) == 0 { allErrs = append(allErrs, field.Invalid(fldPath.Child("user"), spec.User, `at least one of user or group must be specified`)) @@ -49,10 +53,10 @@ func ValidateSubjectAccessReviewSpec(spec authorizationapi.SubjectAccessReviewSp func ValidateSelfSubjectAccessReviewSpec(spec authorizationapi.SelfSubjectAccessReviewSpec, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if spec.ResourceAttributes != nil && spec.NonResourceAttributes != nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("nonResourceAttributes"), spec.NonResourceAttributes, `cannot be specified in combination with resourceAttributes`)) + allErrs = append(allErrs, field.Invalid(fldPath, spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`).WithOrigin("union").MarkCoveredByDeclarative()) } if spec.ResourceAttributes == nil && spec.NonResourceAttributes == nil { - allErrs = append(allErrs, field.Invalid(fldPath.Child("resourceAttributes"), spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`)) + allErrs = append(allErrs, field.Invalid(fldPath, spec.NonResourceAttributes, `exactly one of nonResourceAttributes or resourceAttributes must be specified`).WithOrigin("union").MarkCoveredByDeclarative()) } allErrs = append(allErrs, validateResourceAttributes(spec.ResourceAttributes, field.NewPath("spec.resourceAttributes"))...) @@ -160,3 +164,27 @@ func validateLabelSelectorAttributes(selector *authorizationapi.LabelSelectorAtt return allErrs } + +// ValidateSubjectAccessReviewCreate is the single composition of handwritten and declarative +// SubjectAccessReview validation. +func ValidateSubjectAccessReviewCreate(ctx context.Context, scheme *runtime.Scheme, sar *authorizationapi.SubjectAccessReview) field.ErrorList { + errs := ValidateSubjectAccessReview(sar) + dv := rest.DeclarativeValidation{Scheme: scheme} + return dv.ValidateDeclaratively(ctx, sar, nil, errs, operation.Create, rest.DeclarativeValidationConfig{}) +} + +// ValidateSelfSubjectAccessReviewCreate is the single composition of handwritten and declarative +// SelfSubjectAccessReview validation. +func ValidateSelfSubjectAccessReviewCreate(ctx context.Context, scheme *runtime.Scheme, sar *authorizationapi.SelfSubjectAccessReview) field.ErrorList { + errs := ValidateSelfSubjectAccessReview(sar) + dv := rest.DeclarativeValidation{Scheme: scheme} + return dv.ValidateDeclaratively(ctx, sar, nil, errs, operation.Create, rest.DeclarativeValidationConfig{}) +} + +// ValidateLocalSubjectAccessReviewCreate is the single composition of handwritten and declarative +// LocalSubjectAccessReview validation. +func ValidateLocalSubjectAccessReviewCreate(ctx context.Context, scheme *runtime.Scheme, sar *authorizationapi.LocalSubjectAccessReview) field.ErrorList { + errs := ValidateLocalSubjectAccessReview(sar) + dv := rest.DeclarativeValidation{Scheme: scheme} + return dv.ValidateDeclaratively(ctx, sar, nil, errs, operation.Create, rest.DeclarativeValidationConfig{}) +} diff --git a/pkg/apis/authorization/validation/validation_test.go b/pkg/apis/authorization/validation/validation_test.go index feb8627e0e0be..dc66a4d958fc4 100644 --- a/pkg/apis/authorization/validation/validation_test.go +++ b/pkg/apis/authorization/validation/validation_test.go @@ -95,7 +95,7 @@ func TestValidateSARSpec(t *testing.T) { NonResourceAttributes: &authorizationapi.NonResourceAttributes{}, User: "me", }, - msg: "cannot be specified in combination with resourceAttributes", + msg: "exactly one of nonResourceAttributes or resourceAttributes must be specified", }, { name: "no subject", obj: authorizationapi.SubjectAccessReviewSpec{ @@ -380,7 +380,7 @@ func TestValidateSelfSAR(t *testing.T) { ResourceAttributes: &authorizationapi.ResourceAttributes{}, NonResourceAttributes: &authorizationapi.NonResourceAttributes{}, }, - msg: "cannot be specified in combination with resourceAttributes", + msg: "exactly one of nonResourceAttributes or resourceAttributes must be specified", }, { // here we only test one to be sure the function is called. The more exhaustive suite is tested above. name: "resource attributes: label selector specify both", diff --git a/pkg/registry/authorization/localsubjectaccessreview/rest.go b/pkg/registry/authorization/localsubjectaccessreview/rest.go index de405e93c7296..93b768047ce5c 100644 --- a/pkg/registry/authorization/localsubjectaccessreview/rest.go +++ b/pkg/registry/authorization/localsubjectaccessreview/rest.go @@ -33,10 +33,11 @@ import ( type REST struct { authorizer authorizer.UnconditionalAuthorizer + scheme *runtime.Scheme } -func NewREST(authorizer authorizer.UnconditionalAuthorizer) *REST { - return &REST{authorizer} +func NewREST(authorizer authorizer.UnconditionalAuthorizer, scheme *runtime.Scheme) *REST { + return &REST{authorizer, scheme} } func (r *REST) NamespaceScoped() bool { @@ -65,7 +66,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation return nil, apierrors.NewBadRequest(fmt.Sprintf("not a LocaLocalSubjectAccessReview: %#v", obj)) } - if errs := authorizationvalidation.ValidateLocalSubjectAccessReview(localSubjectAccessReview); len(errs) > 0 { + if errs := authorizationvalidation.ValidateLocalSubjectAccessReviewCreate(ctx, r.scheme, localSubjectAccessReview); len(errs) > 0 { return nil, apierrors.NewInvalid(authorizationapi.Kind(localSubjectAccessReview.Kind), "", errs) } namespace := genericapirequest.NamespaceValue(ctx) diff --git a/pkg/registry/authorization/rest/storage_authorization.go b/pkg/registry/authorization/rest/storage_authorization.go index a779de0336129..b96afa112825e 100644 --- a/pkg/registry/authorization/rest/storage_authorization.go +++ b/pkg/registry/authorization/rest/storage_authorization.go @@ -57,17 +57,17 @@ func (p RESTStorageProvider) v1Storage(apiResourceConfigSource serverstorage.API // subjectaccessreviews if resource := "subjectaccessreviews"; apiResourceConfigSource.ResourceEnabled(authorizationv1.SchemeGroupVersion.WithResource(resource)) { - storage[resource] = subjectaccessreview.NewREST(p.Authorizer) + storage[resource] = subjectaccessreview.NewREST(p.Authorizer, legacyscheme.Scheme) } // selfsubjectaccessreviews if resource := "selfsubjectaccessreviews"; apiResourceConfigSource.ResourceEnabled(authorizationv1.SchemeGroupVersion.WithResource(resource)) { - storage[resource] = selfsubjectaccessreview.NewREST(p.Authorizer) + storage[resource] = selfsubjectaccessreview.NewREST(p.Authorizer, legacyscheme.Scheme) } // localsubjectaccessreviews if resource := "localsubjectaccessreviews"; apiResourceConfigSource.ResourceEnabled(authorizationv1.SchemeGroupVersion.WithResource(resource)) { - storage[resource] = localsubjectaccessreview.NewREST(p.Authorizer) + storage[resource] = localsubjectaccessreview.NewREST(p.Authorizer, legacyscheme.Scheme) } // selfsubjectrulesreviews diff --git a/pkg/registry/authorization/selfsubjectaccessreview/rest.go b/pkg/registry/authorization/selfsubjectaccessreview/rest.go index 17ba46818c264..cf6c032b5c74b 100644 --- a/pkg/registry/authorization/selfsubjectaccessreview/rest.go +++ b/pkg/registry/authorization/selfsubjectaccessreview/rest.go @@ -33,10 +33,11 @@ import ( type REST struct { authorizer authorizer.UnconditionalAuthorizer + scheme *runtime.Scheme } -func NewREST(authorizer authorizer.UnconditionalAuthorizer) *REST { - return &REST{authorizer} +func NewREST(authorizer authorizer.UnconditionalAuthorizer, scheme *runtime.Scheme) *REST { + return &REST{authorizer, scheme} } func (r *REST) NamespaceScoped() bool { @@ -64,7 +65,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("not a SelfSubjectAccessReview: %#v", obj)) } - if errs := authorizationvalidation.ValidateSelfSubjectAccessReview(selfSAR); len(errs) > 0 { + if errs := authorizationvalidation.ValidateSelfSubjectAccessReviewCreate(ctx, r.scheme, selfSAR); len(errs) > 0 { return nil, apierrors.NewInvalid(authorizationapi.Kind(selfSAR.Kind), "", errs) } userToCheck, exists := genericapirequest.UserFrom(ctx) diff --git a/pkg/registry/authorization/subjectaccessreview/rest.go b/pkg/registry/authorization/subjectaccessreview/rest.go index 66151412d8c12..3e9d1788e8dad 100644 --- a/pkg/registry/authorization/subjectaccessreview/rest.go +++ b/pkg/registry/authorization/subjectaccessreview/rest.go @@ -32,10 +32,11 @@ import ( type REST struct { authorizer authorizer.UnconditionalAuthorizer + scheme *runtime.Scheme } -func NewREST(authorizer authorizer.UnconditionalAuthorizer) *REST { - return &REST{authorizer} +func NewREST(authorizer authorizer.UnconditionalAuthorizer, scheme *runtime.Scheme) *REST { + return &REST{authorizer, scheme} } func (r *REST) NamespaceScoped() bool { @@ -63,7 +64,7 @@ func (r *REST) Create(ctx context.Context, obj runtime.Object, createValidation if !ok { return nil, apierrors.NewBadRequest(fmt.Sprintf("not a SubjectAccessReview: %#v", obj)) } - if errs := authorizationvalidation.ValidateSubjectAccessReview(subjectAccessReview); len(errs) > 0 { + if errs := authorizationvalidation.ValidateSubjectAccessReviewCreate(ctx, r.scheme, subjectAccessReview); len(errs) > 0 { return nil, apierrors.NewInvalid(authorizationapi.Kind(subjectAccessReview.Kind), "", errs) } diff --git a/pkg/registry/authorization/subjectaccessreview/rest_test.go b/pkg/registry/authorization/subjectaccessreview/rest_test.go index adb0b01a0643f..bb2bd99a7e068 100644 --- a/pkg/registry/authorization/subjectaccessreview/rest_test.go +++ b/pkg/registry/authorization/subjectaccessreview/rest_test.go @@ -30,6 +30,7 @@ import ( "k8s.io/apiserver/pkg/authorization/authorizer" genericapirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/registry/rest" + "k8s.io/kubernetes/pkg/api/legacyscheme" authorizationapi "k8s.io/kubernetes/pkg/apis/authorization" ) @@ -239,7 +240,7 @@ func TestCreate(t *testing.T) { reason: tc.reason, err: tc.err, } - storage := NewREST(auth) + storage := NewREST(auth, legacyscheme.Scheme) result, err := storage.Create(genericapirequest.NewContext(), &authorizationapi.SubjectAccessReview{Spec: tc.spec}, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil {