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

Merged
merged 14 commits into from
Dec 8, 2024
Merged

Add authorization scriptlet #1412

merged 14 commits into from
Dec 8, 2024

Conversation

bensmrs
Copy link
Contributor

@bensmrs bensmrs commented Nov 22, 2024

Closes: #188

@bensmrs bensmrs requested a review from stgraber as a code owner November 22, 2024 13:49
@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.

@bensmrs bensmrs force-pushed the main branch 2 times, most recently from 16c113b to 54f1402 Compare November 22, 2024 14:12
cmd/incusd/daemon.go Outdated Show resolved Hide resolved
@bensmrs bensmrs marked this pull request as draft November 22, 2024 14:33
@bensmrs bensmrs force-pushed the main branch 2 times, most recently from 3eabfb1 to f5360a8 Compare November 22, 2024 14:54
@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 :)

@github-actions github-actions bot added the Documentation Documentation needs updating label Nov 25, 2024
@stgraber stgraber force-pushed the main branch 2 times, most recently from be212ca to 3a60a1e Compare November 25, 2024 20:56
@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
@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 3, 2024

Hi! Does it help if I rebase the PR?

@stgraber
Copy link
Member

stgraber commented Dec 4, 2024

Yeah, that'd be quite useful, thanks!

@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 5, 2024

There you go. I started freaking out when I saw changes to the OpenFGA driver, but it wasn’t that dramatic in the end.

doc/authorization.md Outdated Show resolved Hide resolved
doc/authorization.md Outdated Show resolved Hide resolved
doc/.wordlist.txt Outdated Show resolved Hide resolved
@stgraber
Copy link
Member

stgraber commented Dec 5, 2024

Looking good.

On the details part, I don't think we want to expose a whole RequestDetails to the scriptlet as the forwarded pieces are confusing and shouldn't be handled by the scriptlet.

Instead we should expose a script which just includes:

  • Protocol string
  • Username string
  • Project string
  • AllProjects bool

And have the username and protocol be pre-expanded as is done in username and authenticationProtocol.

@bensmrs bensmrs marked this pull request as draft December 6, 2024 14:14
@bensmrs bensmrs marked this pull request as ready for review December 6, 2024 15:48
doc/authorization.md Outdated Show resolved Hide resolved
@stgraber
Copy link
Member

stgraber commented Dec 6, 2024

Looks good, just a capitalization nit left ;)

It's nice that the test uses OIDC, makes it easier to try multiple users.

@bensmrs bensmrs force-pushed the main branch 2 times, most recently from 6dfcd68 to 0f79ccf Compare December 6, 2024 22:31
@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 7, 2024

It's nice that the test uses OIDC, makes it easier to try multiple users.

It was actually a mix of “I don’t want to generate certificates and pass them cleanly to Incus” and ”Oh! The tests make it sooo easy to use OIDC”, combined to “I prefer to use explicit user names in the tests”.
Kudos for all the very useful helper functions!

@bensmrs
Copy link
Contributor Author

bensmrs commented Dec 7, 2024

Hey! all these rebases are starting to become quite boring :D

bensmrs and others added 14 commits December 7, 2024 02:32
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Stéphane Graber <stgraber@stgraber.org>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
Signed-off-by: Benjamin Somers <benjamin.somers@imt-atlantique.fr>
@stgraber stgraber merged commit 5437768 into lxc:main Dec 8, 2024
30 checks passed
@stgraber
Copy link
Member

stgraber commented Dec 8, 2024

Thanks for the great work!

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request Dec 13, 2024
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [lxc/incus](https://github.com/lxc/incus) | minor | `v6.7.0` -> `v6.8.0` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>lxc/incus (lxc/incus)</summary>

### [`v6.8.0`](https://github.com/lxc/incus/releases/tag/v6.8.0): Incus 6.8

[Compare Source](lxc/incus@v6.7.0...v6.8.0)

#### What's Changed

-   exec: Consume websocket pings for stderr by [@&#8203;stefanor](https://github.com/stefanor) in lxc/incus#1380
-   incus-simplestreams: Add prune command by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1381
-   internal/instance: Fix validation of volatile.cpu.nodes by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1394
-   Add a function to clone map and use it where appropriate by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1397
-   cgo/process_utils: fix 32bit builds by [@&#8203;brauner](https://github.com/brauner) in lxc/incus#1398
-   Start using goimports by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1399
-   instance/config: Mark user keys as live updatable by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1404
-   incus/internal/server/instance/drivers/: Fix incorrect Vars file mapping in edk2 driver by [@&#8203;cmspam](https://github.com/cmspam) in lxc/incus#1406
-   zfs: load keys for encrypted datasets during pool import by [@&#8203;cyphar](https://github.com/cyphar) in lxc/incus#1384
-   incusd/instance: Lock image access by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1408
-   incus/image: Make use of server-side alias handling by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1409
-   incusd/cluster: Validate cluster HTTPS address on join too by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1411
-   Remove metadata info from space usage calculation by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1417
-   Add ability to set the initial owner of a custom volume by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1415
-   Allow local live-migration between storage pools by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1410
-   incus: Add aliases completion by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1385
-   golangci: Add local prefixes for goimports by [@&#8203;breml](https://github.com/breml) in lxc/incus#1401
-   client: invalidate simple streams cache by [@&#8203;breml](https://github.com/breml) in lxc/incus#1424
-   incusd/instances_post: Fix cluster internal migrations by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1427
-   Fix DHCP client keeping container up by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1430
-   Add support for VGA console screenshots by [@&#8203;breml](https://github.com/breml) in lxc/incus#1431
-   Add --reuse to incus image import by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1428
-   Fix random ETag values due to map ordering by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1432
-   incusd/task: Fix wait group logic (more entries than running tasks) by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1433
-   Allow setting aliases during raw image upload by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1434
-   Fixes an issue when copying a custom volume using the `--refresh` flag by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1437
-   Openfga improvements by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1435
-   doc/instance/properties: Add missing instance properties by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1439
-   incusd/daemon_storage: Ensure corect symlinks for images/backups by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1441
-   incusd/storage/lvm: Handle newer LVM by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1442
-   Tweak rendering of manpage in doc by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1443
-   incusd/storage/lvm: Require 512-bytes physical block size for VM images by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1444
-   incusd: Fill ExpiryDate and remove LastUsedDate in volumeSnapshotToProtobuf by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1448
-   incusd/device/tpm: Wait for swtpm to be ready by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1447
-   incus: Improve completion for `file push` and `file pull` by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1445
-   incusd/auth/tls: Restrict config access to non-admin by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1451
-   incusd/storage: Handle default disk size in GetInstanceUsage by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1452
-   incus: Improve completion for some file sub-commmands by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1453
-   incus: Fix completion for `profile copy` by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1454
-   incus: Add completion for `image alias` subcommands  by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1457
-   doc/installing: Update Fedora instructions by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1456
-   Fix gap in validation of pre-existing certificates when switching to PKI mode by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1458
-   doc/network_forwards: Split configuration into own table by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1460
-   chore: Happy path on the left, early return by [@&#8203;breml](https://github.com/breml) in lxc/incus#1461
-   incus: Fix completion for `image alias create` by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1459
-   incus/top: Ignore CPU idle time by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1462
-   incus: Display the alias expansion when execution of an alias fails by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1464
-   lint: disallow restricted licenses in go-licenses by [@&#8203;breml](https://github.com/breml) in lxc/incus#1466
-   chore: code structure, Go identifier shaddowing by [@&#8203;breml](https://github.com/breml) in lxc/incus#1465
-   incus: Fix alias arguments handling by [@&#8203;montag451](https://github.com/montag451) in lxc/incus#1463
-   incus/file/push Use SFTP client instead of file API by [@&#8203;HassanAlsamahi](https://github.com/HassanAlsamahi) in lxc/incus#1468
-   Fix TPM fd leaks and OpenFGA patching issue by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1469
-   Clarify device override syntax by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1471
-   incusd/auth/openfga: refresh model before applying patches by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1472
-   Add authorization scriptlet by [@&#8203;bensmrs](https://github.com/bensmrs) in lxc/incus#1412
-   doc: add openSUSE installation instructions by [@&#8203;cyphar](https://github.com/cyphar) in lxc/incus#1475
-   OCI image debugging improvements by [@&#8203;danbiagini](https://github.com/danbiagini) in lxc/incus#1478
-   Add function checks to scriptlet validation by [@&#8203;bensmrs](https://github.com/bensmrs) in lxc/incus#1484
-   incus/project: Fix handling of default (unset) project in `get-current` by [@&#8203;irhndt](https://github.com/irhndt) in lxc/incus#1476
-   Translations update from Hosted Weblate by [@&#8203;weblate](https://github.com/weblate) in lxc/incus#1492
-   Add `--force` flag to the console command by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1491
-   Accept io.Writer in RenderTable by [@&#8203;breml](https://github.com/breml) in lxc/incus#1490
-   doc/network_bridge: Fix missing escaping around variable by [@&#8203;irhndt](https://github.com/irhndt) in lxc/incus#1493
-   incusd/cluster: Skip project restrictions during join by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1497
-   incusd/instance/lxc: Skip instances without idmap allocation yet by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1495
-   incusd/storage/drivers/common: Truncate/Discard ahead of sparse write by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1496
-   Add AskPassword/AskPasswordOnce to Asker by [@&#8203;breml](https://github.com/breml) in lxc/incus#1499
-   Add additional check to Cancel method for ConsoleShow operation by [@&#8203;presztak](https://github.com/presztak) in lxc/incus#1500
-   Improve console disconnections by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1501
-   Fix duplicate OVN load-balancer entries by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1502
-   Improve SFTP performance by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1503
-   incusd/instance_post: Expand profiles in scriptlet context by [@&#8203;stgraber](https://github.com/stgraber) in lxc/incus#1504

#### New Contributors

-   [@&#8203;stefanor](https://github.com/stefanor) made their first contribution in lxc/incus#1380
-   [@&#8203;brauner](https://github.com/brauner) made their first contribution in lxc/incus#1398
-   [@&#8203;cyphar](https://github.com/cyphar) made their first contribution in lxc/incus#1384
-   [@&#8203;breml](https://github.com/breml) made their first contribution in lxc/incus#1401
-   [@&#8203;danbiagini](https://github.com/danbiagini) made their first contribution in lxc/incus#1478
-   [@&#8203;irhndt](https://github.com/irhndt) made their first contribution in lxc/incus#1476

**Full Changelog**: lxc/incus@v6.7.0...v6.8.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS42Mi42IiwidXBkYXRlZEluVmVyIjoiMzkuNjIuNiIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
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