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

[FEATURE]: Allow custom annotations for image builders so that IRSA can be used #2931

Closed
ohookins opened this issue May 5, 2022 · 6 comments · Fixed by #3384
Closed

[FEATURE]: Allow custom annotations for image builders so that IRSA can be used #2931

ohookins opened this issue May 5, 2022 · 6 comments · Fixed by #3384

Comments

@ohookins
Copy link
Contributor

ohookins commented May 5, 2022

Feature Request

Background / Motivation

According to the AWS EKS Best Practices Guide, when using IRSA you should set the instance metadata service to IMDSv2 and the hop count to 1. This means that pods will no longer be able to talk to the regular instance metadata endpoint and inherit the role of the node.

Under normal conditions, kaniko or cluster-buildkit will just use the default Service Account, and not have any specific IAM role that grants it ECR push permissions, so it will rely on the instance role and ECR push permissions associated with it. If we want to enforce the above security settings, any ECR pushes will fail. Since there's no opportunity to add annotations to these pods, we can't tell IRSA which IAM role the builder should assume, and hence can't give it the right permissions. This will break effectively all modules using in-cluster built images.

What should the user be able to do?

Add a configuration in the Garden project configuration associated with the chosen builder service, that adds the K8s annotation to the builder's service account. This probably also means that a build service account needs to be created by Garden.

Why do they want to do this? What problem does it solve?

See the description above.

Suggested Implementation(s)

This could just be added to the existing configuration map that you pass for the builder in the project configuration. I'm not sure how things like for GKE or AKS works with respect to pod roles, so I guess it would be good to have a solution that works equally well for those platforms (although I'm not using them).

How important is this feature for you/your team?

🌵 Not having this feature makes using Garden painful

It's a non-functional requirement as far as the day to day developer experience goes, but not having this means that any Garden pod can assume the instance IAM role, and use any permissions a given EKS node can use. This is not great for security.

@thsig
Copy link
Collaborator

thsig commented Jun 9, 2022

Hi @ohookins! Thanks for the detailed suggestion here.

Would you be interested in implementing this functionality and submitting a PR?

We'd be happy to hop on a call and do some pair-programming on this, and to point you to the right places in the codebase to get started (and answer any questions that may come up).

@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it hasn't had any activity in 90 days. It will be closed in 14 days if no further activity occurs (e.g. changing labels, comments, commits, etc.). Please feel free to tag a maintainer and ask them to remove the label if you think it doesn't apply. Thank you for submitting this issue and helping make Garden a better product!

@stale stale bot added the stale Label that's automatically set by stalebot. Stale issues get closed after 14 days of inactivity. label Sep 21, 2022
@vvagaytsev vvagaytsev removed the stale Label that's automatically set by stalebot. Stale issues get closed after 14 days of inactivity. label Sep 22, 2022
@stefreak stefreak assigned stefreak and unassigned Orzelius Nov 15, 2022
@stefreak
Copy link
Member

Just so nobody else duplicates the effort: I am actively working on this :)

@stefreak
Copy link
Member

One thing I noticed during the investigation is that a service account would also be useful for the tasks.

First implementation will only target in-cluster-building, but I will at least create another feature request issue for allowing for IRSA (or injecting credentials through service accounts in general) also for tasks.

stefreak added a commit that referenced this issue Nov 25, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
stefreak added a commit that referenced this issue Dec 1, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
stefreak added a commit that referenced this issue Dec 1, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
stefreak added a commit that referenced this issue Dec 1, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
stefreak added a commit that referenced this issue Dec 14, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
stefreak added a commit that referenced this issue Dec 15, 2022
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
@stefreak
Copy link
Member

We should make sure, when implementing this, that it also can be used to enable using GCP's workload identity: https://cloud.google.com/kubernetes-engine/docs/concepts/workload-identity

@stefreak stefreak removed their assignment Jun 2, 2023
twelvemo pushed a commit that referenced this issue Nov 1, 2023
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
twelvemo pushed a commit that referenced this issue Nov 14, 2023
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
twelvemo pushed a commit that referenced this issue Nov 14, 2023
This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931
github-merge-queue bot pushed a commit that referenced this issue Nov 14, 2023
#3384)

* feat(k8s): add service account and irsa support for in-cluster-builder

This commit introduces a new service account, and makes the annotations
configurable in kaniko and buildkit in-cluster-builders.

This change enables the use of IRSA for in-cluter-building which makes
it more secure.

Fixes #2931

* docs(irsa): add docs for using in-cluster-building with IRSA

I decided to replace the existing docs, because the existing approach
doesn't provide defense-in-depth. Even in dev clusters, the access to
the Docker registries should be restricted to as few places as
possible, to make it harder for attackers to push bad images.

* chore: fix test

* Update core/src/plugins/kubernetes/container/build/common.ts

* chore: resolve conflicts on rebase

* chore: resolve more conflicts on rebase

* revert in-cluster-building.md

* docs: add irsa separately

* chore: add gcr credential helper to builder and util images

* chore: fix serviceAccount for kaniko and support  using different kaniko namespace

* chore: fix configschema

* chore: update reference docs

* chore: fix typo in kubernetes config.ts

* chore: remove var used for debugging

* chore: fix test-framework

* chore: add namespace to serviceAccount manifest

* chore: add integ tests

* chore: remove namespace from util manifest since it is always in project namepsace

* chore: use kaniko and buildkit envs for integ tests

* chore: add docs for google workload identity

* chore: move tests to their own suite

* chore: fix docs

* chore: remove unused kube api in test

* chore: add assertions and use isEqual

* Update core/test/integ/src/plugins/kubernetes/container/build/build.ts

Co-authored-by: Steffen Neubauer <[email protected]>

* chore: use constant

* chore: compare resources in test for better output

---------

Co-authored-by: Anna Mager <[email protected]>
Co-authored-by: Anna Mager <[email protected]>
@stefreak
Copy link
Member

IRSA for Runs is already possible in Garden 0.13 via the kubernetes-pod Run type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants