-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Extensible Credential Providers #1820
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
Changes from 17 commits
ed3de2c
361edb5
4f993f7
363efc3
41b9568
f67e0a4
fd2dc8d
b5547d8
60e579d
f93fedb
8fe3686
4733763
3c6e28d
c43d6b6
dfcefd8
e2d30c2
ab8b132
365569d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,10 @@ def initialize(*args) | |
| end | ||
| end | ||
|
|
||
| # Raised when a credentials provider process returns a JSON | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| # payload with either invalid version number or malformed contents | ||
| class InvalidProcessCredentialsPayload < RuntimeError; end | ||
|
|
||
| # Raised when a client is constructed and region is not specified. | ||
| class MissingRegionError < ArgumentError | ||
| def initialize(*args) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| require 'open3' | ||
|
|
||
| module Aws | ||
|
|
||
| # A credential provider that executes a given process and attempts | ||
| # to read its stdout to recieve a JSON payload containing the credentials | ||
| # | ||
| # Automatically handles refreshing credentials if an Expiration time is | ||
| # provided in the credentials payload | ||
| # | ||
| # credentials = Aws::ProcessCredentials.new('/usr/bin/credential_proc').credentials | ||
| # | ||
| # ec2 = Aws::EC2::Client.new(credentials: credentials) | ||
| # | ||
| # More documentation on process based credentials can be found here: | ||
| # https://docs.aws.amazon.com/cli/latest/topic/config-vars.html#sourcing-credentials-from-external-processes | ||
| class ProcessCredentials | ||
|
|
||
| include CredentialProvider | ||
| include RefreshingCredentials | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you also add some doc here like an example of how to use this? |
||
| # Creates a new ProcessCredentials object, which allows an | ||
| # external process to be used as a credential provider. | ||
| # | ||
| # @param [String] process Invocation string for process | ||
| # credentials provider. | ||
| def initialize(process) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could you add more documentation for |
||
| @process = process | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| @credentials = credentials_from_process(@process) | ||
| end | ||
|
|
||
| private | ||
| def credentials_from_process(proc_invocation) | ||
| begin | ||
| raw_out, process_status = Open3.capture2(proc_invocation) | ||
| rescue Errno::ENOENT | ||
| raise Errors::InvalidProcessCredentialsPayload.new("Could not find process #{proc_invocation}") | ||
| end | ||
|
|
||
| if process_status.success? | ||
| creds_json = JSON.parse(raw_out) | ||
| payload_version = creds_json['Version'] | ||
| if payload_version == 1 | ||
| _parse_payload_format_v1(creds_json) | ||
| else | ||
| raise Errors::InvalidProcessCredentialsPayload.new("Invalid version #{payload_version} for credentials payload") | ||
| end | ||
| else | ||
| raise Errors::InvalidProcessCredentialsPayload.new('credential_process provider failure, the credential process had non zero exit status and failed to provide credentials') | ||
| end | ||
| end | ||
|
|
||
| def _parse_payload_format_v1(creds_json) | ||
| creds = Credentials.new( | ||
| creds_json['AccessKeyId'], | ||
| creds_json['SecretAccessKey'], | ||
| creds_json['SessionToken'] | ||
| ) | ||
|
|
||
| @expiration = creds_json['Expiration'] ? Time.iso8601(creds_json['Expiration']) : nil | ||
| return creds if creds.set? | ||
| raise Errors::InvalidProcessCredentialsPayload.new("Invalid payload for JSON credentials version 1") | ||
| end | ||
|
|
||
| def refresh | ||
| @credentials = credentials_from_process(@process) | ||
| end | ||
|
|
||
| def near_expiration? | ||
| # are we within 5 minutes of expiration? | ||
| @expiration && (Time.now.to_i + 5 * 60) > @expiration.to_i | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,20 @@ module Aws | |
| expect(client.config.credentials.access_key_id).to eq("ACCESS_KEY_SC1") | ||
| end | ||
|
|
||
| it 'prefers process credentials over metadata credentials' do | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| client = ApiHelper.sample_rest_xml::Client.new(profile: "creds_from_process", region: "us-east-1") | ||
| expect(client.config.credentials.access_key_id).to eq("AK_PROC1") | ||
| end | ||
|
|
||
| it 'prefers direct credentials over process credentials' do | ||
| stub_const('ENV', { | ||
| "AWS_ACCESS_KEY_ID" => "AKID_ENV_STUB", | ||
| "AWS_SECRET_ACCESS_KEY" => "SECRET_ENV_STUB" | ||
| }) | ||
| client = ApiHelper.sample_rest_xml::Client.new(profile: "creds_from_process", region: "us-east-1") | ||
| expect(client.config.credentials.access_key_id).to eq("AKID_ENV_STUB") | ||
| end | ||
|
|
||
| it 'attempts to fetch metadata credentials last' do | ||
| stub_request( | ||
| :get, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| require_relative '../spec_helper' | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test suites looks good, just checking does these covered all cli test suits for the process credentials as well? |
||
| module Aws | ||
| describe ProcessCredentials do | ||
|
|
||
| before(:each) do | ||
| stub_const('ENV', {}) | ||
| allow(Dir).to receive(:home).and_raise(ArgumentError) | ||
| end | ||
|
|
||
| it 'will read credentials from a process' do | ||
| creds = ProcessCredentials.new('echo \'{"Version":1,"AccessKeyId":"AK_PROC1","SecretAccessKey":"SECRET_AK_PROC1","SessionToken":"TOKEN_PROC1"}\'').credentials | ||
| expect(creds.access_key_id).to eq('AK_PROC1') | ||
| expect(creds.secret_access_key).to eq('SECRET_AK_PROC1') | ||
| expect(creds.session_token).to eq('TOKEN_PROC1') | ||
| end | ||
|
|
||
| it 'will throw an error when the process credentials payload version is invalid' do | ||
| expect { | ||
| creds = ProcessCredentials.new('echo \'{"Version":3,"AccessKeyId":"","SecretAccessKey":"","SessionToken":""}\'').credentials | ||
| }.to raise_error(Errors::InvalidProcessCredentialsPayload) | ||
| end | ||
|
|
||
| it 'will throw an error when the process credentials payload is malformed' do | ||
| expect { | ||
| creds = ProcessCredentials.new('echo \'{"Version":1}\'').credentials | ||
| }.to raise_error(Errors::InvalidProcessCredentialsPayload) | ||
| end | ||
|
|
||
| it 'will throw an error and expose the stderr output when the credential process has a nonzero exit status' do | ||
| expect { | ||
| creds = ProcessCredentials.new('>&2 echo "Credential Provider Error"; false').credentials | ||
| }.to raise_error(Errors::InvalidProcessCredentialsPayload) | ||
| .and output("Credential Provider Error\n").to_stderr_from_any_process | ||
| end | ||
|
|
||
| it 'will throw an error when the credential process cant be found' do | ||
| expect { | ||
| creds = ProcessCredentials.new('fake_proc').credentials | ||
| }.to raise_error(Errors::InvalidProcessCredentialsPayload) | ||
| end | ||
| end | ||
| end | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make the "else nil" explicit here.