eks-prow-bulid-cluster: extract cluster admin IAM role to a module, fine-grained IAM permissions, docs #5113

xmudrii · 2023-04-10T17:05:41Z

This PR brings the following changes to eks-prow-build-cluster:

extract cluster admin IAM role to a submodule
- This allows us to create IAM role and attach needed policies to it before proceeding with the cluster creation
- Because of that, we can assume the created role and use it to create the cluster
  - Currently, we are creating the cluster using an IAM user, then assume the role only after the cluster is already created. This has some security implications, as the user that created the cluster is cluster administrator and owns all the resources (e.g. KMS keys)
replace AdministratorAccess with fine-grained IAM permissions
- Using AdministratorAccess is risky and not recommended
- @rothgar recommended using iamlive to record required permissions which did the trick 🎉
- I used the CSM mode to record the permissions
- We have two different policies: one for maintaining the cluster and one for destroying the cluster
  - The idea behind this is to increase security and give folks ability to maintain the cluster, but not to destroy it
README is updated with additional explanations and changes introduced in this PR

TODO:

Try to get even more fine-grained IAM permissions
- I used the CSM mode, but the proxy mode might give better results, so I'll give it a try
- This is for a follow up, let's get started with this and then we can iterate later

All changes are applied to the canary cluster. I'll apply changes to the production cluster once the PR is merged.

/assign @pkprzekwas @ameukam @dims @rothgar

Signed-off-by: Marko Mudrinić <[email protected]>

xmudrii · 2023-04-10T17:06:25Z

/hold
for review

xmudrii · 2023-04-10T17:07:20Z

/assign @sftim

Signed-off-by: Marko Mudrinić <[email protected]>

dims · 2023-04-11T19:14:07Z

looks good @xmudrii thanks for confirming that this is already in the canary cluster. please merge when you are ready.

/approve
/lgtm

rothgar · 2023-04-11T19:51:55Z

I didn't run the make targets but the IAM looks like it does a good job separating the ability to create/manage a cluster vs delete 👍

infra/aws/terraform/prow-build-cluster/modules/iam/policies.tf

infra/aws/terraform/prow-build-cluster/modules/iam/users.tf

infra/aws/terraform/prow-build-cluster/README.md

sftim

Some feedback; hope it helps!

sftim · 2023-04-12T14:13:42Z

infra/aws/terraform/prow-build-cluster/README.md

+Destroy:
+
+```bash
+make destroy


Is this equivalent to TF_ARGS=-destroy make apply?

Yes, it should be. For the reference:

-destroy Destroy Terraform-managed infrastructure. The command "terraform destroy" is a convenience alias for this option.

sftim · 2023-04-12T14:16:54Z

infra/aws/terraform/prow-build-cluster/modules/iam/policies.tf

+resource "aws_iam_policy" "prow_cluster_maintainer" {
+  name   = "ProwClusterMaintainer"
+  path   = "/"
+  policy = data.aws_iam_policy_document.prow_cluster_maintainer.json
+}


It looks like this policy allows escalation to AdministratorAccess. We could use that (AdministratorAccess) policy instead for now; it's simpler, and more obvious that the policy carries a risk.

Longer term, we might want to further limit the privileges that we provide here.

Can you help me understand how the policy allows it to be escalated to administrator access?

We discussed using permissions boundary at KubeCon. @pkprzekwas will follow up on and create tracking issues.

sftim · 2023-04-12T14:17:47Z

infra/aws/terraform/prow-build-cluster/modules/iam/policies.tf

+      "iam:DeletePolicy",
+      "iam:DeleteRole",
+      "iam:DeleteRolePolicy",
+      "iam:DetachRolePolicy",


Bear in mind that this can remove deny rules. A comment to that effect might be helpful.

A comment will be added.

infra/aws/terraform/prow-build-cluster/modules/iam/roles.tf

infra/aws/terraform/prow-build-cluster/variables.tf

Signed-off-by: Marko Mudrinić <[email protected]>

infra/aws/terraform/prow-build-cluster/modules/iam/users.tf

Signed-off-by: Marko Mudrinić <[email protected]>

pkprzekwas · 2023-04-24T12:49:25Z

/lgtm
I think it's fine to merge it now. IAM concerns will be addressed in a separate issue.

dims · 2023-04-24T15:28:09Z

thanks @pkprzekwas

/approve
/lgtm

k8s-ci-robot · 2023-04-24T15:28:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, xmudrii

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~infra/aws/terraform/prow-build-cluster/OWNERS~~ [dims,xmudrii]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

xmudrii · 2023-04-24T16:32:39Z

I'm applying this PR to the canary cluster, but suddenly Terraform wants to update this resource:

  # module.eks.aws_iam_openid_connect_provider.oidc_provider[0] will be updated in-place
  ~ resource "aws_iam_openid_connect_provider" "oidc_provider" {
        id              = "arn:aws:iam::468814281478:oidc-provider/oidc.eks.us-east-2.amazonaws.com/id/BE445C1D51F231A55B2A0E3D0140D458"
        tags            = {
            "Name" = "prow-build-canary-cluster-eks-irsa"
        }
      ~ thumbprint_list = [
            # (2 unchanged elements hidden)
            "414a2060b738c635cc7fc243e052615592830c53",
          - "53011a7515ca46ed6233168766a0f1729608be0e",
          + "50879ea7f7c29dd615269e559fb061b46bdd3dbe",
        ]
        # (4 unchanged attributes hidden)
    }

As per this Slack thread, we'll proceed with this update.

Signed-off-by: Marko Mudrinić <[email protected]>

xmudrii · 2023-04-24T16:37:13Z

I had to add some additional permissions in 20d1da9 to be able to reconcile #5113 (comment)

pkprzekwas · 2023-04-24T16:39:14Z

/lgtm

pkprzekwas · 2023-04-24T16:39:46Z

Further IAM improvements will be introduced with #5160

xmudrii · 2023-04-24T17:27:52Z

Applied to both prod and canary
/hold cancel

xmudrii added 2 commits April 10, 2023 18:53

Extract IAM role to a module

6ec0cff

Signed-off-by: Marko Mudrinić <[email protected]>

Use fine-grained IAM permissions

f9651a4

Signed-off-by: Marko Mudrinić <[email protected]>

k8s-ci-robot assigned ameukam, dims, pkprzekwas and rothgar Apr 10, 2023

k8s-ci-robot requested review from dims and pkprzekwas April 10, 2023 17:05

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 10, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 10, 2023

k8s-ci-robot assigned sftim Apr 10, 2023

Extend documentation for eks-prow-build-cluster

c70c8c2

Signed-off-by: Marko Mudrinić <[email protected]>

xmudrii force-pushed the eks-prow-cluster-iam branch from 9a615e6 to c70c8c2 Compare April 10, 2023 17:09

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2023

pkprzekwas reviewed Apr 12, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/modules/iam/policies.tf Show resolved Hide resolved

pkprzekwas reviewed Apr 12, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/modules/iam/users.tf Outdated Show resolved Hide resolved

pkprzekwas reviewed Apr 12, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/README.md Show resolved Hide resolved

sftim reviewed Apr 12, 2023

View reviewed changes

xmudrii added 2 commits April 24, 2023 12:26

Minor fixes

5d4f0c5

Signed-off-by: Marko Mudrinić <[email protected]>

Add ProwClusterViewer policy

3ac0b41

Signed-off-by: Marko Mudrinić <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2023

Add ProwClusterViewer policy and address comments

0f3e512

Signed-off-by: Marko Mudrinić <[email protected]>

xmudrii force-pushed the eks-prow-cluster-iam branch from 4d97d82 to 0f3e512 Compare April 24, 2023 10:44

pkprzekwas reviewed Apr 24, 2023

View reviewed changes

infra/aws/terraform/prow-build-cluster/modules/iam/users.tf Outdated Show resolved Hide resolved

Parametrize list of cluster admins

b0996d9

Signed-off-by: Marko Mudrinić <[email protected]>

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2023

pkprzekwas mentioned this pull request Apr 24, 2023

eks-prow-build-cluster: IAM refactoring and improvements #5160

Closed

2 tasks

Add missing permissions

20d1da9

Signed-off-by: Marko Mudrinić <[email protected]>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 24, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 24, 2023

k8s-ci-robot merged commit ff0e43e into kubernetes:main Apr 24, 2023

k8s-ci-robot added this to the v1.28 milestone Apr 24, 2023

xmudrii deleted the eks-prow-cluster-iam branch April 24, 2023 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eks-prow-bulid-cluster: extract cluster admin IAM role to a module, fine-grained IAM permissions, docs #5113

eks-prow-bulid-cluster: extract cluster admin IAM role to a module, fine-grained IAM permissions, docs #5113

xmudrii commented Apr 10, 2023

xmudrii commented Apr 10, 2023

xmudrii commented Apr 10, 2023

dims commented Apr 11, 2023

rothgar commented Apr 11, 2023

sftim left a comment

sftim Apr 12, 2023

xmudrii Apr 24, 2023

sftim Apr 12, 2023

rothgar Apr 12, 2023

xmudrii Apr 24, 2023 •

edited

Loading

sftim Apr 12, 2023

xmudrii Apr 24, 2023

pkprzekwas commented Apr 24, 2023

dims commented Apr 24, 2023

k8s-ci-robot commented Apr 24, 2023

xmudrii commented Apr 24, 2023

xmudrii commented Apr 24, 2023

pkprzekwas commented Apr 24, 2023

pkprzekwas commented Apr 24, 2023

xmudrii commented Apr 24, 2023

eks-prow-bulid-cluster: extract cluster admin IAM role to a module, fine-grained IAM permissions, docs #5113

eks-prow-bulid-cluster: extract cluster admin IAM role to a module, fine-grained IAM permissions, docs #5113

Conversation

xmudrii commented Apr 10, 2023

xmudrii commented Apr 10, 2023

xmudrii commented Apr 10, 2023

dims commented Apr 11, 2023

rothgar commented Apr 11, 2023

sftim left a comment

Choose a reason for hiding this comment

sftim Apr 12, 2023

Choose a reason for hiding this comment

xmudrii Apr 24, 2023

Choose a reason for hiding this comment

sftim Apr 12, 2023

Choose a reason for hiding this comment

rothgar Apr 12, 2023

Choose a reason for hiding this comment

xmudrii Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

sftim Apr 12, 2023

Choose a reason for hiding this comment

xmudrii Apr 24, 2023

Choose a reason for hiding this comment

pkprzekwas commented Apr 24, 2023

dims commented Apr 24, 2023

k8s-ci-robot commented Apr 24, 2023

xmudrii commented Apr 24, 2023

xmudrii commented Apr 24, 2023

pkprzekwas commented Apr 24, 2023

pkprzekwas commented Apr 24, 2023

xmudrii commented Apr 24, 2023

xmudrii Apr 24, 2023 •

edited

Loading