Skip to content
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.

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

Grant Cluster Service access to MGMT Key Vautls #867

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

Conversation

janboll
Copy link
Collaborator

@janboll janboll commented Nov 21, 2024

What this PR does

Jira: https://issues.redhat.com/browse/ARO-12201
Link to demo recording:

Special notes for your reviewer

@machi1990
Copy link
Collaborator

@miguelsorianod can you also help review this? It's related to the work around certs and secrets persist / retrieval / delete in CS for cluster provision flow

Comment on lines +253 to +257
for role in [
'Key Vault Secrets User'
'Key Vault Certificate User'
'Key Vault Certificates Officer'
]: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These three roles looks about right for me.
Anything else we need to remove / add @miguelsorianod ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess you will need Key Vault Secrets Officer as well to be able to create secrets, for things like the the ACR pull secret token etc

Copy link
Collaborator

@machi1990 machi1990 Nov 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. We need RW and I thought Key Vault Secrets User gives us that by looking at the comment in

// Perform any action on the secrets of a key vault, except manage permissions.
but it seems like the comment wrong it should be Read secret contents including secret portion of a certificate with private key. Non?

Comment on lines +271 to +275
for role in [
'Key Vault Secrets User'
'Key Vault Certificate User'
'Key Vault Certificates Officer'
]: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as what @geoberle mentioned above we need the Key Vault Secrets Officer and not Key Vault Secrets User

@@ -32,6 +32,7 @@ defaults:

# MGMT cluster specifics
mgmt:
clusterServicePrincipalId: /subscriptions/1d3378d3-5a3f-4712-85a1-2485495dfc4b/resourcegroups/hcp-underlay-{{ .ctx.regionShort }}-svc/providers/Microsoft.ManagedIdentity/userAssignedIdentities/image-sync
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
clusterServicePrincipalId: /subscriptions/1d3378d3-5a3f-4712-85a1-2485495dfc4b/resourcegroups/hcp-underlay-{{ .ctx.regionShort }}-svc/providers/Microsoft.ManagedIdentity/userAssignedIdentities/image-sync
clusterServicePrincipalId: /subscriptions/1d3378d3-5a3f-4712-85a1-2485495dfc4b/resourcegroups/hcp-underlay-{{ .ctx.regionShort }}-svc/providers/Microsoft.ManagedIdentity/userAssignedIdentities/clusters-service

like this?

Comment on lines +253 to +257
for role in [
'Key Vault Secrets User'
'Key Vault Certificate User'
'Key Vault Certificates Officer'
]: {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i guess you will need Key Vault Secrets Officer as well to be able to create secrets, for things like the the ACR pull secret token etc

}
]

module msiCluserServiceKeyVaultAccess '../modules/keyvault/keyvault-secret-access.bicep' = [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

@@ -25,6 +27,14 @@ var roleResourceIds = {
'Microsoft.Authorization/roleDefinitions/',
'b86a8fe4-44ce-4948-aee5-eccb2c155cd7'
)
'Key Vault Certificate User': subscriptionResourceId(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add comments to document what this role gives similar to what is done above for the other roles

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read entire certificate contents including secret and key portion. 

'Microsoft.Authorization/roleDefinitions/',
'db79e9a7-68ee-4b58-9aeb-b90e7c24fcba'
)
'Key Vault Certificates Officer': subscriptionResourceId(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perform any action on the certificates of a key vault, excluding reading the secret and key portions, and managing permissions. 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants