Fix: Unbundle the X-Definition Validation from Authentication Features (#6904)

Signed-off-by: Brian Kane <briankane1@gmail.com>

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`              |

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 }}

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 }}

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:

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
 	}
 

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)
+		})
+	}
+}

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) {