From 124a6eaf0983f6191f61306c5134dccfa1f34101 Mon Sep 17 00:00:00 2001 From: BCarvalheira Date: Wed, 20 Dec 2023 15:14:09 -0800 Subject: [PATCH 1/4] Added FP Check for validation --- .../openshiftcluster_validatedynamic.go | 48 ++++++++----------- 1 file changed, 20 insertions(+), 28 deletions(-) diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go index ab1fd8d01f5..7c7c718a1c9 100644 --- a/pkg/validate/openshiftcluster_validatedynamic.go +++ b/pkg/validate/openshiftcluster_validatedynamic.go @@ -115,11 +115,21 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { }) } - var fpClientCred azcore.TokenCredential - var spClientCred azcore.TokenCredential var pdpClient remotepdp.RemotePDPClient spp := dv.oc.Properties.ServicePrincipalProfile + tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID + options := dv.env.Environment().ClientSecretCredentialOptions() + spClientCred, err := azidentity.NewClientSecretCredential( + tenantID, spp.ClientID, string(spp.ClientSecret), options) + if err != nil { + return err + } + fpClientCred, err := dv.env.FPNewClientCertificateCredential(tenantID) + if err != nil { + return err + } + useCheckAccess, err := dv.env.LiveConfig().UseCheckAccess(ctx) dv.log.Info("USE_CHECKACCESS: ", useCheckAccess) if err != nil { @@ -132,21 +142,6 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { ) { // TODO remove after successfully migrating to CheckAccess dv.log.Info("Using CheckAccess instead of ListPermissions") - var err error - fpClientCred, err = dv.env.FPNewClientCertificateCredential(dv.subscriptionDoc.Subscription.Properties.TenantID) - if err != nil { - return err - } - - spClientCred, err = azidentity.NewClientSecretCredential( - dv.subscriptionDoc.Subscription.Properties.TenantID, - spp.ClientID, - string(spp.ClientSecret), - nil, - ) - if err != nil { - return err - } aroEnv := dv.env.Environment() pdpClient = remotepdp.NewRemotePDPClient( @@ -156,20 +151,12 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { ) } - tenantID := dv.subscriptionDoc.Subscription.Properties.TenantID - options := dv.env.Environment().ClientSecretCredentialOptions() - spTokenCredential, err := azidentity.NewClientSecretCredential( - tenantID, spp.ClientID, string(spp.ClientSecret), options) - if err != nil { - return err - } - scopes := []string{dv.env.Environment().ResourceManagerScope} - err = ensureAccessTokenClaims(ctx, spTokenCredential, scopes) + err = ensureAccessTokenClaims(ctx, spClientCred, scopes) if err != nil { return err } - spAuthorizer := azidext.NewTokenCredentialAdapter(spTokenCredential, scopes) + spAuthorizer := azidext.NewTokenCredentialAdapter(spClientCred, scopes) spDynamic := dynamic.NewValidator( dv.log, @@ -184,7 +171,7 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { ) // SP validation - err = spDynamic.ValidateServicePrincipal(ctx, spTokenCredential) + err = spDynamic.ValidateServicePrincipal(ctx, spClientCred) if err != nil { return err } @@ -225,6 +212,11 @@ func (dv *openShiftClusterDynamicValidator) Dynamic(ctx context.Context) error { return err } + err = ensureAccessTokenClaims(ctx, fpClientCred, scopes) + if err != nil { + return err + } + // FP validation fpDynamic := dynamic.NewValidator( dv.log, From 7a378d2314b3a8d29e5acdaaa1317758e26bb9bb Mon Sep 17 00:00:00 2001 From: BCarvalheira Date: Thu, 28 Dec 2023 11:27:05 -0800 Subject: [PATCH 2/4] Cloud Err to surface level backend for SP issue --- pkg/backend/openshiftcluster.go | 6 ++++++ pkg/validate/openshiftcluster_validatedynamic.go | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/pkg/backend/openshiftcluster.go b/pkg/backend/openshiftcluster.go index 2d080e90f08..cee2e690779 100644 --- a/pkg/backend/openshiftcluster.go +++ b/pkg/backend/openshiftcluster.go @@ -6,6 +6,7 @@ package backend import ( "context" "fmt" + "net/http" "strings" "sync/atomic" "time" @@ -339,6 +340,11 @@ func (ocb *openShiftClusterBackend) asyncOperationResultLog(log *logrus.Entry, i return } + if strings.Contains(backendErr.Error(), "one of the claims 'puid' or 'altsecid' or 'oid' should be present") { + backendErr = api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, + "properties.servicePrincipleProfile", "The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'. Please make sure service principal is properly created in the tenant.") + } + _, ok := backendErr.(*api.CloudError) if ok { log = log.WithField("resultType", utillog.UserErrorResultType) diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go index 7c7c718a1c9..9cef2d2af88 100644 --- a/pkg/validate/openshiftcluster_validatedynamic.go +++ b/pkg/validate/openshiftcluster_validatedynamic.go @@ -96,7 +96,7 @@ func ensureAccessTokenClaims(ctx context.Context, spTokenCredential azcore.Token http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, "properties.servicePrincipleProfile", - "The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'.") + "The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'. Please make sure service principal is properly created in the tenant.") } // Dynamic validates an OpenShift cluster From 5afddcc719123cac291636cade71bbc45fbd1f6d Mon Sep 17 00:00:00 2001 From: BCarvalheira Date: Wed, 3 Jan 2024 14:59:04 -0800 Subject: [PATCH 3/4] Updated error message --- pkg/backend/openshiftcluster.go | 4 ++-- pkg/validate/openshiftcluster_validatedynamic.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/backend/openshiftcluster.go b/pkg/backend/openshiftcluster.go index cee2e690779..63a4d205c69 100644 --- a/pkg/backend/openshiftcluster.go +++ b/pkg/backend/openshiftcluster.go @@ -340,9 +340,9 @@ func (ocb *openShiftClusterBackend) asyncOperationResultLog(log *logrus.Entry, i return } - if strings.Contains(backendErr.Error(), "one of the claims 'puid' or 'altsecid' or 'oid' should be present") { + if strings.Contains(strings.ToLower(backendErr.Error()), "one of the claims 'puid' or 'altsecid' or 'oid' should be present") { backendErr = api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, - "properties.servicePrincipleProfile", "The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'. Please make sure service principal is properly created in the tenant.") + "properties.servicePrincipleProfile", "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.") } _, ok := backendErr.(*api.CloudError) diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go index 9cef2d2af88..db39693e8a9 100644 --- a/pkg/validate/openshiftcluster_validatedynamic.go +++ b/pkg/validate/openshiftcluster_validatedynamic.go @@ -96,7 +96,7 @@ func ensureAccessTokenClaims(ctx context.Context, spTokenCredential azcore.Token http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, "properties.servicePrincipleProfile", - "The provided service principal does not give an access token with at least one of the claims 'altsecid', 'oid' or 'puid'. Please make sure service principal is properly created in the tenant.") + "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.") } // Dynamic validates an OpenShift cluster From 7bd9afe86e9365c20769a387c7edd6793c1bcfa0 Mon Sep 17 00:00:00 2001 From: BCarvalheira Date: Thu, 11 Jan 2024 12:15:16 -0800 Subject: [PATCH 4/4] Spelling fix --- pkg/backend/openshiftcluster.go | 2 +- pkg/validate/openshiftcluster_validatedynamic.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/backend/openshiftcluster.go b/pkg/backend/openshiftcluster.go index 63a4d205c69..3d4750d8a07 100644 --- a/pkg/backend/openshiftcluster.go +++ b/pkg/backend/openshiftcluster.go @@ -342,7 +342,7 @@ func (ocb *openShiftClusterBackend) asyncOperationResultLog(log *logrus.Entry, i if strings.Contains(strings.ToLower(backendErr.Error()), "one of the claims 'puid' or 'altsecid' or 'oid' should be present") { backendErr = api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, - "properties.servicePrincipleProfile", "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.") + "properties.servicePrincipalProfile", "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.") } _, ok := backendErr.(*api.CloudError) diff --git a/pkg/validate/openshiftcluster_validatedynamic.go b/pkg/validate/openshiftcluster_validatedynamic.go index db39693e8a9..d5703cd6bee 100644 --- a/pkg/validate/openshiftcluster_validatedynamic.go +++ b/pkg/validate/openshiftcluster_validatedynamic.go @@ -95,7 +95,7 @@ func ensureAccessTokenClaims(ctx context.Context, spTokenCredential azcore.Token return api.NewCloudError( http.StatusBadRequest, api.CloudErrorCodeInvalidServicePrincipalClaims, - "properties.servicePrincipleProfile", + "properties.servicePrincipalProfile", "The Azure Red Hat Openshift resource provider service principal has been removed from your tenant. To restore, please unregister and then re-register the Azure Red Hat OpenShift resource provider.") }