Skip to content

OCPBUGS-58181: Fix nil pointer dereference in ensureRolesAssignedToManagedIdentity #987

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
jstuever wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OCPBUGS-58181: Fix nil pointer dereference in ensureRolesAssignedToManagedIdentity #987

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions pkg/cmd/provisioning/azure/create_managed_identities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,9 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
// at the specified scope
roleAssignmentExists := false
for _, roleAssignment := range existingRoleAssignments {
if roleAssignment.Properties == nil || roleAssignment.Properties.RoleDefinitionID == nil || roleAssignment.Properties.Scope == nil {
continue
}
if *roleDefinition.Properties.RoleName == roleBinding.Role && *roleAssignment.Properties.RoleDefinitionID == *roleDefinition.ID && *roleAssignment.Properties.Scope == scope {
roleAssignmentExists = true
log.Printf("Found existing role assignment %s for user-assigned managed identity with principal ID %s at scope %s", roleBinding.Role, managedIdentityPrincipalID, scope)
Expand Down Expand Up @@ -307,6 +310,10 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
}
}
if !found {
if existingRoleAssignment.Properties == nil || existingRoleAssignment.Properties.RoleDefinitionID == nil || existingRoleAssignment.Properties.Scope == nil {
log.Printf("Skipping role assignment %s with nil Properties, RoleDefinitionID, or Scope", *existingRoleAssignment.Name)
continue
}
Comment on lines +313 to +316
Copy link

@coderabbitai coderabbitai bot Mar 12, 2026

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:

cat -n pkg/cmd/provisioning/azure/create_managed_identities.go | sed -n '300,330p'

Repository: openshift/cloud-credential-operator

Length of output: 1604

🏁 Script executed:

head -50 pkg/cmd/provisioning/azure/create_managed_identities.go | grep -E "^import|armauthorization"

Repository: openshift/cloud-credential-operator

Length of output: 178

🌐 Web query:

azure-sdk-for-go RoleAssignment armauthorization Name field type

💡 Result:

In github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization (v2 and v3), the RoleAssignment resource’s Name field is a read-only *string:

// READ-ONLY; The role assignment name.
Name *string

[1], [2]

Sources:
[1] armauthorization v3 type RoleAssignment struct on pkg.go.dev
[2] armauthorization v2 type RoleAssignment struct on pkg.go.dev

🏁 Script executed:

cat -n pkg/cmd/provisioning/azure/create_managed_identities.go | sed -n '305,320p'

Repository: openshift/cloud-credential-operator

Length of output: 1071

This still leaves a nil-deref path in the cleanup loop.

The new guard only covers Properties/RoleDefinitionID/Scope, but Line 308 dereferences *existingRoleAssignment.Name before this guard runs, and Line 314 dereferences *existingRoleAssignment.Name within the skip block. Since armauthorization.RoleAssignment.Name is a *string pointer, the malformed payloads this PR is handling can still panic. Move the guard ahead of the name comparison at Line 308 and include Name in the nil checks.

Suggested fix 
 	for _, existingRoleAssignment := range existingRoleAssignments {
+		if existingRoleAssignment == nil || existingRoleAssignment.Name == nil ||
+			existingRoleAssignment.Properties == nil ||
+			existingRoleAssignment.Properties.RoleDefinitionID == nil ||
+			existingRoleAssignment.Properties.Scope == nil {
+			name := "<unknown>"
+			if existingRoleAssignment != nil && existingRoleAssignment.Name != nil {
+				name = *existingRoleAssignment.Name
+			}
+			log.Printf("Skipping role assignment %s with nil Name, Properties, RoleDefinitionID, or Scope", name)
+			continue
+		}
+
 		found := false
 		for _, shouldExistRoleAssignment := range shouldExistRoleAssignments {
-			if shouldExistRoleAssignment != nil && *shouldExistRoleAssignment.Name == *existingRoleAssignment.Name {
+			if shouldExistRoleAssignment != nil &&
+				shouldExistRoleAssignment.Name != nil &&
+				*shouldExistRoleAssignment.Name == *existingRoleAssignment.Name {
 				found = true
 			}
 		}
 		if !found {
-			if existingRoleAssignment.Properties == nil || existingRoleAssignment.Properties.RoleDefinitionID == nil || existingRoleAssignment.Properties.Scope == nil {
-				log.Printf("Skipping role assignment %s with nil Properties, RoleDefinitionID, or Scope", *existingRoleAssignment.Name)
-				continue
-			}
 			roleDefinition, err := getRoleDefinitionByID(client, *existingRoleAssignment.Properties.RoleDefinitionID)
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@pkg/cmd/provisioning/azure/create_managed_identities.go` around lines 313 -
316, The cleanup loop may still nil-deref existingRoleAssignment.Name; move the
guard that checks existingRoleAssignment.Properties, RoleDefinitionID, and Scope
to execute before any dereference of existingRoleAssignment.Name and extend it
to also check existingRoleAssignment.Name != nil; specifically, in the loop that
iterates existingRoleAssignment (the cleanup loop logic where the code compares
*existingRoleAssignment.Name and later logs it), add Name to the nil checks and
run that guard before the name comparison and before logging to avoid panics.

roleDefinition, err := getRoleDefinitionByID(client, *existingRoleAssignment.Properties.RoleDefinitionID)
if err != nil {
return errors.Wrapf(err, "failed to get role definition with role definition ID %s", *existingRoleAssignment.Properties.RoleDefinitionID)
Expand Down