OpenFGA authorization driver #12252

markylaing · 2023-09-14T12:13:52Z

This PR adds an OpenFGA authentication driver to LXD https://warthogs.atlassian.net/browse/LXD-389?atlOrigin=eyJpIjoiZWExM2NhNjgwMWU2NDZhMGJhODc5MjAzYmZiZGFkNTYiLCJwIjoiaiJ9.

The spec can be found at https://discourse.ubuntu.com/t/openfga-authorization-driver/38979

Work still to be done:

Setting authentication model ID in database. Avoid race condition.
Permission sync remove dangling permissions.
Remove all usages of context.Background().
Unit and system tests (also system tests for RBAC).
Documentation

tomponline · 2023-09-15T13:16:34Z

@markylaing i know this is heavily under development for now, so no need to do this right away, but when it comes to review and merge, I would appreciate it if you could split this into two PRs; 1 for the interface and hook changes across the code base, and 2 introducing the new openfga authorization driver.

markylaing · 2023-09-15T16:07:46Z

RBAC tests passing also :)

markylaing

@tomponline I've added a few comments to start discussion on a few of the main things I need feedback on. There are still a fair few things to do before this is ready for a proper review.

lxd/auth/driver_openfga_model.openfga

markylaing · 2023-09-18T12:13:41Z

lxd/auth/driver_openfga.go

+
+	var builtinAuthorizationModel client.ClientWriteAuthorizationModelRequest
+
+	err = json.Unmarshal([]byte(authModel), &builtinAuthorizationModel)


Migrations

Here authModel is the json string that is generated via make update-openfga. If there is an existing model in the store, we ensure that this model is exactly equal to the model we have statically defined. This is where we would have an opportunity to detect different model versions and apply update logic.

Migrations in OpenFGA follow a pretty basic idea. The idea is to create an intermediary model which supports both the new and the old, then write all the tuples required to support the new schema, delete all the tuples that are no longer required, and finally write the updated model (see https://openfga.dev/docs/modeling/migrating/migrating-relations).

In practice, I think we will likely have something like:

Under an lxd/auth/openfga directory:

model_v1.openfga

model_v1.go

model_v2_compatible.openfga

model_v2_compatible.go

model_v2.openfga

model_v2.go

migrations.go

synchronise.go

The migrations.go file will contain a map of methods which accept an openfga client and performs the necessary actions to migrate from one schema to the next. These methods will only be valid while the compatible model is in place.

The synchronise.go file will contain a map of methods which accept an fga client and a set of LXD resources and ensure they match 1:1 with what is present in the OpenFGA store. Like migrations, this is also model specific.

When we connect to the OpenFGA server:

Get the current auth model.

Check if it is the latest version. If it is, sync resources and continue. Otherwise....

Write the compatible model.

Call the associated method from migrations.go.

Write the latest model.

Perform a final resource sync from synchronise.go.

Hopefully we won't have to do this very often.

I think we can handle this when we come to update the model.

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

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

This makes it available from other network namespaces when running clustering tests. Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing · 2023-10-27T10:48:37Z

I have added reference and explanation documentation. I think this will also need a how-to but I need to figure out how to easily set up an OIDC server in order to do that (keycloak requires a lot of clicking around and openjdk-17).

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

TLS authorization and RBAC have their own sections in the authorization page now so these are removed. Additionally I have updated the OIDC and Candid sections to notify users that they must configure authorization, else their servers are not secure. Signed-off-by: Mark Laing <mark.laing@canonical.com>

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

tomponline

Thanks, this is great work.
Please can you address/discuss the changes via your existing github card for further minor work.

But I'll merge this now as I can't see any major issues.

Thanks for your thoroughness.

tomponline · 2023-10-27T13:55:00Z

lxd/auth/driver_openfga.go

+	"github.com/canonical/lxd/shared/logger"
+)
+
+func WriteOpenFGAAuthorizationModel(ctx context.Context, apiURL string, apiToken string, storeID string) (string, error) {


Could you add a docblock to this explaining what it does please.

tomponline · 2023-10-27T13:57:28Z

lxd/auth/driver_openfga.go

+	}
+
+	if readModelResponse.AuthorizationModel.SchemaVersion != builtinAuthorizationModel.SchemaVersion {
+		return fmt.Errorf("Existing OpenFGA model has schema version %q, but our model has version %q", readModelResponse.AuthorizationModel.SchemaVersion, builtinAuthorizationModel.SchemaVersion)


Des this stop LXD from starting or just log a warning? Does it prevent the authorizer from working?

tomponline · 2023-10-27T14:00:13Z

lxd/daemon.go

@@ -1802,6 +1803,246 @@ func (d *Daemon) Stop(ctx context.Context, sig os.Signal) error {
 	return err
 }

+// Setup OpenFGA.


A docblock comment explaining what this function does would be nice.

tomponline · 2023-10-27T14:01:41Z

lxd/daemon.go

+		if openFGAAuthorizationModelID == "" {
+			// We should never be missing the model ID at start up if we have other connection details (this means
+			// something went wrong the last time we tried to set it up).
+			logger.Warn("OpenFGA authorization driver is misconfigured, skipping...")


So this will fallback to TLS only mode or only allow unix socket operation?

I think either both should be warnings or both should be errors, as they both result in the authorizer not working right?

tomponline · 2023-10-27T14:03:02Z

lxd/daemon.go

+		} else {
+			err = d.setupOpenFGA(openfgaAPIURL, openfgaAPIToken, openfgaStoreID, openFGAAuthorizationModelID)
+			if err != nil {
+				logger.Error("Failed to configure OpenFGA. Reverting to default TLS authorization", logger.Ctx{"error": err})


Same question as above

tomponline · 2023-10-27T14:04:14Z

lxd/api_1.0.go

@@ -808,6 +813,50 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
 	return response.EmptySyncResponse
 }

+func doApi10PreNotifyTriggers(d *Daemon, clusterChanged map[string]string, newClusterConfig *clusterConfig.Config) error {


missing docblock explaining what this is and what it does

tomponline · 2023-10-27T14:05:05Z

lxd/api_1.0.go

@@ -763,6 +763,11 @@ func doApi10Update(d *Daemon, r *http.Request, req api.ServerPut, patch bool) re
 		}
 	})

+	err = doApi10PreNotifyTriggers(d, clusterChanged, newClusterConfig)


Do we need to support a revert here if something fails later on?

markylaing added the Feature New feature, not a bug label Sep 14, 2023

markylaing self-assigned this Sep 14, 2023

github-actions bot added the Documentation Documentation needs updating label Sep 14, 2023

markylaing force-pushed the openfga branch 5 times, most recently from 29aea6c to fdd4e9b Compare September 15, 2023 00:17

tomponline mentioned this pull request Sep 15, 2023

Add ReBAC authorization with OpenFGA #11905

Closed

markylaing force-pushed the openfga branch 2 times, most recently from e8c2b94 to 4a486fc Compare September 15, 2023 16:07

markylaing commented Sep 18, 2023

View reviewed changes

markylaing force-pushed the openfga branch 10 times, most recently from 6dff40e to 0cd077a Compare September 24, 2023 17:59

markylaing mentioned this pull request Sep 25, 2023

Authorization refactor in preparation for fine-grained authorization #12313

Merged

markylaing force-pushed the openfga branch 3 times, most recently from abf91ee to 3840e42 Compare September 29, 2023 15:53

github-actions bot added the API Changes to the REST API label Sep 29, 2023

markylaing force-pushed the openfga branch from 3840e42 to 501a556 Compare October 5, 2023 16:20

markylaing added 13 commits October 27, 2023 11:40

lxd/auth: Adds Resources type and load option.

cfe169f

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

gomod: Adds openfga dependency.

0d78582

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

lxd/auth: Adds OpenFGA authorization driver.

fcf9018

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

lxd/cluster/config: Adds OpenFGA config keys.

5790b5f

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

lxd-metadata: Runs make update-metadata.

4fe478b

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

lxd/db: Exports StoragePoolVolumeTypeToName function.

44f9cdc

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

lxd: Adds method to daemon to load the OpenFGA authorizer.

0f3088e

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

lxd: Loads OpenFGA authorizer at startup if configured.

89530bc

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

lxd: Load OpenFGA authorizer on config change.

f9f88ba

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

test/lint: Adds linter for openfga model.

5567831

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

test/includes: Adds util for getting certificate fingerprint.

cb94dfd

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

test/includes: Adds utils for running and interacting with an openfga…

94311c5

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

test/includes: Use hostname for RBAC server endpoint.

ec3afff

This makes it available from other network namespaces when running clustering tests. Signed-off-by: Mark Laing <mark.laing@canonical.com>

markylaing force-pushed the openfga branch from a708ca5 to 9f6aa5a Compare October 27, 2023 10:43

markylaing added 11 commits October 27, 2023 12:01

test/suites: Adds OpenFGA test suite.

e27f35c

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

test/suites: Adds OpenFGA clustering test.

3799f95

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

test: Runs OpenFGA tests in main.

edd5831

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

workflows: Installs openfga server and CLI in github action.

c2ac155

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

doc: Adds OpenFGA to wordlist.

b911d44

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

doc: Adds openfga server configuration options.

705d692

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

doc: Adds authorization explanation page.

e4ad714

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

doc: Update references to RBAC section.

a3d58fc

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

doc: Adds authorization page to security related links.

cd88d9f

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

doc: Adds authorization page to explanation section.

3b45b5a

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

markylaing force-pushed the openfga branch from 9f6aa5a to 3b45b5a Compare October 27, 2023 11:01

markylaing requested a review from simondeziel October 27, 2023 11:02

tomponline approved these changes Oct 27, 2023

View reviewed changes

tomponline merged commit db2afaa into canonical:main Oct 27, 2023
25 of 26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenFGA authorization driver #12252

OpenFGA authorization driver #12252

markylaing commented Sep 14, 2023 •

edited

Loading

tomponline commented Sep 15, 2023

markylaing commented Sep 15, 2023

markylaing left a comment

markylaing Sep 18, 2023

markylaing Sep 21, 2023

markylaing commented Oct 27, 2023

tomponline left a comment

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023

tomponline Oct 27, 2023


		var builtinAuthorizationModel client.ClientWriteAuthorizationModelRequest

		err = json.Unmarshal([]byte(authModel), &builtinAuthorizationModel)

OpenFGA authorization driver #12252

OpenFGA authorization driver #12252

Conversation

markylaing commented Sep 14, 2023 • edited Loading

tomponline commented Sep 15, 2023

markylaing commented Sep 15, 2023

markylaing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Migrations

Choose a reason for hiding this comment

markylaing commented Oct 27, 2023

tomponline 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

markylaing commented Sep 14, 2023 •

edited

Loading