Add explicit option for using iam profile for authentication #107

JamesAwesome · 2015-04-07T07:04:01Z

PR #104 broke the ability to use kitchen-ec2 with explicit aws credentials set via .kitchen.yml while on an ec2 host.

When on an ec2 host the #fetch_credentials method which is included from Fog::AWS::CredentialFetcher::ServiceMethods will always return a hash with a value set for the :aws_session_token key. Because of this config[:aws_session_token] will always default to iam_creds[:aws_session_token]. Since config[:aws_session_token] is passed to #connection, Fog::Compute fails to authenticate using any credentials set in .kitchen.yml since the value of :aws_session_token is incorrect.

By adding an option to explicitly use a server's iam profile we can allow hosts which live in ec2 to use .kitchen.yml as well as their iam profile for driver configuration.

* Add 'use_iam_profile' config option, default to false * Have #iam_creds return an empty hash unless use_iam_profile is true * Call iam_creds as driver.iam_creds in `default_config` blocks * Rescue fron NoMethodError in iam_creds `Fog::AWS::CredentialFetcher::ServiceMethods::fetch_credentials` will call super when it fails. Because there is no superclass method this will throw a NoMethodError * Write unit tests for #iam_creds

JamesAwesome · 2015-04-07T13:15:17Z

@tyler-ball Finally got these tests to pass. Sorry for all the chatter.

Igorshp · 2015-04-07T14:12:46Z

@JamesAwesome there is already a 'iam_profile_name' param that assigns the named role to the new node.

I think 'use_iam_profile' may be little misleading next to it.

Would be nice to do the detection automaticaly.

JamesAwesome · 2015-04-07T14:42:45Z

@Igorshp we could change the name to something like :use_iam_role however if we try to autodetect while on an ec2 node we will always receive some form of value for iam_creds[:aws_session_token], whether or not that value is valid.

Igorshp · 2015-04-07T14:50:14Z

I mean the autodetection should check if key_id and/or secret are manualy set and return session_token accordingly.

JamesAwesome · 2015-04-07T14:51:45Z

Might be able to whip that up, I'll futz with this a bit more and update

tyler-ball · 2015-04-07T15:01:14Z

lib/kitchen/driver/ec2.rb

-          fetch_credentials(use_iam_profile:true)
-        rescue RuntimeError => e
+          config[:use_iam_profile] ? fetch_credentials(use_iam_profile: true) : {}
+        rescue RuntimeError, NoMethodError => e


Where do you expect the NoMethodError to come from?

fetch_credentials calls super when it fails, so since we don't have a super for it to fail over to we need to catch NoMethodError, I think I made a note in the commit message.

tyler-ball · 2015-04-07T15:10:05Z

Thanks for adding a test and turning fetch_credentials into an instance method rather than class method. I think it makes more sense for our use case and I didn't think of that.

I'm not familiar enough with AWS - is there a scenario where you would ever set access_key_id/secret_access_key AND session_token? Or are they mutually exclusive forms of authentication?

If you can't use both of them in 1 auth call I agree with @Igorshp. We should detect the presence of access_key_id/secret_access_key and only provide those if available. If those are not available, use the session_token. In this sense, the access_key (if available) overrides the session_token. I think this makes sense because you can simply not provide those and rely on fetch_credentials if you want to use the session_token.

I would love to get @fnichol's opinion on this.

Igorshp · 2015-04-07T15:13:58Z

@tyler-ball the core of the problem is the fact that you can only use one set of credentials that comes from one source.

i.e. either all are from iam_creds or all are user defined (session token can be user provided, but i've never seen it used like that. default is nil), because they are not compatible with each other.

so if even one piece of credentials is manually set, the rest should not be pulled from iam_role.

* Remove :use_iam_profile config setting * Update tests to reflect autodetection vs use of :use_iam_profile * Add `#default_aws_session_token` method to handle logic for setting default value of config[:aws_session_token] Since the :use_iam_profile parameter is confusing instead autodetect if `iam_creds` should be used for setting config[:aws_session_token] by checking if other config settings match the values returned from `iam_creds`. If the config settings match then it's a safe bet that iam_creds should be used for setting config[:aws_session_token].

JamesAwesome · 2015-04-07T16:28:16Z

@tyler-ball whoops I regressed the include to extend at some point. Corrected it. There are in fact instances where session token makes sense however they seem very edge. See docs here.

Auto detection should work fine now.

Igorshp · 2015-04-07T16:31:43Z

👍 sweet, thanks!

JamesAwesome · 2015-04-07T17:39:48Z

Ready for review. No more commits for now.

moimikey · 2015-04-07T20:11:59Z

tyler-ball · 2015-04-08T19:57:09Z

lib/kitchen/driver/ec2.rb

      end
      default_config :aws_secret_access_key do |driver|
-        ENV['AWS_SECRET_KEY'] || ENV['AWS_SECRET_ACCESS_KEY'] || iam_creds[:aws_secret_access_key]
+        ENV['AWS_SECRET_KEY'] || ENV['AWS_SECRET_ACCESS_KEY'] \


If you want to break this across multiple lines you don't need the \ - just leave the || at the end of the line

tyler-ball · 2015-04-08T20:39:46Z

lib/kitchen/driver/ec2.rb

+        if config[:aws_secret_access_key] == iam_creds[:aws_secret_access_key] \
+          && config[:aws_access_key_id] == iam_creds[:aws_access_key_id]
+          iam_creds[:aws_session_token]
+        else


Why is the user required to specify the same access key / secret as returned by the iam_creds query? If they have to do that, why not just use the provided access key / secret for authentication?

I thought the primary point of the iam_creds was they allowed the EC2 node acting as a provisioner to provision nodes using the local credentials.

Actually I'm just checking that config[:aws_secret_access_key] et all had fallen back to the iam_creds statement so that we know it would be logical to return the iam_creds[:aws_session_token] since our other config values are from iam.

tyler-ball · 2015-04-08T22:09:32Z

When running on a non-ec2 node it takes a really long time to figure out the metadata service isn't available. With that in mind I created the following patch:

diff --git a/lib/kitchen/driver/ec2.rb b/lib/kitchen/driver/ec2.rb
index e8df7af..de5eaf9 100644
--- a/lib/kitchen/driver/ec2.rb
+++ b/lib/kitchen/driver/ec2.rb
@@ -41,14 +41,16 @@ module Kitchen
       default_config :iam_profile_name,   nil
       default_config :price,   nil
       default_config :aws_access_key_id do |driver|
-        ENV['AWS_ACCESS_KEY'] || ENV['AWS_ACCESS_KEY_ID'] || driver.iam_creds[:aws_access_key_id]
+        ENV['AWS_ACCESS_KEY'] || ENV['AWS_ACCESS_KEY_ID'] ||
+          driver.iam_creds[:aws_access_key_id]
       end
       default_config :aws_secret_access_key do |driver|
         ENV['AWS_SECRET_KEY'] || ENV['AWS_SECRET_ACCESS_KEY'] ||
           driver.iam_creds[:aws_secret_access_key]
       end
       default_config :aws_session_token do |driver|
-        driver.default_aws_session_token
+        ENV['AWS_SESSION_TOKEN'] || ENV['AWS_TOKEN'] ||
+          driver.iam_creds[:aws_session_token]
       end
       default_config :aws_ssh_key_id do |driver|
         ENV['AWS_SSH_KEY_ID']
@@ -98,10 +100,18 @@ module Kitchen
         end
       end

+      # First we check the existence of the metadata host.  Only fetch_credentials
+      # if we can find the host.  Ping logic taken from
+      # http://stackoverflow.com/questions/7519159/ruby-ping-pingecho-missing
       def iam_creds
+        require 'net/http'
+        require 'timeout'
         @iam_creds ||= begin
+          timeout(5) do
+            Net::HTTP.get(URI.parse('http://169.254.169.254'))
+          end
           fetch_credentials(use_iam_profile: true)
-        rescue RuntimeError, NoMethodError => e
+        rescue Errno::EHOSTUNREACH, Timeout::Error, StandardError => e
           debug("fetch_credentials failed with exception #{e.message}:#{e.backtrace.join("\n")}")
           {}
         end
@@ -167,15 +177,6 @@ module Kitchen
         !!config[:subnet_id]
       end

-      def default_aws_session_token
-        if config[:aws_secret_access_key] == iam_creds[:aws_secret_access_key] \
-          && config[:aws_access_key_id] == iam_creds[:aws_access_key_id]
-          iam_creds[:aws_session_token]
-        else
-          ENV['AWS_SESSION_TOKEN'] || ENV['AWS_TOKEN']
-        end
-      end
-
       private

       def connection

I think thats a better solution for this case. If we can access the metadata service, we do.

JamesAwesome · 2015-04-08T22:25:38Z

My only concern is that http://169.254.169.254 is a generic link-local address and also used to serve metadata for clouds like openstack.

We could work around this by letting fetch_credentials fail and rescue NoMethodError but there may be a better way to check. Will take a look.

tyler-ball · 2015-04-08T23:31:25Z

@JamesAwesome Ah, I see your concern - drat that people are starting to use Test Kitchen in their CI workflow from multiple cloud providers! 😄

But even if we can connect to http://169.254.169.254, fetch_credentials should fail on non-ec2 nodes because it is requesting the path /latest/meta-data/iam/security-credentials/. In this case we will fall through to the NoMethodError rescue.

My concern is that I don't want to wait a full minute for all ec2 creates like the person in #104 is experiencing.

JamesAwesome · 2015-04-09T00:33:01Z

Yeah I'm perfectly happy with that then if you are. 👍 I'll merge in the patch and get the tests adjusted.

JamesAwesome · 2015-04-09T15:56:08Z

So problem:

Because this patch will call driver.iam_creds[:aws_session_token] if config[:aws_session_token] is unset in .kitchen.yml or not present in the environment we are back in the position where if you have explicitly set :aws_access_key_id or :aws_secret_access_key and are running on an ec2 host you will have an unwanted and incorrect :aws_session_token and authentication will fail.

We can deal with this by comparing the configured :aws_access_key_id against the value returned by iam_creds like I do in the current commit, however I'd like to hear @tyler-ball 's opinion on that before I continue so that we can solve this a bit quicker. I'll think on the topic a bit and comment if I come up with anything a bit more clever.

JamesAwesome · 2015-04-09T19:19:04Z

Updated (failing) tests illustrate the problem

* only set :aws_session_token via iam_creds in the event that :aws_access_key_id and :aws_secret_access_key are BOTH already set via iam_creds * prefer environment over iam_creds when setting :aws_session_token I think this is the best I can do. All unprevented combinations of autodetectable credentials via iam_creds and user defined settings should simply fail authentication.

JamesAwesome · 2015-04-10T12:13:58Z

I am now 100% happy with this.

I had to remove the top level before block and move it into appropriate context's since it was calling instance too soon which was causing my tests to fail.

@tyler-ball I promise no more commits unless you comment. 😄

#107

tyler-ball · 2015-04-10T15:57:59Z

@JamesAwesome I did a little bit of test refactoring / cleanup when I merged, but thank you for tackling this issue and doing all the work!

JamesAwesome · 2015-04-10T16:15:23Z

👍 😄

Igorshp · 2015-04-10T16:20:50Z

👍 Thanks

JamesAwesome force-pushed the master branch from ae87e1d to 69d4f41 Compare April 7, 2015 13:09

tyler-ball reviewed Apr 7, 2015
View reviewed changes

James Awesome added 2 commits April 7, 2015 12:16

Use include not extend for Fog::AWS::CredentialFetcher::ServiceMethods

adc082d

Remove unused let() block from tests

389522c

JamesAwesome mentioned this pull request Apr 8, 2015

Add support for IAM role credentials #104

Merged

tyler-ball reviewed Apr 8, 2015
View reviewed changes

Stylefix, remove unnecessary "\"

aaaa892

tyler-ball reviewed Apr 8, 2015
View reviewed changes

Update tests for #iam_creds

1945879

Ensure raising top namespace StandardError from rspec-mock

9853526

Better test name spacing, correct false positive

770557e

JamesAwesome force-pushed the master branch from 7cc697c to 770557e Compare April 9, 2015 20:19

James Awesome added 6 commits April 9, 2015 16:57

Add tests for when iam_creds should be used

f13c073

Add even MORE tests

d352cb4

Since autodetecting iam is all or nothing update specs to reflect that

40ce055

Adjust tests to pass in travis

16302ab

Correct order of stubbing to allow iam_creds to work in test suite

ac3d1c4

JamesAwesome force-pushed the master branch from 919dde5 to ac3d1c4 Compare April 10, 2015 12:09

tyler-ball added a commit that referenced this pull request Apr 10, 2015

Adding changelog and cleaning up tests for #107

9018887

tyler-ball merged commit ac3d1c4 into test-kitchen:master Apr 10, 2015

tyler-ball added a commit that referenced this pull request Apr 10, 2015

Merge branch 'JamesAwesome-master', closes PR

a4e6838

#107

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add explicit option for using iam profile for authentication #107

Add explicit option for using iam profile for authentication #107

JamesAwesome commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

tyler-ball Apr 7, 2015

JamesAwesome Apr 7, 2015

tyler-ball commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

moimikey commented Apr 7, 2015

tyler-ball Apr 8, 2015

tyler-ball Apr 8, 2015

JamesAwesome Apr 8, 2015

tyler-ball commented Apr 8, 2015

JamesAwesome commented Apr 8, 2015

tyler-ball commented Apr 8, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 10, 2015

tyler-ball commented Apr 10, 2015

JamesAwesome commented Apr 10, 2015

Igorshp commented Apr 10, 2015

Add explicit option for using iam profile for authentication #107

Add explicit option for using iam profile for authentication #107

Conversation

JamesAwesome commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

tyler-ball Apr 7, 2015

Choose a reason for hiding this comment

JamesAwesome Apr 7, 2015

Choose a reason for hiding this comment

tyler-ball commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

Igorshp commented Apr 7, 2015

JamesAwesome commented Apr 7, 2015

moimikey commented Apr 7, 2015

tyler-ball Apr 8, 2015

Choose a reason for hiding this comment

tyler-ball Apr 8, 2015

Choose a reason for hiding this comment

JamesAwesome Apr 8, 2015

Choose a reason for hiding this comment

tyler-ball commented Apr 8, 2015

JamesAwesome commented Apr 8, 2015

tyler-ball commented Apr 8, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 9, 2015

JamesAwesome commented Apr 10, 2015

tyler-ball commented Apr 10, 2015

JamesAwesome commented Apr 10, 2015

Igorshp commented Apr 10, 2015