7
\$\begingroup\$

I'm writing a bash script to that picks up a user password from an environment variable, hashes this, and inserts the results into a postgres database.

What I have works and looks fairly readable to me but I'm no expert on bash. Are there better conventions I could be following for what I'm doing?

#!/bin/bash
HASHED=$(echo -n $GUAC_PASSWORD | sha256sum | head -c 64)

PGPASSWORD=$POSTGRES_PASSWORD psql -U postgres << EOF
INSERT INTO guacamole_entity (name, type) VALUES ('guacadmin', 'USER');
INSERT INTO guacamole_user (entity_id, password_hash, password_salt, password_date)
SELECT
    entity_id,
    decode('${HASHED}', 'hex'), 
    null,
    CURRENT_TIMESTAMP
FROM guacamole_entity WHERE name = 'guacadmin' AND guacamole_entity.type = 'USER';
EOF

Edit: I thought up a way to do it without sed feels neater to me.


Update: Thanks @OhMyGoodness and @TobySpeight! The conversations about passwords being in the clear got me twitchy enough to use docker secrets. So now I have the passwords in a file, not environment variables. PGPASSWORD isn't actually needed as

The PostgreSQL image sets up trust authentication locally so you may notice a password is not required when connecting from localhost

The finalized script looks like this now:

#!/bin/bash
hashed=$(tr -d '\n' < /run/secrets/guac_password | sha256sum | tr -dc a-f0-9)

psql -U postgres << EOF
INSERT INTO guacamole_entity (name, type) VALUES ('guacadmin', 'USER');
INSERT INTO guacamole_user (entity_id, password_hash, password_salt, password_date)
SELECT
    entity_id,
    decode('${hashed}', 'hex'), 
    null,
    CURRENT_TIMESTAMP
FROM guacamole_entity WHERE name = 'guacadmin' AND guacamole_entity.type = 'USER';
EOF
\$\endgroup\$
  • \$\begingroup\$ I'm not sure what the protocol is - should I be accepting an answer? Multiple people can help with code review so does an accepted answer make sense? \$\endgroup\$ – russau Sep 6 '19 at 21:45
  • 1
    \$\begingroup\$ There is impedance mismatch between the SE platform's workings and the process of reviewing code, as you've observed. It's customary to accept an answer when one is clearly best. It's fine to not when multiple people made contributions, like here. Knowing that my answer was helpful is more important to me than the points. \$\endgroup\$ – Oh My Goodness Sep 7 '19 at 6:32
6
\$\begingroup\$

If GUAC_PASSWORD is a string like -e foo or /etc/*, that's going to create problems.

Quote the input and avoid echo altogether. While you're at it, whitelist the checksum output, making head redundant:

HASHED=$( tr -d '\n' <<<"$GUAC_PASSWORD" | sha256sum | tr -dc a-f0-9 )

edit: suppress newline added by <<<

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Is this to avoid an injection attack? \$\endgroup\$ – russau Sep 2 '19 at 1:07
  • \$\begingroup\$ it avoids any number of surprises. We're not worried about a future version of sha256sum having ' in its output or error messages. And we're not quoting the assignment: the whitelist makes that unnecessary. \$\endgroup\$ – Oh My Goodness Sep 2 '19 at 6:01
  • \$\begingroup\$ You'll want to deal with the newline that <<< adds to the string. Otherwise, this is great advice - it avoids exposing the plaintext password in command arguments, for one thing. \$\endgroup\$ – Toby Speight Sep 2 '19 at 9:37
  • \$\begingroup\$ @TobySpeight good catch, I'll copy your tr approach there \$\endgroup\$ – Oh My Goodness Sep 2 '19 at 13:44
4
\$\begingroup\$

There's nothing here that requires Bash rather than standard (POSIX) shell, so we can use

#!/bin/sh

However, don't do that, because we can use a Bash feature to avoid exposing the plaintext password to other users (see below).

I recommend making the shell exit if we attempt to expand any unset variables, or if any of the programs we run exits with an error status:

set -eu -o pipefail

Let's look at this line:

HASHED=$(echo -n $GUAC_PASSWORD | sha256sum | head -c 64)

Firstly, use uppercase names only for well-known environment variables to be shared between processes; we should prefer lower-case for our own variables.

Avoid non-standard echo -n and quote variable expansions used as command arguments.

Don't include private information as a command argument. Any process can see command arguments (e.g. using ps or top), so that is a privacy violation right there. We can avoid that by using a Bash here-string, using <<< redirection, but we need to be aware that this will add a newline to the stream. We could use tr to remove that:

hashed=$(tr -d '\n' <<<"$GUAC_PASSWORD" | sha256sum | cut -d' ' -f1)
| improve this answer | |
\$\endgroup\$
3
\$\begingroup\$

Try to avoid using echo in your bash script; use printf instead (why? look here):

hashed=$(printf "%s" "$GUAC_PASSWORD" | sha256sum | cut -f1 -d\ )

Note also the uppercase variable in bash script are for environment variables; use lowercase ones (ref).

At last, do not use <<< in your case because it adds a newline and won't output the same hash as echo -n!

Try this:

echo -n "Hello World" | sha256sum 
a591a6d40bf420404a011733cfb7b190d62c65bf0bcda32b57b277d9ad9f146e  -
sha256sum <<< "Hello World"
d2a84f4b8b650937ec8f73cd8be2c74add5a911ba64df27458ed8229da804a26  -
| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.