From e8428e704cb9f836d50be8b96a86bd308634679e Mon Sep 17 00:00:00 2001 From: Brian Kane Date: Tue, 2 Sep 2025 22:53:56 +0100 Subject: [PATCH] Feature: Validate Definition Permissions on App Creation (#6876) Signed-off-by: Brian Kane --- apis/core.oam.dev/v1beta1/register.go | 18 + apis/core.oam.dev/v1beta1/register_test.go | 117 +++ .../v1beta1/zz_generated.deepcopy.go | 16 + charts/vela-core/README.md | 3 +- charts/vela-core/templates/NOTES.txt | 9 + .../templates/kubevela-controller.yaml | 3 +- charts/vela-core/values.yaml | 5 +- pkg/features/controller_features.go | 3 + .../application/mutating_handler_test.go | 1 + .../v1beta1/application/validating_handler.go | 4 +- .../application/validating_handler_test.go | 103 ++ .../v1beta1/application/validation.go | 284 +++++- .../validation_permissions_test.go | 881 ++++++++++++++++++ .../componentdefinition/validating_handler.go | 2 +- .../policydefinition/validating_handler.go | 2 +- .../traitdefinition/validating_handler.go | 2 +- .../validating_handler.go | 2 +- 17 files changed, 1443 insertions(+), 12 deletions(-) create mode 100644 apis/core.oam.dev/v1beta1/register_test.go create mode 100644 pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go diff --git a/apis/core.oam.dev/v1beta1/register.go b/apis/core.oam.dev/v1beta1/register.go index 10b7c80c3..4942e6936 100644 --- a/apis/core.oam.dev/v1beta1/register.go +++ b/apis/core.oam.dev/v1beta1/register.go @@ -49,6 +49,7 @@ var ( ComponentDefinitionGroupKind = schema.GroupKind{Group: Group, Kind: ComponentDefinitionKind}.String() ComponentDefinitionKindAPIVersion = ComponentDefinitionKind + "." + SchemeGroupVersion.String() ComponentDefinitionGroupVersionKind = SchemeGroupVersion.WithKind(ComponentDefinitionKind) + ComponentDefinitionGVR = SchemeGroupVersion.WithResource("componentdefinitions") ) // WorkloadDefinition type metadata. @@ -65,6 +66,7 @@ var ( TraitDefinitionGroupKind = schema.GroupKind{Group: Group, Kind: TraitDefinitionKind}.String() TraitDefinitionKindAPIVersion = TraitDefinitionKind + "." + SchemeGroupVersion.String() TraitDefinitionGroupVersionKind = SchemeGroupVersion.WithKind(TraitDefinitionKind) + TraitDefinitionGVR = SchemeGroupVersion.WithResource("traitdefinitions") ) // PolicyDefinition type metadata. @@ -73,6 +75,7 @@ var ( PolicyDefinitionGroupKind = schema.GroupKind{Group: Group, Kind: PolicyDefinitionKind}.String() PolicyDefinitionKindAPIVersion = PolicyDefinitionKind + "." + SchemeGroupVersion.String() PolicyDefinitionGroupVersionKind = SchemeGroupVersion.WithKind(PolicyDefinitionKind) + PolicyDefinitionGVR = SchemeGroupVersion.WithResource("policydefinitions") ) // WorkflowStepDefinition type metadata. @@ -81,6 +84,7 @@ var ( WorkflowStepDefinitionGroupKind = schema.GroupKind{Group: Group, Kind: WorkflowStepDefinitionKind}.String() WorkflowStepDefinitionKindAPIVersion = WorkflowStepDefinitionKind + "." + SchemeGroupVersion.String() WorkflowStepDefinitionGroupVersionKind = SchemeGroupVersion.WithKind(WorkflowStepDefinitionKind) + WorkflowStepDefinitionGVR = SchemeGroupVersion.WithResource("workflowstepdefinitions") ) // DefinitionRevision type metadata. @@ -115,6 +119,20 @@ var ( ResourceTrackerKindVersionKind = SchemeGroupVersion.WithKind(ResourceTrackerKind) ) +// DefinitionTypeInfo contains the mapping information for a definition type +type DefinitionTypeInfo struct { + GVR schema.GroupVersionResource + Kind string +} + +// DefinitionTypeMap maps definition types to their corresponding GVR and Kind +var DefinitionTypeMap = map[reflect.Type]DefinitionTypeInfo{ + reflect.TypeOf(ComponentDefinition{}): {GVR: ComponentDefinitionGVR, Kind: ComponentDefinitionKind}, + reflect.TypeOf(TraitDefinition{}): {GVR: TraitDefinitionGVR, Kind: TraitDefinitionKind}, + reflect.TypeOf(PolicyDefinition{}): {GVR: PolicyDefinitionGVR, Kind: PolicyDefinitionKind}, + reflect.TypeOf(WorkflowStepDefinition{}): {GVR: WorkflowStepDefinitionGVR, Kind: WorkflowStepDefinitionKind}, +} + func init() { SchemeBuilder.Register(&ComponentDefinition{}, &ComponentDefinitionList{}) SchemeBuilder.Register(&WorkloadDefinition{}, &WorkloadDefinitionList{}) diff --git a/apis/core.oam.dev/v1beta1/register_test.go b/apis/core.oam.dev/v1beta1/register_test.go new file mode 100644 index 000000000..bdf1a38b2 --- /dev/null +++ b/apis/core.oam.dev/v1beta1/register_test.go @@ -0,0 +1,117 @@ +/* +Copyright 2025 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1 + +import ( + "reflect" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +func TestDefinitionTypeMap(t *testing.T) { + tests := []struct { + name string + defType reflect.Type + expectedGVR schema.GroupVersionResource + expectedKind string + }{ + { + name: "ComponentDefinition", + defType: reflect.TypeOf(ComponentDefinition{}), + expectedGVR: ComponentDefinitionGVR, + expectedKind: ComponentDefinitionKind, + }, + { + name: "TraitDefinition", + defType: reflect.TypeOf(TraitDefinition{}), + expectedGVR: TraitDefinitionGVR, + expectedKind: TraitDefinitionKind, + }, + { + name: "PolicyDefinition", + defType: reflect.TypeOf(PolicyDefinition{}), + expectedGVR: PolicyDefinitionGVR, + expectedKind: PolicyDefinitionKind, + }, + { + name: "WorkflowStepDefinition", + defType: reflect.TypeOf(WorkflowStepDefinition{}), + expectedGVR: WorkflowStepDefinitionGVR, + expectedKind: WorkflowStepDefinitionKind, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + info, ok := DefinitionTypeMap[tt.defType] + assert.Truef(t, ok, "Type %v should exist in DefinitionTypeMap", tt.defType) + assert.Equal(t, tt.expectedGVR, info.GVR) + assert.Equal(t, tt.expectedKind, info.Kind) + + // Verify GVR follows Kubernetes conventions + assert.Equal(t, Group, info.GVR.Group) + assert.Equal(t, Version, info.GVR.Version) + // Resource should be lowercase plural of Kind + assert.Equal(t, strings.ToLower(info.Kind)+"s", info.GVR.Resource) + }) + } +} + +func TestDefinitionTypeMapCompleteness(t *testing.T) { + // Ensure all expected definition types are in the map + expectedTypes := []reflect.Type{ + reflect.TypeOf(ComponentDefinition{}), + reflect.TypeOf(TraitDefinition{}), + reflect.TypeOf(PolicyDefinition{}), + reflect.TypeOf(WorkflowStepDefinition{}), + } + + assert.Equal(t, len(expectedTypes), len(DefinitionTypeMap), "DefinitionTypeMap should contain exactly %d entries", len(expectedTypes)) + + for _, expectedType := range expectedTypes { + _, ok := DefinitionTypeMap[expectedType] + assert.Truef(t, ok, "DefinitionTypeMap should contain %v", expectedType) + } +} + +func TestDefinitionKindValues(t *testing.T) { + // Verify that the Kind values match the actual type names + tests := []struct { + defType interface{} + expectedKind string + }{ + {ComponentDefinition{}, "ComponentDefinition"}, + {TraitDefinition{}, "TraitDefinition"}, + {PolicyDefinition{}, "PolicyDefinition"}, + {WorkflowStepDefinition{}, "WorkflowStepDefinition"}, + } + + for _, tt := range tests { + t.Run(tt.expectedKind, func(t *testing.T) { + actualKind := reflect.TypeOf(tt.defType).Name() + assert.Equal(t, tt.expectedKind, actualKind) + + // Also verify it matches what's in the map + info, ok := DefinitionTypeMap[reflect.TypeOf(tt.defType)] + assert.True(t, ok) + assert.Equal(t, tt.expectedKind, info.Kind) + }) + } +} diff --git a/apis/core.oam.dev/v1beta1/zz_generated.deepcopy.go b/apis/core.oam.dev/v1beta1/zz_generated.deepcopy.go index c42796a55..0e3eb29e2 100644 --- a/apis/core.oam.dev/v1beta1/zz_generated.deepcopy.go +++ b/apis/core.oam.dev/v1beta1/zz_generated.deepcopy.go @@ -552,6 +552,22 @@ func (in *DefinitionRevisionSpec) DeepCopy() *DefinitionRevisionSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DefinitionTypeInfo) DeepCopyInto(out *DefinitionTypeInfo) { + *out = *in + out.GVR = in.GVR +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DefinitionTypeInfo. +func (in *DefinitionTypeInfo) DeepCopy() *DefinitionTypeInfo { + if in == nil { + return nil + } + out := new(DefinitionTypeInfo) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ManagedResource) DeepCopyInto(out *ManagedResource) { *out = *in diff --git a/charts/vela-core/README.md b/charts/vela-core/README.md index ec8bfa07e..5f6959856 100644 --- a/charts/vela-core/README.md +++ b/charts/vela-core/README.md @@ -151,7 +151,8 @@ helm install --create-namespace -n vela-system kubevela kubevela/vela-core --wai | `logFileMaxSize` | Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. | `1024` | | `kubeClient.qps` | The qps for reconcile clients | `400` | | `kubeClient.burst` | The burst for reconcile clients | `600` | -| `authentication.enabled` | Enable authentication for application | `false` | +| `authentication.enabled` | Enable authentication framework for applications | `false` | +| `authentication.definitions.enabled` | Enable definition permission validation (requires authentication.enabled) | `false` | | `authentication.withUser` | Application authentication will impersonate as the request User | `true` | | `authentication.defaultUser` | Application authentication will impersonate as the User if no user provided in Application | `kubevela:vela-core` | | `authentication.groupPattern` | Application authentication will impersonate as the request Group that matches the pattern | `kubevela:*` | diff --git a/charts/vela-core/templates/NOTES.txt b/charts/vela-core/templates/NOTES.txt index 65622ad1c..14cb2559d 100644 --- a/charts/vela-core/templates/NOTES.txt +++ b/charts/vela-core/templates/NOTES.txt @@ -29,3 +29,12 @@ Welcome to use the KubeVela! Enjoy your shipping application journey! You can refer to https://kubevela.io for more details. + +{{- if and .Values.authentication.enabled (not .Values.authentication.definitions.enabled) }} + +WARNING: Authentication is enabled but definition permission validation is disabled. + Applications can reference definitions without RBAC checks. + To enable definition permission validation: + --set authentication.definitions.enabled=true + Ensure users have appropriate RBAC permissions before enabling. +{{- end }} diff --git a/charts/vela-core/templates/kubevela-controller.yaml b/charts/vela-core/templates/kubevela-controller.yaml index 0a5d48006..80bf80282 100644 --- a/charts/vela-core/templates/kubevela-controller.yaml +++ b/charts/vela-core/templates/kubevela-controller.yaml @@ -299,8 +299,8 @@ spec: - "--enable-external-package-for-default-compiler={{ .Values.workflow.enableExternalPackageForDefaultCompiler }}" - "--enable-external-package-watch-for-default-compiler={{ .Values.workflow.enableExternalPackageWatchForDefaultCompiler }}" - "--feature-gates=EnableSuspendOnFailure={{- .Values.workflow.enableSuspendOnFailure | toString -}}" - - "--feature-gates=AuthenticateApplication={{- .Values.authentication.enabled | toString -}}" - "--feature-gates=GzipResourceTracker={{- .Values.featureGates.gzipResourceTracker | toString -}}" + - "--feature-gates=AuthenticateApplication={{- .Values.authentication.enabled | toString -}}" - "--feature-gates=ZstdResourceTracker={{- .Values.featureGates.zstdResourceTracker | toString -}}" - "--feature-gates=ApplyOnce={{- .Values.featureGates.applyOnce | toString -}}" - "--feature-gates=MultiStageComponentApply= {{- .Values.featureGates.multiStageComponentApply | toString -}}" @@ -314,6 +314,7 @@ spec: - "--feature-gates=EnableCueValidation={{- .Values.featureGates.enableCueValidation | toString -}}" - "--feature-gates=EnableApplicationStatusMetrics={{- .Values.featureGates.enableApplicationStatusMetrics | toString -}}" {{ if .Values.authentication.enabled }} + - "--feature-gates=ValidateDefinitionPermissions={{ .Values.authentication.definitions.enabled | toString -}}" {{ if .Values.authentication.withUser }} - "--authentication-with-user" {{ end }} diff --git a/charts/vela-core/values.yaml b/charts/vela-core/values.yaml index 4e3a061b1..2fa9592e3 100644 --- a/charts/vela-core/values.yaml +++ b/charts/vela-core/values.yaml @@ -289,12 +289,15 @@ kubeClient: qps: 400 burst: 600 -## @param authentication.enabled Enable authentication for application +## @param authentication.enabled Enable authentication framework for applications +## @param authentication.definitions.enabled Enable definition permission validation (requires authentication.enabled) ## @param authentication.withUser Application authentication will impersonate as the request User ## @param authentication.defaultUser Application authentication will impersonate as the User if no user provided in Application ## @param authentication.groupPattern Application authentication will impersonate as the request Group that matches the pattern authentication: enabled: false + definitions: + enabled: false withUser: true defaultUser: kubevela:vela-core groupPattern: kubevela:* diff --git a/pkg/features/controller_features.go b/pkg/features/controller_features.go index e7423ff02..841cca988 100644 --- a/pkg/features/controller_features.go +++ b/pkg/features/controller_features.go @@ -54,6 +54,8 @@ const ( // AuthenticateApplication enable the authentication for application AuthenticateApplication featuregate.Feature = "AuthenticateApplication" + // ValidateDefinitionPermissions enables RBAC validation for definition access in applications + ValidateDefinitionPermissions featuregate.Feature = "ValidateDefinitionPermissions" // GzipResourceTracker enables the gzip compression for ResourceTracker. It can be useful if you have large // application that needs to dispatch lots of resources or large resources (like CRD or huge ConfigMap), // which at the cost of slower processing speed due to the extra overhead for compression and decompression. @@ -128,6 +130,7 @@ var defaultFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ DisableReferObjectsFromURL: {Default: false, PreRelease: featuregate.Alpha}, ApplyResourceByReplace: {Default: false, PreRelease: featuregate.Alpha}, AuthenticateApplication: {Default: false, PreRelease: featuregate.Alpha}, + ValidateDefinitionPermissions: {Default: false, PreRelease: featuregate.Alpha}, GzipResourceTracker: {Default: false, PreRelease: featuregate.Alpha}, ZstdResourceTracker: {Default: false, PreRelease: featuregate.Alpha}, ApplyOnce: {Default: false, PreRelease: featuregate.Alpha}, diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/mutating_handler_test.go b/pkg/webhook/core.oam.dev/v1beta1/application/mutating_handler_test.go index af8c7dc0b..b724d610c 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/mutating_handler_test.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/mutating_handler_test.go @@ -1,4 +1,5 @@ /* + /* Copyright 2022 The KubeVela Authors. Licensed under the Apache License, Version 2.0 (the "License"); diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go index da02b1937..cdc278571 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler.go @@ -73,7 +73,7 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a ctx = util.SetNamespaceInCtx(ctx, app.Namespace) switch req.Operation { case admissionv1.Create: - if allErrs := h.ValidateCreate(ctx, app); len(allErrs) > 0 { + if allErrs := h.ValidateCreate(ctx, app, req); len(allErrs) > 0 { // http.StatusUnprocessableEntity will NOT report any error descriptions // to the client, use generic http.StatusBadRequest instead. return admission.Errored(http.StatusBadRequest, mergeErrors(allErrs)) @@ -84,7 +84,7 @@ func (h *ValidatingHandler) Handle(ctx context.Context, req admission.Request) a return admission.Errored(http.StatusBadRequest, simplifyError(err)) } if app.ObjectMeta.DeletionTimestamp.IsZero() { - if allErrs := h.ValidateUpdate(ctx, app, oldApp); len(allErrs) > 0 { + if allErrs := h.ValidateUpdate(ctx, app, oldApp, req); len(allErrs) > 0 { return admission.Errored(http.StatusBadRequest, mergeErrors(allErrs)) } } diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler_test.go b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler_test.go index d859561ce..4ee0a86fb 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler_test.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validating_handler_test.go @@ -248,6 +248,109 @@ var _ = Describe("Test Application Validator", func() { Object: runtime.RawExtension{ Raw: []byte(` {"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"workflow-timeout","namespace":"default","annotations":{"app.oam.dev/autoUpdate":"true"}},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"crccheck/hello-world"}}],"workflow":{"steps":[{"name":"group","type":"suspend","timeout":"1s"}]}}} +`), + }, + }, + } + resp := handler.Handle(ctx, req) + Expect(resp.Allowed).Should(BeTrue()) + }) + + It("Test Application Update with validation errors - duplicate workflow steps", func() { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}],"workflow":{"steps":[{"name":"suspend","type":"suspend"},{"name":"suspend","type":"suspend"}]}}} +`), + }, + OldObject: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}]}} +`), + }, + }, + } + resp := handler.Handle(ctx, req) + Expect(resp.Allowed).Should(BeFalse()) + }) + + It("Test Application Update with invalid workflow timeout", func() { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"workflow-update-test","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}],"workflow":{"steps":[{"name":"group","type":"suspend","timeout":"invalid-timeout"}]}}} +`), + }, + OldObject: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"workflow-update-test","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}]}} +`), + }, + }, + } + resp := handler.Handle(ctx, req) + Expect(resp.Allowed).Should(BeFalse()) + }) + + It("Test Application Update with deletion timestamp should skip validation", func() { + now := metav1.Now() + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default","deletionTimestamp":"` + now.Format("2006-01-02T15:04:05Z") + `"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}],"workflow":{"steps":[{"name":"suspend","type":"suspend"},{"name":"suspend","type":"suspend"}]}}} +`), + }, + OldObject: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}]}} +`), + }, + }, + } + resp := handler.Handle(ctx, req) + // Even with duplicate workflow steps (which would normally fail), it should pass because of deletion timestamp + Expect(resp.Allowed).Should(BeTrue()) + }) + + It("Test Application Update decode old object error", func() { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}]}} +`), + }, + OldObject: runtime.RawExtension{Raw: []byte("invalid json")}, + }, + } + resp := handler.Handle(ctx, req) + Expect(resp.Allowed).Should(BeFalse()) + }) + + It("Test Application Update successful case", func() { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + Operation: admissionv1.Update, + Resource: metav1.GroupVersionResource{Group: "core.oam.dev", Version: "v1alpha2", Resource: "applications"}, + Object: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"nginx"}}]}} +`), + }, + OldObject: runtime.RawExtension{ + Raw: []byte(` +{"apiVersion":"core.oam.dev/v1beta1","kind":"Application","metadata":{"name":"application-sample","namespace":"default"},"spec":{"components":[{"name":"comp","type":"worker","properties":{"image":"busybox"}}]}} `), }, }, diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go index eca6c4fd5..6d2842c00 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go @@ -19,16 +19,21 @@ package application import ( "context" "fmt" + "reflect" "time" "github.com/kubevela/pkg/controller/sharding" "github.com/kubevela/pkg/util/singleton" + authv1 "k8s.io/api/authorization/v1" "k8s.io/apimachinery/pkg/util/validation/field" utilfeature "k8s.io/apiserver/pkg/util/feature" + "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" "github.com/oam-dev/kubevela/pkg/appfile" + "github.com/oam-dev/kubevela/pkg/auth" "github.com/oam-dev/kubevela/pkg/features" "github.com/oam-dev/kubevela/pkg/oam" ) @@ -108,6 +113,278 @@ func (h *ValidatingHandler) ValidateComponents(ctx context.Context, app *v1beta1 return componentErrs } +// checkDefinitionPermission checks if user has permission to access a definition in either system namespace or app namespace +func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req admission.Request, resource, definitionType, appNamespace string) (bool, error) { + // Check permission in system namespace (vela-system) first since most definitions are there + // This optimizes for the common case and reduces API calls + systemNsSar := &authv1.SubjectAccessReview{ + Spec: authv1.SubjectAccessReviewSpec{ + User: req.UserInfo.Username, + Groups: req.UserInfo.Groups, + ResourceAttributes: &authv1.ResourceAttributes{ + Verb: "get", + Group: "core.oam.dev", + Version: "v1beta1", + Resource: resource, + Namespace: oam.SystemDefinitionNamespace, + Name: definitionType, + }, + }, + } + + if err := h.Client.Create(ctx, systemNsSar); err != nil { + return false, fmt.Errorf("failed to check %s permission in system namespace: %w", resource, err) + } + + if systemNsSar.Status.Allowed { + // User has permission in system namespace - no need to check app namespace + return true, nil + } + + // If not in system namespace and app namespace is different, check app namespace + if appNamespace != oam.SystemDefinitionNamespace { + appNsSar := &authv1.SubjectAccessReview{ + Spec: authv1.SubjectAccessReviewSpec{ + User: req.UserInfo.Username, + Groups: req.UserInfo.Groups, + ResourceAttributes: &authv1.ResourceAttributes{ + Verb: "get", + Group: "core.oam.dev", + Version: "v1beta1", + Resource: resource, + Namespace: appNamespace, + Name: definitionType, + }, + }, + } + + if err := h.Client.Create(ctx, appNsSar); err != nil { + return false, fmt.Errorf("failed to check %s permission in namespace %s: %w", resource, appNamespace, err) + } + + if appNsSar.Status.Allowed { + // User has permission in app namespace + return true, nil + } + } + + // User doesn't have permission in either namespace + return false, nil +} + +// workflowStepLocation represents the location of a workflow step +type workflowStepLocation struct { + StepIndex int + SubStepIndex int // -1 if not a sub-step + IsSubStep bool +} + +// definitionUsage tracks where each definition type is used in the application +type definitionUsage struct { + componentTypes map[string][]int + traitTypes map[string][][2]int + policyTypes map[string][]int + workflowStepTypes map[string][]workflowStepLocation +} + +// collectDefinitionUsage collects all unique definition types and their locations in the application +func collectDefinitionUsage(app *v1beta1.Application) *definitionUsage { + usage := &definitionUsage{ + componentTypes: make(map[string][]int), + traitTypes: make(map[string][][2]int), + policyTypes: make(map[string][]int), + workflowStepTypes: make(map[string][]workflowStepLocation), + } + + // Collect component and trait types + for i, comp := range app.Spec.Components { + usage.componentTypes[comp.Type] = append(usage.componentTypes[comp.Type], i) + + for j, trait := range comp.Traits { + usage.traitTypes[trait.Type] = append(usage.traitTypes[trait.Type], [2]int{i, j}) + } + } + + // Collect policy types + for i, policy := range app.Spec.Policies { + usage.policyTypes[policy.Type] = append(usage.policyTypes[policy.Type], i) + } + + // Collect workflow step types (including sub-steps) + if app.Spec.Workflow != nil { + for i, step := range app.Spec.Workflow.Steps { + location := workflowStepLocation{ + StepIndex: i, + SubStepIndex: -1, + IsSubStep: false, + } + usage.workflowStepTypes[step.Type] = append(usage.workflowStepTypes[step.Type], location) + + // Also check sub-steps + for j, subStep := range step.SubSteps { + subLocation := workflowStepLocation{ + StepIndex: i, + SubStepIndex: j, + IsSubStep: true, + } + usage.workflowStepTypes[subStep.Type] = append(usage.workflowStepTypes[subStep.Type], subLocation) + } + } + } + + return usage +} + +// processDefinitionPermissionCheck handles the common logic for processing permission check results +func (h *ValidatingHandler) processDefinitionPermissionCheck( + allowed bool, + err error, + req admission.Request, + definitionKind string, + definitionType string, + appNamespace string, + fieldPaths []*field.Path, +) field.ErrorList { + var errs field.ErrorList + + if err != nil { + klog.Errorf("Failed to check %s permission for user %s: %v", definitionKind, req.UserInfo.Username, err) + for _, fieldPath := range fieldPaths { + errs = append(errs, field.Forbidden(fieldPath, + fmt.Sprintf("unable to verify permissions for %s %q: %v", definitionKind, definitionType, err))) + } + return errs + } + + if !allowed { + klog.Infof("User %q does not have permission to access %s %q in namespace %q or %q", + req.UserInfo.Username, definitionKind, definitionType, appNamespace, oam.SystemDefinitionNamespace) + for _, fieldPath := range fieldPaths { + errs = append(errs, field.Forbidden(fieldPath, + fmt.Sprintf("user %q cannot get %s %q in namespace %q or %q", + req.UserInfo.Username, definitionKind, definitionType, appNamespace, oam.SystemDefinitionNamespace))) + } + } + + return errs +} + +// fieldPathBuilder is a function that builds field paths for a given definition usage +type fieldPathBuilder func(interface{}) []*field.Path + +// validateDefinitions is a generic function to validate any definition type +func (h *ValidatingHandler) validateDefinitions( + ctx context.Context, + req admission.Request, + appNamespace string, + definitionType reflect.Type, + usageMap interface{}, + buildFieldPaths fieldPathBuilder, +) field.ErrorList { + var errs field.ErrorList + + // Get the definition info for the given type + defInfo, ok := v1beta1.DefinitionTypeMap[definitionType] + if !ok { + klog.Errorf("Unknown definition type: %v", definitionType) + return errs + } + + switch typedMap := usageMap.(type) { + case map[string][]int: + for defType, indices := range typedMap { + allowed, err := h.checkDefinitionPermission(ctx, req, defInfo.GVR.Resource, defType, appNamespace) + fieldPaths := buildFieldPaths(indices) + errs = append(errs, h.processDefinitionPermissionCheck( + allowed, err, req, defInfo.Kind, defType, appNamespace, fieldPaths)...) + } + case map[string][][2]int: + for defType, locations := range typedMap { + allowed, err := h.checkDefinitionPermission(ctx, req, defInfo.GVR.Resource, defType, appNamespace) + fieldPaths := buildFieldPaths(locations) + errs = append(errs, h.processDefinitionPermissionCheck( + allowed, err, req, defInfo.Kind, defType, appNamespace, fieldPaths)...) + } + case map[string][]workflowStepLocation: + for defType, locations := range typedMap { + allowed, err := h.checkDefinitionPermission(ctx, req, defInfo.GVR.Resource, defType, appNamespace) + fieldPaths := buildFieldPaths(locations) + errs = append(errs, h.processDefinitionPermissionCheck( + allowed, err, req, defInfo.Kind, defType, appNamespace, fieldPaths)...) + } + } + + return errs +} + +// ValidateDefinitionPermissions validates that the user has permissions to access all definition types +func (h *ValidatingHandler) ValidateDefinitionPermissions(ctx context.Context, app *v1beta1.Application, req admission.Request) field.ErrorList { + // Requires both authentication and definition validation enabled + if !auth.AuthenticationWithUser || !utilfeature.DefaultMutableFeatureGate.Enabled(features.ValidateDefinitionPermissions) { + return nil + } + + var errs field.ErrorList + usage := collectDefinitionUsage(app) + + // Validate ComponentDefinitions + errs = append(errs, h.validateDefinitions(ctx, req, app.Namespace, + reflect.TypeOf(v1beta1.ComponentDefinition{}), usage.componentTypes, + func(indices interface{}) []*field.Path { + var paths []*field.Path + for _, idx := range indices.([]int) { + paths = append(paths, field.NewPath("spec", "components").Index(idx).Child("type")) + } + return paths + })...) + + // Validate TraitDefinitions + errs = append(errs, h.validateDefinitions(ctx, req, app.Namespace, + reflect.TypeOf(v1beta1.TraitDefinition{}), usage.traitTypes, + func(locations interface{}) []*field.Path { + var paths []*field.Path + for _, loc := range locations.([][2]int) { + paths = append(paths, + field.NewPath("spec", "components").Index(loc[0]).Child("traits").Index(loc[1]).Child("type")) + } + return paths + })...) + + // Validate PolicyDefinitions + errs = append(errs, h.validateDefinitions(ctx, req, app.Namespace, + reflect.TypeOf(v1beta1.PolicyDefinition{}), usage.policyTypes, + func(indices interface{}) []*field.Path { + var paths []*field.Path + for _, idx := range indices.([]int) { + paths = append(paths, field.NewPath("spec", "policies").Index(idx).Child("type")) + } + return paths + })...) + + // Validate WorkflowStepDefinitions + errs = append(errs, h.validateDefinitions(ctx, req, app.Namespace, + reflect.TypeOf(v1beta1.WorkflowStepDefinition{}), usage.workflowStepTypes, + func(locations interface{}) []*field.Path { + var paths []*field.Path + for _, loc := range locations.([]workflowStepLocation) { + paths = append(paths, getWorkflowStepFieldPath(loc)) + } + return paths + })...) + + return errs +} + +// getWorkflowStepFieldPath constructs the field path for a workflow step or sub-step +func getWorkflowStepFieldPath(loc workflowStepLocation) *field.Path { + if !loc.IsSubStep { + // Regular step + return field.NewPath("spec", "workflow", "steps").Index(loc.StepIndex).Child("type") + } + // Sub-step + return field.NewPath("spec", "workflow", "steps").Index(loc.StepIndex).Child("subSteps").Index(loc.SubStepIndex).Child("type") +} + // ValidateAnnotations validates whether the application has both autoupdate and publish version annotations func (h *ValidatingHandler) ValidateAnnotations(_ context.Context, app *v1beta1.Application) field.ErrorList { var annotationsErrs field.ErrorList @@ -122,19 +399,20 @@ func (h *ValidatingHandler) ValidateAnnotations(_ context.Context, app *v1beta1. } // ValidateCreate validates the Application on creation -func (h *ValidatingHandler) ValidateCreate(ctx context.Context, app *v1beta1.Application) field.ErrorList { +func (h *ValidatingHandler) ValidateCreate(ctx context.Context, app *v1beta1.Application, req admission.Request) field.ErrorList { var errs field.ErrorList errs = append(errs, h.ValidateAnnotations(ctx, app)...) + errs = append(errs, h.ValidateDefinitionPermissions(ctx, app, req)...) errs = append(errs, h.ValidateWorkflow(ctx, app)...) errs = append(errs, h.ValidateComponents(ctx, app)...) return errs } // ValidateUpdate validates the Application on update -func (h *ValidatingHandler) ValidateUpdate(ctx context.Context, newApp, _ *v1beta1.Application) field.ErrorList { +func (h *ValidatingHandler) ValidateUpdate(ctx context.Context, newApp, _ *v1beta1.Application, req admission.Request) field.ErrorList { // check if the newApp is valid - errs := h.ValidateCreate(ctx, newApp) + errs := h.ValidateCreate(ctx, newApp, req) // TODO: add more validating return errs } diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go b/pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go new file mode 100644 index 000000000..88aa8ade0 --- /dev/null +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go @@ -0,0 +1,881 @@ +/* +Copyright 2025 The KubeVela Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package application + +import ( + "context" + "fmt" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + admissionv1 "k8s.io/api/admission/v1" + authenticationv1 "k8s.io/api/authentication/v1" + authv1 "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + utilfeature "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + + workflowv1alpha1 "github.com/kubevela/workflow/api/v1alpha1" + + "github.com/oam-dev/kubevela/apis/core.oam.dev/common" + "github.com/oam-dev/kubevela/apis/core.oam.dev/v1beta1" + "github.com/oam-dev/kubevela/pkg/auth" + "github.com/oam-dev/kubevela/pkg/features" + "github.com/oam-dev/kubevela/pkg/oam" +) + +func TestValidateDefinitionPermissions(t *testing.T) { + // Enable the authentication features for testing + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, features.ValidateDefinitionPermissions, true) + oldAuthWithUser := auth.AuthenticationWithUser + auth.AuthenticationWithUser = true + defer func() { auth.AuthenticationWithUser = oldAuthWithUser }() + + testCases := []struct { + name string + app *v1beta1.Application + userInfo authenticationv1.UserInfo + allowedDefinitions map[string]bool // resource/namespace/name -> allowed + expectedErrorCount int + expectedErrorFields []string + expectedErrorMsgs []string + }{ + { + name: "user has all permissions", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, + }, + }, + }, + Policies: []v1beta1.AppPolicy{ + {Name: "policy1", Type: "topology"}, + }, + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", + Type: "deploy", + }, + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, + "traitdefinitions/vela-system/scaler": true, + "policydefinitions/vela-system/topology": true, + "workflowstepdefinitions/vela-system/deploy": true, + }, + expectedErrorCount: 0, + }, + { + name: "user lacks ComponentDefinition permission", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + { + Name: "comp2", + Type: "webservice", // Same type, should get two errors + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "restricted-user", + Groups: []string{"restricted-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": false, + "componentdefinitions/test-ns/webservice": false, + }, + expectedErrorCount: 2, // One for each component + expectedErrorFields: []string{"spec.components[0].type", "spec.components[1].type"}, + expectedErrorMsgs: []string{"cannot get ComponentDefinition \"webservice\""}, + }, + { + name: "user lacks TraitDefinition permission", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, + {Type: "gateway"}, + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "restricted-user", + Groups: []string{"restricted-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, + "traitdefinitions/vela-system/scaler": false, + "traitdefinitions/test-ns/scaler": false, + "traitdefinitions/vela-system/gateway": false, + "traitdefinitions/test-ns/gateway": false, + }, + expectedErrorCount: 2, + expectedErrorFields: []string{"spec.components[0].traits[1].type", "spec.components[0].traits[0].type"}, + expectedErrorMsgs: []string{"cannot get TraitDefinition \"gateway\"", "cannot get TraitDefinition \"scaler\""}, + }, + { + name: "user lacks PolicyDefinition permission", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Policies: []v1beta1.AppPolicy{ + { + Name: "topology", + Type: "topology", + Properties: &runtime.RawExtension{ + Raw: []byte(`{"clusters":["local","remote"]}`), + }, + }, + { + Name: "override", + Type: "override", + Properties: &runtime.RawExtension{ + Raw: []byte(`{"components":[{"name":"comp1"}]}`), + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "policy-user", + Groups: []string{"policy-group"}, + }, + allowedDefinitions: map[string]bool{ + "policydefinitions/vela-system/topology": true, + "policydefinitions/test-ns/topology": true, + "policydefinitions/vela-system/override": false, + "policydefinitions/test-ns/override": false, + }, + expectedErrorCount: 1, + expectedErrorFields: []string{"spec.policies[1].type"}, + expectedErrorMsgs: []string{"cannot get PolicyDefinition \"override\""}, + }, + { + name: "empty application", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "empty-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{}, + }, + userInfo: authenticationv1.UserInfo{ + Username: "empty-user", + Groups: []string{"empty-group"}, + }, + allowedDefinitions: map[string]bool{}, + expectedErrorCount: 0, + }, + { + name: "nil workflow", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nil-workflow-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + Workflow: nil, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, + "componentdefinitions/test-ns/webservice": true, + }, + expectedErrorCount: 0, + }, + { + name: "mixed permissions", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mixed-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "allowed-comp", + Type: "webservice", + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, + {Type: "ingress"}, + }, + }, + { + Name: "denied-comp", + Type: "worker", + }, + }, + Policies: []v1beta1.AppPolicy{ + { + Name: "topology", + Type: "topology", + }, + { + Name: "garbage-collect", + Type: "garbage-collect", + }, + }, + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "deploy", + Type: "deploy", + }, + }, + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "notify", + Type: "notification", + }, + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "mixed-user", + Groups: []string{"mixed-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, + "componentdefinitions/test-ns/webservice": true, + "componentdefinitions/vela-system/worker": false, + "componentdefinitions/test-ns/worker": false, + "traitdefinitions/vela-system/scaler": false, + "traitdefinitions/test-ns/scaler": false, + "traitdefinitions/vela-system/ingress": true, + "traitdefinitions/test-ns/ingress": true, + "policydefinitions/vela-system/topology": true, + "policydefinitions/test-ns/topology": true, + "policydefinitions/vela-system/garbage-collect": false, + "policydefinitions/test-ns/garbage-collect": false, + "workflowstepdefinitions/vela-system/deploy": true, + "workflowstepdefinitions/test-ns/deploy": true, + "workflowstepdefinitions/vela-system/notification": false, + "workflowstepdefinitions/test-ns/notification": false, + }, + expectedErrorCount: 4, + expectedErrorFields: []string{ + "spec.components[0].traits[0].type", + "spec.components[1].type", + "spec.policies[1].type", + "spec.workflow.steps[1].type", + }, + expectedErrorMsgs: []string{ + "cannot get TraitDefinition \"scaler\"", + "cannot get ComponentDefinition \"worker\"", + "cannot get PolicyDefinition \"garbage-collect\"", + "cannot get WorkflowStepDefinition \"notification\"", + }, + }, + { + name: "SAR API failure returns error", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "error-trigger", + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/error-trigger": false, + "componentdefinitions/test-ns/error-trigger": false, + }, + expectedErrorCount: 1, + expectedErrorFields: []string{"spec.components[0].type"}, + expectedErrorMsgs: []string{"unable to verify permissions"}, + }, + { + name: "SAR API intermittent failure - fails closed for safety", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "system-error-only", + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/test-ns/system-error-only": true, + }, + expectedErrorCount: 1, + expectedErrorFields: []string{"spec.components[0].type"}, + expectedErrorMsgs: []string{"unable to verify permissions"}, + }, + { + name: "user has permission in app namespace but not system namespace", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "custom-comp", + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "namespace-user", + Groups: []string{"namespace-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/custom-comp": false, + "componentdefinitions/test-ns/custom-comp": true, // Allowed in app namespace + }, + expectedErrorCount: 0, // Should pass as user has permission in app namespace + }, + { + name: "user has permission in system namespace but not app namespace", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "system-user", + Groups: []string{"system-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, // Allowed in system namespace + "componentdefinitions/test-ns/webservice": false, + }, + expectedErrorCount: 0, // Should pass as user has permission in system namespace + }, + { + name: "workflow with substeps", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", + Type: "deploy", + }, + SubSteps: []workflowv1alpha1.WorkflowStepBase{ + { + Name: "substep1", + Type: "suspend", + }, + { + Name: "substep2", + Type: "notification", + }, + }, + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "workflow-user", + Groups: []string{"workflow-group"}, + }, + allowedDefinitions: map[string]bool{ + "workflowstepdefinitions/vela-system/deploy": true, + "workflowstepdefinitions/vela-system/suspend": false, + "workflowstepdefinitions/test-ns/suspend": false, + "workflowstepdefinitions/vela-system/notification": false, + "workflowstepdefinitions/test-ns/notification": false, + }, + expectedErrorCount: 2, + expectedErrorFields: []string{"spec.workflow.steps[0].subSteps[0].type", "spec.workflow.steps[0].subSteps[1].type"}, + expectedErrorMsgs: []string{"cannot get WorkflowStepDefinition \"suspend\"", "cannot get WorkflowStepDefinition \"notification\""}, + }, + { + name: "duplicate definitions only checked once", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, + }, + }, + { + Name: "comp2", + Type: "webservice", // Duplicate + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, // Duplicate + }, + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": false, + "componentdefinitions/test-ns/webservice": false, + "traitdefinitions/vela-system/scaler": false, + "traitdefinitions/test-ns/scaler": false, + }, + expectedErrorCount: 4, // 2 components + 2 traits + expectedErrorFields: []string{ + "spec.components[0].type", + "spec.components[1].type", + "spec.components[0].traits[0].type", + "spec.components[1].traits[0].type", + }, + }, + { + name: "application in vela-system namespace", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: oam.SystemDefinitionNamespace, // vela-system + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + }, + userInfo: authenticationv1.UserInfo{ + Username: "system-user", + Groups: []string{"system-group"}, + }, + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": false, + }, + expectedErrorCount: 1, + expectedErrorFields: []string{"spec.components[0].type"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Create a fake client with mock SubjectAccessReview behavior + scheme := runtime.NewScheme() + _ = v1beta1.AddToScheme(scheme) + _ = authv1.AddToScheme(scheme) + + fakeClient := &mockSARClient{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + allowedDefinitions: tc.allowedDefinitions, + } + + handler := &ValidatingHandler{ + Client: fakeClient, + } + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: tc.userInfo, + }, + } + + // Run the validation + errs := handler.ValidateDefinitionPermissions(context.Background(), tc.app, req) + + // Check error count + assert.Equal(t, tc.expectedErrorCount, len(errs), + "Expected %d errors, got %d: %v", tc.expectedErrorCount, len(errs), errs) + + // Check error fields and messages (order independent) + if len(tc.expectedErrorFields) > 0 { + actualFields := make([]string, len(errs)) + for i, err := range errs { + actualFields[i] = err.Field + } + for _, expectedField := range tc.expectedErrorFields { + assert.Contains(t, actualFields, expectedField, + "Expected field %s not found in errors: %v", expectedField, actualFields) + } + } + if len(tc.expectedErrorMsgs) > 0 { + actualMessages := make([]string, len(errs)) + for i, err := range errs { + actualMessages[i] = err.Detail + } + for _, expectedMsg := range tc.expectedErrorMsgs { + found := false + for _, actualMsg := range actualMessages { + if strings.Contains(actualMsg, expectedMsg) { + found = true + break + } + } + assert.True(t, found, + "Expected message containing %s not found in errors: %v", expectedMsg, actualMessages) + } + } + }) + } +} + +func TestValidateDefinitionPermissions_FeatureDisabled(t *testing.T) { + // Disable the definition validation feature + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, features.ValidateDefinitionPermissions, false) + oldAuthWithUser := auth.AuthenticationWithUser + auth.AuthenticationWithUser = true + defer func() { auth.AuthenticationWithUser = oldAuthWithUser }() + + app := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + } + + handler := &ValidatingHandler{ + Client: fake.NewClientBuilder().Build(), + } + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + }, + }, + } + + // Should return no errors when feature is disabled + errs := handler.ValidateDefinitionPermissions(context.Background(), app, req) + assert.Empty(t, errs, "Expected no errors when feature is disabled") +} + +func TestValidateDefinitionPermissions_AuthenticationDisabled(t *testing.T) { + // Enable the definition validation feature but disable authentication + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, features.ValidateDefinitionPermissions, true) + oldAuthWithUser := auth.AuthenticationWithUser + auth.AuthenticationWithUser = false + defer func() { auth.AuthenticationWithUser = oldAuthWithUser }() + + app := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "test-ns", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + } + + handler := &ValidatingHandler{ + Client: fake.NewClientBuilder().Build(), + } + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + }, + }, + } + + // Should return no errors when AuthenticationWithUser is disabled + errs := handler.ValidateDefinitionPermissions(context.Background(), app, req) + assert.Empty(t, errs, "Expected no errors when AuthenticationWithUser is disabled") +} + +func TestCollectDefinitionUsage(t *testing.T) { + app := &v1beta1.Application{ + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, + {Type: "gateway"}, + }, + }, + { + Name: "comp2", + Type: "webservice", // Duplicate + Traits: []common.ApplicationTrait{ + {Type: "scaler"}, // Duplicate + }, + }, + }, + Policies: []v1beta1.AppPolicy{ + {Name: "policy1", Type: "topology"}, + {Name: "policy2", Type: "override"}, + {Name: "policy3", Type: "topology"}, // Duplicate + }, + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", + Type: "deploy", + }, + SubSteps: []workflowv1alpha1.WorkflowStepBase{ + { + Name: "substep1", + Type: "suspend", + }, + { + Name: "substep2", + Type: "deploy", // Duplicate + }, + }, + }, + }, + }, + }, + } + + usage := collectDefinitionUsage(app) + + // Check component types + assert.Equal(t, 2, len(usage.componentTypes["webservice"])) + assert.Contains(t, usage.componentTypes["webservice"], 0) + assert.Contains(t, usage.componentTypes["webservice"], 1) + + // Check trait types + assert.Equal(t, 2, len(usage.traitTypes["scaler"])) + assert.Equal(t, 1, len(usage.traitTypes["gateway"])) + + // Check policy types + assert.Equal(t, 2, len(usage.policyTypes["topology"])) + assert.Equal(t, 1, len(usage.policyTypes["override"])) + + // Check workflow step types + assert.Equal(t, 2, len(usage.workflowStepTypes["deploy"])) + assert.Equal(t, 1, len(usage.workflowStepTypes["suspend"])) + + // Check that substeps are properly marked + for _, loc := range usage.workflowStepTypes["suspend"] { + assert.True(t, loc.IsSubStep, "Suspend should be marked as a substep") + assert.Equal(t, 0, loc.StepIndex, "Suspend should be in step 0") + assert.Equal(t, 0, loc.SubStepIndex, "Suspend should be substep 0") + } + + // Check regular steps and substeps for "deploy" + deployLocations := usage.workflowStepTypes["deploy"] + assert.Equal(t, 2, len(deployLocations)) + regularStepFound := false + subStepFound := false + for _, loc := range deployLocations { + if !loc.IsSubStep { + regularStepFound = true + assert.Equal(t, 0, loc.StepIndex) + assert.Equal(t, -1, loc.SubStepIndex) + } else { + subStepFound = true + assert.Equal(t, 0, loc.StepIndex) + assert.Equal(t, 1, loc.SubStepIndex) + } + } + assert.True(t, regularStepFound, "Should have regular deploy step") + assert.True(t, subStepFound, "Should have deploy substep") +} + +func TestGetWorkflowStepFieldPath(t *testing.T) { + testCases := []struct { + name string + location workflowStepLocation + expectedPath string + }{ + { + name: "regular step", + location: workflowStepLocation{ + StepIndex: 0, + SubStepIndex: -1, + IsSubStep: false, + }, + expectedPath: "spec.workflow.steps[0].type", + }, + { + name: "regular step index 5", + location: workflowStepLocation{ + StepIndex: 5, + SubStepIndex: -1, + IsSubStep: false, + }, + expectedPath: "spec.workflow.steps[5].type", + }, + { + name: "substep 0 of step 0", + location: workflowStepLocation{ + StepIndex: 0, + SubStepIndex: 0, + IsSubStep: true, + }, + expectedPath: "spec.workflow.steps[0].subSteps[0].type", + }, + { + name: "substep 2 of step 1", + location: workflowStepLocation{ + StepIndex: 1, + SubStepIndex: 2, + IsSubStep: true, + }, + expectedPath: "spec.workflow.steps[1].subSteps[2].type", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + path := getWorkflowStepFieldPath(tc.location) + assert.Equal(t, tc.expectedPath, path.String()) + }) + } +} + +// mockSARClient is a mock client that simulates SubjectAccessReview responses +type mockSARClient struct { + client.Client + allowedDefinitions map[string]bool +} + +func (m *mockSARClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { + if sar, ok := obj.(*authv1.SubjectAccessReview); ok { + // Mock failures + if sar.Spec.ResourceAttributes.Name == "error-trigger" { + return fmt.Errorf("simulated SAR API failure: connection timeout") + } + if sar.Spec.ResourceAttributes.Name == "system-error-only" && + sar.Spec.ResourceAttributes.Namespace == "vela-system" { + return fmt.Errorf("simulated SAR API failure: system namespace unreachable") + } + + key := fmt.Sprintf("%s/%s/%s", + sar.Spec.ResourceAttributes.Resource, + sar.Spec.ResourceAttributes.Namespace, + sar.Spec.ResourceAttributes.Name) + + if allowed, exists := m.allowedDefinitions[key]; exists { + sar.Status.Allowed = allowed + } else { + sar.Status.Allowed = false + } + return nil + } + return m.Client.Create(ctx, obj, opts...) +} diff --git a/pkg/webhook/core.oam.dev/v1beta1/componentdefinition/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/componentdefinition/validating_handler.go index d9c217444..8622e21ae 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/componentdefinition/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/componentdefinition/validating_handler.go @@ -35,7 +35,7 @@ import ( webhookutils "github.com/oam-dev/kubevela/pkg/webhook/utils" ) -var componentDefGVR = v1beta1.SchemeGroupVersion.WithResource("componentdefinitions") +var componentDefGVR = v1beta1.ComponentDefinitionGVR // ValidatingHandler handles validation of component definition type ValidatingHandler struct { diff --git a/pkg/webhook/core.oam.dev/v1beta1/policydefinition/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/policydefinition/validating_handler.go index 1414271f6..464c7e85f 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/policydefinition/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/policydefinition/validating_handler.go @@ -32,7 +32,7 @@ import ( webhookutils "github.com/oam-dev/kubevela/pkg/webhook/utils" ) -var policyDefGVR = v1beta1.SchemeGroupVersion.WithResource("policydefinitions") +var policyDefGVR = v1beta1.PolicyDefinitionGVR // ValidatingHandler handles validation of policy definition type ValidatingHandler struct { diff --git a/pkg/webhook/core.oam.dev/v1beta1/traitdefinition/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/traitdefinition/validating_handler.go index f85fb8a03..a850ae301 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/traitdefinition/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/traitdefinition/validating_handler.go @@ -42,7 +42,7 @@ const ( failInfoDefRefOmitted = "if definition reference is omitted, patch or output with GVK is required" ) -var traitDefGVR = v1beta1.SchemeGroupVersion.WithResource("traitdefinitions") +var traitDefGVR = v1beta1.TraitDefinitionGVR // ValidatingHandler handles validation of trait definition type ValidatingHandler struct { diff --git a/pkg/webhook/core.oam.dev/v1beta1/workflowstepdefinition/validating_handler.go b/pkg/webhook/core.oam.dev/v1beta1/workflowstepdefinition/validating_handler.go index b33045cad..f430d368f 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/workflowstepdefinition/validating_handler.go +++ b/pkg/webhook/core.oam.dev/v1beta1/workflowstepdefinition/validating_handler.go @@ -32,7 +32,7 @@ import ( webhookutils "github.com/oam-dev/kubevela/pkg/webhook/utils" ) -var workflowStepDefGVR = v1beta1.SchemeGroupVersion.WithResource("workflowstepdefinitions") +var workflowStepDefGVR = v1beta1.WorkflowStepDefinitionGVR // ValidatingHandler handles validation of workflow step definition type ValidatingHandler struct {