From 305400e952806ace2e2a09d56a2775aebeadd36e Mon Sep 17 00:00:00 2001 From: Rajdeep Singh Chauhan Date: Wed, 20 Nov 2024 11:15:45 -0500 Subject: [PATCH] ARO-10859 update dynamic validation to reject the create/update flow for unexpected platform workload identity --- .../platformworkloadidentityprofile.go | 125 +++++++----------- .../platformworkloadidentityprofile_test.go | 20 ++- 2 files changed, 63 insertions(+), 82 deletions(-) diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile.go b/pkg/validate/dynamic/platformworkloadidentityprofile.go index 50bf1d36ae8..1cfecf36089 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile.go @@ -12,9 +12,9 @@ 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/azureclient/azuresdk/armmsi" + "github.com/Azure/ARO-RP/pkg/util/platformworkloadidentity" "github.com/Azure/ARO-RP/pkg/util/rbac" "github.com/Azure/ARO-RP/pkg/util/stringutils" - "github.com/Azure/ARO-RP/pkg/util/version" ) // Copyright (c) Microsoft Corporation. @@ -34,93 +34,68 @@ func (dv *dynamic) ValidatePlatformWorkloadIdentityProfile( dv.log.Print("ValidatePlatformWorkloadIdentityProfile") dv.platformIdentitiesActionsMap = map[string][]string{} - dv.platformIdentities = map[string]api.PlatformWorkloadIdentity{} - - for k, pwi := range oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities { - _, ok := platformWorkloadIdentityRolesByRoleName[k] - if ok { - dv.platformIdentitiesActionsMap[k] = nil - dv.platformIdentities[k] = pwi - - identityResourceId, err := azure.ParseResourceID(pwi.ResourceID) - if err != nil { - return err - } - - // validate federated identity credentials - federatedCredentials, err := clusterMsiFederatedIdentityCredentials.List(ctx, identityResourceId.ResourceGroup, identityResourceId.ResourceName, &sdkmsi.FederatedIdentityCredentialsClientListOptions{}) - if err != nil { - return err - } - - for _, federatedCredential := range federatedCredentials { - switch { - case federatedCredential == nil, - federatedCredential.Name == nil, - federatedCredential.Properties == nil: - return fmt.Errorf("received invalid federated credential") - case oc.Properties.ProvisioningState == api.ProvisioningStateCreating: - return api.NewCloudError( - http.StatusBadRequest, - api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, - fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), - "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure this identity is only used for this cluster and does not have any existing federated identity credentials.", - *federatedCredential.Name, - pwi.ResourceID, - k, - ) - case len(federatedCredential.Properties.Audiences) != 1, - *federatedCredential.Properties.Audiences[0] != expectedAudience, - federatedCredential.Properties.Issuer == nil, - *federatedCredential.Properties.Issuer != string(*oc.Properties.ClusterProfile.OIDCIssuer): - return api.NewCloudError( - http.StatusBadRequest, - api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, - fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), - "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure only federated credentials provisioned by the ARO service for this cluster are present.", - *federatedCredential.Name, - pwi.ResourceID, - k, - ) - } - } - } - } + dv.platformIdentities = oc.Properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities // Check if any required platform identity is missing if len(dv.platformIdentities) != len(platformWorkloadIdentityRolesByRoleName) { - requiredOperatorIdentities := []string{} - for _, role := range platformWorkloadIdentityRolesByRoleName { - requiredOperatorIdentities = append(requiredOperatorIdentities, role.OperatorName) + return platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(oc, platformWorkloadIdentityRolesByRoleName) + } + + for k, pwi := range dv.platformIdentities { + role, exists := platformWorkloadIdentityRolesByRoleName[k] + if !exists { + return platformworkloadidentity.GetPlatformWorkloadIdentityMismatchError(oc, platformWorkloadIdentityRolesByRoleName) } - currentOpenShiftVersion, err := version.ParseVersion(oc.Properties.ClusterProfile.Version) + + roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') + actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) if err != nil { return err } - currentMinorVersion := currentOpenShiftVersion.MinorVersion() - v := currentMinorVersion - if oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo != nil { - upgradeableVersion, err := version.ParseVersion(string(*oc.Properties.PlatformWorkloadIdentityProfile.UpgradeableTo)) - if err != nil { - return err - } - upgradeableMinorVersion := upgradeableVersion.MinorVersion() - if currentMinorVersion != upgradeableMinorVersion && currentOpenShiftVersion.Lt(upgradeableVersion) { - v = fmt.Sprintf("%s or %s", v, upgradeableMinorVersion) - } + dv.platformIdentitiesActionsMap[role.OperatorName] = actions + + identityResourceId, err := azure.ParseResourceID(pwi.ResourceID) + if err != nil { + return err } - return api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodePlatformWorkloadIdentityMismatch, - "properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities", "There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '%v'", v, requiredOperatorIdentities) - } - for _, role := range platformWorkloadIdentityRolesByRoleName { - roleDefinitionID := stringutils.LastTokenByte(role.RoleDefinitionID, '/') - actions, err := getActionsForRoleDefinition(ctx, roleDefinitionID, roleDefinitions) + // validate federated identity credentials + federatedCredentials, err := clusterMsiFederatedIdentityCredentials.List(ctx, identityResourceId.ResourceGroup, identityResourceId.ResourceName, &sdkmsi.FederatedIdentityCredentialsClientListOptions{}) if err != nil { return err } - dv.platformIdentitiesActionsMap[role.OperatorName] = actions + for _, federatedCredential := range federatedCredentials { + switch { + case federatedCredential == nil, + federatedCredential.Name == nil, + federatedCredential.Properties == nil: + return fmt.Errorf("received invalid federated credential") + case oc.Properties.ProvisioningState == api.ProvisioningStateCreating: + return api.NewCloudError( + http.StatusBadRequest, + api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, + fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), + "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure this identity is only used for this cluster and does not have any existing federated identity credentials.", + *federatedCredential.Name, + pwi.ResourceID, + k, + ) + case len(federatedCredential.Properties.Audiences) != 1, + *federatedCredential.Properties.Audiences[0] != expectedAudience, + federatedCredential.Properties.Issuer == nil, + *federatedCredential.Properties.Issuer != string(*oc.Properties.ClusterProfile.OIDCIssuer): + return api.NewCloudError( + http.StatusBadRequest, + api.CloudErrorCodePlatformWorkloadIdentityContainsInvalidFederatedCredential, + fmt.Sprintf("properties.platformWorkloadIdentityProfile.platformWorkloadIdentities.%s.resourceId", k), + "Unexpected federated credential '%s' found on platform workload identity '%s' used for role '%s'. Please ensure only federated credentials provisioned by the ARO service for this cluster are present.", + *federatedCredential.Name, + pwi.ResourceID, + k, + ) + } + } } return nil diff --git a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go index 4a27e2c093c..6adf434a52c 100644 --- a/pkg/validate/dynamic/platformworkloadidentityprofile_test.go +++ b/pkg/validate/dynamic/platformworkloadidentityprofile_test.go @@ -263,9 +263,6 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { expectedPlatformIdentity1FederatedCredName := platformworkloadidentity.GetPlatformWorkloadIdentityFederatedCredName(clusterResourceId, platformIdentity1ResourceId, platformIdentity1SAName) expectedOIDCIssuer := "https://fakeissuer.fakedomain/fakecluster" platformWorkloadIdentities := map[string]api.PlatformWorkloadIdentity{ - "Dummy2": { - ResourceID: platformIdentity1, - }, "Dummy1": { ResourceID: platformIdentity1, }, @@ -532,6 +529,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return(nil, fmt.Errorf("something unexpected occurred")) }, @@ -560,6 +558,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -604,6 +603,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -648,6 +648,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -692,6 +693,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { }, }, mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) federatedIdentityCredentials.EXPECT().List(gomock.Any(), gomock.Eq(platformIdentity1ResourceId.ResourceGroup), gomock.Eq(platformIdentity1ResourceId.ResourceName), gomock.Any()). Return([]*sdkmsi.FederatedIdentityCredential{ { @@ -935,7 +937,7 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { wantErr: fmt.Sprintf("400: %s: properties.PlatformWorkloadIdentityProfile.PlatformWorkloadIdentities: There's a mismatch between the required and expected set of platform workload identities for the requested OpenShift minor version '%s'. The required platform workload identities are '[Dummy3]'", api.CloudErrorCodePlatformWorkloadIdentityMismatch, "4.14"), }, { - name: "Fail - Mismatch between desired and provided platform Identities - count mismatch 2", + name: "Fail - Mismatch between desired and provided platform Identities - count mismatch and operator missing", platformIdentityRoles: validRolesForVersion, oc: &api.OpenShiftCluster{ ID: clusterID, @@ -1013,19 +1015,23 @@ func TestValidatePlatformWorkloadIdentityProfile(t *testing.T) { Properties: api.OpenShiftClusterProperties{ PlatformWorkloadIdentityProfile: &api.PlatformWorkloadIdentityProfile{ PlatformWorkloadIdentities: map[string]api.PlatformWorkloadIdentity{ - "Dummy2": { - ResourceID: "Invalid UUID", - }, "Dummy1": { ResourceID: "Invalid UUID", }, }, }, + ClusterProfile: api.ClusterProfile{ + Version: openShiftVersion, + OIDCIssuer: pointerutils.ToPtr(api.OIDCIssuer(expectedOIDCIssuer)), + }, }, Identity: &api.ManagedServiceIdentity{ UserAssignedIdentities: clusterMSI, }, }, + mocks: func(roleDefinitions *mock_armauthorization.MockRoleDefinitionsClient, federatedIdentityCredentials *mock_armmsi.MockFederatedIdentityCredentialsClient) { + roleDefinitions.EXPECT().GetByID(ctx, gomock.Any(), &sdkauthorization.RoleDefinitionsClientGetByIDOptions{}).AnyTimes().Return(platformIdentityRequiredPermissions, nil) + }, wantErr: "parsing failed for Invalid UUID. Invalid resource Id format", }, {