results of running audit script as of 2021-01-13 #1534

spiffxp · 2021-01-15T02:19:49Z

It's been way too long since this was last run and the results checked in. Last commit to audit/ is dated 2020-05-06. So this is ~7mo of changes

I tried to group changes into logical commits, but I won't be able to exhaustively tie each commit back to the issues/PRs that spawned it.

This is WIP because I want to either resolve or open up followup issues for commits that have QQ or TODO in their messages.

I really, really, really want us to get this running and automatically PR'ed by prow (ref: #244). Current state of affairs is broken.

specifically: - one for each "special" pool of projects - ingress, gpu, scale - intended to match equivalent k8s-infra-e2e-boskos projects

changes reflected in here include: - add k8s-infra-prow-viewers@ - add k8s-infra-prow-oncall@ - quotas updated out of band - add ssh-keys (added by kubetest2)

specifically: - enable secretmanager - enable serviceusage - reflect quota updates done out of band - reflect automated cluster changes done out of band

specifically: - windows-remote-docker_ca-pem - windows-remote-docker_cert-pem - windows-remote-docker_key-pem

specifically: - create the service account - bind to trusted prow via workload identity - empower SA to deploy to prow build clusters

also drop permissions given to related kubernetes.io group

specifically: - k8s-staging-addon-manager - k8s-staging-bootkube - k8s-staging-boskos - k8s-staging-ci-images - k8s-staging-cloud-provider-gcp - k8s-staging-cluster-addons - k8s-staging-cri-tools - k8s-staging-etcdadm - k8s-staging-examples - k8s-staging-git-sync - k8s-staging-ingress-nginx - k8s-staging-ingressconformance - k8s-staging-gsm-tools - k8s-staging-kustomize - k8s-staging-mirror - k8s-staging-networking - k8s-staging-provider-aws - k8s-staging-scheduler-plugins - k8s-staging-sig-docs - k8s-staging-sig-storage - k8s-staging-sp-operator - k8s-staging-storage-migrate

specifically - enable containeranalysis - enable containerscanning - enable secretmanager - give k8s-infra-gcr-vuln-scanning SA containeranalysis viewer role

specifically: - add k8s-conform-s390x-k8s bucket, SA for write, key in secretmanager - add k8s-conform-inspur

specifically: - restrict access to gsuite-groups-manager_key to serviceaccount - allow k8s-infra-prow-build-trusted to run as SA via workload identity

cpanato · 2021-01-15T12:13:49Z

@spiffxp I will take a look at this over the weekend

spiffxp · 2021-01-15T22:08:05Z

/uncc @justaugustus
OOO
/cc @thockin
I'm not asking that you look at this in great detail, more as an FYI. If you want to, I would recommend reviewing the commit messages and digging in on those that sound weird to you.

cpanato · 2021-01-18T15:50:07Z

audit/projects/k8s-staging-ci-images/iam.json

