From 96fc32eaeb4b3597ea889ed4faacad8beb309b96 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Tue, 18 Aug 2026 13:03:09 +0100 Subject: [PATCH 1/9] Add monitortest to verify possible Cluster Admin escalation paths --- pkg/defaultmonitortests/types.go | 2 + .../rbacadminescalationtests/monitortest.go | 323 ++++++++++++++++++ .../monitortest_test.go | 168 +++++++++ 3 files changed, 493 insertions(+) create mode 100644 pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go create mode 100644 pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go diff --git a/pkg/defaultmonitortests/types.go b/pkg/defaultmonitortests/types.go index db766d33c702..f625cda0b55f 100644 --- a/pkg/defaultmonitortests/types.go +++ b/pkg/defaultmonitortests/types.go @@ -6,6 +6,7 @@ import ( "github.com/openshift/origin/pkg/monitortestframework" "github.com/openshift/origin/pkg/monitortests/authentication/legacyauthenticationmonitortests" nodefaultserviceaccountoperatortests "github.com/openshift/origin/pkg/monitortests/authentication/nodefaultserviceaccountoperatortests" + "github.com/openshift/origin/pkg/monitortests/authentication/rbacadminescalationtests" "github.com/openshift/origin/pkg/monitortests/authentication/requiredsccmonitortests" admupgradestatus "github.com/openshift/origin/pkg/monitortests/cli/adm_upgrade/status" azuremetrics "github.com/openshift/origin/pkg/monitortests/cloud/azure/metrics" @@ -126,6 +127,7 @@ func newDefaultMonitorTests(info monitortestframework.MonitorTestInitializationI // Authentication monitorTestRegistry.AddMonitorTestOrDie("legacy-authentication-invariants", "apiserver-auth", legacyauthenticationmonitortests.NewLegacyTests()) monitorTestRegistry.AddMonitorTestOrDie("no-default-service-account-operator-checker", "oauth-apiserver", nodefaultserviceaccountoperatortests.NewAnalyzer()) + monitorTestRegistry.AddMonitorTestOrDie("rbac-cluster-admin-escalation-checker", "kube-apiserver", rbacadminescalationtests.NewAnalyzer()) // Cluster Version Operator monitorTestRegistry.AddMonitorTestOrDie("operator-state-analyzer", "Cluster Version Operator", operatorstateanalyzer.NewAnalyzer()) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go new file mode 100644 index 000000000000..5399a0571279 --- /dev/null +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -0,0 +1,323 @@ +package rbacadminescalationtests + +import ( + "context" + "fmt" + "sort" + "strings" + "time" + + "github.com/openshift/origin/pkg/monitor/monitorapi" + "github.com/openshift/origin/pkg/monitortestframework" + "github.com/openshift/origin/pkg/test/ginkgo/junitapi" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" + rbacvalidation "k8s.io/component-helpers/auth/rbac/validation" + rbacv1helpers "k8s.io/kubernetes/pkg/apis/rbac/v1" + "k8s.io/kubernetes/pkg/registry/rbac/validation" +) + +type escalationChecker struct { + kubeClient kubernetes.Interface +} + +func NewAnalyzer() monitortestframework.MonitorTest { + return &escalationChecker{} +} + +// trackedExceptions are (binding, check) escalation pairs that are known issues we intend to fix. +// Each is paired with a tracking Jira. These flake (fail + pass) rather than hard-failing, so they +// stay visible in CI and can be burned down. +// +// No new (binding, check) pairs should be added to this list without the sign off of an OpenShift +// Architect. +var trackedExceptions = []func(binding rbacv1.ClusterRoleBinding, checkID string) (string, bool){} + +// permanentExceptions are (binding, check) escalation pairs that are legitimate and expected forever. +// These are silently accepted: they produce no JUnit result at all. Reserve this list for grants that +// are correct by design and will never be "fixed". +// +// No new (binding, check) pairs should be added to this list without the sign off of an OpenShift +// Architect. +var permanentExceptions = []func(binding rbacv1.ClusterRoleBinding, checkID string) (string, bool){ + // The cluster-admin binding grants cluster-admin to system:masters by design. + exceptionForBindingCheck("cluster-admin", clusterAdminCheckID, "by-design: binds cluster-admin to system:masters"), +} + +// escalationCheck is a single way in which a ClusterRoleBinding can hand a subject a path to +// cluster-admin. Each check becomes its own JUnit test (and its own independently flakeable +// exception) so that allowing one binding to hold one dangerous permission does not silently waive +// every other escalation check for that binding. +type escalationCheck struct { + // id is the stable slug used to key exceptions, e.g. "escalate-rbac". + id string + // desc is the human phrase embedded in the test name, e.g. "escalate or bind RBAC roles". + desc string + // rules are the escalation-enabling permissions. The check fires when the bound role grants ANY + // atom of these rules. + rules []rbacv1.PolicyRule +} + +// clusterAdminCheckID is the id of the full cluster-admin check. It must be listed first in +// escalationChecks so that its short-circuit (skip the subsumed, more specific checks) works. +const clusterAdminCheckID = "cluster-admin" + +// escalationChecks is the curated, tunable set of escalation paths we audit. Broadening this set +// (e.g. cluster-wide secrets read, pod/exec, node proxy) will expand findings and the allowlist, so +// it is deliberately conservative. +var escalationChecks = []escalationCheck{ + { + // All verbs on all resources in all API groups IS cluster-admin: the subject can read every + // secret, mutate any object, and grant itself anything. This is the direct, definitional case + // rather than a path to escalation. + id: clusterAdminCheckID, + desc: "cluster-admin equivalent access (all verbs on all resources)", + rules: []rbacv1.PolicyRule{ + {Verbs: []string{rbacv1.VerbAll}, APIGroups: []string{rbacv1.APIGroupAll}, Resources: []string{rbacv1.ResourceAll}}, + }, + }, + { + // The `escalate` and `bind` verbs are the two ways to bypass Kubernetes' built-in RBAC + // escalation prevention. Plain create/update/patch on clusterroles/clusterrolebindings does + // NOT allow escalation on its own: the rbac policybased storage strategy runs + // ConfirmNoEscalation and rejects writing/binding any permission the caller does not already + // hold, unless the caller has `escalate` (for role rules) or `bind` (for a role reference). + id: "escalate-rbac", + desc: "escalate or bind RBAC roles", + rules: []rbacv1.PolicyRule{ + rbacv1helpers.NewRule("escalate", "bind").Groups(rbacv1.GroupName).Resources("clusterroles", "roles").RuleOrDie(), + }, + }, + { + // Impersonation lets the subject send requests AS another user or group without needing that + // identity's credentials. Impersonating the `system:masters` group (or any cluster-admin-bound + // user/service account) yields cluster-admin immediately, so this bypasses RBAC entirely. + id: "impersonate", + desc: "impersonate users, groups, or service accounts", + rules: []rbacv1.PolicyRule{ + rbacv1helpers.NewRule("impersonate").Groups("").Resources("users", "groups", "serviceaccounts").RuleOrDie(), + }, + }, + { + // Webhook configurations intercept every write to the API server. Whoever can create or modify + // them can point a webhook at their own endpoint to read the contents of (and, for mutating + // webhooks, rewrite) any admitted object cluster-wide — including secrets and RBAC objects — + // giving them an out-of-band path to cluster-admin. + id: "admission-webhooks", + desc: "create or modify admission webhook configurations", + rules: []rbacv1.PolicyRule{ + rbacv1helpers.NewRule("create", "update", "patch").Groups("admissionregistration.k8s.io").Resources("mutatingwebhookconfigurations", "validatingwebhookconfigurations").RuleOrDie(), + }, + }, + { + // Control over certificate issuance is control over identity. A subject that can approve a CSR + // and sign it (via the kubernetes.io signers) can mint a client certificate for an arbitrary + // username and group — e.g. group `system:masters` — and authenticate as cluster-admin. + id: "csr-signing", + desc: "approve or sign certificate signing requests", + rules: []rbacv1.PolicyRule{ + rbacv1helpers.NewRule("approve").Groups("certificates.k8s.io").Resources("certificatesigningrequests").RuleOrDie(), + rbacv1helpers.NewRule("sign").Groups("certificates.k8s.io").Resources("signers").RuleOrDie(), + }, + }, + // Intentionally omitted for noise: `create` on pods/pods/exec (token theft via any mounted SA). + // Add here if the escalation surface should be widened. +} + +// exceptionForBindingCheck returns a predicate that matches a specific (binding, check) pair by +// ClusterRoleBinding name prefix and check id, associating it with a note (a tracking Jira for a +// tracked exception, or a rationale for a permanent one). +func exceptionForBindingCheck(namePrefix, checkID, note string) func(rbacv1.ClusterRoleBinding, string) (string, bool) { + return func(binding rbacv1.ClusterRoleBinding, id string) (string, bool) { + if id == checkID && strings.HasPrefix(binding.Name, namePrefix) { + return note, true + } + return "", false + } +} + +// matchException reports whether the given (binding, check) pair matches any predicate in the +// provided list, returning the associated note. +func matchException(list []func(rbacv1.ClusterRoleBinding, string) (string, bool), binding rbacv1.ClusterRoleBinding, checkID string) (string, bool) { + for _, exception := range list { + if note, ok := exception(binding, checkID); ok { + return note, true + } + } + return "", false +} + +// roleGrantsAny reports whether roleRules cover ANY atomic permission in servantRules, returning the +// matched atoms for reporting. Coverage is wildcard-aware (handled by rbacvalidation.Covers). +func roleGrantsAny(roleRules, servantRules []rbacv1.PolicyRule) (bool, []rbacv1.PolicyRule) { + matched := []rbacv1.PolicyRule{} + for _, servantRule := range servantRules { + for _, atom := range rbacvalidation.BreakdownRule(servantRule) { + if covered, _ := rbacvalidation.Covers(roleRules, []rbacv1.PolicyRule{atom}); covered { + matched = append(matched, atom) + } + } + } + return len(matched) > 0, matched +} + +// subjectString renders a binding's subjects for inclusion in a failure message. +func subjectString(subjects []rbacv1.Subject) string { + rendered := sets.NewString() + for _, s := range subjects { + if s.Namespace != "" { + rendered.Insert(fmt.Sprintf("%s/%s/%s", s.Kind, s.Namespace, s.Name)) + continue + } + rendered.Insert(fmt.Sprintf("%s/%s", s.Kind, s.Name)) + } + return strings.Join(rendered.List(), ", ") +} + +// rulesToString renders matched permissions in a compact, human-readable form. +func rulesToString(rules []rbacv1.PolicyRule) string { + compactRules := rules + if compact, err := validation.CompactRules(rules); err == nil { + compactRules = compact + } + descriptions := sets.NewString() + for _, rule := range compactRules { + descriptions.Insert(rbacv1helpers.CompactString(rule)) + } + return strings.Join(descriptions.List(), "\n") +} + +// evaluateBinding runs every escalation check against a single ClusterRoleBinding and returns the +// resulting JUnit cases. Only checks that fire produce cases, and the outcome depends on the +// exception class of the (binding, check) pair: +// - permanent exception: emit nothing (legitimate, by-design grant). +// - tracked exception: emit a fail plus a passing duplicate (a flake) so it stays visible. +// - no exception: emit a hard fail. +func evaluateBinding(binding rbacv1.ClusterRoleBinding, rolesByName map[string][]rbacv1.PolicyRule) []*junitapi.JUnitTestCase { + // A ClusterRoleBinding's RoleRef always references a ClusterRole. A dangling reference grants + // nothing, so there is nothing to evaluate. + roleRules, ok := rolesByName[binding.RoleRef.Name] + if !ok { + return nil + } + + junits := []*junitapi.JUnitTestCase{} + for _, check := range escalationChecks { + hit, matched := roleGrantsAny(roleRules, check.rules) + if !hit { + continue + } + + // A full cluster-admin grant covers every more specific escalation check, so evaluating those + // as well would be redundant noise. Whatever we decide for the cluster-admin check, we must + // stop after it — including when it is silently excepted, otherwise the subsumed checks would + // fire and hard-fail. Track that decision here and honor it at the end of the iteration. + isClusterAdmin := check.id == clusterAdminCheckID + + // Permanent exceptions are silently accepted: emit nothing at all. + if _, ok := matchException(permanentExceptions, binding, check.id); ok { + if isClusterAdmin { + break + } + continue + } + + testName := fmt.Sprintf("[sig-auth] clusterrolebinding %q must not grant permission to %s", binding.Name, check.desc) + msg := fmt.Sprintf("clusterrolebinding %q (clusterrole %q) grants permission to %s to subjects [%s] via:\n%s", + binding.Name, binding.RoleRef.Name, check.desc, subjectString(binding.Subjects), rulesToString(matched)) + + jira, tracked := matchException(trackedExceptions, binding, check.id) + if tracked { + msg += fmt.Sprintf("\n(tracked exception: %s)", jira) + } + + junits = append(junits, &junitapi.JUnitTestCase{ + Name: testName, + SystemOut: msg, + FailureOutput: &junitapi.FailureOutput{Output: msg}, + }) + + // Tracked exceptions flake rather than hard-fail: emit a passing duplicate with the same name. + if tracked { + junits = append(junits, &junitapi.JUnitTestCase{Name: testName}) + } + + if isClusterAdmin { + break + } + } + return junits +} + +// CollectData implements monitortestframework.MonitorTest. +func (e *escalationChecker) CollectData(ctx context.Context, storageDir string, beginning time.Time, end time.Time) (monitorapi.Intervals, []*junitapi.JUnitTestCase, error) { + if e.kubeClient == nil { + return nil, nil, nil + } + + clusterRoles, err := e.kubeClient.RbacV1().ClusterRoles().List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, nil, err + } + rolesByName := make(map[string][]rbacv1.PolicyRule, len(clusterRoles.Items)) + for _, role := range clusterRoles.Items { + rolesByName[role.Name] = role.Rules + } + + bindings, err := e.kubeClient.RbacV1().ClusterRoleBindings().List(ctx, metav1.ListOptions{}) + if err != nil { + return nil, nil, err + } + // Sort for deterministic ordering of emitted test cases. + sort.Slice(bindings.Items, func(i, j int) bool { + return bindings.Items[i].Name < bindings.Items[j].Name + }) + + junits := []*junitapi.JUnitTestCase{} + for _, binding := range bindings.Items { + // Dynamic (generateName) binding names would break static test naming; skip them. + if binding.GenerateName != "" { + continue + } + junits = append(junits, evaluateBinding(binding, rolesByName)...) + } + return nil, junits, nil +} + +// StartCollection implements monitortestframework.MonitorTest. +func (e *escalationChecker) StartCollection(ctx context.Context, adminRESTConfig *rest.Config, recorder monitorapi.RecorderWriter) error { + var err error + e.kubeClient, err = kubernetes.NewForConfig(adminRESTConfig) + if err != nil { + return err + } + return nil +} + +// PrepareCollection implements monitortestframework.MonitorTest. +func (e *escalationChecker) PrepareCollection(ctx context.Context, adminRESTConfig *rest.Config, recorder monitorapi.RecorderWriter) error { + return nil +} + +// ConstructComputedIntervals implements monitortestframework.MonitorTest. +func (e *escalationChecker) ConstructComputedIntervals(ctx context.Context, startingIntervals monitorapi.Intervals, recordedResources monitorapi.ResourcesMap, beginning time.Time, end time.Time) (monitorapi.Intervals, error) { + return nil, nil +} + +// EvaluateTestsFromConstructedIntervals implements monitortestframework.MonitorTest. +func (e *escalationChecker) EvaluateTestsFromConstructedIntervals(ctx context.Context, finalIntervals monitorapi.Intervals) ([]*junitapi.JUnitTestCase, error) { + return nil, nil +} + +// WriteContentToStorage implements monitortestframework.MonitorTest. +func (e *escalationChecker) WriteContentToStorage(ctx context.Context, storageDir string, timeSuffix string, finalIntervals monitorapi.Intervals, finalResourceState monitorapi.ResourcesMap) error { + return nil +} + +// Cleanup implements monitortestframework.MonitorTest. +func (e *escalationChecker) Cleanup(ctx context.Context) error { + return nil +} diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go new file mode 100644 index 000000000000..5fe71261ec9b --- /dev/null +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -0,0 +1,168 @@ +package rbacadminescalationtests + +import ( + "strings" + "testing" + + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func binding(name, roleName string, subjects ...rbacv1.Subject) rbacv1.ClusterRoleBinding { + return rbacv1.ClusterRoleBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + RoleRef: rbacv1.RoleRef{Kind: "ClusterRole", Name: roleName}, + Subjects: subjects, + } +} + +func rule(verbs, groups, resources []string) rbacv1.PolicyRule { + return rbacv1.PolicyRule{Verbs: verbs, APIGroups: groups, Resources: resources} +} + +// checkID extracts the escalation check that a test case name corresponds to by matching against the +// known checks' descriptions. +func checkIDForName(name string) string { + for _, c := range escalationChecks { + if strings.Contains(name, c.desc) { + return c.id + } + } + return "" +} + +func TestEvaluateBinding(t *testing.T) { + // Seed a tracked exception for the duration of this test so we can exercise the flake path. + originalTracked := trackedExceptions + trackedExceptions = append(append([]func(rbacv1.ClusterRoleBinding, string) (string, bool){}, originalTracked...), + exceptionForBindingCheck("tracked-esc", "escalate-rbac", "https://issues.redhat.com/browse/EXAMPLE-1")) + defer func() { trackedExceptions = originalTracked }() + + saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"} + + clusterAdminRule := rule([]string{"*"}, []string{"*"}, []string{"*"}) + escalateRule := rbacv1.PolicyRule{Verbs: []string{"escalate"}, APIGroups: []string{rbacv1.GroupName}, Resources: []string{"clusterroles"}} + impersonateRule := rbacv1.PolicyRule{Verbs: []string{"impersonate"}, APIGroups: []string{""}, Resources: []string{"users"}} + readOnlyRule := rule([]string{"get", "list", "watch"}, []string{""}, []string{"pods"}) + + tests := []struct { + name string + binding rbacv1.ClusterRoleBinding + rolesByName map[string][]rbacv1.PolicyRule + wantCheckIDs []string // expected failing check ids, in order + wantFlakeChecks map[string]bool + }{ + { + name: "direct cluster-admin", + binding: binding("some-admin", "admin-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"admin-role": {clusterAdminRule}}, + wantCheckIDs: []string{"cluster-admin"}, + }, + { + name: "escalate rbac", + binding: binding("escalator", "escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, + wantCheckIDs: []string{"escalate-rbac"}, + }, + { + name: "impersonate", + binding: binding("imp", "imp-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"imp-role": {impersonateRule}}, + wantCheckIDs: []string{"impersonate"}, + }, + { + name: "two checks tripped", + binding: binding("multi", "multi-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"multi-role": {escalateRule, impersonateRule}}, + // escalate-rbac precedes impersonate in escalationChecks ordering. + wantCheckIDs: []string{"escalate-rbac", "impersonate"}, + }, + { + name: "benign role emits nothing", + binding: binding("benign", "read-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"read-role": {readOnlyRule}}, + wantCheckIDs: nil, + }, + { + name: "missing role reference emits nothing", + binding: binding("dangling", "does-not-exist", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{}, + wantCheckIDs: nil, + }, + { + // The cluster-admin binding is a permanent exception: no JUnit result at all, and the + // cluster-admin short-circuit still suppresses the subsumed checks. + name: "permanent exception emits nothing", + binding: binding("cluster-admin", "cluster-admin", rbacv1.Subject{Kind: "Group", Name: "system:masters"}), + rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, + wantCheckIDs: nil, + }, + { + // A tracked exception flakes: one fail + one pass for that check. + name: "tracked exception flakes", + binding: binding("tracked-esc", "escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, + wantCheckIDs: []string{"escalate-rbac"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + junits := evaluateBinding(tc.binding, tc.rolesByName) + + // Collect the failing cases (non-nil FailureOutput) and passing cases (nil) per name. + failsByName := map[string]int{} + passesByName := map[string]int{} + failOrder := []string{} + for _, j := range junits { + if j.FailureOutput != nil { + failsByName[j.Name]++ + failOrder = append(failOrder, checkIDForName(j.Name)) + } else { + passesByName[j.Name]++ + } + } + + if len(failOrder) != len(tc.wantCheckIDs) { + t.Fatalf("expected %d failing checks %v, got %d: %v", len(tc.wantCheckIDs), tc.wantCheckIDs, len(failOrder), failOrder) + } + for i, want := range tc.wantCheckIDs { + if failOrder[i] != want { + t.Errorf("failing check %d: expected %q, got %q", i, want, failOrder[i]) + } + } + + // A flaked check has exactly one fail and one pass for the same name; a hard-fail has no + // matching pass. + for _, c := range escalationChecks { + name := "[sig-auth] clusterrolebinding \"" + tc.binding.Name + "\" must not grant permission to " + c.desc + wantFlake := tc.wantFlakeChecks[c.id] + if wantFlake { + if failsByName[name] != 1 || passesByName[name] != 1 { + t.Errorf("check %q expected flake (1 fail + 1 pass), got fail=%d pass=%d", c.id, failsByName[name], passesByName[name]) + } + continue + } + if passesByName[name] != 0 { + t.Errorf("check %q expected no passing (green) case, got %d", c.id, passesByName[name]) + } + } + }) + } +} + +func TestRoleGrantsAny(t *testing.T) { + // A role with resource wildcard should be detected as covering escalate on clusterroles. + wildcard := []rbacv1.PolicyRule{rule([]string{"*"}, []string{rbacv1.GroupName}, []string{"*"})} + servant := []rbacv1.PolicyRule{{Verbs: []string{"escalate"}, APIGroups: []string{rbacv1.GroupName}, Resources: []string{"clusterroles"}}} + if hit, matched := roleGrantsAny(wildcard, servant); !hit || len(matched) == 0 { + t.Errorf("expected wildcard role to grant escalate, got hit=%v matched=%v", hit, matched) + } + + // An unrelated role should not match. + unrelated := []rbacv1.PolicyRule{rule([]string{"get"}, []string{""}, []string{"configmaps"})} + if hit, _ := roleGrantsAny(unrelated, servant); hit { + t.Errorf("expected unrelated role not to grant escalate") + } +} From 45ed22dbf7862aaeb683a65b6c9e47f866a8a5c3 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 19 Aug 2026 10:43:20 +0100 Subject: [PATCH 2/9] Ensure binding exceptions are matching role and subjects, not just name --- .../rbacadminescalationtests/monitortest.go | 84 ++++++++++++------- .../monitortest_test.go | 36 ++++++-- 2 files changed, 84 insertions(+), 36 deletions(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 5399a0571279..d09887f7117f 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -28,23 +28,49 @@ func NewAnalyzer() monitortestframework.MonitorTest { return &escalationChecker{} } -// trackedExceptions are (binding, check) escalation pairs that are known issues we intend to fix. -// Each is paired with a tracking Jira. These flake (fail + pass) rather than hard-failing, so they -// stay visible in CI and can be burned down. +// bindingException describes an approved escalation grant. An exception applies only when the actual +// binding matches the approved grant exactly: same name, same referenced role, and the same set of +// subjects. Pinning roleRef and subjects means that repointing an allowlisted binding at a different +// role, or granting it to a new subject, revokes the exemption and re-triggers the finding for +// re-review — the exemption covers the grant we reviewed, not just the name. +type bindingException struct { + name string + checkID string + roleRef string + subjects []rbacv1.Subject + // note is a tracking Jira for a tracked exception, or a rationale for a permanent one. + note string +} + +// matches reports whether the given (binding, check) pair is exactly the approved grant. +func (e bindingException) matches(binding rbacv1.ClusterRoleBinding, checkID string) bool { + return e.checkID == checkID && + e.name == binding.Name && + e.roleRef == binding.RoleRef.Name && + subjectSet(e.subjects).Equal(subjectSet(binding.Subjects)) +} + +// trackedExceptions are approved escalation grants that are known issues we intend to fix. Each is +// paired with a tracking Jira. These flake (fail + pass) rather than hard-failing, so they stay +// visible in CI and can be burned down. // -// No new (binding, check) pairs should be added to this list without the sign off of an OpenShift -// Architect. -var trackedExceptions = []func(binding rbacv1.ClusterRoleBinding, checkID string) (string, bool){} +// No new entries should be added to this list without the sign off of an OpenShift Architect. +var trackedExceptions = []bindingException{} -// permanentExceptions are (binding, check) escalation pairs that are legitimate and expected forever. -// These are silently accepted: they produce no JUnit result at all. Reserve this list for grants that -// are correct by design and will never be "fixed". +// permanentExceptions are approved escalation grants that are legitimate and expected forever. These +// are silently accepted: they produce no JUnit result at all. Reserve this list for grants that are +// correct by design and will never be "fixed". // -// No new (binding, check) pairs should be added to this list without the sign off of an OpenShift -// Architect. -var permanentExceptions = []func(binding rbacv1.ClusterRoleBinding, checkID string) (string, bool){ - // The cluster-admin binding grants cluster-admin to system:masters by design. - exceptionForBindingCheck("cluster-admin", clusterAdminCheckID, "by-design: binds cluster-admin to system:masters"), +// No new entries should be added to this list without the sign off of an OpenShift Architect. +var permanentExceptions = []bindingException{ + { + // The cluster-admin binding grants cluster-admin to system:masters by design. + name: "cluster-admin", + checkID: clusterAdminCheckID, + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}}, + note: "by-design: binds cluster-admin to system:masters", + }, } // escalationCheck is a single way in which a ClusterRoleBinding can hand a subject a path to @@ -127,24 +153,12 @@ var escalationChecks = []escalationCheck{ // Add here if the escalation surface should be widened. } -// exceptionForBindingCheck returns a predicate that matches a specific (binding, check) pair by -// ClusterRoleBinding name prefix and check id, associating it with a note (a tracking Jira for a -// tracked exception, or a rationale for a permanent one). -func exceptionForBindingCheck(namePrefix, checkID, note string) func(rbacv1.ClusterRoleBinding, string) (string, bool) { - return func(binding rbacv1.ClusterRoleBinding, id string) (string, bool) { - if id == checkID && strings.HasPrefix(binding.Name, namePrefix) { - return note, true - } - return "", false - } -} - -// matchException reports whether the given (binding, check) pair matches any predicate in the +// matchException reports whether the given (binding, check) pair matches any approved grant in the // provided list, returning the associated note. -func matchException(list []func(rbacv1.ClusterRoleBinding, string) (string, bool), binding rbacv1.ClusterRoleBinding, checkID string) (string, bool) { +func matchException(list []bindingException, binding rbacv1.ClusterRoleBinding, checkID string) (string, bool) { for _, exception := range list { - if note, ok := exception(binding, checkID); ok { - return note, true + if exception.matches(binding, checkID) { + return exception.note, true } } return "", false @@ -164,6 +178,16 @@ func roleGrantsAny(roleRules, servantRules []rbacv1.PolicyRule) (bool, []rbacv1. return len(matched) > 0, matched } +// subjectSet returns a canonical, order-insensitive set of a binding's subjects for exact matching. +// APIGroup is included so that, e.g., a User and a ServiceAccount of the same name are distinct. +func subjectSet(subjects []rbacv1.Subject) sets.Set[string] { + out := sets.New[string]() + for _, s := range subjects { + out.Insert(fmt.Sprintf("%s/%s/%s/%s", s.APIGroup, s.Kind, s.Namespace, s.Name)) + } + return out +} + // subjectString renders a binding's subjects for inclusion in a failure message. func subjectString(subjects []rbacv1.Subject) string { rendered := sets.NewString() diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go index 5fe71261ec9b..ebe48e04d2e0 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -32,14 +32,20 @@ func checkIDForName(name string) string { } func TestEvaluateBinding(t *testing.T) { - // Seed a tracked exception for the duration of this test so we can exercise the flake path. + saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"} + + // Seed a tracked exception for the duration of this test so we can exercise the flake path. It + // pins the exact grant: name, check, roleRef, and subjects. originalTracked := trackedExceptions - trackedExceptions = append(append([]func(rbacv1.ClusterRoleBinding, string) (string, bool){}, originalTracked...), - exceptionForBindingCheck("tracked-esc", "escalate-rbac", "https://issues.redhat.com/browse/EXAMPLE-1")) + trackedExceptions = append(append([]bindingException{}, originalTracked...), bindingException{ + name: "tracked-esc", + checkID: "escalate-rbac", + roleRef: "escalate-role", + subjects: []rbacv1.Subject{saSubject}, + note: "https://issues.redhat.com/browse/EXAMPLE-1", + }) defer func() { trackedExceptions = originalTracked }() - saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"} - clusterAdminRule := rule([]string{"*"}, []string{"*"}, []string{"*"}) escalateRule := rbacv1.PolicyRule{Verbs: []string{"escalate"}, APIGroups: []string{rbacv1.GroupName}, Resources: []string{"clusterroles"}} impersonateRule := rbacv1.PolicyRule{Verbs: []string{"impersonate"}, APIGroups: []string{""}, Resources: []string{"users"}} @@ -93,10 +99,20 @@ func TestEvaluateBinding(t *testing.T) { // The cluster-admin binding is a permanent exception: no JUnit result at all, and the // cluster-admin short-circuit still suppresses the subsumed checks. name: "permanent exception emits nothing", - binding: binding("cluster-admin", "cluster-admin", rbacv1.Subject{Kind: "Group", Name: "system:masters"}), + binding: binding("cluster-admin", "cluster-admin", rbacv1.Subject{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}), rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, wantCheckIDs: nil, }, + { + // A new subject on the permanently-excepted binding no longer matches the approved grant, + // so the exemption is revoked and the finding hard-fails. + name: "permanent exception revoked by new subject", + binding: binding("cluster-admin", "cluster-admin", + rbacv1.Subject{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}, + rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sneaky"}), + rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, + wantCheckIDs: []string{"cluster-admin"}, + }, { // A tracked exception flakes: one fail + one pass for that check. name: "tracked exception flakes", @@ -105,6 +121,14 @@ func TestEvaluateBinding(t *testing.T) { wantCheckIDs: []string{"escalate-rbac"}, wantFlakeChecks: map[string]bool{"escalate-rbac": true}, }, + { + // Same tracked binding+check but repointed at a different escalating role: the roleRef no + // longer matches the approved grant, so it hard-fails instead of flaking. + name: "tracked exception revoked by roleref change", + binding: binding("tracked-esc", "other-escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"other-escalate-role": {escalateRule}}, + wantCheckIDs: []string{"escalate-rbac"}, + }, } for _, tc := range tests { From e7423968c36f33325cb6834b80da39b6c2cc5926 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 19 Aug 2026 17:48:30 +0100 Subject: [PATCH 3/9] Add initial set of exceptions to track --- .../rbacadminescalationtests/monitortest.go | 325 +++++++++++++++++- 1 file changed, 324 insertions(+), 1 deletion(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index d09887f7117f..61a00d5a8f0e 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -55,7 +55,330 @@ func (e bindingException) matches(binding rbacv1.ClusterRoleBinding, checkID str // visible in CI and can be burned down. // // No new entries should be added to this list without the sign off of an OpenShift Architect. -var trackedExceptions = []bindingException{} +var trackedExceptions = []bindingException{ + { + name: "cloud-credential-operator-rolebinding", + checkID: "admission-webhooks", + roleRef: "cloud-credential-operator-role", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cloud-credential-operator", Name: "cloud-credential-operator"}}, + note: "TODO", + }, + { + name: "cloud-credential-operator-rolebinding", + checkID: "escalate-rbac", + roleRef: "cloud-credential-operator-role", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cloud-credential-operator", Name: "cloud-credential-operator"}}, + note: "TODO", + }, + { + name: "cluster-autoscaler-operator", + checkID: "admission-webhooks", + roleRef: "cluster-autoscaler-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-machine-api", Name: "cluster-autoscaler-operator"}}, + note: "TODO", + }, + { + name: "cluster-baremetal-operator", + checkID: "admission-webhooks", + roleRef: "cluster-baremetal-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-machine-api", Name: "cluster-baremetal-operator"}}, + note: "TODO", + }, + { + name: "cluster-monitoring-operator", + checkID: "admission-webhooks", + roleRef: "cluster-monitoring-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-monitoring", Name: "cluster-monitoring-operator"}}, + note: "TODO", + }, + { + name: "cluster-network-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-network-operator", Name: "cluster-network-operator"}}, + note: "TODO", + }, + { + name: "cluster-olm-operator-role", + checkID: "admission-webhooks", + roleRef: "cluster-olm-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cluster-olm-operator", Name: "cluster-olm-operator"}}, + note: "TODO", + }, + { + name: "cluster-olm-operator-role", + checkID: "escalate-rbac", + roleRef: "cluster-olm-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cluster-olm-operator", Name: "cluster-olm-operator"}}, + note: "TODO", + }, + { + name: "cluster-storage-operator-role", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cluster-storage-operator", Name: "cluster-storage-operator"}}, + note: "TODO", + }, + { + name: "cluster-version-operator-1", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cluster-version", Name: "cluster-version-operator"}}, + note: "TODO", + }, + { + name: "custom-account-openshift-machine-config-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-machine-config-operator", Name: "machine-config-operator"}}, + note: "TODO", + }, + { + name: "machine-api-operator", + checkID: "admission-webhooks", + roleRef: "machine-api-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-machine-api", Name: "machine-api-operator"}}, + note: "TODO", + }, + { + name: "olm-operator-binding-openshift-operator-lifecycle-manager", + checkID: "admission-webhooks", + roleRef: "system:controller:operator-lifecycle-manager", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-operator-lifecycle-manager", Name: "olm-operator-serviceaccount"}}, + note: "TODO", + }, + { + name: "olm-operator-binding-openshift-operator-lifecycle-manager", + checkID: "escalate-rbac", + roleRef: "system:controller:operator-lifecycle-manager", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-operator-lifecycle-manager", Name: "olm-operator-serviceaccount"}}, + note: "TODO", + }, + { + name: "openshift-dns-operator", + checkID: "impersonate", + roleRef: "openshift-dns-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-dns-operator", Name: "dns-operator"}}, + note: "TODO", + }, + { + name: "openshift-ingress-operator", + checkID: "impersonate", + roleRef: "openshift-ingress-operator", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-ingress-operator", Name: "ingress-operator"}}, + note: "TODO", + }, + { + name: "openshift-ingress-operator-sail-library", + checkID: "admission-webhooks", + roleRef: "openshift-ingress-operator-sail-library", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-ingress-operator", Name: "ingress-operator"}}, + note: "TODO", + }, + { + name: "openshift-ingress-operator-sail-library", + checkID: "escalate-rbac", + roleRef: "openshift-ingress-operator-sail-library", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-ingress-operator", Name: "ingress-operator"}}, + note: "TODO", + }, + { + name: "operator-controller-cluster-admin-rolebinding", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-operator-controller", Name: "operator-controller-controller-manager"}}, + note: "TODO", + }, + { + name: "storage-version-migration-migrator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-storage-version-migrator", Name: "kube-storage-version-migrator-sa"}}, + note: "TODO", + }, + { + name: "system:controller:clusterrole-aggregation-controller", + checkID: "escalate-rbac", + roleRef: "system:controller:clusterrole-aggregation-controller", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "kube-system", Name: "clusterrole-aggregation-controller"}}, + note: "TODO", + }, + { + name: "system:controller:generic-garbage-collector", + checkID: "admission-webhooks", + roleRef: "system:controller:generic-garbage-collector", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "kube-system", Name: "generic-garbage-collector"}}, + note: "TODO", + }, + { + name: "system:masters", + checkID: "cluster-admin", + roleRef: "system:master", + subjects: []rbacv1.Subject{{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}}, + note: "TODO", + }, + { + name: "system:openshift:controller:service-ca", + checkID: "admission-webhooks", + roleRef: "system:openshift:controller:service-ca", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-service-ca", Name: "service-ca"}}, + note: "TODO", + }, + { + name: "system:openshift:controller:template-instance-controller:admin", + checkID: "impersonate", + roleRef: "admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-infra", Name: "template-instance-controller"}}, + note: "TODO", + }, + { + name: "system:openshift:controller:template-instance-finalizer-controller:admin", + checkID: "impersonate", + roleRef: "admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-infra", Name: "template-instance-finalizer-controller"}}, + note: "TODO", + }, + { + name: "system:openshift:oauth-apiserver", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-oauth-apiserver", Name: "oauth-apiserver-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:openshift-apiserver", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-apiserver", Name: "openshift-apiserver-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:openshift-authentication", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-authentication", Name: "oauth-openshift"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:authentication", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-authentication-operator", Name: "authentication-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:cluster-kube-scheduler-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-scheduler-operator", Name: "openshift-kube-scheduler-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:etcd-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-etcd-operator", Name: "etcd-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-apiserver-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-apiserver-operator", Name: "kube-apiserver-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-apiserver-recovery", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-apiserver", Name: "localhost-recovery-client"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-controller-manager-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-controller-manager-operator", Name: "kube-controller-manager-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-controller-manager-recovery", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-controller-manager", Name: "localhost-recovery-client"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-scheduler-recovery", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-scheduler", Name: "localhost-recovery-client"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:kube-storage-version-migrator-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-storage-version-migrator-operator", Name: "kube-storage-version-migrator-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-apiserver-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-apiserver-operator", Name: "openshift-apiserver-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-config-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-config-operator", Name: "openshift-config-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-controller-manager-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-controller-manager-operator", Name: "openshift-controller-manager-operator"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-etcd-installer", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-etcd", Name: "installer-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-kube-apiserver-installer", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-apiserver", Name: "installer-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-kube-controller-manager-installer", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-controller-manager", Name: "installer-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:openshift-kube-scheduler-installer", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-kube-scheduler", Name: "installer-sa"}}, + note: "TODO", + }, + { + name: "system:openshift:operator:service-ca-operator", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-service-ca-operator", Name: "service-ca-operator"}}, + note: "TODO", + }, +} // permanentExceptions are approved escalation grants that are legitimate and expected forever. These // are silently accepted: they produce no JUnit result at all. Reserve this list for grants that are From ac488c0792718c3ea88e4a500ea3475bd51d90da Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Wed, 19 Aug 2026 17:53:51 +0100 Subject: [PATCH 4/9] Restrict check to openshift and kube namespaces --- .../rbacadminescalationtests/monitortest.go | 50 ++++++++++++------- .../monitortest_test.go | 40 ++++++++++++--- 2 files changed, 67 insertions(+), 23 deletions(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 61a00d5a8f0e..121f93ba7cc0 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -210,13 +210,6 @@ var trackedExceptions = []bindingException{ subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "kube-system", Name: "generic-garbage-collector"}}, note: "TODO", }, - { - name: "system:masters", - checkID: "cluster-admin", - roleRef: "system:master", - subjects: []rbacv1.Subject{{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}}, - note: "TODO", - }, { name: "system:openshift:controller:service-ca", checkID: "admission-webhooks", @@ -384,17 +377,11 @@ var trackedExceptions = []bindingException{ // are silently accepted: they produce no JUnit result at all. Reserve this list for grants that are // correct by design and will never be "fixed". // +// The canonical cluster-admin -> system:masters binding is not listed here: its only subject is a +// cluster-wide Group, so bindingInScope already excludes it (and every other non-namespaced grant). +// // No new entries should be added to this list without the sign off of an OpenShift Architect. -var permanentExceptions = []bindingException{ - { - // The cluster-admin binding grants cluster-admin to system:masters by design. - name: "cluster-admin", - checkID: clusterAdminCheckID, - roleRef: "cluster-admin", - subjects: []rbacv1.Subject{{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}}, - note: "by-design: binds cluster-admin to system:masters", - }, -} +var permanentExceptions = []bindingException{} // escalationCheck is a single way in which a ClusterRoleBinding can hand a subject a path to // cluster-admin. Each check becomes its own JUnit test (and its own independently flakeable @@ -537,6 +524,30 @@ func rulesToString(rules []rbacv1.PolicyRule) string { return strings.Join(descriptions.List(), "\n") } +// coreNamespacePrefixes are the namespaces that hold core cluster components. We only audit bindings +// that grant to a ServiceAccount in one of these namespaces. +var coreNamespacePrefixes = []string{"kube-", "openshift-"} + +// bindingInScope reports whether the binding grants to at least one ServiceAccount in a core +// namespace (prefixed kube- or openshift-). Bindings that only grant to subjects outside those +// namespaces are out of scope: transient e2e test namespaces come and go with random names (so an +// allowlist entry could never match), and cluster-wide groups/users (e.g. system:masters) are not +// namespaced. Restricting to core namespaces keeps the audit focused on the payload's own +// components. +func bindingInScope(binding rbacv1.ClusterRoleBinding) bool { + for _, subject := range binding.Subjects { + if subject.Kind != rbacv1.ServiceAccountKind { + continue + } + for _, prefix := range coreNamespacePrefixes { + if strings.HasPrefix(subject.Namespace, prefix) { + return true + } + } + } + return false +} + // evaluateBinding runs every escalation check against a single ClusterRoleBinding and returns the // resulting JUnit cases. Only checks that fire produce cases, and the outcome depends on the // exception class of the (binding, check) pair: @@ -544,6 +555,11 @@ func rulesToString(rules []rbacv1.PolicyRule) string { // - tracked exception: emit a fail plus a passing duplicate (a flake) so it stays visible. // - no exception: emit a hard fail. func evaluateBinding(binding rbacv1.ClusterRoleBinding, rolesByName map[string][]rbacv1.PolicyRule) []*junitapi.JUnitTestCase { + // Only audit bindings that grant to a core-component ServiceAccount (see bindingInScope). + if !bindingInScope(binding) { + return nil + } + // A ClusterRoleBinding's RoleRef always references a ClusterRole. A dangling reference grants // nothing, so there is nothing to evaluate. roleRules, ok := rolesByName[binding.RoleRef.Name] diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go index ebe48e04d2e0..7afe652fd627 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -32,7 +32,9 @@ func checkIDForName(name string) string { } func TestEvaluateBinding(t *testing.T) { - saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sa"} + // Subjects must be ServiceAccounts in a core (kube-/openshift-) namespace to be in scope. + saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "openshift-ns", Name: "sa"} + permSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "openshift-perm", Name: "perm-sa"} // Seed a tracked exception for the duration of this test so we can exercise the flake path. It // pins the exact grant: name, check, roleRef, and subjects. @@ -46,6 +48,17 @@ func TestEvaluateBinding(t *testing.T) { }) defer func() { trackedExceptions = originalTracked }() + // Seed a permanent exception (in-scope, so it is reached) to exercise the silent-accept path. + originalPermanent := permanentExceptions + permanentExceptions = append(append([]bindingException{}, originalPermanent...), bindingException{ + name: "perm-admin", + checkID: "cluster-admin", + roleRef: "cluster-admin", + subjects: []rbacv1.Subject{permSubject}, + note: "by-design", + }) + defer func() { permanentExceptions = originalPermanent }() + clusterAdminRule := rule([]string{"*"}, []string{"*"}, []string{"*"}) escalateRule := rbacv1.PolicyRule{Verbs: []string{"escalate"}, APIGroups: []string{rbacv1.GroupName}, Resources: []string{"clusterroles"}} impersonateRule := rbacv1.PolicyRule{Verbs: []string{"impersonate"}, APIGroups: []string{""}, Resources: []string{"users"}} @@ -96,10 +109,25 @@ func TestEvaluateBinding(t *testing.T) { wantCheckIDs: nil, }, { - // The cluster-admin binding is a permanent exception: no JUnit result at all, and the + // A binding that only grants to a ServiceAccount outside the core namespaces (e.g. a + // transient e2e test namespace) is out of scope and emits nothing. + name: "out-of-scope namespace emits nothing", + binding: binding("e2e-test-thing", "admin-role", rbacv1.Subject{Kind: "ServiceAccount", Namespace: "e2e-test-thing", Name: "sa"}), + rolesByName: map[string][]rbacv1.PolicyRule{"admin-role": {clusterAdminRule}}, + wantCheckIDs: nil, + }, + { + // A binding whose only subject is a cluster-wide group (no namespace) is out of scope. + name: "group-only subject emits nothing", + binding: binding("group-admin", "admin-role", rbacv1.Subject{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}), + rolesByName: map[string][]rbacv1.PolicyRule{"admin-role": {clusterAdminRule}}, + wantCheckIDs: nil, + }, + { + // A permanent exception (in scope) is silently accepted: no JUnit result at all, and the // cluster-admin short-circuit still suppresses the subsumed checks. name: "permanent exception emits nothing", - binding: binding("cluster-admin", "cluster-admin", rbacv1.Subject{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}), + binding: binding("perm-admin", "cluster-admin", permSubject), rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, wantCheckIDs: nil, }, @@ -107,9 +135,9 @@ func TestEvaluateBinding(t *testing.T) { // A new subject on the permanently-excepted binding no longer matches the approved grant, // so the exemption is revoked and the finding hard-fails. name: "permanent exception revoked by new subject", - binding: binding("cluster-admin", "cluster-admin", - rbacv1.Subject{Kind: "Group", APIGroup: rbacv1.GroupName, Name: "system:masters"}, - rbacv1.Subject{Kind: "ServiceAccount", Namespace: "ns", Name: "sneaky"}), + binding: binding("perm-admin", "cluster-admin", + permSubject, + rbacv1.Subject{Kind: "ServiceAccount", Namespace: "openshift-ns", Name: "sneaky"}), rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, wantCheckIDs: []string{"cluster-admin"}, }, From f267f3189b80d22ef893118811e74baf3df26051 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 21 Aug 2026 10:45:06 +0100 Subject: [PATCH 5/9] Ensure resource names correctly accounts for ineffective restriction on create/deletecollection --- .../rbacadminescalationtests/monitortest.go | 44 +++++++++++- .../monitortest_test.go | 70 +++++++++++++++++++ 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 121f93ba7cc0..572482d469e8 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -442,6 +442,14 @@ var escalationChecks = []escalationCheck{ // them can point a webhook at their own endpoint to read the contents of (and, for mutating // webhooks, rewrite) any admitted object cluster-wide — including secrets and RBAC objects — // giving them an out-of-band path to cluster-admin. + // + // update/patch scoped to specific resourceNames are NOT an escalation path and are not + // reported: a subject that can only modify a named webhook config it already owns cannot reach + // arbitrary objects, and rbacvalidation.Covers already treats such resourceName-restricted + // rules as not covering the unscoped atoms below (so they never fire). create, on the other + // hand, is always reported even when scoped by resourceName: the API server ignores + // resourceNames for create, so the restriction is ineffective and the subject can still mint an + // arbitrary malicious webhook (see roleGrantsAny / resourceNameIneffectiveVerbs). id: "admission-webhooks", desc: "create or modify admission webhook configurations", rules: []rbacv1.PolicyRule{ @@ -474,13 +482,45 @@ func matchException(list []bindingException, binding rbacv1.ClusterRoleBinding, return "", false } +// resourceNameIneffectiveVerbs are verbs for which the API server ignores a rule's resourceNames. +// The restriction is silently ineffective, so a rule that appears to "scope" one of these verbs in +// fact grants it on every object of the resource. Per the Kubernetes RBAC docs, create and +// deletecollection cannot be restricted by resourceName (for create, the object name is not known at +// authorization time). +var resourceNameIneffectiveVerbs = sets.New("create", "deletecollection") + +// verbIgnoresResourceNames reports whether the atom's verb is one whose resourceNames the API server +// ignores. Atoms come from BreakdownRule, so each has exactly one verb. +func verbIgnoresResourceNames(verbs []string) bool { + return resourceNameIneffectiveVerbs.HasAny(verbs...) +} + +// stripResourceNames returns a copy of rules with ResourceNames cleared. Used to test coverage of a +// verb whose resourceNames the API server ignores, so that an ineffective restriction cannot hide an +// escalation path from rbacvalidation.Covers (which otherwise treats a resourceName-scoped owner rule +// as not covering the unscoped atom). +func stripResourceNames(rules []rbacv1.PolicyRule) []rbacv1.PolicyRule { + out := make([]rbacv1.PolicyRule, len(rules)) + for i, r := range rules { + r.ResourceNames = nil + out[i] = r + } + return out +} + // roleGrantsAny reports whether roleRules cover ANY atomic permission in servantRules, returning the -// matched atoms for reporting. Coverage is wildcard-aware (handled by rbacvalidation.Covers). +// matched atoms for reporting. Coverage is wildcard-aware (handled by rbacvalidation.Covers). For +// verbs whose resourceNames the API server ignores (create, deletecollection), the role's rules are +// compared with resourceNames stripped so an ineffective restriction is still flagged. func roleGrantsAny(roleRules, servantRules []rbacv1.PolicyRule) (bool, []rbacv1.PolicyRule) { matched := []rbacv1.PolicyRule{} for _, servantRule := range servantRules { for _, atom := range rbacvalidation.BreakdownRule(servantRule) { - if covered, _ := rbacvalidation.Covers(roleRules, []rbacv1.PolicyRule{atom}); covered { + candidateRules := roleRules + if verbIgnoresResourceNames(atom.Verbs) { + candidateRules = stripResourceNames(roleRules) + } + if covered, _ := rbacvalidation.Covers(candidateRules, []rbacv1.PolicyRule{atom}); covered { matched = append(matched, atom) } } diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go index 7afe652fd627..8d2a3733d1df 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -20,6 +20,10 @@ func rule(verbs, groups, resources []string) rbacv1.PolicyRule { return rbacv1.PolicyRule{Verbs: verbs, APIGroups: groups, Resources: resources} } +func ruleWithNames(verbs, groups, resources, resourceNames []string) rbacv1.PolicyRule { + return rbacv1.PolicyRule{Verbs: verbs, APIGroups: groups, Resources: resources, ResourceNames: resourceNames} +} + // checkID extracts the escalation check that a test case name corresponds to by matching against the // known checks' descriptions. func checkIDForName(name string) string { @@ -64,6 +68,9 @@ func TestEvaluateBinding(t *testing.T) { impersonateRule := rbacv1.PolicyRule{Verbs: []string{"impersonate"}, APIGroups: []string{""}, Resources: []string{"users"}} readOnlyRule := rule([]string{"get", "list", "watch"}, []string{""}, []string{"pods"}) + webhookGroup := []string{"admissionregistration.k8s.io"} + webhookResources := []string{"validatingwebhookconfigurations", "mutatingwebhookconfigurations"} + tests := []struct { name string binding rbacv1.ClusterRoleBinding @@ -141,6 +148,34 @@ func TestEvaluateBinding(t *testing.T) { rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, wantCheckIDs: []string{"cluster-admin"}, }, + { + // Modifying webhook configs scoped to a specific resourceName cannot be used to point a + // webhook at an arbitrary target, so it is not an escalation path and emits nothing. + name: "webhook update/patch scoped by resourceName emits nothing", + binding: binding("scoped-webhook", "scoped-webhook-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"scoped-webhook-role": {ruleWithNames([]string{"update", "patch"}, webhookGroup, webhookResources, []string{"my-webhook"})}}, + wantCheckIDs: nil, + }, + { + // resourceNames are ineffective for create (the API server ignores them), so a role that + // appears to "restrict" create on webhook configs to a name in fact grants create on all of + // them and is still an escalation path. + name: "webhook create scoped by resourceName still fires", + binding: binding("scoped-create-webhook", "scoped-create-webhook-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"scoped-create-webhook-role": {ruleWithNames([]string{"create"}, webhookGroup, webhookResources, []string{"my-webhook"})}}, + wantCheckIDs: []string{"admission-webhooks"}, + }, + { + // Unrestricted create on webhook configs is always an escalation path (create cannot be + // scoped by resourceName), so it fires even when update/patch are scoped. + name: "webhook unrestricted create fires despite scoped update/patch", + binding: binding("open-create-webhook", "open-create-webhook-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"open-create-webhook-role": { + rule([]string{"create"}, webhookGroup, webhookResources), + ruleWithNames([]string{"update", "patch"}, webhookGroup, webhookResources, []string{"my-webhook"}), + }}, + wantCheckIDs: []string{"admission-webhooks"}, + }, { // A tracked exception flakes: one fail + one pass for that check. name: "tracked exception flakes", @@ -204,6 +239,41 @@ func TestEvaluateBinding(t *testing.T) { } } +// TestWebhookScopedVerbsNotReported pins the reporting granularity for the admission-webhooks check: +// update/patch scoped by resourceName are not an escalation path and must not appear in a finding, +// while an unrestricted create (which cannot be scoped) still is reported. +func TestWebhookScopedVerbsNotReported(t *testing.T) { + saSubject := rbacv1.Subject{Kind: "ServiceAccount", Namespace: "openshift-ns", Name: "sa"} + webhookGroup := []string{"admissionregistration.k8s.io"} + webhookResources := []string{"validatingwebhookconfigurations", "mutatingwebhookconfigurations"} + + b := binding("mixed-webhook", "mixed-webhook-role", saSubject) + rolesByName := map[string][]rbacv1.PolicyRule{"mixed-webhook-role": { + rule([]string{"create"}, webhookGroup, webhookResources), + ruleWithNames([]string{"update", "patch"}, webhookGroup, webhookResources, []string{"my-webhook"}), + }} + + junits := evaluateBinding(b, rolesByName) + + var failures []string + for _, j := range junits { + if j.FailureOutput != nil { + failures = append(failures, j.SystemOut) + } + } + if len(failures) != 1 { + t.Fatalf("expected exactly one failing case, got %d: %v", len(failures), failures) + } + msg := failures[0] + if !strings.Contains(msg, "create") { + t.Errorf("expected finding to report unrestricted create, got:\n%s", msg) + } + // The scoped update/patch grant must not surface as an escalation path. + if strings.Contains(msg, "update") || strings.Contains(msg, "patch") { + t.Errorf("finding must not report resourceName-scoped update/patch, got:\n%s", msg) + } +} + func TestRoleGrantsAny(t *testing.T) { // A role with resource wildcard should be detected as covering escalate on clusterroles. wildcard := []rbacv1.PolicyRule{rule([]string{"*"}, []string{rbacv1.GroupName}, []string{"*"})} From e19a0f8785c72bbc3db1cb287bf99c064c519e2a Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 21 Aug 2026 10:47:58 +0100 Subject: [PATCH 6/9] Add vSphere exception --- .../authentication/rbacadminescalationtests/monitortest.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 572482d469e8..1f72145ff8bd 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -371,6 +371,13 @@ var trackedExceptions = []bindingException{ subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-service-ca-operator", Name: "service-ca-operator"}}, note: "TODO", }, + { + name: "vmware-vsphere-csi-driver-operator-clusterrolebinding", + checkID: "admission-webhooks", + roleRef: "vmware-vsphere-csi-driver-operator-clusterrole", + subjects: []rbacv1.Subject{{Kind: "ServiceAccount", Namespace: "openshift-cluster-csi-drivers", Name: "vmware-vsphere-csi-driver-operator"}}, + note: "TODO", + }, } // permanentExceptions are approved escalation grants that are legitimate and expected forever. These From 7e2366f9e2af2bd806222d04a6db2f3a5eed116a Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 21 Aug 2026 10:48:55 +0100 Subject: [PATCH 7/9] Allow approval only from architects --- .../authentication/rbacadminescalationtests/OWNERS | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 pkg/monitortests/authentication/rbacadminescalationtests/OWNERS diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/OWNERS b/pkg/monitortests/authentication/rbacadminescalationtests/OWNERS new file mode 100644 index 000000000000..cb58d4e18d24 --- /dev/null +++ b/pkg/monitortests/authentication/rbacadminescalationtests/OWNERS @@ -0,0 +1,10 @@ +# See the OWNERS docs at https://go.k8s.io/owners + +options: + no_parent_owners: true + +approvers: +- JoelSpeed +- sdodson +- dgoodwin +- enxebre From d346e9ddffd3d37d8907bb3965f9206751fb42e1 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Fri, 21 Aug 2026 11:50:04 +0100 Subject: [PATCH 8/9] Ensure bind to cluster-admin is flagged as an escalation path --- .../rbacadminescalationtests/monitortest.go | 14 +++++++++++- .../monitortest_test.go | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 1f72145ff8bd..2ff0efcbab0a 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -408,6 +408,10 @@ type escalationCheck struct { // escalationChecks so that its short-circuit (skip the subsumed, more specific checks) works. const clusterAdminCheckID = "cluster-admin" +// clusterAdminRoleName is the name of the built-in cluster-admin ClusterRole. It is the role whose +// `bind` we treat as an escalation path (see the escalate-rbac check). +const clusterAdminRoleName = "cluster-admin" + // escalationChecks is the curated, tunable set of escalation paths we audit. Broadening this set // (e.g. cluster-wide secrets read, pod/exec, node proxy) will expand findings and the allowlist, so // it is deliberately conservative. @@ -428,10 +432,18 @@ var escalationChecks = []escalationCheck{ // NOT allow escalation on its own: the rbac policybased storage strategy runs // ConfirmNoEscalation and rejects writing/binding any permission the caller does not already // hold, unless the caller has `escalate` (for role rules) or `bind` (for a role reference). + // + // Unlike `escalate`, `bind` is only an escalation path when it can reference a role more + // powerful than the caller. `bind` scoped by resourceNames to a set of roles that does NOT + // include cluster-admin cannot grant anything those roles do not already hold, so it is not + // flagged. Restricting the bind atom to the cluster-admin resourceName makes Covers fire for an + // unrestricted bind (which can bind cluster-admin) and for a bind explicitly scoped to include + // cluster-admin, but not for a bind scoped to unrelated roles. id: "escalate-rbac", desc: "escalate or bind RBAC roles", rules: []rbacv1.PolicyRule{ - rbacv1helpers.NewRule("escalate", "bind").Groups(rbacv1.GroupName).Resources("clusterroles", "roles").RuleOrDie(), + rbacv1helpers.NewRule("escalate").Groups(rbacv1.GroupName).Resources("clusterroles", "roles").RuleOrDie(), + rbacv1helpers.NewRule("bind").Groups(rbacv1.GroupName).Resources("clusterroles", "roles").Names(clusterAdminRoleName).RuleOrDie(), }, }, { diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go index 8d2a3733d1df..71801aa67da8 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -176,6 +176,28 @@ func TestEvaluateBinding(t *testing.T) { }}, wantCheckIDs: []string{"admission-webhooks"}, }, + { + // Unrestricted bind can reference the cluster-admin role, so it is an escalation path. + name: "unrestricted bind fires", + binding: binding("binder", "bind-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"bind-role": {rule([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"})}}, + wantCheckIDs: []string{"escalate-rbac"}, + }, + { + // bind scoped by resourceName to the cluster-admin role is a direct path to cluster-admin. + name: "bind scoped to cluster-admin fires", + binding: binding("ca-binder", "ca-bind-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"ca-bind-role": {ruleWithNames([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"}, []string{"cluster-admin"})}}, + wantCheckIDs: []string{"escalate-rbac"}, + }, + { + // bind scoped to roles other than cluster-admin cannot grant more than those roles already + // hold, so it is not an escalation path and emits nothing. + name: "bind scoped to non-cluster-admin emits nothing", + binding: binding("scoped-binder", "scoped-bind-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"scoped-bind-role": {ruleWithNames([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"}, []string{"view", "edit"})}}, + wantCheckIDs: nil, + }, { // A tracked exception flakes: one fail + one pass for that check. name: "tracked exception flakes", From c0b606b29a76cc554831c91cc42dcb6ab16c1523 Mon Sep 17 00:00:00 2001 From: Joel Speed Date: Mon, 24 Aug 2026 15:35:03 +0100 Subject: [PATCH 9/9] Temporarily stop producing flakes for tracked exceptions This moves any untracked issues to flakes and stops flaking tracked issues. This will help us narrow down the exceptions we need to track. --- .../rbacadminescalationtests/monitortest.go | 13 ++- .../monitortest_test.go | 85 +++++++++++-------- 2 files changed, 58 insertions(+), 40 deletions(-) diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go index 2ff0efcbab0a..68339e89e6d3 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest.go @@ -654,6 +654,11 @@ func evaluateBinding(binding rbacv1.ClusterRoleBinding, rolesByName map[string][ jira, tracked := matchException(trackedExceptions, binding, check.id) if tracked { msg += fmt.Sprintf("\n(tracked exception: %s)", jira) + + // Temporarily downgrade a tracked exception so that it doesn't produce any JUnit output. + // This will allow us to see only untracked exceptions in sippy and eventually work out whether + // we have caught all exceptions that need to be tracked. + continue } junits = append(junits, &junitapi.JUnitTestCase{ @@ -663,9 +668,11 @@ func evaluateBinding(binding rbacv1.ClusterRoleBinding, rolesByName map[string][ }) // Tracked exceptions flake rather than hard-fail: emit a passing duplicate with the same name. - if tracked { - junits = append(junits, &junitapi.JUnitTestCase{Name: testName}) - } + // Temporarily dowgrading all findings to flakes while we determine if we have caught all exceptions + // that need to be tracked. + // if tracked { + junits = append(junits, &junitapi.JUnitTestCase{Name: testName}) + //} if isClusterAdmin { break diff --git a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go index 71801aa67da8..5cbc804fbfdf 100644 --- a/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go +++ b/pkg/monitortests/authentication/rbacadminescalationtests/monitortest_test.go @@ -79,29 +79,33 @@ func TestEvaluateBinding(t *testing.T) { wantFlakeChecks map[string]bool }{ { - name: "direct cluster-admin", - binding: binding("some-admin", "admin-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"admin-role": {clusterAdminRule}}, - wantCheckIDs: []string{"cluster-admin"}, + name: "direct cluster-admin", + binding: binding("some-admin", "admin-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"admin-role": {clusterAdminRule}}, + wantCheckIDs: []string{"cluster-admin"}, + wantFlakeChecks: map[string]bool{"cluster-admin": true}, // Temporary }, { - name: "escalate rbac", - binding: binding("escalator", "escalate-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, - wantCheckIDs: []string{"escalate-rbac"}, + name: "escalate rbac", + binding: binding("escalator", "escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, + wantCheckIDs: []string{"escalate-rbac"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true}, // Temporary }, { - name: "impersonate", - binding: binding("imp", "imp-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"imp-role": {impersonateRule}}, - wantCheckIDs: []string{"impersonate"}, + name: "impersonate", + binding: binding("imp", "imp-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"imp-role": {impersonateRule}}, + wantCheckIDs: []string{"impersonate"}, + wantFlakeChecks: map[string]bool{"impersonate": true}, // Temporary }, { name: "two checks tripped", binding: binding("multi", "multi-role", saSubject), rolesByName: map[string][]rbacv1.PolicyRule{"multi-role": {escalateRule, impersonateRule}}, // escalate-rbac precedes impersonate in escalationChecks ordering. - wantCheckIDs: []string{"escalate-rbac", "impersonate"}, + wantCheckIDs: []string{"escalate-rbac", "impersonate"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true, "impersonate": true}, // Temporary }, { name: "benign role emits nothing", @@ -145,8 +149,9 @@ func TestEvaluateBinding(t *testing.T) { binding: binding("perm-admin", "cluster-admin", permSubject, rbacv1.Subject{Kind: "ServiceAccount", Namespace: "openshift-ns", Name: "sneaky"}), - rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, - wantCheckIDs: []string{"cluster-admin"}, + rolesByName: map[string][]rbacv1.PolicyRule{"cluster-admin": {clusterAdminRule}}, + wantCheckIDs: []string{"cluster-admin"}, + wantFlakeChecks: map[string]bool{"cluster-admin": true}, // Temporary }, { // Modifying webhook configs scoped to a specific resourceName cannot be used to point a @@ -160,10 +165,11 @@ func TestEvaluateBinding(t *testing.T) { // resourceNames are ineffective for create (the API server ignores them), so a role that // appears to "restrict" create on webhook configs to a name in fact grants create on all of // them and is still an escalation path. - name: "webhook create scoped by resourceName still fires", - binding: binding("scoped-create-webhook", "scoped-create-webhook-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"scoped-create-webhook-role": {ruleWithNames([]string{"create"}, webhookGroup, webhookResources, []string{"my-webhook"})}}, - wantCheckIDs: []string{"admission-webhooks"}, + name: "webhook create scoped by resourceName still fires", + binding: binding("scoped-create-webhook", "scoped-create-webhook-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"scoped-create-webhook-role": {ruleWithNames([]string{"create"}, webhookGroup, webhookResources, []string{"my-webhook"})}}, + wantCheckIDs: []string{"admission-webhooks"}, + wantFlakeChecks: map[string]bool{"admission-webhooks": true}, // Temporary }, { // Unrestricted create on webhook configs is always an escalation path (create cannot be @@ -174,21 +180,24 @@ func TestEvaluateBinding(t *testing.T) { rule([]string{"create"}, webhookGroup, webhookResources), ruleWithNames([]string{"update", "patch"}, webhookGroup, webhookResources, []string{"my-webhook"}), }}, - wantCheckIDs: []string{"admission-webhooks"}, + wantCheckIDs: []string{"admission-webhooks"}, + wantFlakeChecks: map[string]bool{"admission-webhooks": true}, // Temporary }, { // Unrestricted bind can reference the cluster-admin role, so it is an escalation path. - name: "unrestricted bind fires", - binding: binding("binder", "bind-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"bind-role": {rule([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"})}}, - wantCheckIDs: []string{"escalate-rbac"}, + name: "unrestricted bind fires", + binding: binding("binder", "bind-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"bind-role": {rule([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"})}}, + wantCheckIDs: []string{"escalate-rbac"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true}, // Temporary }, { // bind scoped by resourceName to the cluster-admin role is a direct path to cluster-admin. - name: "bind scoped to cluster-admin fires", - binding: binding("ca-binder", "ca-bind-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"ca-bind-role": {ruleWithNames([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"}, []string{"cluster-admin"})}}, - wantCheckIDs: []string{"escalate-rbac"}, + name: "bind scoped to cluster-admin fires", + binding: binding("ca-binder", "ca-bind-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"ca-bind-role": {ruleWithNames([]string{"bind"}, []string{rbacv1.GroupName}, []string{"clusterroles"}, []string{"cluster-admin"})}}, + wantCheckIDs: []string{"escalate-rbac"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true}, // Temporary }, { // bind scoped to roles other than cluster-admin cannot grant more than those roles already @@ -200,19 +209,21 @@ func TestEvaluateBinding(t *testing.T) { }, { // A tracked exception flakes: one fail + one pass for that check. - name: "tracked exception flakes", - binding: binding("tracked-esc", "escalate-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, - wantCheckIDs: []string{"escalate-rbac"}, - wantFlakeChecks: map[string]bool{"escalate-rbac": true}, + name: "tracked exception flakes", + binding: binding("tracked-esc", "escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"escalate-role": {escalateRule}}, + // Temporarily disable these checks + // wantCheckIDs: []string{"escalate-rbac"}, + // wantFlakeChecks: map[string]bool{"escalate-rbac": true}, }, { // Same tracked binding+check but repointed at a different escalating role: the roleRef no // longer matches the approved grant, so it hard-fails instead of flaking. - name: "tracked exception revoked by roleref change", - binding: binding("tracked-esc", "other-escalate-role", saSubject), - rolesByName: map[string][]rbacv1.PolicyRule{"other-escalate-role": {escalateRule}}, - wantCheckIDs: []string{"escalate-rbac"}, + name: "tracked exception revoked by roleref change", + binding: binding("tracked-esc", "other-escalate-role", saSubject), + rolesByName: map[string][]rbacv1.PolicyRule{"other-escalate-role": {escalateRule}}, + wantCheckIDs: []string{"escalate-rbac"}, + wantFlakeChecks: map[string]bool{"escalate-rbac": true}, // Temporary }, }