Skip to content
This repository has been archived by the owner on Mar 5, 2024. It is now read-only.

Allow trailing / on security-credentials endpoint #42

Merged
merged 1 commit into from
Mar 8, 2018

Conversation

elafarge
Copy link
Contributor

@elafarge elafarge commented Mar 5, 2018

This commit makes it possible to reach the
http://169.254.169.254/meta-data/iam/security-credentials/ without the
trailing slash.

The Golang AWS SDK doesn't seem to call the endpoint with a trailing
slash:
https://github.com/aws/aws-sdk-go/blob/b2dc98bb584e48b0f5f39c93110633173c5da43c/aws/credentials/ec2rolecreds/ec2_role_provider_test.go#L37
I guess the implementation varies accross SDKs (the Java SDK, for
instance, seems to make its call with a trailing slash).

As a consequence, Kiam doesn't swallow requests made with this SDK and
simply forwards the request to the original EC2 instance metadata
service, which returns the instance's role name instead of the one set
in the calling pod's annotation.

This commit makes it possible to reach the
http://169.254.169.254/meta-data/iam/security-credentials/ without the
trailing slash.

The Golang AWS SDK doesn't seem to call the endpoint with a trailing
slash:
https://github.com/aws/aws-sdk-go/blob/b2dc98bb584e48b0f5f39c93110633173c5da43c/aws/credentials/ec2rolecreds/ec2_role_provider_test.go#L37
I guess the implementation varies accross SDKs (the Java SDK, for
instance, seems to make its call with a trailing slash).

As a consequence, Kiam doesn't swallow requests made with this SDK and
simply forwards the request to the original EC2 instance metadata
service, which returns the instance's role name instead of the one set
in the calling pod's annotation.
@elafarge elafarge force-pushed the elafarge/fix-trailing-slash-issue branch from ad34412 to 591b67f Compare March 5, 2018 19:00
@Joseph-Irving
Copy link
Contributor

I'd like to see if someone can replicate this, we have a large number of apps using the golang aws sdk with kiam and we have not encountered this problem. That file you've linked to hasn't changed since 2016 so it seems unlikely that a new release has broken this.

@pingles
Copy link
Contributor

pingles commented Mar 7, 2018

@Joseph-Irving I've seen similar stuff noted with kube2iam:

Be worth investigating why we don't see the same behaviour- maybe we've got people using older versions of the SDK?

@pingles
Copy link
Contributor

pingles commented Mar 7, 2018

Given the kube2iam PRs/issues it's probably safe to just include the change so that the trailing slash is optional but agree with @Joseph-Irving it'd be worth checking why we haven't seen this behaviour yet on our clusters.

@pingles
Copy link
Contributor

pingles commented Mar 8, 2018

Having re-read some of the identical kube2iam issues it seems that this kind of problem also depends on instance type- m5 vs m4 shows different behaviour when accessing .../security-credentials- one redirecting and the other returning the instance role name.

@elafarge
Copy link
Contributor Author

elafarge commented Mar 8, 2018

Ooooh great catch @pingles :o I'm running on m5 instances in the new Paris region, I didn't realize it was worth mentionning when I opened this PR.

If I'm correct, according to what AWS announced, generation 5 instances use a "new type of virtualization" and, therefore, a new hypervisor with a new metadata API that behaves in a slightly different way on this endpoint, I guess :/

I ran a few tests out of curiosity:

  • on a c4 or t2 instance in eu-west-1, running curl -v http://169.254.169.254/latest/meta-data/iam/security-credentials gives
> GET /latest/meta-data/iam/security-credentials HTTP/1.1                                                                                                                
> User-Agent: curl/7.38.0                                                                                                                                                
> Host: 169.254.169.254                                                                                                                                                  
> Accept: */*                                                                                                                                                            
>                                                                                                                                                                        
* HTTP 1.0, assume close after body                                                                                                                                      
< HTTP/1.0 301 Moved Permanently                                                                                                                                         
< Location: http://169.254.169.254/latest/meta-data/iam/security-credentials/                                                                                            
< Content-Length: 0                                                                                                                                                      
< Connection: close                                                                                                                                                      
< Date: Thu, 08 Mar 2018 11:23:39 GMT                                                                                                                                    
< Server: EC2ws                                                                                                                                                          
<     
...

while on an m5 instance in eu-west-3, the same command returns the name of the role attached to the instance:

> GET /latest/meta-data/iam/security-credentials HTTP/1.1
> Host: 169.254.169.254
> User-Agent: curl/7.52.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Content-Type: text/plain
< Accept-Ranges: none
< Last-Modified: Thu, 08 Mar 2018 11:24:42 GMT
< Content-Length: 24
< Date: Thu, 08 Mar 2018 11:27:23 GMT
< Server: EC2ws
< Connection: close
< 
* Curl_http_done: called premature == 0
* Closing connection 0
<name_of_the_role_attached_to_the_instance>

With all that in mind, I don't think that having kiam return the role attached to the pod on both endpoints (with and without trailing slash) would a problem though. Actually, it even prevents an unecessary 301 redirect on old instance types :)

@pingles pingles merged commit 591b67f into uswitch:master Mar 8, 2018
@pingles
Copy link
Contributor

pingles commented Mar 8, 2018

Thanks- I've merged this now.

I'm not going to tag it until we've had a chance to run this on our clusters but you can use immediately with either of these tags:

  • quay.io/uswitch/kiam:master
  • quay.io/uswitch/kiam:6c061b6b326a5d7ce253bfd17deecb76e7f809e1

Thanks!

@elafarge
Copy link
Contributor Author

elafarge commented Mar 8, 2018

Great :) Thanks for the Docker images, I'll quietely wait for the next release though.

Thanks again for developing kiam, for updating the tests wrt. this PR and for being so responsive (I wasn't expecting this to be merged before the end of the week :D ).

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

Successfully merging this pull request may close these issues.

3 participants