Skip to content

nixos/authentik: init module, authentik: 2024.6.4 -> 2024.8.3 - #345940

Closed
jvanbruegge wants to merge 11 commits into
NixOS:masterfrom
jvanbruegge:authentik-2024.8
Closed

nixos/authentik: init module, authentik: 2024.6.4 -> 2024.8.3#345940
jvanbruegge wants to merge 11 commits into
NixOS:masterfrom
jvanbruegge:authentik-2024.8

Conversation

@jvanbruegge

Copy link
Copy Markdown
Contributor

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@jvanbruegge
jvanbruegge requested a review from natsukium as a code owner October 2, 2024 14:38
@github-actions github-actions Bot added 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 2, 2024
@jvanbruegge
jvanbruegge force-pushed the authentik-2024.8 branch 4 times, most recently from 95d79aa to abdb734 Compare October 2, 2024 14:54

@tmarkov tmarkov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice, I've actually been thinking to try out authenik, so maybe this module will push me to do it :D.

You've left some immich in the docs.

Also, I notice that cfg.user if not used for the authentik systemd service, instead it's "authentik" there.

Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
@ofborg ofborg Bot added the 8.has: package (new) This PR adds a new package label Oct 2, 2024
@ofborg
ofborg Bot requested review from jopejoe1, rissson and s1341 October 2, 2024 17:02
@ofborg ofborg Bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels Oct 2, 2024
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment on lines 120 to 147

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be like the DB, with a specific option for local provisioning?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment on lines 30 to 51

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand the comment. Looking at the repo, it looks like it should use an in-memory SQLite DB by default?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, but the tests require pynose and I tried for two hours to get them to work with pytest and had to give up now

Comment thread pkgs/development/python-modules/drf-orjson-renderer/default.nix Outdated
@h7x4 h7x4 added 8.has: module (new) This PR adds a module in `nixos/` 8.has: tests This PR has tests labels Oct 2, 2024
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/doc/manual/release-notes/rl-2411.section.md Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/tests/web-apps/authentik.nix Outdated
Comment thread pkgs/by-name/au/authentik/outposts.nix Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any benefit to adding support for running outposts as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Long term yes, but I want to do this in a later PR

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Fair enough

@rissson

rissson commented Oct 3, 2024

Copy link
Copy Markdown
Member

On another note, I'm not sure if the license should be left as MIT, since authentik is using a custom license, especially for source-available enterprise code: https://github.com/goauthentik/authentik/blob/main/LICENSE

@Ma27

Ma27 commented Oct 3, 2024

Copy link
Copy Markdown
Member

Did you take a look at the module from https://github.com/nix-community/authentik-nix/?
It contains a few nice things like the ak script or a media upload thing that seem to be missing here on a first look.

@jvanbruegge
jvanbruegge force-pushed the authentik-2024.8 branch 2 times, most recently from 3d7beed to 2c94185 Compare October 3, 2024 14:22
@jvanbruegge

Copy link
Copy Markdown
Contributor Author

On another note, I'm not sure if the license should be left as MIT

As far as I know we do not use the enterprise code, the normal build does not include it

@jvanbruegge

Copy link
Copy Markdown
Contributor Author

Did you take a look at the module from https://github.com/nix-community/authentik-nix/?

No, I did not know that exists. I more or less upstreamed the module I've been using in the past year

@ofborg
ofborg Bot requested a review from rissson October 3, 2024 15:58

@SuperSandro2000 SuperSandro2000 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could merge the package updates and new packages with another PR to speed things up and make this PR a little smaller.

Comment thread pkgs/development/python-modules/tenant-schemas-celery/default.nix Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use fetchpypi or download the source from github

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is no tag or release (or even commit) for this version on github and fetchpypi can also not download it

Comment thread pkgs/development/python-modules/django-tenants/default.nix Outdated
Comment thread pkgs/development/python-modules/django-pglock/default.nix Outdated
Comment thread pkgs/development/python-modules/django-pgactivity/default.nix Outdated
Comment thread pkgs/by-name/au/authentik/outposts.nix Outdated
Comment thread pkgs/by-name/au/authentik/ldap.nix Outdated
Comment thread pkgs/by-name/au/authentik/ldap.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated
Comment thread nixos/modules/services/security/authentik.nix Outdated

@Ma27 Ma27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No, I did not know that exists. I more or less upstreamed the module I've been using in the past year

I must say, I'm not too happy about a different module being added to nixpkgs now.

The stuff in authentik-nix got testing by quite a bunch of people and has more (already tested) features already such as

  • support for LDAP & RADIUS outpost
  • units configured to work with media uploads
  • a wrapper for the ak script (useful for a few maintenance tasks)
  • A more extensive VM test verifying the entire setup process.

I fail to understand why we're trying to get an implementation in with less features when there's a community-tested version ready with more features integrated. I think it would be way better to add the entire module from there into nixpkgs or re-use the parts from it here (with credit to the original authors ideally).

Comment thread nixos/modules/services/security/authentik.nix Outdated
@ofborg ofborg Bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Oct 5, 2024
LeSuisse added a commit to LeSuisse/nixpkgs that referenced this pull request Dec 3, 2024
The bump to 2024.10.x is currently stalled in NixOS#345940.

The fix for CVE-2024-52289 involves a backward incompatible
API and DB change so it is also not great for a backport even
if we cherry-picks the security fixes.

Given no NixOS module is available in nixpkgs marking the package
with `knownVulnerabilities` should have a limited impact.
nixpkgs-ci Bot pushed a commit that referenced this pull request Dec 4, 2024
The bump to 2024.10.x is currently stalled in #345940.

The fix for CVE-2024-52289 involves a backward incompatible
API and DB change so it is also not great for a backport even
if we cherry-picks the security fixes.

Given no NixOS module is available in nixpkgs marking the package
with `knownVulnerabilities` should have a limited impact.

(cherry picked from commit 384f6f5)
LeSuisse added a commit to LeSuisse/nixpkgs that referenced this pull request Dec 4, 2024
The bump to 2024.10.x is currently stalled in NixOS#345940.

The fix for CVE-2024-52289 involves a backward incompatible
API and DB change so it is also not great for a backport even
if we cherry-picks the security fixes.

Given no NixOS module is available in nixpkgs marking the package
with `knownVulnerabilities` should have a limited impact.

(cherry picked from commit 384f6f5)
@MarcelCoding

Copy link
Copy Markdown
Member

I agree with @Ma27 we should try to at least include the features of the out of tree flake. Ideally with the same (or close enough) implementation to have something "battle" tested.

Despite the work on this module, I would appreciate splitting the update of the packages into a separate PR.

@jvanbruegge

Copy link
Copy Markdown
Contributor Author

Ok, let's split this. I've opened a new PR for the version update alone here: #363930

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants