Subject: Re: bug#73196: [PATCH] services: postgresql-role: Add support for password files.

From: Maxim Cournoyer <[email-masked]>
Date: Fri, 25 Apr 2025 22:27:27 +0900

Hi,

Giacomo Leidi <[email-masked]> 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