Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement new access API for instances #725

Open
stgraber opened this issue Apr 2, 2024 · 32 comments
Open

Implement new access API for instances #725

stgraber opened this issue Apr 2, 2024 · 32 comments
Assignees
Labels
API Changes to the REST API Documentation Documentation needs updating Easy Good for new contributors Feature New feature, not a bug

Comments

@stgraber
Copy link
Member

stgraber commented Apr 2, 2024

It would be nice to have a /1.0/instances/NAME/access endpoint which one could GET to retrieve the list of who's allowed access and what access level they have on the object.

@stgraber stgraber added Documentation Documentation needs updating Feature New feature, not a bug Easy Good for new contributors API Changes to the REST API labels Apr 2, 2024
@sharathsivakumar
Copy link
Contributor

@stgraber I would like to work on this!

@stgraber
Copy link
Member Author

stgraber commented Apr 3, 2024

I'm currently giving priority for those easier issues to the students at the University of Texas in Austin as they've literally ran out of things to work on but I'll happily assign you to this one if they don't claim it in the next week :)

@NotPranav0
Copy link

Hello, my friend @BajuMcBites , and I are students at UT Austin and after taking a virtualization class, we wanted to contribute to this project. Could we be assigned this issue? Thanks.

@stgraber
Copy link
Member Author

stgraber commented Apr 3, 2024

Sure, assigned it to you!

If you get @BajuMcBites to comment in this issue I can assign them too.

@BajuMcBites
Copy link
Contributor

Hello!

@BajuMcBites
Copy link
Contributor

Hello, I am starting to work on this issue and was wondering if there were any files that I could look at for endpoints that are similar to this for refrence. Thanks.

@stgraber
Copy link
Member Author

The closest in the instance case would probably be what's behind /1.0/instances/NAME/state, so cmd/incusd/instance_state.go.

In this instance, we only need to get a GET implemented. It will need a new Access struct be defined in shared/api/access.go which we'll be able to use here for instances as well as in #724 for projects.

I think that struct will need a minimum of 3 fields:

  • Identifier string
  • Role string
  • Provider string

Note that unlike the state endpoint, we don't need to add any logic to internal/server/instance/... for this one, instead we'll need to add some logic to internal/server/auth so we can query who can access a particular instance (passing in the project name and instance name).

@BajuMcBites
Copy link
Contributor

Thanks, Ill start looking into that

@BajuMcBites
Copy link
Contributor

BajuMcBites commented Apr 30, 2024

Hello, I just want to clarify the approach I was thinking of taking to make sure I’m on the right track.

  • Get endpoint in a new file called at cmd/incusd/access.go similar to the get endpoint of /1.0/instances/NAME/state

  • struct for the return object of the the access endpoint in a new file called shared/api/access.go

  • Logic inside a new file called internal/server/auth/access.go that uses the name passed in the endpoint to query who has access to the instance.

    • This part I’m still a little confused about, do you have any recommendation for documentation that will be useful/ list of some data structures that store the information that I’ll need to query? I wasn't able to find anything that stored who has access to an instance or what instances users have access to.

Then I can test the endpoint by using incus query --wait <endpoint>

@stgraber
Copy link
Member Author

  • Get endpoint in a new file called at cmd/incusd/access.go similar to the get endpoint of /1.0/instances/NAME/state

That should be instance_access.go.

  • struct for the return object of the the access endpoint in a new file called shared/api/access.go

Yep, this one goes as access.go instead of instance_access.go because we'll use the same struct when we do the project one (#724).

Logic inside a new file called internal/server/auth/access.go that uses the name passed in the endpoint to query who has access to the instance.

* This part I’m still a little confused about, do you have any recommendation for documentation that will be useful/ list of some data structures that store the information that I’ll need to query? I wasn't able to find anything that stored who has access to an instance or what instances users have access to.

Yeah, that part will likely be a tiny bit more complex than that.

I think we'll need to:

  • Add a GetInstanceAccess(ctx context.Context, projectName string, instanceName string) (*api.Access, error) function to the Authorizer interface and then implement it for both the tls and openfga drivers.
  • Have the new function in instance_access.go call s.Authorizer.GetInstanceAccess to get the value and return it.

There's a bit of an issue here that just because the authorizer is openfga doesn't mean that tls clients aren't also supported, but that's something I've got to sort out for other things so I can deal with that one later.

@BajuMcBites
Copy link
Contributor

This is my first time working with authorization technology so sorry if I ask a lot of questions.

  • I still am not too sure how to see exactly who has access, I see that the Authorizer interface has CheckPermissions and GetPermissionChecker methods which using those or a similar implementation to how those work could probably be used to check the permissions of every user iteratively, but that doesn’t feel like a good approach.

  • Is access only restricted by project or can it also be restricted by instance as well? In driver_tls.go I only see checks by project so I want to make sure.

@stgraber
Copy link
Member Author

stgraber commented May 1, 2024

  • I still am not too sure how to see exactly who has access, I see that the Authorizer interface has CheckPermissions and GetPermissionChecker methods which using those or a similar implementation to how those work could probably be used to check the permissions of every user iteratively, but that doesn’t feel like a good approach.

I'd say to focus on the tls driver for now as that's the easy one to test.

For that particular driver, permissions are pretty simple:

  • Unrestricted client certificate => access to all projects as admin
  • Unrestricted metrics certificate => access to all projects as view
  • Restricted client certificate => access to a limited set of projects as operator
  • Restricted metrics certificate => access to a limited set of projects as view

Per-instance permissions are possible but they're only supported by the openfga driver at this point and that driver is going to be a bit trickier to figure out who has access to what.

For the tls driver, you can very cheaply iterate over t.certificates, that struct has a function named GetCertificatesAndProjects which gets you two maps, one of all trusted certificates by their fingerprint and types and another map of projects per certificate fingerprint. You can easily iterate over that data to fill in the Access struct.

With that you can easily

@BajuMcBites
Copy link
Contributor

BajuMcBites commented May 1, 2024

I have a few more questions

For the Access Struct, I want to make sure I'm getting a few things right, we have:

  • Identifier string <- user's names from Certificate.subjects.names

  • Role string <- the role associated with the user (ex, admin, view, operator)

  • Provider string <- would this be the issuer of the Certificate? (Certificate.issuer.names)

  • I'm assuming that there would be multiple people with with varying levels of access, so would the access struct be a list of structs with the above 3 properties (identifier, role, provider) for each user.

Some other questions:

  • When getting the role, I see that we can tell if it is client, metric, or server from how we index into the first map, however I don't see how to tell if it is restricted or unrestricted, and I don't see this in the properties of the Certificate struct, how do I go about getting this information?

  • Should I check to see if the certificate is within the time bounds specified on the certificate, or is this unnecessary?

Finally I just want to make sure that I'm getting the maps from GetCertificatesAndProjects() correct.

  • First Map: map[Type]map[string]x509.Certificate
    - map from type -> fingerprint string -> Certificate
  • Second Map: map[string][]string)
    - map from fingerprint string -> project names

Also, once I get the Access struct clarified, I'll make a commit so it can be used in #724.

@BajuMcBites
Copy link
Contributor

BajuMcBites commented May 2, 2024

Hello again, I went ahead and started implementing what with what I described above, with the following struct setup (SingleAccess struct name will probably be changed) for #724:

type Access struct {

	// list of everyone who has access and what access level they have
	// Example: Sahaj03, view, given access by SimoneA 
	Accessors []SingleAccess `json:"Accessors" yaml:"Accessors"`

}

type SingleAccess struct {
	// Name of the user
	// Example: Sahaj03, JonB, SimoneA
	Identifier string  `json:"Identifier" yaml:"Identifier"`

	// The Role the user has on the instance
	// Example: admin, view, operator
	Role string  `json:"Role" yaml:"Role"`

	// Who gave the user this access
	// Example: Sahaj03, JonB, SimoneA
	Provider string  `json:"Provider" yaml:"Provider"`
}

@stgraber
Copy link
Member Author

stgraber commented May 2, 2024

I think we could just define Access as []AccessEntry.

For the fields, for a TLS entry, I'd expect:

  • Identifer => certificate fingerprint
  • Role => admin, operator or view (see above)
  • Provider => tls

@stgraber
Copy link
Member Author

stgraber commented May 2, 2024

  • Should I check to see if the certificate is within the time bounds specified on the certificate, or is this unnecessary?

Not necessary

@stgraber
Copy link
Member Author

stgraber commented May 2, 2024

  • When getting the role, I see that we can tell if it is client, metric, or server from how we index into the first map, however I don't see how to tell if it is restricted or unrestricted, and I don't see this in the properties of the Certificate struct, how do I go about getting this information?

A restricted certificate will be in the project map, an unrestricted one will not.

@BajuMcBites
Copy link
Contributor

BajuMcBites commented May 2, 2024

Hello, I made a PR for the changes I have made, and I believe it should handle the api for the cases where the authorizer is tls. There were a few things I wasn't 100% sure one which was what the context argument passed into GetInstanceAccess was supposed to be used for as well as when to return an error since GetCertificatesAndProjects doesn't return errors, and I don't see any places where errors would appear since the instance name checks are done prior to this method. I was also wondering if you had any advice on how I should go about getting started on openfga?

Also, when running make update-api to update the api documentation, I get the following error:
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe207ef]

@stgraber
Copy link
Member Author

stgraber commented May 3, 2024

Hello, I made a PR for the changes I have made, and I believe it should handle the api for the cases where the authorizer is tls. There were a few things I wasn't 100% sure one which was what the context argument passed into GetInstanceAccess was supposed to be used for as well as when to return an error since GetCertificatesAndProjects doesn't return errors, and I don't see any places where errors would appear since the instance name checks are done prior to this method. I was also wondering if you had any advice on how I should go about getting started on openfga?

That's fine, this implementation doesn't have anything cancelable so the context isn't used and you don't have anything that can throw an error so won't be returning any, but that's setting the stage for the openfga implementation where both are likely to be relevant.

@stgraber
Copy link
Member Author

stgraber commented May 3, 2024

Also, when running make update-api to update the api documentation, I get the following error: panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe207ef]

That's odd, I've never seen it crash that way. Do you have the rest of the stack trace?
Anyway, doesn't matter too much, that kind of mechanical change I can handle myself.

@BajuMcBites
Copy link
Contributor

Also, when running make update-api to update the api documentation, I get the following error: panic: runtime error: invalid memory address or nil pointer dereference [recovered] panic: runtime error: invalid memory address or nil pointer dereference [signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe207ef]

Do you have the rest of the stack trace?

swagger generate spec -o doc/rest-api.yaml -w ./cmd/incusd -m
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
        panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xe207ef]

goroutine 163 [running]:
go/types.(*Checker).handleBailout(0xc000b18600, 0xc000b41bd0)
        /usr/lib/go-1.22/src/go/types/check.go:367 +0x88
panic({0x112c1c0?, 0x19abfa0?})
        /usr/lib/go-1.22/src/runtime/panic.go:770 +0x132
go/types.(*StdSizes).Sizeof(0x0, {0x1404b00, 0x19b6e60})
        /usr/lib/go-1.22/src/go/types/sizes.go:228 +0x30f
go/types.(*Config).sizeof(...)
        /usr/lib/go-1.22/src/go/types/sizes.go:333
go/types.representableConst.func1({0x1404b00?, 0x19b6e60?})
        /usr/lib/go-1.22/src/go/types/const.go:76 +0x9e
go/types.representableConst({0x1409900, 0x19e9890}, 0xc000b18600, 0x19b6e60, 0xc000b3fd70)
        /usr/lib/go-1.22/src/go/types/const.go:92 +0x192
go/types.(*Checker).representation(0xc000b18600, 0xc0006eff00, 0x19b6e60)
        /usr/lib/go-1.22/src/go/types/const.go:256 +0x65
go/types.(*Checker).implicitTypeAndValue(0xc000b18600, 0xc0006eff00, {0x1404b00, 0x19b6e60})
        /usr/lib/go-1.22/src/go/types/expr.go:375 +0x2d7
go/types.(*Checker).assignment(0xc000b18600, 0xc0006eff00, {0x1404b00, 0x19b6e60}, {0x122ba8b, 0xe})
        /usr/lib/go-1.22/src/go/types/assignments.go:52 +0x2e5
go/types.(*Checker).exprInternal(0xc000b18600, 0x0, 0xc0006eff00, {0x1408098, 0xc0006ee780}, {0x1404a38, 0xc0001b2000})
        /usr/lib/go-1.22/src/go/types/expr.go:1175 +0x267a
go/types.(*Checker).rawExpr(0xc000b18600, 0x0, 0xc0006eff00, {0x1408098?, 0xc0006ee780?}, {0x1404a38?, 0xc0001b2000?}, 0x0)
        /usr/lib/go-1.22/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).exprWithHint(0xc000b18600, 0xc0006eff00, {0x1408098, 0xc0006ee780}, {0x1404a38, 0xc0001b2000})
        /usr/lib/go-1.22/src/go/types/expr.go:1563 +0x65
go/types.(*Checker).indexedElts(0xc000b18600, {0xc000b18400, 0x1b, 0xc000b2ce30?}, {0x1404a38, 0xc0001b2000}, 0xffffffffffffffff)
        /usr/lib/go-1.22/src/go/types/index.go:453 +0x129
go/types.(*Checker).exprInternal(0xc000b18600, 0x0, 0xc0006ef840, {0x1408098, 0xc0006eef00}, {0x0, 0x0})
        /usr/lib/go-1.22/src/go/types/expr.go:1247 +0x10dd
go/types.(*Checker).rawExpr(0xc000b18600, 0x0, 0xc0006ef840, {0x1408098?, 0xc0006eef00?}, {0x0?, 0x0?}, 0x0)
        /usr/lib/go-1.22/src/go/types/expr.go:979 +0x19e
go/types.(*Checker).expr(0xc000b18600, 0x0?, 0xc0006ef840, {0x1408098?, 0xc0006eef00?})
        /usr/lib/go-1.22/src/go/types/expr.go:1513 +0x30
go/types.(*Checker).varDecl(0xc000b18600, 0xc0004c65a0, {0xc00006e530, 0x1, 0x1}, {0x0, 0x0}, {0x1408098, 0xc0006eef00})
        /usr/lib/go-1.22/src/go/types/decl.go:521 +0x17b
go/types.(*Checker).objDecl(0xc000b18600, {0x1410280, 0xc0004c65a0}, 0x0)
        /usr/lib/go-1.22/src/go/types/decl.go:194 +0x9e5
go/types.(*Checker).packageObjects(0xc000b18600)
        /usr/lib/go-1.22/src/go/types/resolver.go:693 +0x4dd
go/types.(*Checker).checkFiles(0xc000b18600, {0xc00006e0a8, 0x1, 0x1})
        /usr/lib/go-1.22/src/go/types/check.go:408 +0x1a5
go/types.(*Checker).Files(...)
        /usr/lib/go-1.22/src/go/types/check.go:372
golang.org/x/tools/go/packages.(*loader).loadPackage(0xc0000c28c0, 0xc000a615f0)
        /home/sahaj03/go/pkg/mod/golang.org/x/tools@v0.9.3/go/packages/packages.go:1055 +0xa72
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
        /home/sahaj03/go/pkg/mod/golang.org/x/tools@v0.9.3/go/packages/packages.go:854 +0x1a9
sync.(*Once).doSlow(0x0?, 0x0?)
        /usr/lib/go-1.22/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
        /usr/lib/go-1.22/src/sync/once.go:65
golang.org/x/tools/go/packages.(*loader).loadRecursive(0x0?, 0x0?)
        /home/sahaj03/go/pkg/mod/golang.org/x/tools@v0.9.3/go/packages/packages.go:842 +0x4a
golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1(0x0?)
        /home/sahaj03/go/pkg/mod/golang.org/x/tools@v0.9.3/go/packages/packages.go:849 +0x26
created by golang.org/x/tools/go/packages.(*loader).loadRecursive.func1 in goroutine 117
        /home/sahaj03/go/pkg/mod/golang.org/x/tools@v0.9.3/go/packages/packages.go:848 +0x94
make: *** [Makefile:143: update-api] Error 2

@stgraber
Copy link
Member Author

stgraber commented May 3, 2024

Interesting, it's not obvious what caused this. Anyway, you can leave that part alone and I'll generate it here.

@BajuMcBites
Copy link
Contributor

Hello, I I'm trying to start working on the openfga part, I think I have started the server with :

sudo docker pull openfga/openfga
sudo docker run -p 8080:8080 -p 8081:8081 -p 3000:3000 openfga/openfga run

and got this url back
http://localhost:3000/playground

