Extensible Credential Providers #1820

YashoSharma · 2018-07-03T18:53:09Z

Adds process based credential providers into credential resolution chain
Adds unit tests for credential provider process declarations in config files

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

awood45

I'm not sure about the approach here. It would seem to make more sense for this to actually be its own credential provider. While Shared Config would be a place for the credential_process flag, it should be its own provider in its own class with its own tests.

awood45 · 2018-07-03T19:34:46Z

There are other benefits to it being a standalone credential provider - for example, it would allow users to invoke it directly, rather than as a shared configuration side effect.

awood45 · 2018-07-05T21:16:22Z

gems/aws-sdk-core/lib/aws-sdk-core/credential_provider_chain.rb

+        else
+          ProcessCredentials.new(profile_name: ENV['AWS_PROFILE'].nil? ? 'default' : ENV['AWS_PROFILE'])
+        end
+      end


I'd make the "else nil" explicit here.

awood45 · 2018-07-05T21:20:35Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+    include CredentialProvider
+    include RefreshingCredentials
+
+    def initialize(options = {})


I don't think this is the way to go about this either - I think when this credential provider is created, you should be able to specify the process identifier directly. You can fetch that value in the credential provider chain approach and pass it in to this constructor.

Essentially, we shouldn't strictly require that a shared config file even exists.

awood45 · 2018-07-05T21:24:38Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+
+      @expiration = creds_json['Expiration'] ? Time.iso8601(creds_json['Expiration']) : nil
+      return creds if creds.set?
+      raise ArgumentError.new("Invalid payload for JSON credentials version 1")


I'd use a SDK typed exception here, as above. Similar to https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/Errors/MissingCredentialsError.html but not that exception. Perhaps Aws::Errors::ProcessCredentialsError?

awood45 · 2018-07-05T21:25:29Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+    end
+
+    def refresh
+      proc = Aws.shared_config.credentials_process(@profile_name)


Again, we need to not source from shared config within this class. It's also static, so the place to set this is in the constructor.

proc is also a reserved word, best not to be used as a variable name.

awood45 · 2018-07-05T21:27:29Z

gems/aws-sdk-core/lib/aws-sdk-core/credential_provider_chain.rb

+    def process_credentials(options)
+      if Aws.shared_config.config_enabled?
+        if options[:config]
+          ProcessCredentials.new(profile_name: options[:config].profile)


We should make the "is process credentials enabled by shared config" decision fully within this logic section. If so, we create a ProcessCredentials object and expect it to work. If not, we return nil and move on.

cjyclaire · 2018-07-05T22:22:09Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+    include CredentialProvider
+    include RefreshingCredentials
+
+    def initialize(process)


could you add more documentation for #initialize like what kind of process variable it's expecting?

cjyclaire · 2018-07-05T22:23:48Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+      @credentials = credentials_from_process(@process)
+    end
+
+    # @return [Credentials]


Aws::Credentials?

cjyclaire · 2018-07-05T22:29:53Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+
+    include CredentialProvider
+    include RefreshingCredentials
+


Could you also add some doc here like an example of how to use this?
e.g https://docs.aws.amazon.com/sdk-for-ruby/v3/api/Aws/AssumeRoleCredentials.html

cjyclaire · 2018-07-05T22:31:22Z

gems/aws-sdk-core/spec/aws/process_credentials_spec.rb

@@ -0,0 +1,43 @@
+require_relative '../spec_helper'
+


Test suites looks good, just checking does these covered all cli test suits for the process credentials as well?

cjyclaire · 2018-07-05T22:36:45Z

gems/aws-sdk-core/lib/aws-sdk-core/errors.rb

      end
    end

+    # Raised when a credentials provider process returns a JSON


surprise! ;) you need to update the region line here: https://github.com/aws/aws-sdk-ruby/blob/master/build_tools/spec/region_spec.rb#L12

That spec is for testing we don't hard code region names, since the error file has been modified, the line number need to be modified as well :D That' why you are seeing the travis error

awood45

This looks much better - I'd address Jingyi's comments on documentation in AssumeRoleCredentials (in fact, double bonus for explicitly testing the use case of plugging in a process credential provider there, since that's by far the most common use case for this), and for double-checking against the SEP definition.

awood45 · 2018-07-06T22:00:08Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+    private
+    def credentials_from_process(proc_invocation)
+      begin
+        raw_out = `#{proc_invocation}`


My last point is for us to take another pass on the process invocation choice. Backticks run this way are going to be unescaped, and while we're taking in the data from the credentials file, we may want to be cautious. Take a look at Open3 and let's see if Open3.capture2 works equally well.

awood45 · 2018-07-23T22:33:26Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+  # provided in the credentials payload
+  #
+  #     credentials = Aws::ProcessCredentials.new(
+  #                   'echo \'{


Let's use a different example, closer to what we would want to do. A hard-coded echo with hard-coded credentials would never be recommended. It's just meant to be an illustration, but some may take it literally.

awood45 · 2018-07-24T17:02:08Z

gems/aws-sdk-core/lib/aws-sdk-core/process_credentials.rb

+    # @param [String] process Invocation string for process
+    # credentials provider. 
+    def initialize(process)
+      @process = process


We're close now, at this point we just need to make sure we're splitting and escaping the string in the same way Python/CLI does: https://github.com/boto/botocore/blob/25a6a4c7c8a8b1797fc78c6a34dc7bca3f0bfba1/botocore/credentials.py#L793-L796

awood45 · 2018-07-31T16:38:04Z

So the last thing we need before merging is to add credential provider chain tests to cover what should and should not happen (making sure this is the end of the credential stack, ensuring it's a terminal state).

awood45 · 2018-08-01T20:00:07Z

gems/aws-sdk-core/spec/aws/credential_resolution_chain_spec.rb

        expect(client.config.credentials.access_key_id).to eq("ACCESS_KEY_SC1")
      end

+      it 'prefers process credentials over metadata credentials' do


Last bit here, I would also add some tests with this same profile where we ensure that static profile credentials, for example, are pulled in first. It's a bit thorough, but important for regression.

… process

awood45

I'll do the merge work on this one

Yash Sharma added 4 commits July 2, 2018 18:34

Initial implementation of process based credential resolution

ed3de2c

Initial implementation of process based credential resolution

361edb5

Added test cases for process based credential providers

4f993f7

Updated Changelog

363efc3

awood45 suggested changes Jul 3, 2018

View reviewed changes

Yash Sharma added 2 commits July 3, 2018 17:05

Added process credentials to separate provider

41b9568

Added profile error handling

f67e0a4

awood45 suggested changes Jul 5, 2018

View reviewed changes

Yash Sharma added 4 commits July 5, 2018 14:46

Hoisted resolution of process provider into credential provider chain

fd2dc8d

Use SDK typed exceptions

b5547d8

Updated docs

60e579d

Revert changes to unchanged files

f93fedb

cjyclaire reviewed Jul 5, 2018

View reviewed changes

Yash Sharma added 3 commits July 6, 2018 09:49

Added documentation, fixed broken test

8fe3686

Added docs

4733763

Improved error handling

3c6e28d

awood45 reviewed Jul 6, 2018

View reviewed changes

Yash Sharma added 2 commits July 6, 2018 15:11

Use open3 for process invocation instead of backticks

c43d6b6

removed bad usage example

dfcefd8

awood45 suggested changes Jul 24, 2018

View reviewed changes

Added credential resolution chain test

e2d30c2

awood45 suggested changes Aug 1, 2018

View reviewed changes

Added regression test to ensure direct credentials are preferred over…

ab8b132

… process

awood45 approved these changes Aug 2, 2018

View reviewed changes

Merge branch 'master' into feature/ProcessCredentialProvider

365569d

awood45 merged commit ddd927a into aws:master Aug 2, 2018

YashoSharma deleted the feature/ProcessCredentialProvider branch August 2, 2018 22:12

YakDriver mentioned this pull request Oct 25, 2018

aws/credential: Add credential_process provider aws/aws-sdk-go#2217

Merged

Extensible Credential Providers #1820

Extensible Credential Providers #1820

Uh oh!

Conversation

YashoSharma commented Jul 3, 2018

Uh oh!

awood45 left a comment

Choose a reason for hiding this comment

Uh oh!

awood45 commented Jul 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awood45 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awood45 commented Jul 31, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awood45 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants