Skip to content
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

feat!: Create least privilege default service account #1757

Merged

Conversation

abhikaddy
Copy link
Contributor

@abhikaddy abhikaddy commented Oct 2, 2023

Attempting to fix #1416

Fixes #1416

@abhikaddy abhikaddy requested review from Jberlinsky, ericyz and a team as code owners October 2, 2023 08:27
@apeabody
Copy link
Collaborator

apeabody commented Oct 2, 2023

/gcbrun

@abhikaddy
Copy link
Contributor Author

@Jberlinsky @ericyz

@abhikaddy
Copy link
Contributor Author

@apeabody Can I get what failed in the cloud build check?

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

@apeabody Can I get what failed in the cloud build check?

re-triggered the check

@abhikaddy
Copy link
Contributor Author

Hi Good Day!
Can I get a merge? @apeabody @ericyz @Jberlinsky

@apeabody apeabody changed the title feat: Create least privilege default service account #1416 feat!: Create least privilege default service account #1416 Oct 23, 2023
@apeabody apeabody changed the title feat!: Create least privilege default service account #1416 feat!: Create least privilege default service account Oct 23, 2023
Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @abhikaddy!

As this permissions reduction could be a breaking change for some users, can you please add a quick update note regarding the change to docs/upgrading_to_v29.0.md, similar to those in https://github.com/terraform-google-modules/terraform-google-kubernetes-engine/blob/master/docs

@abhikaddy
Copy link
Contributor Author

Hi @apeabody,

As requested, added docs/upgrading_to_v29.0.md.
Requesting a review.

Thanks.

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody self-assigned this Oct 25, 2023
@apeabody
Copy link
Collaborator

/gcbrun

@apeabody
Copy link
Collaborator

/gcbrun

@apeabody apeabody merged commit 350faa7 into terraform-google-modules:master Oct 26, 2023
4 checks passed
@shinebayar-g
Copy link

Are we supposed to create roles/container.nodeServiceAccount by ourselves? This role doesn't exist.

@legal90
Copy link
Contributor

legal90 commented Dec 21, 2023

That role exists, but it's not visible in GCP Console. It seems that it was deprecated (and hidden?).
gcloud CLI output confirms that: https://cloud.google.com/iam/docs/understanding-roles#container.nodeServiceAccount

$ gcloud iam roles describe roles/container.nodeServiceAccount
description: Least privilege role to use as the service account for GKE Nodes.
etag: AA==
includedPermissions:
- autoscaling.sites.writeMetrics
- logging.logEntries.create
- monitoring.metricDescriptors.create
- monitoring.metricDescriptors.list
- monitoring.timeSeries.create
- monitoring.timeSeries.list
- resourcemanager.projects.get
- resourcemanager.projects.list
- serviceusage.services.use
- storage.objects.get
- storage.objects.list
name: roles/container.nodeServiceAccount
stage: DEPRECATED                                   # <===
title: Kubernetes Engine Node Service Account

This role is also not mentioned here:
cc @apeabody . Should that PR be reverted?

P.s.
In my case I had to disable the SA creation (create_service_account = false) and created it outside of the module instead, as well as create bindings to roles/logging.logWriter, roles/monitoring.metricWriter`.

apeabody added a commit that referenced this pull request Dec 21, 2023
@apeabody
Copy link
Collaborator

Opened #1827

@gorge511
Copy link
Contributor

There is a new role named roles/container.nodeServiceAgent which should be used instead of deprecated roles/container.nodeServiceAccount. Should we migrate to this role instead of reverting the change? Or is there some other reason for revert which I'm not aware of?

$ gcloud iam roles describe roles/container.nodeServiceAgent  
description: Minimal set of permission required by a GKE node to support standard
  capabilities such as logging and monitoring export, and image pulls.
etag: AA==
includedPermissions:
- autoscaling.sites.writeMetrics
- logging.logEntries.create
- monitoring.metricDescriptors.create
- monitoring.metricDescriptors.list
- monitoring.timeSeries.create
- monitoring.timeSeries.list
- resourcemanager.projects.get
- resourcemanager.projects.list
- serviceusage.services.use
- storage.objects.get
- storage.objects.list
name: roles/container.nodeServiceAgent
stage: GA
title: Kubernetes Engine Node Service Agent

@shinebayar-g
Copy link

shinebayar-g commented Jan 12, 2024

There is a new role named roles/container.nodeServiceAgent which should be used

Is there any source that I can refer to? I thought service agent type of iam service accounts are created by GCP used internally only.

@legal90
Copy link
Contributor

legal90 commented Jan 12, 2024

@shinebayar-g You are right. Service Agent roles should not be granted to any principal except service agents:

Warning: Do not grant service agent roles to any principals except service agents. Some service agent roles contain very powerful permissions, and the permissions within these roles can change without notice. Instead, choose a different predefined role, or create a custom role with the permissions you need.

https://cloud.google.com/iam/docs/service-agents

@gorge511
Copy link
Contributor

Ok, you are both right. Agent roles should be used only for Agents, which in this case is probably just Autopilot node agent account.

I just got a message from Google representative that there is currently running effort updating this recommendation and there is new roles/container.defaultNodeServiceAccount currently in ALPHA stage which should replace the old one.

The list of permissions is shorter than in the old role, so I'm little bit scared of using it 🙂

$ gcloud iam roles describe roles/container.defaultNodeServiceAccount
description: Least privilege role to use as the default service account for GKE Nodes.
etag: AA==
includedPermissions:
- autoscaling.sites.writeMetrics
- logging.logEntries.create
- monitoring.metricDescriptors.create
- monitoring.metricDescriptors.list
- monitoring.timeSeries.create
- monitoring.timeSeries.list
- stackdriver.resourceMetadata.write
name: roles/container.defaultNodeServiceAccount
stage: ALPHA
title: Kubernetes Engine Default Node Service Account

Also I came across another docs page which is suggesting usage of 5 different roles.

So I'm confused now 😕

gorge511 added a commit to gorge511/terraform-google-kubernetes-engine that referenced this pull request Jan 13, 2024
This update follows changes from terraform-google-modules#1757 and reverts terraform-google-modules#1827.

The role `roles/container.nodeServiceAccount` is deprecated now and it is replaced with new `roles/container.defaultNodeServiceAccount` role.
Unfortunately this is not yet documented in Google docs.

As the scope of the new role is smaller than the old one, this should be considered breaking change.
@gorge511
Copy link
Contributor

I received confirmation from Google representative that the new role should be ok to use, so I prepared #1844.

 I've asked Engineering for clarification and recommendation about new role - it is good to go and use

@NeckBeardPrince
Copy link

Maybe I'm an outlier here, but anybody else feel a little uneasy switching to a new SA? That's currently, in ALPHA, and there is zero documentation on the new role? Perhaps, we shouldn't switch to using new roles that are still in ALPHA?

@hSATAC
Copy link

hSATAC commented Mar 12, 2024

Is there any downtime for the SA recreation for existing clusters? I simply want to upgrade the google provider...

@NeckBeardPrince
Copy link

Is there any downtime for the SA recreation for existing clusters? I simply want to upgrade the google provider...

No downtime.

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.

Create least privilege default service account
7 participants