Package: guix-patches;
Reported by: Giacomo Leidi <goodoldpaul <at> autistici.org>
Date: Thu, 12 Sep 2024 11:26:01 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #44 received at 73196 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Giacomo Leidi <goodoldpaul <at> autistici.org> Cc: 73196 <at> debbugs.gnu.org, Ludovic Courtès <ludo <at> gnu.org> Subject: Re: bug#73196: [PATCH] services: postgresql-role: Add support for password files. Date: Fri, 25 Apr 2025 22:27:27 +0900
Hi,
Giacomo Leidi <goodoldpaul <at> autistici.org> writes:
> This commit adds a password-file to the postgresql-role field. It
> allows users to provision Postgres roles with a set password.
>
> * gnu/services/databases.scm (postgresql-role): Add password-file field;
> (postgresql-role-configuration): add requirement field;
> (postgresql-create-roles): add support for setting passwords from a
> file without leaking passwords to the command line;
> (postgresql-role-shepherd-service): add support for customizable
> requirements.
> * gnu/tests/databases.scm: Test it.
> * doc/guix.texi: Document it.
Looks useful!
Nitpick: Each change can be a sentence, starting with a capitalized
letter and ending with a period.
> Change-Id: I3aabaa10b0c5e826c5aa874e5649e25a3508a585
> ---
> doc/guix.texi | 15 +++++++++---
> gnu/services/databases.scm | 48 +++++++++++++++++++++++++++++++++-----
> gnu/tests/databases.scm | 38 +++++++++++++++++++++++++++---
> 3 files changed, 89 insertions(+), 12 deletions(-)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index bee80cd4e2b..8b32557f76a 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -27588,9 +27588,10 @@ Database Services
>
> @lisp
> (service-extension postgresql-role-service-type
> - (const (postgresql-role
> - (name "alice")
> - (create-database? #t))))
> + (const (list
> + (postgresql-role
> + (name "alice")
> + (create-database? #t)))))
Are you sure this is correct? If it's a bug fix, it's not listed in the
changelog.
> @end lisp
> @end defvar
>
> @@ -27613,6 +27614,10 @@ Database Services
> @item @code{create-database?} (default: @code{#f})
> whether to create a database with the same name as the role.
>
> +@item @code{password-file} (default: @code{#f})
> +A string representing the path of a file that contains the password to be set
> +for the role.
> +
> @item @code{encoding} (default: @code{"UTF8"})
> The character set to use for storing text in the database.
>
> @@ -27641,6 +27646,10 @@ Database Services
> @item @code{log} (default: @code{"/var/log/postgresql_roles.log"})
> File name of the log file.
>
> +@item @code{requirement} (default: @code{'()}) (type: list-of-symbols)
> +Set additional Shepherd services dependencies to the provisioned
> +Shepherd service.
It seems the canonical/most conventional configuration field name to use
for this is 'shepherd-requirement', not 'requirement'.
[...]
> postgresql-role?
> postgresql-role-name
> + postgresql-role-password-file
> postgresql-role-permissions
> postgresql-role-create-database?
> postgresql-role-configuration
> postgresql-role-configuration?
> postgresql-role-configuration-host
> + postgresql-role-configuration-requirement
> postgresql-role-configuration-roles
>
> postgresql-role-service-type
> @@ -374,6 +378,8 @@ (define-record-type* <postgresql-role>
> postgresql-role make-postgresql-role
> postgresql-role?
> (name postgresql-role-name) ;string
> + (password-file postgresql-role-password-file ;string
> + (default #f))
> (permissions postgresql-role-permissions
> (default '(createdb login))) ;list
> (create-database? postgresql-role-create-database? ;boolean
> @@ -392,6 +398,8 @@ (define-record-type* <postgresql-role-configuration>
> postgresql-role-configuration?
> (host postgresql-role-configuration-host ;string
> (default "/var/run/postgresql"))
> + (requirement postgresql-role-configuration-requirement ;list-of-symbols
> + (default '()))
as mentioned above, this should be 'shepherd-requirement'.
> (log postgresql-role-configuration-log ;string
> (default "/var/log/postgresql_roles.log"))
> (roles postgresql-role-configuration-roles
> @@ -409,19 +417,36 @@ (define (postgresql-create-roles config)
> permissions)
> " ")))
>
> + (define (password-value role)
> + (string-append "password_" (postgresql-role-name role)))
> +
> + (define (role->password-variable role)
> + (define file-name
> + (postgresql-role-password-file role))
I'd maybe use a let here, just for stylistic preferences.
> + (if (string? file-name)
> + ;; This way passwords do not leak to the command line
Please use complete sentences, including the trailing period, for
stand-alone comments.
> + #~(string-append "-v \"" #$(password-value role)
> + "=$(" #+coreutils "/bin/cat " #$file-name ")\"")
Are you sure this should use ungexp-native (#+) on coreutils? If this
runs in a snippet, it should use the regular ungexp (#$). It could also
use file-append. Only if it runs as part of a derivation build should
it use #+.
> + ""))
> +
> (define (roles->queries roles)
> (apply mixed-text-file "queries"
> (append-map
> (lambda (role)
> (match-record role <postgresql-role>
> (name permissions create-database? encoding collation ctype
> - template)
> + template password-file)
> `("SELECT NOT(EXISTS(SELECT 1 FROM pg_catalog.pg_roles WHERE \
> rolname = '" ,name "')) as not_exists;\n"
> "\\gset\n"
> "\\if :not_exists\n"
> "CREATE ROLE \"" ,name "\""
> " WITH " ,(format-permissions permissions)
> +,(if (and (string? password-file)
> + (not (string-null? password-file)))
Since you already check for the string-null? case, perhaps the default
password value should be "" instead of #f? And then you could do away
with the string? check (though ideally there could be a sanitizer to
verify that a string is passed -- perhaps best leaving it for the day
this gets ported to define-configuration, which auto-generates sanitizers
based on the type specified for the field in its form.
> + (string-append
> + "\nPASSWORD :'" (password-value role) "'")
> + "")
> ";\n"
> ,@(if create-database?
> `("CREATE DATABASE \"" ,name "\""
> @@ -436,20 +461,30 @@ (define (postgresql-create-roles config)
>
> (let ((host (postgresql-role-configuration-host config))
> (roles (postgresql-role-configuration-roles config)))
> - #~(let ((psql #$(file-append postgresql "/bin/psql")))
> - (list psql "-a" "-h" #$host "-f" #$(roles->queries roles)))))
> + (program-file "run-queries"
> + #~(let ((bash #$(file-append bash-minimal "/bin/bash"))
> + (psql #$(file-append postgresql "/bin/psql")))
> + (define command
> + (string-append
> + "set -e; exec " psql " -a -h " #$host " -f "
> + #$(roles->queries roles) " "
> + (string-join
> + (list
> + #$@(map role->password-variable roles))
> + " ")))
> + (execlp bash bash "-c" command)))))
>
> (define (postgresql-role-shepherd-service config)
> (match-record config <postgresql-role-configuration>
> - (log)
> + (log requirement)
> (list (shepherd-service
> - (requirement '(user-processes postgres))
> + (requirement `(user-processes postgres ,@requirement))
I see two styles being used in Guix services: 'shepherd-requirement'
extend the requirements, or overrides it, with the 'user-processes at
least normally found in the default values. I think at this point in
time the later fits better in Guix (think of arguments such as #:modules
and #:imported-modules, etc. the user has full control on them, for the
better and worst).
> (provision '(postgres-roles))
> (one-shot? #t)
> (start
> #~(lambda args
> (zero? (spawn-command
> - #$(postgresql-create-roles config)
> + (list #$(postgresql-create-roles config))
Why is this change now necessary? I didn't follow.
> #:user "postgres"
> #:group "postgres"
> ;; XXX: As of Shepherd 1.0.2, #:log-file is not
> @@ -468,6 +503,7 @@ (define postgresql-role-service-type
> (match-record config <postgresql-role-configuration>
> (host roles)
> (postgresql-role-configuration
> + (inherit config)
and this one? Was it a preexisting bug?
> (host host)
> (roles (append roles extended-roles))))))
> (default-value (postgresql-role-configuration))
> diff --git a/gnu/tests/databases.scm b/gnu/tests/databases.scm
> index fd5041344b6..c5da603565d 100644
> --- a/gnu/tests/databases.scm
> +++ b/gnu/tests/databases.scm
> @@ -142,6 +142,8 @@ (define %role-log-file
>
> (define %postgresql-os
> (simple-operating-system
> + (extra-special-file "/password"
> + (plain-file "password" "hello"))
> (service postgresql-service-type
> (postgresql-configuration
> (postgresql postgresql)
> @@ -158,6 +160,10 @@ (define %postgresql-os
> (roles
> (list (postgresql-role
> (name "root")
> + (create-database? #t))
> + (postgresql-role
> + (name "alice")
> + (password-file "/password")
> (create-database? #t))))))))
>
> (define (run-postgresql-test)
> @@ -230,14 +236,40 @@ (define (run-postgresql-test)
> (marionette-eval
> '(begin
> (use-modules (gnu services herd)
> + (srfi srfi-1)
> (ice-9 popen))
> (current-output-port
> (open-file "/dev/console" "w0"))
> + (every
> + (lambda (role)
> + (let* ((port (open-pipe*
> + OPEN_READ
> + #$(file-append postgresql "/bin/psql")
> + "-tA" "-c"
> + (string-append
> + "SELECT 1 FROM pg_database WHERE"
> + " datname='" role "'")))
> + (output (get-string-all port)))
> + (close-pipe port)
> + (string-contains output "1")))
> + '("root" "alice")))
> + marionette))
> +
> + (test-assert "database passwords are set"
> + (marionette-eval
> + '(begin
> + (use-modules (gnu services herd)
> + (ice-9 match)
> + (ice-9 popen))
> + (current-output-port
> + (open-file "/dev/console" "w0"))
> + (setgid (passwd:gid (getpwnam "alice")))
> + (setuid (passwd:uid (getpw "alice")))
> + (setenv "PGPASSWORD" "hello")
> (let* ((port (open-pipe*
> OPEN_READ
> - #$(file-append postgresql "/bin/psql")
> - "-tA" "-c" "SELECT 1 FROM pg_database WHERE
> - datname='root'"))
> + #$(file-append postgresql "/bin/psql") "-tA" "-c"
> + "SELECT 1 FROM pg_database WHERE datname='alice'"))
Nice to have tests! Could you please send a v7 taking care of my
comments? Then I guess it'd be good to go.
--
Thanks,
Maxim
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.