[Michael Flanakin (flanakin)]

Summary

Adds an enableRbacAuthorization parameter to the FinOps hub Bicep templates that switches the remote-hub Key Vault from the legacy access-policy model to Azure RBAC. This is required by Cloud Adoption Framework (CAF) / Enterprise-Scale landing zone Azure Policy guardrails (e.g., "Enforce recommended guardrails for Azure Key Vault"), which flag vaults that use access policies instead of RBAC.

Fixes #1067

What changed

• src/templates/finops-hub/modules/fx/hub-app.bicep — the Key Vault resource now sets enableRbacAuthorization: app.hub.options.keyVaultEnableRbacAuthorization instead of a hardcoded false. When RBAC is enabled:
  • accessPolicies is forced to an empty array (Azure rejects a non-empty accessPolicies array when enableRbacAuthorization: true).
  • A new keyVaultRoleAssignment resource grants the Data Factory managed identity the built-in Key Vault Secrets User role (4633458b-17de-408a-b874-0445c86b69e6) on the vault — the RBAC equivalent of the secrets: ['get'] access policy it previously relied on. This is the only identity that reads secrets from this vault (verified below), so it's the only equivalent role assignment needed.
• src/templates/finops-hub/modules/fx/hub-types.bicep — added keyVaultEnableRbacAuthorization to HubProperties.options and threaded it through newHubInternal/newHub.
• src/templates/finops-hub/modules/hub.bicep and main.bicep — added an enableRbacAuthorization bool = false parameter, following the exact pattern already used for enablePurgeProtection.
• createUiDefinition.json — added a matching "Enable Key Vault RBAC authorization" checkbox next to the existing purge protection checkbox, gated to the remote-hub analytics engine path (the only path that deploys this vault).
• docs-mslearn/toolkit/changelog.md — added an entry under Unreleased.

Purge protection (enablePurgeProtection / enableSoftDelete) was already fully implemented on this branch (not something I needed to add) — enableSoftDelete: true is hardcoded, and enablePurgeProtection is already wired end-to-end as an opt-in parameter defaulting to false, following the exact convention I followed for RBAC here.

Investigation: what currently depends on access policies

I traced every consumer of the Key Vault (Microsoft.KeyVault/vaults in src/templates/finops-hub/modules/fx/hub-app.bicep:471, only deployed when the RemoteHub app requests the 'KeyVault' feature — modules/Microsoft.FinOpsHubs/RemoteHub/app.bicep:52):

1. Data Factory's AzureKeyVault linked service (hub-app.bicep:201, linkedService_keyVault) — ADF authenticates as its own system-assigned managed identity at runtime to read the remoteHubStorage connection secret via AzureKeyVaultSecret. This is the only data-plane secret reader, and it's exactly what the pre-existing keyVaultAccessPolicies var (secrets: ['get']) granted. Now granted via RBAC (Key Vault Secrets User) when enableRbacAuthorization: true.
2. The keyVault_secret module (fx/hub-vault.bicep, invoked from RemoteHub/app.bicep:59) writes the storage key as a nested ARM Microsoft.KeyVault/vaults/secrets resource. This is a management-plane (ARM) write authorized by the deploying principal's Azure RBAC role on the resource group (e.g., Contributor), not by the vault's own access-policy/RBAC-authorization setting — so it is unaffected either way.
3. No other hub app (Core, Exports, Analytics, AzureResourceGraph, etc.) references this Key Vault at all.

So the only identity needing a new role assignment is Data Factory's managed identity, which I added.

PR #1349 (closed, not merged)

Read gh pr diff 1349 before starting. It only added enablePurgeProtection (12 lines across main.bicep/hub.bicep/keyVault.bicep) against the pre-reorganization template layout — those exact file paths (modules/keyVault.bicep) no longer exist; the Key Vault resource now lives in modules/fx/hub-app.bicep as part of the namespace-based fx/Microsoft.FinOpsHubs restructuring. It didn't touch RBAC at all, and its purge-protection change has since been superseded by a more complete implementation already on dev/v15-prep. I did not build on it — nothing to build on for RBAC, and the purge-protection piece it targeted was already done more thoroughly elsewhere.

Design decision: opt-in, not forced-on

Both enableRbacAuthorization and enablePurgeProtection are irreversible per Azure's API contract:

• Once enablePurgeProtection: true is set on a vault, it cannot be reverted.
• Switching enableRbacAuthorization from false to true on an existing vault immediately stops honoring access policies; any identity not covered by an equivalent RBAC role assignment loses access with no rollback path (short of recreating the vault).

