Alerting: Fix Alertmanager change detection for receivers with secure settings (#71307)

* Alerting: Make ApplyAlertmanagerConfiguration only decrypt/encrypt new/changed secure settings

Previously, ApplyAlertmanagerConfiguration would decrypt and re-encrypt all secure settings. However, this caused re-encrypted secure settings to be included in the raw configuration when applied to the embedded alertmanager, resulting in changes to the hash. Consequently, even if no actual modifications were made, saving any alertmanager configuration triggered an apply/restart and created a new historical entry in the database.

To address the issue, this modifies ApplyAlertmanagerConfiguration, which is called by POST `api/alertmanager/grafana/config/api/v1/alerts`, to decrypt and re-encrypt only new and updated secure settings. Unchanged secure settings are loaded directly from the database without alteration.

We determine whether secure settings have changed based on the following (already in-use) assumption: Only new or updated secure settings are provided via the POST `api/alertmanager/grafana/config/api/v1/alerts` request, while existing unchanged settings are omitted.

* Ensure saving a grafana-managed contact point will only send new/changed secure settings

Previously, when saving a grafana-managed contact point, empty string values were transmitted for all unset secure settings. This led to potential backend issues, as it assumed that only newly added or updated secure settings would be provided.

To address this, we now exclude empty ('', null, undefined) secure settings, unless there was a pre-existing entry in secureFields for that specific setting. In essence, this means we only transmit an empty secure setting if a previously configured value was cleared.

* Fix linting

* refactor omitEmptyUnlessExisting

* fixup

---------

Co-authored-by: Gilles De Mey <gilles.de.mey@gmail.com>

pkg/services/ngalert/api/api_alertmanager_test.go

@@ -174,7 +174,7 @@ func TestAlertmanagerConfig(t *testing.T) {
 				OrgID: 12,
 			},
 		}
-		request := createAmConfigRequest(t)
+		request := createAmConfigRequest(t, validConfig)
 
 		response := sut.RoutePostAlertingConfig(&rc, request)
 
@@ -191,7 +191,7 @@ func TestAlertmanagerConfig(t *testing.T) {
 				OrgID: 1,
 			},
 		}
-		request := createAmConfigRequest(t)
+		request := createAmConfigRequest(t, validConfig)
 
 		response := sut.RoutePostAlertingConfig(&rc, request)
 
@@ -208,13 +208,45 @@ func TestAlertmanagerConfig(t *testing.T) {
 				OrgID: 3, // Org 3 was initialized with broken config.
 			},
 		}
-		request := createAmConfigRequest(t)
+		request := createAmConfigRequest(t, validConfig)
 
 		response := sut.RoutePostAlertingConfig(&rc, request)
 
 		require.Equal(t, 202, response.Status())
 	})
 
