4
\$\begingroup\$

From the previous 2 versions:
Sign an EC2/S3 URL Version 2

I fixed the comments from the previous version.
Most of the code remains the same (so I have split out the new code for review). The main body of the code is provided as reference.

The main addition is allowing six flags to be used. Four of these flags allow the addition of more headers and/or query parameters from the command line. One allows you customize the expirey time. The last one allows you to pass the authentication info in the headers rather than query parameters.

I think that is all the features I need.
Enjoy.

#!/usr/bin/env bash

declare -a ARGS
declare -a headers
declare -a querys
expires=3600
signHeaders=0

for var in "$@"; do
    if [[ "${var%=*}" = '--headers' ]]; then
        file=${var#*=}
        while IFS= read -r line; do
            headers+=("$line")
        done < ${file}
        continue
    fi  
    if [[ "${var%=*}" = '--header' ]]; then
        keyValue=${var#*=}
        headers+=(${keyValue})
        continue
    fi  
    if [[ "${var%=*}" = '--querys' ]]; then
        file=${var#*=}
        while IFS= read -r keyValue; do
            key=${keyValue%:*}
            val=${keyValue#*:}
            querys+=("${key}=${value}")
        done < ${file}
        continue
    fi  
    if [[ "${var%=*}" = '--query' ]]; then
        keyValue=${var#*=}
        key=${keyValue%:*}
        val=${keyValue#*:}
        querys+=("${key}=${value}")
        continue
    fi  
    if [[ "${var}" = '--signHeaders' ]]; then
        signHeaders=1
        continue
    fi  
    if [[ "${var}" = '--expires' ]]; then
        expires=${var#*=}
        continue
    fi  
    ARGS+=("$var")
done

if [[ ${#ARGS[@]} < 3  || ${#ARGS[@]} > 4 ]]; then
    echo "Usage:"
    echo "    sign <url> <key> <secret> [<TimeStamp>] [--headers=<fileName>] [--header=<Key>:<Value>] [--querys=<fileName>] [--query=<Key>:<Value>] [--signHeaders] [--expires=<TimeInSecs>]"
    echo
    echo "Note 1: TimeStamp: must be YYYYMMDD'T'hhmmsa'Z's"
    echo "        eg 20170901T230559Z"
    echo "        YYYY => Year MM => Month DD => day hh => hour mm => minute ss => second"
    echo
    echo "Note 2: Currently does not support URL with Query or Fragment sections."
    exit 1
fi


url=${ARGS[0]}
key=${ARGS[1]}
secret=${ARGS[2]}
dateTime=${ARGS[3]-$(date -u +"%Y%m%dT%H%M%SZ")}

This puts all the query parameters and headers into two arrays. We use these arrays to build all the parts we need:

headers+=("Host:${host}")
if [[ "${signHeaders}" == "1" ]]; then
    headers+=("X-Amz-Date:${dateTime}")
    headers+=("X-Amz-Content-SHA256:UNSIGNED-PAYLOAD")
fi

crheaders=()
headerExtra=""
headerList=""
for loop in "${headers[@]}"; do
    name=$(tr '[A-Z]' '[a-z]' <<< "${loop%:*}")
    crheaders+=("${name}:${loop#*:}")
done
IFS=$'\n' sortedHeaders=($(sort <<<"${crheaders[*]}"))
for loop in "${sortedHeaders[@]}"; do
    name=${loop%:*}
    headerExtra+="%3B${name}"
    headerList+=";${name}"
done


if [[ "${signHeaders}" == "0" ]]; then
    querys+=("X-Amz-Algorithm=AWS4-HMAC-SHA256")
    querys+=("X-Amz-Credential=${key}%2F${date}%2F${region}%2F${service}%2Faws4_request")
    querys+=("X-Amz-Date=${dateTime}")
    querys+=("X-Amz-Expires=${expires}")
    querys+=("X-Amz-SignedHeaders=${headerExtra#%3B}")
fi

IFS=$'\n' sortedQuerys=($(sort <<<"${querys[*]}"))
queryParam=""
querySep='?'
for loop in "${sortedQuerys[@]}"; do
    queryParam+="${querySep}${loop}"
    querySep='&'
done

The full code is available here for reference. But this has mostly been reviewed. If you want to or have comments then please feel free to read it and comment on it.

All The Code

#!/usr/bin/env bash

declare -a ARGS
declare -a headers
declare -a querys
expires=3600
signHeaders=0
for var in "$@"; do
    if [[ "${var%=*}" = '--headers' ]]; then
        file=${var#*=}
        while IFS= read -r line; do
            headers+=("$line")
        done < ${file}
        continue
    fi
    if [[ "${var%=*}" = '--header' ]]; then
        keyValue=${var#*=}
        headers+=(${keyValue})
        continue
    fi
    if [[ "${var%=*}" = '--querys' ]]; then
        file=${var#*=}
        while IFS= read -r keyValue; do
            key=${keyValue%:*}
            val=${keyValue#*:}
            querys+=("${key}=${val}")
        done < ${file}
        continue
    fi
    if [[ "${var%=*}" = '--query' ]]; then
        keyValue=${var#*=}
        key=${keyValue%:*}
        val=${keyValue#*:}
        querys+=("${key}=${val}")
        continue
    fi
    if [[ "${var}" = '--signHeaders' ]]; then
        signHeaders=1
        continue
    fi
    if [[ "${var}" = '--expires' ]]; then
        expires=${var#*=}
        continue
    fi
    ARGS+=("$var")
done

if [[ ${#ARGS[@]} < 3  || ${#ARGS[@]} > 4 ]]; then
    echo "Usage:"
    echo "    sign <url> <key> <secret> [<TimeStamp>] [--headers=<fileName>] [--header=<Key>:<Value>] [--querys=<fileName>] [--query=<Key>:<Value>] [--signHeaders] [--expires=<TimeInSecs>] "
    echo
    echo "Note 1: TimeStamp: must be YYYYMMDD'T'hhmmsa'Z's"
    echo "        eg 20170901T230559Z"
    echo "        YYYY => Year MM => Month DD => day hh => hour mm => minute ss => second"
    echo
    echo "Note 2: Currently does not support URL with Query or Fragment sections."
    exit 1
fi


url=${ARGS[0]}
key=${ARGS[1]}
secret=${ARGS[2]}
dateTime=${ARGS[3]-$(date -u +"%Y%m%dT%H%M%SZ")}

#
# The First part of <dateTime> before the T
date=${dateTime%%T*}

#
# Amazon Hosted URLS are built up in sections.
#   http://<service>-<region>.<Amazon End Point><Path>
#
#   Strip out these parts from the url
urlNoSchema=${url#https://}
host=${urlNoSchema%%/*}
serviceRegion=${host%%\.*}
service=${serviceRegion%%-*}
region=${serviceRegion#*-}
path=/${url#https://*/}
file=${path##*/}


headers+=("Host:${host}")
if [[ "${signHeaders}" == "1" ]]; then
    headers+=("X-Amz-Date:${dateTime}")
    headers+=("X-Amz-Content-SHA256:UNSIGNED-PAYLOAD")
fi

crheaders=()
headerExtra=""
headerList=""
for loop in "${headers[@]}"; do
    name=$(tr '[A-Z]' '[a-z]' <<< "${loop%:*}")
    crheaders+=("${name}:${loop#*:}")
done
IFS=$'\n' sortedHeaders=($(sort <<<"${crheaders[*]}"))
for loop in "${sortedHeaders[@]}"; do
    name=${loop%:*}
    headerExtra+="%3B${name}"
    headerList+=";${name}"
done


if [[ "${signHeaders}" == "0" ]]; then
    querys+=("X-Amz-Algorithm=AWS4-HMAC-SHA256")
    querys+=("X-Amz-Credential=${key}%2F${date}%2F${region}%2F${service}%2Faws4_request")
    querys+=("X-Amz-Date=${dateTime}")
    querys+=("X-Amz-Expires=${expires}")
    querys+=("X-Amz-SignedHeaders=${headerExtra#%3B}")
fi

IFS=$'\n' sortedQuerys=($(sort <<<"${querys[*]}"))
queryParam=""
querySep='?'
for loop in "${sortedQuerys[@]}"; do
    queryParam+="${querySep}${loop}"
    querySep='&'
done

#
# Build the canonical request
IFS=$'\n'
cr=$(uniq <<CanonicalRequest
GET
${path}
${queryParam#?}
${sortedHeaders[*]}

${headerList#;}
UNSIGNED-PAYLOAD
CanonicalRequest
)

#
# Hash the canonical request
HashedCanonicalRequest=$(echo -n "${cr}" | openssl dgst -sha256)

#
# Build the String to sign.
ss=$(cat - <<StringToSign
AWS4-HMAC-SHA256
${dateTime}
${date}/${region}/${service}/aws4_request
${HashedCanonicalRequest}
StringToSign
)

#
# Calculate the signature
kDate=$(echo -n ${date}        | openssl dgst -sha256 -binary -hmac "AWS4${secret}")
kRegn=$(echo -n ${region}      | openssl dgst -sha256 -binary -hmac "${kDate}")
kServ=$(echo -n ${service}     | openssl dgst -sha256 -binary -hmac "${kRegn}")
kSign=$(echo -n "aws4_request" | openssl dgst -sha256 -binary -hmac "${kServ}")
signature=$(echo -n "${ss}"    | openssl dgst -sha256 -hmac "${kSign}")


#
# Dump intermediate values to compare against language specific implementation.
kDateH=$(openssl dgst -sha256 -hmac "AWS4${secret}" <<< "${date}")
kRegnH=$(openssl dgst -sha256 -hmac "${kDate}" <<< "${region}")
kServH=$(openssl dgst -sha256 -hmac "${kRegn}" <<< "${service}")
kSignH=$(openssl dgst -sha256 -hmac "${kServ}" <<< "aws4_request")

cat - <<IntermediateValues
Intermediate Values
url:                      ${url}
key:                      ${key}
secret:                   ${secret}
dateTime:                 ${dateTime}
date:                     ${date}
host:                     ${host}
path:                     ${path}
service:                  ${service}
region:                   ${region}
expires:                  ${expires}
HashedCanonicalRequest:   ${HashedCanonicalRequest}
kDate:                    ${kDateH}
kRegn:                    ${kRegnH}
kServ:                    ${kServH}
kSign:                    ${kSignH}
signature:                ${signature}


Cononical Request:
==================
${cr}

Signing String:
===============
${ss}

Signed URL:
===========
${url}${queryParam}${querySep}X-Amz-Signature=${signature}


Headers:
========
IntermediateValues

printf "%s\n" ${headers[@]}
if [[ "${signHeaders}" == "1" ]]; then
    echo "Authorization: AWS4-HMAC-SHA256 Credential=${key}/${date}/${region}/${service}/aws4_request, SignedHeaders=${headerList#;}, Signature=${signature}"
fi
echo
echo

echo "CURL COMMAND:"
echo "============="


echo -n "curl -o ${file} "
for loop in "${headers[@]}"; do
    if [[ ${loop%:*} != "Host" ]]; then
        echo -n "--header \"${loop}\" "
    fi
done
if [[ "${signHeaders}" == "1" ]]; then
    echo "--header \"Authorization: AWS4-HMAC-SHA256 Credential=${key}/${date}/${region}/${service}/aws4_request, SignedHeaders=${headerList#;}, Signature=${signature}\" \"${url}${queryParam}\""
else
    echo "\"${url}${queryParam}${querySep}X-Amz-Signature=${signature}\""
fi
\$\endgroup\$

2 Answers 2

2
\$\begingroup\$

When running the provided code on my laptop (Mint 19 / openssl 1.1.0g / bash 4.4.19(1)), it failed to generate a valid signature.

This was the section causing problems:

# Calculate the signature
kDate=$(echo -n ${date}        | openssl dgst -sha256 -binary -hmac "AWS4${secret}")
kRegn=$(echo -n ${region}      | openssl dgst -sha256 -binary -hmac "${kDate}")
kServ=$(echo -n ${service}     | openssl dgst -sha256 -binary -hmac "${kRegn}")
kSign=$(echo -n "aws4_request" | openssl dgst -sha256 -binary -hmac "${kServ}")
signature=$(echo -n "${ss}"    | openssl dgst -sha256 -hmac "${kSign}")

There are 2 issues:

  • openssl dgst returns an undesirable "(stdin) " prefix with the value. This has to be stripped in order to only get the resulting hash.
  • bash was complaining about null bytes in the binary values. Overall it's not safe to carry binary contents around, so one safer approach is to only exchange hexadecimal values in between calls to openssl dgst

Both issues are covered here: https://stackoverflow.com/a/22369607/8046487

# get rid of the "(stdin) " prefix with sed
$ echo -n "$data" | openssl dgst "-$digest" -hmac "$key" | sed -e 's/^.* //'

# pass a hex encoded key to openssl dgst to avoid moving binary values around
$ echo -n "$data" | openssl dgst "-$digest" -mac HMAC -macopt "hexkey:$key" | sed -e 's/^.* //'
$

Applied to your code:

# Calculate the signature
kDate=$(echo -n ${date}        | openssl dgst -sha256 -binary -hmac "AWS4${secret}" | sed -e 's/^.* //')
kRegn=$(echo -n ${region}      | openssl dgst -sha256 -mac HMAC -macopt "hexkey:${kDate}" | sed -e 's/^.* //')
kServ=$(echo -n ${service}     | openssl dgst -sha256 -mac HMAC -macopt "hexkey:${kRegn}" | sed -e 's/^.* //')
kSign=$(echo -n "aws4_request" | openssl dgst -sha256 -mac HMAC -macopt "hexkey:${kServ}" | sed -e 's/^.* //')
signature=$(echo -n "${ss}"    | openssl dgst -sha256 -mac HMAC -macopt "hexkey:${kSign}" | sed -e 's/^.* //')
\$\endgroup\$
3
  • 3
    \$\begingroup\$ Welcome to Code Review! I'm not sure your answer constitutes a review. You start by saying it may have unwanted side-effects, but after that I'm completely lost by what your answer has to do with the question. \$\endgroup\$
    – Mast
    Oct 26, 2018 at 6:05
  • \$\begingroup\$ Alright, I edited my answer to clarify where was the bug and how to fix it. \$\endgroup\$ Nov 3, 2018 at 8:58
  • \$\begingroup\$ Much appreciated, looks good :-) \$\endgroup\$
    – Mast
    Nov 3, 2018 at 9:53
1
\$\begingroup\$

I'm not familiar with Amazon's API at all. I consulted https://docs.aws.amazon.com/general/latest/gr/sigv4-create-canonical-request.html about the request format, I hope this is the applicable reference. This is not a thorough review, I was mostly looking for security risks.

You should add set -e at the beginning of the script so that it will exit if a command fails.

 key=${ARGS[1]}
 secret=${ARGS[2]}

My understanding is that key is a key identifier (not secret) and secret is the actual key. You should use a different name for key to make this less confusing, e.g. key_name.

You should not include secrets in a command line. Command lines are visible with ps and tend to be logged in command line histories, audit logs, etc. Environment variables are safer: at least they aren't visible to other users, but they can show up in logs and you need to take care not to pass them to subprocesses that might accidentally leak them. Files are best because you can write the file once and for all and then access it securely.

Therefore I propose to replace the secret argument by a secret_file.

key_name=${ARGS[1]}
secret_file=${ARGS[2]}
secret=$(cat -- "$secret_file")
if [[ "${signHeaders}" == "1" ]]; then

This is cosmetic, but since signHeaders is an integer representing a boolean, I prefer to test it through an arithmetic expression.

if ((signHeaders)); then
for loop in "${headers[@]}"; do

Using loop as a variable name is bizarre. loop is not a loop, it's a header. You aren't looping over loops! Use a variable name that reflects what's stored in the variable. Here it's a header.

     name=$(tr '[A-Z]' '[a-z]' <<< "${loop%:*}")

This should actually be tr A-Z a-z: with the brackets, you're saying that [ must be transformed to [ and ] must be transformed to ]. Fortunately this doesn't change the output.

But anyway there's a built-in bash construct to change the case of a string: ${VAR,,}. Put export LC_CTYPE=C or export LC_ALL=C near the top of the script to avoid running into any locale issue, although I think you're only doing case transcriptions with ASCII strings anyway so that wouldn't be a problem.

There's an actual bug here, which is that ${loop%:*} removes the suffix starting from the last colon. This doesn't do what you need if the text of a header contains a colon. You need ${loop%%:*}.

Here's how I'd write this loop.

for header in "${headers[@]}"; do
    name="${header%%:*}"
    crheaders+=("${name,,}:${header#*:}")
done
IFS=$'\n' sortedHeaders=($(sort <<<"${crheaders[*]}"))

The output of an unquoted variable of command substitution undergoes both field splitting and filename generation (globbing). See Why does my shell script choke on whitespace or other special characters? for some general advice on the topic. You're correctly double-quoting almost everything in the script. Here, you want splitting so you can't double-quote, but you're still getting globbing. This is a problem if a header ever contains some shell wildcards and happens to match a file name on your system (e.g. Mime-Type: text/* if there happens to be a subdirectory called text in the current directory). You don't use file globbing anywhere in your script, so a simple solution is to turn it off: put set -f near the top of the script.

Note that IFS=$'\n' sortedHeaders=… sets IFS for the rest of the script. It's two assignments, not the VAR=VALUE EXTERNAL-COMMAND syntax to set an environment variable (which IFS isn't) for the duration of an external command. It's equivalent to IFS=$'\n'; sortedHeaders=…. Put the assignments on separate lines to make it clearer. Since you never rely on the default value of IFS, you could put IFS=$'\n' near the top of the script.

Since you're calling sort, you need to set LC_LOCATE to C, otherwise the order depends on your locale. Put export LC_CTYPE=C or export LC_ALL=C near the top of the script.

queryParam=""
querySep='?'
for loop in "${sortedQuerys[@]}"; do
    queryParam+="${querySep}${loop}"
    querySep='&'
done

I found this code less clear than it could be. Below you reuse querySep in the following way: ${url}${queryParam}${querySep}. There's always a ? except when you're building the canonical request string and there you strip off a leading ? anyway. So I'd change this to make queryParam always start with a ?, and keep querySep as the separator to put after $queryParam, which is & unless there are no parameters. Here, you don't need a loop.

IFS='&'
queryParam="?${sortedQuerys[*]}"
IFS=$'\n'
if ((${#querys[@]} == 0)); then
  querySep=
else
  querySep='&'
fi
cr=$(uniq <<CanonicalRequest

I'm not sure what you're trying to do here, but it's not working. uniq requires sorted input. If you need to remove duplicate headers (but I don't see this in the AWS specification), apply uniq to `"${sortedHeaders[*]}" only.

kDate=$(echo -n ${date}        | openssl dgst -sha256 -binary -hmac "AWS4${secret}")

Per my remark above about not passing secrets on the command line, you shouldn't pass the HMAC key here. Unfortunately that doesn't seem to be possible with the openssl utility. Generally speaking, the openssl utility is designed for use in OpenSSL's test suite, not for production. Some of its commands can read a key from the environment or from a file, it's a pity that dgst -hmac can't. I would recommend calling some other utility here.

There's a further problem here, and I don't see how your script could possibly work. You're generating the binary representation of the HMAC value, which is correct according to the specification. Each byte of the HMAC has a 1/256 change of being 0. You can't put a null byte in a bash string: bash will skip them. You either need to encode and decode each step into printable characters (e.g. with a hexadecimal or Base64 representation, or avoid these problems by writing at least this section in some other language that's suitable for manipulating binary data, such as Python.

\$\endgroup\$
1
  • \$\begingroup\$ Thanks for the input. The final version is in github but split into two files: sign signature I will endeavor to go over your recommendations this weekend. \$\endgroup\$ May 11, 2018 at 15:15

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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