ansible-community / molecule-plugins

Collection on molecule plugins
MIT License
114 stars 75 forks source link

Proposal: playbook to grant minimal required permissions #126

Open beargiles opened 1 year ago

beargiles commented 1 year ago

This is based on current experience. (augh!)

We could use a cloud account with unlimited permissions... but that would be a Bad Idea. It's much better to use an account with the minimum required permissions.

(The user will be responsible for adding any additional permissions required by their role.)

It might seem to be unnecessary but I've spent over a day trying to track down a problem due to the EC2 RunInstances requiring an overlooked permission. I still don't know what it is - just that even full 'ReadOnlyAccess' fails with an opaque 'Unauthorized Operation' error while adding 'FullAccess' succeeds.

This is clearly something we should document anyway - but since we're creating ansible tasks it makes sense to provide one that adds the minimum required permissions to an account.

(The details will vary by cloud service, of course, but for AWS it would make more sense to create a custom policy and then let the user add that policy to the test accounts vs. having a task directly update the permissions on a specific account.)

beargiles commented 1 year ago

(P.S., I know there's a way to identify the permissions actually required over a period of time. It simply hadn't occurred to me that this was the cause - I've been focused on verifying that the proper values were being provided to the amazon.aws module.)

beargiles commented 1 year ago

Here's my local policy, rewritten as a template. Ansible playbook to create this policy to follow...

If you want to be really paranoid you could split this into a read-only policy and a write-only policy that's created at runtime once you know the ephemeral key_name, ephemeral security_group, and instance_id(s). This policy would be added and removed from the molecule user's/role's IAM policy. That would make it impossible for the molecule driver to modify or delete anything other than the ephemeral resources it creates.

But the same paranoia will ask if we really want to give the molecule driver the ability to edit IAM policies. It could certainly be limited to modifying a single write-only policy shared by all molecule tests - but I don't think we can limit it to only modifying specific permissions. (I could be wrong though.) However there may be solutions to this, e.g., doing that via a carefully vetted lambda function.

(Hmm...)

#
# Minimal IAM policy for Molecule EC2 driver
#
# Note: separate policies should be created for any tests requiring
# additional access. Those policies can be attached to the IAM user/role
# that runs the molecule tests.
#
# Required Properties
#
# - subnet_id
#
# Recommended Properties
#
# - account
# - image_id | default('*') (as list)
# - region
#
# Optional Properties
#
# - instance_id
# - keypair_name
# - network_interface_id
# - security_group_id
# - vpc_id
#
# Note: if no keypair_name is specified the template will use
# 'molecule-*' instead of just '*'. This reduces the chance of
# the playbooks accidently deleting an unrelated key. 
#
# Best Practices
#
# - Run all tests as a 'molecule user' and/or an assumed 'molecule role'.
#   Both should use the IAM policy below.
#
# - Run the molecule tests in an isolated VPC (ideally), or at least
#   an isolated subnet (mandatory).
#
# - Use an explicit list of acceptable AMI images. This may not be
#   possible if you use a product code so you're always testing the
#   most recent version of the AMI.
#
# This stanza (in molecule.yml) will use a product code instead of
# an explicit image id.
#
#    image_filters:
#       product-code: $ROCKY_LINUX_9_PRODUCT_CODE
#       product-code.type: marketplace
#
# Ansible playbook to create this policy to follow...
#
# Implementation note - I thought there was a "| required()" or
# "| mandatory()" filter but I can't find it now. It should be
# added to the 'subnet_id' since this is a mandatory field in
# the molecule.yml file and it adding it to the IAM profile
# can dramatically reduce the risk of unintended ec2 terminations.
#
- molecule_iam_policy:
    - Effect: Allow
      Action:
        - "ec2:DescribeImages"
        - "ec2:DescribeInstances"
        - "ec2:DescribeInstanceStatus"
        - "ec2:DescribeInstanceTypeOfferings"
        - "ec2:DescribeInstanceTypes"
        - "ec2:DescribeKeyPairs"
        - "ec2:DescribeRegions"
        - "ec2:DescribeSecurityGroupRules"
        - "ec2:DescribeSecurityGroups"
        - "ec2:DescribeSnapshots"
        - "ec2:DescribeStaleSecurityGroups"
        - "ec2:DescribeSubnets"
        - "ec2:DescribeTags"
        - "ec2:DescribeVolumes"
        - "ec2:DescribeVolumesModifications"
        - "ec2:DescribeVolumeStatus"
        - "ec2:DescribeVpcs"
      Resource:
        - '*'

    - Effect: Allow
      Action:
        - "ec2:DescribeInstanceAttribute"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:instance/{{ instance_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:security-group/{{ security_group_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:subnet/{{ subnet_id }}"

    - Effect: Allow 
      Action:
        - "ec2:CreateTags"
        - "ec2:DeleteTags"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:key-pair/{{ keypair_name | default('molecule-*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:instance/{{ instance_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:security-group/{{ security_group_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:subnet/{{ subnet_id }}"

    - Effect: Allow
      Action:
        - "ec2:CreateKeyPair"
        - "ec2:DeleleKeyPair"
        - "ec2:ImportKeyPair"
      Resource:
         - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:key-pair/{{ keypair_name | default('molecule-*') }}"

    - Effect: Allow
      Action:
        - "ec2:AuthorizeSecurityGroupEgress"
        - "ec2:AuthorizeSecurityGroupIngress"
        - "ec2:CreateSecurityGroup"
        - "ec2:DeleteSecurityGroup"
        - "ec2:UpdateSecurityGroupRuleDescriptionsEgress"
        - "ec2:UpdateSecurityGroupRuleDescriptionsIngress"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:security-group/{{ security_group_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:vpc/{{ vpc_id | default('*') }}"

    - Effect: Allow
      Action:
        - "ec2:RunInstances"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:image/{{ image_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:instance/{{ instance_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:key-pair/{{ keypair_name | default('molecule-*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:network-interface/{{ network_interface_id |default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:security-group/{{ security_group_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:subnet/{{ subnet_id }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:volume/{{ volume_id | default('*') }}"

    - Effect: Allow
      Action:
        - "ec2:DescribeInstanceAttribute"
        - "ec2:RebootInstances"
        - "ec2:StartInstances"
        - "ec2:StopInstances"
        - "ec2:TerminateInstances"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:instance/{{ instance_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:key-pair/{{ keypair_name | default('molecule-*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:network-interface/{{ network_interface_id |default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:security-group/{{ security_group_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:subnet/{{ subnet_id }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:volume/{{ volume_id | default('*') }}"

    - Effect: Allow
      Action:
        - "ec2:AttachVolume"
        - "ec2:CreateVolume"
        - "ec2:DeleteVolume"
        - "ec2:DetachVolume"
        - "ec2:ModifyVolume"
        - "ec2:ModifyVolumeAttribute"
      Resource:
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:instance/{{ instance_id | default('*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:key-pair/{{ keypair_name | default('molecule-*') }}"
        - "arn:aws:ec2:{{ region | default(omit) }}:{{ account | default(omit) }}:volume/{{ volume_id | default('*') }}"
apatard commented 1 year ago

I guess that anything improving security is welcome. The thing is that I fear I won't be able to review it: I'm not a ec2 user so it's hard to me to fully understand and test the PR.

zhan9san commented 1 year ago

@beargiles

It's a good idea.

Would you mind adding the minimal required permissions to the EC2 driver document?

As for the format, I don't think it need to be limited in ansible-playbook(amazon.aws module). How to create IAM policy is outside the scope of this project.

I suggest we follow the IAM native JSON policy syntax like how Jenkins EC2 plugin does,

e.g.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Sid": "Stmt1312295543082",
            "Action": [
                "ec2:DescribeSpotInstanceRequests",
                "ec2:CancelSpotInstanceRequests",
                "ec2:GetConsoleOutput",
                "ec2:RequestSpotInstances",
                "ec2:RunInstances",
                "ec2:StartInstances",
                "ec2:StopInstances",
                "ec2:TerminateInstances",
                "ec2:CreateTags",
                "ec2:DeleteTags",
                "ec2:DescribeInstances",
                "ec2:DescribeInstanceTypes",
                "ec2:DescribeKeyPairs",
                "ec2:DescribeRegions",
                "ec2:DescribeImages",
                "ec2:DescribeAvailabilityZones",
                "ec2:DescribeSecurityGroups",
                "ec2:DescribeSubnets",
                "iam:ListInstanceProfilesForRole",
                "iam:PassRole",
                "ec2:GetPasswordData"
            ],
            "Effect": "Allow",
            "Resource": "*"
        }
    ]
}
beargiles commented 1 year ago

I have it in yml format solely because I think that's the format amazon.aws uses. I haven't gotten to that point yet.