Add guards against using distinct subscriptions between owner and child resources

v2/internal/reconcilers/arm/azure_generic_arm_reconciler_instance.go

@@ -293,7 +293,7 @@ func (r *azureDeploymentReconcilerInstance) BeginCreateOrUpdateResource(
 
 	resourceID := genruntime.GetResourceIDOrDefault(r.Obj)
 	if resourceID != "" {
-		err = checkSubscription(resourceID, r.ARMConnection.SubscriptionID())
+		err := r.checkSubscription(resourceID)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -303,7 +303,6 @@ func (r *azureDeploymentReconcilerInstance) BeginCreateOrUpdateResource(
 	if err != nil {
 		return ctrl.Result{}, err
 	}
-
 	// Use conditions.SetConditionReasonAware here to override any Warning conditions set earlier in the reconciliation process.
 	// Note that this call should be done after all validation has passed and all that is left to do is send the payload to ARM.
 	conditions.SetConditionReasonAware(r.Obj, r.PositiveConditions.Ready.Reconciling(r.Obj.GetGeneration()))
@@ -369,13 +368,15 @@ func (r *azureDeploymentReconcilerInstance) preReconciliationCheck(ctx context.C
 	return check, nil
 }
 
-func checkSubscription(resourceID string, clientSubID string) error {
+// checkSubscription checks if subscription on resource matches with credentials used while creating a resource.
+// Which prevents users to modify subscription in their credential.
+func (r *azureDeploymentReconcilerInstance) checkSubscription(resourceID string) error {
 	parsedRID, err := arm.ParseResourceID(resourceID)
 	// Some resources like '/providers/Microsoft.Subscription/aliases' do not have subscriptionID, so we need to make sure subscriptionID exists before we check.
 	// TODO: we need a better way?
 	if err == nil {
-		if parsedRID.ResourceGroupName != "" && parsedRID.SubscriptionID != clientSubID {
-			err = errors.Errorf("SubscriptionID %q for %q resource does not match with Client Credential: %q", parsedRID.SubscriptionID, resourceID, clientSubID)
+		if parsedRID.ResourceGroupName != "" && parsedRID.SubscriptionID != r.ARMConnection.SubscriptionID() {
+			err = errors.Errorf("SubscriptionID %q for %q resource does not match with Client Credential: %q", parsedRID.SubscriptionID, resourceID, r.ARMConnection.SubscriptionID())
 			return conditions.NewReadyConditionImpactingError(err, conditions.ConditionSeverityError, conditions.ReasonSubscriptionMismatch)
 		}
 	}
@@ -809,7 +810,7 @@ func (r *azureDeploymentReconcilerInstance) deleteResource(
 		return ctrl.Result{}, nil
 	}
 
-	err := checkSubscription(resourceID, r.ARMConnection.SubscriptionID()) // TODO: Possibly we should pass this in as a parameter?
+	err := r.checkSubscription(resourceID) // TODO: Possibly we should pass this in as a parameter?
 	if err != nil {
 		return ctrl.Result{}, err
 	}

v2/internal/resolver/resolver_test.go

@@ -7,6 +7,7 @@ package resolver_test
 
 import (
 	"context"
+	"fmt"
 	"testing"
 
 	. "github.com/onsi/gomega"
@@ -89,6 +90,9 @@ func createResourceGroup(name string) *resources.ResourceGroup {
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      name,
 			Namespace: testNamespace,
+			Annotations: map[string]string{
+				genruntime.ResourceIDAnnotation: fmt.Sprintf("/subscriptions/1234/resourceGroups/%s", name),
+			},
 		},
 		Spec: resources.ResourceGroup_Spec{
 			Location:  to.Ptr("West US"),

v2/internal/resolver/resource_hierarchy.go

@@ -142,6 +142,22 @@ func (h ResourceHierarchy) fullyQualifiedARMIDImpl(subscriptionID string, origin
 			return "", err
 		}
 
+		root := h[0]
+
+		err = genruntime.VerifyResourceOwnerARMID(root)
+		if err != nil {
+			return "", err
+		}
+
+		armID, err := genruntime.GetAndParseResourceID(root)
+		if err != nil {
+			return "", err
+		}
+
+		if err = h.matchOwnerSubscription(subscriptionID, armID); err != nil {
+			return "", err
+		}
+
 		// Ensure that we have the same number of names and types
 		if len(remainingNames) != len(resourceTypes) {
 			return "", errors.Errorf(
@@ -202,13 +218,9 @@ func (h ResourceHierarchy) fullyQualifiedARMIDImpl(subscriptionID string, origin
 		if err != nil {
 			return "", err
 		}
-		// armIDSub may be empty if there is no subscription in the user specified ARM ID (for example because the resource roots
-		// at the tenant level)
-		if armID.SubscriptionID != "" {
-			// Confirm that the subscription ID the user specified matches the subscription ID we're using from our credential
-			if !strings.EqualFold(armID.SubscriptionID, subscriptionID) {
-				return "", core.NewSubscriptionMismatchError(armID.SubscriptionID, subscriptionID)
-			}
+
+		if err = h.matchOwnerSubscription(subscriptionID, armID); err != nil {
+			return "", err
 		}
 
 		// Rooting to an ARM ID means that some of the resourceTypes may not actually be included explicitly in our
@@ -261,6 +273,18 @@ func (h ResourceHierarchy) fullyQualifiedARMIDImpl(subscriptionID string, origin
 	}
 }
 
+func (h ResourceHierarchy) matchOwnerSubscription(subscriptionID string, ownerRID *arm.ResourceID) error {
+	// armIDSub may be empty if there is no subscription in the user specified ARM ID (for example because the resource roots
+	// at the tenant level)
+	if ownerRID.SubscriptionID != "" {
+		// Confirm that the subscription ID the user specified matches the subscription ID we're using from our credential
+		if !strings.EqualFold(ownerRID.SubscriptionID, subscriptionID) {
+			return core.NewSubscriptionMismatchError(ownerRID.SubscriptionID, subscriptionID)
+		}
+	}
+	return nil
+}
+
 // rootKind returns the ResourceHierarchyRoot type of the hierarchy.
 // There are 6 cases here:
 //  1. The hierarchy is comprised solely of a resource group. This is subscription rooted.

v2/internal/resolver/resource_hierarchy_test.go

@@ -101,6 +101,20 @@ func Test_ResourceHierarchy_ResourceGroup_NestedResource(t *testing.T) {
 	g.Expect(hierarchy.FullyQualifiedARMID("1234")).To(Equal(expectedARMID))
 }
 
+func Test_ResourceHierarchy_ResourceGroup_NestedResource_MatchSubscriptionWithOwner(t *testing.T) {
+	t.Parallel()
+	g := NewGomegaWithT(t)
+
+	resourceGroupName := "myrg"
+	resourceName := "myresource"
+	childResourceName := "mychildresource"
+
+	hierarchy := createDeeplyNestedResource(resourceGroupName, resourceName, childResourceName)
+
+	_, err := hierarchy.FullyQualifiedARMID("4567")
+	g.Expect(err).To(MatchError("resource subscription \"4567\" does not match parent subscription \"1234\""))
+}
+
 func Test_ResourceHierarchy_ExtensionOnResourceGroup(t *testing.T) {
 	t.Parallel()
 	g := NewGomegaWithT(t)

v2/test/single_operator_multitenant_test.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"testing"
 
+	"github.com/google/uuid"
 	"github.com/pkg/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
@@ -133,6 +134,44 @@ func Test_Multitenant_SingleOperator_PerResourceCredential(t *testing.T) {
 	tc.DeleteResourcesAndWait(acct, rg)
 }
 
+func Test_Multitenant_SingleOperator_PerResourceCredential_MatchSubscriptionWithOwner(t *testing.T) {
+	t.Parallel()
+
+	tc := globalTestContext.ForTest(t)
+
+	secret, err := newClientSecretCredential(uuid.New().String(), tc.AzureTenant, "credential", tc.Namespace)
+	tc.Expect(err).To(BeNil())
+
+	tc.CreateResource(secret)
+
+	rg := tc.CreateTestResourceGroupAndWait()
+
+	acct := newStorageAccount(tc, rg)
+	acct.Annotations = map[string]string{annotations.PerResourceSecret: secret.Name}
+
+	// Creating new storage account in with restricted permissions per resource secret should fail.
+	tc.CreateResourceAndWaitForState(acct, metav1.ConditionFalse, conditions.ConditionSeverityError)
+	tc.Expect(acct.Status.Conditions[0].Message).To(ContainSubstring("does not match parent subscription"))
+
+	// Deleting the per-resource credential annotation would default to applying the global credential with all permissions
+	old := acct.DeepCopy()
+	delete(acct.Annotations, annotations.PerResourceSecret)
+	tc.PatchResourceAndWait(old, acct)
+
+	objKey := client.ObjectKeyFromObject(acct)
+	tc.GetResource(objKey, acct)
+	tc.Expect(acct.Annotations).ToNot(HaveKey(annotations.PerResourceSecret))
+
+	resID := genruntime.GetResourceIDOrDefault(acct)
+
+	// Make sure the StorageAccount is created successfully in Azure.
+	exists, _, err := tc.AzureClient.CheckExistenceWithGetByID(tc.Ctx, resID, string(storage.APIVersion_Value))
+	tc.Expect(err).ToNot(HaveOccurred())
+	tc.Expect(exists).To(BeTrue())
+
+	tc.DeleteResourcesAndWait(acct, rg)
+}
+
 func newClientSecretCredential(subscriptionID, tenantID, name, namespace string) (*v1.Secret, error) {
 	clientSecret := os.Getenv(AzureClientSecretMultitenantVar)
 	if clientSecret == "" {