Add support for loading shared config #1391

jeskew · 2017-03-07T00:23:02Z

This PR adds support for loading configuration from ~/.aws/config, including credentials, the default region to use, and role assumption metadata. This is an opt-in feature and will only take effect if the AWS_SDK_LOAD_CONFIG environment variable is set to something truthy.

I also added support for some configuration environment variables that we do not currently support, namely AWS_SHARED_CREDENTIALS_FILE and AWS_CONFIG_FILE, both of which will only be used if AWS_SDK_LOAD_CONFIG is set.

This should resolve #1196 as well (via AWS_SHARED_CREDENTIALS_FILE).

Resolves #1296
Resolves #1039
Resolves #993

coveralls · 2017-03-07T00:28:17Z

Coverage decreased (-0.02%) to 90.382% when pulling 3e73060 on jeskew:jans510-Add-AWS_CREDENTIAL_PROFILES_FILE-Environment-Variable into cd6f728 on aws:master.

alsmola · 2017-04-12T23:20:05Z

Anything holding back a merge here? This functionality would be really useful for compatibility with other SDKs.

chrisradek · 2017-04-12T23:55:55Z

lib/shared_ini.js

+/**
+ * @api private
+ */
+AWS.SharedIniFile = AWS.util.inherit({


Since this is private, you could treat this as a regular module instead of hanging the class on AWS. Then you won't have to do the side-effects require in node_loader.js.

chrisradek · 2017-04-12T23:57:12Z

lib/credentials/shared_ini_file_credentials.js

@@ -111,8 +126,7 @@ AWS.SharedIniFileCredentials = AWS.util.inherit(AWS.Credentials, {
    if (this.disableAssumeRole) {
      throw AWS.util.error(
        new Error('Role assumption profiles are disabled. ' +
-                  'Failed to load profile ' + this.profile + ' from ' +
-                  this.filename),
+                  'Failed to load profile ' + this.profile),


Could you still get this.filename from creds.filename instead? It might still be useful to have the filename in the message, especially now that we will be looking at credentials and config.

chrisradek · 2017-04-13T00:04:25Z

lib/node_loader.js

-    return process.env.AWS_REGION || process.env.AMAZON_REGION;
+    var env = process.env;
+    var region = env.AWS_REGION || env.AMAZON_REGION;
+    if (!region && env.AWS_SDK_LOAD_CONFIG) {


I believe we're actually supposed to check the credentials file for region, then the config file, similar to the CLI/boto.

chrisradek · 2017-04-13T00:06:53Z

lib/credentials/shared_ini_file_credentials.js

@@ -2,9 +2,14 @@ var AWS = require('../core');
 var path = require('path');
 var STS = require('../../clients/sts');

+var configOptInEnv = 'AWS_SDK_LOAD_CONFIG';
+var sharedFileEnv = 'AWS_SHARED_CREDENTIALS_FILE';
+var defaultProfile = 'default';


Should we grab defaultProfile from AWS.util.defaultProfile instead?

chrisradek · 2017-04-13T00:13:45Z

test/credentials.spec.coffee

+        expect(AWS.util.readFileSync.calls[0].arguments[0]).to.match(/[\/\\]home[\/\\]user[\/\\].aws[\/\\]config/)
+        expect(AWS.util.readFileSync.calls[1].arguments[0]).to.equal(process.env.AWS_SHARED_CREDENTIALS_FILE)
+
+      it 'loads credentials from ~/.aws/config if AWS_SDK_LOAD_CONFIG is set', ->


Can we add a test to make sure the credentials from ~/.aws/credentials is used preferentially over the credentials in ~/.aws/config if the same profile exists in both files?

I'll add a test for that.

chrisradek · 2017-04-13T00:15:11Z

One thing I'm not clear on, does the following scenario work:

profile in credentials file contains a roleArn and source profile
source profile is read from config file

…ILE if AWS_SDK_LOAD_CONFIG is set

…ed in AWS_CONFIG_FILE if AWS_SDK_LOAD_CONFIG is set

…rce profiles defined in ~/.aws/config

jeskew · 2017-04-13T02:40:29Z

I had to force push due to a merge conflict in AWS.util. The last commit add the ability to assume a role for a profile defined in ~/.aws/credentials even if the source profile is defined ~/.aws/config and vice versa.

codecov-io · 2017-04-13T02:44:39Z

Codecov Report

Merging #1391 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1391      +/-   ##
==========================================
- Coverage   95.35%   95.34%   -0.02%     
==========================================
  Files         176      177       +1     
  Lines        6222     6270      +48     
  Branches     1278     1293      +15     
==========================================
+ Hits         5933     5978      +45     
- Misses        289      292       +3

Impacted Files	Coverage Δ
lib/util.js	`93.33% <ø> (ø)`	⬆️
lib/shared_ini.js	`100% <100%> (ø)`
lib/node_loader.js	`85.71% <100%> (-11.73%)`	⬇️
lib/credentials/shared_ini_file_credentials.js	`98.59% <100%> (+4.38%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a8f68...7528c83. Read the comment docs.

chrisradek · 2017-04-13T16:23:40Z

test/credentials.spec.coffee

+
+      it 'prefers credentials from ~/.aws/credentials if AWS_SDK_LOAD_CONFIG is set', ->
+        process.env.AWS_SDK_LOAD_CONFIG = '1'
+        mock = '''


This mock can probably be removed.

chrisradek · 2017-04-13T16:29:26Z

test/credentials.spec.coffee

+        </AssumeRoleResult>
+      </AssumeRoleResponse>
+      '''
+      debugger;


Extra statement :)

chrisradek · 2017-04-13T16:46:30Z

test/credentials.spec.coffee

+          aws_access_key_id = akid2
+          aws_secret_access_key = secret2
+          '''
+      helpers.mockHttpResponse 200, {}, '''


I'm not sure this test is actually ensuring that the creds from profile foo are used instead of default.

What do you think about spying on AWS.STS or AWS.Credentials to get the accessKeyId that was used as the source?

chrisradek · 2017-04-13T16:46:47Z

test/credentials.spec.coffee

+        </AssumeRoleResult>
+      </AssumeRoleResponse>
+      '''
+      debugger;


Extra statement.

et304383 · 2017-04-18T13:08:49Z

Am I to understand that the SDK currently doesn't support ~/.aws at all? Or just doesn't support using assumed roles through the ~/.aws entries, which has only recently been added to PHP and worked a while in Python?

chrisradek · 2017-04-18T15:28:33Z

@eric-tucker
The SDK currently sources credentials using ~/.aws/credentials by default. It actually can use assumed roles if they are defined in the credentials file. This change also allows ~/.aws/config to be used when sourcing credentials, since most of the other SDKs support this now as well.

chrisradek · 2017-04-19T17:21:42Z

lib/credentials/shared_ini_file_credentials.js

-          profiles[availableProfiles[i]] || {},
-          creds.getProfile(availableProfiles[i])
-        );
+      for (i = 0, availableProfiles = creds.getProfiles(); i < availableProfiles.length; i++) {


Nit: Can we either use var i here, or declare var i at the top of the function? Just worried if we someday change i in the for loop above, then this i becomes a global.

chrisradek

dotchev · 2017-09-16T05:36:18Z

Why not load ~/.aws/confi by default as AWS CLI does?

chrisradek · 2017-09-18T16:14:39Z

@dotchev
We can't do that until we make a major version bump, because this could lead to the SDK loading different credentials without any code changes in consuming code. We do plan on loading ~/.aws/config by default like the CLI does when we do a major bump, but I can't give a timeline on when that will be.

see aws/aws-sdk-js#1391

This appears to be a bug upstream as a result of aws/aws-sdk-js#1391 We now check for the presence of ~/.aws prior to setting this env-var.

lucleray · 2018-01-29T18:31:23Z

Hi,

This page is still confusing : https://docs.aws.amazon.com/sdk-for-javascript/v2/developer-guide/setting-region.html#setting-region-config-file.

"The SDK for JavaScript automatically searches for a config file when it loads."

I understand here that I need to set AWS_SDK_LOAD_CONFIG to something truthy to get that behavior. Is this documented somewhere ?

jeskew · 2018-01-29T18:56:58Z

The AWS_SDK_LOAD_CONFIG environment variable is only documented on that page in the "Order of Precedence for Setting the Region" section. I'll try to get the "Using a Shared Config File" section updated to reflect this.

lucleray · 2018-01-30T03:38:39Z

Thanks!

villasv · 2018-07-18T19:58:41Z

@chrisradek any update on the version bump? Having this inconsistency for over a year is enough justification for a major release IMHO

rezarajabii · 2018-09-13T06:43:30Z

It would have been nice if you could add an example of using this feature. I am having issues of using assume role with JS.

ctmlr · 2019-01-09T05:24:35Z

This seems to be missing support for the mfa_serial option.

lock · 2019-09-28T01:56:49Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

chrisradek reviewed Apr 13, 2017

View reviewed changes

Jan Stroppel and others added 4 commits April 12, 2017 17:22

Added AWS_CREDENTIAL_PROFILES_FILE Environment Variable

b480c5f

Load credentials file from path specified in AWS_SHARED_CREDENTIALS_F…

2daf856

…ILE if AWS_SDK_LOAD_CONFIG is set

Load configuration and credentials from ~/.aws/config or path specifi…

44b1594

…ed in AWS_CONFIG_FILE if AWS_SDK_LOAD_CONFIG is set

Allow profiles defined in ~/.aws/credentials to assume roles with sou…

fc4d210

…rce profiles defined in ~/.aws/config

jeskew force-pushed the jans510-Add-AWS_CREDENTIAL_PROFILES_FILE-Environment-Variable branch from 3e73060 to fc4d210 Compare April 13, 2017 02:37

chrisradek reviewed Apr 13, 2017

View reviewed changes

Spy on STS constructor to ensure proper source profile is used

79ebbd0

chrisradek reviewed Apr 19, 2017

View reviewed changes

Avoid merging profiles

7528c83

jeskew force-pushed the jans510-Add-AWS_CREDENTIAL_PROFILES_FILE-Environment-Variable branch from 12e966a to 7528c83 Compare April 19, 2017 17:41

chrisradek approved these changes Apr 19, 2017

View reviewed changes

jeskew merged commit 5ce1233 into aws:master Apr 19, 2017

jeskew deleted the jans510-Add-AWS_CREDENTIAL_PROFILES_FILE-Environment-Variable branch April 19, 2017 23:29

This was referenced Apr 20, 2017

config file isn't checked for region #1039

Closed

Release separate parts of the whole SDK to npm? #1093

Closed

clark-pan mentioned this pull request Apr 26, 2017

Getting CredentialsError: Missing credentials in config starting in v2.44.0 #1483

Closed

vhartikainen mentioned this pull request Jun 1, 2017

SDK doesn't work with roles requiring MFA #1543

Closed

kubek2k mentioned this pull request Jun 1, 2017

Fetch default region from .aws/config kubek2k/lambdoku#10

Closed

cg-cnu mentioned this pull request Sep 1, 2017

Read credentials and config from the shared .ini file markitx/dynamo-backup-to-s3#52

Open

vandrewskis mentioned this pull request Oct 24, 2017

Utilize a named profile from .aws/credentials file if profile option … mifi/dynamodump#4

Merged

tavisrudd added a commit to unbounce/iidy that referenced this pull request Oct 29, 2017

set AWS_SDK_LOAD_CONFIG earlier to fix missing region in output

4819b8e

see aws/aws-sdk-js#1391

chrisradek mentioned this pull request Aug 14, 2018

AWS.SharedIniFileCredentials loadRoleProfile using non existent options #2197

Closed

srchase mentioned this pull request Oct 4, 2018

Not loading creds from Shared Credentials File #2289

Closed

trivikr mentioned this pull request May 24, 2019

chore: add settings for lock bot aws/aws-sdk-js-v3#262

Merged

tvvxaop mentioned this pull request Jun 14, 2019

SharedIniFileCredentials: add parameter to overwrite env.AWS_SDK_LOAD_CONFIG #2722

Closed

AllanZhengYP mentioned this pull request Jul 12, 2019

Error accessing AWS.SharedIniFileCredentials when credentials file does not exist #2771

Closed

3 tasks

lock bot locked as resolved and limited conversation to collaborators Sep 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for loading shared config #1391

Add support for loading shared config #1391

jeskew commented Mar 7, 2017 •

edited

Loading

coveralls commented Mar 7, 2017

alsmola commented Apr 12, 2017

chrisradek Apr 12, 2017

chrisradek Apr 12, 2017

chrisradek Apr 13, 2017

chrisradek Apr 13, 2017

chrisradek Apr 13, 2017

jeskew Apr 13, 2017

chrisradek commented Apr 13, 2017

jeskew commented Apr 13, 2017

codecov-io commented Apr 13, 2017 •

edited

Loading

chrisradek Apr 13, 2017

chrisradek Apr 13, 2017

chrisradek Apr 13, 2017

chrisradek Apr 13, 2017

et304383 commented Apr 18, 2017 •

edited

Loading

chrisradek commented Apr 18, 2017

chrisradek Apr 19, 2017

chrisradek left a comment

dotchev commented Sep 16, 2017 •

edited

Loading

chrisradek commented Sep 18, 2017

lucleray commented Jan 29, 2018

jeskew commented Jan 29, 2018 •

edited

Loading

lucleray commented Jan 30, 2018

villasv commented Jul 18, 2018

rezarajabii commented Sep 13, 2018

ctmlr commented Jan 9, 2019

lock bot commented Sep 28, 2019

Add support for loading shared config #1391

Add support for loading shared config #1391

Conversation

jeskew commented Mar 7, 2017 • edited Loading

coveralls commented Mar 7, 2017

alsmola commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek commented Apr 13, 2017

jeskew commented Apr 13, 2017

codecov-io commented Apr 13, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

et304383 commented Apr 18, 2017 • edited Loading

chrisradek commented Apr 18, 2017

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

dotchev commented Sep 16, 2017 • edited Loading

chrisradek commented Sep 18, 2017

lucleray commented Jan 29, 2018

jeskew commented Jan 29, 2018 • edited Loading

lucleray commented Jan 30, 2018

villasv commented Jul 18, 2018

rezarajabii commented Sep 13, 2018

ctmlr commented Jan 9, 2019

lock bot commented Sep 28, 2019

jeskew commented Mar 7, 2017 •

edited

Loading

codecov-io commented Apr 13, 2017 •

edited

Loading

et304383 commented Apr 18, 2017 •

edited

Loading

dotchev commented Sep 16, 2017 •

edited

Loading

jeskew commented Jan 29, 2018 •

edited

Loading