Compare commits

...

2 Commits

Author SHA1 Message Date
github-actions[bot]
f89622eec7 Fix: Fix issue with imports/packages in status validations (#6963) (#6971)
(cherry picked from commit 8e3749f970)

Signed-off-by: Brian Kane <briankane1@gmail.com>
Co-authored-by: Brian Kane <briankane1@gmail.com>
2025-11-06 18:56:43 -08:00
Ayush Kumar
8401ff4d85 Fix: Prevent namespace admins from accessing vela-system definitions without explicit permissions (#6967)
* fix: webhook validation to check definition existence in namespaces and privilege checks

Signed-off-by: Reetika Malhotra <malhotra.reetika25@gmail.com>
Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: make reviewable changes

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* Update Ingress API version and enhance output validation tests

- Changed Ingress API version from v1beta1 to v1 in multiple test files to align with Kubernetes API updates.
- Added pathType specification to Ingress rules for better compatibility.
- Introduced a new e2e test for validating outputs in ComponentDefinition, TraitDefinition, PolicyDefinition, and WorkflowStepDefinition, ensuring proper handling of valid and invalid resources.
- Enhanced existing tests to check for non-existent CRDs in outputs and validate definitions with mixed valid and invalid resources.

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: update comment for expected error count in definition permissions test

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: improve error handling message in definitionExistsInNamespace function

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: enhance definition permission checks and add corresponding test cases

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: clarify comment for definition permission check in ValidateComponents

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

* fix: add existing definitions to validation permissions tests for improved coverage

Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>

---------

Signed-off-by: Reetika Malhotra <malhotra.reetika25@gmail.com>
Signed-off-by: Ayush Kumar <ayushshyamkumar888@gmail.com>
Co-authored-by: Reetika Malhotra <malhotra.reetika25@gmail.com>
2025-11-06 06:31:05 -08:00
6 changed files with 474 additions and 19 deletions

View File

@@ -199,6 +199,18 @@ webhooks:
admissionReviewVersions: ["v1", "v1beta1"]
sideEffects: None
failurePolicy: Fail
- name: applications.core.oam.dev
clientConfig:
url: https://${HOST_IP}:9445/validating-core-oam-dev-v1beta1-applications
caBundle: ${CA_BUNDLE}
rules:
- apiGroups: ["core.oam.dev"]
apiVersions: ["v1beta1"]
resources: ["applications"]
operations: ["CREATE", "UPDATE"]
admissionReviewVersions: ["v1", "v1beta1"]
sideEffects: None
failurePolicy: Fail
EOF
kubectl apply -f /tmp/webhook-config.yaml

View File

@@ -21,7 +21,6 @@ import (
"strings"
"cuelang.org/go/cue/ast"
"cuelang.org/go/cue/parser"
"cuelang.org/go/cue/token"
)
@@ -111,14 +110,15 @@ func unmarshalField[T ast.Node](field *ast.Field, key string, validator func(T)
}
unquoted := strings.TrimSpace(TrimCueRawString(basicLit.Value))
expr, err := parser.ParseExpr("-", WrapCueStruct(unquoted))
if err != nil {
return fmt.Errorf("unexpected error re-parsing validated %s string: %w", key, err)
structLit, hasImports, hasPackage, parseErr := ParseCueContent(unquoted)
if parseErr != nil {
return fmt.Errorf("unexpected error re-parsing validated %s string: %w", key, parseErr)
}
structLit, ok := expr.(*ast.StructLit)
if !ok {
return fmt.Errorf("expected struct after validation in field %s", key)
if hasImports || hasPackage {
// Keep as string literal to preserve imports/package
return nil
}
statusField.Value = structLit

View File

@@ -22,6 +22,7 @@ import (
"cuelang.org/go/cue/ast"
"cuelang.org/go/cue/format"
"cuelang.org/go/cue/parser"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
@@ -176,6 +177,50 @@ func TestMarshalAndUnmarshalMetadata(t *testing.T) {
`,
expectContains: "$local",
},
{
name: "status details with import statement should work",
input: `
attributes: {
status: {
details: #"""
import "strconv"
replicas: strconv.Atoi(context.output.status.replicas)
"""#
}
}
`,
expectContains: "import \"strconv\"",
},
{
name: "status details with package declaration",
input: `
attributes: {
status: {
details: #"""
package status
ready: true
phase: "Running"
"""#
}
}
`,
expectContains: "package status",
},
{
name: "status details with import cannot bypass validation",
input: `
attributes: {
status: {
details: #"""
import "strings"
data: { nested: "structure" }
"""#
}
}
`,
expectMarshalErr: "unsupported expression type",
},
}
for _, tt := range tests {
@@ -379,6 +424,21 @@ func TestMarshalAndUnmarshalHealthPolicy(t *testing.T) {
`,
expectContains: "isHealth",
},
{
name: "healthPolicy with package declaration",
input: `
attributes: {
status: {
healthPolicy: #"""
package health
isHealth: context.output.status.phase == "Running"
"""#
}
}
`,
expectContains: "package health",
},
}
for _, tt := range tests {
@@ -610,6 +670,96 @@ func TestMarshalAndUnmarshalCustomStatus(t *testing.T) {
`,
expectContains: "message",
},
{
name: "customStatus with import statement should work",
input: `
attributes: {
status: {
customStatus: #"""
import "strings"
message: strings.Join(["hello", "world"], ",")
"""#
}
}
`,
expectContains: "import \"strings\"",
},
{
name: "customStatus with multiple imports",
input: `
attributes: {
status: {
customStatus: #"""
import "strings"
import "strconv"
count: strconv.Atoi("42")
message: strings.Join(["Count", strconv.FormatInt(count, 10)], ": ")
"""#
}
}
`,
expectContains: "import \"strconv\"",
},
{
name: "customStatus with import alias",
input: `
attributes: {
status: {
customStatus: #"""
import str "strings"
message: str.ToUpper(str.Join(["hello", "world"], " "))
"""#
}
}
`,
expectContains: "import str \"strings\"",
},
{
name: "customStatus with package declaration",
input: `
attributes: {
status: {
customStatus: #"""
package mytest
message: "Package test"
"""#
}
}
`,
expectContains: "package mytest",
},
{
name: "customStatus with package and imports",
input: `
attributes: {
status: {
customStatus: #"""
package mytest
import "strings"
message: strings.ToUpper("hello world")
"""#
}
}
`,
expectContains: "package mytest",
},
{
name: "customStatus with import still requires message field",
input: `
attributes: {
status: {
customStatus: #"""
import "strings"
someOtherField: "value"
"""#
}
}
`,
expectMarshalErr: "customStatus must contain a 'message' field",
},
}
for _, tt := range tests {
@@ -951,6 +1101,57 @@ func TestCustomStatusEdgeCases(t *testing.T) {
}
}
func TestMixedFieldsWithAndWithoutImports(t *testing.T) {
input := `
attributes: {
status: {
healthPolicy: #"""
isHealth: context.output.status.phase == "Running"
"""#
customStatus: #"""
import "strings"
message: strings.ToLower(context.output.status.phase)
"""#
}
}
`
file, err := parser.ParseFile("-", input)
require.NoError(t, err)
var rootField *ast.Field
for _, decl := range file.Decls {
if f, ok := decl.(*ast.Field); ok {
rootField = f
break
}
}
require.NotNil(t, rootField)
// Encode (struct -> string)
err = EncodeMetadata(rootField)
require.NoError(t, err)
// Decode (string -> struct/string based on imports)
err = DecodeMetadata(rootField)
require.NoError(t, err)
// Check healthPolicy (no imports) - should be decoded to struct
healthField, ok := GetFieldByPath(rootField, "attributes.status.healthPolicy")
require.True(t, ok)
_, isStruct := healthField.Value.(*ast.StructLit)
assert.True(t, isStruct, "healthPolicy without imports should be decoded to struct")
// Check customStatus (has imports) - should remain as string
customField, ok := GetFieldByPath(rootField, "attributes.status.customStatus")
require.True(t, ok)
basicLit, isString := customField.Value.(*ast.BasicLit)
assert.True(t, isString, "customStatus with imports should remain as string")
if isString {
assert.Contains(t, basicLit.Value, "import \"strings\"")
}
}
func TestBackwardCompatibility(t *testing.T) {
tests := []struct {
name string

View File

@@ -152,18 +152,15 @@ func ValidateCueStringLiteral[T ast.Node](lit *ast.BasicLit, validator func(T) e
return nil
}
wrapped := WrapCueStruct(raw)
expr, err := parser.ParseExpr("-", wrapped)
structLit, _, _, err := ParseCueContent(raw)
if err != nil {
return fmt.Errorf("invalid cue content in string literal: %w", err)
}
node, ok := expr.(T)
node, ok := ast.Node(structLit).(T)
if !ok {
return fmt.Errorf("parsed expression is not of expected type %T", *new(T))
}
return validator(node)
}
@@ -197,6 +194,36 @@ func WrapCueStruct(s string) string {
return fmt.Sprintf("{\n%s\n}", s)
}
// ParseCueContent parses CUE content and extracts struct fields, skipping imports/packages
func ParseCueContent(content string) (*ast.StructLit, bool, bool, error) {
if strings.TrimSpace(content) == "" {
return &ast.StructLit{Elts: []ast.Decl{}}, false, false, nil
}
file, err := parser.ParseFile("-", content)
if err != nil {
return nil, false, false, err
}
hasImports := len(file.Imports) > 0
hasPackage := file.PackageName() != ""
structLit := &ast.StructLit{
Elts: []ast.Decl{},
}
for _, decl := range file.Decls {
switch decl.(type) {
case *ast.ImportDecl, *ast.Package:
// Skip imports and package declarations
default:
structLit.Elts = append(structLit.Elts, decl)
}
}
return structLit, hasImports, hasPackage, nil
}
// FindAndValidateField searches for a field at the top level or within top-level if statements
func FindAndValidateField(sl *ast.StructLit, fieldName string, validator fieldValidator) (found bool, err error) {
// First check top-level fields

View File

@@ -25,6 +25,7 @@ import (
"github.com/kubevela/pkg/controller/sharding"
"github.com/kubevela/pkg/util/singleton"
authv1 "k8s.io/api/authorization/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/util/validation/field"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/klog/v2"
@@ -114,7 +115,7 @@ func (h *ValidatingHandler) ValidateComponents(ctx context.Context, app *v1beta1
// 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
// Check permission in vela-system namespace first since most definitions are there
// This optimizes for the common case and reduces API calls
systemNsSar := &authv1.SubjectAccessReview{
Spec: authv1.SubjectAccessReviewSpec{
@@ -136,8 +137,19 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
}
if systemNsSar.Status.Allowed {
// User has permission in system namespace - no need to check app namespace
return true, nil
// User has permission in system namespace
// Verify the definition actually exists in vela-system
if exists, err := h.definitionExistsInNamespace(ctx, resource, definitionType, oam.SystemDefinitionNamespace); err != nil {
klog.V(4).Infof("Failed to check if %s %q exists in vela-system: %v", resource, definitionType, err)
// On error checking existence, deny access for safety
return false, nil
} else if !exists {
klog.V(4).Infof("%s %q does not exist in vela-system, checking app namespace", resource, definitionType)
// Definition doesn't exist in vela-system, fall through to check app namespace
} else {
// Definition exists in vela-system and user has permission
return true, nil
}
}
// If not in system namespace and app namespace is different, check app namespace
@@ -163,6 +175,17 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
if appNsSar.Status.Allowed {
// User has permission in app namespace
// But we need to verify the definition actually exists in the app namespace
// to prevent users with wildcard permissions from using definitions that only exist in vela-system
if exists, err := h.definitionExistsInNamespace(ctx, resource, definitionType, appNamespace); err != nil {
klog.V(4).Infof("Failed to check if %s %q exists in namespace %q: %v", resource, definitionType, appNamespace, err)
// On error checking existence, deny access for safety
return false, nil
} else if !exists {
klog.V(4).Infof("%s %q does not exist in namespace %q, denying access", resource, definitionType, appNamespace)
return false, nil
}
// Definition exists and user has permission
return true, nil
}
}
@@ -171,6 +194,38 @@ func (h *ValidatingHandler) checkDefinitionPermission(ctx context.Context, req a
return false, nil
}
// definitionExistsInNamespace checks if a definition actually exists in the specified namespace
func (h *ValidatingHandler) definitionExistsInNamespace(ctx context.Context, resource, name, namespace string) (bool, error) {
// Determine the object type based on the resource
var obj client.Object
switch resource {
case "componentdefinitions":
obj = &v1beta1.ComponentDefinition{}
case "traitdefinitions":
obj = &v1beta1.TraitDefinition{}
case "policydefinitions":
obj = &v1beta1.PolicyDefinition{}
case "workflowstepdefinitions":
obj = &v1beta1.WorkflowStepDefinition{}
default:
return false, fmt.Errorf("unknown resource type: %s", resource)
}
// Try to get the definition from the namespace
key := client.ObjectKey{Name: name, Namespace: namespace}
if err := h.Client.Get(ctx, key, obj); err != nil {
if !errors.IsNotFound(err) {
// Handle other errors than not found
return false, err
}
// Definition not found
return false, nil
}
// Definition exists
return true, nil
}
// workflowStepLocation represents the location of a workflow step
type workflowStepLocation struct {
StepIndex int

View File

@@ -26,6 +26,7 @@ import (
admissionv1 "k8s.io/api/admission/v1"
authenticationv1 "k8s.io/api/authentication/v1"
authv1 "k8s.io/api/authorization/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
utilfeature "k8s.io/apiserver/pkg/util/feature"
@@ -55,6 +56,7 @@ func TestValidateDefinitionPermissions(t *testing.T) {
app *v1beta1.Application
userInfo authenticationv1.UserInfo
allowedDefinitions map[string]bool // resource/namespace/name -> allowed
existingDefinitions map[string]bool // namespace/name -> exists
expectedErrorCount int
expectedErrorFields []string
expectedErrorMsgs []string
@@ -101,6 +103,12 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"policydefinitions/vela-system/topology": true,
"workflowstepdefinitions/vela-system/deploy": true,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
"vela-system/scaler": true,
"vela-system/topology": true,
"vela-system/deploy": true,
},
expectedErrorCount: 0,
},
{
@@ -166,6 +174,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"traitdefinitions/vela-system/gateway": false,
"traitdefinitions/test-ns/gateway": false,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
},
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\""},
@@ -206,6 +217,10 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"policydefinitions/vela-system/override": false,
"policydefinitions/test-ns/override": false,
},
existingDefinitions: map[string]bool{
"vela-system/topology": true,
"test-ns/topology": true,
},
expectedErrorCount: 1,
expectedErrorFields: []string{"spec.policies[1].type"},
expectedErrorMsgs: []string{"cannot get PolicyDefinition \"override\""},
@@ -251,6 +266,10 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"componentdefinitions/vela-system/webservice": true,
"componentdefinitions/test-ns/webservice": true,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
"test-ns/webservice": true,
},
expectedErrorCount: 0,
},
{
@@ -325,6 +344,12 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"workflowstepdefinitions/vela-system/notification": false,
"workflowstepdefinitions/test-ns/notification": false,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
"vela-system/ingress": true,
"vela-system/topology": true,
"vela-system/deploy": true,
},
expectedErrorCount: 4,
expectedErrorFields: []string{
"spec.components[0].traits[0].type",
@@ -418,7 +443,11 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"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
existingDefinitions: map[string]bool{
// Definition exists in app namespace
"test-ns/custom-comp": true,
},
expectedErrorCount: 0, // Should pass as user has permission in their namespace
},
{
name: "user has permission in system namespace but not app namespace",
@@ -444,6 +473,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"componentdefinitions/vela-system/webservice": true, // Allowed in system namespace
"componentdefinitions/test-ns/webservice": false,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
},
expectedErrorCount: 0, // Should pass as user has permission in system namespace
},
{
@@ -487,6 +519,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
"workflowstepdefinitions/vela-system/notification": false,
"workflowstepdefinitions/test-ns/notification": false,
},
existingDefinitions: map[string]bool{
"vela-system/deploy": true,
},
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\""},
@@ -561,6 +596,108 @@ func TestValidateDefinitionPermissions(t *testing.T) {
expectedErrorCount: 1,
expectedErrorFields: []string{"spec.components[0].type"},
},
{
name: "namespace admin cannot use vela-system definitions without explicit access",
app: &v1beta1.Application{
ObjectMeta: metav1.ObjectMeta{
Name: "test-app",
Namespace: "test",
},
Spec: v1beta1.ApplicationSpec{
Components: []common.ApplicationComponent{
{
Name: "hello",
Type: "hello-cm", // This definition exists only in vela-system
},
},
},
},
userInfo: authenticationv1.UserInfo{
Username: "system:serviceaccount:test:app-writer",
Groups: []string{"system:serviceaccounts", "system:serviceaccounts:test"},
},
allowedDefinitions: map[string]bool{
// User has wildcard permissions in test namespace
"componentdefinitions/test/hello-cm": true,
// But no explicit access to vela-system
"componentdefinitions/vela-system/hello-cm": false,
},
existingDefinitions: map[string]bool{
// Definition exists in vela-system but not in test namespace
"vela-system/hello-cm": true,
"test/hello-cm": false,
},
expectedErrorCount: 1,
expectedErrorFields: []string{"spec.components[0].type"},
expectedErrorMsgs: []string{"cannot get ComponentDefinition \"hello-cm\""},
},
{
name: "user has vela-system permission but definition does not exist there",
app: &v1beta1.Application{
ObjectMeta: metav1.ObjectMeta{
Name: "test-app",
Namespace: "test",
},
Spec: v1beta1.ApplicationSpec{
Components: []common.ApplicationComponent{
{
Name: "phantom",
Type: "phantom-def", // User has permission but doesn't exist
},
},
},
},
userInfo: authenticationv1.UserInfo{
Username: "phantom-user",
Groups: []string{"phantom-group"},
},
allowedDefinitions: map[string]bool{
// User has explicit permission to phantom-def in vela-system
"componentdefinitions/vela-system/phantom-def": true,
// And also in test namespace
"componentdefinitions/test/phantom-def": true,
},
existingDefinitions: map[string]bool{
// But definition doesn't exist in either namespace
"vela-system/phantom-def": false,
"test/phantom-def": false,
},
expectedErrorCount: 1,
expectedErrorFields: []string{"spec.components[0].type"},
expectedErrorMsgs: []string{"cannot get ComponentDefinition \"phantom-def\""},
},
{
name: "user has vela-system permission but definition only exists in app namespace",
app: &v1beta1.Application{
ObjectMeta: metav1.ObjectMeta{
Name: "test-app",
Namespace: "test",
},
Spec: v1beta1.ApplicationSpec{
Components: []common.ApplicationComponent{
{
Name: "local-only",
Type: "local-only-def", // Exists only in app namespace
},
},
},
},
userInfo: authenticationv1.UserInfo{
Username: "mixed-user",
Groups: []string{"mixed-group"},
},
allowedDefinitions: map[string]bool{
// User has permission in both namespaces
"componentdefinitions/vela-system/local-only-def": true,
"componentdefinitions/test/local-only-def": true,
},
existingDefinitions: map[string]bool{
// Definition only exists in app namespace
"vela-system/local-only-def": false,
"test/local-only-def": true,
},
expectedErrorCount: 0, // Should succeed using test namespace version
},
}
for _, tc := range testCases {
@@ -571,8 +708,9 @@ func TestValidateDefinitionPermissions(t *testing.T) {
_ = authv1.AddToScheme(scheme)
fakeClient := &mockSARClient{
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
allowedDefinitions: tc.allowedDefinitions,
Client: fake.NewClientBuilder().WithScheme(scheme).Build(),
allowedDefinitions: tc.allowedDefinitions,
existingDefinitions: tc.existingDefinitions,
}
handler := &ValidatingHandler{
@@ -696,6 +834,10 @@ func TestValidateDefinitionPermissions_AuthenticationDisabled(t *testing.T) {
"componentdefinitions/vela-system/webservice": true,
"componentdefinitions/test-ns/webservice": true,
},
existingDefinitions: map[string]bool{
"vela-system/webservice": true,
"test-ns/webservice": true,
},
}
handler := &ValidatingHandler{
@@ -865,7 +1007,8 @@ func TestGetWorkflowStepFieldPath(t *testing.T) {
// mockSARClient is a mock client that simulates SubjectAccessReview responses
type mockSARClient struct {
client.Client
allowedDefinitions map[string]bool
allowedDefinitions map[string]bool
existingDefinitions map[string]bool // namespace/name -> exists
}
func (m *mockSARClient) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
@@ -893,3 +1036,20 @@ func (m *mockSARClient) Create(ctx context.Context, obj client.Object, opts ...c
}
return m.Client.Create(ctx, obj, opts...)
}
func (m *mockSARClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
// Handle definition existence checks
switch obj.(type) {
case *v1beta1.ComponentDefinition, *v1beta1.TraitDefinition, *v1beta1.PolicyDefinition, *v1beta1.WorkflowStepDefinition:
defKey := fmt.Sprintf("%s/%s", key.Namespace, key.Name)
if m.existingDefinitions != nil {
if exists, ok := m.existingDefinitions[defKey]; ok && exists {
// Definition exists - return success
return nil
}
}
// Definition not found
return errors.NewNotFound(v1beta1.SchemeGroupVersion.WithResource("componentdefinitions").GroupResource(), key.Name)
}
return m.Client.Get(ctx, key, obj, opts...)
}