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 #47 received at 73196 <at> debbugs.gnu.org (full text, mbox):
From: paul <goodoldpaul <at> autistici.org> To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> 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: Sun, 27 Apr 2025 17:07:01 +0200
[Message part 1 (text/plain, inline)]
Hi Maxim,
On 4/25/25 3:27 PM, Maxim Cournoyer wrote:
> 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.
Should be fixed now.
>
>> 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.
Yes I am sure, the service type has
(compose concatenate)
this is now mentioned in the changelog.
>> 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'.
I know at least two examples that are already in master which use
'requirement' (I may be biased as I authored both of them): the
restic-backup-service-type and the oci-container-service-type. If you
don't find this to be a blocker I would defer this change to a specific
commit setting a convention for the whole codebase after an analysis of
which services use simply 'requirement' and which use
'shepherd-requirement'. Is it ok for you?
>> (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.
I changed the define to let
>
>> + (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.
I added the period
>
>> + #~(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 #+.
this runs at system activation so in my understanding it should be #$ .
Thank you for explaining the difference, I was not aware :)
>
>> + ""))
>> +
>> (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.
in its ideal form (using define-configuration) this would be a
'maybe-string' type, as it represents an optional path. if you don't
find this a blocker I would leave this for a later change where we could
use 'maybe-value-set?' and leave the type check to a sanitizer. What do
you think?
>> + (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).
Thank you for pointing this out, I created a
%postgresql-role-shepherd-requirement value and exported it to allow
users which want to override the Guix defaults, while retaining them can
simply append to this list.
>
>> (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.
the return value of postgresql-create-roles changed, before this change
it returned a list containing a psql command line, after this change it
returns a gexp which will evaluate to the entrypoint responsible for
reading password files and then calling psql.
>
>> #: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?
Yes, for example without this change we would lose the 'log' field's
value upon extension.
>
>> (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.
I should have addressed all your comments, I'm about to send a v7. Thank
you for your help!
cheers
giacomo
[Message part 2 (text/html, inline)]
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.