feat: update creds script for permanent creds #143
Conversation
|
Note that we have removed the unbound variable test, since this is a condition we are expecting and checking for. |
| echo "aws_session_token=$AWS_SESSION_TOKEN" >> $CREDS | ||
| echo "aws_secret_access_key=$AWS_SECRET_ACCESS_KEY" >> $CREDS | ||
| # This is if we have non-temp credentials... | ||
| if [[ -z "${AWS_SESSION_TOKEN+x}" ]]; then |
There was a problem hiding this comment.
Checking my understanding here, we just want to test if the env var has a value. If the env var has a value, we don't want to actually interpolate it, so we replace it with x as described here and the reason we do so is because the var may contain sensitive information.
There was a problem hiding this comment.
Partially; the other half of this is that we want to check to see if the variable is set or not.
In this example, if AWS_SESSION_TOKEN is set, that expression evaluates to x, which is then tested with -z (which returns true on a zero-length string), and found false so (in this case) we know we have a value in that variable.
However, if the variable is unset then that expression evaluates to nothing, which when tested by -z is true (since it is a zero-length string).
Why do we go through this trouble? One thing is that if we have set -o unbound in the script our script will blow up if it hits an unbound variable (which is the point, but it can bite one in the ass at times). The other thing is that doing a -z on just the variable will allow for two conditions - the unset variable we care about and a variable set to an empty string (which we may not want).
I think this makes a good argument for rewriting this in python... ;)
bin/aws_write_creds.sh
Outdated
| echo "aws_secret_access_key=$AWS_SECRET_ACCESS_KEY" >> $CREDS | ||
| # This is if we have non-temp credentials... | ||
| if [[ -z "${AWS_SESSION_TOKEN+x}" ]]; then | ||
| echo "Not using AWS Session Token" |
There was a problem hiding this comment.
optional nit: Would it be more diagnosable to say "AWS_SESSION_TOKEN was empty or unset. Not using AWS Session Token". That way there's a clear distinction between "the script has decided not to use an aws session token and "we checked this var and it was empty so we're continuing without it". Might help in the case where the user thought they'd set it but didn't.
There was a problem hiding this comment.
Yeah, that sounds quite a bit better than what I had...let me make a change so it is more clear.
Proposed changes
Small change to allow for permanent AWS credentials (which do not contain a session token variable). This logic will account for both temporary and permanent creds. To use permanent credentials, one just needs to ensure that they have not set the AWS_SESSION_TOKEN variable in the Jenkins creds configuration.
Checklist
Before creating a PR, run through this checklist and mark each as complete.