get_aws_connection_info: Explicitly specified credentials are overridden by the profile with boto3

[abadger]

SUMMARY

I was looking at the code in get_aws_connection_info https://github.com/ansible-collections/amazon.aws/blob/main/plugins/module_utils/ec2.py#L331 and think I found a bug with profile and other credentials.

If the user explicitly specifies both profile and other credentials (like security_token) in the playbook, profile silently overrides any of the given credentials. This is undesirable as the user has the most control over the playbook parameters so playbook parameters should either take precedence over profile or these playbook parameters should be mutually_exclusive with the profile.

I also see that the profile is being retrieved from environment variables if the profile_name is not set. That will have to be rethought in this case too.... Perhaps implementing the playbook parameters overriding the profile is the only way to make this work.

Precedence of values should be (earliest entry should override anything later):

• Explicitly specified via a playbook parameter
• Part of the profile explicitly specified via a playbook parameter
• Part of the profile specified by env vars on the target machine.

ISSUE TYPE

• Bug Report

COMPONENT NAME

lib/ansible/module_utils/ec2.py

ANSIBLE VERSION

devel

This exists on 2.8 as well but I'm not sure if it should be backported as it is a new deprecation.

CONFIGURATION

N/A

OS / ENVIRONMENT

N/A

STEPS TO REPRODUCE

EXPECTED RESULTS

ACTUAL RESULTS

[tremble]

@abadger @jillr @s-hertel :

Given that this would be a fairly substantial change my feeling is that we take the opportunity to massively simplify the logic:

1. Add a lookup plugin for querying boto.config and boto3.config
2. Deprecate using os.environ as a source of credentials entirely, recommending that folks use lookup('env') if they need to simulate the current behaviour
3. Deprecate using boto.config as a source of credentials entirely, recommending that folks use the new lookup plugins if they need to simulate the current behaviour
4. Deprecate the passing of both a profile and credential sets at the same time as each other (preparing to make them mutually exclusive)

Thoughts? Objections?

Note:

• There are valid use cases for no credentials being supplied (specifically the use of Instance Profiles) so I'd not make the passing of credentials mandatory.

[tremble]

It's been pointed out that using ENV variables helps for general use, I think if we continue to use them there needs to be an easy to describe way of saying "Read everything from Profile" "Read everything from ENV" "Read everything from creds parameters", right now it's easy to end up with an ugly mix.

[flowerysong]

@tremble Bear in mind that lookups are not a direct replacement for the usage of environment variables or boto configuration, because they run on the controller instead of the target host.

E.g. the following playbook:

- hosts: mighty-hossu.mx.x.mail.umich.edu
  become: false
  tasks:
    - name: This uses the secrets from the environment on the target
      aws_s3_sync:
        direction: pull
        bucket: "{{ yum_hostname }}"
        region: "{{ yum_aws_region }}"
        prefix: "{{ yum_repo_path }}"
        path: /var/tmp/yum

    - name: This uses the secrets from the environment on the controller, which don't work
      aws_s3_sync:
        direction: pull
        bucket: "{{ yum_hostname }}"
        region: "{{ yum_aws_region }}"
        prefix: "{{ yum_repo_path }}"
        path: /var/tmp/yum
        aws_access_key: "{{ lookup('env', 'AWS_ACCESS_KEY') }}"
        aws_secret_key: "{{ lookup('env', 'AWS_SECRET_KEY') }}"

Results in this:

TASK [Gathering Facts] *********************************************************
ok: [mighty-hossu.mx.x.mail.umich.edu]

TASK [This uses the secrets from the environment on the target] ****************
ok: [mighty-hossu.mx.x.mail.umich.edu]

TASK [This uses the secrets from the environment on the controller, which don't work] ***
An exception occurred during task execution. To see the full traceback, use -vvv
. The error was: ClientError: An error occurred (SignatureDoesNotMatch) when cal
ling the ListObjectsV2 operation: The request signature we calculated does not m
atch the signature you provided. Check your key and signing method.
[...]

[jillr]

I don't think we want to remove support for ENV vars, or anything that AWS themselves support that we do today. Removing the silent sharp edges from what we have now though, and simplifying that logic, is a good idea.

It might be good to start with documenting what are all the methods/combinations of methods a user could use for auth today, and making sure we have good test coverage for each of those scenarios. Then we can look at

• what auth methods / combinations make sense to support and which should be mutually_exclusive / unsupported
• what if anything should be deprecated
• refactoring the logic in get_aws_connection_info().

[s-hertel]

I agree with all of @jillr's points. The bug looks like a breaking change from #99 (though 2.8 is mentioned, so maybe there are additional details that were glossed over?). I think that bit of code should be removed, just leaving the CA bundle addition. There are definitely interesting corner cases for credentials and it would be great to rework those though.

(Also cc @AlanCoding in case Tower would be impacted by the features suggested)

[tremble]

Profile has always overridden the other credential types, the difference is that we now support the AWS_PROFILE env var.

Upstream documentation is https://boto3.amazonaws.com/v1/documentation/api/latest/guide/configuration.html

Reading through the comments I think the rough priorities/behaviour we want is:

1. explicit access_key/secret_key/session_token (as parameter)
   • explicit profile and env vars if set at same time should raise a deprecation warning and be ignored
2. explicit profile (as parameter)
   • env vars if set at same time should raise a deprecation warning and be ignored
3. access_key/secret_key/session_token (as env)
   • profile in env var if set at same time should raise a deprecation warning and be ignored
4. profile (as env)
5. .aws/config file

What's unclear is how we should behave if there's a partial set of credentials configured

[s-hertel]

@tremble Profile as a direct task parameter taking precedence is fine. Of course, we could possibly make it mutually exclusive in the future as you suggested (I haven't fully thought through the repercussions of doing that). The environment var for change seems wrong in retrospect and I probably should have known/remembered why additional sources for profile would add a bad corner case.

The bigger picture, improving the mixed-credential situation, could break legitimate playbooks that have adopted Ansible's logic in that regard, so I see it needing a deprecation period and/or being featureish.

I don't agree with the order you listed. Swapping the order of profile and keys is going to break so many playbooks.

[tremble]

So 2,1, 4,3, 5 ?

[s-hertel]

Yeah. I'm not sure if support for boto.config should be removed until we remove boto content.

[tremble]

The boto3 docs say that it's a potential source, so I guess it should stay.