RBAC using Casbin #256

speza · 2020-06-08T22:04:15Z

WIP

# Conflicts: # handlers/handler.go

- Better error handling - Renaming of some properties - Renaming of API mappings file - Use keyMatch in rbac-model.conf to allow wildcards - Fix import order - Rename mapping endpoint - Remove unused bolt bucket

* Use rice instead of pkger * Move enforcer instantiation to server.go * Fix makefile issue

speza · 2020-06-09T19:58:40Z

Ok, so this is getting there now. This change introduces Casbin to enforce RBAC.

We have a policy model that is defined like so (see static/rbac-model.conf).
p, role:admin, pipelines, get, *, allow

Namespace = pipelines, secrets, etc.
Action = create, get, delete, etc
Resource = pipelineid, etc

We use manage a standard set of policy rules loaded in on server start ( static/rbac-policy.csv).

Summary:

Implemented an API mapping yaml file that stores all our endpoints and stores which namespace/action/resource to apply when we enforce RBAC
We now instantiate a casbin adapter that sits on top of boltdb and saves each policy rule line as a k/v pair. On server startup we have to iterate over every k/v item within Bolt to populate the in-memory Casbin model - this isn't ideal but is only needed on startup. We use bolt this way because it give us more flexibility using Cabin's auto-save functionality with the persist.batchAdapter interface. It means we don't just store a single blob and can dynamically add/remove policy lines.
I have wrapped the Casbin enforcer into our own enforcer service for flexibility (security/rbac/service.go)
Use rice to include some more static assets (rbac-model.conf, rbac-policy.csv, rbac-api-mappings.yml
Implemented some new endpoints to manage rbac

// RBAC - Management
apiAuthGrp.GET("rbac/roles", rbacHandler.getAllRoles)
apiAuthGrp.PUT("rbac/roles/:role", rbacHandler.addRole)
apiAuthGrp.DELETE("rbac/roles/:role", rbacHandler.deleteRole)
apiAuthGrp.PUT("rbac/roles/:role/attach/:username", rbacHandler.attachRole)
apiAuthGrp.DELETE("rbac/roles/:role/attach/:username", rbacHandler.detatchRole)
apiAuthGrp.GET("rbac/roles/:role/attached", rbacHandler.getRolesAttachedUsers)
// RBAC - Users
apiAuthGrp.GET("users/:username/rbac/roles", rbacHandler.getUserAttachedRoles)

Add more logic to the authMiddleware to enforce RBAC

TODO:

Add the ability to turn RBAC on/off
More testing
Another PR to implement the UI

hsluoyz · 2020-06-10T04:22:54Z

static/rbac-policy.csv

+# Policy format:
+# p, <role>, <namespace>, <action>, <resource>, <effect = allow/deny>
+
+p, role:admin, pipelines, get, *, allow


If role:admin is assumed to access all, you can just write p, role:admin, *, *, *, allow one rule to allow all.

Nice! That would definitely be handy here!

hsluoyz · 2020-06-10T04:23:19Z

security/rbac/endpoint_enforcer_test.go

+
+import (
+	"errors"
+	"github.com/casbin/casbin/v2"


Sort the imports.

Skarlso

Overall I agree with the approach. Well done. :) Just have a few remarks regarding the code itself and a tiny bit of structure in storage.

Skarlso · 2020-06-11T08:42:56Z

security/rbac/endpoint_enforcer.go

+		return true, nil
+	}
+
+	splitAction := strings.Split(perm, "/")


will we never have something like worker/pipeline/create? When the namespace can be multiple embedded ones for clarity? Not saying we should, I'm just wondering if we should rather just check the last / and everything after that is the action. Since the namespace could be defined as embedded namespaces. What do you think?

Hm that's an interesting thought. We should probably handle it you're right.
Unless we name it something like worker::pipeline/create? Actually I think worker/pipeline::create maybe even looks nicer?

My main thoughts are that we should seperate the namespace and action with something unique (in this case /).

A single colon is fine actually, I think. No need to bring PHP into this. :D

PHP.... Java... anything else? :P

Skarlso · 2020-06-11T08:44:54Z