+	t.Run("assert config hash doesn't change when sending RouteGetAlertingConfig back to RoutePostAlertingConfig", func(t *testing.T) {
+		rc := contextmodel.ReqContext{
+			Context: &web.Context{
+				Req: &http.Request{},
+			},
+			SignedInUser: &user.SignedInUser{
+				OrgID: 1,
+			},
+		}
+		request := createAmConfigRequest(t, validConfigWithSecureSetting)
+
+		r := sut.RoutePostAlertingConfig(&rc, request)
+		require.Equal(t, 202, r.Status())
+
+		am, err := sut.mam.AlertmanagerFor(1)
+		require.NoError(t, err)
+		hash := am.Base.ConfigHash()
+
+		getResponse := sut.RouteGetAlertingConfig(&rc)
+		require.Equal(t, 200, getResponse.Status())
+		postable, err := notifier.Load(getResponse.Body())
+		require.NoError(t, err)
+
+		r = sut.RoutePostAlertingConfig(&rc, *postable)
+		require.Equal(t, 202, r.Status())
+
+		am, err = sut.mam.AlertmanagerFor(1)
+		require.NoError(t, err)
+		newHash := am.Base.ConfigHash()
+		require.Equal(t, hash, newHash)
+	})
+
 	t.Run("when objects are not provisioned", func(t *testing.T) {
 		t.Run("route from GET config has no provenance", func(t *testing.T) {
 			sut := createSut(t)
@@ -259,7 +291,7 @@ func TestAlertmanagerConfig(t *testing.T) {
 		t.Run("contact point from GET config has expected provenance", func(t *testing.T) {
 			sut := createSut(t)
 			rc := createRequestCtxInOrg(1)
-			request := createAmConfigRequest(t)
+			request := createAmConfigRequest(t, validConfig)
 
 			_ = sut.RoutePostAlertingConfig(rc, request)
 
@@ -609,11 +641,11 @@ func createSut(t *testing.T) AlertmanagerSrv {
 	}
 }
 
-func createAmConfigRequest(t *testing.T) apimodels.PostableUserConfig {
+func createAmConfigRequest(t *testing.T, config string) apimodels.PostableUserConfig {
 	t.Helper()
 
 	request := apimodels.PostableUserConfig{}
-	err := request.UnmarshalJSON([]byte(validConfig))
+	err := request.UnmarshalJSON([]byte(config))
 	require.NoError(t, err)
 
 	return request
@@ -676,6 +708,41 @@ var validConfig = `{
 }
 `
 
+var validConfigWithSecureSetting = `{
+	"template_files": {
+		"a": "template"
+	},
+	"alertmanager_config": {
+		"route": {
+			"receiver": "grafana-default-email"
+		},
+		"receivers": [{
+			"name": "grafana-default-email",
+			"grafana_managed_receiver_configs": [{
+				"uid": "",
+				"name": "email receiver",
+				"type": "email",
+				"isDefault": true,
+				"settings": {
+					"addresses": "<example@email.com>"
+				}
+			}]},
+			{
+			"name": "slack",
+			"grafana_managed_receiver_configs": [{
+				"uid": "",
+				"name": "slack1",
+				"type": "slack",
+				"settings": {"text": "slack text"},
+				"secureSettings": {
+					"url": "secure url"
+				}
+			}]
+		}]
+	}
+}
+`
+
 var brokenConfig = `
 	"alertmanager_config": {
 		"route": {

pkg/services/ngalert/api/tooling/definitions/alertmanager.go

@@ -277,7 +277,11 @@ type TestReceiversConfigBodyParams struct {
 }
 
 func (c *TestReceiversConfigBodyParams) ProcessConfig(encrypt EncryptFn) error {
-	return processReceiverConfigs(c.Receivers, encrypt)
+	err := encryptReceiverConfigs(c.Receivers, encrypt)
+	if err != nil {
+		return err
+	}
+	return assignReceiverConfigsUIDs(c.Receivers)
 }
 
 type TestReceiversConfigAlertParams struct {
@@ -615,9 +619,14 @@ func (c *PostableUserConfig) GetGrafanaReceiverMap() map[string]*PostableGrafana
 	return UIDs
 }
 
-// ProcessConfig parses grafana receivers, encrypts secrets and assigns UUIDs (if they are missing)
-func (c *PostableUserConfig) ProcessConfig(encrypt EncryptFn) error {
-	return processReceiverConfigs(c.AlertmanagerConfig.Receivers, encrypt)
+// EncryptConfig parses grafana receivers and encrypts secrets.
+func (c *PostableUserConfig) EncryptConfig(encrypt EncryptFn) error {
+	return encryptReceiverConfigs(c.AlertmanagerConfig.Receivers, encrypt)
+}
+
+// AssignMissingConfigUIDs assigns missing UUIDs to receiver configs.
+func (c *PostableUserConfig) AssignMissingConfigUIDs() error {
+	return assignReceiverConfigsUIDs(c.AlertmanagerConfig.Receivers)
 }
 
 // MarshalYAML implements yaml.Marshaller.
@@ -1285,8 +1294,7 @@ type PostableGrafanaReceivers struct {
 
 type EncryptFn func(ctx context.Context, payload []byte) ([]byte, error)
 
-func processReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error {
-	seenUIDs := make(map[string]struct{})
+func encryptReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error {
 	// encrypt secure settings for storing them in DB
 	for _, r := range c {
 		switch r.Type() {
@@ -1299,6 +1307,20 @@ func processReceiverConfigs(c []*PostableApiReceiver, encrypt EncryptFn) error {
 					}
 					gr.SecureSettings[k] = base64.StdEncoding.EncodeToString(encryptedData)
 				}
+			}
+		default:
+		}
+	}
+	return nil
+}
+
+func assignReceiverConfigsUIDs(c []*PostableApiReceiver) error {
+	seenUIDs := make(map[string]struct{})
+	// encrypt secure settings for storing them in DB
+	for _, r := range c {
+		switch r.Type() {
+		case GrafanaReceiverType:
+			for _, gr := range r.PostableGrafanaReceivers.GrafanaManagedReceivers {
 				if gr.UID == "" {
 					retries := 5
 					for i := 0; i < retries; i++ {

pkg/services/ngalert/notifier/alertmanager_config.go

@@ -166,13 +166,21 @@ func (moa *MultiOrgAlertmanager) ApplyAlertmanagerConfiguration(ctx context.Cont
 		}
 	}
 
+	// First, we encrypt the new or updated secure settings. Then, we load the existing secure settings from the database
+	// and add back any that weren't updated.
+	// We perform these steps in this order to ensure the hash of the secure settings remains stable when no secure
+	// settings were modified.
+	if err := config.EncryptConfig(func(ctx context.Context, payload []byte) ([]byte, error) {
+		return moa.Crypto.Encrypt(ctx, payload, secrets.WithoutScope())
+	}); err != nil {
+		return fmt.Errorf("failed to post process Alertmanager configuration: %w", err)
+	}
+
 	if err := moa.Crypto.LoadSecureSettings(ctx, org, config.AlertmanagerConfig.Receivers); err != nil {
 		return err
 	}
 
-	if err := config.ProcessConfig(func(ctx context.Context, payload []byte) ([]byte, error) {
-		return moa.Crypto.Encrypt(ctx, payload, secrets.WithoutScope())
-	}); err != nil {
+	if err := config.AssignMissingConfigUIDs(); err != nil {
 		return fmt.Errorf("failed to post process Alertmanager configuration: %w", err)
 	}
 

pkg/services/ngalert/notifier/crypto.go

@@ -74,19 +74,13 @@ func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64
 
 			// Frontend sends only the secure settings that have to be updated
 			// Therefore we have to copy from the last configuration only those secure settings not included in the request
-			for key := range cgmr.SecureSettings {
+			for key, encryptedValue := range cgmr.SecureSettings {
 				_, ok := gr.SecureSettings[key]
 				if !ok {
-					decryptedValue, err := c.getDecryptedSecret(cgmr, key)
-					if err != nil {
-						return fmt.Errorf("failed to decrypt stored secure setting: %s: %w", key, err)
-					}
-
 					if receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings == nil {
 						receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings = make(map[string]string, len(cgmr.SecureSettings))
 					}
-
-					receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings[key] = decryptedValue
+					receivers[i].PostableGrafanaReceivers.GrafanaManagedReceivers[j].SecureSettings[key] = encryptedValue
 				}
 			}
 		}

public/app/features/alerting/unified/utils/receiver-form.test.ts

@@ -1,4 +1,4 @@
-import { omitEmptyValues } from './receiver-form';
+import { omitEmptyValues, omitEmptyUnlessExisting } from './receiver-form';
 
 describe('Receiver form utils', () => {
   describe('omitEmptyStringValues', () => {
@@ -41,4 +41,26 @@ describe('Receiver form utils', () => {
       expect(omitEmptyValues(original)).toEqual(expected);
     });
   });
+  describe('omitEmptyUnlessExisting', () => {
+    it('should omit empty strings if no entry in existing', () => {
+      const existing = {
+        five_keep: true,
+      };
+      const original = {
+        one: 'two',
+        two_remove: '',
+        three: 0,
+        four_remove: null,
+        five_keep: '',
+      };
+
+      const expected = {
+        one: 'two',
+        three: 0,
+        five_keep: '',
+      };
+
+      expect(omitEmptyUnlessExisting(original, existing)).toEqual(expected);
+    });
+  });
 });

public/app/features/alerting/unified/utils/receiver-form.ts

@@ -1,4 +1,4 @@
-import { isArray } from 'lodash';
+import { isArray, isNil, omitBy } from 'lodash';
 
 import {
   AlertManagerCortexConfig,
@@ -221,7 +221,7 @@ export function formChannelValuesToGrafanaChannelConfig(
       ...(existing && existing.type === values.type ? existing.settings ?? {} : {}),
       ...(values.settings ?? {}),
     }),
-    secureSettings: values.secureSettings ?? {},
+    secureSettings: omitEmptyUnlessExisting(values.secureSettings, existing?.secureFields),
     type: values.type,
     name,
     disableResolveMessage:
@@ -233,6 +233,9 @@ export function formChannelValuesToGrafanaChannelConfig(
   return channel;
 }
 
+// null, undefined and '' are deemed unacceptable
+const isUnacceptableValue = (value: unknown) => isNil(value) || value === '';
+
 // will remove properties that have empty ('', null, undefined) object properties.
 // traverses nested objects and arrays as well. in place, mutates the object.
 // this is needed because form will submit empty string for not filled in fields,
@@ -243,7 +246,7 @@ export function omitEmptyValues<T>(obj: T): T {
     obj.forEach(omitEmptyValues);
   } else if (typeof obj === 'object' && obj !== null) {
     Object.entries(obj).forEach(([key, value]) => {
-      if (value === '' || value === null || value === undefined) {
+      if (isUnacceptableValue(value)) {
         delete (obj as any)[key];
       } else {
         omitEmptyValues(value);
@@ -252,3 +255,9 @@ export function omitEmptyValues<T>(obj: T): T {
   }
   return obj;
 }
+
+// Will remove empty ('', null, undefined) object properties unless they were previously defined.
+// existing is a map of property names that were previously defined.
+export function omitEmptyUnlessExisting(settings = {}, existing = {}): Record<string, unknown> {
+  return omitBy(settings, (value, key) => isUnacceptableValue(value) && !(key in existing));
+}