-
Notifications
You must be signed in to change notification settings - Fork 540
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 AWS Static Roles support #1877
Add AWS Static Roles support #1877
Conversation
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.
Looking great!
) | ||
|
||
func TestAccDataSourceAWSStaticCredentials(t *testing.T) { | ||
_, _ = testutil.GetTestAWSCreds(t) |
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.
Related to this, which looks a little weird, I imagine:
I found if i included access_key
and secret_key
in the backend specification for the test, it would break vault's ability to get information from IAM ("Invalid security token"). I'm not sure whether this is a bug with my local vault, a possible issue in the static roles implementation in vault, or some other weird local artifact.
If I run solely with access credentials set via env var, things work.
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.
Ah yes, I think this has something to do with how the AWS Secrets Engine always needs the STS token passed as an environment variable.
Going to cc @calvn since they recently confirmed this behavior in the AWS Secrets Engine when the team was working on the static roles implementation for Vault. It doesn't look like token info can be passed to the AWS Config Root endpoint, is that correct?
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.
I looked back at the investigation and this is what I found: There is not a way to provide the session token to the AWS secrets engine to the config other than the aws env var. This is likely intentional since sts creds are not supposed to be long lived and therefore not intended to be used as creds you provide to the root config endpoint (outside of testing purposes). The typical recommendation is to provide IAM user creds and then rotate root creds.
That being said @kpcraig can you check to see if you have any env variables set in your shell session where you vault server and/or TF tests are running? Maybe these are causing issues?
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.
As a followup, it seems like the minimum version that works is having env-vars set in the vault environment, although setting actively incorrect settings in a terraform file will break things.
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.
Should we add a comment explaining why we are calling GetTestAWSCreds but not using the return values?
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.
Also, I don't think I quite follow what the problem is. I see in resource_aws_secret_backend_role_test.go we are passing the values to the config:
accessKey, secretKey := testutil.GetTestAWSCreds(t) |
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.
If my understanding is correct, it has to do with the availability of the AWS_SESSION_TOKEN
env variable which has to be passed to Vault in order to generate credentials properly. I think to avoid confusion and maintain consistency, we can still use the accessKey, secretKey
values returned here and pass them to the TF config to the vault_aws_secret_backend
resource.
In addition, we can also document in a comment that for this test to work, the AWS_SESSION_TOKEN
must also be seeded as an environment variable. Thoughts?
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.
Normally I would pass the values into the HCL even if they did nothing, just to keep parallel with the other aws tests, but I have found that in this case anyway, setting the backend values at all breaks the test, even if they are correct, and match those that the vault instance knows. I find this vexing, otherwise my expectation would match @vinay-gopalan's and @fairclothjm's, that it's only the lack of Session Token, and that having access/secret keys as part of the HCL ought to be harmless.
I'm not sure what the difference is - I imagine the most likely culprit is my environment.
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.
So, after some investigation, it looks like in some cases the Access/Secret pair is insufficient, and the method I was using to obtain credentials required a session token for access. Using my personal AWS account, I was able to get it working, so I've re-added the access/secret fields for the aws backend in the tests.
I've pulled this out of draft to indicate it's ready for deeper review |
using 'backend' is deprecated, but it's probably more important to maintain internal-backend consistency in naming here.
I spent some time trying to decide if there were any cases where omitting them would cause problems, then realized my and my reviewers' time was better spent elsewhere and just added them
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.
Looking great! A couple more comments
I also want to check: this should still be merged against |
Branch updated after @vinay-gopalan's merge of |
The LDAP and build matrix changes should vanish when |
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! Thanks for all the work on this 🙏🏼
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!
This PR adds support for the new (ideally, in vault 1.14) AWS static roles feature.