Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rbac: Audit * verbs of kubevirt-tekton-tasks #684

Merged
merged 1 commit into from
Oct 10, 2023
Merged

rbac: Audit * verbs of kubevirt-tekton-tasks #684

merged 1 commit into from
Oct 10, 2023

Conversation

jcanocan
Copy link
Contributor

@jcanocan jcanocan commented Sep 7, 2023

What this PR does / why we need it:
It drops * verbs of tekton tasks. For this purpose, the process followed is:

  • Drop all tekton tasks permissions using * verbs.
  • Run unit tests.
  • Add required permissions.
  • Run functional tests.
  • Add required permissions.

The auditing process has been split in the next action items:

  • Deploying the operator in a CRC cluster and running the unit and function test.
  • Deploying the operator in a 3 masters and 3 workers nodes and running the unit and functional tests.

This process ensures that only strictly required permissions are added.

jira-ticket: CNV-24031

Which issue(s) this PR fixes:

Fixes # https://bugzilla.redhat.com/show_bug.cgi?id=2223775

Special notes for your reviewer:

Release note:

Replace `*`  tekton tasks verbs by individual verbs

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Sep 7, 2023
@kubevirt-bot kubevirt-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M labels Sep 7, 2023
@jcanocan
Copy link
Contributor Author

jcanocan commented Sep 7, 2023

/hold
This PR need to be merged first: kubevirt/kubevirt-tekton-tasks#259

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023
akrejcir
akrejcir reviewed Sep 7, 2023
View reviewed changes
internal/operands/tekton-tasks/tekton-tasks.go Outdated
// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines/finalizers,verbs=*
// +kubebuilder:rbac:groups=*,resources=persistentvolumeclaims,verbs=*
// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines/finalizers,verbs=get
// +kubebuilder:rbac:groups=*,resources=persistentvolumeclaims,verbs=get;update;delete
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you change the groups=* to groups=core everywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. There are another files where groups=* or groups="" are used. I think this should be tackled in a follow-up PR.

@jcanocan jcanocan force-pushed the audit-asterisk-verbs branch from d0ed113 to 2fad24f Compare September 7, 2023 14:17
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2023
@akrejcir
Copy link
Collaborator

akrejcir commented Sep 7, 2023

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2023
@akrejcir akrejcir modified the milestones: v0.18.4, v0.19.0 Sep 18, 2023
codingben
codingben reviewed Oct 2, 2023
View reviewed changes
internal/operands/tekton-tasks/reconcile.go Outdated
// +kubebuilder:rbac:groups=kubevirt.io,resources=virtualmachines/finalizers,verbs=*
// +kubebuilder:rbac:groups=*,resources=persistentvolumeclaims,verbs=*
// +kubebuilder:rbac:groups=*,resources=pods,verbs=create
// +kubebuilder:rbac:groups=*,resources=secrets,verbs=*
Copy link
Member

Choose a reason for hiding this comment

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

Hi, I already did the same change and changed groups to core in #641, but your PR also changes verbs which is good. Thanks :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I did it following your advice. So I'm the one should thank you 😊 In a follow-up, I plan to review all groups and switch them to core.

@jcanocan jcanocan force-pushed the audit-asterisk-verbs branch from 2fad24f to d9bbc92 Compare October 4, 2023 14:12
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2023
@jcanocan jcanocan force-pushed the audit-asterisk-verbs branch from d9bbc92 to bbbafe9 Compare October 4, 2023 14:12
@jcanocan
Copy link
Contributor Author

jcanocan commented Oct 4, 2023

/retest-required

@jcanocan
Copy link
Contributor Author

jcanocan commented Oct 4, 2023

/hold cancel

@kubevirt-bot kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 4, 2023
@ksimon1
Copy link
Member

ksimon1 commented Oct 9, 2023

/retest

@jcanocan
Copy link
Contributor Author

jcanocan commented Oct 9, 2023

/retest

1 similar comment
@jcanocan
Copy link
Contributor Author

/retest

@jcanocan jcanocan force-pushed the audit-asterisk-verbs branch from bbbafe9 to a5705e4 Compare October 10, 2023 09:51
0xFelix
0xFelix approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2023
@jcanocan
rbac: Audit * verbs from kubevirt-tekton-tasks
4e73df0 
It drops `*` verbs of tekton tasks. For this purpose, the process
followed is:

* Drop all tekton tasks permissions using `*` verbs.
* Run unit tests.
* Add required permissions.
* Run functional tests.
* Add required permissions.

This process ensures that only strictly required permissions are added.
Fix: https://bugzilla.redhat.com/show_bug.cgi?id=2223775

Signed-off-by: Javier Cano Cano <jcanocan@redhat.com>
@jcanocan jcanocan force-pushed the audit-asterisk-verbs branch from a5705e4 to 4e73df0 Compare October 10, 2023 10:26
@kubevirt-bot kubevirt-bot added size/S and removed lgtm Indicates that a PR is ready to be merged. size/M labels Oct 10, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

0xFelix
0xFelix approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

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

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 10, 2023
@jcanocan
Copy link
Contributor Author

/retest-required

@kubevirt-bot kubevirt-bot merged commit 345d81d into kubevirt:main Oct 10, 2023
@jcanocan
Copy link
Contributor Author

/cherry-pick release-v0.18

@kubevirt-bot
Copy link
Contributor

@jcanocan: #684 failed to apply on top of branch "release-v0.18":

Applying: rbac: Audit `*` verbs from kubevirt-tekton-tasks
Using index info to reconstruct a base tree...
M	config/rbac/role.yaml
M	data/olm-catalog/ssp-operator.clusterserviceversion.yaml
A	internal/operands/tekton-tasks/reconcile.go
Falling back to patching base and 3-way merge...
Auto-merging internal/operands/tekton-tasks/tekton-tasks.go
CONFLICT (content): Merge conflict in internal/operands/tekton-tasks/tekton-tasks.go
Auto-merging data/olm-catalog/ssp-operator.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in data/olm-catalog/ssp-operator.clusterserviceversion.yaml
Auto-merging config/rbac/role.yaml
CONFLICT (content): Merge conflict in config/rbac/role.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 rbac: Audit `*` verbs from kubevirt-tekton-tasks
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.18

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.

@jcanocan jcanocan mentioned this pull request Jan 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codingben codingben codingben left review comments

@akrejcir akrejcir akrejcir left review comments

@0xFelix 0xFelix 0xFelix approved these changes

@ksimon1 ksimon1 Awaiting requested review from ksimon1

Assignees

@0xFelix 0xFelix

@akrejcir akrejcir

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jcanocan @akrejcir @kubevirt-bot @ksimon1 @0xFelix @codingben