diff --git a/internal/controller/appconfigurationprovider_controller.go b/internal/controller/appconfigurationprovider_controller.go index 88eaad1..bfe923b 100644 --- a/internal/controller/appconfigurationprovider_controller.go +++ b/internal/controller/appconfigurationprovider_controller.go @@ -296,7 +296,7 @@ func (reconciler *AzureAppConfigurationProviderReconciler) verifyTargetObjectExi return false, err } - return true, verifyExistingTargetObject(obj, targetName, provider.Name) + return true, verifyExistingTargetObject(obj, targetName, provider) } // collectExistingSecret verifies the existence of a secret and adds it to the existingSecrets map if it exists diff --git a/internal/controller/appconfigurationprovider_controller_test.go b/internal/controller/appconfigurationprovider_controller_test.go index 22a90c3..b08d873 100644 --- a/internal/controller/appconfigurationprovider_controller_test.go +++ b/internal/controller/appconfigurationprovider_controller_test.go @@ -1427,4 +1427,134 @@ var _ = Describe("AppConfiguationProvider controller", func() { _ = k8sClient.Delete(ctx, configProvider) }) }) + + Describe("When target Secret already exists with a foreign owner reference", func() { + It("Should refuse to overwrite a plain pre-existing Secret it does not own", func() { + By("pre-creating a Secret with no owner references") + ctx := context.Background() + victimName := "victim-plain-secret" + attackerProvider := "attacker-provider-plain" + + victim := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: victimName, Namespace: ProviderNamespace}, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"protected": []byte("ORIGINAL-VALUE")}, + } + Expect(k8sClient.Create(ctx, victim)).Should(Succeed()) + + attackerSettings := &loader.TargetKeyValueSettings{ + SecretSettings: map[string]corev1.Secret{ + victimName: { + Data: map[string][]byte{"protected": []byte("ATTACKER-INJECTED")}, + Type: corev1.SecretTypeOpaque, + }, + }, + K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{ + victimName: { + Type: corev1.SecretTypeOpaque, + SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata), + }, + }, + } + mockConfigurationSettings.EXPECT().CreateTargetSettings(gomock.Any(), gomock.Any()).Return(attackerSettings, nil).AnyTimes() + + provider := &acpv1.AzureAppConfigurationProvider{ + ObjectMeta: metav1.ObjectMeta{Name: attackerProvider, Namespace: ProviderNamespace}, + Spec: acpv1.AzureAppConfigurationProviderSpec{ + Endpoint: &EndpointName, + Target: acpv1.ConfigurationGenerationParameters{ConfigMapName: "cm-plain"}, + Secret: &acpv1.SecretReference{ + Target: acpv1.SecretGenerationParameters{SecretName: victimName}, + }, + }, + } + Expect(k8sClient.Create(ctx, provider)).Should(Succeed()) + + By("verifying the victim Secret data remains unchanged") + Consistently(func() string { + s := &corev1.Secret{} + _ = k8sClient.Get(ctx, types.NamespacedName{Name: victimName, Namespace: ProviderNamespace}, s) + return string(s.Data["protected"]) + }, duration, interval).Should(Equal("ORIGINAL-VALUE")) + + _ = k8sClient.Delete(ctx, provider) + _ = k8sClient.Delete(ctx, victim) + }) + + It("Should refuse to overwrite a Secret whose non-provider owner reference name matches the provider name", func() { + By("pre-creating a Secret owned by a Deployment sharing the attacker provider's name") + ctx := context.Background() + victimName := "victim-owned-secret" + // The attacker names their provider CR to match an ownerReference already + // present on the victim Secret. Here the Secret is owned by a Deployment + // named "frontend" (models cert-manager Certificate / workload / operator CR). + attackerProvider := "frontend" + + victim := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: victimName, + Namespace: ProviderNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: "apps/v1", + Kind: "Deployment", + Name: "frontend", // non-provider owner, different Kind and UID + UID: types.UID("11111111-1111-1111-1111-111111111111"), + }, + }, + }, + Type: corev1.SecretTypeOpaque, + Data: map[string][]byte{"protected": []byte("ORIGINAL-VALUE")}, + } + Expect(k8sClient.Create(ctx, victim)).Should(Succeed()) + + attackerSettings := &loader.TargetKeyValueSettings{ + SecretSettings: map[string]corev1.Secret{ + victimName: { + Data: map[string][]byte{ + "protected": []byte("ATTACKER-INJECTED"), + "extra": []byte("PWNED"), + }, + Type: corev1.SecretTypeOpaque, + }, + }, + K8sSecrets: map[string]*loader.TargetK8sSecretMetadata{ + victimName: { + Type: corev1.SecretTypeOpaque, + SecretsKeyVaultMetadata: make(map[string]loader.KeyVaultSecretMetadata), + }, + }, + } + mockConfigurationSettings.EXPECT().CreateTargetSettings(gomock.Any(), gomock.Any()).Return(attackerSettings, nil).AnyTimes() + + provider := &acpv1.AzureAppConfigurationProvider{ + ObjectMeta: metav1.ObjectMeta{Name: attackerProvider, Namespace: ProviderNamespace}, + Spec: acpv1.AzureAppConfigurationProviderSpec{ + Endpoint: &EndpointName, + Target: acpv1.ConfigurationGenerationParameters{ConfigMapName: "cm-owned"}, + Secret: &acpv1.SecretReference{ + Target: acpv1.SecretGenerationParameters{SecretName: victimName}, + }, + }, + } + Expect(k8sClient.Create(ctx, provider)).Should(Succeed()) + + By("verifying the victim Secret data and owner reference remain unchanged") + Consistently(func() string { + s := &corev1.Secret{} + _ = k8sClient.Get(ctx, types.NamespacedName{Name: victimName, Namespace: ProviderNamespace}, s) + return string(s.Data["protected"]) + }, duration, interval).Should(Equal("ORIGINAL-VALUE")) + + s := &corev1.Secret{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: victimName, Namespace: ProviderNamespace}, s)).Should(Succeed()) + Expect(s.Data).ShouldNot(HaveKey("extra")) + Expect(len(s.OwnerReferences)).Should(Equal(1)) + Expect(s.OwnerReferences[0].Kind).Should(Equal("Deployment")) + Expect(s.OwnerReferences[0].Name).Should(Equal("frontend")) + + _ = k8sClient.Delete(ctx, provider) + _ = k8sClient.Delete(ctx, victim) + }) + }) }) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 1da8a8a..07589bf 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -199,15 +199,23 @@ func verifyAuthObject(auth *acpv1.AzureAppConfigurationProviderAuth) error { return nil } -func verifyExistingTargetObject[T client.Object](targetObj T, targetName string, providerName string) error { +func verifyExistingTargetObject[T client.Object](targetObj T, targetName string, provider *acpv1.AzureAppConfigurationProvider) error { objectKind := targetObj.GetObjectKind().GroupVersionKind().Kind if targetObj.GetName() != targetName { return nil } - // If existing object is created by current provider, just skip it. + // If existing object is owned by the current provider, just skip it. + // A valid ownership must identify the same AzureAppConfigurationProvider by + // API version, kind, name and UID. Comparing the name alone is not sufficient: + // a foreign owner reference that happens to share the provider's name (for + // example, a different resource kind) must not authorize adoption or update. + expectedAPIVersion := acpv1.GroupVersion.String() for _, ownerRef := range targetObj.GetOwnerReferences() { - if ownerRef.Name == providerName { + if ownerRef.APIVersion == expectedAPIVersion && + ownerRef.Kind == ProviderName && + ownerRef.Name == provider.Name && + ownerRef.UID == provider.UID { return nil } } diff --git a/internal/controller/utils_test.go b/internal/controller/utils_test.go index 125146f..3ffd14b 100644 --- a/internal/controller/utils_test.go +++ b/internal/controller/utils_test.go @@ -1063,10 +1063,18 @@ func TestVerifyAuthObject(t *testing.T) { func TestVerifyExistingTargetObject(t *testing.T) { providerNamespace := "default" + expectedAPIVersion := acpv1.GroupVersion.String() t.Run("Should return no error if existing configMap is valid", func(t *testing.T) { providerName := "providerName" configMapName := "configMapName" + provider := &acpv1.AzureAppConfigurationProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: providerName, + Namespace: providerNamespace, + UID: "provider-uid", + }, + } // Case 1: ConfigMap owned by the provider configMap := &corev1.ConfigMap{ @@ -1074,7 +1082,7 @@ func TestVerifyExistingTargetObject(t *testing.T) { Name: configMapName, Namespace: providerNamespace, OwnerReferences: []metav1.OwnerReference{ - {Kind: "AzureAppConfigurationProvider", Name: providerName}, + {APIVersion: expectedAPIVersion, Kind: "AzureAppConfigurationProvider", Name: providerName, UID: "provider-uid"}, }, }, } @@ -1085,7 +1093,7 @@ func TestVerifyExistingTargetObject(t *testing.T) { Name: "anotherConfigMap", Namespace: providerNamespace, OwnerReferences: []metav1.OwnerReference{ - {Kind: "AzureAppConfigurationProvider", Name: providerName}, + {APIVersion: expectedAPIVersion, Kind: "AzureAppConfigurationProvider", Name: providerName, UID: "provider-uid"}, }, }, } @@ -1093,14 +1101,21 @@ func TestVerifyExistingTargetObject(t *testing.T) { // Case 3: Empty ConfigMap emptyConfigMap := &corev1.ConfigMap{} - assert.Nil(t, verifyExistingTargetObject(emptyConfigMap, configMapName, providerName)) - assert.Nil(t, verifyExistingTargetObject(configMap, configMapName, providerName)) - assert.Nil(t, verifyExistingTargetObject(configMap2, configMapName, providerName)) + assert.Nil(t, verifyExistingTargetObject(emptyConfigMap, configMapName, provider)) + assert.Nil(t, verifyExistingTargetObject(configMap, configMapName, provider)) + assert.Nil(t, verifyExistingTargetObject(configMap2, configMapName, provider)) }) t.Run("Should return error if configMap is not valid", func(t *testing.T) { providerName := "providerName" configMapName := "configMapName" + provider := &acpv1.AzureAppConfigurationProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: providerName, + Namespace: providerNamespace, + UID: "provider-uid", + }, + } // Case 1: ConfigMap exists but is not owned by the provider configMap1 := &corev1.ConfigMap{ @@ -1122,16 +1137,52 @@ func TestVerifyExistingTargetObject(t *testing.T) { Name: configMapName, Namespace: providerNamespace, OwnerReferences: []metav1.OwnerReference{ - {Kind: "AzureAppConfigurationProvider", Name: "anotherProvider"}, + {APIVersion: expectedAPIVersion, Kind: "AzureAppConfigurationProvider", Name: "anotherProvider", UID: "another-uid"}, + }, + }, + } + + // Case 3: ConfigMap owned by a foreign resource that shares the provider's name. + // The name matches but the API version, kind and UID do not, so it must be rejected. + configMap3 := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: configMapName, + Namespace: providerNamespace, + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "apps/v1", Kind: "Deployment", Name: providerName, UID: "foreign-uid"}, + }, + }, + } + + // Case 4: ConfigMap owned by an AzureAppConfigurationProvider with the same + // name but a different UID (for example, a deleted and recreated provider). + configMap4 := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{ + Kind: "ConfigMap", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: configMapName, + Namespace: providerNamespace, + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: expectedAPIVersion, Kind: "AzureAppConfigurationProvider", Name: providerName, UID: "stale-uid"}, }, }, } - err1 := verifyExistingTargetObject(configMap1, configMapName, providerName) + err1 := verifyExistingTargetObject(configMap1, configMapName, provider) assert.Equal(t, "a ConfigMap with name 'configMapName' already exists in namespace 'default'", err1.Error()) - err2 := verifyExistingTargetObject(configMap2, configMapName, providerName) + err2 := verifyExistingTargetObject(configMap2, configMapName, provider) assert.Equal(t, "a ConfigMap with name 'configMapName' already exists in namespace 'default'", err2.Error()) + + err3 := verifyExistingTargetObject(configMap3, configMapName, provider) + assert.Equal(t, "a ConfigMap with name 'configMapName' already exists in namespace 'default'", err3.Error()) + + err4 := verifyExistingTargetObject(configMap4, configMapName, provider) + assert.Equal(t, "a ConfigMap with name 'configMapName' already exists in namespace 'default'", err4.Error()) }) }