-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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 assume role with web identity support for the S3 backend #31276
Add assume role with web identity support for the S3 backend #31276
Conversation
I'm going to wait for some feedback on this PR, whether it fits the roadmap, etc. before finalizing this. |
Can't wait <3 |
@bschaatsbergen I don't know how long you are planning on waiting, but you might want to set the PR to 'Ready for review' to get it reviewed by one of the reviewers. |
I'll have to dive into the PR again to sort a few things out. I'll see if I can spend some time on this over the weekend. |
@gdavison and I plan on revisiting this PR soon 👍🏼 |
This capability would be really useful and could reduce a lot of complexity. Any new information to share? Thanks, guys! |
When can we get this functionality in? |
Looking forward to this! Thanks @bschaatsbergen 🥇 |
Hey @bschaatsbergen , thanks for taking the time to develop this, it will be incredibly handy for setting up secure s3 backends : ) Are there any updates on the state of this PR? From my understanding of the feature, it should be ready to review? |
Thanks for reaching out @MattPalladino, I've internally flagged this feature PR within Hashicorp and it's on the list of things to pick up - I believe that the S3 backend is part of the team that maintains the AWS Provider, which is currently busy working on their 5.0.0 release. Tagging @breathingdust for visibility on this. |
Given AWS Provider 5.x is out for almost 2 months now, is there any news on this been released in next coming weeks/months? It would improve security and reduce operational complexity immensely. |
Hi @bschaatsbergen! We have just begun work on our S3 backend maintenance work in which this is included so we should have some movement on this very soon 🚀 |
Ah that's amazing @breathingdust, thanks for the update 👍🏼 |
f4fe0ac
to
56e1363
Compare
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.
Thanks for submitting, @bschaatsbergen. Looks good! 🚀
I've rebased this onto our dev branch, s3/modernization
. If you could either create a new PR on this branch onto s3/modernization
or point this PR at s3/modernization
, we can get it merged into our dev work
Ah great to hear, @gdavison fixed! 👍🏼
|
56e1363
to
28f0381
Compare
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.
LGTM 🚀
% TF_ACC=1 go test -count=1 ./internal/backend/remote-state/s3/...
ok github.com/hashicorp/terraform/internal/backend/remote-state/s3 98.630s
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
Thanks for your contribution, @bschaatsbergen! The |
@jar-b I'm trying to test it with IRSA, using the docker build
Which is the same problem describe in hashicorp/go-getter#313
When I try to use the explicit configurations block:
I've the following error:
|
Hi @manobi - Thanks for the report! It appears the initial implementation in the 1.6 beta is not checking for the presence of the The explicit configuration should work with a minor adjustment in syntax ( assume_role_with_web_identity = {
role_arn = "arn:aws:iam::837424938642:role/example-role"
web_identity_token_file = "/example/tokenfile"
} Please let us know if you continue to see issues with an explicit backend configuration, and thanks again for the bug report. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
This pull request implements the feature requested in #31244.
Summary
Currently the S3 Backend does not support assuming a role with web identity. As we already support assuming a different role as part of the S3 backend this seems like a valid request.
Next to that I think that it's also right to have feature parity with the Terraform AWS provider as they added the OpenID Connect (OIDC) support too recently.
OIDC allows GitHub Actions and other CI/CD pipelines to access resources in Amazon Web Services (AWS), without needing to store the AWS credentials as long-lived GitHub secrets. Security wise this seems like a huge win for those that assume a different role for the S3 backend.
A little heads-up
There seems to be no support yet for
AssumeRoleWithWebIdentity
in the awsbase go package we're using. As support for AssumeRoleWithWebIdentity through awsbase is only added to the awsbase/v2 package.I've refactored a part of the S3 backend code to be compatible with the awsbase/v2 package.
Next to that, to separate the assume role and assume role through web identity configurations I've offloaded the existing attributes to 2 different configuration blocks.
assume_role
andassume_role_with_web_identity
. For both a cleaner and simpler interface as well as to be inline with the existing configuration blocks coming from the Terraform AWS provder.For example the below existing Terraform configuration
Becomes as following:
Similar to the new
assume_role
block, aassume_role_with_web_identity
block has been added to support the feature request.(Optional) backwards compatible - if we want no breaking changes
This doesn't have to be a breaking change, even though existing assume role related attributes are moved to a nested attribute and renamed. We could make the change backwards compatible, the old attributes can still be provided and override the
assume_role
block and a deprecation warning is shown to the user.Implementation
Due to the S3 backend code being fairly outdated, I was forced to refactor a portion of the code to be compatible with the awsbase/v2. Which isn't a bad thing as this helped to stay inline with the Terraform AWS provider, most of the code was years old and up for a refactor nevertheless.
I've created 2 new configuration blocks,
assume_role
andassume_role_with_web_identity_provider
. I've also added a new parameterduration
to theassume_role
block to be inline with theassume_role_with_web_identity_provider
block.To be done
Here some points that I'm still working on:
SkipMetadataApiCheck
as it's been replaced infavor of new tri-state EC2MetadataServiceEnableState -- I have to sort this out to avoid a breaking change...Update S3 backend documentation(if we plan on having backwards compatible)
Tests
To be added..
Example usage
To be added..