Skip to content

capg/image-builder: add extra role to be able to create gcp vms#2124

Merged
k8s-ci-robot merged 1 commit intokubernetes:mainfrom
cpanato:capg-imagebuilder
Jun 4, 2021
Merged

capg/image-builder: add extra role to be able to create gcp vms#2124
k8s-ci-robot merged 1 commit intokubernetes:mainfrom
cpanato:capg-imagebuilder

Conversation

@cpanato
Copy link
Copy Markdown
Member

@cpanato cpanato commented Jun 3, 2021

Add extra role "roles/iam.serviceAccountUser" for the service account

more context: kubernetes-sigs/image-builder#625 (comment)

/assign @ameukam @spiffxp @dims

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. wg/k8s-infra labels Jun 3, 2021
@k8s-ci-robot k8s-ci-robot requested review from dims and thockin June 3, 2021 11:52
@ameukam
Copy link
Copy Markdown
Member

ameukam commented Jun 4, 2021

Little experimentation done here : kubernetes-sigs/image-builder#625 (comment)

@ameukam
Copy link
Copy Markdown
Member

ameukam commented Jun 4, 2021

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2021
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ameukam, cpanato

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1fbe1ff into kubernetes:main Jun 4, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 4, 2021
@ameukam
Copy link
Copy Markdown
Member

ameukam commented Jun 4, 2021

Running: ./infra/gcp/ensure-staging-storage.sh

Copy link
Copy Markdown
Contributor

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

Post merge review


ensure_services "${STAGING_PROJECT}" compute.googleapis.com
ensure_project_role_binding "${STAGING_PROJECT}" "serviceAccount:${serviceaccount}" "roles/compute.instanceAdmin.v1"
ensure_project_role_binding "${STAGING_PROJECT}" "serviceAccount:${serviceaccount}" "roles/iam.serviceAccountUser"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@cpanato @ameukam So the issue I have with this is that it allows this service account to act as any other service account in this project. That's privilege escalation waiting to happen. Can we not constrain this binding to the actual service account that needs to be used?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the same comment I had here #2061 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@spiffxp we could bind the service account to the default SA for the compute service.

gcloud iam service-accounts list --project k8s-staging-cluster-api-gcp | grep 'compute'
Compute Engine default service account                                         606075400249-compute@developer.gserviceaccount.com                               False

I'll investigate this week-end.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

sorry for the delay to reply, i tried to be off this weekend

How can we fix that? I will try to create the same environment again and apply other roles to check.
but the comment from @ameukam is something that we can do?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Basically it would help to know what service account packer is trying to use, so we could apply this role just to that service account

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought the service account it is using is the one we set in the Job Config : https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/wg-k8s-infra/trusted/image-builder/image-builder-periodics.yaml#L12

it is available in the container and packer uses that one.
This is my understanding, now I don't know if that is correct. :/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The service account that the job (and thus packer) runs as is what we set in the job config, correct. However packer's instructions [1] indicate that whatever is running packer needs the Service Account User role, meaning they expect packer to be able to impersonate a different service account [2]

I'm going to take a wild guess that this is to allow packer to attach the service account it's currently running as to any other instances it happens to create [4]. So probably narrow the binding to itself. I would hope that logs or the code could confirm the guess and allow us to narrow the binding's scope.

References:
[1]: https://www.packer.io/docs/builders/googlecompute#running-on-google-cloud
[2]: https://cloud.google.com/iam/docs/impersonating-service-accounts
[3]: https://cloud.google.com/iam/docs/impersonating-service-accounts#attaching-to-resources
[4]: https://cloud.google.com/compute/docs/access/service-accounts#associating_a_service_account_to_an_instance

@cpanato cpanato deleted the capg-imagebuilder branch June 7, 2021 11:07
ameukam added a commit to ameukam/k8s.io that referenced this pull request Jun 7, 2021
Revert of kubernetes#2124 which
introduces an exposure to privilege escalation.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
@ameukam
Copy link
Copy Markdown
Member

ameukam commented Jun 7, 2021

@spiffxp made a revert : #2147.

ameukam added a commit to ameukam/k8s.io that referenced this pull request Jun 7, 2021
Fix exposure to privilege escalation introduced in kubernetes#2124.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
ameukam added a commit to ameukam/k8s.io that referenced this pull request Jun 10, 2021
Fix exposure to privilege escalation introduced in kubernetes#2124.

Signed-off-by: Arnaud Meukam <ameukam@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants