nixos/postgresql: add ensureUsers.*.passwordFile option

[Atemu]

Previously you had to roll your own systemd service that runs some SQL and risk inadvertently exposing your password to the Nix store by doing it wrong.

This provides a standard method to set up user passwords via password files which can safely be deployed via secrets management solutions for those who care about security or alternatively also simply generated for those who don't.

Adapted from https://discourse.nixos.org/t/assign-password-to-postgres-user-declaratively/9726/3

Description of changes

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:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/assign-password-to-postgres-user-declaratively/9726/4

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/set-password-for-a-postgresql-user-from-a-file-agenix/41377/16

[TLATER]

Can't believe none of us in the thread saw the obvious need for this.

Looks great, thank you so much @Atemu

Edit: Hm, maybe I spoke too soon, is there a full postgres restart on activation so the postStart has a chance to trigger? If not this could end up leading to inonsistencies with other services, making a switch somewhat annoying. Not a hard blocker, but it'd be better if that just worked.

[Atemu]

This should have the same semantics w.r.t. restarts as the rest of the user settings.

[Ma27]

I'm not sure if I like this.
The whole ensure* thing is an antipattern anyways and has caused pain for the maintainers for the last two releases which is why it's now kept super minimal.

[Ma27]

I still think that types.path for secrets is a huge footgun. As soon as somebody does ${foo} rather than toString foo in a subsequent change somewhere, the file will be written into the store without a warning.

[Atemu]

Indeed. Would apply = toString remedy this?

[Atemu]

I tested this a little and it does at least cover trivial cases. #78640 would likely be a more complete solution but this works in the meantime.

[Ma27]

Personally, I don't think it's a good idea to use a syntax construct with the intention of copying sources into the store to specify secrets. At least that's my impression given its behavior.

Does your change solve the core problem I have in mind? I think so, yes.

However, I have the impression that it takes a while until people are aware of the semantics of the path type and until then they easily run into errors like this. It may be implemented correctly here, but there are still enough other wrongly implemented options out there where this is a footgun. And that's understandable given that we work against the semantics of the path-type here IMHO.

The only benefit I see by using the path-type is that we get some kind of validation for free[1] from the Nix parser when using the literal path syntax (the variant of the path-type that I have an issue with to be clear). Personally, I think it's much better to force a string there and then do a check = x: builtins.substring 0 1 (toString x) == "/" as it's the case for the string-case in types.path.

Is there any other benefit you have in mind?

[1] and normalization of relative paths to absolute ones, but I'd argue that this isn't used that much for cases like this.

[Atemu]

I don't think it's a good idea to use a syntax construct with the intention of copying sources into the store to specify secrets.

Could you elaborate on what you mean by this?

There is no intention to copy the secret to the Nix store here whatsoever; exactly the opposite is the case. The apply prevents having the specified path being brought into the Nix store even if you declare it as a path literal which would otherwise be a shot in the foot.

There is likely a way you could still break this but it at least covers the trivial case.

It may be implemented correctly here, but there are still enough other wrongly implemented options out there where this is a footgun.

I'd love a more generic solution but it simply doesn't exist yet and there doesn't appear to have been any progress on that in the past few years.

The only benefit I see by using the path-type is that we get some kind of validation for free[1] from the Nix parser when using the literal path syntax (the variant of the path-type that I have an issue with to be clear). Personally, I think it's much better to force a string there and then do a check = x: builtins.substring 0 1 (toString x) == "/" as it's the case for the string-case in types.path.

Is there any other benefit you have in mind?

Nope, merely that the value passed should look like a path. types.path is little more than that check anyways and having it be a types.str would cause a type error if a path was passed which would be an even better behaviour.

[TLATER]

Is there a reason there is no types.secret that's just an alias to types.str with that substring check (and maybe some way to force allow a store path for tests)? It'd be clearer for users and also avoid this problem.

A bit besides the point though, is there an issue this could be spun out into instead of blocking the PR on it? Not that this isn't valuable discussion, but given it's a widespread problem 1) it should be solved generally and 2) the audience here is far smaller than it should be.

[Ma27]

Could you elaborate on what you mean by this?

The path literal - that's allowed by types.path - is something being used to copy stuff into the store. At least that's what its semantics are for.

This syntax is allowed by types.path and I think it's wrong to accept this in any case.

Nope, merely that the value passed should look like a path. types.path is little more than that check anyways and having it be a types.str would cause a type error if a path was passed which would be an even better behaviour.

OK, we're aligned on this I think (given the last change you pushed).

A bit besides the point though, is there an issue this could be spun out into instead of blocking the PR on it? Not that this isn't valuable discussion, but given it's a widespread problem 1) it should be solved generally and 2) the audience here is far smaller than it should be.

Fair.
Would you be motivated to do so? I may be able to get to it in the upcoming days otherwise.

[TLATER]

Would you be motivated to do so? I may be able to get to it in the upcoming days otherwise.

I'll give it a shot! Should be the kind of change to start with a PR.

[Atemu]

The problem is that there is a need for declarative user management for postgres and it if there isn't an option in the postgres module, it will be made to happen elsewhere and I think that's worse than having it happen in here.

[Ma27]

The problem is that there is a need for declarative user management for postgres and it if there isn't an option in the postgres module, it will be made to happen elsewhere and I think that's worse than having it happen in here.

Personally, I think that if there's a strong need for that, it should be invested into proper tooling. See for instance the discussion in #206467 (comment) / #237838

The problem is that this is not only "unclean", but also had an increased maintenance effort during the last two release cycles.

[Atemu]

See for instance the discussion in #206467 (comment) / #237838

That discussion seems more concerned with whether the ensure* pattern itself is a good idea and I largely agree with the points made. I wouldn't mind if postgres users were stateless just like unix users; that'd be great actually. I never had a need for (semi-) stateful users in either case.

What I don't really see is the relation to this PR because what's added here would work the same whether users are semi-stateful or stateless and would be trivial to migrate to the stateless version if it existed.

Even if it had a direct relation, again, the alternative would be that people would implement this functionality in a decentral/disorganised manner and that'd inherit the same issues of semi-statefulness and adds a few more on top.

The problem is that this is not only "unclean", but also had an increased maintenance effort during the last two release cycles.

Could you describe the ways in which your forsee this option to increase maintenance overhead? I have never been involved in the maintenance of the postgres module and therefore cannot reasonably judge this aspect.

[Ma27]

Could you describe the ways in which your forsee this option to increase maintenance overhead? I have never been involved in the maintenance of the postgres module and therefore cannot reasonably judge this aspect.

• 23.11: giving all privileges to a user that isn't the owner of a database did not include the CREATE permission on the public schema anymore. In other words: users created with ensureUsers/ensurePermissions on fresh postgresql15 installs couldn't do CREATE TABLE causing migration scripts to fail (nixos/postgresql: deprecate ensurePermissions, fix ensureUsers for postgresql15 #266270) which was rushed into 23.11 with the help of @RaitoBezarius .
• 24.05: we discovered that glibc updates (causing collation updates) broke ensureDatabases: you couldn't do CREATE TABLE anymore if template1 had a collation mismatch (postgres fails to start when template database with collation version mismatch is needed at startup #318777). Yes, its status is wontfix/cantfix, but it still cost maintainer time.

That discussion seems more concerned with whether the ensure* pattern itself is a good idea and I largely agree with the points made. I wouldn't mind if postgres users were stateless just like unix users; that'd be great actually. I never had a need for (semi-) stateful users in either case.

The former comment describes an approach I'd prefer to see rather than expanding on the convergent approach.