7
\$\begingroup\$

I have some code that is loading passwords from AWS SSM and then using them through the script. I am really concerned that some funky password character is going to be used that is going to get interpreted/escaped/munged by bash. The code in question is below. Are there any obvious problems here? Can I improve this so that no password can trip this up?

updateEnvFile() {
    local user=$1
    local pass=$2
    local host=$3

    cat "${bin}/../environments/development.properties" |
    sed "s;url=jdbc:mysql://localhost/mysql;url=jdbc:mysql://$host/mysql;" |
    sed "s/username=root/username=$user/" | sed "s/password=my-secret-pw/password=$pass/"
}

updateConfig() {
    local dbs=$1
    for db in ${dbs}; do
        local user=${params["/databases/migrate/${env}/${db}.user"]}
        local pass=${params["/databases/migrate/${env}/${db}.password"]}
        local host=${params["/databases/migrate/${env}/${db}.host"]}
        updateEnvFile "${user}" "${pass}" "${host}" > "${bin}/../environments/${db}.properties"
    done
}
\$\endgroup\$
2
\$\begingroup\$

Do you really pass a single argument with space-separated names to updateConfig like this?:

updateConfig "db1 db2 db3"

It would be simpler to let the function accept any number of arguments, then the loop becomes simply

for db
do
    #...
done

(in "$@" is inferred)


I'm not a fan of the repetition of /databases/migrate/${env}/${db} - consider a local variable for that:

local basepath="/databases/migrate/$env/$db"
for db; do
    local user=${params["$basepath.user"]}
    local pass=${params["$basepath.password"]}
    local host=${params["$basepath.host"]}
    updateEnvFile "$user" "$pass" "$host" >"$bin/../environments/$db.properties"
done

When substituting shell variables using sed s command, be sure to transform the replacement to quote any backslashes or separators in the substitution:

updateEnvFile() {
    local user=${1//\\/\\\\}
    local pass=${2//\\/\\\\}
    local host=${3//\\/\\\\}

    sed -e "/^url=/s;localhost;${host//;/\\;};" \
        -e  "/^username=/s;=.*;=${user//;/\\;};" \
        -e "/^password=/s;=.*;=${pass//;/\\;};" \
        "$bin/../environments/development.properties"
}

I've also combined your long pipeline into a singled sed invocation, and simplified each replacement to separate the matching and replacement parts.

\$\endgroup\$
  • \$\begingroup\$ The number of databases in question is discovered from the SSM params. So, it looks for anything with $basePath/(db[0-9]+) - literal code for that: echo "${!params[@]}" | tr " " "\n" | sed 's;.*\(db[0-9]\).*;\1;' | sort| uniq I am going to see about what you have and apply it \$\endgroup\$ – Christian Bongiorno Jun 26 '18 at 1:05
  • \$\begingroup\$ I tried your example: The syntax for declaring user/pass/host isn't right. I am using bash v4 \$\endgroup\$ – Christian Bongiorno Sep 12 '18 at 21:32
  • \$\begingroup\$ If I've made an error, please go ahead and propose a correction. Thanks. \$\endgroup\$ – Toby Speight Sep 13 '18 at 7:08
  • \$\begingroup\$ I would if I was sure exactly how it worked! :) \$\endgroup\$ – Christian Bongiorno Sep 13 '18 at 19:32
2
\$\begingroup\$

That cat is entirely unnecessary. Also, multiple pipes to sed are redundant.

sed by design can take a single long expression and process the data.

As for your delimiters, I'd suggest using non-printable characters when in sed:

SED_DELIM=$(echo -en "\001")

and then:

sed "s${SED_DELIM}url=jdbc:mysql://localhost/mysql${SED_DELIM}url=jdbc:mysql://$host/mysql${SED_DELIM}; s${SED_DELIM}username=root${SED_DELIM}username=$user${SED_DELIM}; s${SED_DELIM}password=my-secret-pw${SED_DELIM}password=$pass${SED_DELIM}" $file_name

Now, that looks quite long. You can evaluate individual expressions:

sed -e "s${SED_DELIM}url=jdbc:mysql://localhost/mysql${SED_DELIM}url=jdbc:mysql://$host/mysql${SED_DELIM};" \
-e "s${SED_DELIM}username=root${SED_DELIM}username=$user${SED_DELIM};" \
-e "s${SED_DELIM}password=my-secret-pw${SED_DELIM}password=$pass${SED_DELIM}" \
$file_name

I'll go ahead and also use back-references:

sed -e "s${SED_DELIM}\(url=jdbc:mysql://\)localhost\(/mysql\)${SED_DELIM}\1$host\2${SED_DELIM};" \
-e "s${SED_DELIM}\(username=\)root${SED_DELIM}\1$user${SED_DELIM};" \
-e "s${SED_DELIM}\(password=\)my-secret-pw${SED_DELIM}\1$pass${SED_DELIM}" \
$file_name
\$\endgroup\$
  • \$\begingroup\$ It didn't even occur to me that sed might choke on something but a slash is entirely within the realm of password chars that might be chosen. Thanks! I was thinking more like bash and "" doing weird things \$\endgroup\$ – Christian Bongiorno Jun 23 '18 at 2:55
  • 1
    \$\begingroup\$ Portable sed command lines have exactly one command per -e argument (writing ; in place of the newline is a common, but not universal, extension to POSIX). Also, given this is Bash, $'\001' is simpler than your echo command (but I don't agree with your recommendation not to simply use distinct characters for the delimiters; I like ! or # when / is unsuitable, others choose , - instead, make the variables safe for interpolation, e.g. s!pattern!${replacement//!/\!}!) \$\endgroup\$ – Toby Speight Jun 25 '18 at 7:45

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.