Skip to content
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

Allow use of IAM Instance Roles #138

Closed
wants to merge 3 commits into from
Closed

Conversation

akremer
Copy link

@akremer akremer commented Mar 9, 2023

I've been following the discussion in #131 and the associated pull in #133. I too am in need of this functionality, but believe the attached approach is better.

It introduces an optional useInstanceRole boolean in the configuration object, which drops the access key & secret, and makes the region optional. Introducing this optional boolean allows all existing tests and approaches to work, rather than simply making the access key & secret optional.

This solution also covers the call to STS (which was missed in #133) as well as adds a test which can be run by setting the INSTANCE_ROLE_TEST environment variable for Cypress.

@vercel
Copy link

vercel bot commented Mar 9, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
next-s3-upload ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Mar 9, 2023 at 11:01PM (UTC)

@ryanto
Copy link
Owner

ryanto commented Mar 10, 2023

Awesome, this looks great! Just FYI expect the tests to fail because github does not share the secrets that upload to S3 with PRs. I'll pull this down and run locally.

Were you able to deploy this code to an EC2 instance and get the access/uploading working correct?

@akremer
Copy link
Author

akremer commented Mar 11, 2023

Well, the good news is, I finally got all the tests to remotely run on an ec2 instance. The bad news is, this entire concept won't work. The reason is that IAM Instance Roles use temporary credentials themselves, and so that call to STS to get a temporary token for the upload will fail since temporary credentials can't call STS for other temporary credentials.

There is potentially a way to use AssumeRole instead of GetFederationToken to get around this, but this would introduce a ton of complexity in terms of setup and code. I'm going to have to accept defeat here and use tokens for this use case.

@akremer akremer closed this Mar 11, 2023
@ryanto
Copy link
Owner

ryanto commented Mar 11, 2023

Bummer :(

Really appreciate you taking the time to do this and write everything up. Thanks!

@ryanto
Copy link
Owner

ryanto commented Apr 20, 2023

Hey @akremer had a question about this PR... I know STS makes this kind of a stuff difficult, but do you think this PR would work with presigned uploads? The presigned uploads don't use STS, they instead generate a signed post url using the S3 client.

Any thoughts? Think this would a nice middle ground.

@akremer
Copy link
Author

akremer commented Apr 20, 2023

I don't see why that wouldn't work. The only test that failed in this PR once I got them running was the call to STS, so a presigned upload would probably work flawlessly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants