Conversation
…s#943) `postAddSecret` paired every AddSecret-capable role with every application and service principal in the tenant, with no discrimination. A managed identity is ingested as a plain `AZServicePrincipal`, so it was swept in and given an inbound `AZAddSecret` edge — a privilege-escalation path that cannot be walked, because a managed identity's credentials are issued and rotated by Azure and there is no addPassword/addKey on it. The fix excludes managed-identity service principals in `postAddSecret`. A managed identity is distinguished only by an inbound `AZManagedIdentity` edge (it has no dedicated node kind), so `managedIdentityServicePrincipalIDs` collects the end nodes of those edges and the loop skips them. The exclusion is applied in `postAddSecret` and not in the shared `TenantApplicationsAndServicePrincipals` query, which `postAddOwner` also uses — whether `AZAddOwner` to a managed identity is likewise bogus is a separate question and is left untouched here (happy to follow up on it). The `AZAddSecretHarness` gains a VM-backed managed identity, so the regression is load-bearing: without the exclusion the AddSecret count rises from 4 to 6, and the test also asserts no `AZAddSecret` edge targets the managed-identity SP. rvazarkar identified `packages/go/analysis/azure/post.go#addSecret` as the location on the issue; this is that change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughAzure analysis now identifies service principals linked to managed identities and excludes them from ChangesManaged identity AddSecret filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change prevents AddSecret privilege edges from targeting managed-identity service principals while retaining valid application and enterprise-application edges. The scoped regression coverage supports the intended behavior, with no current merge-blocking risk identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. (1 skipped: 1 too large.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
1 participant
Fixes #943.
The problem
postAddSecret(packages/go/analysis/azure/post.go) paired every AddSecret-capable role with every application and service principal in the tenant, with no discrimination. A managed identity is ingested as a plainAZServicePrincipal, so it was swept in and given an inboundAZAddSecretedge. That edge is bogus: a managed identity's credentials are issued and rotated by Azure — there is noaddPassword/addKeyon it — so an Application Administrator cannot mint one. The path renders as a privilege-escalation route that cannot be walked.The fix
There is no managed-identity node kind.
AZManagedIdentityis a relationship (resource --AZManagedIdentity--> ServicePrincipal), so the only way to recognise a managed identity is an inboundAZManagedIdentityedge — a fix written as a node-kind or property check silently matches nothing.managedIdentityServicePrincipalIDscollects the end nodes ofAZManagedIdentityedges, andpostAddSecretskips those targets. The exclusion is applied inpostAddSecret, not in the sharedTenantApplicationsAndServicePrincipalsquery;AddOwneris left untouched (a separate question).The managed-identity set is resolved once in
AppRoleAssignmentsand threaded through, rather than re-queried per tenant insidepostAddSecret(addresses the reviewer's earlier performance note).Test
AZAddSecretHarnessgains a VM-backed managed identity (a VM, a managed-identity SP, and theAZManagedIdentityedge).RoleAddSecretasserts exactly 4 edges and additionally that noAZAddSecretedge targets the managed-identity SP.go buildandgo vetpass; the integration test binary compiles with the harness changes.3 files, +68/-9.
Note: this supersedes #3240, which GitHub auto-locked after a bad force-push corrupted its diff view. This branch is now clean.
Summary by CodeRabbit
AZAddSecretrelationships from being assigned to service principals backed by Azure-managed identities.AZAddSecretrelationships.