From 3e2f1dc390a3545aaaa66a5ecbcc28184bd0e601 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Mon, 7 Oct 2024 21:46:14 -0400 Subject: [PATCH 1/5] Fix Dynamic Validation Flow for Workload Identity --- Makefile | 1 + docs/deploy-development-rp.md | 1 + env.example | 1 + hack/devtools/local_dev_env.sh | 6 + pkg/cluster/cluster.go | 2 +- pkg/cluster/deploybaseresources.go | 7 +- pkg/env/prod.go | 3 +- pkg/util/azblob2/manager.go | 127 ++++++++++++++++++ .../armauthorization/roledefinitions.go | 10 +- .../platformworkloadidentityprofile.go | 2 +- .../platformworkloadidentityprofile_test.go | 16 +-- 11 files changed, 163 insertions(+), 13 deletions(-) create mode 100644 pkg/util/azblob2/manager.go 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/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..41861943fc6 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_clientID() { + 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 "$mockMSI") 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/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/azblob2/manager.go b/pkg/util/azblob2/manager.go new file mode 100644 index 00000000000..2880e0bd133 --- /dev/null +++ b/pkg/util/azblob2/manager.go @@ -0,0 +1,127 @@ +package azblob2 + +// Copyright (c) Microsoft Corporation. +// Licensed under the Apache License 2.0. + +import ( + "context" + "io" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/sas" + "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/service" + "github.com/sirupsen/logrus" +) + +type AZBlobClient interface { + DownloadStream(ctx context.Context, containerName string, blobName string, o *azblob.DownloadStreamOptions) ([]byte, error) + UploadBuffer(ctx context.Context, containerName string, blobName string, buffer []byte) error + DeleteBlob(ctx context.Context, containerName string, blobName string) error + Exists(ctx context.Context, container string, blobPath string) (bool, error) +} + +type azBlobClient struct { + client *azblob.Client +} + +func NewAZBlobClient(ctx context.Context, blobContainerURL string, credential azcore.TokenCredential, options *azblob.ClientOptions, isUserDesignated bool, log *logrus.Entry) (AZBlobClient, error) { + client, err := azblob.NewClient(blobContainerURL, credential, options) + if err != nil { + return nil, err + } + blobClient := &azBlobClient{client: client} + if isUserDesignated { + sasURL, err := blobClient.signBlobURL(ctx, blobContainerURL, time.Now().UTC().Add(2*time.Hour)) + log.Printf("sasURL -------- %s", sasURL) + if err != nil { + return nil, err + } + + client, err = azblob.NewClientWithNoCredential(sasURL, options) + if err != nil { + return nil, err + } + blobClient = &azBlobClient{client: client} + } + return blobClient, nil +} + +func (azBlobClient *azBlobClient) DownloadStream(ctx context.Context, containerName string, blobName string, o *azblob.DownloadStreamOptions) ([]byte, error) { + response, err := azBlobClient.client.DownloadStream(ctx, containerName, blobName, o) + if err != nil { + return nil, err + } + defer response.Body.Close() + return io.ReadAll(response.Body) +} + +func (azBlobClient *azBlobClient) UploadBuffer(ctx context.Context, containerName string, blobName string, buffer []byte) error { + _, err := azBlobClient.client.UploadBuffer(ctx, containerName, blobName, buffer, &azblob.UploadBufferOptions{}) + return err +} + +func (azBlobClient *azBlobClient) DeleteBlob(ctx context.Context, containerName string, blobName string) error { + _, err := azBlobClient.client.DeleteBlob(ctx, containerName, blobName, &azblob.DeleteBlobOptions{}) + return err +} + +func (azBlobClient *azBlobClient) signBlobURL(ctx context.Context, blobURL string, expires time.Time) (string, error) { + urlParts, err := sas.ParseURL(blobURL) + if err != nil { + return "", err + } + // perms := sas.BlobPermissions{Read: true, Write: true, Create: true} + perms := sas.BlobPermissions{Read: true, Write: true, Create: true} + signatureValues := sas.BlobSignatureValues{ + Protocol: sas.ProtocolHTTPS, + StartTime: time.Now().UTC().Add(-10 * time.Second), + ExpiryTime: expires, + Permissions: perms.String(), + ContainerName: "aro", + BlobName: "graph", + } + urlParts.SAS, err = azBlobClient.sign(ctx, &signatureValues) + if err != nil { + return "", err + } + return urlParts.String(), nil +} + +func (azBlobClient *azBlobClient) sign(ctx context.Context, signatureValues *sas.BlobSignatureValues) (sas.QueryParameters, error) { + currentTime := time.Now().UTC().Add(-10 * time.Second) + expiryTime := currentTime.Add(2 * time.Hour) + + info := service.KeyInfo{ + Start: to.Ptr(currentTime.UTC().Format(sas.TimeFormat)), + Expiry: to.Ptr(expiryTime.UTC().Format(sas.TimeFormat)), + } + + udc, err := azBlobClient.client.ServiceClient().GetUserDelegationCredential(ctx, info, nil) + if err != nil { + return sas.QueryParameters{}, err + } + return signatureValues.SignWithUserDelegation(udc) +} + +func (azBlobClient *azBlobClient) Exists(ctx context.Context, container string, blobPath string) (bool, error) { + blobRef := azBlobClient.client.ServiceClient().NewContainerClient(container).NewBlobClient(blobPath) + _, err := blobRef.GetProperties(ctx, nil) + if err != nil { + if bloberror.HasCode( + err, + bloberror.BlobNotFound, + bloberror.ContainerNotFound, + bloberror.ResourceNotFound, + bloberror.CannotVerifyCopySource, + ) { + return false, nil + } else { + return false, err + } + } + return true, nil +} diff --git a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go index a5251e76ef1..531c1d14d20 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%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..8a2985739ca 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile.go @@ -89,7 +89,7 @@ func (dv *dynamic) validateClusterMSI(ctx context.Context, oc *api.OpenShiftClus // Validate that the cluster MSI has all permissions specified in AzureRedHatOpenShiftFederatedCredentialRole over each platform managed identity func (dv *dynamic) validateClusterMSIPermissions(ctx context.Context, oid string, platformIdentities []api.PlatformWorkloadIdentity, roleDefinitions armauthorization.RoleDefinitionsClient) error { - actions, err := getActionsForRoleDefinition(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, roleDefinitions) + actions, err := getActionsForRoleDefinition(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), roleDefinitions) if err != nil { return err } diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go index 6534221fd2a..a33d43486db 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go @@ -201,7 +201,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -232,7 +232,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -267,7 +267,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -431,7 +431,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) }, wantErr: "Generic Error", }, @@ -458,7 +458,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) }, wantErr: "parsing failed for Invalid UUID. Invalid resource Id format", }, @@ -479,7 +479,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -508,7 +508,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -537,7 +537,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, errors.New("Generic Error")) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { From 5d630e0d54568b243fc8cfd07bc357f9df02c595 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Tue, 8 Oct 2024 11:04:27 -0400 Subject: [PATCH 2/5] fix the mock msi object id export --- hack/devtools/local_dev_env.sh | 4 ++-- hack/devtools/msi.sh | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/hack/devtools/local_dev_env.sh b/hack/devtools/local_dev_env.sh index 41861943fc6..42206588d3a 100755 --- a/hack/devtools/local_dev_env.sh +++ b/hack/devtools/local_dev_env.sh @@ -121,7 +121,7 @@ get_mock_msi_tenantID() { echo "$1" | jq -r .tenant } -get_mock_msi_clientID() { +get_mock_msi_objectID() { az ad sp list --all --filter "appId eq '$1'" | jq -r ".[] | .id" } @@ -238,7 +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 "$mockMSI") + mockObjectID=$(get_mock_msi_objectID "$mockClientID") setup_platform_identity cluster_msi_role_assignment "${mockClientID}" 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 From b19e34f8cdd781db263aca93db58e8a48a905dc7 Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Tue, 8 Oct 2024 11:36:57 -0400 Subject: [PATCH 3/5] Remove unwanted code --- hack/devtools/local_dev_env.sh | 2 +- pkg/util/azblob2/manager.go | 127 --------------------------------- 2 files changed, 1 insertion(+), 128 deletions(-) delete mode 100644 pkg/util/azblob2/manager.go diff --git a/hack/devtools/local_dev_env.sh b/hack/devtools/local_dev_env.sh index 42206588d3a..7f3818bbd72 100755 --- a/hack/devtools/local_dev_env.sh +++ b/hack/devtools/local_dev_env.sh @@ -248,7 +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_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/pkg/util/azblob2/manager.go b/pkg/util/azblob2/manager.go deleted file mode 100644 index 2880e0bd133..00000000000 --- a/pkg/util/azblob2/manager.go +++ /dev/null @@ -1,127 +0,0 @@ -package azblob2 - -// Copyright (c) Microsoft Corporation. -// Licensed under the Apache License 2.0. - -import ( - "context" - "io" - "time" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/to" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/bloberror" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/sas" - "github.com/Azure/azure-sdk-for-go/sdk/storage/azblob/service" - "github.com/sirupsen/logrus" -) - -type AZBlobClient interface { - DownloadStream(ctx context.Context, containerName string, blobName string, o *azblob.DownloadStreamOptions) ([]byte, error) - UploadBuffer(ctx context.Context, containerName string, blobName string, buffer []byte) error - DeleteBlob(ctx context.Context, containerName string, blobName string) error - Exists(ctx context.Context, container string, blobPath string) (bool, error) -} - -type azBlobClient struct { - client *azblob.Client -} - -func NewAZBlobClient(ctx context.Context, blobContainerURL string, credential azcore.TokenCredential, options *azblob.ClientOptions, isUserDesignated bool, log *logrus.Entry) (AZBlobClient, error) { - client, err := azblob.NewClient(blobContainerURL, credential, options) - if err != nil { - return nil, err - } - blobClient := &azBlobClient{client: client} - if isUserDesignated { - sasURL, err := blobClient.signBlobURL(ctx, blobContainerURL, time.Now().UTC().Add(2*time.Hour)) - log.Printf("sasURL -------- %s", sasURL) - if err != nil { - return nil, err - } - - client, err = azblob.NewClientWithNoCredential(sasURL, options) - if err != nil { - return nil, err - } - blobClient = &azBlobClient{client: client} - } - return blobClient, nil -} - -func (azBlobClient *azBlobClient) DownloadStream(ctx context.Context, containerName string, blobName string, o *azblob.DownloadStreamOptions) ([]byte, error) { - response, err := azBlobClient.client.DownloadStream(ctx, containerName, blobName, o) - if err != nil { - return nil, err - } - defer response.Body.Close() - return io.ReadAll(response.Body) -} - -func (azBlobClient *azBlobClient) UploadBuffer(ctx context.Context, containerName string, blobName string, buffer []byte) error { - _, err := azBlobClient.client.UploadBuffer(ctx, containerName, blobName, buffer, &azblob.UploadBufferOptions{}) - return err -} - -func (azBlobClient *azBlobClient) DeleteBlob(ctx context.Context, containerName string, blobName string) error { - _, err := azBlobClient.client.DeleteBlob(ctx, containerName, blobName, &azblob.DeleteBlobOptions{}) - return err -} - -func (azBlobClient *azBlobClient) signBlobURL(ctx context.Context, blobURL string, expires time.Time) (string, error) { - urlParts, err := sas.ParseURL(blobURL) - if err != nil { - return "", err - } - // perms := sas.BlobPermissions{Read: true, Write: true, Create: true} - perms := sas.BlobPermissions{Read: true, Write: true, Create: true} - signatureValues := sas.BlobSignatureValues{ - Protocol: sas.ProtocolHTTPS, - StartTime: time.Now().UTC().Add(-10 * time.Second), - ExpiryTime: expires, - Permissions: perms.String(), - ContainerName: "aro", - BlobName: "graph", - } - urlParts.SAS, err = azBlobClient.sign(ctx, &signatureValues) - if err != nil { - return "", err - } - return urlParts.String(), nil -} - -func (azBlobClient *azBlobClient) sign(ctx context.Context, signatureValues *sas.BlobSignatureValues) (sas.QueryParameters, error) { - currentTime := time.Now().UTC().Add(-10 * time.Second) - expiryTime := currentTime.Add(2 * time.Hour) - - info := service.KeyInfo{ - Start: to.Ptr(currentTime.UTC().Format(sas.TimeFormat)), - Expiry: to.Ptr(expiryTime.UTC().Format(sas.TimeFormat)), - } - - udc, err := azBlobClient.client.ServiceClient().GetUserDelegationCredential(ctx, info, nil) - if err != nil { - return sas.QueryParameters{}, err - } - return signatureValues.SignWithUserDelegation(udc) -} - -func (azBlobClient *azBlobClient) Exists(ctx context.Context, container string, blobPath string) (bool, error) { - blobRef := azBlobClient.client.ServiceClient().NewContainerClient(container).NewBlobClient(blobPath) - _, err := blobRef.GetProperties(ctx, nil) - if err != nil { - if bloberror.HasCode( - err, - bloberror.BlobNotFound, - bloberror.ContainerNotFound, - bloberror.ResourceNotFound, - bloberror.CannotVerifyCopySource, - ) { - return false, nil - } else { - return false, err - } - } - return true, nil -} From b2aa10816dc4353e5fbf9b10da5b31f154705b6f Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Tue, 8 Oct 2024 12:27:29 -0400 Subject: [PATCH 4/5] update OIDC_STORAGE_ACCOUNT_NAME local env setup --- docs/prepare-a-shared-rp-development-environment.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ``` From 1febcdd964e2a9fef376d9698a5d6c588f45e25d Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 9 Oct 2024 21:28:10 -0400 Subject: [PATCH 5/5] ARO-11049 resolve comments --- .../azuresdk/armauthorization/roledefinitions.go | 2 +- .../dynamic/platformworkloadidentityprofile.go | 6 ++++-- .../platformworkloadidentityprofile_test.go | 16 ++++++++-------- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go index 531c1d14d20..727500cd376 100644 --- a/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go +++ b/pkg/util/azureclient/azuresdk/armauthorization/roledefinitions.go @@ -32,6 +32,6 @@ func NewArmRoleDefinitionsClient(credential azcore.TokenCredential, subscription } func (client ArmRoleDefinitionsClient) GetByID(ctx context.Context, roleDefinitionID string, options *armauthorization.RoleDefinitionsClientGetByIDOptions) (armauthorization.RoleDefinitionsClientGetByIDResponse, error) { - roleID := fmt.Sprintf("/subscriptions/%s%s", client.subscriptionID, roleDefinitionID) + 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 8a2985739ca..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 } @@ -89,7 +91,7 @@ func (dv *dynamic) validateClusterMSI(ctx context.Context, oc *api.OpenShiftClus // Validate that the cluster MSI has all permissions specified in AzureRedHatOpenShiftFederatedCredentialRole over each platform managed identity func (dv *dynamic) validateClusterMSIPermissions(ctx context.Context, oid string, platformIdentities []api.PlatformWorkloadIdentity, roleDefinitions armauthorization.RoleDefinitionsClient) error { - actions, err := getActionsForRoleDefinition(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), roleDefinitions) + actions, err := getActionsForRoleDefinition(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, roleDefinitions) if err != nil { return err } diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go index a33d43486db..6534221fd2a 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go @@ -201,7 +201,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -232,7 +232,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -267,7 +267,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -431,7 +431,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, errors.New("Generic Error")) }, wantErr: "Generic Error", }, @@ -458,7 +458,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) }, wantErr: "parsing failed for Invalid UUID. Invalid resource Id format", }, @@ -479,7 +479,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -508,7 +508,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) { @@ -537,7 +537,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient) { - roleDefinitions.EXPECT().GetByID(ctx, fmt.Sprintf("/providers/Microsoft.Authorization/roleDefinitions/%s", rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) + roleDefinitions.EXPECT().GetByID(ctx, rbac.RoleAzureRedHatOpenShiftFederatedCredentialRole, &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).Return(msiRequiredPermissions, nil) roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, errors.New("Generic Error")) }, checkAccessMocks: func(cancel context.CancelFunc, pdpClient *mock_remotepdp.MockRemotePDPClient, tokenCred *mock_azcore.MockTokenCredential) {