Add support for assuming roles from web identities. #63
Conversation
…b-identity Feature/support assume role with web identity
There was a problem hiding this comment.
Hi there
Thanks for taking the time to write this Mark, and for your patience while I got back to you!
I have some comments on the changes which I've put on the relevant lines, though I'm happy to talk about any and all of them.
One other comment on this, and I appreciate this is probably a limitation of how the package is currently written, but I think this code is going to result in the package creating new temporary credentials for each request to AWS. This doesn't seem 100% ideal. But I don't have an easy fix for that.
DESCRIPTION
Outdated
| ) | ||
| Description: Generates version 2 and version 4 request signatures for Amazon Web Services ('AWS') <https://aws.amazon.com/> Application Programming Interfaces ('APIs') and provides a mechanism for retrieving credentials from environment variables, 'AWS' credentials files, and 'EC2' instance metadata. For use on 'EC2' instances, users will need to install the suggested package 'aws.ec2metadata' <https://cran.r-project.org/package=aws.ec2metadata>. | ||
| License: GPL (>= 2) | ||
| Imports: | ||
| digest, | ||
| base64enc | ||
| base64enc, | ||
| httr |
There was a problem hiding this comment.
I'm not super keen on adding a big set of dependencies like httr to a package that previously had almost none. Can this be revised to use the curl and jsonlite packages as a slightly smaller set?
There was a problem hiding this comment.
Addressed this to just use curl and jsonlite
R/assume_role.R
Outdated
| content <- httr::content(response) | ||
|
|
||
| if (httr::status_code(response) == 200) { | ||
| message("Successfully fetched token.") |
There was a problem hiding this comment.
This should probably have the verbosity flagging as similar code does through the package
There was a problem hiding this comment.
👍 addressed this in 1eac62d
R/locate_credentials.R
Outdated
| @@ -12,7 +12,7 @@ | |||
| #' @details These functions locate values of AWS credentials (access key, secret access key, session token, and region) from likely sources. The order in which these are searched is as follows: | |||
| #' \enumerate{ | |||
| #' \item user-supplied values passed to the function | |||
| #' \item environment variables (\env{AWS_ACCESS_KEY_ID}, \env{AWS_SECRET_ACCESS_KEY}, \env{AWS_DEFAULT_REGION}, and \env{AWS_SESSION_TOKEN}) | |||
| #' \item environment variables, first checking for Web Identity credentials (\env{AWS_ROLE_ARN} and \env{AWS_WEB_IDENTITY_TOKEN_FILE}), then default credentials (\env{AWS_ACCESS_KEY_ID}, \env{AWS_SECRET_ACCESS_KEY}, \env{AWS_DEFAULT_REGION}, and \env{AWS_SESSION_TOKEN}) | |||
There was a problem hiding this comment.
This seems to be a divergence from how boto3 grabs credentials, which favours env variables over provider type credentials like these. I would prefer the package remains consistent with the boto3 ordering.
There was a problem hiding this comment.
Moved to favouring the AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY and AWS_SESSION_TOKEN over AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE.
Remove `httr` dependency
This favors default environment variables over the web identity provider environment variables. This was done to address package author's PR comment.
Change credential ordering
|
@jon-mago - we've addressed all the review comments now 😄 |
Please ensure the following before submitting a PR:
/Rnot/manand rundevtools::document()to update documentation/testsfor any new functionality or bug fixR CMD checkruns without error before submitting the PRThis PR adds support for assuming roles from web identities as requested in #62