Issue #906 add support for $AWS_PROFILE #966

taraspos · 2018-04-28T18:15:42Z

Add support for reading credentials from standardized AWS credentials file [1] with multiple profiles #906

New logic works as in PR #570 but with additional changes.

Reading credentials from the AWS credentials file can be triggered by setting any of following env variables:

AWS_CREDENTIAL_FILE
AWS_PROFILE

Default value for AWS_PROFILE is default and for AWS_CREDENTIAL_FILE is ~/.aws/credentials.

If AWS_CREDENTIAL_FILE set to non-existing file, it will fallback to the default value. This way you can use AWS_CREDENTIAL_FILE=true if you want to read credentials from the default location.

Also, custom logic for parsing AWS credentials file was replaced by default configparser lib, because this custom logic was written in 2013 before credentials file format was standardized in 2014.

[1] https://aws.amazon.com/blogs/security/a-new-and-standardized-way-to-manage-credentials-in-the-aws-sdks/

taraspos · 2018-05-07T08:39:16Z

Hello @fviard,
Could you take a look at this PR, when possible?

fviard · 2018-05-07T12:46:34Z

@trane9991 Thank you for your contribution, and sorry for my slow response time.
I wanted to take the time to read carefully to ensure that it doesn't break anything.
But the direction of the patch is good.

A few things I see:

error("%d accessing credentials file %s" % (e.errno,os.environ['AWS_CREDENTIAL_FILE']))

It would probably be better to keep it as it was before.
The thing can be open for discussion, but I think that if no aws configure can be loaded, that is not really an error. That could be a normal case.
Maybe that can be just a warning? more explicit, something like no aws credential file found or found but can't be loaded.

error(e)

at the end. Don't do this, always put an explicit error message, because otherwise some orphan errors would appear in the log and no one will understand where it came from.
As for the previous one, as we just pass without caring, maybe it should be only an warning or debug.

It would probably be better to keep the log that was done of "keys" that are overridden. That allows to understand where the keys that are in use come from.
It would be better to only set config variable through the update_config like it was done before. Some treatment can be done on the config by it.

           if data["orig_key"] == "AWSAccessKeyId" \	
                       or data["orig_key"] == "aws_access_key_id":

I don't know a lot of history of s3cmd, but I think that if there was this line, it is probably that there is another aws config file or a previous version of its format that used such a key?
Would be good to understand where it came from to be sure to not break support for something existing.

This one is just a matter of taste I guess, but for the import i would have preferred something like :

try: 
 from configparser import NoOption, NoSection, ConfiParser as PyConfigParser
except importerror:
  # Python2 fallback code
  from Configparser import ....

And so you can use them directly in the code.
the name PyConfigParser is to be very very explicit to not think by accident that defaultconfigparser is a kind of object of s3cmd related to s3cmd configparser

I hope that you will still like to complete your PR despite this long review ^^

taraspos · 2018-05-07T13:08:52Z

Thanks for the in-depth review, the advice is highly appreciated!

Regarding the:

error("%d accessing credentials file %s" % (e.errno,os.environ['AWS_CREDENTIAL_FILE']))

The idea was if person specified AWS_CREDENTIAL_FILE then he/she wanted to read credentials from the AWS_CREDENTIAL_FILE, and if it wasn't found, then it is an error. But if you still think it should be warning it is fine with me as well :)

if data["orig_key"] == "AWSAccessKeyId" \	
           or data["orig_key"] == "aws_access_key_id":

I'm not sure about this one as well, but it was added a long time ago since then credentials file format was standardized and usage of the credentials in the five years old and the unsupported format should be considered as a bad practice, and not sure if it needs to be supported.

…- better error logging- use `Config().update_option()` to set keys

fviard · 2018-05-07T22:29:57Z

S3/Config.py

+
+            config = PyConfigParser()
+
+            debug("Reading AWS credentials from", aws_credential_file)


I think that you forgot the "%s" in the "string" template part.

fviard · 2018-05-07T22:51:34Z

S3/Config.py

+            if 'AWS_PROFILE' in os.environ:
+                profile = os.environ['AWS_PROFILE']
+
+            debug("Using AWS profile '{}'".format(profile))


Please use the old style string template to be coherent with the rest of the code.
Ex: debug("Using %s", profile)

fviard · 2018-05-07T23:03:55Z

S3/Config.py

-            cred_content = cred_file.read()
+            aws_credential_file = os.path.expanduser('~/.aws/credentials') 
+            if 'AWS_CREDENTIAL_FILE' in os.environ and os.path.isfile(os.environ['AWS_CREDENTIAL_FILE']):
+                aws_credential_file = os.environ['AWS_CREDENTIAL_FILE']


I know that it was not done before, but as an improvement, it would be great if you can ensure to pass the strings you get from os.environ to config_unicodise.
The tests to be done to check if things are good, is like to create a folder with chinese characters, and try to put some chinese chars inside the configuration file itself. Normally keys don't have special chars, but so we are sure that the code is proof to nasty issues in the future.

fviard · 2018-05-07T23:05:46Z

S3/Config.py

+
+            profile = "default"
+            if 'AWS_PROFILE' in os.environ:
+                profile = os.environ['AWS_PROFILE']


2 previous lines can be simplified in to:
profile = os.environ.get('AWS_PROFILE', 'default')
So with the unicode safe conversion:
profile = config_unicodise(os.environ.get('AWS_PROFILE', 'default')

fviard · 2018-05-07T23:09:10Z

S3/Config.py

+            profile_secret_key = config.get(profile, 'aws_secret_access_key')
+            debug('Setting "aws_secret_access_key" from file {} as "secret_key"'.format(aws_credential_file))
+            Config().update_option('secret_key', config_unicodise(profile_secret_key))
+


Based on the previous code behavior, I think that you should take care that maybe only a part of access/secret/token keys could be stored in the config file.
Like only the session token, or only the access_key, ...
The rest being provided on the command line for example.

fviard · 2018-05-07T23:09:41Z

S3/Config.py

-                        debug("env_Config: %s->%s" % (data["key"], print_value))
+            error("%d accessing credentials file %s" % (e.errno, aws_credential_file))
+        except NoOptionError as e:
+            error("Couldn't find required key '{}' for the AWS Profile '{}' in the credentials file '{}'".format(e.option, e.section, aws_credential_file))


As discussed, "warning" would probably be enough. For the next exception also.

fviard · 2018-05-07T23:16:08Z

S3/Config.py

-                    if data["orig_key"] == "AWSAccessKeyId" \
-                       or data["orig_key"] == "aws_access_key_id":
-                        data["key"] = "access_key"
-                    elif data["orig_key"]=="AWSSecretKey" \


I have found a reference about that:
https://blog.csanchez.org/2011/05/
Looks like it used to be the case up to 2011 at least.
Its a little bit annoying to have to take care of that, but that would probably be better to still handle that case.
Note: a warning might still be issued.
But users could have inherited config files or found old tutorial on internet and still be using that.
It is bad to break user existing setups just for updates when it is not strictly necessary.

- unicodize env variables as well - treat each param from aws credentials as optional - print errors from reading aws credentials file as warnings

taraspos · 2018-05-08T10:00:00Z

Hey @fviard,
your comments are very appreciated, originally I'm not the Python developer so could miss some obvious things :)

I pushed the fixes for all your comments, had to refactor the func to support the legacy options without much uglinesses and repetition in the code.

fviard · 2018-05-08T22:57:07Z

Thank you for your reactivity anyway.
Looks good, i will give it a try tomorrow and merge it.

taraspos · 2018-05-16T10:20:39Z

@fviard sorry for bothering you, but did you have any success testing this?
Also, is there any chance to create the new release version after we merge this?

Frantch · 2018-05-30T19:31:33Z

Consider adding credentials refresh.

Imagine one wants to sync a very large data set and provides a profile that is valid for 60minutes. The sync will fail after 60 min and the command needs to be re run, this is very slow since it will have to re enumerate the full bucket

However if for example every 35 minutes a background process update the credentials file , s3cmd when it fails for a token expires reason, it should retry loading the credentials file.

taraspos · 2018-05-30T19:36:53Z

@Frantch
AFAIK aws_access_key_id and aws_secret_access_key keys that are provided for the AWS IAM User are entirely static and do not expire ever(automatically).

The ones you are talking about is the credentials provided by AWS IAM Role and read from ec2 instance metadata. This type of credentials not covered in this PR since they are already supported.

fviard · 2018-06-03T15:26:10Z

Merged, thank you very much @trane9991 and sorry for the long delay to push it after you being very reactive.
I can't do a release immediately, I have a few annoying bug fixed so I will do a new release very soon.

fviard · 2018-06-03T15:28:09Z

@Frantch As said by Trane9991, this patch is not related to the same credentials.
For iam, I think that it will not expire because sadly credentials are refreshed every requests.
That is not optimal for performances but at least you should not encounter expiration issues.
It is something that I have on my todo list for improvements.

alexanderkjeldaas · 2020-02-17T14:01:33Z

@trane9991 I don't understand this:

The ones you are talking about is the credentials provided by AWS IAM Role and read from ec2 instance metadata. This type of credentials not covered in this PR since they are already supported.

AWS IAM Roles look like this and I don't think they are supported. Is $AWS_PROFILE really supported?

[profile myprofile]
role_arn = arn:aws:iam::<acct>:role/my-iam-role
source_profile = default

taraspos · 2020-02-17T14:08:43Z

@alexanderkjeldaas
IAM roles don't work this way. You can't simply provide the role ARN and use it.

The role needs to be attached to the EC2 instance, then tool, that runs on this instance my use the role (get the credentials from Instance Metadata and user them). See Using an IAM Role to Grant Permissions to Applications Running on Amazon EC2 Instances.

Technically, what you want, could be achieved by Assume Role mechanism and it is not supported AFAIK (at least not by this PR).

taraspos and others added 3 commits April 28, 2018 20:59

Issue s3tools#906 add support for $AWS_PROFILE

66d9ff8

make it work with python2

c49d497

replace Config() with self

9c93747

taraspos added 3 commits May 7, 2018 17:15

Merge branch 'master' of https://github.com/s3tools/s3cmd

eda474d

Merge branch 'master' of https://github.com/Trane9991/s3cmd

406608d

Completed some PR tasks:- better imports handling- more debug logging…

e384565

…- better error logging- use `Config().update_option()` to set keys

taraspos force-pushed the master branch from 010d081 to e384565 Compare May 7, 2018 15:08

fviard reviewed May 7, 2018

View reviewed changes

taraspos added 3 commits May 8, 2018 11:27

pr tasks:

a2065f4

- unicodize env variables as well - treat each param from aws credentials as optional - print errors from reading aws credentials file as warnings

add helper function to read the deprecated credentials format

077027c

add more comments

0483b54

taraspos force-pushed the master branch from bb07472 to 0483b54 Compare May 8, 2018 09:56

fviard merged commit 3b30393 into s3tools:master Jun 3, 2018

mtmiller mentioned this pull request Jul 26, 2018

Problem: AttributeError: ConfigParser instance has no attribute 'read_string' #989

Closed

fviard mentioned this pull request Jul 4, 2021

$AWS_PROFILE is not honored #906

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #906 add support for $AWS_PROFILE #966

Issue #906 add support for $AWS_PROFILE #966

taraspos commented Apr 28, 2018 •

edited

Loading

taraspos commented May 7, 2018

fviard commented May 7, 2018 •

edited

Loading

taraspos commented May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

fviard May 7, 2018

taraspos commented May 8, 2018

fviard commented May 8, 2018

taraspos commented May 16, 2018

Frantch commented May 30, 2018 •

edited

Loading

taraspos commented May 30, 2018

fviard commented Jun 3, 2018

fviard commented Jun 3, 2018

alexanderkjeldaas commented Feb 17, 2020 •

edited

Loading

taraspos commented Feb 17, 2020


		config = PyConfigParser()

		debug("Reading AWS credentials from", aws_credential_file)

Issue #906 add support for $AWS_PROFILE #966

Issue #906 add support for $AWS_PROFILE #966

Conversation

taraspos commented Apr 28, 2018 • edited Loading

taraspos commented May 7, 2018

fviard commented May 7, 2018 • edited Loading

taraspos commented May 7, 2018

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

fviard May 7, 2018

Choose a reason for hiding this comment

taraspos commented May 8, 2018

fviard commented May 8, 2018

taraspos commented May 16, 2018

Frantch commented May 30, 2018 • edited Loading

taraspos commented May 30, 2018

fviard commented Jun 3, 2018

fviard commented Jun 3, 2018

alexanderkjeldaas commented Feb 17, 2020 • edited Loading

taraspos commented Feb 17, 2020

taraspos commented Apr 28, 2018 •

edited

Loading

fviard commented May 7, 2018 •

edited

Loading

Frantch commented May 30, 2018 •

edited

Loading

alexanderkjeldaas commented Feb 17, 2020 •

edited

Loading