Skip to content

feat: initial commit for module iam_users#6

Merged
sshi100 merged 10 commits intomainfrom
223-security-add-new-member
Sep 24, 2020
Merged

feat: initial commit for module iam_users#6
sshi100 merged 10 commits intomainfrom
223-security-add-new-member

Conversation

@sshi100
Copy link
Contributor

@sshi100 sshi100 commented Sep 18, 2020

Description

Please explain the changes you made here and link to any relevant issues.

Checklist

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

description = "Account ID of the current IAM user"
}

variable "iam_role_arns" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please put the proper type here? Also maybe we can call this "iam_role_mapping" or something to indicate that it's not just ARNs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

name = r.name
}
]
description = "List of role and users"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "List of mappings of users to roles"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +50 to +53
resource "aws_iam_role" "access_assumerole" {
count = length(local.role_users)
name = "${var.project}-kubernetes-${local.role_users[count.index].name}-${var.environment}"
assume_role_policy = data.aws_iam_policy_document.access_assumerole_policy[count.index].json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, these shouldn't be roles. We'll either want groups or to attach policies directly to users. Probably groups.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is kind of combining two things.. We need a way to specify a user's access to AWS resources, and then also a way to specify access to k8s. AWS access should probably be controlled via groups, and k8s access should be via roles, but then we also need a way to specify the access within k8s that each role has. Right now it's hard-coded below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can chat more about this tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

| cluster\_version | EKS cluster version number to use. Incrementing this will start a cluster upgrade | `any` | n/a | yes |
| environment | The environment (development/staging/production) | `any` | n/a | yes |
| iam\_account\_id | Account ID of the current IAM user | `any` | n/a | yes |
| iam\_role\_arns | IAM roles with arn | `any` | n/a | yes |
Copy link
Contributor

@bmonkman bmonkman Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs need to be regenerated with pre-commit run -a

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

policy_arn = aws_iam_policy.access_group[count.index].arn
}

## Group policy
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this if it's not necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

#}

## Users
resource "aws_iam_user" "access_user" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed in slack we'll need to consider how to give a user access to both prod and staging resources. Most likely conditionally use a -resource or data here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done with remote state

metadata {
name = var.roles[count.index].name
}
rule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll have to talk about how to configure this per group as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@@ -0,0 +1,36 @@
# eks
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong name and description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

group = aws_iam_group.access_group[count.index].name
policy_arn = aws_iam_policy.access_group[count.index].arn
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of having the assumerole policy included in the policy that is passed in, we should be creating it here. (this code from zero-aws-eks-stack:)

  statement {
    effect = "Allow"
    actions = [
      "iam:ListRoles",
      "sts:AssumeRole"
    ]
    resources = ["arn:aws:iam::<% index .Params `accountId` %>:role/<% .Name %>-kubernetes-developer-prod"]
  }

It shouldn't be up to the user to have to include that policy block, we can create it dynamically here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done automatically in module side

@github-actions
Copy link

Terraform Format and Style 🖌success

Terraform Initialization ⚙️success

Terraform Validation 🤖Success! The configuration is valid.

@sshi100 sshi100 merged commit 04f0d48 into main Sep 24, 2020
@sshi100 sshi100 deleted the 223-security-add-new-member branch September 24, 2020 22:16
@sshi100 sshi100 self-assigned this Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants