Allows profiles to automatically assume a role #34

morris25 · 2018-03-08T21:28:49Z

In response to #30

samoconnor · 2018-03-08T21:35:24Z

Nice one!
Can you add a test? (Maybe not one that runs as part of runtests.jl but a stand-alone test with a README that describes how to create the appropriate .config files to run the test).

samoconnor · 2018-03-09T00:42:58Z

src/AWSCredentials.jl

@@ -254,27 +254,62 @@ using IniFile
 dot_aws_credentials_file() = get(ENV, "AWS_CONFIG_FILE",
                                 joinpath(homedir(), ".aws", "credentials"))


It seems based on the current doc that this should be changed to:

dot_aws_credentials_file() = get(ENV, "AWS_SHARED_CREDENTIALS_FILE", joinpath(homedir(), ".aws", "credentials"))

Oops, I forgot to change that. Thanks!

samoconnor · 2018-03-09T00:45:28Z

src/AWSCredentials.jl

 function dot_aws_credentials()

    @assert isfile(dot_aws_credentials_file())
+    @assert isfile(dot_aws_config_file())


We need to support systems with only one of these files. So the assert should probably be creds || conf.

Up in function AWSCredentials() where dot_aws_credentials() is called we'll need to apply the same test.

samoconnor · 2018-03-09T01:09:15Z

src/AWSCredentials.jl

-    ini = read(Inifile(), dot_aws_credentials_file())
-
+    config_ini = read(Inifile(), dot_aws_config_file())
+    creds_ini = read(Inifile(), dot_aws_credentials_file())


