Authorization validation bypass / Validation weaknesses in SubjectAccessReview and related resources

diff --git a/pkg/api/testing/validation.go b/pkg/api/testing/validation.go index 7f5fdb8a4ac47..65b166ff64173 100644 --- a/pkg/api/testing/validation.go +++ b/pkg/api/testing/validation.go @@ -327,6 +327,23 @@ func VerifyUpdateValidationEquivalence(t *testing.T, ctx context.Context, obj, o VerifyVersionedValidationEquivalence(t, obj, old, testConfigs...) } +// VerifyValidationEquivalenceFunc is a variant of VerifyValidationEquivalence +// for callers that produce handwritten and declarative validation errors directly, rather +// than through a RESTCreateStrategy. The validate closure should return the combined +// ErrorList for (ctx, obj). +func VerifyValidationEquivalenceFunc(t *testing.T, ctx context.Context, obj runtime.Object, validate func(ctx context.Context, obj runtime.Object) field.ErrorList, expectedErrs field.ErrorList, testConfigs ...ValidationTestConfig) { + t.Helper() + opts := &validationOption{} + for _, testcfg := range testConfigs { + testcfg(opts) + } + + verifyValidationEquivalence(t, expectedErrs, func(c context.Context) field.ErrorList { + return validate(c, obj) + }, ctx, opts, obj) + VerifyVersionedValidationEquivalence(t, obj, nil, testConfigs...) +} + // VerifyUpdateValidationEquivalenceFunc is a variant of VerifyUpdateValidationEquivalence // for callers that produce handwritten and declarative validation errors directly, rather // than through a RESTUpdateStrategy. The validate closure should return the combined 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..073ff6a7b761f 100644 --- a/pkg/registry/authorization/subjectaccessreview/rest_test.go +++ b/pkg/registry/authorization/subjectaccessreview/rest_test.go @@ -30,7 +30,9 @@ 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" + _ "k8s.io/kubernetes/pkg/apis/authorization/install" ) type fakeAuthorizer struct { @@ -239,9 +241,20 @@ 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{}) + ctx := genericapirequest.WithRequestInfo( + genericapirequest.NewContext(), + &genericapirequest.RequestInfo{ + APIGroup: "authorization.k8s.io", + APIVersion: "v1", + Resource: "subjectaccessreviews", + IsResourceRequest: true, + Verb: "create", + }, + ) + + result, err := storage.Create(ctx, &authorizationapi.SubjectAccessReview{Spec: tc.spec}, rest.ValidateAllObjectFunc, &metav1.CreateOptions{}) if err != nil { if tc.expectedErr != "" { if !strings.Contains(err.Error(), tc.expectedErr) { diff --git a/staging/src/k8s.io/api/authorization/v1/generated.proto b/staging/src/k8s.io/api/authorization/v1/generated.proto index 7f97bde126132..20f1d41274b46 100644 --- a/staging/src/k8s.io/api/authorization/v1/generated.proto +++ b/staging/src/k8s.io/api/authorization/v1/generated.proto @@ -224,10 +224,14 @@ message SelfSubjectAccessReview { message SelfSubjectAccessReviewSpec { // resourceAtt ... [truncated]