audit as of 2020-02-17 #1676
audit as of 2020-02-17 #1676
Conversation
I'm not sure this is necessary? She is already an org admin
add roles/secretmanager.viewer
noise picked up in this: - some boskos projects added kubetest ssh keys
these were used by gcb jobs in one other staging project, the thought was there might be more so house them in a 'centra' project; but now windows builds can happen with docker buildx, so these secrets aren't needed anymore
I did this manually to support the development of the ci-k8sio-audit job currently being developed by the team that runs the cncf-ci github account
this seems a lot like an org admin made a typo when trying to create the k8s-staging-e2e-test-images project, and have since manually removed the project
this was done as part of migrating vendors off of google.com-owned k8s-federated-conformance
this was done to support the use case of e2e jobs being able to build and push images to whatever project they happen to be running in, for a cluster to then pull down and consume
I am used to seeing kubernetes-staging-1234 as part of normal CI jobs that use kube-up.sh. But, I am surprised that - we didn't already have these in the last audit I did in jan 2021 - the -eu and -asia suffixes are showing up, this is new...
I am going to need to undo/resolve this, I meant to delete the service account related to vulndash and must have copy-pasted the wrong thing. This isn't world-breaking because IIRC we have silenced the auditor's alerts, but I'm glad we caught this
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
area/audit
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
wg/k8s-infra
approved
|
/retest |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
Took off WIP because I think this basically contains all the same changes as the linked PRs, plus more. Would rather see the outstanding audit changes land sooner than later, if it's going to take a while to get the CLA resolved for cncf-ci-bot |
|
I've escalated via https://jira.linuxfoundation.org/plugins/servlet/theme/portal/4/SUPPORT-4208 Apparently I initiated the CLA a year ago and it's stuck. The reset should allow it to be cleared, and I'll push for this on North American Monday. |
|
I only covered the I was iterating over projects in sets to limit the size of the PR for review. |
|
Guessing this needs a rebase now. May close in favor of auto generated pr |
k8s-ci-robot
added
the
needs-rebase
|
@spiffxp: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
| @@ -0,0 +1,4 @@ | |||
| Bucket Policy Only setting for gs://artifacts.k8s-staging-releng-test.appspot.com: | |||
There was a problem hiding this comment.
I see no trace of this in the codebase? Why does it exist?
| @@ -0,0 +1,4 @@ | |||
| Bucket Policy Only setting for gs://artifacts.k8s-staging-provider-openstack.appspot.com: | |||
There was a problem hiding this comment.
I see no trace of this in the codebase? Why does it exist?
| @@ -0,0 +1,4 @@ | |||
| Bucket Policy Only setting for gs://artifacts.k8s-staging-experimental.appspot.com: | |||
There was a problem hiding this comment.
I see no trace of this in the codebase? Why does it exist?
| @@ -0,0 +1,4 @@ | |||
| Bucket Policy Only setting for gs://k8s-conform-provider-openstack: | |||
There was a problem hiding this comment.
I see no trace of this in the codebase? Why does it exist?
There was a problem hiding this comment.
looks like we have a bunch of these. i only run scripts, i don't know enough to meddle in the UI :) I believe i was re-running some of the conform buckets
[dims@dims-a01 07:15] ~/go/src/k8s.io/k8s.io ⟩ rg -i "Bucket Policy Only" | wc -l
244
In this instance i think i was trying to re-run scripts again to see how to help with:
kubernetes/test-infra#20914
There was a problem hiding this comment.
There was a problem hiding this comment.
Ultimately I would like for us to have this enabled across the org, and enforced via an org policy
per-object ACLs are much trickier to audit and enforce
There was a problem hiding this comment.
Sorry, my point in tagging @dims was that I can't find any trace of these projects in git. Did someone forget to send a PR?
There was a problem hiding this comment.
i know we cleaned some stuff up in:
#1311 (comment)
Only reference to k8s-conform-provider-openstack i can find is:
theopenlab/openlab#691
may be @chrigl knows more?
There was a problem hiding this comment.
@thockin it is in the codebase
- these buckets (and the k8s-conform project) are created by https://github.com/kubernetes/k8s.io/blob/main/infra/gcp/ensure-conformance-storage.sh
- the PR adding this bucket was Added new groups for cloud-provider-openstack #1645
- dims ran the script afterwards, like he was supposed to
There was a problem hiding this comment.
I apologize. I got bitten by master/main - I had not resynced this copy in a while and was trying to sync master and not noticing that it failed.
Indeed, it is in the tree. Mea culpa, my apologies.
| @@ -1,6 +1,9 @@ | |||
| NAME TITLE | |||
| compute.googleapis.com Compute Engine API | |||
| containerregistry.googleapis.com Container Registry API | |||
There was a problem hiding this comment.
Is this scripted? Who will ultimately clean these up?
There was a problem hiding this comment.
how to cleanup will be followed up in #1675
Yes this is scripted: https://github.com/kubernetes/k8s.io/blob/main/infra/gcp/prow/ensure-e2e-projects.sh
This was added via #1536
| @@ -0,0 +1,3 @@ | |||
| Bucket Policy Only setting for gs://artifacts.k8s-infra-e2e-boskos-scale-01.appspot.com: | |||
There was a problem hiding this comment.
If we scripted this, the script should init these for every project, no?
There was a problem hiding this comment.
Probably a good idea
After some of the digging I did into GCR/GCB permissions in kubernetes/test-infra#20884 (comment) I'd like to bucket this under a "we need to revisit GCR/GCB permissions" issue
I wasn't around for the initial set of scripts that set us down the path of bucket pre-creation, but I gather we want to be more conservative with storage.buckets.(create|delete|update) permissions than the default roles provided by these services?
| @@ -0,0 +1,3 @@ | |||
| Bucket Policy Only setting for gs://kubernetes-staging-485128143e-asia: | |||
There was a problem hiding this comment.
This is why test SAs shouldn't be able to make new buckets, I guess? How do we root cause it?
There was a problem hiding this comment.
Covered this in #1664 (comment)
I'll open a followup issue for how to avoid these growing unbounded
| @@ -1 +1 @@ | |||
| prow-build us-central1 us-central1-c;us-central1-f;us-central1-b 67 RUNNING | |||
| prow-build us-central1 us-central1-c;us-central1-f;us-central1-b 73 RUNNING | |||
There was a problem hiding this comment.
Casn we script to filter the count out?
| @@ -42,7 +42,7 @@ | |||
| }, | |||
| { | |||
| "members": [ | |||
| "serviceAccount:k8s-infra-gcr-auditor@k8s-artifacts-prod.iam.gserviceaccount.com" | |||
| "deleted:serviceAccount:k8s-infra-gcr-auditor@k8s-artifacts-prod.iam.gserviceaccount.com?uid=111422293292441494221" | |||
|
My Point is that you made (at least) 4 projects
(k8s-staging-releng-test, k8s-staging-provider-openstack,
k8s-staging-experimental, k8s-conform-provider-openstack)
with no representation (that I can find) in git.
Either you created them by hand, or you edited the scripts and then never
sent PRs. Both are pretty undesirable. Now we have no idea why these
exist or who is actually using them, or if they are derelict and should be
deleted.
Can we PLEASE not do this?
Aaron - can we exclude these from the audit merge until we know what is
going on?
…On Tue, Mar 2, 2021 at 9:15 AM Davanum Srinivas ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
audit/projects/k8s-conform/buckets/k8s-conform-provider-openstack/bucketpolicyonly.txt
<#1676 (comment)>:
> @@ -0,0 +1,4 @@
+Bucket Policy Only setting for gs://k8s-conform-provider-openstack:
i know we cleaned some stuff up in:
#1311 (comment)
<#1311 (comment)>
Only reference to k8s-conform-provider-openstack i can find is:
theopenlab/openlab#691 <theopenlab/openlab#691>
may be @chrigl <https://github.com/chrigl> knows more?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1676 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABKWAVBRORZ5PPEGG23FTODTBUMKFANCNFSM4XZM25UA>
.
|
|
Tim,
Let's turn them all down please. my bad. Apologies.
k8s-staging-releng-test - #1592
k8s-staging-experimental - #1594
the other two are possibly from:
#459
#351
Just to confirm, I *do* NOT do anything from the UI as i don't know where
anything is.
…-- Dims
On Tue, Mar 2, 2021 at 1:10 PM Tim Hockin ***@***.***> wrote:
My Point is that you made (at least) 4 projects
(k8s-staging-releng-test, k8s-staging-provider-openstack,
k8s-staging-experimental, k8s-conform-provider-openstack)
with no representation (that I can find) in git.
Either you created them by hand, or you edited the scripts and then never
sent PRs. Both are pretty undesirable. Now we have no idea why these
exist or who is actually using them, or if they are derelict and should be
deleted.
Can we PLEASE not do this?
Aaron - can we exclude these from the audit merge until we know what is
going on?
On Tue, Mar 2, 2021 at 9:15 AM Davanum Srinivas ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In
>
audit/projects/k8s-conform/buckets/k8s-conform-provider-openstack/bucketpolicyonly.txt
> <#1676 (comment)>:
>
> > @@ -0,0 +1,4 @@
> +Bucket Policy Only setting for gs://k8s-conform-provider-openstack:
>
> i know we cleaned some stuff up in:
> #1311 (comment)
> <#1311 (comment)
>
>
> Only reference to k8s-conform-provider-openstack i can find is:
> theopenlab/openlab#691 <theopenlab/openlab#691
>
>
> may be @chrigl <https://github.com/chrigl> knows more?
>
> —
> You are receiving this because you were assigned.
> Reply to this email directly, view it on GitHub
> <#1676 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/ABKWAVBRORZ5PPEGG23FTODTBUMKFANCNFSM4XZM25UA
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1676 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAFWCCGF5OKU5WI4UCIBADTBUS2RANCNFSM4XZM25UA>
.
--
Davanum Srinivas :: https://twitter.com/dims
|
Slow down @thockin. ALL of these are in git. It's just that our bash and/or commit message hygiene is terrible and not easy to map to this audit output (hence why I opened #1657) Pro-tip: if the project has
Now. If you want to get into whether and how we should be more judicious in deciding which PRs get approved, I'm super open to having that discussion. But @dims did nothing wrong here, and I'd like to put a stop to the "can we really trust our WG leads" line of thinking. I regret keeping this PR open, most of what's in it has merged via the PR's in #1676 (comment). I reviewed them and found them mostly unsurprising (though I do have some followup issues to create, I see @thockin already addressed "stop dumping cluster node count") I am hopeful that more frequent, smaller audit PR's will make these things less of a surprise. I am also hopeful that at some point, we can decide to either trust the bash we have, or replace it, so that we start running things in postsubmit with appropriately constrained service accounts. |
|
I will close this PR when I either:
Because right now it's the only thing showing org-level changes in a reviewable format. |
|
/close |
|
@spiffxp: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Split into commits with more context/commits from a human (me)
I care less about seeing this PR merge vs. merging automated PRs from our audit job
Mostly pushing this up as a note to self (and others) about some other things uncovered by audit that should probably be turned into issues to resolve