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.

Sign up for GitHub

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

nixos/postgresql: add ensureUsers.*.passwordFile option #326306

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

nixos/postgresql: add ensureUsers.*.passwordFile option #326306

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e2a0431
nixos/postgresql: add ensureUsers.*.passwordFile option
Atemu Jul 11, 2024
3757773
nixos/postgresql: mitigate adding the passwordFile to the Nix store
Atemu Jul 19, 2024
c56832f
nixos/postgresql: enforce passing the path as a string
Atemu Jul 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions nixos/modules/services/databases/postgresql.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,19 @@ in
'';
};

passwordFile = mkOption {
type = with lib.types; nullOr path;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed. Would apply = toString remedy this?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

example = "/run/keys/db_user_pw";
default = null;
description = ''
The path to a file containing a password that is to be set as the user's PASSWORD field.

A path must be given because Nix strings would be exposed in Nix store through the .drv files.

Note that the contents of this file are stripped of newlines and that surrounding whitespace is trimmed.
'';
};

ensureClauses = mkOption {
description = ''
An attrset of clauses to grant to the user. Under the hood this uses the
Expand Down Expand Up @@ -586,13 +599,27 @@ in
user.ensureDBOwnership
''$PSQL -tAc 'ALTER DATABASE "${user.name}" OWNER TO "${user.name}";' '';

setPW =
pkgs.writeText
"set-pw.sql"
# You can't use prepared statements for ALTER ROLE, we must wrap it in a procedure
''
DO $$
DECLARE password TEXT;
BEGIN
password := trim(both from replace(pg_read_file('${user.passwordFile}'), E'\n', '''));
EXECUTE 'ALTER ROLE "${user.name}" WITH PASSWORD' || quote_literal(password);
END $$;
'';

filteredClauses = filterAttrs (name: value: value != null) user.ensureClauses;

clauseSqlStatements = attrValues (mapAttrs (n: v: if v then n else "no${n}") filteredClauses);

userClauses = ''$PSQL -tAc 'ALTER ROLE "${user.name}" ${concatStringsSep " " clauseSqlStatements}' '';
in ''
$PSQL -tAc "SELECT 1 FROM pg_roles WHERE rolname='${user.name}'" | grep -q 1 || $PSQL -tAc 'CREATE USER "${user.name}"'
${lib.optionalString (user.passwordFile != null) "$PSQL -tAf ${setPW}"}
${userClauses}

${dbOwnershipStmt}
Expand Down
17 changes: 17 additions & 0 deletions nixos/tests/postgresql.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,15 @@ let
services.postgresql = {
enable = true;
package = postgresql-package;
enableTCPIP = true;
ensureDatabases = [ "pw-user" ];
ensureUsers = [
{
name = "pw-user";
passwordFile = pkgs.writeText "password" "password";
ensureDBOwnership = true;
}
];
};

services.postgresqlBackup = {
Expand Down Expand Up @@ -71,6 +80,14 @@ let
machine.fail(check_count("SELECT * FROM sth;", 4))
machine.succeed(check_count("SELECT xpath('/test/text()', doc) FROM xmltest;", 1))

with subtest("Connection with user password works correctly"):
machine.succeed(
"sudo -u postgres PGPASSWORD=password psql -h localhost -U pw-user -tc '\c'"
)
machine.fail(
"sudo -u postgres PGPASSWORD=wrong-password psql -h localhost -U pw-user -tc '\c'"
)

with subtest("Backup service works"):
machine.succeed(
"systemctl start ${backupService}.service",
Expand Down