Skip to content

CCO-687: Fix nil pointer dereference in Azure role assignment handling #879

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
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CCO-687: Fix nil pointer dereference in Azure role assignment handling #879

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
62 changes: 52 additions & 10 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 @@ -73,7 +73,9 @@ type: Opaque`
func createManagedIdentity(client *azureclients.AzureClientWrapper, name, resourceGroupName, subscriptionID, region, issuerURL, outputDir string, scopingResourceGroupNames []string, resourceTags map[string]string, credentialsRequest *credreqv1.CredentialsRequest, dryRun bool) error {
// Write dummy secrets with blank clientID and tenantID when doing a dry run.
if dryRun {
writeCredReqSecret(credentialsRequest, outputDir, "", "", subscriptionID, region)
if err := writeCredReqSecret(credentialsRequest, outputDir, "", "", subscriptionID, region); err != nil {
return fmt.Errorf("failed to write credential request secret: %w", err)
}
return nil
}

Expand Down Expand Up @@ -105,6 +107,11 @@ func createManagedIdentity(client *azureclients.AzureClientWrapper, name, resour
crProviderSpec.RoleBindings = append(crProviderSpec.RoleBindings, credreqv1.RoleBinding{Role: shortenedManagedIdentityName})
}

// Ensure the managed identity has valid properties
if userAssignedManagedIdentity.Properties == nil || userAssignedManagedIdentity.Properties.PrincipalID == nil {
return fmt.Errorf("user-assigned managed identity has invalid properties")
}

// Ensure roles from CredentialsRequest are assigned to the user-assigned managed identity
err = ensureRolesAssignedToManagedIdentity(client, *userAssignedManagedIdentity.Properties.PrincipalID, subscriptionID, crProviderSpec.RoleBindings, scopingResourceGroupNames)
if err != nil {
Expand All @@ -119,7 +126,14 @@ func createManagedIdentity(client *azureclients.AzureClientWrapper, name, resour
}
}

writeCredReqSecret(credentialsRequest, outputDir, *userAssignedManagedIdentity.Properties.ClientID, *userAssignedManagedIdentity.Properties.TenantID, subscriptionID, region)
// Ensure managed identity has client ID and tenant ID before writing secret
if userAssignedManagedIdentity.Properties.ClientID == nil || userAssignedManagedIdentity.Properties.TenantID == nil {
return fmt.Errorf("user-assigned managed identity is missing ClientID or TenantID")
}

if err := writeCredReqSecret(credentialsRequest, outputDir, *userAssignedManagedIdentity.Properties.ClientID, *userAssignedManagedIdentity.Properties.TenantID, subscriptionID, region); err != nil {
return fmt.Errorf("failed to write credential request secret: %w", err)
}
return nil
}

Expand Down Expand Up @@ -153,7 +167,7 @@ func ensureCustomRole(client *azureclients.AzureClientWrapper, roleName string,
if err != nil {
return err
}
roleDefinitions = append(roleDefinitions, pageResponse.RoleDefinitionListResult.Value...)
roleDefinitions = append(roleDefinitions, pageResponse.Value...)
}

var roleID string
Expand Down Expand Up @@ -219,7 +233,7 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
if err != nil {
return err
}
existingRoleAssignments = append(existingRoleAssignments, pageResponse.RoleAssignmentListResult.Value...)
existingRoleAssignments = append(existingRoleAssignments, pageResponse.Value...)
}

// shouldExistRoleAssignments are role assignments we validated should exist or were created.
Expand Down Expand Up @@ -256,7 +270,8 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
// at the specified scope
roleAssignmentExists := false
for _, roleAssignment := range existingRoleAssignments {
if *roleDefinition.Properties.RoleName == roleBinding.Role && *roleAssignment.Properties.RoleDefinitionID == *roleDefinition.ID && *roleAssignment.Properties.Scope == scope {
if roleAssignment.Properties != nil && roleAssignment.Properties.RoleDefinitionID != nil && roleAssignment.Properties.Scope != nil &&
*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)
shouldExistRoleAssignments = append(shouldExistRoleAssignments, roleAssignment)
Expand Down Expand Up @@ -285,9 +300,15 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
}

for _, existingRoleAssignment := range existingRoleAssignments {
// Skip role assignments with missing properties
if existingRoleAssignment.Properties == nil || existingRoleAssignment.Properties.RoleDefinitionID == nil || existingRoleAssignment.Properties.Scope == nil {
log.Printf("Skipping role assignment with missing properties")
continue
}

found := false
for _, shouldExistRoleAssignment := range shouldExistRoleAssignments {
if shouldExistRoleAssignment != nil && *shouldExistRoleAssignment.Name == *existingRoleAssignment.Name {
if shouldExistRoleAssignment != nil && existingRoleAssignment.Name != nil && shouldExistRoleAssignment.Name != nil && *shouldExistRoleAssignment.Name == *existingRoleAssignment.Name {
found = true
}
}
Expand All @@ -296,6 +317,19 @@ func ensureRolesAssignedToManagedIdentity(client *azureclients.AzureClientWrappe
if err != nil {
return errors.Wrapf(err, "failed to get role definition with role definition ID %s", *existingRoleAssignment.Properties.RoleDefinitionID)
}

// Check if roleDefinition has valid properties before using them
if roleDefinition.Properties == nil || roleDefinition.Properties.RoleName == nil {
log.Printf("Skipping deletion of role assignment with invalid role definition properties")
continue
}

// Check if existingRoleAssignment.Name is not nil before using it
if existingRoleAssignment.Name == nil {
log.Printf("Skipping deletion of role assignment with missing name")
continue
}

err = deleteRoleAssignment(client,
managedIdentityPrincipalID,
*existingRoleAssignment.Name,
Expand Down Expand Up @@ -329,7 +363,7 @@ func getRoleDefinitionByRoleName(client *azureclients.AzureClientWrapper, roleNa
if err != nil {
return nil, err
}
roleDefinitions = append(roleDefinitions, pageResponse.RoleDefinitionListResult.Value...)
roleDefinitions = append(roleDefinitions, pageResponse.Value...)
}
switch len(roleDefinitions) {
case 0:
Expand Down Expand Up @@ -462,7 +496,11 @@ func ensureUserAssignedManagedIdentity(client *azureclients.AzureClientWrapper,

// Found and validated existing user-assigned managed identity
if !needToCreateUserAssignedManagedIdentity && !needToUpdateUserAssignedManagedIdentity {
log.Printf("Found existing user-assigned managed identity %s", *getUserAssignedManagedIdentityResp.Identity.ID)
if getUserAssignedManagedIdentityResp.ID != nil {
log.Printf("Found existing user-assigned managed identity %s", *getUserAssignedManagedIdentityResp.ID)
} else {
log.Printf("Found existing user-assigned managed identity")
}
return &getUserAssignedManagedIdentityResp.Identity, nil
}

Expand All @@ -485,7 +523,11 @@ func ensureUserAssignedManagedIdentity(client *azureclients.AzureClientWrapper,
if needToCreateUserAssignedManagedIdentity {
verb = "Created"
}
log.Printf("%s user-assigned managed identity %s", verb, *userAssignedManagedIdentity.ID)
if userAssignedManagedIdentity.ID != nil {
log.Printf("%s user-assigned managed identity %s", verb, *userAssignedManagedIdentity.ID)
} else {
log.Printf("%s user-assigned managed identity", verb)
}
return &userAssignedManagedIdentity.Identity, nil
}

Expand Down Expand Up @@ -554,7 +596,7 @@ func ensureFederatedIdentityCredential(client *azureclients.AzureClientWrapper,
}
}
if !needToCreateFederatedIdentityCredential && !needToUpdateFederatedIdentityCredential {
log.Printf("Found existing federated identity credential %s", *federatedIdentityCredentialGetResp.FederatedIdentityCredential.ID)
log.Printf("Found existing federated identity credential %s", *federatedIdentityCredentialGetResp.ID)
return nil
}

Expand Down