auth: Add entitlements to LXD resources if the current identity is fine-grained

[gabrielmougard]

JIRA ticket: https://warthogs.atlassian.net/browse/LXD-2208
Specification link: https://docs.google.com/document/d/1GxWV5J57MLrjGEY5RDG7eS86J99A8RMmjKTpn0mEhgY/edit?tab=t.0

How to test

# Add TLS certificate to LXD (this can be done through LXD UI. Download the .pfx file)
# Create an auth group
lxc auth group create test-group

# Add an new identity to this group ("lxd-ui" is my TLS certificate identifier created by LXD UI)
lxc auth identity group add tls/lxd-ui test-group-1

# Create resources and add permissions to them
lxc auth group permission add test-group-1 instance c1 can_view project=default

# Create a .pfx certificate (with password "1234") and associate it to an auth group and give it some permissions. You can call the API like the following to perform fine-grained calls:
curl --insecure --cert-type P12 --cert lxd-ui-fine-grained-tls.pfx:1234 "https://127.0.0.1:8443/1.0/instances/c1?recursion=1&with-entitlements=true" | jq '.'

The CURL output should give you something like:

{
  "type": "sync",
  "status": "Success",
  "status_code": 200,
  "operation": "",
  "error_code": 0,
  "error": "",
  "metadata": {
    "entitlements": [
      "can_view"
    ],
    "name": "c1",
    "description": "this is a test ",
    "status": "Running",
    "status_code": 103,
    "created_at": "2024-11-12T09:58:54.960586253Z",
    "last_used_at": "2024-11-16T12:15:13.572661467Z",
    "location": "none",
    "type": "container",
    ...
  }
}

[markylaing]

I'm very confused by this. There are a few things to address here.

1. We can't bypass calls to OpenFGA by accessing the cache directly. We don't know that the cache contains all the necessary information. We will need to call the OpenFGA server itself to check the entitlements on a per-entity basis. The cache should be read from/written to only from in OpenFGA datastore implementation itself.
2. This is leaking a lot of OpenFGA specific logic from the lxd/db/openfga and lxd/auth/drivers packages. We should try our utmost to keep that logic where it is.
3. Using reflection should be a last resort IMO. If we absolutely require it then we need to have an awful lot of comments to explain why and what it's doing.

For 1, we still need to call the OpenFGA server. I suggest calling CheckPermission for each entitlement in the auth.EntitlementsByEntityType map value for the entity type. This will be inefficient when listing entitlements for a list of resources (e.g. GET /1.0/instances?recursion=1&with-entitlements=true) so I suggest in this calling auth.GetPermissionChecker with each entitlement in the auth.EntitlementsByEntityType map.

Alternatively, I suggest adding two methods to the auth.Authorizer interface to perform the enrichment. One for a single entity which will call Check on the OpenFGA server, and one for a list of entities that will call ListObjects. The implementation for the TLS authorizer can return a 501 not implemented. The authorizer itself can return a bad request error if the caller is not fine-grained.

For 3, I suggest creating an interface with method AddEntitlements([]auth.Entitlement) and implement it for the API types in question (all API types represented by the OpenFGA model).

[gabrielmougard]

Thanks. I agree with you on the three points. I'll come up with a new approach.

[gabrielmougard]

@markylaing I updated the approach. What do you think now?

[markylaing]

The approach looks good :) It might be possible to make the new Authorizer functions more efficient by calling ListObjects and Check on the OpenFGA server directly. We can revisit that at a later date though, for now re-using CheckPermission and GetPermissionChecker is reasonable.

[markylaing]

Looking good overall. Just a few tweaks and we'll be nearly there I think.

[markylaing]

I'm not sure on these function signatures. Especially in the second case where the caller must prepare a list of URLs to correspond by index to the list of entities.

It seems odd to me that we're passing in e.g. an api.Instance, along with it's URL and it's type, both of which can be derived from the fact that it is an api.Instance.

Do you think it's worth adding two more methods to EntityWithEntitlements e.g.:

type EntityWithEntitlements interface {
    AddEntitlement(auth.Entitlement)
    URL() string
    EntityType() entity.Type
}

Then these function signatures can become:

AddEntityEntitlements(ctx context.Context, entity entity.EntityWithEntitlements)
AddEntitiesEntitlements(ctx context.Context, entities entity.EntityWithEntitlements)

Let me know what you think.

[gabrielmougard]

I like the the idea but I'd prefer to call the URL() string method EntityURL() string instead to be more explicit. For example, the URL scheme used for an LXD instance can be defined like:
url := api.NewURL().Path(version.APIVersion, instancePath, instance.Name).Project(instance.Project)
and if instance.Project is default then no query param will be added whereas an OpenFGA entity URL will systematically have the project query param set even if its default.

[gabrielmougard]

@markylaing Actually, I realize this will be a bit more complicated than that:

There is an existing logic for URL generation in the shared/entity package, but I won't be able to use that from the shared/api package (circular dependency issues) where my implementations of EntityWithEntitlements are defined...

[gabrielmougard]

@markylaing can't we just move the url.go file from shared/entity into the shared/api package? We'd keep the type.go in shared/entity. This way we can reuse the existing <Entity>URL functions for the API LXD entity that would implement the EntityWithEntitlements interface. I think this would make the circular dep issues go away..

[gabrielmougard]

This seems difficult. Instead could we do something like:

type EntityWithEntitlements interface {
	AddEntitlement(entitlement string)
	EntityType() Type
}

type EntityWithEntitlementsAndURL struct {
	Entity EntityWithEntitlements
	EntityURL    *api.URL
}

...

// In the interface
AddEntityEntitlements(ctx context.Context, *entity entity.EntityWithEntitlementsAndURL)
AddEntitiesEntitlements(ctx context.Context, entities []*entity.EntityWithEntitlementsAndURL)

This way, we can set the EntityURL in the lxd package when calling the AddEntityEntitlements / AddEntitiesEntitlements function without having any issues. What do you think?

Here is an example of calling this interface for enriching an api.Instance:

err = d.authorizer.AddEntitlements(
	r.Context(),
	&entity.EntityWithEntitlementsAndURL{
		Entity: state.(*api.Instance),
		EntityURL: entity.InstanceURL(c.Project().Name, c.Name()),
	},
)
if err != nil {
	return response.SmartError(err)
}

[markylaing]

Can we just define the interface somewhere else? In fact, it is only used internally, so perhaps it is better placed in lxd/auth? Would that help with the circular dependencies?

[markylaing]

I think leave it as it is for now. These are finer details that we can work out before merging. For now it might be worth getting good test coverage and waiting on reviews of the specification and PR from Tom. Then we can discuss together :)

[tomponline]

@gabrielmougard closing for now until you can work on this in the new year.