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

Add authorization scriptlet #1412

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Add authorization scriptlet #1412

wants to merge 14 commits into from

Conversation

bensmrs
Copy link
Contributor

@bensmrs bensmrs commented Nov 22, 2024

Closes: #188

@bensmrs
Copy link
Contributor Author

bensmrs commented Nov 22, 2024

Some context is needed for a few changes.
Scriptlet code refactoring was done to avoid dependency cycles, as the instance placement scriptlet interacts with the cluster, which checks authorization, which in turn now uses a scriptlet. So (un)marshalling and log functions are now in their own packages, and the authorization scriptlet also gets its own package.
Authorization code refactoring was done so that the authorization scriptlet can have access to required struct fields, and also to break a dependency cycle.
The rest should be straightforward.

// Fail if not using the default tls or scriptlet authorizer.
switch d.authorizer.(type) {
case *auth.TLS, *auth.Scriptlet:
d.authorizer, err = auth.LoadAuthorizer(d.shutdownCtx, auth.DriverScriptlet, logger.Log, d.clientCerts)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to add a StopService there, please tell. I didn’t add it because basically, declaring two authorizers is forbidden, and both the default authorizer and the scriptlet authorizer’s StopServices are a no-op.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That should be fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well now that I’m thinking about it, if we create a new authorizer that needs to be properly cleaned (closing a fd, releasing a lock…), that may be problematic.
On the other hand, the way authorizers work now feels like it has only be thought for two authorizers (the default one and one non-default).

I feel that the {Stop the default -> Load the new -> [if something isn’t right,] Stop the new -> Load the default again} process needs to be rewritten for the OpenFGA authorizer, and it needs to be more generic to allow new authorizers to be written easily.

We should also probably be more defensive here and check the unicity of authorizer configuration keys before initializing the authorizers…

So we keep it this way for this PR and later think of a cleaner approach? I can probably work on that next month. If that sounds good, I’ll summarize it and open another issue.

@bensmrs bensmrs marked this pull request as draft November 22, 2024 14:33
@bensmrs bensmrs marked this pull request as ready for review November 22, 2024 16:23
@bensmrs
Copy link
Contributor Author

bensmrs commented Nov 22, 2024

I don’t really get why the code checks are failing, the errors are not something my knowledge of Go allows me to decipher :)

@stgraber
Copy link
Member

Alright, got all the tests to behave. Now I can actually review this PR :)

@bensmrs
Copy link
Contributor Author

bensmrs commented Nov 25, 2024

Nice!
Btw, even if one of my comments/reviews is marked as outdated, it’s very much not :)

cmd/incusd/daemon.go Outdated Show resolved Hide resolved
@stgraber
Copy link
Member

Looks pretty good to me!
I'll do a more detailed pass later, going commit by commit as for this pass I just looked at the overall diff.

What we still need to add to this are two commits:

  • api: authorization_scriptlet
  • doc/authorization: Add authorization scriptlet

@stgraber
Copy link
Member

With this change I'm getting pretty tempted to rename openfga.* to authorization.openfga.* but we'll possibly do that a bit later as I have nagging feeling we'll want to do the same with loki.* when we start adding some more options for logging too.

@github-actions github-actions bot added the API Changes to the REST API label Nov 26, 2024
Signed-off-by: Benjamin Somers <[email protected]>
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
Development

Successfully merging this pull request may close these issues.

Basic scriptlet-based authorizer
2 participants