Skip to content

roles: add more pre-defined roles to audit.viewer and prow.viewer#2099

Merged
k8s-ci-robot merged 3 commits intokubernetes:mainfrom
spiffxp:audit-viewer-bigquery
May 25, 2021
Merged

roles: add more pre-defined roles to audit.viewer and prow.viewer#2099
k8s-ci-robot merged 3 commits intokubernetes:mainfrom
spiffxp:audit-viewer-bigquery

Conversation

@spiffxp
Copy link
Copy Markdown
Contributor

@spiffxp spiffxp commented May 25, 2021

This will hopefully address the audit job not seeing all bigquery assets: #2029 (comment)

For audit.viewer...

Include permissions from service-specific viewer roles based on a survey of services that are active across the organization:

  • roles/bigquery.metadataViewer
  • roles/bigquery.resourceViewer
  • roles/cloudkms.publicKeyViewer
  • roles/container.clusterViewer
  • roles/logging.viewer # see note below
  • roles/monitoring.viewer
  • roles/pubsub.viewer

NOTE: the logging.queries.* permissions that would normally come in from roles/logging.viewer have been excluded, to prevent auditors from modifying existing logging queries in projects

Add TODO's for two services we should audit, but may need to avoid if there are potentially sensitive values accidentally stored in configuration-related resources:

  • roles/cloudbuild.builds.viewer
  • roles/run.viewer

Rephrase TODO for serviceusage role. We should really stop applying it at the organization level.

For prow.viewer, add:

  • roles/browser
  • roles/pubsub.viewer
  • roles/secretmanager.viewer

spiffxp added 3 commits May 25, 2021 17:23
Include permissions from service-specific viewer roles based on a survey
of services that are active across the organization:

- roles/bigquery.metadataViewer
- roles/bigquery.resourceViewer
- roles/cloudkms.publicKeyViewer
- roles/container.clusterViewer
- roles/logging.viewer # see note below
- roles/monitoring.viewer
- roles/pubsub.viewer

NOTE: the logging.queries.* permissions that would normally come in from
roles/logging.viewer have been excluded, to prevent auditors from
modifying existing logging queries in projects

Add TODO's for two services we should audit, but may need to avoid if
there are potentially sensitive values accidentally stored in
configuration-related resources:

- roles/cloudbuild.builds.viewer
- roles/run.viewer

Rephrase TODO for serviceusage role. We should really stop applying it
at the organization level.
Include some of the same roles included for audit.viewer
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. wg/k8s-infra labels May 25, 2021
@k8s-ci-robot k8s-ci-robot requested review from nikhita and thockin May 25, 2021 21:25
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2021
@spiffxp
Copy link
Copy Markdown
Contributor Author

spiffxp commented May 25, 2021

/cc @hh @bernokl
/cc @dims @ameukam

@k8s-ci-robot k8s-ci-robot requested review from ameukam, dims and hh May 25, 2021 21:30
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

@spiffxp: GitHub didn't allow me to request PR reviews from the following users: bernokl.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @hh @bernokl
/cc @dims @ameukam

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.

@spiffxp
Copy link
Copy Markdown
Contributor Author

spiffxp commented May 25, 2021

/hold
Going to deploy this and then verify whether this addresses the bigquery access issue for the audit job

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021
@spiffxp
Copy link
Copy Markdown
Contributor Author

spiffxp commented May 25, 2021

Ran ./infra/gcp/ensure-organization.sh and role updates are deployed

Using #2100 to test

@spiffxp
Copy link
Copy Markdown
Contributor Author

spiffxp commented May 25, 2021

/hold cancel
I think this should work, but may also need a bugfix in #2100 to merge to fully close out

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021
@dims
Copy link
Copy Markdown
Member

dims commented May 25, 2021

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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 merged commit a56cb56 into kubernetes:main May 25, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 25, 2021
Copy link
Copy Markdown
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

late /lgtm

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants