Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions bindata/network/ovn-kubernetes/dpu-rbac.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
---
# Grant lease permissions to the DPU service account so the DPU can renew
# the health check lease in the openshift-ovn-kubernetes namespace.
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
{{ if .NETWORK_NODE_IDENTITY_ENABLE }}
name: openshift-ovn-kubernetes-node-dpu-service-identity-limited
{{ else }}
name: openshift-ovn-kubernetes-node-dpu-service-limited
{{ end }}
namespace: openshift-ovn-kubernetes
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: openshift-ovn-kubernetes-node-limited
subjects:
{{ if .NETWORK_NODE_IDENTITY_ENABLE }}
- kind: Group
name: system:ovn-nodes
apiGroup: rbac.authorization.k8s.io
{{ else }}
- kind: ServiceAccount
name: ovn-kubernetes-node-dpu-service
namespace: openshift-ovn-kubernetes
{{ end }}

{{ if .NETWORK_NODE_IDENTITY_ENABLE }}
---
# Allow the DPU service account to impersonate node users and groups
# so it can act on behalf of the DPU host node.
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: openshift-ovn-kubernetes-node-dpu-host-impersonator
rules:
- apiGroups: [""]
resources:
- users
verbs:
- impersonate
Comment on lines +37 to +41
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "dpu-rbac.yaml" -type f

Repository: openshift/cluster-network-operator

Length of output: 124


🏁 Script executed:

find . -path "*/pkg/network/ovn_kubernetes.go" -type f

Repository: openshift/cluster-network-operator

Length of output: 109


🏁 Script executed:

git ls-files | grep -E "(rbac|ovn|dpu)" | head -20

Repository: openshift/cluster-network-operator

Length of output: 1127


🏁 Script executed:

cat -n ./bindata/network/ovn-kubernetes/dpu-rbac.yaml

Repository: openshift/cluster-network-operator

Length of output: 2187


🏁 Script executed:

wc -l ./pkg/network/ovn_kubernetes.go

Repository: openshift/cluster-network-operator

Length of output: 114


🏁 Script executed:

sed -n '470,490p' ./pkg/network/ovn_kubernetes.go

Repository: openshift/cluster-network-operator

Length of output: 1076


🏁 Script executed:

rg -i "dpuhostmode|dpuhost" --context=5

Repository: openshift/cluster-network-operator

Length of output: 36646


🏁 Script executed:

rg -i "dpuhostnodeusers"

Repository: openshift/cluster-network-operator

Length of output: 60


🏁 Script executed:

rg -i "system:node:" --context=3

Repository: openshift/cluster-network-operator

Length of output: 60


🏁 Script executed:

sed -n '460,490p' ./pkg/network/ovn_kubernetes.go

Repository: openshift/cluster-network-operator

Length of output: 1553


🏁 Script executed:

sed -n '474,480p' ./pkg/network/ovn_kubernetes.go | cat -n

Repository: openshift/cluster-network-operator

Length of output: 489


🏁 Script executed:

rg "DpuHostModeNodes" -B 5 -A 5 ./pkg/network/ovn_kubernetes.go | head -50

Repository: openshift/cluster-network-operator

Length of output: 2391


🏁 Script executed:

rg "type OVNConfigBoostrapResult struct" -A 20 ./pkg/bootstrap/types.go

Repository: openshift/cluster-network-operator

Length of output: 883


Restrict users impersonation to specific system:node:* principals.

The rule at lines 37-41 permits ovn-kubernetes-node-dpu-service to impersonate any user in the cluster. This is inconsistent with the groups rule (lines 42-48), which correctly restricts impersonation to system:nodes and system:authenticated. Arbitrary user impersonation turns a DPU pod compromise into cluster-wide privilege escalation.

Add a DPUHostNodeUsers field to the render data in pkg/network/ovn_kubernetes.go (populate before line 477) by transforming each node name from DpuHostModeNodes into system:node:<nodeName> format, then update the RBAC template to include resourceNames for the users rule:

Suggested fix
 - apiGroups: [""]
   resources:
   - users
   verbs:
   - impersonate
+  resourceNames:
+{{ range .DPUHostNodeUsers }}
+  - {{ . }}
+{{ end }}

In pkg/network/ovn_kubernetes.go, populate data.Data["DPUHostNodeUsers"] with the transformed node names (e.g., system:node:node-1) before rendering dpu-rbac.yaml.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bindata/network/ovn-kubernetes/dpu-rbac.yaml` around lines 37 - 41, The RBAC
allows impersonation of any user; fix by populating
data.Data["DPUHostNodeUsers"] in pkg/network/ovn_kubernetes.go (before the
render call for dpu-rbac.yaml, e.g., around where DpuHostModeNodes is set/used)
by mapping each name in DpuHostModeNodes to the string "system:node:<nodeName>"
and assigning the resulting slice to that key, then update the dpu-rbac.yaml
template's users impersonation rule to include resourceNames: {{
.DPUHostNodeUsers }} so impersonation is restricted to those specific
system:node principals.

- apiGroups: [""]
resources:
- groups
verbs:
- impersonate
resourceNames:
- system:nodes
- system:authenticated
Comment on lines +42 to +49
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find the file mentioned in the review
find . -name "dpu-rbac.yaml" -o -name "*dpu*rbac*" | head -20

Repository: openshift/cluster-network-operator

Length of output: 124


🏁 Script executed:

# Check if the file exists and get its context
ls -la bindata/network/ovn-kubernetes/ 2>/dev/null | grep -i dpu

Repository: openshift/cluster-network-operator

Length of output: 141


🏁 Script executed:

# Search for NETWORK_NODE_IDENTITY_ENABLE to understand the conditional logic
rg "NETWORK_NODE_IDENTITY_ENABLE" -A 5 -B 5

Repository: openshift/cluster-network-operator

Length of output: 32328


🏁 Script executed:

# Search for system:ovn-nodes to understand its usage in RBAC
rg "system:ovn-nodes" -B 3 -A 3

Repository: openshift/cluster-network-operator

Length of output: 2218


🏁 Script executed:

# Search for the RoleBinding that grants lease access
rg "openshift-ovn-kubernetes-node-limited" -B 5 -A 5

Repository: openshift/cluster-network-operator

Length of output: 8347


🏁 Script executed:

cat -n bindata/network/ovn-kubernetes/dpu-rbac.yaml

Repository: openshift/cluster-network-operator

Length of output: 2187


🏁 Script executed:

# Search for the impersonation ClusterRole mentioned in the test
rg "openshift-ovn-kubernetes-node-dpu-host-impersonator" -A 20 -B 2

Repository: openshift/cluster-network-operator

Length of output: 4132


🏁 Script executed:

cat -n pkg/network/ovn_kubernetes_dpu_host_test.go | head -120

Repository: openshift/cluster-network-operator

Length of output: 4414


🏁 Script executed:

# Find the test case definition and structure
rg "TestOVNKubernetesDPU|expectImpersonation" -B 5 -A 10 | head -80

Repository: openshift/cluster-network-operator

Length of output: 3717


🏁 Script executed:

# Get more detail on the test to see if it validates the rules content
rg "TestOVNKubernetesDPURBAC" -A 100 | grep -A 50 "rule0"

Repository: openshift/cluster-network-operator

Length of output: 1335


system:ovn-nodes is missing from the groups impersonation resourceNames, blocking DPU lease renewal when node identity is enabled.

When NETWORK_NODE_IDENTITY_ENABLE is true, the DPU RoleBinding grants lease permissions to the system:ovn-nodes group. However, the impersonation ClusterRole restricts groups impersonation to system:nodes and system:authenticated, excluding system:ovn-nodes. This prevents impersonated requests from the DPU matching the RoleBinding, causing lease-renewal operations to fail authorization. Add system:ovn-nodes to the resourceNames list.

Suggested fix
 - apiGroups: [""]
   resources:
   - groups
   verbs:
   - impersonate
   resourceNames:
+  - system:ovn-nodes
   - system:nodes
   - system:authenticated
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- apiGroups: [""]
resources:
- groups
verbs:
- impersonate
resourceNames:
- system:nodes
- system:authenticated
- apiGroups: [""]
resources:
- groups
verbs:
- impersonate
resourceNames:
- system:ovn-nodes
- system:nodes
- system:authenticated
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bindata/network/ovn-kubernetes/dpu-rbac.yaml` around lines 42 - 49, The
impersonation ClusterRole's groups impersonation list is missing
system:ovn-nodes which prevents DPU lease renewals when
NETWORK_NODE_IDENTITY_ENABLE is true; update the ClusterRole that grants
"impersonate" for "groups" (the impersonation rule with resources: ["groups"]
and verbs: ["impersonate"]) to include "system:ovn-nodes" in its resourceNames
alongside "system:nodes" and "system:authenticated" so impersonated requests
from DPUs match the RoleBinding that grants lease permissions.

---
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
name: openshift-ovn-kubernetes-node-dpu-host-impersonator
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: ClusterRole
name: openshift-ovn-kubernetes-node-dpu-host-impersonator
subjects:
- kind: ServiceAccount
name: ovn-kubernetes-node-dpu-service
namespace: openshift-ovn-kubernetes
{{ end }}
8 changes: 8 additions & 0 deletions pkg/network/ovn_kubernetes.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,14 @@ func renderOVNKubernetes(conf *operv1.NetworkSpec, bootstrapResult *bootstrap.Bo
objs = append(objs, manifests...)
}

if len(bootstrapResult.OVN.OVNKubernetesConfig.DpuHostModeNodes) > 0 || len(bootstrapResult.OVN.OVNKubernetesConfig.DpuModeNodes) > 0 {
manifests, err = render.RenderTemplate(filepath.Join(manifestDir, "network/ovn-kubernetes/dpu-rbac.yaml"), &data)
if err != nil {
return nil, progressing, errors.Wrap(err, "failed to render DPU RBAC manifests")
}
objs = append(objs, manifests...)
}

if len(bootstrapResult.OVN.OVNKubernetesConfig.DpuModeNodes) > 0 {
// "OVN_NODE_MODE" not set when render.RenderDir() called above,
// so render just the error-cni.yaml with "OVN_NODE_MODE" set.
Expand Down
90 changes: 90 additions & 0 deletions pkg/network/ovn_kubernetes_dpu_host_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package network

import (
"strings"
"testing"

"github.com/ghodss/yaml"
Expand Down Expand Up @@ -355,3 +356,92 @@ func TestOVNKubernetesNodeSelectorOperator(t *testing.T) {
}
}
}

func TestOVNKubernetesDPURBAC(t *testing.T) {
rbacTemplatePath := "../../bindata/network/ovn-kubernetes/dpu-rbac.yaml"

testCases := []struct {
name string
identityEnable bool
expectRoleBinding string
expectSubjectKind string
expectSubjectName string
expectImpersonation bool
}{
{
name: "without identity",
identityEnable: false,
expectRoleBinding: "openshift-ovn-kubernetes-node-dpu-service-limited",
expectSubjectKind: "ServiceAccount",
expectSubjectName: "ovn-kubernetes-node-dpu-service",
expectImpersonation: false,
},
{
name: "with identity enabled",
identityEnable: true,
expectRoleBinding: "openshift-ovn-kubernetes-node-dpu-service-identity-limited",
expectSubjectKind: "Group",
expectSubjectName: "system:ovn-nodes",
expectImpersonation: true,
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
g := NewGomegaWithT(t)

data := render.MakeRenderData()
data.Data["NETWORK_NODE_IDENTITY_ENABLE"] = tc.identityEnable

objs, err := render.RenderTemplate(rbacTemplatePath, &data)
g.Expect(err).NotTo(HaveOccurred())

var foundRoleBinding bool
var foundImpersonationCR, foundImpersonationCRB bool

for _, obj := range objs {
kind := obj.GetKind()
name := obj.GetName()

if kind == "RoleBinding" && strings.HasPrefix(name, "openshift-ovn-kubernetes-node-dpu-service") {
foundRoleBinding = true
g.Expect(name).To(Equal(tc.expectRoleBinding))

subjects, found, err := uns.NestedSlice(obj.Object, "subjects")
g.Expect(err).NotTo(HaveOccurred())
g.Expect(found).To(BeTrue())
g.Expect(subjects).To(HaveLen(1))

subj := subjects[0].(map[string]interface{})
subjKind, _, _ := uns.NestedString(subj, "kind")
subjName, _, _ := uns.NestedString(subj, "name")
g.Expect(subjKind).To(Equal(tc.expectSubjectKind))
g.Expect(subjName).To(Equal(tc.expectSubjectName))

roleRefName, _, _ := uns.NestedString(obj.Object, "roleRef", "name")
g.Expect(roleRefName).To(Equal("openshift-ovn-kubernetes-node-limited"))
}

if kind == "ClusterRole" && name == "openshift-ovn-kubernetes-node-dpu-host-impersonator" {
foundImpersonationCR = true
rules, found, err := uns.NestedSlice(obj.Object, "rules")
g.Expect(err).NotTo(HaveOccurred())
g.Expect(found).To(BeTrue())
g.Expect(len(rules)).To(BeNumerically(">=", 2))

rule0 := rules[0].(map[string]interface{})
verbs0, _, _ := uns.NestedStringSlice(rule0, "verbs")
g.Expect(verbs0).To(ContainElement("impersonate"))
}

if kind == "ClusterRoleBinding" && name == "openshift-ovn-kubernetes-node-dpu-host-impersonator" {
foundImpersonationCRB = true
}
}

g.Expect(foundRoleBinding).To(BeTrue(), "DPU service RoleBinding should be present")
g.Expect(foundImpersonationCR).To(Equal(tc.expectImpersonation))
g.Expect(foundImpersonationCRB).To(Equal(tc.expectImpersonation))
})
}
}