riboseinc / terraform-aws-iam-authenticating-group

Dynamically manage IAM group membership through an authenticated HTTPS endpoint
1 stars 2 forks source link

Add or delete users using IAM #26

Open erikbor opened 6 years ago

erikbor commented 6 years ago

Hi @phuonghuynh,

We have a variable for our user_names in iam_groups which is stored in an external file dyn-iam-access-users.tf:

iam_group_dyn_auth.tf:

module "dyn-iam-access" {
  source = "github.com/riboseinc/terraform-aws-iam-authenticating-group"

  providers = {
    aws = "aws"
  }

  name = "dyn-iam-access"
  description = "dyn-iam-access"
  time_to_expire = 300
  deployment_stage = "dev"
  log_level = "DEBUG"

  iam_groups = [
    {
      "group_name" = "${aws_iam_group.dyn_iam_access.name}",
      "user_names" = [ "${var.dyn-iam-access-users}" ]
    }
  ]
}

iam_group_dyn_iam.tf:

resource "aws_iam_policy" "dyn_iam_access" {
  name = "dyn-iam-access-policy"
  description = "${dyn-iam-access-policy"
  policy = "${data.aws_iam_policy_document.dyn_iam_access.json}"
}

data "aws_iam_policy_document" "dyn_iam_access" {
  statement {
    effect    = "Allow"
    actions   = [ "execute-api:Invoke" ]
    resources = [ "${module.dyn-iam-access.execution_resources}" ]
  }
}

resource "aws_iam_group" "dyn_iam_access" {
  name = "dyn-iam-access"
  path = "/dyn-iam-access/"
}

resource "aws_iam_group_policy_attachment" "dyn_iam_access_staff" {
  group      = "test-group"
  policy_arn = "${aws_iam_policy.dyn_iam_access.arn}"
}

dyn_iam_group_access_list.tf:

variable "dyn-iam-access-users" {
  default = [
    "user1",
    "user2"
  ]
}

Every time when we update the dyn-iam-access-users variable a new resource is required:

-/+ module.global.module.dyn-iam-access.module.python.template_dir.this (new resource required)
      id:                  "42bc2499467700bd32712400af21d76da7bdf015" => <computed> (forces new resource)
      destination_dir:     "/Users/test/test/.terraform/modules/7570797e3f23c67283886ad901bf48db/.src-JfSxV2e+zSxCS6PjoayRnzRr19M8koWAd5AMxZQyY0U=" => "/Users/test/test/.terraform/modules/7570797e3f23c67283886ad901bf48db/.src-Xn674YtlSl4JMPHBMHKNnarfy/QWj4qDy90LSmtc7fc=" (forces new resource)
      source_dir:          "/Users/test/test/.terraform/modules/7570797e3f23c67283886ad901bf48db/src" => "/Users/test/test/.terraform/modules/7570797e3f23c67283886ad901bf48db/src"
      vars.%:              "4" => "4"
      vars.iam_groups:     "[{\"group_name\":\"dyn-iam-access\",\"user_names\":[\"user1\",\"user2\"]}]" => "[{\"group_name\":\"dyn-iam-access\",\"user_names\":[\"user1\",\"user2\",\"user3\"]}]" (forces new resource)
      vars.log_level:      "DEBUG" => "DEBUG"
      vars.module_name:    "dyn-iam-access" => "dyn-iam-access"
      vars.time_to_expire: "300" => "300"

Can you change this so whenever we add or remove users the module just stays in place? Can you solve this with IAM group memberships or something? Then we'll just use IAM to add and remove those users.

phuonghuynh commented 6 years ago

I am on this

ronaldtse commented 6 years ago

@phuonghuynh I believe #20 will be fixed by this too since it will prevent the module being re-created on every change. Thanks!

phuonghuynh commented 6 years ago

Can we create some fake groups ? We can add/remove users to those fake groups. We also need to implement an Event to move users from fake groups to real groups.

ronaldtse commented 6 years ago

It’s better not to use fake groups because each IAM user can only have a maximum of 10 IAM groups, and this limit is not extendable.

phuonghuynh commented 6 years ago

@ronaldtse We can remove usernames param from the terraform config, then we add user when he invoke the API. That way, we dont need to rerun the terraform if we dont want to change groups. How you think?

since AWS limit number of groups

erikbor commented 6 years ago

@phuonghuynh

I'm testing your IAM/S3 changes. Can you make the following modifications please:

1) Can you give the bucket name an explicit name that can be set in the module instead of bucket = "${var.name}". That way it will be possible for us and other users to use prefixes and align with existing naming conventions. E.g. something like s3_bucket_name in the module configuration block. 2) Update the README

Thanks so far, I will get back to you again shortly

phuonghuynh commented 6 years ago

Yes, @erikbor

erikbor commented 6 years ago

@phuonghuynh nice one thank you.