Because the FinOps hub template supports redeploying over existing hub instances (upgrade scenario), forcing either property on unconditionally would silently break already-deployed hubs that don't opt in, with no way back. I followed the existing precedent set by enablePurgeProtection (already opt-in, defaulting to false, surfaced identically in main.bicep/hub.bicep/createUiDefinition.json) and applied the same pattern to enableRbacAuthorization. Organizations that need CAF/Enterprise-Scale compliance can set both parameters to true explicitly; organizations upgrading an existing hub are not force-migrated into a breaking, irreversible change they didn't ask for.

I believe this is the safer default, but it does mean the compliance gap in #1067 isn't closed by default — only when explicitly enabled. If maintainers prefer defaulting new deployments to true while keeping upgrades safe, that would need a way to distinguish first-deploy from redeploy, which Bicep can't do natively (no reliable "does this resource already exist" check without a existing lookup that fails hard on first deploy). Flagging this as an open discussion point for reviewers.

Verification

• bicep build src/templates/finops-hub/main.bicep --stdout — clean, no errors (2 pre-existing unrelated warnings in Recommendations/app.bicep, confirmed present before this change via git stash).
• bicep build on hub.bicep, hub-app.bicep, and hub-types.bicep individually — all clean.
• pwsh -Command "./src/scripts/Test-PowerShell.ps1 -Lint" — 3418/3418 passed, including ms.date frontmatter checks (already current at 08/12/2026, no update needed).
• Did not run a live az deployment ... what-if — no Azure credentials available in this environment. Everything else that could be verified statically was.

Open questions for reviewers

1. Should new (first-time) deployments default enableRbacAuthorization/enablePurgeProtection to true while upgrades stay opt-in? Flagged above as not straightforwardly expressible in Bicep.
2. Confirm Key Vault Secrets User is the right role scope (read-only get/list on secrets) rather than Key Vault Secrets Officer — I matched the existing access policy's secrets: ['get'] scope exactly, so Secrets User (read-only) seemed correct, but worth a second look given ADF also needs to enumerate the linked service's secret at authoring/refresh time.

Test plan

• Lint tests (PowerShell / bicep build)
• PS -WhatIf / az validate (no Azure credentials in this environment)
• Manually deployed + verified
• Unit tests
• Integration tests

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Roland Krummenacher (RolandKrummenacher)]

Review — PR #2249

Approving the approach. The consumer trace is the part that matters most here and it holds up: the ADF AzureKeyVault linked service is the only data-plane secret reader, the keyVault_secret module write is management-plane and genuinely unaffected by the vault's authorization model, and no other hub app touches this vault. Granting exactly one Key Vault Secrets User assignment is the right scope — not a broader role, not a second assignment for the deploying principal.

Opt-in defaulting to false is also right, and for the reason stated: flipping enableRbacAuthorization on an existing vault silently stops honoring access policies with no rollback short of recreating the vault, and this template is explicitly redeploy-over-existing. Forcing it on would break deployed hubs at upgrade time. Following the enablePurgeProtection precedent keeps that consistent.

Two notes, neither blocking.

newHub() positional-argument fragility

keyVaultEnableRbacAuthorization is inserted mid-signature in both newHubInternal and newHub, and both are called positionally. There's exactly one caller today (modules/hub.bicep:190), so this is safe as written — I checked. But the signature is now 15+ positional bools and strings, and the next insertion is one where a mis-ordered pair of same-typed args compiles clean and misconfigures a hub silently. Not this PR's job to fix, but worth an issue: these should take a single object parameter, or at minimum new options should append rather than insert.

RBAC propagation vs. access-policy immediacy

Access policy grants take effect essentially immediately; RBAC role assignments can take a few minutes to propagate. ADF reads the secret at pipeline runtime rather than at deployment time, so in the normal case the assignment has long since propagated by the time anything needs it. The edge case is a pipeline triggered immediately after a first deployment with the flag on — that could see a transient 403 that looks like a misconfiguration rather than a timing artifact.

Not worth adding a dependsOn dance over, but if the deployment surfaces a "hub is ready" message anywhere, a sentence in the docs noting that RBAC-authorized vaults may need a few minutes before the first ingestion run succeeds would save someone a support round-trip.

Small thing

The enablePurgeProtection tooltip gained "This cannot be disabled once enabled" — good addition. The enableRbacAuthorization tooltip doesn't carry the equivalent warning, and its irreversibility is arguably the sharper one of the two (purge protection costs you a 90-day wait; a botched RBAC switch costs you vault access). Consider mirroring the wording from the main.bicep parameter description, which does say it.

[Roland Krummenacher (RolandKrummenacher)]

Re-review — PR #2249

One new commit since my 2026-08-18 review (3d9b06ab, the rename), plus the two replies. Approach and implementation still hold up; one gap worth closing before merge.

Resolved

Rename enableRbacAuthorization → enableKeyVaultRbacAuthorization — complete. Grepped the branch: the only surviving enableRbacAuthorization is the ARM schema property on Microsoft.KeyVault/vaults in hub-app.bicep, correctly left alone. Parameter declaration, newHub() call site, createUiDefinition element name and output binding, and the changelog entry all agree.

Dual-auth question — the answer is correct, and I verified the mechanism rather than taking it on faith. accessPolicies is a direct property on the vault resource, not the additive Microsoft.KeyVault/vaults/accessPolicies child type, and the vault is redeclared on every deploy. Incremental mode's "leave absent things alone" guarantee covers resources, not properties of a resource that is declared — so the [] array genuinely replaces the live policy. Clean cutover, no dual-auth state. Resolving that thread.

One gap: the PowerShell deployment path

Deploy-FinOpsHub exposes purge protection but not this. src/powershell/Public/Deploy-FinOpsHub.ps1 declares -EnablePurgeProtection as a switch (line 146), documents it (line 34), and forwards it under a >= '13.0' version gate (line 322). There is no equivalent for enableKeyVaultRbacAuthorization, so anyone deploying through the PowerShell module can't enable RBAC at all — portal and raw Bicep only.

The PR follows "the exact pattern already used for enablePurgeProtection," and that pattern spans four surfaces: main.bicep, hub.bicep, createUiDefinition.json, and the cmdlet. Three of four are done. The fix is small, with a template right beside it:

[Parameter()]
[switch]
$EnableKeyVaultRbacAuthorization,

plus a >= '15.0' gate block (matching how enableNatGateway is gated — branch is 15.0.0-dev.0) and a .PARAMETER help entry. I'd treat this as blocking-ish: the module is the scripted-deployment path for exactly the CAF/landing-zone customers this feature targets. Commented inline on the param declaration since the file itself isn't in the diff.

Still open from last review

Details inline: the tooltip irreversibility warning (actionable), RBAC propagation timing (cosmetic), and newHub() positional fragility (separate issue, not this PR's job).

Housekeeping

PR shows CONFLICTING against flanakin/v15-prep. I ran the merge locally — the only conflict is changelog.md. Details inline.

Verdict: sound work and a clean rename. Add the Deploy-FinOpsHub switch and the tooltip sentence, merge the base in, and this is ready to go.

[Michael Flanakin (flanakin)]

🤖 [AI][Claude] PR Update Summary

Addressed: 5 thread(s)

• ✅ Implemented: 2
• 🤔 Needs discussion: 3

Implemented the two concrete, low-risk gaps Roland flagged: added the missing
EnableKeyVaultRbacAuthorization switch to Deploy-FinOpsHub.ps1 (mirroring
the EnablePurgeProtection/enableNatGateway pattern, gated to template
version >= 15.0), and added the irreversibility warning to the portal
tooltip, matching the wording already in the main.bicep parameter
description. Lint suite: 3418/3418 passed.

Left for human judgment: the changelog.md merge conflict against
flanakin/v15-prep (git merge requires explicit approval), the newHub()
positional-argument fragility (explicitly flagged as a separate follow-up
issue, not this PR), and the RBAC-propagation-timing doc note (non-blocking,
no existing "hub is ready" message surface to attach it to).

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed after the rebase. The template, UI definition and tests carry over cleanly. One regression from the merge with dev: the two new version gates still cast $Version directly, so the single-number release format that dev now supports breaks when the RBAC switch is set.

[microsoft-github-policy-service]

@Michael Flanakin (@flanakin): you have some new feedback!

Please review and resolve all comments and I'll let reviewers know by removing the Needs: Attention label. If I miss anything, just reply with #needs-review and I'll update the status.

[Roland Krummenacher (RolandKrummenacher)]

Re-reviewed the fix commit (828e4f7). Both Key Vault RBAC gates now use the normalized $templateVersion, so -Version 15 and v15 pass and 14 gets the friendly error instead of a cast exception. Tests cover all five cases and Pester is green. The remaining merge conflict with flanakin/v15-prep is only in changelog.md; keeping both sides' entries should resolve it.