Hi Maxim,
Should be fixed now.Hi, Giacomo Leidi <goodoldpaul@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.
Yes I am sure, the service type has
(compose concatenate)
this is now mentioned in the changelog.
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?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 changed the define to let(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 added the period+ (if (string? file-name) + ;; This way passwords do not leak to the command linePlease use complete sentences, including the trailing period, for stand-alone comments.
this runs at system activation so in my understanding it should be #$ . Thank you for explaining the difference, I was not aware :)+ #~(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 #+.
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?+ "")) + (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.
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.+ (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.
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.
Yes, for example without this change we would lose the 'log' field's value upon extension.#: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.
I should have addressed all your comments, I'm about to send a v7. Thank you for your help!
cheers
giacomo