Can you do two more things please:

  1. The "group_name" = "group_name_1" is now inside iam_groups, but when we use an external file on s3 then it will not be possible to insert a Terraform variable. For this particular module I think it's okay to have just a single group that be inside the module configuration. Then we don't have to hardcode the group name in the external file.
  2. Create some examples for the README using an external file. I have been experimenting with putting user_names in an external JSON and uploading it to S3 but I don't have any luck.

Thank you!!!

phuonghuynh commented 6 years ago

Yes @erikbor

phuonghuynh commented 6 years ago

@erikbor could you explain more details for # 1 ? The # 2 is fixed in last PR #29

erikbor commented 6 years ago

@phuonghuynh, thank you for fixing #2.

Regarding #1:

So this is the iam_groups.json file:

[
    {
        "group_name": "test1",
        "user_names": [
            "phuonghqh1", "phuonghqh2"
        ]
    },
    {
        "group_name": "test2",
        "user_names": [
            "phuonghqh1", "phuonghqh2"
        ]
    }
]

It is not possible to use Terraform variables for group_name, so we need to hardcode them.

What I want to propose is that:

  1. We don't do inline anymore because that requires the Lambda function to be always updated and is error prone. I don't see any problems or issues with forcing to store the JSON on S3.
  2. we simplify the JSON file and only have user_names in there and put a single group_name in the module configuration so it can use a Terraform variable.
erikbor commented 6 years ago

It would look like this:

main.tf:

module "dynamic-iam-group" {
  source         = "../../"
  name           = "example-dynamic-iam-groups"
  bucket_name    = "example-dynamic-iam-groups"
  description    = "example usage of terraform-aws-authenticating-iam"
  time_to_expire = 300
  log_level      = "DEBUG"
  group          = "${aws_iam_group.dev_dyn_access.name}"
  iam_users     = "iam_users.json"
}

iam_users.json:

[
    {
        "user_names": [
            "phuonghqh1", "phuonghqh2"
        ]
    }
]

What do you think?

Also the iam_users (which is the updated/renamed iam_groups) now does not uses file but is a name of the file on S3

phuonghuynh commented 6 years ago

@erikbor "iam_users.json" is local or S3 file? Lambda can not read local file if there is any updates.

group = "${aws_iam_group.dev_dyn_access.name}"

The module only work with 1 group ?

ronaldtse commented 6 years ago

@phuonghuynh I think what @erikbor meant was to use a static S3 file to store users. A bucket for this purpose could use a separate file per group too.

We can also create that S3 file using Terraform's aws_s3_bucket_object (https://www.terraform.io/docs/providers/aws/r/s3_bucket_object.html).

This means the lambda function won't have a direct dependency in terraform to this S3 file because the correlation is only at the path (s3://my-bucket/group-name.json)

phuonghuynh commented 6 years ago

@ronaldtse what do you think if use would like to use multi groups ? There is only one group in @erikbor's example

If user need an other group, he need to create new module, like:

module "dynamic-iam-group-2" {
  source         = "../../"
  name           = "example-dynamic-iam-groups-2"
  bucket_name    = "example-dynamic-iam-groups-2"
  description    = "example usage of terraform-aws-authenticating-iam"
  time_to_expire = 300
  log_level      = "DEBUG"
  group          = "${aws_iam_group.dev_dyn_access.other_name}"
  iam_users     = "iam_users.json"
}

this module will create an other Lambdas...

ronaldtse commented 6 years ago

Perhaps we can use multiple files to handle more than one group?

erikbor commented 6 years ago

Something like this?

module "dynamic-iam-group" {
  source         = "../../"
  name           = "example-dynamic-iam-groups"
  bucket_name    = "example-dynamic-iam-groups"
  description    = "example usage of terraform-aws-authenticating-iam"
  time_to_expire = 300
  log_level      = "DEBUG"
  groups = [
    {
        "group_name": "${aws_iam_group.dyn_iam_access.name}",
        "iam_user_file": "foo_bar_baz.json"
    },
    {
        "group_name": "${aws_iam_group.dyn_developers_access.name}",
        "iam_user_file": "masterfoo.json"
    }
  ]
}
phuonghuynh commented 6 years ago

Yes, thats good. I will implement python to load all files inside the bucket, and use the filename as group_name. Thus you can add/remove groups without rerun the module.

erikbor commented 6 years ago

@phuonghuynh nice one!

Can you confirm that upon a POST to the endpoint, Lambda/python will everytime load and parse the file(s) inside the bucket, and then use the JSON?

phuonghuynh commented 6 years ago

Can you confirm that upon a POST to the endpoint, Lambda/python will everytime load and parse the file(s) inside the bucket, and then use the JSON?

yes, Lambda should use S3 files as it inputs, both POST, DELETE and Cloud Watch Clear event

erikbor commented 6 years ago

thanks!!!

ronaldtse commented 6 years ago

@phuonghuynh could we have this completed soon? We are rather in a hurry. Thanks!

phuonghuynh commented 6 years ago

@ronaldtse committed in https://github.com/riboseinc/terraform-aws-iam-authenticating-group/pull/30/commits/2cfb7fa8b3a89ad6262267f92160511a1204ff72