which I then set the incus config with:

incus config set openfga.api.url http://localhost:3000/playground

Is this the proper way to set up openfga?

Also, how do I go about querying the accesses? Is there something similar to the maps for tls? Thanks.

@stgraber
Copy link
Member Author

stgraber commented May 4, 2024

Incus requires both openfga.api.url and openfga.api.token, it also uses the REST API of OpenFGA which I think would be port 8080 in this case.

You should be able to specify the preshared method with something like:

sudo docker run -p 8080:8080 -p 8081:8081 -p 3000:3000 openfga/openfga run --authn-method=preshared --authn-preshared-keys="foobar"

Then have Incus access it through openfga.api.url=http://localhost:8080 openfga.api.token=foobar

@BajuMcBites
Copy link
Contributor

I tried running the above command and config set up, however the access endpoint is still defaulting to the tls driver instead of the fga driver

@stgraber
Copy link
Member Author

stgraber commented May 4, 2024

Hmm, you could try running incus monitor --pretty --type=logging while setting the openfga.api.url and openfga.api.token keys (unset/set), that may show you an error explaining what's going on.

@BajuMcBites
Copy link
Contributor

BajuMcBites commented May 4, 2024

I didn't get any errors, only:

DEBUG  [2024-05-04T00:23:45-05:00] Handling API request                          ip=@ method=GET protocol=unix url=/1.0 username=sahaj03
DEBUG  [2024-05-04T00:23:45-05:00] Handling API request                          ip=@ method=GET protocol=unix url=/1.0 username=sahaj03
DEBUG  [2024-05-04T00:23:45-05:00] Handling API request                          ip=@ method=PUT protocol=unix url=/1.0 username=sahaj03

Which I assume is the normal output

@stgraber
Copy link
Member Author

stgraber commented May 4, 2024

Oh, doh, I forgot you also need a store to be created!

I don't remember if their web interface lets you create it, from the CLI, you'd do fga store create foo, that will show you an id for it which you then need to put in with incus set openfga.store.id=XYZ

@BajuMcBites
Copy link
Contributor

Thanks, that got it working.

server -> admin: Full access to Incus.
server -> operator: Full access to Incus, without edit access on server configuration, certificates, or storage pools.
server -> viewer: Can view all server level configuration but cannot edit. Cannot view projects or their contents.
project -> manager: Full access to a single project, including edit access.
project -> operator: Full access to a single project, without edit access.
project -> viewer: View access for a single project.
instance -> manager: Full access to a single instance, including edit access.
instance -> operator: Full access to a single instance, without edit access.
instance -> user: View access to a single instance, plus permissions for exec, console, and file APIs.
instance -> viewer: View access to a single instance.

  • I found this specification on the incus doc website, however I'm not sure how I go about getting the certificates with their permissions from the openfga server. Any tips on that?
  • Also, to test, I'm going to have to add certificates, but it doesn't appear incus config trust add <client_name> will do the trick since it it doesn't have any flags to restrict to instances, only projects. Is there a different command that works?

@rhamzeh
Copy link

rhamzeh commented May 7, 2024

It would be nice to have a /1.0/instances/NAME/access endpoint which one could GET to retrieve the list of who's allowed access and what access level they have on the object.

Per-instance permissions are possible but they're only supported by the openfga driver at this point and that driver is going to be a bit trickier to figure out who has access to what.

OpenFGA has just introduced experimental support for a ListUsers endpoint. I don't know what your threshold is to rely on an experimental method. Assuming we find no issues with it and no user requests that imply drastically changing the interface, it should be moving to non-experimental in a month or two

@BajuMcBites
Copy link
Contributor

Thanks, I'll look into that. I'm going to be out of town for the next 2 weeks, but I'll resume work on this issue when I get back.

stgraber pushed a commit to BajuMcBites/incus that referenced this issue May 9, 2024
Closes lxc#725

Signed-off-by: Sahaj Bhakta <sahajbhakta@gmail.com>
@stgraber
Copy link
Member Author

stgraber commented May 9, 2024

@rhamzeh ah, that's great, it will make handling this kind of stuff a LOT easier!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes to the REST API Documentation Documentation needs updating Easy Good for new contributors Feature New feature, not a bug
Development

No branches or pull requests

5 participants