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

authelia: 4.37.5 -> 4.38.9 #299309

Merged
merged 3 commits into from
Jul 17, 2024
Merged

authelia: 4.37.5 -> 4.38.9 #299309

merged 3 commits into from
Jul 17, 2024

Conversation

nicomem
Copy link
Contributor

@nicomem nicomem commented Mar 26, 2024

Description of changes

Closes #296502

Package Update

Updated manually using the update.sh script (but as said in #296502, the nixpkgs-update bot will probably not be able to automatically update it).

The 4.38.0 version has some big changes and some configuration depreciation, but simply by keeping my old configuration, everything seems to work fine (apart from the depreciation warnings).

Changelogs:
https://github.com/authelia/authelia/releases/tag/v4.38.0
https://github.com/authelia/authelia/releases/tag/v4.38.1
https://github.com/authelia/authelia/releases/tag/v4.38.2
https://github.com/authelia/authelia/releases/tag/v4.38.3
https://github.com/authelia/authelia/releases/tag/v4.38.4
https://github.com/authelia/authelia/releases/tag/v4.38.5
https://github.com/authelia/authelia/releases/tag/v4.38.6
https://github.com/authelia/authelia/releases/tag/v4.38.7
https://github.com/authelia/authelia/releases/tag/v4.38.8
https://github.com/authelia/authelia/releases/tag/v4.38.9

Module Update

Removals

  • Removed options services.authelia.instances.<name>.settings.server.{host,port}, and added services.authelia.instances.<name>.settings.server.address.

If any of the former options are used, an assertion will be triggered.

Other changes

  • services.authelia.instances.<name>.secrets.jwtSecretFile now sets the AUTHELIA_IDENTITY_VALIDATION_RESET_PASSWORD_JWT_SECRET_FILE environment variable instead of AUTHELIA_JWT_SECRET_FILE
  • services.authelia.instances.<name>.secrets.oidcIssuerPrivateKeyFile now creates a new YAML config file to set identity_providers.oidc.key using templates

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@nicomem
Copy link
Contributor Author

nicomem commented Mar 28, 2024

Notify maintainers : @06kellyjac @RaitoBezarius @dit7ya

@GGG-KILLER
Copy link
Contributor

Gave a try setting this up and the module will need reworking for people who want to take advantage of unix sockets

@nicomem nicomem changed the title authelia: 4.37.5 -> 4.38.6 authelia: 4.37.5 -> 4.38.7 Apr 2, 2024
@b12f
Copy link
Contributor

b12f commented Apr 3, 2024

The service config will have to be updated. Using server.address , the new and recommended way to set the listening address, crashes Authelia. This is because the nix options explicitly declare server.host and server.port, and declaring both is invalid.

@nicomem
Copy link
Contributor Author

nicomem commented Apr 4, 2024

Gave a try at updating the module (I do not have much experience on that), and have currently the following to still have the host+port settings working but transformed into the address (for backwards compatibility):

# In autheliaOpts, remove server.host & server.port and add server.address

imports =
    # settings.server.address should be set instead of host + port
    (map
      (name:
        let
          serverConfigPath = [ "services" "authelia" "instances" name "settings" "server" ];
          hostConfigPath = serverConfigPath ++ [ "host" ];
          portConfigPath = serverConfigPath ++ [ "port" ];
          addressConfigPath = serverConfigPath ++ [ "address" ];
        in
        lib.mkMergedOptionModule
          [
            hostConfigPath
            portConfigPath
          ]
          addressConfigPath
          (config:
            let
              host = lib.attrByPath hostConfigPath "" config;
              port = lib.attrByPath portConfigPath 9091 config;
            in
            "tcp://${host}:${port}/"
          )
      )
      (lib.attrNames cfg.instances));

However, this has two problems:

  • nix encounters an infinite recursion at the map (...) (lib.attrNames cfg.instances)
  • And if I test with a single instance name, I get the following error:
    The option `services.authelia.instances' in module `<...>/security/authelia.nix' would be a parent of the following options, but its type `attribute set of (submodule)' does not support nested options.
          - option(s) with prefix `services.authelia.instances.main' in module `<...>/security/authelia.nix'
    
    Which I assume is due to mkMergedOptionModule trying to define the old options but clashes with the usage of type = types.submodule { ... } which defines a specific attribute set

If someone has any idea on how to fix this or a better way of doing things, feel free to share your thoughts.

@b12f
Copy link
Contributor

b12f commented Apr 5, 2024

To make life simple on yourself, I would just deprecate the config option and show the user where to find the new one. You can use lib.mkRemovedModuleOption for this: https://github.com/NixOS/nixpkgs/blob/master/lib/modules.nix#L1091

@nicomem
Copy link
Contributor Author

nicomem commented Apr 6, 2024

lib.mkRemovedModuleOption did not work for the same reasons as lib.mkMergedOptionModule, so I instead added an assertion checking if either option was set

@nicomem nicomem force-pushed the authelia-4.38 branch 3 times, most recently from a525c37 to 4753af3 Compare April 9, 2024 19:07
@nicomem
Copy link
Contributor Author

nicomem commented Apr 9, 2024

Updated again the module due to other options having problems with the new settings:

  • secrets.jwtSecretFile was mapped to the AUTHELIA_JWT_SECRET_FILE env variable

    • It is now mapped to AUTHELIA_IDENTITY_VALIDATION_RESET_PASSWORD_JWT_SECRET_FILE (the newer setting)
  • secrets.oidcIssuerPrivateKeyFile has been removed

    • Authelia now does not allow setting this using env variable, but by setting identity_providers.oidc.jwks.key (jwks being an array)
    • Authelia advises using a file filter as such:
      identity_providers:
        oidc:
          jwks:
            - key: {{ secret "/config/jwks/rsa.2048.pem" | mindent 10 "|" | msquote }}
    • However, this way of doing does not seem to work with the Nix -> YAML conversion
    • So the most secure way I found of setting this is by using services.authelia.<name>.settingsFiles, and using a file that is only readable by the authelia user (for example, in my conf, I use agenix)

Updated the manual & added an assertion in the same way as the previously removed options

@nicomem nicomem force-pushed the authelia-4.38 branch 3 times, most recently from 430aa2a to 1a815cf Compare April 15, 2024 16:01
@nicomem nicomem changed the title authelia: 4.37.5 -> 4.38.7 authelia: 4.37.5 -> 4.38.8 Apr 15, 2024
@nicomem
Copy link
Contributor Author

nicomem commented Apr 15, 2024

Added a commit to add myself as a maintainer.

Re-pinging the maintainers : @06kellyjac @dit7ya

In case the maintainers still do not respond, also adding @AndersonTorres who merged the init PR (#180469), and @marsam who merged the last authelia PR (#266455)

@nicomem
Copy link
Contributor Author

nicomem commented Apr 15, 2024

Tried to see if we could avoid vendoring the package-lock.json file, as the Authelia repo has a pnpm-lock.yaml, but it seems like there is not yet the required tooling for that (see #231513)

@nicomem
Copy link
Contributor Author

nicomem commented Jun 21, 2024

The solution seems to work well! I added this modification to the PR

@nicomem
Copy link
Contributor Author

nicomem commented Jun 22, 2024

Changes: depreciates -> deprecates

@nicomem nicomem requested a review from bendlas June 22, 2024 18:18
Copy link
Contributor

@bendlas bendlas left a comment

Choose a reason for hiding this comment

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

Found more problems during testing: error occurred performing deprecation mapping for keys 'server.host', 'server.port', and 'server.path' to new key server.address: the new key already exists with value 'unix:///run/authelia/http' but the deprecated keys and the new key can't both be configured

  • According to the error message, the address option not just supersedes host and port, but also the path option.

  • I am using the the path configuration option. Right now, I'm not sure how to combine that into the address option, together with a unix socket path. This is not a problem to necessarily be solved as part of this PR - I will ask about it upstream - but it is an example for why it's preferrable to not put additional restrictions on what's accepted upstream. EDIT: found out, you can pass a ?path=... argument to the unix socket address.

Second Error: identity_providers: oidc: option jwks must not be configured at the same time as 'issuer_private_key' or 'issuer_certificate_chain' EDIT: solved it. I could update my cert chain config. Will contribute this.

  • This one looks, like it should be fixed in my config. I'll check on how to do that.

nixos/modules/services/security/authelia.nix Outdated Show resolved Hide resolved
@bendlas
Copy link
Contributor

bendlas commented Jun 24, 2024

So, I managed to get everything running with this PR now, but I feel like the assertion doesn't cut it. To summarize:

  • Using address also precludes using path: This should be added to the assertion / warning logic
  • Doing an assertion precludes using the still valid (if deprecated) options: I recommend downgrading to a warning, or removing the check outright (since we won't be validating all the authelia options anyway)

@nicomem
Copy link
Contributor Author

nicomem commented Jun 28, 2024

@bendlas I removed the assertion because as you said, we won't validate all the options.

Another thing that I thought is about the default value of the server.address setting:

  • Defining a default value in the module will break the config of those that are defining host, port or path
  • But most NixOS modules define a default value for the host+port/address ; and when the deprecated settings will be outright removed, the config will stop to work in any case

@nicomem nicomem requested a review from bendlas June 28, 2024 16:34
Copy link
Contributor

@bendlas bendlas left a comment

Choose a reason for hiding this comment

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

Excellent!

default = "localhost";
example = "0.0.0.0";
default = "tcp://:9091/";
example = "unix:///var/run/authelia.sock";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
example = "unix:///var/run/authelia.sock";
example = "unix:///var/run/authelia.sock?path=authelia&umask=0117";

this is how I'm running it. I think that's useful as an example, because it shows how to do socket permissions, and in particular, how to translate the path setting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried and did not manage to make it working.
Do you have a complete authelia config example available on github ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modification applied

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried and did not manage to make it working. Do you have a complete authelia config example available on github ?

What are you having your issue with @hatch01?

Have you had a working configuration with authelia on a subpath before?

I do have a working configuration. It's part of a larger approach, that I'm looking at open-sourcing real soon (tm). If you'd be interested in having a look at that, possibly helping out, I'm happy to share privately. Feel free to DM.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll reply in this thread, in order to avoid spamming the main thread more ...

I can obviously write in the file using sudo -u authelia echo test > /var/log/authelia/authelia.log and read it in the same way.

I think the usual way to do this within a nixos module is systemd.tmpfiles.rules.

If that still doesn't work let's take this elsewhere and let's try to keep this PR focussed on the out-of-box config + stuff that was verified to work before, please.

type = types.str;
default = "localhost";
example = "0.0.0.0";
default = "tcp://:9091/";
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Defining a default value in the module will break the config of those that are defining host, port or path

That's an excellent point.

Suggested change
default = "tcp://:9091/";
default = if not isNull cfg.settings.host or cfg.settings.port or cfg.settings.path or null then warn "Please replace services.authelia.setings.{host,port,path} with services.authelia.settings.address, before release 4.39" null else "tcp://:9091/";

WARNING untested and probably not working

Maybe something like this could smooth over the transition ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not find a way to make this work:

The first problem is that there can be multiple authelia instances, some using the old settings, and others using the new one -> The default value being applied to all, there will be problems.

If we suppose that there is only one instance, after fixing the condition (the settings are accessed with cfg.instances.<name>.settings.server.{host,port,path}), this still generates the server.address setting in the YAML file, with the value null.

I tried to instead conditionnaly set the default value by doing this:

address = mkOption ({
    type = types.str;

    example = "unix:///var/run/authelia.sock";
    description = "The address to listen on.";
  } // lib.optionalAttrs
    (
      let
        instance0 = builtins.elemAt (builtins.attrValues cfg.instances) 0;
      in
      if (instance0.settings.server.host != null) ||
        (instance0.settings.server.port != null) ||
        (instance0.settings.server.path != null) then
        warn "Please replace services.authelia.setings.{host,port,path} with services.authelia.settings.address, before release 4.39" false
      else true
    )
    {
      default = "tcp://:9091/";
    });
};

, but nix encounters an infinite recursion...

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, yeah, I think the infinite recursion comes from builtins.elemAt and/or builtins.attrValues being eager.

Also it seems like that snippet would set the default for each authelia instance, based on the setting of the (arbitrary) first one. I don't think, that's what we want to be doing.

Try taking a config argument in the function for autheliaOpts, see comment below.

@hatch01
Copy link
Contributor

hatch01 commented Jul 7, 2024

Hi, are you using the log.file_path parameter.
Because in my installation, setting it to "/var/log/authelia/authelia.log" just make authelia crashing:

juil. 07 16:17:57 nixos authelia[18380]: time="2024-07-07T16:17:57+02:00" level=fatal msg="Cannot configure logger: open /var/log/authelia/authelia.log: read-only file system" stack="github.com/authelia/authelia/v4/internal/commands/root.go:69 (*CmdCtx).RootRunE\ngithub.com/spf13/cobra@v1.8.1/command.go:985                 (*Command).execute\ngithub.com/spf13/cobra@v1.8.1/command.go:1117                (*Command).ExecuteC\ngithub.com/spf13/cobra@v1.8.1/command.go:1041                (*Command).Execute\ngithub.com/authelia/authelia/v4/cmd/authelia/main.go:10      main\nruntime/internal/atomic/types.go:194                         (*Uint32).Load\nruntime/asm_amd64.s:1695                                     goexit"

I don't know if it is caused by the update or if it was broken before.

Ps:
I can obviously write in the file using sudo -u authelia echo test > /var/log/authelia/authelia.log and read it in the same way.

@nicomem
Copy link
Contributor Author

nicomem commented Jul 7, 2024

@hatch01

Hi, are you using the log.file_path parameter.
Because in my installation, setting it to "/var/log/authelia/authelia.log" just make authelia crashing:

The authelia instance services.authelia.<name> declares in its systemd settings the state directory /var/lib/authelia-<name> (and not /var/lib/authelia). Changing the log.file_path to point to the correct directory should probably solve your problem.

@hatch01
Copy link
Contributor

hatch01 commented Jul 7, 2024

@hatch01

Hi, are you using the log.file_path parameter.
Because in my installation, setting it to "/var/log/authelia/authelia.log" just make authelia crashing:

The authelia instance services.authelia.<name> declares in its systemd settings the state directory /var/lib/authelia-<name> (and not /var/lib/authelia). Changing the log.file_path to point to the correct directory should probably solve your problem.

Sorry I forgot to precise my config : https://github.com/hatch01/server-flake/blob/354a795dff56383e466ee414652a0e2441ccf2f3/apps/authelia.nix
I already set file_path which is the settings which causes the problem.

@nicomem
Copy link
Contributor Author

nicomem commented Jul 7, 2024

Sorry if my explanation was not clear, but in your config, you are setting the log.file_path to a file in the /var/lib/authelia directory whereas the systemd service uses /var/lib/authelia-<name> (which in your case would be authelia-main).

Try setting log.file_path to /var/lib/authelia-${autheliaInstance}/authelia.log.

If for some reason you still want/need to use the /var/lib/authelia directory, you will probably need to change the service's systemd settings to allow read-write access to it.

@@ -8,7 +8,6 @@ let
cfg = config.services.authelia;

format = pkgs.formats.yaml { };
configFile = format.generate "config.yml" cfg.settings;

autheliaOpts = with lib; { name, ... }: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
autheliaOpts = with lib; { name, ... }: {
autheliaOpts = with lib; { name, config, ... }: {

This config will be scoped to per-instance, because autheliaOpts is passed to types.submodule.

That way, it should be possible to do warnings per affected instance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using config here allows for accessing the instance configuration (thank you for the tip), however I still get the infinite recursion :/

I think that it is because I try to lazily define settings (i.e. the address option of the submodule settings) while trying to access the "final state" of settings (i.e. the host/port/path).

When I set the condition to something outside settings (e.g. config.enable), it works.
It is when I try to use something in settings that it breaks.

Also, when using eager functions (e.g. lib.optionalString), it compiles, but the key is still defined in the YAML configuration file (address: ''), and Authelia yells about defining both old and new keys.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm :/ I'd have to take a look to say more, but I guess last resort would be a top-level mapAttrsToList, to generate config.warnings manually ....

@winterqt winterqt mentioned this pull request Jul 10, 2024
13 tasks
@dimakow
Copy link

dimakow commented Jul 13, 2024

Hi folks,

may I ask if you have any ETA in mind for this PR ?

Regards

- Remove settings.server.{host,port} options
  - Replaced by settings.server.address
  - If any of settings.server.{host,port,path} are specified in the
    configuration, a warning is displayed and these values will be used
    instead of settings.server.address

- Change what secrets.oidcIssuerPrivateKeyFile maps to
  - Previously: AUTHELIA_IDENTITY_PROVIDERS_OIDC_ISSUER_PRIVATE_KEY_FILE
  - Now: identity_providers.oidc.jwks[0].key
    - Not done directly in the NixOS settings config but as a separate
      YAML config file
    - Done that way because Go templates are not correctly handled by
      the YAML generator (NixOS#319716)

- Change secrets.jwtSecretFile env variable mapping
  - Previously: AUTHELIA_JWT_SECRET_FILE
  - Now: AUTHELIA_IDENTITY_VALIDATION_RESET_PASSWORD_JWT_SECRET_FILE
@nicomem
Copy link
Contributor Author

nicomem commented Jul 13, 2024

Finally found a way to make the default value of address work with host/port/path!!!

Instead of trying to modify whether we set the default value of address depending on whether the others have been set, we can instead take the instance.settings attribute set and remove the address value if the others have been set, before passing it to the YAML formatter.

This seems to work well:

  • Setting any of the old settings displays a warning, and address does not appear in the config file
    • The warning is per-instance (i.e. only the instances that use the old settings will trigger a warning, one for each)
    • The warning indicates which instance defines the old settings (e.g. Please replace services.authelia.instances.main.settings.{host,port,path} with services.authelia.instances.main.settings.address, before release 5.0.0)
  • Not specifying the old settings, and either setting or leaving as default address does not display any warning, and the value is set in the config file

This now means that this PR now does not bring any breaking change!

Thanks again @bendlas for all the comments and ideas to make this work

@bendlas
Copy link
Contributor

bendlas commented Jul 17, 2024

Tested and works. Thanks @nicomem, for all your work and being receptive to feedback!

I would merge this right now, except it feels like this should be something done by package maintainers. @06kellyjac and @dit7ya don't seem to have participated so far. Let's wait for the weekend, if they get around, otherwise I'll merge.

may I ask if you have any ETA in mind for this PR ?

@dimakow Generally, there are no ETAs, as NixOS is volunteer work. A more productive way to ask something like this, is to pick the PR onto your branch, try it out, and provide feedback based on your experience, or simply ask if there is any way to help move this along.

Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

image

image

I've not seen it necessary to repeat valid suggestions already posted on quite an active PR.

I've not requested the commit-bit for nixpkgs so I can't merge.

LGTM I don't mind merging now or waiting for @dit7ya

@bendlas
Copy link
Contributor

bendlas commented Jul 17, 2024

@06kellyjac my bad, sorry. Thanks for approving. I'll merge.

@bendlas bendlas merged commit 98d46bc into NixOS:master Jul 17, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: authelia 4.37.5 → 4.38.2