One or the other of these files may not exist, so read(Inifile(), ... will throw ERROR: SystemError: opening file food: No such file or directory. Probably best to have an if isfile around the block of code that uses that file.

The cli doc says that .credentials has precedence over .config for creds. So the logic should be something like:

if there is a .config file:

if there are credentials for profile in the config file load them

if there is a role_arn and source_profile for profile, grab the role_arnand overwrite theprofile` variable.

try to load credentials for profile in the config file again in case the source_profile refers to another profile in the config file.

if there is a .credentials file:

if there are credentials for profile in the .credentials file load them (overwriting whatever was loaded from the .config file.

create the AWSCredentials object

If there is a role_arn, call AssumeRole and create a new AWSCredentials object

return the AWSCredentials object

samoconnor · 2018-03-13T10:07:58Z

Thinking a bit more about how to test this...
A test in runtests.jl could set AWS_SHARED_CREDENTIALS_FILE andAWS_CONFIG_FILE to point to temporary test files. The test could call AWSCredentials() to and inspect the resulting creds object for the expected keys based on the precedence rules for creds file vs config file. AWS_DEFAULT_PROFILE should also be set to something, and unset to ensure the correct default profile is used. In the case where STS AssumeRole is called, the test could check for the an access denied error, which would at least ensure that the AssumeRole call is reached.

omus · 2018-03-13T15:01:11Z

Note that the source_profile may or may not have associated credentials.

# Has credentials
[profile root]
output = json
region = us-east-1

[profile root:dev]
source_profile = root
role_arn = arn:aws:iam::123456789000:role/Dev

[profile root:sub-dev]
source_profile = root:dev
role_arn = arn:aws:iam::123456789000:role/SubDev

omus · 2018-03-13T15:05:06Z

src/AWSCredentials.jl


    profile = get(ENV, "AWS_DEFAULT_PROFILE",
              get(ENV, "AWS_PROFILE", "default"))
+
+    region = get(ENV, "AWS_DEFAULT_REGION", "us-east-1")


Shouldn't be assuming a default region. If the profile doesn't have a region set or AWS_DEFAULT_REGION isn't set there should be an exception

Should that also be the case in aws_config()?

I believe so. Maybe @samoconnor disagrees?

I do :)
I have tried to include support for the env vars and config files that are uses by boto3 (and the AWS cli) as a convenience to end users. However, I'm not attempting to follow boto3's behaviour in every respect.

My intention is to try to make the Julia AWS interface as easy as possible for first time users. As such, I see having a default region as a feature that's good for people who don't know or care what a region is but just wan't to fetch some files from this thing called S3 where their collaborators keep their data.

Some examples:

The S3 interface automatically follows (and caches) bucket-region redirects so that bucket regions can be completely ignored.

The list objects interface handles pagination internally rather than requiring the user to deal with paginators as boto3 does.

Having said that, it doesn't seem quite right to duplicate the region env lookup and default (the other place is here: https://github.com/JuliaCloud/AWSCore.jl/blob/master/src/AWSCore.jl#L109)

There is a slight problem in that struct AWSCredentials does not have a region field so the region from the .config or .credentials file can't currently be accessed by the aws_config function. I propose the following:

Add a default_region::String field to struct AWSCredentials.

Set default_region=get(ENV, "AWS_DEFAULT_REGION", "us-east-1") in the default constructor

In dot_aws_credentials, set default_region if specified by the config files.

In the aws_config function, change the region= kw parameter to region=creds.default_region.

I think this should preserve existing behaviour, keep the default in one place, and add support for reading the region from the config files.

morris25 · 2018-03-27T19:30:58Z

Nested roles should work now.

samoconnor · 2018-03-29T00:14:53Z

Thanks @morris25! I won't have time to review this immediately. Easter school holidays are about to begin...

morris25 · 2018-04-18T14:14:27Z

Is this still alive?

samoconnor · 2018-03-13T22:53:11Z

src/AWSCredentials.jl


    profile = get(ENV, "AWS_DEFAULT_PROFILE",
              get(ENV, "AWS_PROFILE", "default"))
+
+    region = get(ENV, "AWS_DEFAULT_REGION", "us-east-1")


Having said that, it doesn't seem quite right to duplicate the region env lookup and default (the other place is here: https://github.com/JuliaCloud/AWSCore.jl/blob/master/src/AWSCore.jl#L109)

There is a slight problem in that struct AWSCredentials does not have a region field so the region from the .config or .credentials file can't currently be accessed by the aws_config function. I propose the following:

Add a default_region::String field to struct AWSCredentials.

Set default_region=get(ENV, "AWS_DEFAULT_REGION", "us-east-1") in the default constructor

In dot_aws_credentials, set default_region if specified by the config files.

In the aws_config function, change the region= kw parameter to region=creds.default_region.

I think this should preserve existing behaviour, keep the default in one place, and add support for reading the region from the config files.

samoconnor · 2018-04-19T00:05:11Z

@morris25 sorry about the delay, and thanks for your efforts!

Allows profiles to automatically assume a role

f6c3682

Adds details to the README

092276e

samoconnor reviewed Mar 9, 2018

View reviewed changes

Fixes credential file precedence and existence checks

d2d97b7

omus reviewed Mar 13, 2018

View reviewed changes

morris25 added 2 commits March 15, 2018 12:26

Adds tests for credentials and assuming a role

054db76

Allows recursive role assumption

7c468bc

samoconnor approved these changes Apr 19, 2018

View reviewed changes

samoconnor mentioned this pull request Apr 19, 2018

Read default region from config file #36

Open

samoconnor merged commit 8273c07 into JuliaCloud:master Apr 19, 2018

omus mentioned this pull request Jul 23, 2018

Tag a new release #42

Closed

samoconnor mentioned this pull request Jul 25, 2018

WIP: Updates for Julia 0.7 #40

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows profiles to automatically assume a role #34

Allows profiles to automatically assume a role #34

morris25 commented Mar 8, 2018 •

edited

Loading

samoconnor commented Mar 8, 2018

samoconnor Mar 9, 2018

morris25 Mar 9, 2018

samoconnor Mar 9, 2018

samoconnor Mar 9, 2018

samoconnor Mar 9, 2018

samoconnor commented Mar 13, 2018

omus commented Mar 13, 2018

omus Mar 13, 2018

morris25 Mar 13, 2018 •

edited

Loading

omus Mar 13, 2018

samoconnor Mar 13, 2018

samoconnor Mar 13, 2018

morris25 commented Mar 27, 2018

samoconnor commented Mar 29, 2018

morris25 commented Apr 18, 2018

samoconnor Mar 13, 2018

samoconnor commented Apr 19, 2018

		@@ -254,27 +254,62 @@ using IniFile
		dot_aws_credentials_file() = get(ENV, "AWS_CONFIG_FILE",
		joinpath(homedir(), ".aws", "credentials"))

Allows profiles to automatically assume a role #34

Allows profiles to automatically assume a role #34

Conversation

morris25 commented Mar 8, 2018 • edited Loading

samoconnor commented Mar 8, 2018

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

samoconnor commented Mar 13, 2018

omus commented Mar 13, 2018

Choose a reason for hiding this comment

morris25 Mar 13, 2018 • edited Loading

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

morris25 commented Mar 27, 2018

samoconnor commented Mar 29, 2018

morris25 commented Apr 18, 2018

Choose a reason for hiding this comment

samoconnor commented Apr 19, 2018

morris25 commented Mar 8, 2018 •

edited

Loading

morris25 Mar 13, 2018 •

edited

Loading