Skip to content

Comments

CNF-16428: gitops: Reduce ArgoCD ClusterRole with minimal privilege#111

Open
leo8a wants to merge 3 commits intoopenshift-kni:mainfrom
leo8a:gitops-minimal-clusterrole
Open

CNF-16428: gitops: Reduce ArgoCD ClusterRole with minimal privilege#111
leo8a wants to merge 3 commits intoopenshift-kni:mainfrom
leo8a:gitops-minimal-clusterrole

Conversation

@leo8a
Copy link
Contributor

@leo8a leo8a commented Feb 19, 2025

The current cluster-admin ClusterRole assigned to ArgoCD in Hub clusters grants excessive permissions that are not necessary for its intended functionality. By adopting a minimal privilege ClusterRole, we align with the Principle of Least Privilege, reducing the attack surface and limiting access to only the required resources. This minimizes the potential for accidental misuse, privilege escalation, or unauthorized access in the event of a compromise.

/cc @imiller0 @yuvalk @sabbir-47

@openshift-ci-robot
Copy link
Collaborator

@leo8a: This pull request references CNF-16428 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

Details

In response to this:

The current cluster-admin ClusterRole assigned to ArgoCD in Hub clusters grants excessive permissions that are not necessary for its intended functionality. By adopting a minimal privilege ClusterRole, we align with the Principle of Least Privilege, reducing the attack surface and limiting access to only the required resources. This minimizes the potential for accidental misuse, privilege escalation, or unauthorized access in the event of a compromise.

/cc @imiller0 @yuvalk @sabbir-47

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 openshift-eng/jira-lifecycle-plugin repository.

@leo8a
Copy link
Contributor Author

leo8a commented Feb 25, 2025

/hold

doing some more testing

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2025
@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch from a761dcf to 7b5dffe Compare February 25, 2025 16:52
@leo8a
Copy link
Contributor Author

leo8a commented Feb 25, 2025

/unhold

Ready for review

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2025
@imiller0
Copy link
Collaborator

These get copied over from the telco-ran reference. We should update them there at the same time so that a future sync between the two doesn't overwrite these.
cc: @jgato

labels:
rbac.authorization.k8s.io/aggregate-to-ocm-cluster-manager-admin: "true"
rules:
- apiGroups:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also grant permission to Secrets and ConfigMaps at least?
Also, I'm wondering why list, get, watch were not included for the ClusterInstance. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

@leo8a , tagging just for attention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey Irina, thanks for your ping

Shouldn't we also grant permission to Secrets and ConfigMaps at least?

IIRC those resources are accessed using the openshift-gitops-openshift-gitops-argocd-application-controller ClusterRole (installed by default as part of the ArgoCD operator)

Also, I'm wondering why list, get, watch were not included for the ClusterInstance. 🤔

I'll add those verbs also to the list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/hold

In any case, I'm holding this PR for now, I want to run a couple of new tests, given that it has been a while since I validated this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Coming back to your questions:

Shouldn't we also grant permission to Secrets and ConfigMaps at least?
Also, I'm wondering why list, get, watch were not included for the ClusterInstance. 🤔

The ClusterRole/openshift-gitops-openshift-gitops-argocd-application-controller (installed by default with Argo CD) already grants full ([*]) access to configmaps:

configmaps  []  []  [*]

It also includes a wildcard rule:

*.*  []  []  [get list watch]

This provides read-only access (get, list, watch) to any resource in any API group—including custom resources like ClusterInstance. So technically, explicit permissions for ClusterInstance aren't required in this case. However, I'm happy to add them explicitly if that helps make the access rules clearer or easier to audit.

On the other hand, access to secrets is not included in the Argo CD ClusterRole above, but it is granted via the ClusterRole/open-cluster-management:cluster-manager-admin (installed by default with ACM):

-> oc describe ClusterRole/open-cluster-management:cluster-manager-admin | grep secrets
  secrets                                                                        []                 []              [create get list watch update delete deletecollection patch]

Copy link
Contributor

Choose a reason for hiding this comment

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

