Skip to content

Added support for assuming administrator roles. #7

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ryankurte
Copy link
Collaborator

This should support https://github.com/99designs/aws-vault with mfa on assume-role and is towards removing admin privileges from base aws keys.

Implementation loosely based on https://github.com/cloudposse/terraform-aws-iam-assumed-roles/blob/master/main.tf.

Needs a sanity check before deploying, but it's only adding roles atm so should be safe?

plan:

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create
 <= read (data resources)

Terraform will perform the following actions:

 <= data.aws_iam_policy_document.assume_role_admin
      id:                             <computed>
      json:                           <computed>
      statement.#:                    "1"
      statement.0.actions.#:          "1"
      statement.0.actions.2528466339: "sts:AssumeRole"
      statement.0.effect:             "Allow"
      statement.0.resources.#:        <computed>

  + aws_iam_group_policy_attachment.assume_role_admin
      id:                             <computed>
      group:                          "Administrators"
      policy_arn:                     "${aws_iam_policy.assume_role_admin.arn}"

  + aws_iam_policy.assume_role_admin
      id:                             <computed>
      arn:                            <computed>
      description:                    "Allow administrators to assume admin role"
      name:                           "administrators-permit-assume-role"
      path:                           "/"
      policy:                         "${data.aws_iam_policy_document.assume_role_admin.json}"

  + aws_iam_role.administrators
      id:                             <computed>
      arn:                            <computed>
      assume_role_policy:             "{\n  \"Version\": \"2012-10-17\",\n  \"Statement\": [\n    {\n      \"Sid\": \"\",\n      \"Effect\": \"Allow\",\n      \"Action\": \"sts:AssumeRole\",\n      \"Principal\": {\n        \"AWS\": \"arn:aws:iam::537658973298:root\"\n      }\n    }\n  ]\n}"
      create_date:                    <computed>
      force_detach_policies:          "false"
      max_session_duration:           "3600"
      name:                           "assume-admin"
      path:                           "/"
      unique_id:                      <computed>


Plan: 3 to add, 0 to change, 0 to destroy.

This is towards supporting https://github.com/99designs/aws-vault with mfa on assume-role and removing admin privileges from aws keys, loosely based on https://github.com/cloudposse/terraform-aws-iam-assumed-roles/blob/master/main.tf.

Needs a sanity check before deploying, but it's only adding roles atm so _should_ be safe?
@ryankurte ryankurte requested a review from nastevens November 7, 2018 01:13
@nastevens
Copy link
Member

This looks great - I much prefer the assume-role setup from a security perspective (that's what we use at work). I'd say go ahead and do the apply of this on your end so we get a confirmation that everything works there.

Also, as a general note: if you need to do any iterative development (i.e. multiple terraform apply cycles to get everything working), please feel free. How do the following guidelines sound?

  1. Make an initial PR review to get eyes on the goals of the change
  2. Any changes that get a terraform apply are committed to git before the apply
  3. Terraform state files are committed to git after every apply
  4. Push any changes beyond the initial PR review after iterative development is done, and then merge the PR

That's pretty much what I did with the DNS stuff... sometimes unfortunately it takes a couple revs to get this stuff right as there's not a great way to test without having a separate dev environment or the like.

@ryankurte
Copy link
Collaborator Author

I was actually going to suggest we move to S3 backed state which will mitigate the need for a bunch of commits / act as a mutex for changes, but the approach sounds roughly good to me ^_^

@ryankurte
Copy link
Collaborator Author

hmm, not yet working as expected, now to work out why ^_^

Added to ~/.aws/config

[profile rust-embedded]
region = us-east-1
output=json
source_profile=default
role_arn=arn:aws:iam::537658973298:role/assume-admin
role_session_name=ryankurte

I can't see a policy attached to the assume-admin role in the cloud console, not sure whether that's it.

@nastevens
Copy link
Member

I was actually going to suggest we move to S3 backed state

I agree 1000%, I definitely want this. Created #8.

@nastevens
Copy link
Member

nastevens commented Nov 11, 2018

hmm, not yet working as expected, now to work out why ^_^

Just tried as well and no luck. One thing I noticed - the policy attached to assume-admin only gives access to assume the assume-admin role:

"Action": "sts:AssumeRole",
"Resource": "arn:aws:iam::537658973298:role/assume-admin"

Isn't that kind of circular? I.e. should the policy for the role assume-admin be the equivalent of admin permissions?

 {
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": "*",
      "Resource": "*"
    }
  ]
}

The administrators-permit-assume-role attached to the administrators group is what actually prevents someone from leveraging the assume-admin role.

@ryankurte
Copy link
Collaborator Author

Hmm yeah that does seem circular, I must have missed something in that example.
Will keep playing with it / dial a terraform friend later in the week.

@ryankurte
Copy link
Collaborator Author

Havent had a chance to look yet but https://github.com/duckalini/my_first_terraform (released at our con yesterday ^_^) has examples of assume role use that might help.

@ryankurte ryankurte mentioned this pull request Nov 20, 2018
@mathk
Copy link

mathk commented Feb 25, 2019

Ping from triage:I am a bit lost for tracking this PR. we have one approve but more commit behind. Do we still need a review ?

@nastevens nastevens self-requested a review February 27, 2019 13:23
@nastevens nastevens added S-waiting-on-author Status: Author needs to make changes and removed S-waiting-on-review labels Feb 27, 2019
@nastevens
Copy link
Member

@mathk I flipped back to needing a review and added the "waiting on author" tag - @ryankurte has some issues with the in-process changes that he's trying to resolve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: Author needs to make changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants