-
-
Notifications
You must be signed in to change notification settings - Fork 224
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
base: main
Are you sure you want to change the base?
Conversation
Some context is needed for a few changes. |
16c113b
to
54f1402
Compare
cmd/incusd/daemon.go
Outdated
// 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) |
There was a problem hiding this comment.
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 StopService
s are a no-op.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That should be fine
There was a problem hiding this comment.
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.
3eabfb1
to
f5360a8
Compare
I don’t really get why the code checks are failing, the errors are not something my knowledge of Go allows me to decipher :) |
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Alright, got all the tests to behave. Now I can actually review this PR :) |
Nice! |
Looks pretty good to me! What we still need to add to this are two commits:
|
With this change I'm getting pretty tempted to rename |
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Stéphane Graber <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Signed-off-by: Benjamin Somers <[email protected]>
Closes: #188