fix(aws): handle empty resource policy in PushSecret for SecretsManager (#6145)

Co-authored-by: Maksim Ryzhukhin <mryzhukhin@rightandabove.com>
Co-authored-by: Gergely Bräutigam <gergely.brautigam@sap.com>

providers/v1/aws/secretsmanager/secretsmanager.go

@@ -866,6 +866,33 @@ func (sm *SecretsManager) resolveResourcePolicy(ctx context.Context, policyRef *
 	}
 }
 
+// unmarshalPolicyJSON parses a JSON policy string into a map.
+// Returns nil map for empty input, allowing comparison with a populated policy.
+func unmarshalPolicyJSON(policy string) (map[string]any, error) {
+	if policy == "" {
+		return nil, nil
+	}
+	var m map[string]any
+	if err := json.Unmarshal([]byte(policy), &m); err != nil {
+		return nil, err
+	}
+	return m, nil
+}
+
+func (sm *SecretsManager) deleteResourcePolicy(ctx context.Context, secretID *string) error {
+	deletePolicyInput := &awssm.DeleteResourcePolicyInput{
+		SecretId: secretID,
+	}
+	_, err := sm.client.DeleteResourcePolicy(ctx, deletePolicyInput)
+	metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMDeleteResourcePolicy, err)
+
+	var nf *types.ResourceNotFoundException
+	if err != nil && !errors.As(err, &nf) {
+		return fmt.Errorf("failed to delete resource policy: %w", err)
+	}
+	return nil
+}
+
 // manageResourcePolicy applies or removes the resource policy based on metadata.
 func (sm *SecretsManager) manageResourcePolicy(ctx context.Context, metadata *apiextensionsv1.JSON, secretID *string) error {
 	meta, err := sm.constructMetadataWithDefaults(metadata)
@@ -875,18 +902,7 @@ func (sm *SecretsManager) manageResourcePolicy(ctx context.Context, metadata *ap
 
 	// Delete policy if policyRef is nil and the policy exists.
 	if meta.Spec.ResourcePolicy == nil {
-		deletePolicyInput := &awssm.DeleteResourcePolicyInput{
-			SecretId: secretID,
-		}
-		_, err = sm.client.DeleteResourcePolicy(ctx, deletePolicyInput)
-		metrics.ObserveAPICall(constants.ProviderAWSSM, constants.CallAWSSMDeleteResourcePolicy, err)
-
-		var nf *types.ResourceNotFoundException
-		if err != nil && !errors.As(err, &nf) {
-			return fmt.Errorf("failed to delete resource policy: %w", err)
-		}
-
-		return nil
+		return sm.deleteResourcePolicy(ctx, secretID)
 	}
 
 	// Normal flow, is to create the policy.
@@ -894,6 +910,9 @@ func (sm *SecretsManager) manageResourcePolicy(ctx context.Context, metadata *ap
 	if err != nil {
 		return fmt.Errorf("failed to resolve resource policy: %w", err)
 	}
+	if policyJSON == "" {
+		return sm.deleteResourcePolicy(ctx, secretID)
+	}
 
 	getCurrentPolicyInput := &awssm.GetResourcePolicyInput{
 		SecretId: secretID,
@@ -911,20 +930,17 @@ func (sm *SecretsManager) manageResourcePolicy(ctx context.Context, metadata *ap
 		currentPolicy = *currentPolicyOutput.ResourcePolicy
 	}
 
-	// convert to maps so we can do a stable comparison.
-	var (
-		currentPolicyMap map[string]any
-		policyJSONMaps   map[string]any
-	)
-
-	if err := json.Unmarshal([]byte(currentPolicy), &currentPolicyMap); err != nil {
+	currentPolicyMap, err := unmarshalPolicyJSON(currentPolicy)
+	if err != nil {
 		return fmt.Errorf("failed to unmarshal current resource policy: %w", err)
 	}
-	if err := json.Unmarshal([]byte(policyJSON), &policyJSONMaps); err != nil {
-		return fmt.Errorf("failed to unmarshal current resource policy: %w", err)
+
+	desiredPolicyMap, err := unmarshalPolicyJSON(policyJSON)
+	if err != nil {
+		return fmt.Errorf("failed to unmarshal desired resource policy: %w", err)
 	}
 
-	if reflect.DeepEqual(currentPolicyMap, policyJSONMaps) {
+	if reflect.DeepEqual(currentPolicyMap, desiredPolicyMap) {
 		return nil
 	}
 

providers/v1/aws/secretsmanager/secretsmanager_test.go

@@ -1054,6 +1054,54 @@ func TestSetSecret(t *testing.T) {
 				err: nil,
 			},
 		},
+		"SetSecretWithEmptyExistingResourcePolicy": {
+			reason: "sync a resource policy when no existing policy is present",
+			args: args{
+				store: makeValidSecretStore().Spec.Provider.AWS,
+				client: fakesm.Client{
+					GetSecretValueFn:    fakesm.NewGetSecretValueFn(secretValueOutput, nil),
+					PutSecretValueFn:    fakesm.NewPutSecretValueFn(putSecretOutput, nil),
+					DescribeSecretFn:    fakesm.NewDescribeSecretFn(tagSecretOutput, nil),
+					TagResourceFn:       fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
+					UntagResourceFn:     fakesm.NewUntagResourceFn(&awssm.UntagResourceOutput{}, nil),
+					GetResourcePolicyFn: fakesm.NewGetResourcePolicyFn(&awssm.GetResourcePolicyOutput{}, nil),
+					PutResourcePolicyFn: fakesm.NewPutResourcePolicyFn(&awssm.PutResourcePolicyOutput{}, nil),
+				},
+				pushSecretData: fake.PushSecretData{
+					SecretKey: secretKey,
+					RemoteKey: fakeKey,
+					Property:  "",
+					Metadata: &apiextensionsv1.JSON{
+						Raw: []byte(`{
+							"apiVersion": "kubernetes.external-secrets.io/v1alpha1",
+							"kind": "PushSecretMetadata",
+							"spec": {
+								"secretPushFormat": "string",
+								"resourcePolicy": {
+										"blockPublicPolicy": true,
+										"policySourceRef": {
+											"kind": "ConfigMap",
+											"name": "resource-policy",
+											"key": "policy.json"
+									}
+								}
+							}
+						}`),
+					},
+				},
+				kubeclient: clientfake.NewFakeClient(&corev1.ConfigMap{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "resource-policy",
+					},
+					Data: map[string]string{
+						"policy.json": `{"Version":"2012-10-17","Statement":[{"Sid":"DenyAll","Effect":"Deny","Principal":"*","Action":"secretsmanager:GetSecretValue","Resource":"*"}]}`,
+					},
+				}),
+			},
+			want: want{
+				err: nil,
+			},
+		},
 		"SetSecretWithExistingNonChangingResourcePolicy": {
 			reason: "sync an existing secret without syncing resource policy that has no change",
 			args: args{
@@ -1397,6 +1445,89 @@ func TestPushSecretResourcePolicyUpdatedWhenValueUnchanged(t *testing.T) {
 	assert.JSONEq(t, newPolicy, *capturedPolicyInput.ResourcePolicy)
 }
 
+func TestPushSecretEmptyExistingResourcePolicy(t *testing.T) {
+	secretKey := fakeSecretKey
+	secretValue := []byte("fake-value")
+	fakeSecret := &corev1.Secret{
+		Data: map[string][]byte{
+			secretKey: secretValue,
+		},
+	}
+	arn := testARN
+	defaultVersion := testDefaultVersion
+	managed := managedBy
+	manager := externalSecrets
+
+	putResourcePolicyCalled := false
+
+	newPolicy := `{"Version":"2012-10-17","Statement":[{"Sid":"DenyAll","Effect":"Deny","Principal":"*","Action":"secretsmanager:GetSecretValue","Resource":"*"}]}`
+
+	client := fakesm.Client{
+		GetSecretValueFn: fakesm.NewGetSecretValueFn(&awssm.GetSecretValueOutput{
+			ARN:          &arn,
+			SecretBinary: secretValue,
+			VersionId:    &defaultVersion,
+		}, nil),
+		DescribeSecretFn: fakesm.NewDescribeSecretFn(&awssm.DescribeSecretOutput{
+			ARN: &arn,
+			Tags: []types.Tag{
+				{Key: &managed, Value: &manager},
+			},
+			VersionIdsToStages: map[string][]string{
+				defaultVersion: {"AWSCURRENT"},
+			},
+		}, nil),
+		PutSecretValueFn:    fakesm.NewPutSecretValueFn(&awssm.PutSecretValueOutput{}, nil),
+		TagResourceFn:       fakesm.NewTagResourceFn(&awssm.TagResourceOutput{}, nil),
+		UntagResourceFn:     fakesm.NewUntagResourceFn(&awssm.UntagResourceOutput{}, nil),
+		GetResourcePolicyFn: fakesm.NewGetResourcePolicyFn(&awssm.GetResourcePolicyOutput{}, nil),
+		PutResourcePolicyFn: fakesm.NewPutResourcePolicyFn(&awssm.PutResourcePolicyOutput{}, nil, func(input *awssm.PutResourcePolicyInput) {
+			putResourcePolicyCalled = true
+		}),
+	}
+
+	kubeclient := clientfake.NewFakeClient(&corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "resource-policy",
+		},
+		Data: map[string]string{
+			"policy.json": newPolicy,
+		},
+	})
+
+	sm := SecretsManager{
+		client: &client,
+		kube:   kubeclient,
+	}
+
+	pushSecretData := fake.PushSecretData{
+		SecretKey: secretKey,
+		RemoteKey: fakeKey,
+		Property:  "",
+		Metadata: &apiextensionsv1.JSON{
+			Raw: []byte(`{
+				"apiVersion": "kubernetes.external-secrets.io/v1alpha1",
+				"kind": "PushSecretMetadata",
+				"spec": {
+					"secretPushFormat": "string",
+					"resourcePolicy": {
+						"blockPublicPolicy": true,
+						"policySourceRef": {
+							"kind": "ConfigMap",
+							"name": "resource-policy",
+							"key": "policy.json"
+						}
+					}
+				}
+			}`),
+		},
+	}
+
+	err := sm.PushSecret(context.Background(), fakeSecret, pushSecretData)
+	require.NoError(t, err)
+	assert.True(t, putResourcePolicyCalled, "PutResourcePolicy should be called when existing policy is empty")
+}
+
 func TestDeleteSecret(t *testing.T) {
 	fakeClient := fakesm.Client{}
 	managed := managedBy