Skip to content

Commit

Permalink
fix: code cleanup, added checks in tests to verify groups are passed …
Browse files Browse the repository at this point in the history 
…in SAR API call

Signed-off-by: Dhiraj Bokde <[email protected]>
  • Loading branch information
@dhirajsb
dhirajsb committed Nov 18, 2024
1 parent 7c326b2 commit 8a90020
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 42 deletions.
6 changes: 5 additions & 1 deletion controllers/auth_config_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -504,13 +504,17 @@ func (r *AuthConfigReconciler) translateAuthConfig(ctx context.Context, authConf
}

var authorinoGroups expressions.Value
// look for authorizationGroups first
if authorization.KubernetesSubjectAccessReview.AuthorizationGroups != nil {
authorinoGroups, err = valueFrom(authorization.KubernetesSubjectAccessReview.AuthorizationGroups)
if err != nil {
return nil, err
}
} else if len(authorization.KubernetesSubjectAccessReview.Groups) > 0 {
// use deprecated Groups property otherwise
authorinoGroups = &json.JSONValue{Static: authorization.KubernetesSubjectAccessReview.Groups}
}
translatedAuthorization.KubernetesAuthz, err = authorization_evaluators.NewKubernetesAuthz(authorinoUser, authorization.KubernetesSubjectAccessReview.Groups, authorinoGroups, authorinoResourceAttributes)
translatedAuthorization.KubernetesAuthz, err = authorization_evaluators.NewKubernetesAuthz(authorinoUser, authorinoGroups, authorinoResourceAttributes)
if err != nil {
return nil, err
}
Expand Down
14 changes: 3 additions & 11 deletions pkg/evaluators/authorization/kubernetes_authz.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ type kubernetesSubjectAccessReviewer interface {
SubjectAccessReviews() kubeAuthzClient.SubjectAccessReviewInterface
}

func NewKubernetesAuthz(user expressions.Value, groups []string, authorizationGroups expressions.Value, resourceAttributes *KubernetesAuthzResourceAttributes) (*KubernetesAuthz, error) {
func NewKubernetesAuthz(user expressions.Value, authorizationGroups expressions.Value, resourceAttributes *KubernetesAuthzResourceAttributes) (*KubernetesAuthz, error) {
config, err := rest.InClusterConfig()
if err != nil {
return nil, err
Expand All @@ -36,7 +36,6 @@ func NewKubernetesAuthz(user expressions.Value, groups []string, authorizationGr

return &KubernetesAuthz{
User: user,
Groups: groups,
AuthorizationGroups: authorizationGroups,
ResourceAttributes: resourceAttributes,
authorizer: k8sClient.AuthorizationV1(),
Expand All @@ -54,7 +53,6 @@ type KubernetesAuthzResourceAttributes struct {

type KubernetesAuthz struct {
User expressions.Value
Groups []string
AuthorizationGroups expressions.Value
ResourceAttributes *KubernetesAuthzResourceAttributes

Expand Down Expand Up @@ -132,14 +130,8 @@ func (k *KubernetesAuthz) Call(pipeline auth.AuthPipeline, ctx gocontext.Context
}
}

if len(k.Groups) > 0 {
subjectAccessReview.Spec.Groups = k.Groups
} else if k.AuthorizationGroups != nil {
resolvedValue, err := k.AuthorizationGroups.ResolveFor(authJSON)
if err != nil {
return nil, err
}
stringJson, err := json.StringifyJSON(resolvedValue)
if k.AuthorizationGroups != nil {
stringJson, err := jsonValueToStr(k.AuthorizationGroups)
if err != nil {
return nil, err
}
Expand Down
40 changes: 10 additions & 30 deletions pkg/evaluators/authorization/kubernetes_authz_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
envoy_auth "github.com/envoyproxy/go-control-plane/envoy/service/auth/v3"
"github.com/golang/mock/gomock"
"gotest.tools/assert"
is "gotest.tools/assert/cmp"
kubeAuthz "k8s.io/api/authorization/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kubeAuthzClient "k8s.io/client-go/kubernetes/typed/authorization/v1"
Expand Down Expand Up @@ -59,10 +60,9 @@ func (client *k8sAuthorizationClientMock) GetRequest() kubeAuthz.SubjectAccessRe
return client.request
}

func newKubernetesAuthz(user expressions.Value, groups []string, authorizationGroups expressions.Value, resourceAttributes *KubernetesAuthzResourceAttributes, subjectAccessReviewResponseStatus kubeAuthz.SubjectAccessReviewStatus) *KubernetesAuthz {
func newKubernetesAuthz(user expressions.Value, authorizationGroups expressions.Value, resourceAttributes *KubernetesAuthzResourceAttributes, subjectAccessReviewResponseStatus kubeAuthz.SubjectAccessReviewStatus) *KubernetesAuthz {
return &KubernetesAuthz{
User: user,
Groups: groups,
AuthorizationGroups: authorizationGroups,
ResourceAttributes: resourceAttributes,

Expand All @@ -81,18 +81,13 @@ func TestKubernetesAuthzNonResource_Allowed(t *testing.T) {
request := &envoy_auth.AttributeContext_HttpRequest{Method: "GET", Path: "/hello"}
pipelineMock.EXPECT().GetHttp().Return(request)

kubernetesAuth := newKubernetesAuthz(
&json.JSONValue{Pattern: "auth.identity.username"},
[]string{},
&json.JSONValue{Pattern: "auth.identity.groups"},
nil,
kubeAuthz.SubjectAccessReviewStatus{Allowed: true, Reason: ""},
)
kubernetesAuth := newKubernetesAuthz(&json.JSONValue{Pattern: "auth.identity.username"}, &json.JSONValue{Pattern: "auth.identity.groups"}, nil, kubeAuthz.SubjectAccessReviewStatus{Allowed: true, Reason: ""})
authorized, err := kubernetesAuth.Call(pipelineMock, context.TODO())

client, _ := kubernetesAuth.authorizer.(subjectAccessReviewTestClient)
requestData := client.GetRequest()
assert.Equal(t, requestData.User, "john")
assert.DeepEqual(t, requestData.Groups, []string{"group1", "group2"})
assert.Equal(t, requestData.NonResourceAttributes.Path, "/hello")
assert.Equal(t, requestData.NonResourceAttributes.Verb, "get")

Expand All @@ -110,18 +105,13 @@ func TestKubernetesAuthzNonResource_Denied(t *testing.T) {
request := &envoy_auth.AttributeContext_HttpRequest{Method: "GET", Path: "/hello"}
pipelineMock.EXPECT().GetHttp().Return(request)

kubernetesAuth := newKubernetesAuthz(
&json.JSONValue{Pattern: "auth.identity.username"},
[]string{},
nil,
nil,
kubeAuthz.SubjectAccessReviewStatus{Allowed: false, Reason: "some-reason"},
)
kubernetesAuth := newKubernetesAuthz(&json.JSONValue{Pattern: "auth.identity.username"}, nil, nil, kubeAuthz.SubjectAccessReviewStatus{Allowed: false, Reason: "some-reason"})
authorized, err := kubernetesAuth.Call(pipelineMock, context.TODO())

client, _ := kubernetesAuth.authorizer.(subjectAccessReviewTestClient)
requestData := client.GetRequest()
assert.Equal(t, requestData.User, "john")
assert.Assert(t, is.Len(requestData.Groups, 0))
assert.Equal(t, requestData.NonResourceAttributes.Path, "/hello")
assert.Equal(t, requestData.NonResourceAttributes.Verb, "get")

Expand All @@ -136,13 +126,7 @@ func TestKubernetesAuthzResource_Allowed(t *testing.T) {
pipelineMock := mock_auth.NewMockAuthPipeline(ctrl)
pipelineMock.EXPECT().GetAuthorizationJSON().Return(`{"context":{"request":{"http":{"method":"GET","path":"/hello"}}},"auth":{"identity":{"username":"john", "groups":["group1","group2"]}}}`)

kubernetesAuth := newKubernetesAuthz(
&json.JSONValue{Pattern: "auth.identity.username"},
[]string{},
&json.JSONValue{Pattern: "auth.identity.groups"},
&KubernetesAuthzResourceAttributes{Namespace: &json.JSONValue{Static: "default"}},
kubeAuthz.SubjectAccessReviewStatus{Allowed: true, Reason: ""},
)
kubernetesAuth := newKubernetesAuthz(&json.JSONValue{Pattern: "auth.identity.username"}, &json.JSONValue{Pattern: "auth.identity.groups"}, &KubernetesAuthzResourceAttributes{Namespace: &json.JSONValue{Static: "default"}}, kubeAuthz.SubjectAccessReviewStatus{Allowed: true, Reason: ""})
authorized, err := kubernetesAuth.Call(pipelineMock, context.TODO())

assert.Check(t, authorized.(bool))
Expand All @@ -151,6 +135,7 @@ func TestKubernetesAuthzResource_Allowed(t *testing.T) {
client, _ := kubernetesAuth.authorizer.(subjectAccessReviewTestClient)
requestData := client.GetRequest()
assert.Equal(t, requestData.User, "john")
assert.DeepEqual(t, requestData.Groups, []string{"group1", "group2"})
assert.Equal(t, requestData.ResourceAttributes.Namespace, "default")
}

Expand All @@ -161,13 +146,7 @@ func TestKubernetesAuthzResource_Denied(t *testing.T) {
pipelineMock := mock_auth.NewMockAuthPipeline(ctrl)
pipelineMock.EXPECT().GetAuthorizationJSON().Return(`{"context":{"request":{"http":{"method":"GET","path":"/hello"}}},"auth":{"identity":{"username":"john"}}}`)

kubernetesAuth := newKubernetesAuthz(
&json.JSONValue{Pattern: "auth.identity.username"},
[]string{},
nil,
&KubernetesAuthzResourceAttributes{Namespace: &json.JSONValue{Static: "default"}},
kubeAuthz.SubjectAccessReviewStatus{Allowed: false, Reason: "some-reason"},
)
kubernetesAuth := newKubernetesAuthz(&json.JSONValue{Pattern: "auth.identity.username"}, &json.JSONValue{Static: []string{"group1", "group2"}}, &KubernetesAuthzResourceAttributes{Namespace: &json.JSONValue{Static: "default"}}, kubeAuthz.SubjectAccessReviewStatus{Allowed: false, Reason: "some-reason"})
authorized, err := kubernetesAuth.Call(pipelineMock, context.TODO())

assert.Check(t, !authorized.(bool))
Expand All @@ -176,5 +155,6 @@ func TestKubernetesAuthzResource_Denied(t *testing.T) {
client, _ := kubernetesAuth.authorizer.(subjectAccessReviewTestClient)
requestData := client.GetRequest()
assert.Equal(t, requestData.User, "john")
assert.DeepEqual(t, requestData.Groups, []string{"group1", "group2"})
assert.Equal(t, requestData.ResourceAttributes.Namespace, "default")
}

0 comments on commit 8a90020

Please sign in to comment.