Auth: Embedded OpenFGA authorization driver #12976

markylaing · 2024-02-27T19:31:31Z

Adds the embedded OpenFGA authorization driver for use with OIDC authentication. We will make this the default for TLS authenticated users when we are confident in its performance and reliability.

Implements https://discourse.ubuntu.com/t/identity-and-access-management-for-lxd/41516 with some changes. These will be updated in the spec.
Completes https://warthogs.atlassian.net/browse/LXD-679

Warning: All users that currently authenticate to LXD with OIDC will lose access to their cluster. To regain access, a user must:

Make a call to LXD (e.g. lxc info) to ensure their OIDC identity has been added to the database.
Create a group lxc auth group create my-group
Grant the group a suitable permission. As all current OIDC users are administrators, a suitable permission is "admin" on "server". This can be performed with lxc auth group permission add my-group server admin.
Add themselves to the group. This can be performed with lxc auth identity group add oidc/<email-address> my-group.

To perform steps 2-4, another authentication method must be used. This can be via an unrestricted TLS certificate or directly via the unix socket.

markylaing · 2024-02-27T21:42:50Z

I'm not sure why this is failing the compatibility check:

https://github.com/canonical/lxd/actions/runs/8071688009/job/22051847315?pr=12976#step:8:212
go: github.com/openfga/openfga@v1.4.3 requires go@1.21.5, but 1.21 is requested

It's I'm using 1.21.7 locally.

tomponline · 2024-02-27T22:29:18Z

I'm not sure why this is failing the compatibility check:
* https://github.com/canonical/lxd/actions/runs/8071688009/job/22051847315?pr=12976#step:8:212

* `go: github.com/openfga/openfga@v1.4.3 requires go@1.21.5, but 1.21 is requested`
It's I'm using 1.21.7 locally.

Think we need to switch that to use go get go@ call like the earlier PR https://github.com/canonical/lxd/blob/main/.github/workflows/tests.yml#L228

tomponline · 2024-02-28T10:13:37Z

Can be rebased now.

tomponline · 2024-02-28T12:03:54Z

Ready for rebase

doc/api-extensions.md

markylaing · 2024-02-28T15:43:05Z

I've rebased onto main. Hopefully this looks a lot less daunting to review now 😆

markylaing · 2024-02-28T21:07:28Z

While testing I've come across this error: https://github.com/canonical/lxd/actions/runs/8083078860/job/22085325564?pr=12976#step:13:7250

The test isn't catching it but this was caused by the OpenFGA datastore trying to resolve the URL of a permission whose entity no longer existed.

I have a fix for this already but it's actually only a one-line change in the driver, the majority of the changes are in the permission API handlers and some already existing DB methods. I think for the sake of keeping this PR simple I will raise it as a separate PR and this PR can be rebased onto that one when merged. So for now I'm putting this back to draft status.

markylaing · 2024-02-28T23:23:03Z

While testing I've come across this error: https://github.com/canonical/lxd/actions/runs/8083078860/job/22085325564?pr=12976#step:13:7250

The test isn't catching it but this was caused by the OpenFGA datastore trying to resolve the URL of a permission whose entity no longer existed.

I have a fix for this already but it's actually only a one-line change in the driver, the majority of the changes are in the permission API handlers and some already existing DB methods. I think for the sake of keeping this PR simple I will raise it as a separate PR and this PR can be rebased onto that one when merged. So for now I'm putting this back to draft status.

Once #12992 is merged I'll rebase and unmark as draft.

markylaing · 2024-02-28T23:24:21Z

While testing I've come across this error: https://github.com/canonical/lxd/actions/runs/8083078860/job/22085325564?pr=12976#step:13:7250

The test isn't catching it but this was caused by the OpenFGA datastore trying to resolve the URL of a permission whose entity no longer existed.

I have a fix for this already but it's actually only a one-line change in the driver, the majority of the changes are in the permission API handlers and some already existing DB methods. I think for the sake of keeping this PR simple I will raise it as a separate PR and this PR can be rebased onto that one when merged. So for now I'm putting this back to draft status.

Once #12992 is merged I'll rebase and unmark as draft.

mas-who · 2024-03-01T12:48:19Z

@markylaing as discussed, I will just leave this comment here regarding idp groups for future reference. So it was found that after an OIDC identity logs in, if its idp groups were successfully mapped via the custom claim, the identity gets assigned to the correct lxd groups and therefore gets the right permissions. However, when we query the API for group memberships, the association between the lxd groups and the identity is missing. It was mentioned for idp mapped groups, the relationship between those groups and the identity is not currently stored in the database, so we can't get that data.

mas-who · 2024-03-01T15:19:44Z

@markylaing found another issue with the identity-provider-group endpoint. See below terminal output:

root@lxd-permissions-dev:~/lxd# lxc query /1.0/auth/groups
[
        "/1.0/auth/groups/admin-group",
        "/1.0/auth/groups/big-permission",
        "/1.0/auth/groups/small-permission"
]

root@lxd-permissions-dev:~/lxd# lxc query /1.0/auth/identity-provider-groups/test?recursion=1
{
        "groups": [],
        "name": "test"
}

root@lxd-permissions-dev:~/lxd# lxc auth identity-provider-group group add test admin-group
Error: Failed to write identity provider group mappings: FOREIGN KEY constraint failed

For some reason trying to add lxd group to an idp group fails foreign key constraint. But using the same CLI command I was able to add a lxd group to the owner idp group. For some more context, see below table data for the relevant endpoints:

root@lxd-permissions-dev:~/lxd# lxd sql global "select * from auth_groups"
DEBUG  [2024-03-01T15:14:29Z] Connecting to a local LXD over a Unix socket 
DEBUG  [2024-03-01T15:14:29Z] Sending request to LXD                        etag= method=POST url="http://unix.socket/internal/sql"
DEBUG  [2024-03-01T15:14:29Z] 
        {
                "database": "global",
                "query": "select * from auth_groups"
        } 
+----+------------------+-------------+
| id |       name       | description |
+----+------------------+-------------+
| 1  | admin-group      |             |
| 8  | big-permission   |             |
| 9  | small-permission |             |
+----+------------------+-------------+
root@lxd-permissions-dev:~/lxd# lxd sql global "select * from identity_provider_groups"
DEBUG  [2024-03-01T15:14:36Z] Connecting to a local LXD over a Unix socket 
DEBUG  [2024-03-01T15:14:36Z] Sending request to LXD                        etag= method=POST url="http://unix.socket/internal/sql"
DEBUG  [2024-03-01T15:14:36Z] 
        {
                "database": "global",
                "query": "select * from identity_provider_groups"
        } 
+----+-------+
| id | name  |
+----+-------+
| 1  | owner |
| 5  | test  |
+----+-------+
root@lxd-permissions-dev:~/lxd# lxd sql global "select * from auth_groups_identity_provider_groups"
DEBUG  [2024-03-01T15:14:48Z] Connecting to a local LXD over a Unix socket 
DEBUG  [2024-03-01T15:14:48Z] Sending request to LXD                        etag= method=POST url="http://unix.socket/internal/sql"
DEBUG  [2024-03-01T15:14:48Z] 
        {
                "database": "global",
                "query": "select * from auth_groups_identity_provider_groups"
        } 
+----+---------------+----------------------------+
| id | auth_group_id | identity_provider_group_id |
+----+---------------+----------------------------+
| 1  | 1             | 1                          |
+----+---------------+----------------------------+

Seems like an insert into the auth_groups_identity_provider_groups is causing issues?

edlerd · 2024-03-01T17:45:39Z

I suggest to use 403 over 404 when users have no permission.

Consider the scenario:

Alice has no access to secret-project on the server
In the current state, lxd responds with

403 on 1.0/instances?project=nonexisting-project, but also with a
403 on 1.0/instances?project=secret-project

It should definitely respond with 404 to 1.0/instances?project=nonexisting-project, because that project doesn't exist.

We shouldn't expose the existence of secret-project to Alice, hence we should always use 404 when users have no permission.

mas-who · 2024-03-04T10:47:18Z

@markylaing I was playing around with different permissions and found a potential issue. I created a group with a single permission can_view_instances on project. Then I allocated this group to an oidc identity. Upon login, I can't view any instances and looking at the server debug logs, I see the below info:

DEBUG  [2024-03-04T10:40:41Z] Error Response
        {
                "type": "error",
                "status": "",
                "status_code": 0,
                "operation": "",
                "error_code": 403,
                "error": "User does not have entitlement \"can_view\" on object \"project:/1.0/projects/default\"",
                "metadata": null
        }  http_code=403

Is this intended behaviour? Furthermore, I then changed the group permission to have can_view entitlement on project, the 403 response went away but no instances were returned from the server, seems like it was filtered out somehow.

markylaing · 2024-03-04T11:01:00Z

@markylaing I was playing around with different permissions and found a potential issue. I created a group with a single permission can_view_instances on project. Then I allocated this group to an oidc identity. Upon login, I can't view any instances and looking at the server debug logs, I see the below info:
DEBUG  [2024-03-04T10:40:41Z] Error Response
        {
                "type": "error",
                "status": "",
                "status_code": 0,
                "operation": "",
                "error_code": 403,
                "error": "User does not have entitlement \"can_view\" on object \"project:/1.0/projects/default\"",
                "metadata": null
        }  http_code=403
Is this intended behaviour? Furthermore, I then changed the group permission to have can_view entitlement on project, the 403 response went away but no instances were returned from the server, seems like it was filtered out somehow.

That sounds like a bug to me. Will investigate when I get back to this after #12992. Thanks!

tomponline · 2024-03-08T10:19:58Z

ready for rebase

…URL. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

This commit updates the method to extract the identity provider groups and additionally use the more concise `request.GetCtxValue` method. Signed-off-by: Mark Laing <mark.laing@canonical.com>

Signed-off-by: Mark Laing <mark.laing@canonical.com>

tomponline · 2024-03-08T10:31:24Z

go.mod

 	github.com/olekukonko/tablewriter v0.0.5
+	github.com/openfga/api/proto v0.0.0-20240307175852-df09bc1e8e82
+	github.com/openfga/language/pkg/go v0.0.0-20240307152633-772cdd8b6a9c
+	github.com/openfga/openfga v1.5.0


This is pulling in lots of stuff, two surprising items are mysql driver and docker client?

Is it possible for us to consume a more specific package rather than the general openfga package.

Not a blocker ofc.

I don't think this is possible as the go.mod is in the root of the repo. I could look into which specific dependencies are required by the packages we are using and potentially we can write exclude directives to get rid of mysql and docker etc.

i think go mod tidy would have done this for us already if it wasnt needed - i was wondering if we did need to import the main package or whether we can be more specific somehow?

tomponline · 2024-03-08T10:33:50Z

lxd/db/openfga.go

+	return nil, nil
+}
+
+// ReadUsersetTuples is called on check requests. It is used to read all the "users" that have a given relation to


Could we add a request context optimisation here to only check for the current user rather than checking for all users who have access? (in the future).

This is a little nuanced. "user" in this context is not the identity that made the request. OpenFGA tuples are composed of a user, a relation and an object. In the datastore implementation whenever we see a user that is an identity, we typically ignore it because all of their relations have been passed in contextually. Here are some examples:

user: server:/1.0 relation: server object: project:/1.0/projects/default

user: project:/1.0/projects/default relation: project object: instance:/1.0/instances/foo?project=default

user: identity:/1.0/auth/identities/oidc/jane.doe@example.com relation: member object: group:/1.0/auth/groups/foo-group

We would ignore this one, identities can't be given entitlements directly.

user: identity:/1.0/auth/identities/oidc/jane.doe@example.com relation: can_view object: instance:/1.0/instances/foo?project=bar

tomponline · 2024-03-08T10:35:02Z

lxd/db/openfga.go

+		}
+
+		// Get all groups with the permission.
+		q := `


As part of the refactoring tasks please can this query be moved into the clusterDB package. Thanks

tomponline · 2024-03-08T10:37:45Z

lxd/db/openfga.go

+	if err != nil && !api.StatusErrorCheck(err, http.StatusNotFound) {
+		return nil, err
+	} else if err != nil {
+		// If we have a not found error then there are no tuples to return, but the datastore shouldn't return an error.


This is quite awkwardly structured, where the specific scenario that occurs when there is a not found error is checking err != nil. In my view clearer would have been:

if err != nil { if api.StatusErrorCheck(err, http.StatusNotFound) { // If we have a not found error then there are no tuples to return, but the datastore shouldn't return an error. return storage.NewStaticTupleIterator(nil), nil } return nil, err }

tomponline

In general we need to move DB queries out of the openfga driver and into the clusterDB package. As part of future refactor.

tomponline · 2024-03-08T10:44:42Z

lxd/auth/driver_openfga.go

+	// Inspect request.
+	details, err := e.requestDetails(r)
+	if err != nil {
+		return api.StatusErrorf(http.StatusForbidden, "Failed to extract request details: %v", err)


Should be %w

Actually in this case I'm not sure it can be? Do we need api.StatusError to implement Unwrap for this to work properly?

ah yeah ofcourse you're correct. Although now im looking closer how do we know that the err means forbidden anyway?

api.StatusErrorCheck is able to unwrap until it gets to the api.StatusError and then it can check the status code. I've just implemented the interface for it now so we can pass in %w and wrap underlying errors. Should mean we will be able to distinguish a general 404 into sql.ErrNoRows or io.ErrNotExist in future.

OK, but what I mean is that this actually feels like the wrong place to be returning api.StatusErrorf(http.StatusForbidden), shouldn't that be done in e.requestDetails so it has the option of returning some other sort of status code (internal server error for example or bad request) if the issue a bad request or an internal issue?

Ahhh I see what you mean. Yes I will change it.

can include in #13077

tomponline · 2024-03-08T10:51:48Z

test/suites/auth.sh

@@ -136,3 +143,245 @@ EOF
  lxc config unset oidc.issuer
  lxc config unset oidc.client.id
 }
+
+
+fine_grained_authorization() {


We should expand this to test for user_by filtering too in the refactor PRs.

tomponline

LGTM, thanks!

tomponline · 2024-03-08T10:52:26Z

There's a few follow up points for the forthcoming refactor but no blockers i could see.

markylaing · 2024-03-08T10:54:29Z

In general we need to move DB queries out of the openfga driver and into the clusterDB package. As part of future refactor.

There are no queries in the driver itself, the queries are in the datastore implementation in the lxd/db package. Should I move them into the cluster package?

tomponline · 2024-03-08T11:02:49Z

In general we need to move DB queries out of the openfga driver and into the clusterDB package. As part of future refactor.

There are no queries in the driver itself, the queries are in the datastore implementation in the lxd/db package. Should I move them into the cluster package?

Ah ok lets leave as is then. Thanks

markylaing · 2024-03-08T10:29:39Z

lxd/db/cluster/entities.go

+		return nil, fmt.Errorf("Could not get entity ID from URL: No statement found for entity type %q", entityType)
+	}
+
+	args := []any{0, projectName, location}


Will update the comments in this function as the 0 here looks suspicious. It's there because the entity ID queries take an index so that we can correlate the output (see PopulateEntityReferencesFromURLs). In this case we're only querying for one ID so I'm just passing in a placeholder value.

markylaing · 2024-03-08T11:04:21Z

lxd/db/openfga.go

+	return nil, nil
+}
+
+// ReadUsersetTuples is called on check requests. It is used to read all the "users" that have a given relation to


This is a little nuanced. "user" in this context is not the identity that made the request. OpenFGA tuples are composed of a user, a relation and an object. In the datastore implementation whenever we see a user that is an identity, we typically ignore it because all of their relations have been passed in contextually. Here are some examples:

user: server:/1.0 relation: server object: project:/1.0/projects/default

user: project:/1.0/projects/default relation: project object: instance:/1.0/instances/foo?project=default

user: identity:/1.0/auth/identities/oidc/jane.doe@example.com relation: member object: group:/1.0/auth/groups/foo-group

We would ignore this one, identities can't be given entitlements directly.

user: identity:/1.0/auth/identities/oidc/jane.doe@example.com relation: can_view object: instance:/1.0/instances/foo?project=bar

tomponline · 2024-03-08T11:07:23Z

This is a little nuanced. "user" in this context is not the identity that made the request. OpenFGA tuples are composed of a user, a relation and an object. In the datastore implementation whenever we see a user that is an identity, we typically ignore it because all of their relations have been passed in contextually. Here are some examples:

Thanks I think that would be worth clarifying in the comment then as it confused me a little.

markylaing added the Feature New feature, not a bug label Feb 27, 2024

markylaing self-assigned this Feb 27, 2024

github-actions bot added Documentation Documentation needs updating API Changes to the REST API labels Feb 27, 2024

markylaing force-pushed the embedded-openfga branch 2 times, most recently from c3b46ef to f5b107d Compare February 27, 2024 21:37

markylaing force-pushed the embedded-openfga branch from f5b107d to bf0bc76 Compare February 27, 2024 21:52

tomponline reviewed Feb 28, 2024

View reviewed changes

doc/api-extensions.md Outdated Show resolved Hide resolved

markylaing force-pushed the embedded-openfga branch from 5ed23d1 to 846014c Compare February 28, 2024 15:40

github-actions bot removed Documentation Documentation needs updating API Changes to the REST API labels Feb 28, 2024

markylaing marked this pull request as ready for review February 28, 2024 15:42

markylaing mentioned this pull request Feb 28, 2024

API: Change authorization_apis extension name to access_management. #12987

Merged

markylaing marked this pull request as draft February 28, 2024 21:07

markylaing mentioned this pull request Feb 28, 2024

Auth: Handle dangling permissions #12992

Merged

markylaing force-pushed the embedded-openfga branch 2 times, most recently from fd6d29b to aef530f Compare February 29, 2024 11:59

markylaing force-pushed the embedded-openfga branch from 5a5c91c to 510f2de Compare March 8, 2024 10:14

markylaing added 11 commits March 8, 2024 10:20

lxd/db/cluster: Add a method to get a single entity reference from a …

1746373

…URL. Signed-off-by: Mark Laing <mark.laing@canonical.com>

gomod: Add OpenFGA dependencies.

e51f12c

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/db: Adds the OpenFGA datastore implementation.

57b95ea

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/auth: Add the OpenFGA model.

be6b406

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/auth: Add identity provider groups to request details.

61ad209

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/auth: Update requestDetails method.

8389cf5

This commit updates the method to extract the identity provider groups and additionally use the more concise `request.GetCtxValue` method. Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd/auth: Add the embedded OpenFGA authorization driver.

74aff1a

Signed-off-by: Mark Laing <mark.laing@canonical.com>

lxd: Load the embedded OpenFGA driver by default.

1bbd495

Signed-off-by: Mark Laing <mark.laing@canonical.com>

test: Add a util for getting the fingerprint from a certificate.

0270135

Signed-off-by: Mark Laing <mark.laing@canonical.com>

test/suites: Add tests for fine-grained authorization.

a0fdce4

Signed-off-by: Mark Laing <mark.laing@canonical.com>

Makefile: Exclude dependency from spec generation.

28b5fd2

Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing force-pushed the embedded-openfga branch from 510f2de to 28b5fd2 Compare March 8, 2024 10:24

markylaing marked this pull request as ready for review March 8, 2024 10:24

markylaing requested a review from tomponline March 8, 2024 10:24

tomponline reviewed Mar 8, 2024

View reviewed changes

tomponline approved these changes Mar 8, 2024

View reviewed changes

tomponline merged commit ca62409 into canonical:main Mar 8, 2024
27 checks passed

markylaing commented Mar 8, 2024

View reviewed changes

Auth: Embedded OpenFGA authorization driver #12976

Auth: Embedded OpenFGA authorization driver #12976

Conversation

markylaing commented Feb 27, 2024 • edited Loading

markylaing commented Feb 27, 2024

tomponline commented Feb 27, 2024

tomponline commented Feb 28, 2024

tomponline commented Feb 28, 2024

markylaing commented Feb 28, 2024

markylaing commented Feb 28, 2024

markylaing commented Feb 28, 2024

markylaing commented Feb 28, 2024

mas-who commented Mar 1, 2024

mas-who commented Mar 1, 2024

edlerd commented Mar 1, 2024 • edited Loading

Consider the scenario:

mas-who commented Mar 4, 2024

markylaing commented Mar 4, 2024

tomponline commented Mar 8, 2024

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 Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

tomponline left a comment • edited Loading

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 left a comment

Choose a reason for hiding this comment

tomponline commented Mar 8, 2024

markylaing commented Mar 8, 2024

tomponline commented Mar 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomponline commented Mar 8, 2024

markylaing commented Feb 27, 2024 •

edited

Loading

edlerd commented Mar 1, 2024 •

edited

Loading

tomponline Mar 8, 2024 •

edited

Loading

tomponline left a comment •

edited

Loading