From faf2fc5fa387c1461bd36f98501e5b91b2b9d1cc Mon Sep 17 00:00:00 2001 From: Erica Hinkle Date: Tue, 1 Sep 2026 11:24:57 -0400 Subject: [PATCH] OCPBUGS-17664: Improve error messages for SCP-denied IAM operations When CCO in mint mode calls iam:PutUserPolicy and it is blocked by an AWS Service Control Policy (SCP), the error message now includes the specific IAM operation that was denied, the desired policy document, and guidance for manual remediation. This allows cluster administrators to see exactly what policy change CCO was attempting and apply it manually. Non-AccessDenied errors also now include the IAM user and policy name for improved diagnosability. The policy-bearing error is not logged directly to avoid exposing customer data; the detailed error propagates to the CredentialsRequest status condition instead. Co-Authored-By: Claude Opus 4.6 --- pkg/aws/actuator/actuator.go | 23 ++++++++- pkg/aws/actuator/actuator_test.go | 42 +++++++++++++++ .../credentialsrequest_controller_test.go | 51 +++++++++++++++++++ 3 files changed, 115 insertions(+), 1 deletion(-) diff --git a/pkg/aws/actuator/actuator.go b/pkg/aws/actuator/actuator.go index e80f9ffba0..2ab937f1b0 100644 --- a/pkg/aws/actuator/actuator.go +++ b/pkg/aws/actuator/actuator.go @@ -29,6 +29,7 @@ import ( awssdk "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/iam" iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types" + smithy "github.com/aws/smithy-go" configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" @@ -653,6 +654,10 @@ func (a *AWSActuator) syncMint(ctx context.Context, cr *minterv1.CredentialsRequ } err = a.setUserPolicy(ctx, rootAWSClient, awsStatus.User, awsStatus.Policy, desiredUserPolicy) if err != nil { + logger.WithFields(log.Fields{ + "userName": awsStatus.User, + "policyName": awsStatus.Policy, + }).Error("failed to set user policy") return err } logger.Info("successfully set user policy") @@ -1186,7 +1191,15 @@ func (a *AWSActuator) setUserPolicy(ctx context.Context, awsClient ccaws.Client, PolicyName: awssdk.String(policyName), }) if err != nil { - return fmt.Errorf("unknown error setting user policy in AWS: %v", err) + if isAccessDenied(err) { + return fmt.Errorf("access denied setting IAM user policy (user: %s, policy: %s): "+ + "this is typically caused by an AWS Service Control Policy (SCP) blocking iam:PutUserPolicy. "+ + "To resolve, manually apply the following policy document to the IAM user, or adjust the SCP to allow iam:PutUserPolicy. "+ + "Desired policy document: %s Original error: %v", + userName, policyName, userPolicy, err) + } + return fmt.Errorf("error setting user policy in AWS (user: %s, policy: %s): %v", + userName, policyName, err) } return nil @@ -1337,6 +1350,14 @@ func (a *AWSActuator) loadClusterUUID(logger log.FieldLogger) (configv1.ClusterI return clusterVer.Spec.ClusterID, nil } +func isAccessDenied(err error) bool { + var apiErr smithy.APIError + if errors.As(err, &apiErr) { + return apiErr.ErrorCode() == "AccessDenied" + } + return false +} + func isAWSCredentials(providerSpec *runtime.RawExtension) (bool, error) { unknown := runtime.Unknown{} err := minterv1.Codec.DecodeProviderSpec(providerSpec, &unknown) diff --git a/pkg/aws/actuator/actuator_test.go b/pkg/aws/actuator/actuator_test.go index d026ebcfb8..d30bcf4208 100644 --- a/pkg/aws/actuator/actuator_test.go +++ b/pkg/aws/actuator/actuator_test.go @@ -27,6 +27,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + smithy "github.com/aws/smithy-go" + configv1 "github.com/openshift/api/config/v1" operatorv1 "github.com/openshift/api/operator/v1" @@ -717,3 +719,43 @@ func testAuthentication(issuer string) *configv1.Authentication { } return conf } + +func TestIsAccessDenied(t *testing.T) { + tests := []struct { + name string + err error + expected bool + }{ + { + name: "AccessDenied API error", + err: &smithy.GenericAPIError{Code: "AccessDenied", Message: "blocked by SCP"}, + expected: true, + }, + { + name: "wrapped AccessDenied", + err: fmt.Errorf("outer: %w", &smithy.GenericAPIError{Code: "AccessDenied", Message: "blocked"}), + expected: true, + }, + { + name: "different error code", + err: &smithy.GenericAPIError{Code: "InvalidInput", Message: "bad input"}, + expected: false, + }, + { + name: "generic error", + err: fmt.Errorf("some other error"), + expected: false, + }, + { + name: "nil error", + err: nil, + expected: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := isAccessDenied(tt.err) + assert.Equal(t, tt.expected, got) + }) + } +} diff --git a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go index 06b60cddae..6a363916a5 100644 --- a/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go +++ b/pkg/operator/credentialsrequest/credentialsrequest_controller_test.go @@ -903,6 +903,47 @@ func TestCredentialsRequestReconcile(t *testing.T) { }, }, }, + { + name: "SCP denied PutUserPolicy", + existing: []runtime.Object{ + testOperatorConfig(""), + testInfrastructure(testInfraName), + createTestNamespace(testNamespace), + createTestNamespace(testSecretNamespace), + testCredentialsRequest(t), + testAWSCredsSecret("openshift-cloud-credential-operator", "cloud-credential-operator-iam-ro-creds", testReadAWSAccessKeyID, testReadAWSSecretAccessKey), + testClusterVersion(), + }, + existingAdmin: []runtime.Object{ + testAWSCredsSecret("kube-system", "aws-creds", testRootAWSAccessKeyID, testRootAWSSecretAccessKey), + }, + mockRootAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUser(mockAWSClient) + mockCreateUser(mockAWSClient) + mockTagUser(mockAWSClient) + mockPutUserPolicyAccessDenied(mockAWSClient) + return mockAWSClient + }, + mockReadAWSClient: func(mockCtrl *gomock.Controller) *mockaws.MockClient { + mockAWSClient := mockaws.NewMockClient(mockCtrl) + mockGetUserNotFound(mockAWSClient) + mockGetUserPolicyMissing(mockAWSClient) + return mockAWSClient + }, + validate: func(c client.Client, t *testing.T) { + cr := getCR(c) + assert.False(t, cr.Status.Provisioned) + }, + expectErr: true, + expectedConditions: []ExpectedCondition{ + { + conditionType: minterv1.CredentialsProvisionFailure, + reason: "CredentialsProvisionFailure", + status: corev1.ConditionTrue, + }, + }, + }, { name: "cred deletion failure condition", existing: []runtime.Object{ @@ -1962,6 +2003,16 @@ func mockPutUserPolicy(mockAWSClient *mockaws.MockClient) { mockAWSClient.EXPECT().PutUserPolicy(gomock.Any(), gomock.Any()).Return(&iam.PutUserPolicyOutput{}, nil) } +func mockPutUserPolicyAccessDenied(mockAWSClient *mockaws.MockClient) { + mockAWSClient.EXPECT().PutUserPolicy(gomock.Any(), gomock.Any()).Return( + nil, + &smithy.GenericAPIError{ + Code: "AccessDenied", + Message: "User: arn:aws:iam::123456789:user/root is not authorized to perform: iam:PutUserPolicy with an explicit deny in a service control policy", + }, + ) +} + func mockGetUserPolicy(mockAWSClient *mockaws.MockClient, policyDoc string) { policyDoc = url.QueryEscape(policyDoc) mockAWSClient.EXPECT().GetUserPolicy(gomock.Any(), gomock.Any()).Return(&iam.GetUserPolicyOutput{