-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add IAM based auth for S3 policy repo #691
base: master
Are you sure you want to change the base?
Add IAM based auth for S3 policy repo #691
Conversation
I can't seem to figure out how non-prefixed vars should be read from when using confi.
✅ Deploy Preview for opal-docs canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiesenverseist very nice. I would add tests for that either using some mocks, or given credentials it would test agains specified aws account.
@@ -136,14 +223,34 @@ def build_auth_headers(self, token=None, path=None): | |||
and token is not None | |||
and self.token_id is not None | |||
): | |||
logger.info("Using provided token to log in to AWS_S3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiesenverseist hi, you don't really need elif here, right as there is a return from each case before moving to the next. so you could just have the if's one after the other starting from line 221.
@@ -136,14 +223,34 @@ def build_auth_headers(self, token=None, path=None): | |||
and token is not None | |||
and self.token_id is not None | |||
): | |||
logger.info("Using provided token to log in to AWS_S3") | |||
|
|||
split_url = urlparse(self.remote_source_url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kiesenverseist this is duplicate with the code below, I would refactor the section of both cases of S3 to use common code. right?
Changes proposed
This pull request introduces an additional method for retrieving credentials to access an s3 bucket. When using Amazon's EKS it is possible to create a service account that corresponds to an IAM role. That role ARN and a web identity token file are injected into running pods and can be used with STS to generate temporary credentials.
This PR makes the following changes to accommodate this:
OPAL_
prefix, which is why the default for those fields directly reads from the environment. I'm not sure if this is the best way to do this.build_auth_headers
function. I've added an extra branch where if the ARN and token file are present, it will attempt to use that. I have also made that function async as it now has to read a file, and make requests. I've also added additional logging to indicate which authentication method is being used.get_temporary_sts_credentials
function. This handles the reading of the token file, and sending the request to STS. It also parses the response. This makes use of a time based cache decorator so that the temporary credentials are refreshed on a timer.build_aws_rest_auth_headers
function to include an optional session token. Temporary credentials also include a session token which needs to be included in auth headers when they are used.Check List (Check all the applicable boxes)
Note to reviewers