[WIP] Resource yaml based rbac #246

speza · 2020-05-08T22:17:49Z

Massive WIP.

Description TBD.

Todo:

Refinement
Proper testing
Cache eviction
UI (another branch/PR maybe?)

Skarlso

First glance comments. I'll have to do another rounds for a deeper inspection. These are surface remarks for now. :) Generally, I saw what I expect I would see mostly, things I think are missing yet right? Like calling the cache eviction and the likes. Unless my glance missed that :)

Skarlso · 2020-05-10T15:45:47Z

handlers/auth.go

@@ -89,6 +101,17 @@ type AuthConfig struct {
 	RoleCategories []*gaia.UserRoleCategory
 }

+func getPoliciesFromClaims(policyClaims interface{}) []string {
+	policies := []string{}
+	if policyClaims == nil {


If this could potentially be nil inside the interface, this check will not be enough.

You'll have to also check the value in the interface to be nil or not using reflection.

if policyClaims == nil || reflect.ValueOf(policyClaims).IsNil() {

Skarlso · 2020-05-10T15:47:23Z

handlers/auth.go

+	if policyClaims == nil {
+		return policies
+	}
+	for _, p := range policyClaims.([]interface{}) {


Do a type assert here on []interface{} to make sure it doesn't throw a type exception.

Skarlso · 2020-05-10T15:47:31Z

handlers/auth.go

+		return policies
+	}
+	for _, p := range policyClaims.([]interface{}) {
+		policies = append(policies, p.(string))


Skarlso · 2020-05-10T15:48:14Z

handlers/auth.go

@@ -89,6 +101,17 @@ type AuthConfig struct {
 	RoleCategories []*gaia.UserRoleCategory
 }

+func getPoliciesFromClaims(policyClaims interface{}) []string {
+	policies := []string{}


In this case, I'd prefer a named return value for clarity. But I'll leave it up to you. :)

Skarlso · 2020-05-10T15:49:23Z

handlers/handler.go

@@ -2,6 +2,14 @@ package handlers

 import (
 	"net/http"
+	"time"


Please sort the includes according to ( in all other files too ):

stdlib
empty line

outside libraries
empty line

external gaia libraries
empty line

internal gaia libraries.

Just need to sort my fmt out, np :)

@Skarlso are you doing these manually or using a formatter?

By hand, I'm afraid. 😁

Skarlso · 2020-05-10T15:51:45Z

helper/cachehelper/cachehelper.go

+)
+
+// Cache represents the interface for a simple cache.
+type Cache interface {


Since we know that these will be string values, I think this cache doesn't need to be interface valued, right? Unless I'm missing something and there could be other types values?

I thought the cache could be generic enough that we could reuse if required?

we'll alter it when we get there I think. :) We might never reuse it. :)

If we would, it's very easy to change it to interface.

Sure thing 👍

Skarlso · 2020-05-10T15:52:49Z

helper/cachehelper/cachehelper.go

+}
+
+type cacheItem struct {
+	Value      interface{}


Why are these exported?

Skarlso · 2020-05-10T15:56:49Z

security/rbac/enforcer.go

+	// iterate through the user policy names provided
+	for _, policy := range policyNames {
+		// get the policy from the service/cache
+		policyResource, _ := s.svc.GetPolicy(policy)


Do check for errors here though... If the policy doesn't exists, this should throw some kind of warning.

Skarlso · 2020-05-10T15:58:20Z

security/rbac/enforcer.go

+		for _, stmt := range policyResource.Statement {
+			// iterate through the actions in the statement
+			for _, stmtAction := range stmt.Action {
+				// parse the namespace and action from the statement value


We should refactor this O(n^3)... :))) This should be very quick considering this will happen for every request.

Yeah agreed! This was the first basic impl. Now I have some passing tests I will refactor it for sure,

speza · 2020-05-10T16:10:33Z

First glance comments. I'll have to do another rounds for a deeper inspection. These are surface remarks for now. :) Generally, I saw what I expect I would see mostly, things I think are missing yet right? Like calling the cache eviction and the likes. Unless my glance missed that :)

Yep, correct mate. This was a rough PR that is definitely not ready. I just wanted some thoughts before I carried on. Its suprsingly a lot of code to get this system in! Especially to then refine it. I'd usually do this sort of work in many multiple PRs!

I left a short list of the required work still:

Code refinement
Testing
Cache eviction and update when a resource has been modified
UI (another branch/PR maybe?)
Think about how we handle default policies for existing users...

Skarlso · 2020-05-10T19:17:15Z

@speza Quick question. Did we ever consider using Casbin with echo: https://echo.labstack.com/middleware/casbin-auth

Skarlso · 2020-05-10T19:18:42Z

casbin and it's configration based role set + middleware actually looks kind of interesting....
https://casbin.org/docs/en/syntax-for-models

speza · 2020-05-10T19:33:12Z

Hmm @Skarlso. I do remember seeing this in the past. Not for Gaia though. I wonder if this seemed a little heavy weight... but arguably we’re heading towards this now.

- Cache evaluated user permissions (all permissions from all policies combined) - Clear cache when any policy resource is changed - Rename some bits from Auth to RBAC - Store RBAC bindings (user policy bindings)

…encapsulate in service) - Fixed more failing tests

Skarlso

I have some general comments for now and one question regarding the rbac stuff. What do you think?

Skarlso · 2020-05-23T09:44:16Z

handlers/auth.go

+	if _, ok := policyClaims.(map[string]interface{}); !ok {
+		return nil, errors.New("policyClaims is not correct type")
+	}
+	return policyClaims.(map[string]interface{}), nil


Suggested change

if _, ok := policyClaims.(map[string]interface{}); !ok {

return nil, errors.New("policyClaims is not correct type")

}

return policyClaims.(map[string]interface{}), nil

v, ok := policyClaims.(map[string]interface{})

if !ok {

return nil, errors.New("policyClaims is not correct type")

}

return v, nil

Skarlso · 2020-05-23T10:09:24Z

handlers/handler.go

-		e.DELETE(p+"pipeline/:pipelineid", pipelineProvider.PipelineDelete)
-		e.POST(p+"pipeline/:pipelineid/start", pipelineProvider.PipelineStart)
+
+		e.POST(p+"pipeline", pipelineProvider.CreatePipeline, policyEnforcer.do(rbac.PipelineNamespace, rbac.CreateAction))


So, I don't think we need this here. You can register a high level rbac middleware and that should just get what resources and actions are available from the rbac policy based on the URI that the middleware gets.

There could be another yaml mapping providing this information. That way, we can change things on the fly and there won't be any need for rebuilding.

What do you think?

Let me have a think mate. I did something similar, albeit structs instead of YAML with the old system.

Skarlso · 2020-05-23T13:13:24Z

handlers/rbac.go

+}
+
+// RBACPolicyResourcePut creates or updates a new authorization.policy resource.
+func (h rbacHandler) RBACPolicyResourcePut(c echo.Context) error {


Handlers in here should be pointer receivers.

Skarlso · 2020-05-23T13:15:05Z

handlers/rbacmiddleware.go

+func (pe *policyEnforcerMiddleware) do(namespace gaia.RBACPolicyNamespace, action gaia.RBACPolicyAction) echo.MiddlewareFunc {
+	return func(next echo.HandlerFunc) echo.HandlerFunc {
+		return func(c echo.Context) error {
+			if ctx, ok := c.(AuthContext); ok {


This is just style, but I would prefer to bail out early if !ok and continue otherwise.

ctx, ok := c.(AuthContext) if ! ok { return c.String(http.StatusInternalServerError, "An error has occurred.") } .... rest of the things with `ctx`.

Completely agree. I would usually do this 👍🏻

Skarlso · 2020-05-23T18:36:11Z

security/rbac/cache.go

+func NewCache(expiration time.Duration, interval time.Duration) Cache {
+	c := &cache{
+		items:      make(map[string]cacheItem),
+		expiration: expiration,
+		mu:         sync.Mutex{},
+	}
+
+	ticker := time.NewTicker(interval)
+	go func() {
+		for {
+			select {
+			case <-ticker.C:
+				c.EvictExpired()
+			}
+		}
+	}()
+
+	return c
+}


I don't think this will work the way you think it would. Make it look something like this...

First, have something like Run on cache. With a * function receiver. And there, do this:

for { c.EvictExpired() select { case <-time.After(15 * time.Minute): // Continue } }

This will now properly block and wait. And once you return the pointer to cache it will still have a context. So later on, when we want to cancel it for whatever reason, we can do something like this:

for { c.EvictExpired() select { case <-time.After(15 * time.Minute): // Continue case <-ctx.Done(): // Context canceled return } }

Will take a look. Still getting my head around channels and goroutines!

Skarlso · 2020-05-23T18:54:44Z

security/rbac/cache.go

+	defer c.mu.Unlock()
+	c.mu.Lock()


Please reverse the order to follow sequential reading.

This is probably okay, but first, throws off the reader, second, it's actually risky as in, if there is another defer, and the lock fails with inconsistent mutex state for some reason, you call defer unlock which will case further problems. So, lock then defer unlock please. :)

Agree here. I think this was just a mistake on my part!

Skarlso · 2020-05-23T19:00:41Z

security/rbac/enforcer.go

+		return errActionNotFound
+	}
+
+	if cfg.Resource == "" {


If the resource is empty it's all resources? I would think if the resource is empty, we shouldn't automatically allow all. * should be something very explicit. If new resources are defined, that means nothing is allowed.

Yep, this was just temporary. I hadn’t got to implementing proper resource authentication yet mate 👍🏻 Was just to unblock testing on!

Skarlso · 2020-05-23T19:01:55Z

security/rbac/enforcer.go

+	eval := make(gaia.RBACEvaluatedPermissions)
+
+	// Evaluate all the policies, creating a single map and point of reference fro the user.
+	// This is not particularly efficient O(n2), but we have to parse all namespaces and actions.


*fro -> for
O(n2) -> O(n^2)

- Support a new API group (mapping of endpoints to perms) - Blend the RBAC middleware into the Auth middleware (didn't want to dupe logic for protected endpoints... for now)

hsluoyz · 2020-05-25T10:46:35Z

Hmm @Skarlso. I do remember seeing this in the past. Not for Gaia though. I wonder if this seemed a little heavy weight... but arguably we’re heading towards this now.

@Skarlso @speza Hi guys! I'm Casbin author. I looked through the committed files. And it seems Casbin has already got all functionalities you need. Casbin only has 1 dependency (the evaluator) so it isn't very "heavy" in my sense. Of course, Casbin's code is complex than this PR's code but its correctness is proved by hundreds of test cases in the CI and production use of many companies. But your customized authz code may contain bugs and it's not easy to find them all via code review only. If one bug is not found here, maybe it will be a severe security vulnerability. If we can switch to using Casbin, we are very happy to solve your issues using it and then you can concentrate on your main directions instead of authz.

Skarlso · 2020-05-25T11:00:52Z

@hsluoyz Hi!

Absolutely awesome from you to reach out to us. :)

Let us formulate an answer for you and we'll see if you can help out with that or maybe we misunderstood something about Casbin that you could clarify. :)

speza · 2020-05-25T12:33:33Z

Yeah thanks for reaching out @hsluoyz!

I will try and remember my reasons for thinking Casbin was heavy. I definitely think the code I have written here isn’t particularly simple anymore to be honest, and also 100% not a fan of not reinventing the wheel that’s for sure.

speza added 9 commits May 2, 2020 19:22

Add the concept of a RBAC policy resource (yaml)

ea2532b

Rename to auth policy

bdfb8b8

Rough implementation of rbac enforcement

3d8b80e

Merge remote-tracking branch 'upstream/master' into resource-yaml-rbac

e321cc7

Move rbac policies

04e8707

Separate rbac middleware into own file

0aeabb2

Lint and tidying up

f68e0de

Fix compilation issue

9e79d27

Fix Errorf (older go version CI/deployed?)

5c3d50d

Skarlso requested changes May 10, 2020

View reviewed changes

speza added 4 commits May 10, 2020 22:23

Tidy up some more

4f09de4

Loads of changes to RBAC

eccad1d

- Cache evaluated user permissions (all permissions from all policies combined) - Clear cache when any policy resource is changed - Rename some bits from Auth to RBAC - Store RBAC bindings (user policy bindings)

Code improvements, fix linting & existing tests

ac5d560

Fix test

8e21c68

speza force-pushed the resource-yaml-rbac branch from b2c88cc to 18e1884 Compare May 17, 2020 13:15

Write some cache tests and remove store dependency for RBAC handler (…

7e6bc69

…encapsulate in service) - Fixed more failing tests

speza force-pushed the resource-yaml-rbac branch from 18e1884 to 7e6bc69 Compare May 17, 2020 13:34

Skarlso requested changes May 23, 2020

View reviewed changes

speza mentioned this pull request May 24, 2020

Improve the REST API consistentcy #247

Open

Lots of improvements and continuation around suggestions

fba8b69

- Support a new API group (mapping of endpoints to perms) - Blend the RBAC middleware into the Auth middleware (didn't want to dupe logic for protected endpoints... for now)

speza force-pushed the resource-yaml-rbac branch from c2447a3 to fba8b69 Compare May 24, 2020 22:04

Skarlso self-requested a review May 28, 2020 18:52

speza closed this Jun 8, 2020

[WIP] Resource yaml based rbac #246

[WIP] Resource yaml based rbac #246

Conversation

speza commented May 8, 2020

Skarlso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speza commented May 10, 2020

Skarlso commented May 10, 2020

Skarlso commented May 10, 2020

speza commented May 10, 2020 • edited Loading

Skarlso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsluoyz commented May 25, 2020

Skarlso commented May 25, 2020

speza commented May 25, 2020

speza commented May 10, 2020 •

edited

Loading