@irinamihai please review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @leo8a. I think a comment would also be helpful, to clear things up a bit since the permissions are spread out between multiple ClusterRoles.

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, I've added comments explaining how the permissions are spread out between multiple ClusterRoles... hopefully helpful for folks in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great, @leo8a. Given that this is a reference configuration, having as much information as possible is extremely valuable. Thank you!

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2025
@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch 2 times, most recently from ccd540c to 5b63da2 Compare July 8, 2025 12:54
@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch 2 times, most recently from 14b3e02 to 2b853cb Compare July 10, 2025 08:03
@leo8a
Copy link
Contributor Author

leo8a commented Jul 10, 2025

/unhold

PTAL

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2025
@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch from 2b853cb to 992997c Compare July 11, 2025 08:37
@irinamihai
Copy link
Contributor

Just 2 minor comments.
LGTM otherwise.

@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch from 992997c to 937854b Compare July 11, 2025 14:48
@irinamihai
Copy link
Contributor

/hold
Let's wait for a review from the gitops folks before merging this.
Thank you.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 14, 2025
@gnunn1
Copy link

gnunn1 commented Jul 14, 2025

@irinamihai asked me to have a look at this. Argo CD will typically require view all permissions on the cluster but I do not see it being changed here. Any plans to reduce those permissions and couple that change with either resourceExclusion or the auto respect RBAC feature in Argo CD?

@leo8a
Copy link
Contributor Author

leo8a commented Jul 15, 2025

Hey @gnunn1, thanks for taking a look.

Argo CD will typically require view all permissions on the cluster but I do not see it being changed here. Any plans to reduce those permissions and couple that change with either resourceExclusion or the auto respect RBAC feature in Argo CD?

Not in the scope of this PR. We are just targeting to reduce the permissions granted during the Initial Phase of the GitOps ZTP workflow for Telco (see cluster and gitops ClusterRoleBindings currently assigned).

That said, I agree that tightening Argo CD’s access (possibly in conjunction with resourceExclusions or auto-reconcile RBAC features) would be a worthwhile improvement. However, it would require a broader effort—including reducing the permissions granted to other components like ACM, which currently uses the ClusterRole/open-cluster-management:cluster-manager-admin (which is even more permissive than Argo CD’s defaults), or any other dependency component of this workflow.

@leo8a
Copy link
Contributor Author

leo8a commented Jul 15, 2025

/rebasing due to conflicts merged to main

@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch from 937854b to cead88e Compare July 15, 2025 10:16
@irinamihai
Copy link
Contributor

/unhold
/lgtm

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 15, 2025
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2025
leo8a added 3 commits October 21, 2025 12:06
The current cluster-admin ClusterRole assigned to ArgoCD in Hub clusters grants excessive permissions that are not necessary for its intended functionality. By adopting a minimal privilege ClusterRole, we align with the Principle of Least Privilege, reducing the attack surface and limiting access to only the required resources. This minimizes the potential for accidental misuse, privilege escalation, or unauthorized access in the event of a compromise.

Signed-off-by: Leonardo Ochoa-Aday <lochoa@redhat.com>
Since this dir is the canonical source for files in telco-hub ztp-installation, I'm copying the files also here.

Signed-off-by: Leonardo Ochoa-Aday <lochoa@redhat.com>
…ct filename

- Change "ZTP Role Bindings:" to "ZTP Roles and Bindings:" in SYNC-WAVES.md
- Update file reference from gitops-cluster-rolebinding.yaml to gitops-cluster-clusterrole.yaml
- Update compare_ignore to match the corrected filename

This ensures documentation accurately reflects the actual ClusterRole resource
rather than a ClusterRoleBinding.

Signed-off-by: Leonardo Ochoa-Aday <lochoa@redhat.com>
@leo8a leo8a force-pushed the gitops-minimal-clusterrole branch from cead88e to f1acbae Compare October 21, 2025 10:31
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2025
@openshift-ci
Copy link

openshift-ci bot commented Oct 21, 2025

New changes are detected. LGTM label has been removed.

@openshift-ci
Copy link

openshift-ci bot commented Oct 21, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: leo8a
Once this PR has been reviewed and has the lgtm label, please ask for approval from irinamihai. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@leo8a
Copy link
Contributor Author

leo8a commented Oct 21, 2025

hey there, so many changes in both (telco-hub and telco-ran) since this was last reviewed, had to rebase to pick up the new changes and adjust the proposed ones.

@irinamihai @sabbir-47 @imiller0 @lack PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants