Added support for new Namespace scope format (#339)

* namespace Signed-off-by: Anumita <[email protected]> * add use new namespace scope format Signed-off-by: Anumita <[email protected]> * added tests Signed-off-by: Anumita <[email protected]> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
kubeguard · Apr 4, 2022 · d2a160d · d2a160d
1 parent 61066e6
commit d2a160d
Show file tree

Hide file tree

Showing 4 changed files with 128 additions and 48 deletions.
diff --git a/authz/providers/azure/options/options.go b/authz/providers/azure/options/options.go
@@ -33,21 +33,23 @@ const (
 )
 
 type Options struct {
-	AuthzMode                      string
-	ResourceId                     string
-	AKSAuthzTokenURL               string
-	ARMCallLimit                   int
-	SkipAuthzCheck                 []string
-	SkipAuthzForNonAADUsers        bool
-	AllowNonResDiscoveryPathAccess bool
+	AuthzMode                       string
+	ResourceId                      string
+	AKSAuthzTokenURL                string
+	ARMCallLimit                    int
+	SkipAuthzCheck                  []string
+	SkipAuthzForNonAADUsers         bool
+	AllowNonResDiscoveryPathAccess  bool
+	UseNamespaceResourceScopeFormat bool
 }
 
 func NewOptions() Options {
 	return Options{
-		ARMCallLimit:                   defaultArmCallLimit,
-		SkipAuthzCheck:                 []string{""},
-		SkipAuthzForNonAADUsers:        true,
-		AllowNonResDiscoveryPathAccess: true,
+		ARMCallLimit:                    defaultArmCallLimit,
+		SkipAuthzCheck:                  []string{""},
+		SkipAuthzForNonAADUsers:         true,
+		AllowNonResDiscoveryPathAccess:  true,
+		UseNamespaceResourceScopeFormat: false,
 	}
 }
 
@@ -59,6 +61,7 @@ func (o *Options) AddFlags(fs *pflag.FlagSet) {
 	fs.StringSliceVar(&o.SkipAuthzCheck, "azure.skip-authz-check", o.SkipAuthzCheck, "name of usernames/email for which authz check will be skipped")
 	fs.BoolVar(&o.SkipAuthzForNonAADUsers, "azure.skip-authz-for-non-aad-users", o.SkipAuthzForNonAADUsers, "skip authz for non AAD users")
 	fs.BoolVar(&o.AllowNonResDiscoveryPathAccess, "azure.allow-nonres-discovery-path-access", o.AllowNonResDiscoveryPathAccess, "allow access on Non Resource paths required for discovery, setting it false will require explicit non resource path role assignment for all users in Azure RBAC")
+	fs.BoolVar(&o.UseNamespaceResourceScopeFormat, "azure.use-ns-resource-scope-format", o.UseNamespaceResourceScopeFormat, "use namespace as resource scope format for making rbac checkaccess calls at namespace scope")
 }
 
 func (o *Options) Validate(azure azure.Options) []error {

diff --git a/authz/providers/azure/rbac/checkaccessreqhelper.go b/authz/providers/azure/rbac/checkaccessreqhelper.go
@@ -32,6 +32,7 @@ const (
 	AccessAllowedVerboseVerdict = "Access allowed by Azure RBAC Role Assignment %s of Role %s to user %s"
 	Allowed                     = "allowed"
 	AccessNotAllowedVerdict     = "User does not have access to the resource in Azure. Update role assignment to allow access."
+	NamespaceResourceFormat     = "/providers/Microsoft.KubernetesConfiguration/namespaces"
 	namespaces                  = "namespaces"
 	NoOpinionVerdict            = "Azure does not have opinion for this user."
 	NonAADUserNoOpVerdict       = "Azure does not have opinion for this non AAD user. If you are an AAD user, please set Extra:oid parameter for impersonated user in the kubeconfig"
@@ -127,9 +128,13 @@ type AuthorizationDecision struct {
 	TimeToLiveInMs      int                 `json:"timeToLiveInMs"`
 }
 
-func getScope(resourceId string, attr *authzv1.ResourceAttributes) string {
+func getScope(resourceId string, attr *authzv1.ResourceAttributes, useNamespaceResourceScopeFormat bool) string {
 	if attr != nil && attr.Namespace != "" {
-		return path.Join(resourceId, namespaces, attr.Namespace)
+		if useNamespaceResourceScopeFormat {
+			return path.Join(resourceId, NamespaceResourceFormat, attr.Namespace)
+		} else {
+			return path.Join(resourceId, namespaces, attr.Namespace)
+		}
 	}
 	return resourceId
 }
@@ -245,7 +250,7 @@ func getResultCacheKey(subRevReq *authzv1.SubjectAccessReviewSpec) string {
 	return cacheKey
 }
 
-func prepareCheckAccessRequestBody(req *authzv1.SubjectAccessReviewSpec, clusterType, resourceId string) (*CheckAccessRequest, error) {
+func prepareCheckAccessRequestBody(req *authzv1.SubjectAccessReviewSpec, clusterType, resourceId string, useNamespaceResourceScopeFormat bool) (*CheckAccessRequest, error) {
 	/* This is how sample SubjectAccessReview request will look like
 		{
 			"kind": "SubjectAccessReview",
@@ -314,15 +319,19 @@ func prepareCheckAccessRequestBody(req *authzv1.SubjectAccessReviewSpec, cluster
 	action := make([]AuthorizationActionInfo, 1)
 	action[0] = getDataAction(req, clusterType)
 	checkaccessreq.Actions = action
-	checkaccessreq.Resource.Id = getScope(resourceId, req.ResourceAttributes)
+	checkaccessreq.Resource.Id = getScope(resourceId, req.ResourceAttributes, useNamespaceResourceScopeFormat)
 
 	return &checkaccessreq, nil
 }
 
-func getNameSpaceScope(req *authzv1.SubjectAccessReviewSpec) (bool, string) {
+func getNameSpaceScope(req *authzv1.SubjectAccessReviewSpec, useNamespaceResourceScopeFormat bool) (bool, string) {
 	var namespace string = ""
 	if req.ResourceAttributes != nil && req.ResourceAttributes.Namespace != "" {
-		namespace = path.Join(namespaces, req.ResourceAttributes.Namespace)
+		if useNamespaceResourceScopeFormat {
+			namespace = path.Join(NamespaceResourceFormat, req.ResourceAttributes.Namespace)
+		} else {
+			namespace = path.Join(namespaces, req.ResourceAttributes.Namespace)
+		}
 		return true, namespace
 	}
 	return false, namespace

diff --git a/authz/providers/azure/rbac/checkaccessreqhelper_test.go b/authz/providers/azure/rbac/checkaccessreqhelper_test.go
@@ -20,29 +20,38 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	authzv1 "k8s.io/api/authorization/v1"
 )
 
+const resourceId = "resourceId"
+
 func Test_getScope(t *testing.T) {
 	type args struct {
-		resourceId string
-		attr       *authzv1.ResourceAttributes
+		resourceId                      string
+		attr                            *authzv1.ResourceAttributes
+		useNamespaceResourceScopeFormat bool
 	}
 	tests := []struct {
 		name string
 		args args
 		want string
 	}{
-		{"nilAttr", args{"resourceId", nil}, "resourceId"},
-		{"bothnil", args{"", nil}, ""},
-		{"emptyRes", args{"", &authzv1.ResourceAttributes{Namespace: ""}}, ""},
-		{"emptyNS", args{"resourceId", &authzv1.ResourceAttributes{Namespace: ""}}, "resourceId"},
-		{"bothPresent", args{"resourceId", &authzv1.ResourceAttributes{Namespace: "test"}}, "resourceId/namespaces/test"},
+		{"nilAttr", args{"resourceId", nil, false}, "resourceId"},
+		{"bothnil", args{"", nil, false}, ""},
+		{"emptyRes", args{"", &authzv1.ResourceAttributes{Namespace: ""}, false}, ""},
+		{"emptyNS", args{"resourceId", &authzv1.ResourceAttributes{Namespace: ""}, false}, "resourceId"},
+		{"bothPresent", args{"resourceId", &authzv1.ResourceAttributes{Namespace: "test"}, false}, "resourceId/namespaces/test"},
+		{"nilAttrNewScope", args{"resourceId", nil, true}, "resourceId"},
+		{"bothnilNewScope", args{"", nil, true}, ""},
+		{"emptyResNewScope", args{"", &authzv1.ResourceAttributes{Namespace: ""}, true}, ""},
+		{"emptyNSNewScope", args{"resourceId", &authzv1.ResourceAttributes{Namespace: ""}, true}, "resourceId"},
+		{"bothPresentNewScope", args{"resourceId", &authzv1.ResourceAttributes{Namespace: "test"}, true}, "resourceId/providers/Microsoft.KubernetesConfiguration/namespaces/test"},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := getScope(tt.args.resourceId, tt.args.attr); got != tt.want {
+			if got := getScope(tt.args.resourceId, tt.args.attr, tt.args.useNamespaceResourceScopeFormat); got != tt.want {
 				t.Errorf("getScope() = %v, want %v", got, tt.want)
 			}
 		})
@@ -228,7 +237,7 @@ func Test_getDataAction(t *testing.T) {
 func Test_getNameSpaceScope(t *testing.T) {
 	req := authzv1.SubjectAccessReviewSpec{ResourceAttributes: nil}
 	want := false
-	got, str := getNameSpaceScope(&req)
+	got, str := getNameSpaceScope(&req, false)
 	if got || str != "" {
 		t.Errorf("Want:%v, got:%v", want, got)
 	}
@@ -237,7 +246,7 @@ func Test_getNameSpaceScope(t *testing.T) {
 		ResourceAttributes: &authzv1.ResourceAttributes{Namespace: ""},
 	}
 	want = false
-	got, str = getNameSpaceScope(&req)
+	got, str = getNameSpaceScope(&req, false)
 	if got || str != "" {
 		t.Errorf("Want:%v, got:%v", want, got)
 	}
@@ -247,38 +256,95 @@ func Test_getNameSpaceScope(t *testing.T) {
 	}
 	outputstring := "namespaces/dev"
 	want = true
-	got, str = getNameSpaceScope(&req)
+	got, str = getNameSpaceScope(&req, false)
+	if !got || str != outputstring {
+		t.Errorf("Want:%v - %s, got: %v - %s", want, outputstring, got, str)
+	}
+}
+
+func Test_getNameSpaceScopeUsingNewNsFormat(t *testing.T) {
+	req := authzv1.SubjectAccessReviewSpec{ResourceAttributes: nil}
+	want := false
+	got, str := getNameSpaceScope(&req, true)
+	if got || str != "" {
+		t.Errorf("Want:%v, got:%v", want, got)
+	}
+
+	req = authzv1.SubjectAccessReviewSpec{
+		ResourceAttributes: &authzv1.ResourceAttributes{Namespace: ""},
+	}
+	want = false
+	got, str = getNameSpaceScope(&req, true)
+	if got || str != "" {
+		t.Errorf("Want:%v, got:%v", want, got)
+	}
+
+	req = authzv1.SubjectAccessReviewSpec{
+		ResourceAttributes: &authzv1.ResourceAttributes{Namespace: "dev"},
+	}
+	outputstring := "/providers/Microsoft.KubernetesConfiguration/namespaces/dev"
+	want = true
+	got, str = getNameSpaceScope(&req, true)
 	if !got || str != outputstring {
 		t.Errorf("Want:%v - %s, got: %v - %s", want, outputstring, got, str)
 	}
 }
 
 func Test_prepareCheckAccessRequestBody(t *testing.T) {
 	req := &authzv1.SubjectAccessReviewSpec{Extra: nil}
-	resouceId := "resourceId"
 	clusterType := "aks"
 	var want *CheckAccessRequest = nil
 	wantErr := errors.New("oid info not sent from authenticatoin module")
 
-	got, gotErr := prepareCheckAccessRequestBody(req, clusterType, resouceId)
+	got, gotErr := prepareCheckAccessRequestBody(req, clusterType, resourceId, false)
 
 	if got != want && gotErr != wantErr {
 		t.Errorf("Want:%v WantErr:%v, got:%v, gotErr:%v", want, wantErr, got, gotErr)
 	}
 
 	req = &authzv1.SubjectAccessReviewSpec{Extra: map[string]authzv1.ExtraValue{"oid": {"test"}}}
-	resouceId = "resourceId"
 	clusterType = "arc"
 	want = nil
 	wantErr = errors.New("oid info sent from authenticatoin module is not valid")
 
-	got, gotErr = prepareCheckAccessRequestBody(req, clusterType, resouceId)
+	got, gotErr = prepareCheckAccessRequestBody(req, clusterType, resourceId, false)
 
 	if got != want && gotErr != wantErr {
 		t.Errorf("Want:%v WantErr:%v, got:%v, gotErr:%v", want, wantErr, got, gotErr)
 	}
 }
 
+func Test_prepareCheckAccessRequestBodyWithNamespace(t *testing.T) {
+	dummyUuid := uuid.New()
+	req := &authzv1.SubjectAccessReviewSpec{ResourceAttributes: &authzv1.ResourceAttributes{Namespace: "dev"}, Extra: map[string]authzv1.ExtraValue{"oid": {dummyUuid.String()}}}
+	clusterType := "aks"
+
+	// testing with new ns scope format
+	var want string = "resourceId/providers/Microsoft.KubernetesConfiguration/namespaces/dev"
+
+	got, gotErr := prepareCheckAccessRequestBody(req, clusterType, resourceId, true)
+
+	if got == nil {
+		t.Errorf("Want: not nil Got: nil, gotErr:%v", gotErr)
+	}
+
+	if got != nil && got.Resource.Id != want {
+		t.Errorf("Want:%v, got:%v", want, got)
+	}
+
+	// testing with the old namespace format
+	want = "resourceId/namespaces/dev"
+
+	got, gotErr = prepareCheckAccessRequestBody(req, clusterType, resourceId, false)
+	if got == nil {
+		t.Errorf("Want: not nil Got: nil, gotErr:%v", gotErr)
+	}
+
+	if got != nil && got.Resource.Id != want {
+		t.Errorf("Want:%v, got:%v", want, got)
+	}
+}
+
 func Test_getResultCacheKey(t *testing.T) {
 	type args struct {
 		subRevReq *authzv1.SubjectAccessReviewSpec

diff --git a/authz/providers/azure/rbac/rbac.go b/authz/providers/azure/rbac/rbac.go
@@ -65,14 +65,15 @@ type AccessInfo struct {
 	// These allow us to mock out the URL for testing
 	apiURL *url.URL
 
-	tokenProvider                  graph.TokenProvider
-	clusterType                    string
-	azureResourceId                string
-	armCallLimit                   int
-	skipCheck                      map[string]void
-	skipAuthzForNonAADUsers        bool
-	allowNonResDiscoveryPathAccess bool
-	lock                           sync.RWMutex
+	tokenProvider                   graph.TokenProvider
+	clusterType                     string
+	azureResourceId                 string
+	armCallLimit                    int
+	skipCheck                       map[string]void
+	skipAuthzForNonAADUsers         bool
+	allowNonResDiscoveryPathAccess  bool
+	useNamespaceResourceScopeFormat bool
+	lock                            sync.RWMutex
 }
 
 var (
@@ -112,12 +113,13 @@ func newAccessInfo(tokenProvider graph.TokenProvider, rbacURL *url.URL, opts aut
 			"Content-Type": []string{"application/json"},
 			"User-Agent":   []string{fmt.Sprintf("guard-%s-%s-%s-%s", v.Version.Platform, v.Version.GoVersion, v.Version.Version, opts.AuthzMode)},
 		},
-		apiURL:                         rbacURL,
-		tokenProvider:                  tokenProvider,
-		azureResourceId:                opts.ResourceId,
-		armCallLimit:                   opts.ARMCallLimit,
-		skipAuthzForNonAADUsers:        opts.SkipAuthzForNonAADUsers,
-		allowNonResDiscoveryPathAccess: opts.AllowNonResDiscoveryPathAccess,
+		apiURL:                          rbacURL,
+		tokenProvider:                   tokenProvider,
+		azureResourceId:                 opts.ResourceId,
+		armCallLimit:                    opts.ARMCallLimit,
+		skipAuthzForNonAADUsers:         opts.SkipAuthzForNonAADUsers,
+		allowNonResDiscoveryPathAccess:  opts.AllowNonResDiscoveryPathAccess,
+		useNamespaceResourceScopeFormat: opts.UseNamespaceResourceScopeFormat,
 	}
 
 	u.skipCheck = make(map[string]void, len(opts.SkipAuthzCheck))
@@ -234,15 +236,15 @@ func (a *AccessInfo) setReqHeaders(req *http.Request) {
 }
 
 func (a *AccessInfo) CheckAccess(request *authzv1.SubjectAccessReviewSpec) (*authzv1.SubjectAccessReviewStatus, error) {
-	checkAccessBody, err := prepareCheckAccessRequestBody(request, a.clusterType, a.azureResourceId)
+	checkAccessBody, err := prepareCheckAccessRequestBody(request, a.clusterType, a.azureResourceId, a.useNamespaceResourceScopeFormat)
 	if err != nil {
 		return nil, errors.Wrap(err, "error in preparing check access request")
 	}
 
 	checkAccessURL := *a.apiURL
 	// Append the path for azure cluster resource id
 	checkAccessURL.Path = path.Join(checkAccessURL.Path, a.azureResourceId)
-	exist, nameSpaceString := getNameSpaceScope(request)
+	exist, nameSpaceString := getNameSpaceScope(request, a.useNamespaceResourceScopeFormat)
 	if exist {
 		checkAccessURL.Path = path.Join(checkAccessURL.Path, nameSpaceString)
 	}