From 0302fa373fd9a12ef5c42478bb4e3c4b7944324c Mon Sep 17 00:00:00 2001 From: James Glennan Date: Tue, 8 Oct 2024 09:37:19 +1100 Subject: [PATCH 1/4] Add CertificateRequest username to CEL Validator with serviceaccount functions Signed-off-by: James Glennan --- go.mod | 2 +- .../validation/certificaterequest.pb.go | 24 +++++-- .../validation/certificaterequest.proto | 1 + .../approver/validation/serviceaccount.go | 64 +++++++++++++++++++ pkg/internal/approver/validation/validator.go | 3 + .../approver/validation/validator_test.go | 46 +++++++++++++ 6 files changed, 132 insertions(+), 8 deletions(-) create mode 100644 pkg/internal/approver/validation/serviceaccount.go diff --git a/go.mod b/go.mod index bea50914..746d7cc5 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( k8s.io/api v0.31.1 k8s.io/apiextensions-apiserver v0.31.1 k8s.io/apimachinery v0.31.1 + k8s.io/apiserver v0.31.1 k8s.io/cli-runtime v0.31.1 k8s.io/client-go v0.31.1 k8s.io/component-base v0.31.1 @@ -111,7 +112,6 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect - k8s.io/apiserver v0.31.1 // indirect k8s.io/kube-openapi v0.0.0-20240903163716-9e1beecbcb38 // indirect sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.30.3 // indirect sigs.k8s.io/gateway-api v1.1.0 // indirect diff --git a/pkg/internal/approver/validation/certificaterequest.pb.go b/pkg/internal/approver/validation/certificaterequest.pb.go index 8435d578..c8698776 100644 --- a/pkg/internal/approver/validation/certificaterequest.pb.go +++ b/pkg/internal/approver/validation/certificaterequest.pb.go @@ -27,6 +27,7 @@ type CertificateRequest struct { Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` Namespace string `protobuf:"bytes,2,opt,name=namespace,proto3" json:"namespace,omitempty"` + Username string `protobuf:"bytes,3,opt,name=username,proto3" json:"username,omitempty"` } func (x *CertificateRequest) Reset() { @@ -75,6 +76,13 @@ func (x *CertificateRequest) GetNamespace() string { return "" } +func (x *CertificateRequest) GetUsername() string { + if x != nil { + return x.Username + } + return "" +} + var File_pkg_internal_approver_validation_certificaterequest_proto protoreflect.FileDescriptor var file_pkg_internal_approver_validation_certificaterequest_proto_rawDesc = []byte{ @@ -84,17 +92,19 @@ var file_pkg_internal_approver_validation_certificaterequest_proto_rawDesc = []b 0x71, 0x75, 0x65, 0x73, 0x74, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x2d, 0x63, 0x6d, 0x2e, 0x69, 0x6f, 0x2e, 0x70, 0x6f, 0x6c, 0x69, 0x63, 0x79, 0x2e, 0x70, 0x6b, 0x67, 0x2e, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2e, 0x61, 0x70, 0x70, 0x72, 0x6f, 0x76, 0x65, 0x72, 0x2e, - 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x46, 0x0a, 0x12, 0x43, 0x65, + 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x22, 0x62, 0x0a, 0x12, 0x43, 0x65, 0x72, 0x74, 0x69, 0x66, 0x69, 0x63, 0x61, 0x74, 0x65, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x12, 0x0a, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x09, 0x52, 0x04, 0x6e, 0x61, 0x6d, 0x65, 0x12, 0x1c, 0x0a, 0x09, 0x6e, 0x61, 0x6d, 0x65, 0x73, 0x70, 0x61, 0x63, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x09, 0x6e, 0x61, 0x6d, 0x65, 0x73, 0x70, 0x61, - 0x63, 0x65, 0x42, 0x4a, 0x5a, 0x48, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, - 0x2f, 0x63, 0x65, 0x72, 0x74, 0x2d, 0x6d, 0x61, 0x6e, 0x61, 0x67, 0x65, 0x72, 0x2f, 0x61, 0x70, - 0x70, 0x72, 0x6f, 0x76, 0x65, 0x72, 0x2d, 0x70, 0x6f, 0x6c, 0x69, 0x63, 0x79, 0x2f, 0x70, 0x6b, - 0x67, 0x2f, 0x69, 0x6e, 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x61, 0x70, 0x70, 0x72, 0x6f, - 0x76, 0x65, 0x72, 0x2f, 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x62, 0x06, - 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x63, 0x65, 0x12, 0x1a, 0x0a, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x03, + 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x42, 0x4a, + 0x5a, 0x48, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x63, 0x65, 0x72, + 0x74, 0x2d, 0x6d, 0x61, 0x6e, 0x61, 0x67, 0x65, 0x72, 0x2f, 0x61, 0x70, 0x70, 0x72, 0x6f, 0x76, + 0x65, 0x72, 0x2d, 0x70, 0x6f, 0x6c, 0x69, 0x63, 0x79, 0x2f, 0x70, 0x6b, 0x67, 0x2f, 0x69, 0x6e, + 0x74, 0x65, 0x72, 0x6e, 0x61, 0x6c, 0x2f, 0x61, 0x70, 0x70, 0x72, 0x6f, 0x76, 0x65, 0x72, 0x2f, + 0x76, 0x61, 0x6c, 0x69, 0x64, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, + 0x6f, 0x33, } var ( diff --git a/pkg/internal/approver/validation/certificaterequest.proto b/pkg/internal/approver/validation/certificaterequest.proto index 8793fe56..67ff8b5e 100644 --- a/pkg/internal/approver/validation/certificaterequest.proto +++ b/pkg/internal/approver/validation/certificaterequest.proto @@ -7,4 +7,5 @@ option go_package = "github.com/cert-manager/approver-policy/pkg/internal/approv message CertificateRequest { string name = 1; string namespace = 2; + string username = 3; } diff --git a/pkg/internal/approver/validation/serviceaccount.go b/pkg/internal/approver/validation/serviceaccount.go new file mode 100644 index 00000000..d80590c5 --- /dev/null +++ b/pkg/internal/approver/validation/serviceaccount.go @@ -0,0 +1,64 @@ +package validation + +import ( + "github.com/google/cel-go/cel" + "github.com/google/cel-go/common/types" + "github.com/google/cel-go/common/types/ref" + "k8s.io/apiserver/pkg/authentication/serviceaccount" +) + +func ServiceAccount() cel.EnvOption { + return cel.Lib(saLib) +} + +var saLib = &sa{} + +type sa struct{} + +var saLibraryDecls = map[string][]cel.FunctionOpt{ + "serviceaccount.getName": { + cel.Overload("serviceaccount_get_name", []*cel.Type{cel.StringType}, cel.StringType, + cel.UnaryBinding(getServiceAccountName))}, + "serviceaccount.getNamespace": { + cel.Overload("serviceaccount_get_namespace", []*cel.Type{cel.StringType}, cel.StringType, + cel.UnaryBinding(getServiceAccountNamespace))}, +} + +func getServiceAccountName(arg ref.Val) ref.Val { + s, ok := arg.Value().(string) + if !ok { + return types.MaybeNoSuchOverloadErr(arg) + } + + _, name, err := serviceaccount.SplitUsername(s) + if err != nil { + // Return an empty string if unable to parse the username field for circumstances where non-k8s serviceaccount username is presented + return types.String("") + } + return types.String(name) +} + +func getServiceAccountNamespace(arg ref.Val) ref.Val { + s, ok := arg.Value().(string) + if !ok { + return types.MaybeNoSuchOverloadErr(arg) + } + namespace, _, err := serviceaccount.SplitUsername(s) + if err != nil { + // Return an empty string if unable to parse the username field for circumstances where non-k8s serviceaccount username is presented + return types.String("") + } + return types.String(namespace) +} + +func (*sa) CompileOptions() []cel.EnvOption { + options := []cel.EnvOption{} + for name, overloads := range saLibraryDecls { + options = append(options, cel.Function(name, overloads...)) + } + return options +} + +func (*sa) ProgramOptions() []cel.ProgramOption { + return []cel.ProgramOption{} +} diff --git a/pkg/internal/approver/validation/validator.go b/pkg/internal/approver/validation/validator.go index 853e0bb4..b5f0367e 100644 --- a/pkg/internal/approver/validation/validator.go +++ b/pkg/internal/approver/validation/validator.go @@ -61,7 +61,9 @@ func (v *validator) compile() error { cel.Variable(varSelf, cel.StringType), cel.Variable(varRequest, cel.ObjectType("cm.io.policy.pkg.internal.approver.validation.CertificateRequest")), ext.Strings(), + ServiceAccount(), ) + if err != nil { return err } @@ -89,6 +91,7 @@ func (v *validator) Validate(value string, request cmapi.CertificateRequest) (bo varRequest: &CertificateRequest{ Name: request.GetName(), Namespace: request.GetNamespace(), + Username: request.Spec.Username, }, } diff --git a/pkg/internal/approver/validation/validator_test.go b/pkg/internal/approver/validation/validator_test.go index f1f9647e..925b2e37 100644 --- a/pkg/internal/approver/validation/validator_test.go +++ b/pkg/internal/approver/validation/validator_test.go @@ -38,6 +38,8 @@ func Test_Validator_Compile(t *testing.T) { {name: "err-undeclared-vars", expr: "foo = bar", wantErr: true}, {name: "err-must-return-bool", expr: "size('foo')", wantErr: true}, {name: "err-invalid-property", expr: "size(cr.foo) < 24", wantErr: true}, + {name: "check-username-property", expr: "size(cr.username) > 0", wantErr: false}, + {name: "check-serviceaccount-getname", expr: "self.startsWith(serviceaccount.getName(cr.username))", wantErr: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -91,3 +93,47 @@ func newCertificateRequest(namespace string) cmapi.CertificateRequest { request.SetNamespace(namespace) return request } + +func Test_Validator_Validate_ServiceAccount(t *testing.T) { + v := &validator{expression: "self.startsWith('spiffe://acme.com/ns/%s/sa/%s'.format([serviceaccount.getNamespace(cr.username),serviceaccount.getName(cr.username)]))"} + err := v.compile() + assert.NoError(t, err) + + type args struct { + val string + cr cmapi.CertificateRequest + } + tests := []struct { + name string + args args + want bool + wantErr bool + }{ + {name: "correct-namespace-and-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("system:serviceaccount:foo-ns:bar")}, want: true}, + {name: "correct-namespace-wrong-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/foo", cr: newCertificateRequestWithUsername("system:serviceaccount:foo-ns:bar")}, want: false}, + {name: "wrong-namespace-correct-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("system:serviceaccount:bar-ns:bar")}, want: false}, + {name: "not-serviceaccount", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("bar")}, want: false}, + {name: "unrelated", args: args{val: "spiffe://example.com", cr: newCertificateRequestWithUsername("system:serviceaccount:foo-ns:bar")}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := v.Validate(tt.args.val, tt.args.cr) + if tt.wantErr { + assert.Error(t, err) + return + } else { + assert.NoError(t, err) + } + assert.Equal(t, tt.want, got) + }) + } +} + +func newCertificateRequestWithUsername(username string) cmapi.CertificateRequest { + request := cmapi.CertificateRequest{ + Spec: cmapi.CertificateRequestSpec{ + Username: username, + }, + } + return request +} From 51ba5bc59e0e59fa37537c7df435081f799cb3e6 Mon Sep 17 00:00:00 2001 From: James Glennan Date: Wed, 9 Oct 2024 12:04:12 +1100 Subject: [PATCH 2/4] Switch Serviceaccount library to follor URL library in k8s Signed-off-by: James Glennan --- .../approver/validation/serviceaccount.go | 123 ++++++++++++++---- pkg/internal/approver/validation/validator.go | 2 +- .../approver/validation/validator_test.go | 7 +- 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/pkg/internal/approver/validation/serviceaccount.go b/pkg/internal/approver/validation/serviceaccount.go index d80590c5..7bae6a3e 100644 --- a/pkg/internal/approver/validation/serviceaccount.go +++ b/pkg/internal/approver/validation/serviceaccount.go @@ -1,57 +1,136 @@ package validation import ( + "fmt" + reflect "reflect" + "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" "github.com/google/cel-go/common/types/ref" "k8s.io/apiserver/pkg/authentication/serviceaccount" ) -func ServiceAccount() cel.EnvOption { - return cel.Lib(saLib) +func ServiceAccountLib() cel.EnvOption { + return cel.Lib(&saLib{}) +} + +type saLib struct{} +type ServiceAccount struct { + Name string + Namespace string + IsServiceAccount bool } -var saLib = &sa{} +var ( + SAType = cel.ObjectType("cm.io.policy.pkg.internal.approver.validation.ServiceAccount") +) + +// ConvertToNative implements ref.Val.ConvertToNative. +func (sa ServiceAccount) ConvertToNative(typeDesc reflect.Type) (interface{}, error) { + if reflect.TypeOf(sa).AssignableTo(typeDesc) { + return sa, nil + } + if reflect.TypeOf("").AssignableTo(typeDesc) { + return sa, nil + } + return nil, fmt.Errorf("type conversion error from 'serviceaccount' to '%v'", typeDesc) +} -type sa struct{} +// ConvertToType implements ref.Val.ConvertToType. +func (sa ServiceAccount) ConvertToType(typeVal ref.Type) ref.Val { + switch typeVal { + case SAType: + return sa + case types.TypeType: + return SAType + } + return types.NewErr("type conversion error from '%s' to '%s'", SAType, typeVal) +} + +// Equal implements ref.Val.Equal. +func (sa ServiceAccount) Equal(other ref.Val) ref.Val { + otherDur, ok := other.(ServiceAccount) + if !ok { + return types.MaybeNoSuchOverloadErr(other) + } + return types.Bool(sa.IsServiceAccount && otherDur.IsServiceAccount && sa.Name == otherDur.Name && sa.Namespace == otherDur.Namespace) +} + +// Type implements ref.Val.Type.Y +func (sa ServiceAccount) Type() ref.Type { + return SAType +} + +// Value implements ref.Val.Value. +func (sa ServiceAccount) Value() interface{} { + return sa +} var saLibraryDecls = map[string][]cel.FunctionOpt{ - "serviceaccount.getName": { - cel.Overload("serviceaccount_get_name", []*cel.Type{cel.StringType}, cel.StringType, + "ServiceAccount": { + cel.Overload("username_to_serviceaccount", []*cel.Type{cel.StringType}, SAType, + cel.UnaryBinding(stringToServiceAccount))}, + "getName": { + cel.MemberOverload("serviceaccount_get_name", []*cel.Type{SAType}, cel.StringType, cel.UnaryBinding(getServiceAccountName))}, - "serviceaccount.getNamespace": { - cel.Overload("serviceaccount_get_namespace", []*cel.Type{cel.StringType}, cel.StringType, + "getNamespace": { + cel.MemberOverload("serviceaccount_get_namespace", []*cel.Type{SAType}, cel.StringType, cel.UnaryBinding(getServiceAccountNamespace))}, + "isServiceAccount": { + cel.MemberOverload("serviceaccount_is_sa", []*cel.Type{SAType}, cel.BoolType, + cel.UnaryBinding(isServiceAccount))}, } -func getServiceAccountName(arg ref.Val) ref.Val { +func stringToServiceAccount(arg ref.Val) ref.Val { s, ok := arg.Value().(string) if !ok { return types.MaybeNoSuchOverloadErr(arg) } - _, name, err := serviceaccount.SplitUsername(s) + ns, name, err := serviceaccount.SplitUsername(s) + if err != nil { - // Return an empty string if unable to parse the username field for circumstances where non-k8s serviceaccount username is presented - return types.String("") + return ServiceAccount{ + Name: "", + Namespace: "", + IsServiceAccount: false, + } + } + + return ServiceAccount{ + Name: name, + Namespace: ns, + IsServiceAccount: true, } - return types.String(name) } -func getServiceAccountNamespace(arg ref.Val) ref.Val { - s, ok := arg.Value().(string) +func isServiceAccount(arg ref.Val) ref.Val { + s, ok := arg.Value().(ServiceAccount) if !ok { return types.MaybeNoSuchOverloadErr(arg) } - namespace, _, err := serviceaccount.SplitUsername(s) - if err != nil { - // Return an empty string if unable to parse the username field for circumstances where non-k8s serviceaccount username is presented - return types.String("") + + return types.Bool(s.IsServiceAccount) +} +func getServiceAccountName(arg ref.Val) ref.Val { + s, ok := arg.Value().(ServiceAccount) + if !ok { + return types.MaybeNoSuchOverloadErr(arg) } - return types.String(namespace) + + return types.String(s.Name) +} + +func getServiceAccountNamespace(arg ref.Val) ref.Val { + s, ok := arg.Value().(ServiceAccount) + if !ok { + return types.MaybeNoSuchOverloadErr(arg) + } + + return types.String(s.Namespace) } -func (*sa) CompileOptions() []cel.EnvOption { +func (*saLib) CompileOptions() []cel.EnvOption { options := []cel.EnvOption{} for name, overloads := range saLibraryDecls { options = append(options, cel.Function(name, overloads...)) @@ -59,6 +138,6 @@ func (*sa) CompileOptions() []cel.EnvOption { return options } -func (*sa) ProgramOptions() []cel.ProgramOption { +func (*saLib) ProgramOptions() []cel.ProgramOption { return []cel.ProgramOption{} } diff --git a/pkg/internal/approver/validation/validator.go b/pkg/internal/approver/validation/validator.go index b5f0367e..e4b311da 100644 --- a/pkg/internal/approver/validation/validator.go +++ b/pkg/internal/approver/validation/validator.go @@ -61,7 +61,7 @@ func (v *validator) compile() error { cel.Variable(varSelf, cel.StringType), cel.Variable(varRequest, cel.ObjectType("cm.io.policy.pkg.internal.approver.validation.CertificateRequest")), ext.Strings(), - ServiceAccount(), + ServiceAccountLib(), ) if err != nil { diff --git a/pkg/internal/approver/validation/validator_test.go b/pkg/internal/approver/validation/validator_test.go index 925b2e37..d95c55fb 100644 --- a/pkg/internal/approver/validation/validator_test.go +++ b/pkg/internal/approver/validation/validator_test.go @@ -39,7 +39,9 @@ func Test_Validator_Compile(t *testing.T) { {name: "err-must-return-bool", expr: "size('foo')", wantErr: true}, {name: "err-invalid-property", expr: "size(cr.foo) < 24", wantErr: true}, {name: "check-username-property", expr: "size(cr.username) > 0", wantErr: false}, - {name: "check-serviceaccount-getname", expr: "self.startsWith(serviceaccount.getName(cr.username))", wantErr: false}, + {name: "check-serviceaccount-getname", expr: "self.startsWith(ServiceAccount(cr.username).getName())", wantErr: false}, + {name: "check-serviceaccount-getnamespace", expr: "self.startsWith(ServiceAccount(cr.username).getNamespace())", wantErr: false}, + {name: "check-serviceaccount-isSA", expr: "ServiceAccount(cr.username).isServiceAccount()", wantErr: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -95,7 +97,7 @@ func newCertificateRequest(namespace string) cmapi.CertificateRequest { } func Test_Validator_Validate_ServiceAccount(t *testing.T) { - v := &validator{expression: "self.startsWith('spiffe://acme.com/ns/%s/sa/%s'.format([serviceaccount.getNamespace(cr.username),serviceaccount.getName(cr.username)]))"} + v := &validator{expression: "ServiceAccount(cr.username).isServiceAccount() && self.startsWith('spiffe://acme.com/ns/%s/sa/%s'.format([ServiceAccount(cr.username).getNamespace(),ServiceAccount(cr.username).getName()]))"} err := v.compile() assert.NoError(t, err) @@ -110,6 +112,7 @@ func Test_Validator_Validate_ServiceAccount(t *testing.T) { wantErr bool }{ {name: "correct-namespace-and-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("system:serviceaccount:foo-ns:bar")}, want: true}, + {name: "correct-namespace-and-name2", args: args{val: "spiffe://acme.com/ns/bar-ns/sa/foo", cr: newCertificateRequestWithUsername("system:serviceaccount:bar-ns:foo")}, want: true}, {name: "correct-namespace-wrong-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/foo", cr: newCertificateRequestWithUsername("system:serviceaccount:foo-ns:bar")}, want: false}, {name: "wrong-namespace-correct-name", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("system:serviceaccount:bar-ns:bar")}, want: false}, {name: "not-serviceaccount", args: args{val: "spiffe://acme.com/ns/foo-ns/sa/bar", cr: newCertificateRequestWithUsername("bar")}, want: false}, From f4a493ae8faf46f9cdd9dca68649c9ce024954a6 Mon Sep 17 00:00:00 2001 From: James Glennan Date: Fri, 11 Oct 2024 09:16:18 +1100 Subject: [PATCH 3/4] Add boilerplate legal notice and switch functions Signed-off-by: James Glennan --- .../approver/validation/serviceaccount.go | 50 +++++++++++++------ .../approver/validation/validator_test.go | 8 +-- 2 files changed, 39 insertions(+), 19 deletions(-) diff --git a/pkg/internal/approver/validation/serviceaccount.go b/pkg/internal/approver/validation/serviceaccount.go index 7bae6a3e..16a0e584 100644 --- a/pkg/internal/approver/validation/serviceaccount.go +++ b/pkg/internal/approver/validation/serviceaccount.go @@ -1,8 +1,24 @@ +/* +Copyright 2024 The cert-manager 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 validation import ( "fmt" - reflect "reflect" + "reflect" "github.com/google/cel-go/cel" "github.com/google/cel-go/common/types" @@ -16,9 +32,8 @@ func ServiceAccountLib() cel.EnvOption { type saLib struct{} type ServiceAccount struct { - Name string - Namespace string - IsServiceAccount bool + Name string + Namespace string } var ( @@ -53,7 +68,7 @@ func (sa ServiceAccount) Equal(other ref.Val) ref.Val { if !ok { return types.MaybeNoSuchOverloadErr(other) } - return types.Bool(sa.IsServiceAccount && otherDur.IsServiceAccount && sa.Name == otherDur.Name && sa.Namespace == otherDur.Namespace) + return types.Bool(sa.Name == otherDur.Name && sa.Namespace == otherDur.Namespace) } // Type implements ref.Val.Type.Y @@ -67,7 +82,7 @@ func (sa ServiceAccount) Value() interface{} { } var saLibraryDecls = map[string][]cel.FunctionOpt{ - "ServiceAccount": { + "serviceAccount": { cel.Overload("username_to_serviceaccount", []*cel.Type{cel.StringType}, SAType, cel.UnaryBinding(stringToServiceAccount))}, "getName": { @@ -77,7 +92,7 @@ var saLibraryDecls = map[string][]cel.FunctionOpt{ cel.MemberOverload("serviceaccount_get_namespace", []*cel.Type{SAType}, cel.StringType, cel.UnaryBinding(getServiceAccountNamespace))}, "isServiceAccount": { - cel.MemberOverload("serviceaccount_is_sa", []*cel.Type{SAType}, cel.BoolType, + cel.Overload("serviceaccount_is_sa", []*cel.Type{cel.StringType}, cel.BoolType, cel.UnaryBinding(isServiceAccount))}, } @@ -91,27 +106,32 @@ func stringToServiceAccount(arg ref.Val) ref.Val { if err != nil { return ServiceAccount{ - Name: "", - Namespace: "", - IsServiceAccount: false, + Name: "", + Namespace: "", } } return ServiceAccount{ - Name: name, - Namespace: ns, - IsServiceAccount: true, + Name: name, + Namespace: ns, } } func isServiceAccount(arg ref.Val) ref.Val { - s, ok := arg.Value().(ServiceAccount) + s, ok := arg.Value().(string) if !ok { return types.MaybeNoSuchOverloadErr(arg) } - return types.Bool(s.IsServiceAccount) + _, _, err := serviceaccount.SplitUsername(s) + + if err != nil { + return types.False + } + + return types.True } + func getServiceAccountName(arg ref.Val) ref.Val { s, ok := arg.Value().(ServiceAccount) if !ok { diff --git a/pkg/internal/approver/validation/validator_test.go b/pkg/internal/approver/validation/validator_test.go index d95c55fb..1ef06d29 100644 --- a/pkg/internal/approver/validation/validator_test.go +++ b/pkg/internal/approver/validation/validator_test.go @@ -39,9 +39,9 @@ func Test_Validator_Compile(t *testing.T) { {name: "err-must-return-bool", expr: "size('foo')", wantErr: true}, {name: "err-invalid-property", expr: "size(cr.foo) < 24", wantErr: true}, {name: "check-username-property", expr: "size(cr.username) > 0", wantErr: false}, - {name: "check-serviceaccount-getname", expr: "self.startsWith(ServiceAccount(cr.username).getName())", wantErr: false}, - {name: "check-serviceaccount-getnamespace", expr: "self.startsWith(ServiceAccount(cr.username).getNamespace())", wantErr: false}, - {name: "check-serviceaccount-isSA", expr: "ServiceAccount(cr.username).isServiceAccount()", wantErr: false}, + {name: "check-serviceaccount-getname", expr: "self.startsWith(serviceAccount(cr.username).getName())", wantErr: false}, + {name: "check-serviceaccount-getnamespace", expr: "self.startsWith(serviceAccount(cr.username).getNamespace())", wantErr: false}, + {name: "check-serviceaccount-isSA", expr: "isServiceAccount(cr.username)", wantErr: false}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -97,7 +97,7 @@ func newCertificateRequest(namespace string) cmapi.CertificateRequest { } func Test_Validator_Validate_ServiceAccount(t *testing.T) { - v := &validator{expression: "ServiceAccount(cr.username).isServiceAccount() && self.startsWith('spiffe://acme.com/ns/%s/sa/%s'.format([ServiceAccount(cr.username).getNamespace(),ServiceAccount(cr.username).getName()]))"} + v := &validator{expression: "isServiceAccount(cr.username) && self.startsWith('spiffe://acme.com/ns/%s/sa/%s'.format([serviceAccount(cr.username).getNamespace(),serviceAccount(cr.username).getName()]))"} err := v.compile() assert.NoError(t, err) From 5ccd683693ff056880eb1388a33f2b6f13d0c688 Mon Sep 17 00:00:00 2001 From: James Glennan Date: Mon, 21 Oct 2024 14:48:09 +1100 Subject: [PATCH 4/4] tweak conversion functions and lint Signed-off-by: James Glennan --- .../approver/validation/serviceaccount.go | 23 ++++++++----------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/pkg/internal/approver/validation/serviceaccount.go b/pkg/internal/approver/validation/serviceaccount.go index 16a0e584..694eaf53 100644 --- a/pkg/internal/approver/validation/serviceaccount.go +++ b/pkg/internal/approver/validation/serviceaccount.go @@ -26,9 +26,9 @@ import ( "k8s.io/apiserver/pkg/authentication/serviceaccount" ) -func ServiceAccountLib() cel.EnvOption { - return cel.Lib(&saLib{}) -} +var ( + SAType = cel.ObjectType("cm.io.policy.pkg.internal.approver.validation.ServiceAccount") +) type saLib struct{} type ServiceAccount struct { @@ -36,9 +36,9 @@ type ServiceAccount struct { Namespace string } -var ( - SAType = cel.ObjectType("cm.io.policy.pkg.internal.approver.validation.ServiceAccount") -) +func ServiceAccountLib() cel.EnvOption { + return cel.Lib(&saLib{}) +} // ConvertToNative implements ref.Val.ConvertToNative. func (sa ServiceAccount) ConvertToNative(typeDesc reflect.Type) (interface{}, error) { @@ -46,7 +46,7 @@ func (sa ServiceAccount) ConvertToNative(typeDesc reflect.Type) (interface{}, er return sa, nil } if reflect.TypeOf("").AssignableTo(typeDesc) { - return sa, nil + return serviceaccount.MakeUsername(sa.Namespace, sa.Name), nil } return nil, fmt.Errorf("type conversion error from 'serviceaccount' to '%v'", typeDesc) } @@ -64,11 +64,11 @@ func (sa ServiceAccount) ConvertToType(typeVal ref.Type) ref.Val { // Equal implements ref.Val.Equal. func (sa ServiceAccount) Equal(other ref.Val) ref.Val { - otherDur, ok := other.(ServiceAccount) + otherSA, ok := other.(ServiceAccount) if !ok { return types.MaybeNoSuchOverloadErr(other) } - return types.Bool(sa.Name == otherDur.Name && sa.Namespace == otherDur.Namespace) + return types.Bool(sa.Name == otherSA.Name && sa.Namespace == otherSA.Namespace) } // Type implements ref.Val.Type.Y @@ -105,10 +105,7 @@ func stringToServiceAccount(arg ref.Val) ref.Val { ns, name, err := serviceaccount.SplitUsername(s) if err != nil { - return ServiceAccount{ - Name: "", - Namespace: "", - } + return types.NewErr("Unable to convert to serviceaccount: err: %s, username: %s", err, s) } return ServiceAccount{