diff --git a/charts/vela-core/README.md b/charts/vela-core/README.md index 5f6959856..00664e1e0 100644 --- a/charts/vela-core/README.md +++ b/charts/vela-core/README.md @@ -152,10 +152,10 @@ helm install --create-namespace -n vela-system kubevela kubevela/vela-core --wai | `kubeClient.qps` | The qps for reconcile clients | `400` | | `kubeClient.burst` | The burst for reconcile clients | `600` | | `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.withUser` | Application authentication will impersonate as the request User (must be true for security) | `true` | +| `authentication.defaultUser` | Application authentication will impersonate as the User if no user provided or withUser is false | `kubevela:vela-core` | | `authentication.groupPattern` | Application authentication will impersonate as the request Group that matches the pattern | `kubevela:*` | +| `authorization.definitionValidationEnabled` | Enable definition permission validation for RBAC checks on definitions | `false` | | `sharding.enabled` | When sharding enabled, the controller will run as master mode. Refer to https://github.com/kubevela/kubevela/blob/master/design/vela-core/sharding.md for details. | `false` | | `sharding.schedulableShards` | The shards available for scheduling. If empty, dynamic discovery will be used. | `""` | | `core.metrics.enabled` | Enable metrics for vela-core | `false` | diff --git a/charts/vela-core/templates/NOTES.txt b/charts/vela-core/templates/NOTES.txt index 14cb2559d..9758dc8f2 100644 --- a/charts/vela-core/templates/NOTES.txt +++ b/charts/vela-core/templates/NOTES.txt @@ -30,11 +30,35 @@ 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) }} +{{- if and .Values.authentication.enabled (not .Values.authentication.withUser) }} -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. +WARNING: Authentication is enabled but withUser is disabled. + This configuration provides NO security benefit: + - All applications will run as '{{ .Values.authentication.defaultUser }}' regardless of who creates them + - User groups matching '{{ .Values.authentication.groupPattern }}' are still collected but not used effectively + - Service account annotations are blocked + + To enable true user impersonation for security: + --set authentication.withUser=true +{{- end }} + +{{- if and (not .Values.authorization.definitionValidationEnabled) (not .Values.authentication.enabled) }} + +SECURITY RECOMMENDATION: Both authentication and definition validation are disabled. + If KubeVela is running with cluster-admin or other high-level permissions, + consider enabling one or both security features: + + 1. Authentication with impersonation (recommended for multi-tenant environments): + --set authentication.enabled=true + --set authentication.withUser=true + This makes KubeVela impersonate the requesting user, applying their RBAC permissions. + Note: Both flags must be enabled for user impersonation to work. + + 2. Definition permission validation (lightweight RBAC for definitions): + --set authorization.definitionValidationEnabled=true + This ensures users can only reference definitions they have access to. + + Using both features together provides defense in depth. + Without these protections, users can leverage KubeVela's permissions to deploy + resources beyond their intended access level. {{- end }} diff --git a/charts/vela-core/templates/kubevela-controller.yaml b/charts/vela-core/templates/kubevela-controller.yaml index 80bf80282..bcf8a0e1d 100644 --- a/charts/vela-core/templates/kubevela-controller.yaml +++ b/charts/vela-core/templates/kubevela-controller.yaml @@ -313,8 +313,8 @@ spec: - "--feature-gates=DisableWorkflowContextConfigMapCache={{- .Values.featureGates.disableWorkflowContextConfigMapCache | toString -}}" - "--feature-gates=EnableCueValidation={{- .Values.featureGates.enableCueValidation | toString -}}" - "--feature-gates=EnableApplicationStatusMetrics={{- .Values.featureGates.enableApplicationStatusMetrics | toString -}}" + - "--feature-gates=ValidateDefinitionPermissions={{ .Values.authorization.definitionValidationEnabled | 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 2fa9592e3..f659035c8 100644 --- a/charts/vela-core/values.yaml +++ b/charts/vela-core/values.yaml @@ -290,18 +290,27 @@ kubeClient: burst: 600 ## @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 +## SECURITY NOTE: When enabled WITH authentication.withUser=true, KubeVela impersonates the requesting user +## when deploying resources, ensuring that users cannot deploy resources beyond their RBAC permissions. +## This is strongly recommended when KubeVela has cluster-admin or other high-level permissions. +## @param authentication.withUser Application authentication will impersonate as the request User (must be true for security) +## @param authentication.defaultUser Application authentication will impersonate as the User if no user provided or withUser is false ## @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:* +## @param authorization.definitionValidationEnabled Enable definition permission validation for RBAC checks on definitions +## SECURITY NOTE: If KubeVela is running with cluster-admin or high-level permissions, +## consider enabling this feature and/or authentication.enabled for security: +## - This feature: Validates users can only reference definitions they have RBAC access to +## - authentication.enabled: Makes KubeVela impersonate users, applying their full RBAC permissions +## Both features can be used together for defense in depth. +authorization: + definitionValidationEnabled: false + ## @param sharding.enabled When sharding enabled, the controller will run as master mode. Refer to https://github.com/kubevela/kubevela/blob/master/design/vela-core/sharding.md for details. ## @param sharding.schedulableShards The shards available for scheduling. If empty, dynamic discovery will be used. sharding: diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go index 6d2842c00..a600cba19 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validation.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation.go @@ -33,7 +33,6 @@ import ( "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" ) @@ -319,8 +318,8 @@ func (h *ValidatingHandler) validateDefinitions( // 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) { + // Check if definition validation is enabled + if !utilfeature.DefaultMutableFeatureGate.Enabled(features.ValidateDefinitionPermissions) { return nil } diff --git a/pkg/webhook/core.oam.dev/v1beta1/application/validation_handlers_test.go b/pkg/webhook/core.oam.dev/v1beta1/application/validation_handlers_test.go new file mode 100644 index 000000000..4b926aa7c --- /dev/null +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation_handlers_test.go @@ -0,0 +1,418 @@ +/* +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" + "testing" + + "github.com/kubevela/pkg/controller/sharding" + "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/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/features" + "github.com/oam-dev/kubevela/pkg/oam" +) + +func TestValidateCreate(t *testing.T) { + // Disable the definition validation feature for this test + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, features.ValidateDefinitionPermissions, false) + + // Enable sharding to skip component validation (since we don't have component definitions in tests) + oldSharding := sharding.EnableSharding + sharding.EnableSharding = true + defer func() { sharding.EnableSharding = oldSharding }() + + scheme := runtime.NewScheme() + _ = v1beta1.AddToScheme(scheme) + _ = authv1.AddToScheme(scheme) + + testCases := []struct { + name string + app *v1beta1.Application + expectedErrorCount int + expectedErrorField string + }{ + { + name: "valid application", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + }, + expectedErrorCount: 0, + }, + { + name: "application with both autoUpdate and publishVersion", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + Annotations: map[string]string{ + oam.AnnotationAutoUpdate: "true", + oam.AnnotationPublishVersion: "v1.0.0", + }, + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + }, + expectedErrorCount: 1, + expectedErrorField: "metadata.annotations", + }, + { + name: "application with duplicate workflow step names", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", + Type: "deploy", + }, + }, + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", // Duplicate name + Type: "deploy", + }, + }, + }, + }, + }, + }, + expectedErrorCount: 1, + expectedErrorField: "spec.workflow.steps", + }, + { + name: "application with invalid timeout", + app: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + Workflow: &v1beta1.Workflow{ + Steps: []workflowv1alpha1.WorkflowStep{ + { + WorkflowStepBase: workflowv1alpha1.WorkflowStepBase{ + Name: "step1", + Type: "deploy", + Timeout: "invalid", + }, + }, + }, + }, + }, + }, + expectedErrorCount: 1, + expectedErrorField: "spec.workflow.steps.timeout", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + handler := &ValidatingHandler{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + }, + } + + errs := handler.ValidateCreate(context.Background(), tc.app, req) + assert.Equal(t, tc.expectedErrorCount, len(errs), + "Expected %d errors, got %d: %v", tc.expectedErrorCount, len(errs), errs) + + if tc.expectedErrorField != "" && len(errs) > 0 { + assert.Contains(t, errs[0].Field, tc.expectedErrorField, + "Expected error field to contain %s, got %s", tc.expectedErrorField, errs[0].Field) + } + }) + } +} + +func TestValidateUpdate(t *testing.T) { + // Disable the definition validation feature for this test + featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultMutableFeatureGate, features.ValidateDefinitionPermissions, false) + + // Enable sharding to skip component validation (since we don't have component definitions in tests) + oldSharding := sharding.EnableSharding + sharding.EnableSharding = true + defer func() { sharding.EnableSharding = oldSharding }() + + scheme := runtime.NewScheme() + _ = v1beta1.AddToScheme(scheme) + _ = authv1.AddToScheme(scheme) + + handler := &ValidatingHandler{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + } + + oldApp := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + } + + testCases := []struct { + name string + newApp *v1beta1.Application + expectedErrorCount int + }{ + { + name: "valid update", + newApp: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + { + Name: "comp2", + Type: "worker", + }, + }, + }, + }, + expectedErrorCount: 0, + }, + { + name: "update with invalid annotations", + newApp: &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + Annotations: map[string]string{ + oam.AnnotationAutoUpdate: "true", + oam.AnnotationPublishVersion: "v1.0.0", + }, + }, + Spec: v1beta1.ApplicationSpec{ + Components: []common.ApplicationComponent{ + { + Name: "comp1", + Type: "webservice", + }, + }, + }, + }, + expectedErrorCount: 1, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + req := admission.Request{ + AdmissionRequest: admissionv1.AdmissionRequest{ + UserInfo: authenticationv1.UserInfo{ + Username: "test-user", + Groups: []string{"test-group"}, + }, + }, + } + + errs := handler.ValidateUpdate(context.Background(), tc.newApp, oldApp, req) + assert.Equal(t, tc.expectedErrorCount, len(errs), + "Expected %d errors, got %d: %v", tc.expectedErrorCount, len(errs), errs) + }) + } +} + +func TestValidateTimeout(t *testing.T) { + handler := &ValidatingHandler{} + + testCases := []struct { + name string + timeout string + expectedError bool + }{ + { + name: "valid duration - seconds", + timeout: "30s", + expectedError: false, + }, + { + name: "valid duration - minutes", + timeout: "5m", + expectedError: false, + }, + { + name: "valid duration - hours", + timeout: "2h", + expectedError: false, + }, + { + name: "valid duration - complex", + timeout: "1h30m45s", + expectedError: false, + }, + { + name: "invalid duration", + timeout: "invalid", + expectedError: true, + }, + { + name: "invalid duration - missing unit", + timeout: "30", + expectedError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + errs := handler.ValidateTimeout("test-step", tc.timeout) + if tc.expectedError { + assert.NotEmpty(t, errs, "Expected validation error for timeout %s", tc.timeout) + } else { + assert.Empty(t, errs, "Expected no validation error for timeout %s", tc.timeout) + } + }) + } +} + +func TestValidateAnnotations(t *testing.T) { + handler := &ValidatingHandler{} + + testCases := []struct { + name string + annotations map[string]string + expectedErrorCount int + }{ + { + name: "no annotations", + annotations: nil, + expectedErrorCount: 0, + }, + { + name: "only autoUpdate", + annotations: map[string]string{ + oam.AnnotationAutoUpdate: "true", + }, + expectedErrorCount: 0, + }, + { + name: "only publishVersion", + annotations: map[string]string{ + oam.AnnotationPublishVersion: "v1.0.0", + }, + expectedErrorCount: 0, + }, + { + name: "both autoUpdate and publishVersion", + annotations: map[string]string{ + oam.AnnotationAutoUpdate: "true", + oam.AnnotationPublishVersion: "v1.0.0", + }, + expectedErrorCount: 1, + }, + { + name: "autoUpdate false with publishVersion", + annotations: map[string]string{ + oam.AnnotationAutoUpdate: "false", + oam.AnnotationPublishVersion: "v1.0.0", + }, + expectedErrorCount: 0, + }, + { + name: "autoUpdate true with empty publishVersion", + annotations: map[string]string{ + oam.AnnotationAutoUpdate: "true", + oam.AnnotationPublishVersion: "", + }, + expectedErrorCount: 0, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + app := &v1beta1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + Annotations: tc.annotations, + }, + } + + errs := handler.ValidateAnnotations(context.Background(), app) + assert.Equal(t, tc.expectedErrorCount, len(errs), + "Expected %d errors, got %d: %v", tc.expectedErrorCount, len(errs), 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 index 88aa8ade0..a50ca153a 100644 --- a/pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go +++ b/pkg/webhook/core.oam.dev/v1beta1/application/validation_permissions_test.go @@ -685,8 +685,21 @@ func TestValidateDefinitionPermissions_AuthenticationDisabled(t *testing.T) { }, } + // Use mock client that allows the definition + scheme := runtime.NewScheme() + _ = v1beta1.AddToScheme(scheme) + _ = authv1.AddToScheme(scheme) + + fakeClient := &mockSARClient{ + Client: fake.NewClientBuilder().WithScheme(scheme).Build(), + allowedDefinitions: map[string]bool{ + "componentdefinitions/vela-system/webservice": true, + "componentdefinitions/test-ns/webservice": true, + }, + } + handler := &ValidatingHandler{ - Client: fake.NewClientBuilder().Build(), + Client: fakeClient, } req := admission.Request{ @@ -697,9 +710,10 @@ func TestValidateDefinitionPermissions_AuthenticationDisabled(t *testing.T) { }, } - // Should return no errors when AuthenticationWithUser is disabled + // Definition validation should still work even when AuthenticationWithUser is disabled + // It validates based on the user info in the request regardless of auth flag errs := handler.ValidateDefinitionPermissions(context.Background(), app, req) - assert.Empty(t, errs, "Expected no errors when AuthenticationWithUser is disabled") + assert.Empty(t, errs, "Expected no errors when user has permission even with AuthenticationWithUser disabled") } func TestCollectDefinitionUsage(t *testing.T) {