e2e test with rbac authorization instead of access policies on keyvault#4755
e2e test with rbac authorization instead of access policies on keyvault#4755cadenmarchese merged 7 commits intomasterfrom
Conversation
yithian
requested review from
alcasim,
bennerv,
cadenmarchese,
hawkowl,
hlipsig,
jharrington22,
kevinobriendotca,
kimorris27,
mociarain,
mrWinston,
rogbas,
sankur-codes,
tiguelu,
tsatam,
tuxerrante,
ventifus,
wanghaoran1988 and
yjst2012
as code owners
yithian
force-pushed
the
yithian/ARO-25803
branch
from
7b224d4 to
ca66976
Compare
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
bizz001
left a comment
bizz001
left a comment
There was a problem hiding this comment.
This switches the vault to Key Vault RBAC, but I only see the DES permissions being granted via access policy. Since access policies no longer authorize data-plane access once RBAC is enabled, shouldn’t this PR also add the corresponding Key Vault RBAC grant for the DES identity?
Docs:
https://learn.microsoft.com/en-us/azure/key-vault/general/rbac-guide
https://learn.microsoft.com/en-us/azure/virtual-machines/linux/disks-enable-customer-managed-keys-cli
@bizz001 Wait, is this a single keyvault for the whole e2e environment (that all clusters share)? Or is it one keyvault-per-cluster? If it's a single keyvault, I'll need to figure out a way to test this change (and thus, our existing documentation) without breaking things for everyone else |
| func (g *generator) diskEncryptionKeyVaultRBAC() *arm.Resource { | ||
| // use the Azure built-in Key Vault Crypto Service Encryption User role | ||
| // See: https://learn.microsoft.com/en-us/azure/key-vault/general/rbac-guide?tabs=azure-cli#azure-built-in-roles-for-key-vault-data-plane-operations | ||
| return rbac.ResourceRoleAssignment("e147488a-f6f5-4113-8e2d-b22465e65bf6", tenantIDHack, "Microsoft.KeyVault", "parameters['kvName']") |
There was a problem hiding this comment.
Please closely review this line! The listed ID is the Key Vault Crypto Service Encryption User built in Azure role, which I think matches the former access policy. However, I'm not sure if the tenant ID is correct here?
There was a problem hiding this comment.
The Tenant ID is incorrect here, I think. I would expect this role assignment to be granted over the SystemAssigned Managed Identity of the keyvault resource itself (similar to what the deleted KV Access Policy resource used for the ObjectID field).
yithian
force-pushed
the
yithian/ARO-25803
branch
from
00f464b to
3772d13
Compare
Because `rbac.ResourceRoleAssignment` already puts square brackets around the object ID for you
kimorris27
left a comment
kimorris27
left a comment
There was a problem hiding this comment.
Noting that you were gone for the day and you were hoping your last commit would have CI green tomorrow morning, I decided to push a commit myself that I think will fix it :D
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yithian
force-pushed
the
yithian/ARO-25803
branch
4 times, most recently
from
7f5043f to
d1bd819
Compare
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
yithian
force-pushed
the
yithian/ARO-25803
branch
from
d1bd819 to
8c1508d
Compare
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run ci |
|
Azure Pipelines successfully started running 1 pipeline(s). |
cadenmarchese
left a comment
cadenmarchese
left a comment
There was a problem hiding this comment.
Access policies are cleaned up along with the keyvault, but a role assignment is left behind until it's explicitly deleted. Can we ensure that we clean up this role assignment when we destroy the e2e cluster?
The code should be fine as-is. This is a per-E2E run key vault, and since the role assignment is done at the scope of the key vault resource, the role assignment gets deleted when the key vault gets deleted. A role assignment doesn't get deleted when its assignee is deleted, but it does get deleted when its scope is deleted. |
|
@kimorris27 thanks for the correction! |
cadenmarchese
dismissed
bizz001’s stale review
Comment requiring new role assignment was addressed and included in a later commit.
5 participants
Which issue this PR addresses:
For ARO-25803
What this PR does / why we need it:
This PR switches the e2e keyvault to be created with rbac instead of access policies. This keeps us aligned with Azure documentation and ensures that what we're telling customers to do actually works
Test plan for issue:
Run e2e
Is there any documentation that needs to be updated for this PR?
No, this keeps us aligned with what the documentation currently says
How do you know this will function as expected in production?
Checking the e2e results and the e2e cluster keyvault to see if it was created with rbac