+    },
+    {
+      "members": [
+        "user:[email protected]"


All LGTM I think, it took some time to go over all the files related, but I did not see anything that calls my attention

just one comment, in several iam.json files we only see one "roles/owner" it is not better to have at least one more for backup in case it is needed? or this is expected? (asking because might be discussed before to be like this)

Whoever runs the script that creates a project gets assigned as owner (we have a bug open about this #299). Org owners implicitly get owner on projects within the org, so we're available for fallback.

I believe the thinking goes: don't give owner access to projects, thereby requiring a gitops workflow for things like changing iam permissions. It's just unfortunate that the execution part of our gitops model is currently "humans run scripts" vs. bots.

thanks for the clarification Aaron

k8s-ci-robot · 2021-01-18T15:50:37Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [spiffxp]

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

spiffxp · 2021-01-20T16:54:51Z

/assign @thockin

spiffxp · 2021-02-11T07:22:43Z

This is now N weeks out of date. I'd like to re-run audit and add some more commits to this, or merge this as-is and do a followup PR.

@hh is making progress on an automated CI job (kubernetes/test-infra#20742) but I'd like to get us to a smaller more digestible delta before we accept PR's from that job. If we close this PR and wait for that job to work, we're going to land something larger than this PR, but with one commit. I don't want to review that.

spiffxp · 2021-02-16T14:49:11Z

I am re-running audit.sh and will add commits to this. I'm thinking maybe it's best to leave the questionable changes listed in QQ commits out of this, to merge the non-controversial stuff here, and save discussion of those for another PR.

thockin

I asked the same set of questions you did. I filed some issues on other weirdnesses that are not as urgent feeling.

Mostly LGTM

thockin · 2021-02-16T22:28:54Z

audit/org_kubernetes.io/iam.json

@@ -72,6 +72,8 @@
        "group:[email protected]",
        "user:[email protected]",
        "user:[email protected]",
+        "user:[email protected]",


We shouldn't need these (since we have group:[email protected]). But they are OK as a "safety".

Yup, "safety" is why they're explicitly defined here, is there someplace we should doc this?

thockin · 2021-02-16T22:31:04Z

audit/org_kubernetes.io/roles/prow.viewer.json

+  ],
+  "name": "organizations/758905017065/roles/prow.viewer",
+  "stage": "ALPHA",
+  "title": "Prow Viewer"


I find nothing in our repo setting this up. Aaron, your fingerprint is in the access log. We should set up a script or terraform to sync the org.

#1659

thockin · 2021-02-16T23:41:20Z

audit/projects/k8s-infra-e2e-boskos-scale-01/description.json

@@ -0,0 +1,11 @@
+{


I wrote a quick script to diff all 30 of these.

06-30 have higher quotas than 01-05.

Not enough to abort this audit, but worth noting. Issue filed

thockin · 2021-02-16T23:48:15Z

audit/projects/k8s-infra-e2e-boskos-gpu-01/description.json

@@ -0,0 +1,11 @@
+{


I ran a script to diff these and it was clean.

thockin · 2021-02-16T23:50:18Z

audit/projects/k8s-infra-e2e-boskos-041/description.json

@@ -0,0 +1,11 @@
+{


I ran a script to diff these again 001.

41 + are missing the IAM binding:

{ "members": [ "group:[email protected]" ], "role": "roles/viewer" }

thockin · 2021-02-17T00:49:24Z

audit/projects/k8s-staging-artifact-promoter/iam.json

@@ -22,6 +22,32 @@
    },
    {
      "members": [
+        "serviceAccount:[email protected]"


Almost certainly what happened was that you or I or someone with "enough" privs clicked on the "compute" tab while in this project (that's my default tab) and it "helpfully" enables the service. Sigh. We should just go and disable it in the script. so it will get cleaned up on every run.

opened #1675

thockin · 2021-02-17T00:49:55Z

audit/projects/k8s-staging-capi-docker/iam.json

@@ -20,6 +20,24 @@
      ],
      "role": "roles/cloudbuild.serviceAgent"
    },
+    {


See comment on compute. I suspect the same, but I have less evidence). We should nuke it.

added to #1675

thockin · 2021-02-17T00:50:21Z

audit/projects/k8s-staging-e2e-test-images/iam.json

@@ -20,10 +20,42 @@
      ],
      "role": "roles/cloudbuild.serviceAgent"
    },
+    {


agree - should be disabled.

given your explanation for "I clicked on console and this happens automatically" this is almost undoubtedly my fault, I navigate these services all the time on a number of projects

added to #1675

thockin · 2021-02-17T00:51:17Z

...netes-public/service-accounts/[email protected]/description.json

@@ -0,0 +1,8 @@
+{
+  "displayName": "App Engine default service account",


Maybe something to do with promoter? Can't think of a reason

I have been in app engine before (looking at gubernator), it's possible this is the same deal as everything else in #1675

I'm surprised clicking around on console auto-activates services. Is it because our accounts are over-privileged? I'd rather see a "nope, you can't use this because the API isn't enabled for this project"

thockin · 2021-02-17T00:51:48Z

audit/projects/kubernetes-public/iam.json

@@ -18,6 +18,12 @@
      ],
      "role": "roles/bigquery.jobUser"
    },
+    {


Maybe related to promoter? @listx may know.

thockin · 2021-02-18T00:06:19Z

Yes, it's because people like you can do anything. :(

…

On Wed, Feb 17, 2021 at 4:04 PM Aaron Crickenberger < ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In ***@***.***/description.json <#1534 (comment)>: > @@ -0,0 +1,8 @@ +{ + "displayName": "App Engine default service account", I have been in app engine before (looking at gubernator), it's possible this is the same deal as everything else in #1675 <#1675> I'm surprised clicking around on console auto-activates services. Is it because our accounts are over-privileged? I'd rather see a "nope, you can't use this because the API isn't enabled for this project" — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#1534 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABKWAVESVW7JIUKVPUAGPU3S7RKSHANCNFSM4WDMGRCA> .

spiffxp · 2021-02-18T00:49:55Z

Forget what I said in #1534 (comment)

I did re-run audit, but I'd rather save those for another PR. And since we've opened a bunch of followup issues for the QQ commits, I would rather merge them as-is, and we can go back and reference that these have been closed out in a later audit PR

So... looking for lgtm?

dims · 2021-02-18T01:27:49Z

/lgtm

( i checked all the previous comments ... no, i did not go through 38k lines! )

spiffxp added 14 commits January 14, 2021 18:23

Fix audit script secret list typo

e8c3d1a

Add TODOs for unhandled services

c640149

audit: update org admin membership

0c2ca8c

audit: add custom prow.viewer role to org

4083417

audit: add k8s-infra-e2e-boskos-scale-* projects

c102166

audit: add k8s-infra-e2e-boskos-gpu-* projects

9fbc0f0

audit: add k8s-infra-e2e-boskos-[041-120] projects

5caaf90

audit: add k8s-infra-e2e-* manual projects

8add7de

specifically: - one for each "special" pool of projects - ingress, gpu, scale - intended to match equivalent k8s-infra-e2e-boskos projects

audit: update existing k8s-infra-e2e-boskos-[001-040] projects

ab02be3

changes reflected in here include: - add k8s-infra-prow-viewers@ - add k8s-infra-prow-oncall@ - quotas updated out of band - add ssh-keys (added by kubetest2)

audit: enable services for prow build clusters

b9b614e

specifically: - enable secretmanager - enable serviceusage - reflect quota updates done out of band - reflect automated cluster changes done out of band

audit: add prow viewer role to manual k8s-infra-e2e projects

29426db

audit: add prow viewer/oncall groups to k8s-infra-prow-build-trusted

af431d1

audit: add k8s-infra-prow-build-trusted secrets

e3d188c

specifically: - windows-remote-docker_ca-pem - windows-remote-docker_cert-pem - windows-remote-docker_key-pem

audit: add prow-deployer service account

6660eae

specifically: - create the service account - bind to trusted prow via workload identity - empower SA to deploy to prow build clusters

k8s-ci-robot requested review from bartsmykla and dims January 15, 2021 02:20

k8s-ci-robot added wg/k8s-infra approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 15, 2021

audit: rm k8s-staging-release-test project

5b54a5c

also drop permissions given to related kubernetes.io group

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 15, 2021

spiffxp added 6 commits January 14, 2021 21:20

audit: rm k8s-sig-release-prototype project

7cd3c63

audit: k8s-staging-* service enables

1ff4c21

specifically - enable containeranalysis - enable containerscanning - enable secretmanager - give k8s-infra-gcr-vuln-scanning SA containeranalysis viewer role

audit: add k8s-conform buckets

d64f009

specifically: - add k8s-conform-s390x-k8s bucket, SA for write, key in secretmanager - add k8s-conform-inspur

audit: allow k8s-infra-prow-build-trusted to manage gsuite groups

924a495

specifically: - restrict access to gsuite-groups-manager_key to serviceaccount - allow k8s-infra-prow-build-trusted to run as SA via workload identity

audit: add k8s-gsuite/wg-k8s-infra-billing_pw secret

e84f81f

k8s-ci-robot requested a review from justaugustus January 15, 2021 02:24

k8s-ci-robot requested review from thockin and removed request for justaugustus January 15, 2021 22:08

cpanato approved these changes Jan 18, 2021

View reviewed changes

k8s-ci-robot assigned thockin Jan 20, 2021

Base automatically changed from master to main February 9, 2021 00:35

This was referenced Feb 16, 2021

Audit Update for kubernetes-public #1650

Closed

Audit Update for k8s-infra-boskos 64/160 #1652

Closed

Audit Update for k8s-infra-* (non-boskos) #1651

Closed

spiffxp mentioned this pull request Feb 16, 2021

Setup a job to automatically run and PR the results of audit/audit-gcp.sh #244

Closed

thockin reviewed Feb 17, 2021

View reviewed changes

This was referenced Feb 17, 2021

audit followup: remove k8s-prow iam bindings for jobs that no longer run in k8s-prow #1674

Closed

audit followup: k8s-staging-* projects should not have misc services enabled #1675

Closed

spiffxp changed the title ~~[wip] results of running audit script as of 2021-01-13~~ results of running audit script as of 2021-01-13 Feb 18, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2021

k8s-ci-robot assigned dims Feb 18, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2021

k8s-ci-robot merged commit ed90c4d into kubernetes:main Feb 18, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 18, 2021

spiffxp deleted the audit-2021-01-13 branch February 18, 2021 01:30

spiffxp mentioned this pull request Jul 29, 2021

Deprecate and migrate away from gs://kubernetes-release-dev and gcr.io/kubernetes-ci-images #2318

Closed

42 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

results of running audit script as of 2021-01-13 #1534

results of running audit script as of 2021-01-13 #1534

spiffxp commented Jan 15, 2021 •

edited

Loading

cpanato commented Jan 15, 2021

spiffxp commented Jan 15, 2021

cpanato Jan 18, 2021

spiffxp Jan 21, 2021

cpanato Jan 22, 2021

k8s-ci-robot commented Jan 18, 2021

spiffxp commented Jan 20, 2021

spiffxp commented Feb 11, 2021

spiffxp commented Feb 16, 2021

thockin left a comment

thockin Feb 16, 2021

spiffxp Feb 17, 2021

thockin Feb 16, 2021

thockin Feb 16, 2021

thockin Feb 16, 2021

thockin Feb 16, 2021

thockin Feb 16, 2021

thockin Feb 17, 2021

spiffxp Feb 18, 2021

spiffxp Feb 18, 2021

thockin Feb 17, 2021

spiffxp Feb 18, 2021

thockin Feb 17, 2021

spiffxp Feb 18, 2021

thockin Feb 17, 2021

spiffxp Feb 18, 2021

thockin Feb 17, 2021

thockin commented Feb 18, 2021 via email

spiffxp commented Feb 18, 2021

dims commented Feb 18, 2021

		@@ -0,0 +1,8 @@
		{
		"displayName": "App Engine default service account",

results of running audit script as of 2021-01-13 #1534

results of running audit script as of 2021-01-13 #1534

Conversation

spiffxp commented Jan 15, 2021 • edited Loading

cpanato commented Jan 15, 2021

spiffxp commented Jan 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 18, 2021

spiffxp commented Jan 20, 2021

spiffxp commented Feb 11, 2021

spiffxp commented Feb 16, 2021

thockin 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Feb 18, 2021 via email

spiffxp commented Feb 18, 2021

dims commented Feb 18, 2021

spiffxp commented Jan 15, 2021 •

edited

Loading