provider/aws: Allow account ID checks on EC2 instances & w/ federated accounts

[radeksimko]

I'm building on top of @ColinHebert 's PR and also recently merged PR in AWS SDK which made this fairly clean approach possible.

This is closing #3431

Here's an overview of the implemented approaches per provider:

provider/approaches	1st	2nd	3rd
EC2RoleProvider	Metadata API	ERR
EnvProvider	iam:GetUser	iam:ListRoles	ERR
SharedCredentialsProvider	iam:GetUser	iam:ListRoles	ERR
StaticCredentialsProvider	iam:GetUser	iam:ListRoles	ERR
CustomProvider	iam:GetUser	iam:ListRoles	ERR

I would love to add some tests for getAccountId() but mocking the IAM service seems quite tricky at the moment.

I have tested all of the providers (launched EC2 instance w/ instance profile + test all other providers each locally).

[ColinHebert]

Will this work? Don't we need to add /meta-data/iam/info? AWS_METADATA_URL used to be ts.URL+"/latest"
If it's deprecated, could failing completely here be an option?

[radeksimko]

Well, my assumption is that this has only been used in tests and never documented, so I was even thinking of just removing this ENV variable. Erroring out is another option. Unfortunately we don't have any nice way to return warnings from this scope.

[radeksimko]

AWS_METADATA_URL used to be ts.URL+"/latest"

Correct, that is why am I treating it differently and just take the whole URL, instead of building it.
That function is used in two places, have a look into the full diff - we call this API first when deciding whether to use EC2Role provider at all and then I used it here (with different path) to get the role ARN and Account ID from it.

[ColinHebert]

I'd be all for removing that Env variable, if it isn't documented (plus given the nature of the variable) it's safe to assume it hasn't been used and is safe to remove.

[radeksimko]

@ColinHebert I think I addressed all of your comments. I have also separated the PR into more commits to make each refactoring phase isolated and more readable. Let me know if you see anything else.

Pinging @catsby as he was behind the recent December refactoring of this. Can you specifically look at eeea864 , please? As @ColinHebert suggested, this seems to be much cleaner approach to address that problem with metadata API timeout (which is 5 secs by default).

btw. after this refactoring which makes the code a bit cleaner, there's one concern left, see aws/aws-sdk-go#543

[ColinHebert]

This is so awesome! Nice work @radeksimko!

[radeksimko]

This is currently on hold.

I'm waiting for response from AWS in regards to suggestions on best way to verify EC2 metadata API endpoint: aws/aws-sdk-go#543 , https://forums.aws.amazon.com/thread.jspa?threadID=226140

[radeksimko]

@catsby I will refactor this and then retest on real EC2 instance and w/ real federated credentials.

[radeksimko]

This is now refactored. I also spent some time creating mock IAM server which allowed me writing couple of tests.

Besides those tests I also tested this manually with real federated credentials and in a real EC2 instance w/ instance profile. Both worked ok.

I also exposed both methods (GetCredentials and GetAccountId) as public to make it useful for #5270

[radeksimko]

This is now squashed into 2 commits (vendor + actual changes), as discussed w/ @catsby

The plan is to review & merge this after the next release (0.6.15). This is to give more time for testing of such a sensitive part of the provider "in the wild" to allow capturing any potential edge-cases/bugs that may not have been covered by tests.

[catsby]

LGTM, unit tests and I ran some acceptance tests locally which passed, I think we're good to merge here thanks!

[radeksimko]

Just FYI: Although account ID checking by itself does work with EC2 instances and federated credentials now, Terraform still calls iam:GetUser as part of credentials validation. This still fails on EC2 & w/ federated credentials.

Feel free to subscribe to #6523 where we track this issue.

[radeksimko]

Actually, apologies, what I said is not entirely true, it does work for some cases which can be characterised by the following error check for iam:GetUser:

if awsErr.Code() == "AccessDenied" || awsErr.Code() == "ValidationError" {

The linked issue & upcoming PR will hopefully actually validate the credentials at all cases instead of ignoring the check though, so the note is still partially valid.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.