empower KSA used in the promoter's prow jobs to use Workload Identity #655

listx · 2020-03-10T22:33:58Z

This gives the "k8s-prow-builds.svc.id.goog[test-pods/k8s-artifacts-prod]"
service account powers to act as the
"k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com" for
purposes of Workload Identity. This way, the Prow jobs for the promoter (which live in the k8s-prow-builds GKE project, and run in the test-pods namespace with the k8s-artifacts-prod KSA)
won't have to have service account keys injected into them.

/cc @thockin @fejta

More context: kubernetes-sigs/promo-tools#180 (comment)

This gives the "k8s-prow-builds.svc.id.googtest-pods/k8s-artifacts-prod" Prow account powers to act as the "k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com" for purposes of Workload Identity. This way, the Prow jobs for the promoter won't have to have service account keys injected into them.

fejta

LGTM

typo
some wording nits:

There are service accounts for a kubernetes cluster and GCP service accounts.
- There is no such thing as a prow service account (prow just creates pods)
- Any pod using the kubernetes service account will authenticate as the specified GCP account (not just prow's pods)
The binding just authenticates the user, doesn't control authorization
- AKA the gcloud command authorizes a (cluster, namespace, name) kubernetes service account identity to authenticate as the specified GCP service account to the rest of GCP
- Whether or not the GCP SA has access to GCR depends on the normal IAM rights for that GCP SA.

fejta · 2020-03-10T23:22:56Z

infra/gcp/ensure-prod-storage.sh

@@ -220,4 +220,9 @@ empower_group_to_admin_artifact_auditor \
 empower_artifact_auditor "${PROD_PROJECT}"
 empower_artifact_auditor_invoker "${PROD_PROJECT}"

+# Special case: empower Prow account to have access to GCR for Workload Identity.
+empower_prowacct_for_workload_identity \


IMO this should be more like:

empower_pods_to_identify_as_gcp_service_account

bind_k8s_service_account_to_act_as_gcp_service_account

fejta · 2020-03-10T23:27:38Z

infra/gcp/lib_gcr.sh

+        return 1
+    fi
+
+    prow_acct="$1"


I'd consider separating out these variables into their parts so its more clear, I'd recommend this:

gke_project=$1 namespace=$2 name=$3 gcp_acct=$4 gcp_project=$5 gcloud iam service-accounts add-iam-policy-binding \ --role roles/iam.workloadIdentityUser \ --member "serviceAccount:${gke_project}.svc.id.goog[${namespace}/${name}]" \ "$gcp_acct" --project "$gcp_project"

I don;t think we need to go quite this far.

I would position this function as empower_ksa_to_svcacct(project, k8s_svcacct, gcp_svcacct)

fejta · 2020-03-10T23:28:14Z

infra/gcp/ensure-prod-storage.sh

@@ -220,4 +220,9 @@ empower_group_to_admin_artifact_auditor \
 empower_artifact_auditor "${PROD_PROJECT}"
 empower_artifact_auditor_invoker "${PROD_PROJECT}"

+# Special case: empower Prow account to have access to GCR for Workload Identity.
+empower_prowacct_for_workload_identity \
+    "k8s-prow-builds.svc.id.googtest-pods/k8s-artifacts-prod" \


k8s-prow-builds.svc.id.goog[test-pods/k8s-artifacts-prod] (you need the [])

thockin · 2020-03-11T00:25:38Z

infra/gcp/lib_gcr.sh

@@ -149,3 +149,22 @@ function empower_svcacct_to_admin_gcr () {

    empower_svcacct_to_admin_gcs_bucket "${group}" "${bucket}"
 }
+


First note - this does not belong in this file. This file just for very low-level GCR-centric primitives. Put this back in lib.sh until that can be broken up more.

thockin · 2020-03-11T00:30:55Z

infra/gcp/lib_gcr.sh

+    prow_acct="$1"
+    gcp_acct="$2"
+
+    gcloud iam service-accounts add-iam-policy-binding \


you need --project, don't you?

thockin · 2020-03-11T00:34:52Z

infra/gcp/lib_gcr.sh

+        return 1
+    fi
+
+    prow_acct="$1"


I don;t think we need to go quite this far.

I would position this function as empower_ksa_to_svcacct(project, k8s_svcacct, gcp_svcacct)

listx · 2020-03-11T02:26:53Z

@fejta Thanks for the wording clarifications --- hopefully I captured the intent more accurately. But please take another look!

dims · 2020-03-11T10:12:46Z

/assign @thockin @justaugustus

thockin · 2020-03-11T15:31:55Z

@fejta This says that "test-pods/k8s-artifacts-prod" is the namespace/sa right? That implies that other pods run is the same namespace ("test-pods"), is that right? If so, can you compare this to running in a dedicated namespace that doesn't risk cross-pod leaks of the service account?

thockin · 2020-03-11T15:38:59Z

infra/gcp/lib.sh

+
+    gcloud iam service-accounts add-iam-policy-binding \
+        --role roles/iam.workloadIdentityUser \
+        --member "serviceAccount:${gke_project}.svc.id.goog[${k8s_namespace}/${gcp_project}]" \


Rather than construct the IDNS this way, I'd rather we pass it in fully formed. This ensures that, should that format ever change or extend, we don't need to change this function or duplicate it or anything.

Also I think the assumption that there's one SA per project is dubious - why couldn't we have 2 GSAs in k8s-artifacts-prod that were each used by different prow jobs? This conflates the name of the KSA in the GKE cluster with the name of the project the GSA exists in. It's fine to name them the same, but don't assume they are ALWAYS named the same.

IOW, change args 2 and 3 into a single string, with the documented example "${gke_project}.svc.id.goog[${k8s_namespace}/${k8s_svcacct}]"

An existing example where the service account name in the GKE cluster (k8s-kops-test) does not match the GCP project name (kubernetes-jenkins-pull).

An existing example where we are using mulitple GCP service accounts (resultstore, pusher, deployer) in the same project (k8s-prow).

Also I think the assumption that there's one SA per project is dubious - why couldn't we have 2 GSAs in k8s-artifacts-prod that were each used by different prow jobs? This conflates the name of the KSA in the GKE cluster with the name of the project the GSA exists in. It's fine to name them the same, but don't assume they are ALWAYS named the same.

As I was writing the code, I felt that this assumption was on shaky ground. Thanks for the guidance!

fejta · 2020-03-11T17:28:19Z

infra/gcp/lib.sh

+        --role roles/iam.workloadIdentityUser \
+        --member "serviceAccount:${gke_project}.svc.id.goog[${k8s_namespace}/${gcp_project}]" \
+        "${gcp_svcacct}" \
+        --project="${project}"


gcp_project

fejta · 2020-03-11T17:32:57Z

infra/gcp/lib.sh

+
+    gcloud iam service-accounts add-iam-policy-binding \
+        --role roles/iam.workloadIdentityUser \
+        --member "serviceAccount:${gke_project}.svc.id.goog[${k8s_namespace}/${gcp_project}]" \


An existing example where the service account name in the GKE cluster (k8s-kops-test) does not match the GCP project name (kubernetes-jenkins-pull).

An existing example where we are using mulitple GCP service accounts (resultstore, pusher, deployer) in the same project (k8s-prow).

fejta · 2020-03-11T18:57:32Z

This says that "test-pods/k8s-artifacts-prod" is the namespace/sa right? That implies that other pods run is the same namespace ("test-pods"), is that right?

Yes and yes (although we could use unit tests to mitigate against configuring jobs to use this SA).

We already do this mitigation for trusted jobs (which should have excluded these jobs from running there, but we updated the unit test to whitelist them).

If so, can you compare this to running in a dedicated namespace that doesn't risk cross-pod leaks of the service account?

Can we use the new #wg-k8s-infra scripting to schedule these pods in a community owned cluster somewhere? It is easy to configure prow to schedule pods in any conformant k8s cluster.

Yes, my request has been from the start (and continues to be) that we run these jobs outside of prow's trusted context, ideally in their own GKE cluster.

The trusted context is for jobs which prow fully trusts, which should not include these jobs: the core prow oncall/maintainer team does not own/review/maintain the code behind these jobs.

These belong in a different trust domain, which means in a different context than the prow control plane, ideally their own cluster.

In particular, pass in the serviceAccount argument as a single string.

listx · 2020-03-11T19:37:33Z

Updated... PTAL, esp. how I expanded the comments.

thockin · 2020-03-11T21:01:43Z

@fejta yes and no. In theory yes. In practice the apiserver IP is firewalled off and so we'd have to think about how to actually do it.

fejta

/lgtm
/approve
/hold

LGTM from my end, /hold cancel when you're ready for it to merge.

fejta · 2020-03-11T21:41:07Z

In practice the apiserver IP is firewalled off

Interesting. Which API server? How are things deployed/upgraded/etc when kubectl cannot talk to it?

thockin · 2020-03-11T21:46:02Z

Cloud shell is how we manage it. trying to follow "most secure" practices.

…

On Wed, Mar 11, 2020 at 2:41 PM Erick Fejta ***@***.***> wrote: In practice the apiserver IP is firewalled off Interesting. Which API server? How are things deployed/upgraded/etc when kubectl cannot talk to it? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#655 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVBUF5BRMLEIBT34DDDRHAAQBANCNFSM4LFJJMRQ> .

thockin

Does this mean I can revoke the JSON key we previously issued this SA?

/lgtm
/approve

k8s-ci-robot · 2020-03-11T22:21:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, listx, thockin

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:

~~OWNERS~~ [thockin]

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

listx · 2020-03-11T22:24:21Z

I think we have to actuate this first, then un-revert kubernetes/test-infra#16463 and see if the ci-k8sio-cip job (or post-k8sio-cip job) still works. Once all the jobs have migrated to WI, then the JSON keys can be revoked.

I think we need to set aside a day to test all of the above; maybe next week? Busy today and most likely tomorrow with kubernetes-sigs/promo-tools#191

thockin · 2020-03-11T22:27:09Z

file a followup bug? I know we have other things to finish (auditor -> alerts :)

listx · 2020-03-11T22:48:06Z

file a followup bug? I know we have other things to finish (auditor -> alerts :)

kubernetes-sigs/promo-tools#180

thockin · 2020-03-11T23:24:11Z

applied

Ever since kubernetes/k8s.io#655, this job should already run with an authenticated GCP SA (before the job's code even runs). This means there is no need to use the keyfile to authenticate (again), as it is redundant. We still keep the `-use-service-account` flag for now because the promoter currently still tries to extract OAUTH tokens as the GCP account at the very beginning of its run. It's a nice sanity check. Once we confirm that the OAUTH tokens do still get populated in the business logic of the job, we don't need to keep this flag any more because the GCRs that the promoter deals with today are all public (the promoter was designed to deal with private repos), and so the very first "read GCR" API calls do not need the OAUTH tokens for just reading the repos. That takes care of the GCR read API calls. As for the GCR write calls, the gcrane.Copy() method that perform the GCR writes do their own authentication dance, but that is separate and automatic as long as the GCP account is authenticated before that function call. This is why the `-use-service-account` flag is marked for future deprecation in this commit.

This is a follow-up to kubernetes/k8s.io#655 and kubernetes#16883.

k8s-ci-robot requested review from fejta and thockin March 10, 2020 22:34

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. wg/k8s-infra labels Mar 10, 2020

fejta reviewed Mar 10, 2020

View reviewed changes

thockin reviewed Mar 11, 2020

View reviewed changes

address review comments

bfd6e4e

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 11, 2020

fix wording

c1b6a3c

k8s-ci-robot assigned justaugustus and thockin Mar 11, 2020

thockin reviewed Mar 11, 2020

View reviewed changes

fejta reviewed Mar 11, 2020

View reviewed changes

address review comments

4c048d8

In particular, pass in the serviceAccount argument as a single string.

listx changed the title ~~empower prow build acct to use Workload Identity~~ empower KSA used in the promoter's prow jobs to use Workload Identity Mar 11, 2020

fejta approved these changes Mar 11, 2020

View reviewed changes

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

k8s-ci-robot assigned fejta Mar 11, 2020

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

thockin reviewed Mar 11, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 11, 2020

thockin removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 11, 2020

k8s-ci-robot merged commit 9e3204a into kubernetes:master Mar 11, 2020

listx mentioned this pull request Mar 11, 2020

use workload identity for all prow integrations kubernetes-sigs/promo-tools#180

Closed

This was referenced Mar 20, 2020

ci-k8sio-cip: do not use secret kubernetes/test-infra#16883

Merged

Optimize backups #677

Merged

listx pushed a commit to listx/test-infra that referenced this pull request Mar 24, 2020

ci-k8sio-cip: use k8s service account

815fef2

This is a follow-up to kubernetes/k8s.io#655 and kubernetes#16883.

listx mentioned this pull request Mar 24, 2020

ci-k8sio-cip: use k8s service account kubernetes/test-infra#16917

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

empower KSA used in the promoter's prow jobs to use Workload Identity #655

empower KSA used in the promoter's prow jobs to use Workload Identity #655

listx commented Mar 10, 2020 •

edited

Loading

fejta left a comment

fejta Mar 10, 2020

fejta Mar 10, 2020

thockin Mar 11, 2020

fejta Mar 10, 2020

thockin Mar 11, 2020

thockin Mar 11, 2020

thockin Mar 11, 2020

listx commented Mar 11, 2020

dims commented Mar 11, 2020

thockin commented Mar 11, 2020

thockin Mar 11, 2020

fejta Mar 11, 2020

listx Mar 11, 2020

fejta Mar 11, 2020

fejta Mar 11, 2020

fejta commented Mar 11, 2020 •

edited

Loading

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

fejta left a comment

fejta commented Mar 11, 2020

thockin commented Mar 11, 2020 via email

thockin left a comment

k8s-ci-robot commented Mar 11, 2020

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

		@@ -149,3 +149,22 @@ function empower_svcacct_to_admin_gcr () {

		empower_svcacct_to_admin_gcs_bucket "${group}" "${bucket}"
		}

empower KSA used in the promoter's prow jobs to use Workload Identity #655

empower KSA used in the promoter's prow jobs to use Workload Identity #655

Conversation

listx commented Mar 10, 2020 • edited Loading

fejta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

listx commented Mar 11, 2020

dims commented Mar 11, 2020

thockin commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta commented Mar 11, 2020 • edited Loading

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

fejta left a comment

Choose a reason for hiding this comment

fejta commented Mar 11, 2020

thockin commented Mar 11, 2020 via email

thockin left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 11, 2020

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

listx commented Mar 11, 2020

thockin commented Mar 11, 2020

listx commented Mar 10, 2020 •

edited

Loading

fejta commented Mar 11, 2020 •

edited

Loading