Skip to content
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

iam_role not idempotent #115

Closed
mdavis-xyz opened this issue Jun 19, 2020 · 6 comments · Fixed by ansible-collections/amazon.aws#98
Closed

iam_role not idempotent #115

mdavis-xyz opened this issue Jun 19, 2020 · 6 comments · Fixed by ansible-collections/amazon.aws#98

Comments

@mdavis-xyz
Copy link
Contributor

mdavis-xyz commented Jun 19, 2020

Moving this ticket from ansible-base.

SUMMARY

If I try to create an IAM role, I can.
When I run the task a second time, it fails, because I don't have iam:UpdateAssumeRolePolicy permissions in my IAM role.

But if the role policy document hasn't changed, I shouldn't need iam:UpdateAssumeRolePolicy.

ISSUE TYPE
  • Bug Report
COMPONENT NAME

iam_role

ANSIBLE VERSION
$ ansible --version
ansible 2.9.0
  config file = /home/ec2-user/.ansible.cfg
  configured module search path = ['/home/ec2-user/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/ec2-user/.local/lib/python3.6/site-packages/ansible
  executable location = /home/ec2-user/.local/bin/ansible
  python version = 3.6.10 (default, Feb 10 2020, 19:55:14) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]

Note that I tried to reproduce this off the devel branch, but got

ERROR! couldn't resolve module/action 'iam_role'. This often indicates a misspelling, missing collection, or incorrect module path.

It seems that all the cloud modules have been removed from devel?
link
Is that deliberate?

CONFIGURATION
ANSIBLE_PIPELINING(/home/ec2-user/.ansible.cfg) = True
DEFAULT_LOCAL_TMP(/home/ec2-user/.ansible.cfg) = /dev/shm/ansible/tmp_local/ansible-local-12013vjaxtg0x
OS / ENVIRONMENT

Amazon Linux

STEPS TO REPRODUCE
---
- hosts: localhost
  connection: local
  tasks:
  - name: "Create role for SMS logging"
    iam_role:
      name: SNSSMSDeliveryStatusLogging
      assume_role_policy_document: 
        Statement:
        - Action:
          - "sts:AssumeRole"
          Effect: Allow
          Principal:
            Service:
            - "sns.amazonaws.com"
      managed_policy:
 
        # let SNS log to CloudWatch
        - "arn:aws:iam::aws:policy/service-role/AmazonSNSRole"
      boundary: "arn:aws:iam::aws:policy/PowerUserAccess" # should be "{{ boundary_policy_arn }}"
      create_instance_profile: False # must be false when assigning a boundary policy

Run this playbook twice, running this as an IAM role with iam:UpdateAssumeRolePolicy denied.

EXPECTED RESULTS

The playbook should succeed. The first run creates the role. The second run does nothing,

ACTUAL RESULTS

The first run successfully creates the role.
When I try the second time:

TASK [Create role for SMS logging] *****************************************************************************************************************
An exception occurred during task execution. To see the full traceback, use -vvv. The error was: botocore.exceptions.ClientError: An error occurred (AccessDenied) when calling the UpdateAssumeRolePolicy operation: User: arn:aws:sts::123456:assumed-role/deployer/i-abc is not authorized to perform: iam:UpdateAssumeRolePolicy on resource: role SNSSMSDeliveryStatusLogging
fatal: [localhost]: FAILED! => changed=false 
  error:
    code: AccessDenied
    message: 'User: arn:aws:sts::123456:assumed-role/deployer/i-abc is not authorized to perform: iam:UpdateAssumeRolePolicy on resource: role SNSSMSDeliveryStatusLogging'
    type: Sender
  msg: 'Unable to update assume role policy for role SNSSMSDeliveryStatusLogging: An error occurred (AccessDenied) when calling the UpdateAssumeRolePolicy operation: User: arn:aws:sts::123456:assumed-role/deployer/i-abcd is not authorized to perform: iam:UpdateAssumeRolePolicy on resource: role SNSSMSDeliveryStatusLogging'
  response_metadata:
    http_headers:
      content-length: '420'
      content-type: text/xml
      date: Fri, 19 Jun 2020 05:46:33 GMT
      x-amzn-requestid: 576771dd-620d-4ed9-b3e1-d9638f879437
    http_status_code: 403
    request_id: 576771dd-620d-4ed9-b3e1-d9638f879437
    retry_attempts: 0

I wondered whether it's because assume_role_policy_document converts the yaml to json in a non-deterministic way. When I extracted that policy into json and did lookup('file', 'policy.json'), the result is the same. So I don't think that's the cause.

@mdavis-xyz
Copy link
Contributor Author

I'm unable to tag the author @wimnat

@mdavis-xyz
Copy link
Contributor Author

mdavis-xyz commented Jun 23, 2020

Oh, it seems tags do work. It just doesn't look like it when I'm typing.

Anyway, I'm trying to look at the code to figure out what's happening.

I think the problem is that compare_policies, called from compare_assume_role_policy_document is comparing a json string to a dictionary.

I find it hard to read Ansible code. But I'm guessing that the call in get_role is calling boto3.client('iam').get_role(), which returns the existing policy as a string, not a dictionary.

If compare_policies is passed two identical policies, except one is a dict and one is a json string, after looking at this, I think it would say that they are different. (As opposed to throwing an exception.)

@tremble
Copy link
Contributor

tremble commented Jul 9, 2020

I can see that you're testing against 2.9.0, there's been some fairly major surgery to the module between the version in Ansible 2.9 and this collection. Are you able to reproduce the issue using the version from this repo?

@mdavis-xyz
Copy link
Contributor Author

I haven't tested against the latest version.

After the big split, is this collection usable yet? Do I just do pip install ansible --pre and then galaxy install community.aws?

Note that whilst I haven't executed the code to test my PR (due to #120 ) I have read the code in the master branch, and it looks like the bug is still there. Both in terms of functionality, and a missing not from the test.

I'll try executing with the latest release.

@tremble
Copy link
Contributor

tremble commented Jul 9, 2020

The 'not' in the test is correct. I've done a little testing and I think I've narrowed down the actual bug.

The following results in changed

- hosts: localhost
  collections:
  - amazon.aws
  - community.ws
  tasks:
  - name: "Create role for SMS logging"
    iam_role:
      name: testing-SNSSMSDeliveryStatusLogging
      assume_role_policy_document:
        Statement:
        - Action:
          - "sts:AssumeRole"
          Effect: Allow
          Principal:
            Service:
            - "sns.amazonaws.com"
      managed_policy:
        # let SNS log to CloudWatch
        - "arn:aws:iam::aws:policy/service-role/AmazonSNSRole"
      boundary: "arn:aws:iam::aws:policy/PowerUserAccess" # should be "{{ boundary_policy_arn }}"
      create_instance_profile: False # must be false when assigning a boundary policy

This, however, does not:

- hosts: localhost
  collections:
  - amazon.aws
  - community.ws
  tasks:
  - name: "Create role for SMS logging"
    iam_role:
      name: testing-SNSSMSDeliveryStatusLogging
      assume_role_policy_document:
        Statement:
        - Action:
          - "sts:AssumeRole"
          Effect: Allow
          Principal:
            Service:
            - "sns.amazonaws.com"
        Version: "2008-10-17"
      managed_policy:
        # let SNS log to CloudWatch
        - "arn:aws:iam::aws:policy/service-role/AmazonSNSRole"
      boundary: "arn:aws:iam::aws:policy/PowerUserAccess" # should be "{{ boundary_policy_arn }}"
      create_instance_profile: False # must be false when assigning a boundary policy

Notice the added "Version" in the policy

What's complex is that this would technically be a bug over in amazon.aws (the compare_policy function lives over there)

@tremble
Copy link
Contributor

tremble commented Jul 9, 2020

After the big split, is this collection usable yet? Do I just do pip install ansible --pre and then galaxy install community.aws?

I believe so, Yes

tremble added a commit to tremble/amazon.aws that referenced this issue Jul 9, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
tremble added a commit to tremble/amazon.aws that referenced this issue Jul 10, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
tremble added a commit to tremble/amazon.aws that referenced this issue Jul 10, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
tremble added a commit to tremble/amazon.aws that referenced this issue Jul 10, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
tremble added a commit to tremble/amazon.aws that referenced this issue Jul 10, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
tremble added a commit to tremble/amazon.aws that referenced this issue Jul 10, 2020
The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115
jillr added a commit to ansible-collections/amazon.aws that referenced this issue Jul 17, 2020
…omparisons (#98)

* Update compare_policies to add a Version string when missing

The Version component of an IAM policy is optional.  However, AWS will
automatically add a Version entry once a policy is uploaded.  This means
that comparing a 'live' policy to the policy that created it only gives
the correct result if we add a Version entry in when missing.

fixes: ansible-collections/community.aws#115

* Cope with missing/None policies

* update comment to match what's tested

Co-authored-by: Jill R <4121322+jillr@users.noreply.github.com>

Co-authored-by: Jill R <4121322+jillr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment