Skip to content

WINC-1147: Implement node-specific RBAC for WICD #3133

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
Merged
merged 1 commit into from
Oct 9, 2025
Merged

WINC-1147: Implement node-specific RBAC for WICD #3133

merged 1 commit into from
Oct 9, 2025

Conversation

mansikulkarni96
Copy link
Member

@mansikulkarni96 mansikulkarni96 commented Jul 22, 2025 •
edited
Loading

Update:

  • Certificates automatically renew at 80% of their lifetime (8 minutes for 10-minute certs)
  • Used certificate.Manager from k8s.io/client-go instead of custom certificate handling
  • Webhook implementation in follow-up PR to address cross node access

Certificate-Based Authentication:

  • Add certificate-based ClusterRole (system-wicd-nodes) with enhanced permissions
  • Add ClusterRoleBinding that grants permissions to system:wicd-nodes group
  • Certificate identity format: system:wicd-node:nodename

RBAC Permissions:

  • Bootstrap ServiceAccount: Minimal permissions for CSR creation and ConfigMap access
  • Certificate-based users: Enhanced permissions for node patching

Security Model:

  1. ServiceAccount (bootstrap): Limited permissions during initial setup
  2. CSR Controller: Validates certificate requests from nodes
  3. Node-specific access enforced by webhook
  4. Webhook: Future enhancement in a follow-up PR, preventing cross-node access and unauthorized annotations

Reference ovn-controller implementation: openshift/ovn-kubernetes@7dc4804

The flow is:

  1. WICD → Creates node-specific CSR
  2. WMCO CSR Controller → Auto-approves the CSR
  3. K8s Certificate Authority → Signs and issues the certificate
  4. WICD → Uses the certificate for all subsequent node operations like applying annotations
    This gives each WICD instance a unique, node-specific certificate identity that the admission webhook can validate against in the follow-up.

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jul 22, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2025
@red-hat-konflux Red Hat Konflux
Copy link
Contributor

Caution

There are some errors in your PipelineRun template.

PipelineRun Error
wmco-idms no kind "ImageDigestMirrorSet" is registered for version "config.openshift.io/v1" in scheme "k8s.io/client-go/kubernetes/scheme/register.go:83"

1 similar comment
@red-hat-konflux Red Hat Konflux
Copy link
Contributor

Caution

There are some errors in your PipelineRun template.

PipelineRun Error
wmco-idms no kind "ImageDigestMirrorSet" is registered for version "config.openshift.io/v1" in scheme "k8s.io/client-go/kubernetes/scheme/register.go:83"

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2025
@mansikulkarni96 mansikulkarni96 force-pushed the WINC-1147 branch 5 times, most recently from 3c9fbd8 to 79daf6b Compare July 24, 2025 13:53
@mansikulkarni96 mansikulkarni96 changed the title WIP WINC-1147 WINC-1147: Implement node-specific RBAC for WICD Jul 24, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 24, 2025 •
edited by openshift-ci bot
Loading

@mansikulkarni96: This pull request references WINC-1147 which is a valid jira issue.

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

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 24, 2025
@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96 mansikulkarni96 force-pushed the WINC-1147 branch 2 times, most recently from b63342b to 3880ade Compare July 25, 2025 14:29
@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96
Copy link
Member Author

/cc: @openshift/openshift-team-windows-containers

@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 28, 2025 •
edited by openshift-ci bot
Loading

@mansikulkarni96: This pull request references WINC-1147 which is a valid jira issue.

In response to this:

Implement node-specific RBAC for WICD
Implement RBAC controls to ensure each WICD instance can only
access the specific Windows node it is running on, following
the principle of least privilege.

  • Add pkg/rbac/node_rbac.go with creation and cleanup logic
  • Create individual ServiceAccounts, ClusterRoles,
    and ClusterRoleBindings for nodes

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.

@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96
Copy link
Member Author

/test aws-e2e-operator

@mansikulkarni96 mansikulkarni96 force-pushed the WINC-1147 branch 2 times, most recently from a4d4d9e to c626d76 Compare July 29, 2025 15:43
jrvaldes
jrvaldes reviewed Jul 29, 2025
View reviewed changes
config/wicd/windows-instance-config-daemon-cluster-role.yaml Outdated
labels:
app.kubernetes.io/name: "windows-machine-config-operator"
app.kubernetes.io/part-of: "wicd"
wicd.openshift.io/scope: "bootstrap" # Mark as bootstrap-only permissions
Copy link
Contributor

Choose a reason for hiding this comment

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

consider using full name for the annotation

daemon.windowsinstanceconfig.openshift.io/scope: "bootstrap"

controllers/configmap_controller.go Outdated
ObjectMeta: meta.ObjectMeta{
Name: wicdRBACResourceName,
Labels: map[string]string{
"app.kubernetes.io/name": "windows-machine-config-operator",
Copy link
Contributor

Choose a reason for hiding this comment

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

include openshift in the app name. i.e. openshift-windows-machine-config-operator

@jrvaldes
Copy link
Contributor

/test lint

mansikulkarni96
mansikulkarni96 commented Oct 6, 2025
View reviewed changes
bundle/manifests/windows-instance-config-daemon_rbac.authorization.k8s.io_v1_clusterrole.yaml
Comment on lines 24 to 25
- patch
- update
Copy link
Member Author

Choose a reason for hiding this comment

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

added back for upgrade scenario, since during upgrade previous version of WICD binary will not have the capability and permissions to remove annotations from node using the certificate method.

Copy link
Contributor

Choose a reason for hiding this comment

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

good catch

@jrvaldes
Copy link
Contributor

jrvaldes commented Oct 6, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2025
@mansikulkarni96
Copy link
Member Author

/hold cancel

@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 Oct 7, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 26f5ad3 and 2 for PR HEAD ea07d8e in total

@mansikulkarni96
Copy link
Member Author

/retest-required

azure cluster install issue

@deepsm007
Copy link

/retest-required

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 4168c85 and 1 for PR HEAD ea07d8e in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 3c630a6 and 0 for PR HEAD ea07d8e in total

@openshift-ci-robot
Copy link

/hold

Revision ea07d8e was retested 3 times: holding

@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 Oct 8, 2025
@mansikulkarni96
Copy link
Member Author

/test azure-e2e-upgrade

@mansikulkarni96
Copy link
Member Author

/hold cancel

@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 Oct 8, 2025
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 93b7a71 and 2 for PR HEAD ea07d8e in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 0be2fbd and 1 for PR HEAD ea07d8e in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD da88249 and 0 for PR HEAD ea07d8e in total

@openshift-ci-robot
Copy link

/hold

Revision ea07d8e was retested 3 times: holding

@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 Oct 9, 2025
@mansikulkarni96
Copy link
Member Author

/override ci/prow/nutanix-e2e-operator ci/prow/vsphere-e2e-operator ci/prow/vsphere-proxy-e2e-operator
test passed on previous runs

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 9, 2025

@mansikulkarni96: Overrode contexts on behalf of mansikulkarni96: ci/prow/nutanix-e2e-operator, ci/prow/vsphere-e2e-operator, ci/prow/vsphere-proxy-e2e-operator

In response to this:

/override ci/prow/nutanix-e2e-operator ci/prow/vsphere-e2e-operator ci/prow/vsphere-proxy-e2e-operator
test passed on previous runs

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-sigs/prow repository.

@jrvaldes
Copy link
Contributor

jrvaldes commented Oct 9, 2025

@mansikulkarni96 is it safe to remove the hold?

@jrvaldes
Copy link
Contributor

jrvaldes commented Oct 9, 2025

cc @sebsoto

@mansikulkarni96
Copy link
Member Author

/hold cancel

@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 Oct 9, 2025
@mansikulkarni96
Copy link
Member Author

/override ci/prow/nutanix-e2e-operator ci/prow/vsphere-proxy-e2e-operator ci/prow/aws-e2e-operator ci/prow/azure-e2e-operator ci/prow/vsphere-disconnected-e2e-operator ci/prow/azure-e2e-upgrade
test passed on previous runs

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 9, 2025

@mansikulkarni96: Overrode contexts on behalf of mansikulkarni96: ci/prow/aws-e2e-operator, ci/prow/azure-e2e-operator, ci/prow/azure-e2e-upgrade, ci/prow/nutanix-e2e-operator, ci/prow/vsphere-disconnected-e2e-operator, ci/prow/vsphere-proxy-e2e-operator

In response to this:

/override ci/prow/nutanix-e2e-operator ci/prow/vsphere-proxy-e2e-operator ci/prow/aws-e2e-operator ci/prow/azure-e2e-operator ci/prow/vsphere-disconnected-e2e-operator ci/prow/azure-e2e-upgrade
test passed on previous runs

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-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 7cbd244 into openshift:master Oct 9, 2025
21 checks passed
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 9, 2025

@mansikulkarni96: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

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

Reviewers

@sebsoto sebsoto Awaiting requested review from sebsoto

1 more reviewer

@jrvaldes jrvaldes jrvaldes left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jrvaldes jrvaldes

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mansikulkarni96 @openshift-ci-robot @jrvaldes @deepsm007 @sebsoto