Add ReBAC authorization with OpenFGA #11905

monstermunchkin · 2023-06-28T20:46:57Z

https://discuss.linuxcontainers.org/t/lxd-rebac-authorization-using-openfga/17094

lxd/openfga/model.go

markylaing

Looks good. I've left a few comments on things to double check.

One thing I've noticed is that a few handlers have a userHasPermission(fmt.Sprintf("%s_%s", projectName, resourceName)). I think these might be better as first class functions in the openfga package since the handler shouldn't necessarily know about the naming conventions for FGA resource/relation names.

Makefile

lxd/openfga/client.go

markylaing · 2023-07-03T15:19:02Z

lxd/cluster/config/config.go

@@ -268,6 +268,11 @@ func (c *Config) ClusterHealingThreshold() time.Duration {
 	return healingThreshold
 }

+// OpenFGA returns all OpenFGA settings need to interact with an OpenFGA server.
+func (c *Config) OpenFGA() (string, string, string) {


I'm not usually a fan of named returns but here it might be useful to know the order of the returns since they are all of the same type.

lxd/openfga.go

lxd/images.go

lxd/network_acls.go

lxd/network_zones.go

tomponline · 2023-07-11T07:22:40Z

@roosterfish @MusicDin hey if you chaps have chance please can you take a look at this before I do a review. Thanks

markylaing

I think this looks good, I just have a question about how we want to structure access handling. We already have access handlers like e.g. allowProjectPermission("containers", "view") which check RBAC permissions, so I wonder if we can perform the ReBAC check there instead of adding the ReBACAccessHandler to endpoint actions.

markylaing · 2023-07-11T11:18:20Z

lxd/daemon.go

+					return resp
+				}
+			}
+


Does this mean if a ReBACAccessHandler is defined for the endpoint action it is always called, regardless of whether or not we have an openfga client configured?

Also, if ReBAC is configured, do we still need to call the standard AccessHandler?

I wonder if a better pattern would be to remove the ReBACAccessHandler and perform the check inside the AccessHandler instead if the client is set. We could have utils for building access handlers e.g. access.NewHandler().Authenticated().AllowedReBACRelations("admin", "images_view").AllowedCandidRoles("admin", "images_view") or similar.

@monstermunchkin I too am a bit confused by the ramifications of this and am thinking the same as @markylaing above. It seems odd to have 2 access handlers defined for a single API route. Can we not check ReBAC inside the currently access handler?

monstermunchkin · 2023-07-11T14:48:30Z

We already have access handlers like e.g. allowProjectPermission("containers", "view") which check RBAC permissions, so I wonder if we can perform the ReBAC check there instead of adding the ReBACAccessHandler to endpoint actions.

The problem is that RBAC isn't as fine-grained as ReBAC. For example, ReBAC has separate handlers for networks, network ACLs, and network zones, whereas RBAC only does networks which includes ACLs and zones. If we changed that in the current access handler, it would break things as RBAC doesn't know of network ACLs.

Also, for ReBAC, some permission handling is done inside of the actual handler. For example when listing instances, a user should only see the instances which they are allowed to view. RBAC would just show all instances.

So, for simplicity and backward compatibility I believe it's best to add ReBACAccessHandler.

markylaing · 2023-07-12T11:19:14Z

The problem is that RBAC isn't as fine-grained as ReBAC. For example, ReBAC has separate handlers for networks, network ACLs, and network zones, whereas RBAC only does networks which includes ACLs and zones. If we changed that in the current access handler, it would break things as RBAC doesn't know of network ACLs.

I'm not suggesting we mangle permissions to make RBAC and ReBAC equivalent, I'm suggesting that we have a configurable AccessHandler that we can set both RBAC and ReBAC permissions against. You could then have something like access.NewHandler().RBAC("admin", "network-manager").ReBAC("admin", "network_acl#manager") (we have a similar pattern in LXD Cloud). Then if the ReBAC client is set, check against that, falling back to RBAC.

So, for simplicity and backward compatibility I believe it's best to add ReBACAccessHandler.

Yeah I guess in practice my suggestion will be more difficult to implement and there is more risk of breaking existing access control, so I'm happy to keep the ReBACAccessHandler 👍

tomponline · 2023-07-14T11:54:37Z

@monstermunchkin before I review this more thoroughly, it looks like there is no docs update for this PR?
Please can you add something to explain how to use it. @ru-fu any suggestions on what/where to add?

ru-fu · 2023-07-14T12:25:10Z

@monstermunchkin before I review this more thoroughly, it looks like there is no docs update for this PR? Please can you add something to explain how to use it. @ru-fu any suggestions on what/where to add?

https://documentation.ubuntu.com/lxd/en/latest/authentication/ will need some updates in the "OpenID Connect authentication" section and a new section similar to the RBAC section, describing what you can do and what roles there are.
https://documentation.ubuntu.com/lxd/en/latest/howto/projects_confine/ needs instructions for how to set this up. (Ideally a bit more detailed than the RBAC section - we kept that one short because we knew it's going away.)

tomponline · 2023-07-17T08:46:32Z

lxd/openfga/client.go

+func (c *Client) HasPermission(user string, relation Relation, object string) bool {
+	fullUser := UserObject(user)
+
+	logger.Debug("Checking ReBAC permission", logger.Ctx{"user": fullUser, "relation": relation, "object": object})


Can you create a new logger here using l := logger.AddContext( logger.Ctx{"user": fullUser, "relation": relation, "object": object}), then you can use l.Debug() and include all the context each time, including on line 86.

tomponline · 2023-07-17T08:46:40Z

lxd/openfga/client.go

+
+	data, _, err := c.apiClient.OpenFgaApi.Check(context.Background()).Body(body).Execute()
+	if err != nil {
+		logger.Debug("Failed checking permissions", logger.Ctx{"err": err})


Please use l.Debug here

tomponline · 2023-07-17T08:47:30Z

lxd/openfga/client.go

+	fullUser := fmt.Sprintf("user:%s", user)
+	body := openfga.NewListObjectsRequest(string(objectType), string(relation), fullUser)
+
+	objectResponse, _, err := c.apiClient.OpenFgaApi.ListObjects(context.Background()).Body(*body).Execute()


For new code we should be accepting context arguments not using context.Background.

tomponline · 2023-07-17T08:48:10Z

lxd/openfga/client.go

+	return objects, nil
+}
+
+func (c *Client) AddTuple(user string, relation Relation, object string) error {


Missing doc block saying what it does.

tomponline · 2023-07-17T08:48:33Z

lxd/openfga/client.go

+		},
+		AuthorizationModelId: openfga.PtrString(c.authModelID)}
+
+	_, _, err := c.apiClient.OpenFgaApi.Write(context.Background()).Body(body).Execute()


Needs to accept a context argument.

tomponline · 2023-07-17T08:49:01Z

lxd/openfga/client.go

+		},
+		AuthorizationModelId: openfga.PtrString(c.authModelID)}
+
+	_, _, err := c.apiClient.OpenFgaApi.Write(context.Background()).Body(body).Execute()


This function need to accept a context argument.

tomponline · 2023-07-17T08:49:44Z

lxd/openfga/client.go

+}
+
+// ListObjects returns the a list of objects the user is related to. It also returns whether the user is an admin.
+func (c *Client) ListObjects(user string, relation Relation, objectType ObjectType) ([]string, error) {


It also returns whether the user is an admin.

How does it do that?

tomponline · 2023-07-17T08:50:07Z

lxd/openfga/client.go

+	return nil
+}
+
+func (c *Client) DeleteTuple(user string, relation Relation, object string) error {


Missing doc block

tomponline · 2023-07-17T08:51:45Z

lxd/openfga/model.go

@@ -0,0 +1,5 @@
+package openfga
+
+// Code generated by Makefile; DO NOT EDIT.


Could we change this to "Code generated by make update-openfga` so its clear what needs to be run to update it?

tomponline · 2023-07-17T08:52:24Z

lxd/openfga/model.openfga

@@ -0,0 +1,61 @@
+model


Can this file have comments explaining what it is and what it defines?

Also, in the future if we add new entity types to LXD, will this file need to be updated?
What are the ramifications if it isn't does the new resource become inaccessible to these users or does it "fail open"?

tomponline · 2023-07-17T08:54:23Z

lxd/openfga/objects.go

+	"fmt"
+)
+
+func GroupObject(groupName string) string {


Please can we change these functions to start with Object so they are grouped alphabetically by what type of info they return?

Like you've done with the Relation constants.

tomponline · 2023-08-09T16:34:06Z

This can be rebased now please

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

This adds the update-openfga target which generates a go source file containing the JSON representation of the OpenFGA authorization model. Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

tomponline · 2023-09-15T14:18:25Z

@monstermunchkin @markylaing shall we close this in preference to #12252 ?

tomponline reviewed Jun 30, 2023

View reviewed changes

lxd/openfga/model.go Outdated Show resolved Hide resolved

monstermunchkin force-pushed the features/openfga branch 4 times, most recently from 939ccc4 to f03eda4 Compare July 3, 2023 09:48

markylaing reviewed Jul 3, 2023

View reviewed changes

monstermunchkin force-pushed the features/openfga branch 4 times, most recently from 47d4734 to 6f1ba9f Compare July 6, 2023 12:53

tomponline requested a review from markylaing July 11, 2023 07:22

markylaing reviewed Jul 11, 2023

View reviewed changes

monstermunchkin force-pushed the features/openfga branch 2 times, most recently from 17c9255 to b376e2a Compare July 14, 2023 14:01

tomponline reviewed Jul 17, 2023

View reviewed changes

monstermunchkin added 26 commits August 28, 2023 10:14

gomod: Add github.com/openfga/go-sdk

8c79fec

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

Makefile: Add update-openfga

0d6750d

This adds the update-openfga target which generates a go source file containing the JSON representation of the OpenFGA authorization model. Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

test/lint: Add check for OpenFGA auth model

3dbd5c1

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Add ErrNotSupported

78683c1

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Extend Authorizer interface

c53781a

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Update TLS driver

244ec84

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Add Relation and ObjectType constants

289a96a

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Add helper function to get full object names

3adaf41

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Update commonAuthorizer

51bbda3

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/cluster/config: Add OpenFGA config keys

404e075

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/auth: Add OpenFGA authorization driver

151e905

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/metrics: Add function to filter samples

cc04f7e

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd: Add OpenFGA setup

1472a17

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

daemon: Update allowPermission to use ObjectType and Relation

a647053

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/metrics: Support fine-grained auth

b0df8cc

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/projects: Support fine-grained auth

40b78e2

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/images: Support fine-grained auth

df3ef74

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/instances: Support fine-grained auth

62f58f0

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/network_acls: Support fine-grained auth

28fb027

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/network_zones: Support fine-grained auth

914e0b9

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/network: Support fine-grained auth

9a2b783

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/profiles: Support fine-grained auth

70910c9

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/storage_buckets: Support fine-grained auth

c3d9f1e

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/storage_volumes: Support fine-grained auth

32bf5b7

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd: Support relations when cancelling operations

304b6ab

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

lxd/project: Update FilterUsedBy

a991356

Signed-off-by: Thomas Hipp <thomas.hipp@canonical.com>

monstermunchkin force-pushed the features/openfga branch from b376e2a to a991356 Compare August 29, 2023 11:02

monstermunchkin closed this Sep 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReBAC authorization with OpenFGA #11905

Add ReBAC authorization with OpenFGA #11905

monstermunchkin commented Jun 28, 2023

markylaing left a comment

markylaing Jul 3, 2023

tomponline commented Jul 11, 2023

markylaing left a comment

markylaing Jul 11, 2023

tomponline Jul 17, 2023

monstermunchkin commented Jul 11, 2023

markylaing commented Jul 12, 2023

tomponline commented Jul 14, 2023

ru-fu commented Jul 14, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline Jul 17, 2023

tomponline commented Aug 9, 2023

tomponline commented Sep 15, 2023

		@@ -0,0 +1,5 @@
		package openfga

		// Code generated by Makefile; DO NOT EDIT.

Add ReBAC authorization with OpenFGA #11905

Add ReBAC authorization with OpenFGA #11905

Conversation

monstermunchkin commented Jun 28, 2023

markylaing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline commented Jul 11, 2023

markylaing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monstermunchkin commented Jul 11, 2023

markylaing commented Jul 12, 2023

tomponline commented Jul 14, 2023

ru-fu commented Jul 14, 2023

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

tomponline commented Aug 9, 2023

tomponline commented Sep 15, 2023