Passphrase configuration is limited

[jeaye]

The things which currently go wrong

• Docker Compose tries to dereference any passphrase containing $foo and requires $$foo
• The default passphrases are wrapped in double quotes, which require escapes in YAML
• After switching to single quotes, then single quotes within the passphrases need to be doubled ''
• Single quotes, even when doubled, then fail to pass through to Postgres when it creates the initial user
• ~ isn't permitted, due to a regex assumption within entrypoint.sh
• Setting new passphrases requires changing a git-tracked file

Things which can be fixed

• Passphrases should be escaped when they're sent to Postgres and other services
• The regex assumption should be replaced with something which doesn't care about the value
• The passphrases should be read from env vars and handled entirely by the user; no more changes to git-tracked files should be needed (this encourages people to commit the passphrases to git)
• We should invite and encourage people, in our docs, to use the .env file which docker-compose reads; docs here
• The variables used for passphrases should fail if not supplied; i.e.

SECRETS_secret_key: "${SECRETS_secret_key:?err}"

[timabbott]

This fixes the regex issue 78480d4 (and also makes the code a lot cleaner).

[timabbott]

Single quotes are obviously the better choice than double-quotes in YAML; just merged b2bb979 to migrate the project.

[timabbott]

For

 The variables used for passphrases should fail if not supplied; i.e.
SECRETS_secret_key: "${SECRETS_secret_key:?err}"

It seems to me that we should be making a set variable that's empty fatal here:

    for SECRET_KEY in "${SECRETS[@]}"; do                                                                                                   
        local key="SECRETS_$SECRET_KEY"                                                                                                     
        local SECRET_VAR="${!key}"                                                                                                          
        if [ -z "$SECRET_VAR" ]; then                                                                                                       
            echo "Empty secret for key \"$SECRET_KEY\"."                                                                                    
        fi                                                                                                                                  
        crudini --set "$DATA_DIR/zulip-secrets.conf" "secrets" "${SECRET_KEY}" "${SECRET_VAR}"                                              
    done                                                                                                                                    

@galexrt what's the background here?

[jeaye]

"${SECRETS_secret_key:?err}" is fatal (as per the ?err), as described here.

[timabbott]

@jeaye for this one:

 Passphrases should be escaped when they're sent to Postgres and other services

is the block that needs escaping this, or something else?

    export PGPASSWORD="$SECRETS_postgres_password"                                                                                          
    local TIMEOUT=60                                                                                                                        
    echo "Waiting for database server to allow connections ..."                                                                             
    while ! /usr/bin/pg_isready -h "$DB_HOST" -p "$DB_HOST_PORT" -U "$DB_USER" -t 1 >/dev/null 2>&1                                         

[jeaye]

Not sure anymore, but that looks likely. Adding a ' to the middle of your passphrase should raise the issue.

[timabbott]

"${SECRETS_secret_key:?err}" is fatal (as per the ?err), as described here.

Understood, I'm just noticing we seem to have code checking for blank secrets and just printing a line of output (that surely gets lost in the noise), and I'm curious why.

[timabbott]

On the database front, it looks like it fails here:

database_1   | 2018-08-23 20:20:50.925 UTC [66] ERROR:  unrecognized role option "fun" at character 52
database_1   | 2018-08-23 20:20:50.925 UTC [66] STATEMENT:  CREATE USER "zulip" WITH SUPERUSER PASSWORD 'zulip'fun' ;
database_1   | ERROR:  unrecognized role option "fun"
database_1   | LINE 1: CREATE USER "zulip" WITH SUPERUSER PASSWORD 'zulip'fun' ;
database_1   |                                                            ^

That's going to be an upstream issue in https://github.com/docker-library/postgres/. We can report an issue upstream, but I think probably our solution should just be to document that certain characters are invalid in a comment in docker-compose.yml; what do you think @jeaye?

[jeaye]

Glad you could repro. Seems like reporting it upstream and making it clear within docker-zulip is the best we can do for now, unless it's a quick fix. If we provided a tool for generating passphrases into .env, we could take care of this for the user.

[timabbott]

Agreed re: reporting upstream; can you take reporting this one? And yeah, if we're providing a tool, we can just have the tool not use $ and '.

[jeaye]

Done.

[jeaye]

docker-library/postgres#488 has been resolved.

[timabbott]

Awesome! I guess we need to wait for them to do a release before we can take advantage of this?

[jeaye]

Yeah, I'm not sure how that works. This was the first time I'd ever used Docker.

[timabbott]

https://hub.docker.com/_/postgres/ appears to have been last pushed to 7 hours ago, so maybe that means we can grab the latest image and we'll be OK? Worth testing...