security/rbac/endpoint_enforcer.go

+	namespace := splitAction[0]
+	action := splitAction[1]
+
+	fullResource := "*"


Does this mean that we are allow all by default? I think that should be deny by default, right?

So we need to use * for endpoints where there isn't a resource id specified in the api-mappings yaml. Right now that is something like secrets/create that doesn't need the enforcement of a resource id on it.

Wait, why isn't there a secrets and why doesn't that need enforcement? :D

I think you have misunderstood mate! We have permissions on secrets. I just mean that secrets/create doesn't have resource (a key ID) enforced because you haven't created something yet!

So:
not specified < cannot create secrets
p, secrets, create, *, deny < cannot create secrets
p, secrets, create, *, allow < can create secrets

Skarlso · 2020-06-11T09:09:16Z

security/rbac/service.go

+func loadModel() (model.Model, error) {
+	modelStr, err := assethelper.LoadRBACModel()
+	if err != nil {
+		return nil, err


Add logging here as well, and all the rest of the errors. The more messages we get the more we know what went wrong. Pass in the logger as a dependency so you can use it with a method receiver.

WIll go through and do this as I tidy up mate

I'd prefer to add logging at the caller level. So in this case it would be where we call NewEnforcerSvc. The rest of the logging I will add into the handler code, etc.

One thing I will do is wrap the errors though to give more context 👍

That's good as well. High level logging is fine but that logging wraps at least 3 or more different errors. If even one of them is similar we'll have no idea where the error happened. Wrapping is a good solution to that. Love the new error system. 😊

Where’s a good old Java stacktrace when you need it! Haha

Bwuhahaha, pssss, not so loud.. The other gophers might hear you. 😂😂😂

Skarlso · 2020-06-11T09:10:02Z

security/rbac/service.go

+		return gaia.RBACAPIMappings{}, err
+	}
+
+	var rbacapiMappings gaia.RBACAPIMappings


Declare this above and then return it.

Skarlso · 2020-06-11T09:11:57Z

security/rbac/service.go

+	}
+
+	var rbacapiMappings gaia.RBACAPIMappings
+	if err := yaml.Unmarshal([]byte(mappings), &rbacapiMappings); err != nil {


I wonder if we should lock this mapping down for tempering with UnmarshalStrict. That will throw errors in case of duplicate fields and mappings which do not have corresponding structs.

Skarlso · 2020-06-11T09:13:25Z

security/rbac/service.go

+		return err
+	}
+	if !exists {
+		return errors.New("role does not exists for user")


helper/assethelper/helper.go

Skarlso · 2020-06-11T09:35:49Z

helper/assethelper/helper.go

+
+// LoadRBACBuiltinPolicy loads the builtin rbac-policy.csv
+func LoadRBACBuiltinPolicy() (string, error) {
+	return loadStaticFile("rbac-policy.csv")


Put the file names into constants.

store/store.go

* Updated our bolt dependency (the adapter is supporting latest - so makes sense to bump us too) * Log errors in handlers and correct wrap errors * Improve static rbac-policy.csv

speza · 2020-06-12T15:07:32Z

Just to top it off @Skarlso circle is still using Go 1.12 so won’t compile the error wrapping!

Skarlso · 2020-06-12T15:23:28Z

#256 (comment)

Yes. Right now circleci is failing until we can update it and push a new image.

* We now load in a different file format at start up for the RBAC api mappings. This one is more flexible, and I believe more user friendly (see the comment in the .yml file). Once loaded, we still have an O(1) lookup available for the endpoints :) * Moved some models out of Gaia into rbac as they we only being used internally by the rbac package.

* Split the settings store interface out into its own (but nested in store.GaiaStore) * Added unit tests for settings.go and improved DI on existing ones * Created a new SettingsHandler to encapsulate the store dependency * Add ability to save RBAC enabled state into database * RBAC service is switched to EnforcerService or NoOpService depeneding on enabled state * RBAC can be enabled or disabled with cmd flag `-rbac-enabled=true/false`

Add RBAC settings flag (enabled/disabled) and some improvements * Split the settings store interface out into its own (but nested in store.GaiaStore) * Added unit tests for settings.go and improved DI on existing ones * Created a new SettingsHandler to encapsulate the store dependency * Add ability to save RBAC enabled state into database * RBAC service is switched to EnforcerService or NoOpService depending on enabled state * RBAC can be enabled or disabled with cmd flag `-rbac-enabled=true/false`

* Fixed issues with missing or incorrect rbac-api-mappings.yml * Started some acceptance tests for RBAC (probably to be finished outside this PR - its a lot of work) * Added some tests for the content of the rbac-api-mappings.yml to alert us to any future change * Endpoint Enforce now returns a PermissionDenied error

# Conflicts: # go.mod # go.sum

…ring literal See https://github.com/GeertJohan/go.rice/blob/master/README.md for why

codecov-commenter · 2020-06-16T20:34:00Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 65.66038% with 91 lines in your changes missing coverage. Please review.

Project coverage is 62.80%. Comparing base (532ac3f) to head (8c10fdd).
Report is 25 commits behind head on master.

Files with missing lines	Patch %	Lines
security/rbac/service.go	16.86%	67 Missing and 2 partials ⚠️
handlers/settings.go	66.66%	5 Missing and 4 partials ⚠️
handlers/auth.go	73.68%	3 Missing and 2 partials ⚠️
security/rbac/endpoint_enforcer.go	87.50%	3 Missing and 1 partial ⚠️
store/store.go	42.85%	3 Missing and 1 partial ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #256      +/-   ##
==========================================
+ Coverage   62.33%   62.80%   +0.47%     
==========================================
  Files          46       51       +5     
  Lines        3746     4001     +255     
==========================================
+ Hits         2335     2513     +178     
- Misses       1027     1092      +65     
- Partials      384      396      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

security/rbac/noop.go

Skarlso · 2020-07-01T06:43:56Z

store/store.go

+
+// SettingsStore is the interface that defines methods needed to save settings config into the store.
+type SettingsStore interface {
+	SettingsPut(config *gaia.StoreConfig) error


why can't an RBACPut be a SettingsPut? Why are they two different functions? An interface shouldn't be explicit. It should be generic mostly. If there is a settingsRbacPut that should most likely be either a more specialised interface or a concrete implementation of settings put.

I see that it's putting / getting an RBACConfig instead of a StoreConfig, but that also means that it can't be a SettingsStore.

What I mean is, either make rbac use settingsput or have a separate interface. :)

server/server.go

# Conflicts: # handlers/handler.go # handlers/hook_test.go # handlers/providers/pipelines/settings.go # handlers/settings_test.go

# Conflicts: # handlers/settings.go # handlers/settings_test.go

Casbin rbac settings

Skarlso · 2020-07-06T19:46:17Z

Testing was successful on the deployed instance! Well done @speza! Thanks for this awesome work and effort you've put into this and your perseverance. :) Fantastic work! 👍

speza added 11 commits May 25, 2020 19:57

Hack Casbin RBAC in for POC

3c867c8

Fix issue with apigroup-core.yml

8bf5b0b

Externalise casbin adapter

fdf3836

Merge remote-tracking branch 'upstream/master' into casbin-rbac

701a6dd

Merge remote-tracking branch 'upstream/master' into casbin-rbac

db9f96a

# Conflicts: # handlers/handler.go

Load in some default builtin roles (always)

9ec315a

General improvements to the code

b927639

- Better error handling - Renaming of some properties - Renaming of API mappings file - Use keyMatch in rbac-model.conf to allow wildcards - Fix import order - Rename mapping endpoint - Remove unused bolt bucket

Make the endpoint_enforcer.go generic and add tests

186f07d

Introduce some REST handlers and allow/deny effect

0a8893b

Introduce some REST handlers and allow/deny effect

f7cd682

Add pkger and mocked endpoint_enforcer tests

391f047

Skarlso self-requested a review June 9, 2020 08:51

Static file and dependency improvements

dd54218

* Use rice instead of pkger * Move enforcer instantiation to server.go * Fix makefile issue

speza force-pushed the casbin-rbac branch from fc70e88 to dd54218 Compare June 9, 2020 18:07

Unit test the new handlers

e7a1db6

hsluoyz reviewed Jun 10, 2020

View reviewed changes

Skarlso reviewed Jun 11, 2020

View reviewed changes

Some improvements including some from the CR

a243d91

* Updated our bolt dependency (the adapter is supporting latest - so makes sense to bump us too) * Log errors in handlers and correct wrap errors * Improve static rbac-policy.csv

speza force-pushed the casbin-rbac branch from 9cca8dd to a243d91 Compare June 12, 2020 14:56

speza added 7 commits June 13, 2020 23:54

Fix the import order

a11745b

Merge remote-tracking branch 'upstream/master' into casbin-rbac

d701e26

# Conflicts: # go.mod # go.sum

Resolve conflicts

c966a9d

Omit rbac_endpoint_test.go (for now) and update rice.FindBox to be st…

358f1e8

…ring literal See https://github.com/GeertJohan/go.rice/blob/master/README.md for why

speza added 3 commits June 17, 2020 20:17

Add some more unit tests

24264ea

Merge remote-tracking branch 'upstream/master' into casbin-rbac

5e3f6ad

Fix enforcer logic and test assertions

2ddb883

speza force-pushed the casbin-rbac branch from adfb5ce to 2ddb883 Compare June 28, 2020 14:08

Add flag for RBAC debugging logging

48942a2

speza changed the title ~~WIP: RBAC using Casbin~~ RBAC using Casbin Jun 28, 2020

speza marked this pull request as ready for review June 28, 2020 18:27

Skarlso reviewed Jul 1, 2020

View reviewed changes

security/rbac/noop.go Show resolved Hide resolved

Skarlso reviewed Jul 1, 2020

View reviewed changes

server/server.go Outdated Show resolved Hide resolved

speza added 7 commits July 4, 2020 10:35

Refactor RBAC settings code

26adcce

Merge remote-tracking branch 'upstream/master' into casbin-rbac

87414ad

# Conflicts: # handlers/handler.go # handlers/hook_test.go # handlers/providers/pipelines/settings.go # handlers/settings_test.go

Fix all merge conflicts around settings.go

e8de117

Merge branch 'casbin-rbac' into casbin-rbac-settings

5aa2665

# Conflicts: # handlers/settings.go # handlers/settings_test.go

Couple of quick fixes post merge

3f67758

Merge pull request #3 from speza/casbin-rbac-settings

8ea0098

Casbin rbac settings

Fix test and add missing pull route

8c10fdd

Skarlso merged commit b419af3 into gaia-pipeline:master Jul 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RBAC using Casbin #256

RBAC using Casbin #256

speza commented Jun 8, 2020

speza commented Jun 9, 2020 •

edited

Loading

hsluoyz Jun 10, 2020

speza Jun 10, 2020

hsluoyz Jun 10, 2020

Skarlso left a comment

Skarlso Jun 11, 2020

speza Jun 11, 2020

Skarlso Jun 12, 2020

speza Jun 12, 2020

Skarlso Jun 11, 2020

speza Jun 11, 2020

Skarlso Jun 12, 2020

speza Jun 12, 2020

Skarlso Jun 11, 2020

speza Jun 11, 2020

speza Jun 11, 2020

Skarlso Jun 11, 2020

speza Jun 11, 2020

Skarlso Jun 12, 2020

Skarlso Jun 11, 2020

Skarlso Jun 11, 2020

Skarlso Jun 11, 2020

Skarlso Jun 11, 2020

speza commented Jun 12, 2020

Skarlso commented Jun 12, 2020

codecov-commenter commented Jun 16, 2020 •

edited

Loading

Skarlso Jul 1, 2020

Skarlso commented Jul 6, 2020

RBAC using Casbin #256

RBAC using Casbin #256

Conversation

speza commented Jun 8, 2020

speza commented Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

speza commented Jun 12, 2020

Skarlso commented Jun 12, 2020

codecov-commenter commented Jun 16, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Skarlso commented Jul 6, 2020

speza commented Jun 9, 2020 •

edited

Loading

codecov-commenter commented Jun 16, 2020 •

edited

Loading