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

Fix IAM issue when using mock credential provider #5014

Closed
wants to merge 1 commit into from

Conversation

akazakov
Copy link

@akazakov akazakov commented Feb 5, 2016

Example: Hologram (by adroll) does not implement all the handlers on
http://169.254.169.254/
instead it only responds to the IAM related urls, such as

http://169.254.169.254/latest/meta-data/iam/security-credentials/
Therefore change it to be the url we connect to.
To determine if the IAM provider is available we simply check for no error on GET for that url.

Example: Hologram (by adroll) does not implement all the handlers on
http://169.254.169.254/
instead it only responds to the IAM related urls, such as

http://169.254.169.254/latest/meta-data/iam/security-credentials/
Therefore change it to be the url we connect.
@ikalinin
Copy link

ikalinin commented Feb 9, 2016

@akazakov I have also made Hologram mimic the behaviour terraform is expecting here AdRoll/hologram#71

@akazakov
Copy link
Author

@ikalinin thanks!

@akazakov
Copy link
Author

akazakov commented Mar 1, 2016

@radeksimko Please let me know if I need to do something here to help this get merged.

@copumpkin
Copy link

cc @catsby

Given that hologram has made this change, this isn't as important anymore, but I'm still trying to understand your motivation for this:

        // AWS will add a "Server: EC2ws" header value for the metadata request. We     
        // check the headers for this value to ensure something else didn't just        
        // happent to be listening on that IP:Port

What other things do you expect to be listening on an obscure link-local/unroutable IP address? Is this intended as a GCE vs. EC2 check? It seems unlikely that one would find anything else that happens to be listening on that IP:port that also responds sensibly to the meta-data/iam/security-credentials/ path, right?

@radeksimko
Copy link
Member

Hi all,

What other things do you expect to be listening on an obscure link-local/unroutable IP address?

I'd say potentially some things may listen on the APIPA range if you're running terraform off your laptop as the network conditions change often.

The current check is likely to change, as AWS said the header we're checking is not documented and may not work in the future. However I'm still in favour of some kind of checks as well as @catsby is (I believe):

The odds of that are small, but small odds have bitten me in the past

I think the recently suggested way of checking in the linked issue would probably make it slightly easier for Hologram to implement:
http://docs.aws.amazon.com/AWSEC2/latest/UserGuide/instance-identity-documents.html

as far as I understand the docs we could just check that http://169.254.169.254/latest/dynamic/instance-identity/document returns something like this (or just any valid JSON):

{
  "devpayProductCodes" : null,
  "availabilityZone" : "us-east-1d",
  "privateIp" : "10.158.112.84",
  "version" : "2010-08-31",
  "region" : "us-east-1",
  "instanceId" : "i-3d168483",
  "billingProducts" : null,
  "instanceType" : "t1.micro",
  "accountId" : "123456789012",
  "pendingTime" : "2015-11-19T16:32:11Z",
  "imageId" : "ami-5fb8c835",
  "kernelId" : "aki-919dcaf8",
  "ramdiskId" : null,
  "architecture" : "x86_64"
}

I have also refactored the auth code over in #5030 - it is currently missing the checks - effectively includes the change in this PR, but as I mentioned, I'd like to put some checks back while keeping the code clean (e.g. not having the metadata API endpoint URL hardcoded, but relying on the SDK instead).

If AWS says they don't want that built into the SDK, I'm likely going to add such check into #5030 .
I may also separate out the PR into multiple PRs per commits so it's easier to review & merge.

@radeksimko
Copy link
Member

I'm currently working on the mentioned verification logic in upstream (AWS Go SDK) and I was re-reading my code in #5030 and I think we can come up with a compromise.

By default Terraform will keep verifying authenticity of EC2 metadata endpoint, but will also allow the user to specify the endpoint URL via ENV variable AWS_METADATA_ENDPOINT. If user decides to do so Terraform won't perform any checks of that endpoint. The ENV variable can obviously be still the same as the default endpoint URL.

What do you think of the proposed solution?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Mar 13, 2016
@radeksimko radeksimko changed the title Fix IAM ussue when using mock credential provider Fix IAM issue when using mock credential provider Mar 25, 2016
@radeksimko
Copy link
Member

Hi all,
this PR is not necessary anymore since #5030 was merged.

The refactored AWS credentials provider isn't checking headers anymore. It instead uses standard way of verifying EC2 metadata endpoint as recommended by AWS:
https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/auth_helpers.go#L118

This method seems to be supported by Hologram, so no changes should be necessary on that side either - the next release 0.6.16 should just work with Hologram.

Thank you very much for bringing our attention to this.

Also see #6535

@radeksimko radeksimko closed this May 8, 2016
@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 8, 2016
@ghost
Copy link

ghost commented Apr 26, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants