From 16834d8ddd1319c74a3d28e754ddcfd0d03f89ed Mon Sep 17 00:00:00 2001 From: Rajdeep Chauhan Date: Thu, 10 Oct 2024 14:12:06 -0400 Subject: [PATCH] Fix Managed Identity Cluster creation dynamic validation flow (#3891) * Fix Dynamic Validation Flow for Workload Identity * fix the mock msi object id export * Remove unwanted code * update OIDC_STORAGE_ACCOUNT_NAME local env setup * ARO-11049 resolve comments --- Makefile | 1 + docs/deploy-development-rp.md | 1 + docs/prepare-a-shared-rp-development-environment.md | 2 +- env.example | 1 + hack/devtools/local_dev_env.sh | 6 ++++++ hack/devtools/msi.sh | 2 ++ pkg/cluster/cluster.go | 2 +- pkg/cluster/deploybaseresources.go | 7 ++++++- pkg/env/prod.go | 3 ++- .../azuresdk/armauthorization/roledefinitions.go | 10 +++++++++- .../dynamic/platformworkloadidentityprofile.go | 4 +++- 11 files changed, 33 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 0ab96cab914..0bffd9b7fb1 100644 --- a/Makefile +++ b/Makefile @@ -533,6 +533,7 @@ run-rp: ci-rp podman-secrets -e ARO_ADOPT_BY_HIVE="true" \ -e MOCK_MSI_TENANT_ID \ -e MOCK_MSI_CLIENT_ID \ + -e MOCK_MSI_OBJECT_ID \ -e MOCK_MSI_CERT \ --secret aks.kubeconfig,target=/app/secrets/aks.kubeconfig \ --secret proxy-client.key,target=/app/secrets/proxy-client.key \ diff --git a/docs/deploy-development-rp.md b/docs/deploy-development-rp.md index 0ba363cf97e..0b126d2a8f5 100644 --- a/docs/deploy-development-rp.md +++ b/docs/deploy-development-rp.md @@ -77,6 +77,7 @@ mock a cluster MSI. This script will also create the platform identities, platfo - `RP_MODE`: Set to `development` to use a development RP running at https://localhost:8443/. - `MOCK_MSI_CLIENT_ID`: Client ID for service principal that mocks cluster MSI (see previous step). + - `MOCK_MSI_OBJECT_ID`: Object ID for service principal that mocks cluster MSI (see previous step). - `MOCK_MSI_CERT`: Base64 encoded certificate for service principal that mocks cluster MSI (see previous step). - `MOCK_MSI_TENANT_ID`: Tenant ID for service principal that mocks cluster MSI (see previous step). - `PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS`: The platform workload identity role sets (see previous step or value in `local_dev_env.sh`). diff --git a/docs/prepare-a-shared-rp-development-environment.md b/docs/prepare-a-shared-rp-development-environment.md index ee65fdf1a82..f08cd0adf66 100644 --- a/docs/prepare-a-shared-rp-development-environment.md +++ b/docs/prepare-a-shared-rp-development-environment.md @@ -436,7 +436,7 @@ az ad app credential reset \ PARENT_DOMAIN_RESOURCEGROUP='$PARENT_DOMAIN_RESOURCEGROUP' export DOMAIN_NAME="\$LOCATION.\$PARENT_DOMAIN_NAME" export AZURE_ENVIRONMENT='AzurePublicCloud' - export OIDC_STORAGE_ACCOUNT_NAME="${RESOURCEGROUP//-}oic" + export OIDC_STORAGE_ACCOUNT_NAME="\${RESOURCEGROUP//-}oic" EOF ``` diff --git a/env.example b/env.example index 0079395ee0b..96e92defd3d 100644 --- a/env.example +++ b/env.example @@ -13,6 +13,7 @@ export ARO_IMAGE=arointsvc.azurecr.io/aro:latest # out or remove them from your env file if you're only going to be creating service principal # clusters. export MOCK_MSI_CLIENT_ID="replace_with_value_output_by_hack/devtools/msi.sh" +export MOCK_MSI_OBJECT_ID="replace_with_value_output_by_hack/devtools/msi.sh" export MOCK_MSI_CERT="replace_with_value_output_by_hack/devtools/msi.sh" export MOCK_MSI_TENANT_ID="replace_with_value_output_by_hack/devtools/msi.sh" export PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS="replace_with_value_output_by_hack/devtools/msi.sh" diff --git a/hack/devtools/local_dev_env.sh b/hack/devtools/local_dev_env.sh index 4d1a8c39013..7f3818bbd72 100755 --- a/hack/devtools/local_dev_env.sh +++ b/hack/devtools/local_dev_env.sh @@ -121,6 +121,10 @@ get_mock_msi_tenantID() { echo "$1" | jq -r .tenant } +get_mock_msi_objectID() { + az ad sp list --all --filter "appId eq '$1'" | jq -r ".[] | .id" +} + get_mock_msi_cert() { certFilePath=$(echo "$1" | jq -r '.fileWithCertAndPrivateKey') base64EncodedCert=$(base64 -w 0 "$certFilePath") @@ -234,6 +238,7 @@ create_miwi_env_file() { mockClientID=$(get_mock_msi_clientID "$mockMSI") mockTenantID=$(get_mock_msi_tenantID "$mockMSI") mockCert=$(get_mock_msi_cert "$mockMSI") + mockObjectID=$(get_mock_msi_objectID "$mockClientID") setup_platform_identity cluster_msi_role_assignment "${mockClientID}" @@ -243,6 +248,7 @@ export LOCATION=eastus export ARO_IMAGE=arointsvc.azurecr.io/aro:latest export RP_MODE=development # to use a development RP running at https://localhost:8443/ export MOCK_MSI_CLIENT_ID="$mockClientID" +export MOCK_MSI_OBJECT_ID="$mockObjectID" export MOCK_MSI_TENANT_ID="$mockTenantID" export MOCK_MSI_CERT="$mockCert" export PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS="$PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS" diff --git a/hack/devtools/msi.sh b/hack/devtools/msi.sh index 00b767d84f5..664dc0e05cf 100755 --- a/hack/devtools/msi.sh +++ b/hack/devtools/msi.sh @@ -29,12 +29,14 @@ fi mockClientID=$(get_mock_msi_clientID "$sp") mockTenantID=$(get_mock_msi_tenantID "$sp") base64EncodedCert=$(get_mock_msi_cert "$sp") +mockObjectID=$(get_mock_msi_objectID "$mockClientID") setup_platform_identity cluster_msi_role_assignment "${mockClientID}" # Print the extracted values echo "Cluster MSI Client ID: $mockClientID" +echo "Cluster MSI Object ID: $mockObjectID" echo "Cluster MSI Tenant ID: $mockTenantID" echo "Cluster MSI Base64 Encoded Certificate: $base64EncodedCert" echo "Platform workload identity role sets: $PLATFORM_WORKLOAD_IDENTITY_ROLE_SETS" \ No newline at end of file diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index eb0d166c482..7c1716ffc9d 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -225,7 +225,7 @@ func New(ctx context.Context, log *logrus.Entry, _env env.Interface, db database return nil, err } - armRoleDefinitionsClient, err := armauthorization.NewArmRoleDefinitionsClient(fpCredClusterTenant, clientOptions) + armRoleDefinitionsClient, err := armauthorization.NewArmRoleDefinitionsClient(fpCredClusterTenant, r.SubscriptionID, clientOptions) if err != nil { return nil, err } diff --git a/pkg/cluster/deploybaseresources.go b/pkg/cluster/deploybaseresources.go index 831d6cdf95b..7015ef21e2e 100644 --- a/pkg/cluster/deploybaseresources.go +++ b/pkg/cluster/deploybaseresources.go @@ -195,11 +195,16 @@ func (m *manager) deployBaseResourceTemplate(ctx context.Context) error { m.storageAccount(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, azureRegion, ocpSubnets, true), m.storageAccountBlobContainer(m.doc.OpenShiftCluster.Properties.ImageRegistryStorageAccountName, "image-registry"), m.clusterNSG(infraID, azureRegion), - m.clusterServicePrincipalRBAC(), m.networkPrivateLinkService(azureRegion), m.networkInternalLoadBalancer(azureRegion), } + if !m.doc.OpenShiftCluster.UsesWorkloadIdentity() { + resources = append(resources, + m.clusterServicePrincipalRBAC(), + ) + } + // Create a public load balancer routing if needed if m.doc.OpenShiftCluster.Properties.NetworkProfile.OutboundType == api.OutboundTypeLoadbalancer { m.newPublicLoadBalancer(ctx, &resources) diff --git a/pkg/env/prod.go b/pkg/env/prod.go index e46e454c949..72877d0af20 100644 --- a/pkg/env/prod.go +++ b/pkg/env/prod.go @@ -406,6 +406,7 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy if p.FeatureIsSet(FeatureUseMockMsiRp) { keysToValidate := []string{ "MOCK_MSI_CLIENT_ID", + "MOCK_MSI_OBJECT_ID", "MOCK_MSI_CERT", "MOCK_MSI_TENANT_ID", } @@ -425,6 +426,7 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy ClientID: ptr.To(os.Getenv("MOCK_MSI_CLIENT_ID")), ClientSecret: ptr.To(os.Getenv("MOCK_MSI_CERT")), TenantID: ptr.To(os.Getenv("MOCK_MSI_TENANT_ID")), + ObjectID: ptr.To(os.Getenv("MOCK_MSI_OBJECT_ID")), ResourceID: ptr.To(msiResourceId.String()), AuthenticationEndpoint: ptr.To(p.Environment().Cloud.ActiveDirectoryAuthorityHost), CannotRenewAfter: &placeholder, @@ -437,7 +439,6 @@ func (p *prod) MsiDataplaneClientOptions(msiResourceId *arm.ResourceID) (*policy XMSAzNwperimid: []*string{&placeholder}, XMSAzTm: &placeholder, }, - ObjectID: &placeholder, }, }, }, diff --git a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go index a5251e76ef1..727500cd376 100644 --- a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go +++ b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go @@ -5,6 +5,7 @@ package armauthorization import ( "context" + "fmt" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm" @@ -17,13 +18,20 @@ type RoleDefinitionsClient interface { type ArmRoleDefinitionsClient struct { *armauthorization.RoleDefinitionsClient + subscriptionID string } var _ RoleDefinitionsClient = &ArmRoleDefinitionsClient{} -func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, options *arm.ClientOptions) (*ArmRoleDefinitionsClient, error) { +func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, subscriptionID string, options *arm.ClientOptions) (*ArmRoleDefinitionsClient, error) { client, err := armauthorization.NewRoleDefinitionsClient(credential, options) return &ArmRoleDefinitionsClient{ RoleDefinitionsClient: client, + subscriptionID: subscriptionID, }, err } + +func (client ArmRoleDefinitionsClient) GetByID(ctx context.Context, roleDefinitionID string, options *armauthorization.RoleDefinitionsClientGetByIDOptions) (armauthorization.RoleDefinitionsClientGetByIDResponse, error) { + roleID := fmt.Sprintf("/subscriptions/%s/providers/Microsoft.Authorization/roleDefinitions/%s", client.subscriptionID, roleDefinitionID) + return client.RoleDefinitionsClient.GetByID(ctx, roleID, options) +} diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile.go b/pkg/validate/dynamic/platformworkloadidentityprofile.go index c1674e6b3ac..f5b17adf737 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile.go @@ -11,6 +11,7 @@ import ( "github.com/Azure/ARO-RP/pkg/api" "github.com/Azure/ARO-RP/pkg/util/azureclient/azuresdk/armauthorization" "github.com/Azure/ARO-RP/pkg/util/rbac" + "github.com/Azure/ARO-RP/pkg/util/stringutils" "github.com/Azure/ARO-RP/pkg/util/version" ) @@ -63,7 +64,8 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile(ctx context.Context, } for _, role := range platformWorkloadIdentityRolesByRoleName { - actions, err := getActionsForRoleDefinition(ctx, role.RoleDefinitionID, roleDefinitions) + roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') + actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) if err != nil { return err }