[release-10.4.17] Alerting: Re-encrypt existing contact points before get and patch in legacy config API (#101834)

* Alerting: Re-encrypt existing contact points before get and patch in legacy config API (#101263)

* Test covering Get+Save interaction for newly secret fields

* Alerting: Re-encrypt existing contact points before get and patch

(cherry picked from commit b73c59547c)

* Fix provisioning tests

* googlechat url test fix

pkg/services/ngalert/api/api_alertmanager_test.go

@@ -3,6 +3,7 @@ package api
 import (
 	"context"
 	"crypto/md5"
+	"encoding/base64"
 	"encoding/json"
 	"math/rand"
 	"net/http"
@@ -16,6 +17,7 @@ import (
 	amv2 "github.com/prometheus/alertmanager/api/v2/models"
 	"github.com/prometheus/alertmanager/pkg/labels"
 	"github.com/prometheus/client_golang/prometheus"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"github.com/grafana/grafana/pkg/api/response"
@@ -329,6 +331,118 @@ func TestAlertmanagerConfig(t *testing.T) {
 	})
 }
 
+func TestGetAlertmanagerConfiguration_NewSecretField(t *testing.T) {
+	// This test has the following goals:
+	// Given:
+	// - A saved notifier config with an existing secret field stored unencrypted in Settings.
+	// Ensure:
+	// - The secret field is not returned in plaintext.
+	// - The secret field is returned as a bool in SecureFields.
+	// - The secret field is correctly saved and encrypted in SecureSettings when saving the notifier config without changes.
+	// - The secret field is removed from Settings when saving the notifier config.
+
+	sut := createSut(t)
+	orgId := int64(1)
+
+	// This config has the secret field "integrationKey" stored incorrectly and unencrypted in Settings.
+	configs := map[int64]*ngmodels.AlertConfiguration{
+		1: {
+			OrgID: orgId,
+			AlertmanagerConfiguration: `{
+	"alertmanager_config": {
+		"route": {
+			"receiver": "configWithNewlySecretSetting"
+		},
+		"receivers": [{
+			"name": "configWithNewlySecretSetting",
+			"grafana_managed_receiver_configs": [{
+				"uid": "configWithNewlySecretSetting-uid",
+				"name": "configWithNewlySecretSetting",
+				"type": "pagerduty",
+				"settings": {"integrationKey": "unencrypted secure secret"},
+				"secureSettings": {}
+			}]
+		}]
+	}
+}
+`,
+			CreatedAt: time.Now().Unix(),
+			Default:   false,
+		},
+	}
+
+	// Store the config as-is in the database. Bypasses normal save route so it doesn't get pre-emptively fixed.
+	mam := createMultiOrgAlertmanager(t, configs)
+	sut.mam = mam
+
+	rc := createRequestCtxInOrg(orgId)
+	res := sut.RouteGetAlertingConfig(rc)
+	gettable := asGettableUserConfig(t, res)
+
+	integration := gettable.GetGrafanaReceiverMap()["configWithNewlySecretSetting-uid"]
+	require.NotNil(t, integration)
+
+	var settings map[string]string
+	err := json.Unmarshal(integration.Settings, &settings)
+	require.NoError(t, err)
+
+	// The secret field "integrationKey" should not be returned in plaintext.
+	assert.NotEqual(t, "unencrypted secure secret", settings["integrationKey"])
+	// Just in case let's look for the unencrypted value anywhere in the settings.
+	assert.NotContains(t, string(integration.Settings), "unencrypted")
+
+	// The secret fields should be returned as a bool in SecureFields.
+	assert.True(t, integration.SecureFields["integrationKey"])
+
+	// Now we save the config without changes. This should encrypt the field "integrationKey" into SecureSettings and
+	// remove it from Settings.
+
+	// Simulates FE-API interaction, "integrationKey" is not sent in Settings as the caller.
+	// Instead, it leaves it out of "SecureSettings" to indicate the API should keep the existing value.
+	var postWithoutChanges = `{
+	"alertmanager_config": {
+		"route": {
+			"receiver": "configWithNewlySecretSetting"
+		},
+		"receivers": [{
+			"name": "configWithNewlySecretSetting",
+			"grafana_managed_receiver_configs": [{
+				"uid": "configWithNewlySecretSetting-uid",
+				"name": "configWithNewlySecretSetting",
+				"type": "pagerduty",
+				"settings": {},
+				"secureSettings": {}
+			}]
+		}]
+	}
+}
+`
+	postable := createAmConfigRequest(t, postWithoutChanges)
+
+	res = sut.RoutePostAlertingConfig(rc, postable)
+	require.Equal(t, 202, res.Status())
+
+	// Check that the secret field "integrationKey" is now encrypted in SecureSettings.
+	savedConfig := &apimodels.PostableUserConfig{}
+	err = json.Unmarshal([]byte(configs[orgId].AlertmanagerConfiguration), savedConfig)
+	require.NoError(t, err)
+
+	savedIntegration := savedConfig.GetGrafanaReceiverMap()["configWithNewlySecretSetting-uid"]
+	require.NotNil(t, savedIntegration)
+
+	// No longer in Settings.
+	assert.Equal(t, "{}", string(savedIntegration.Settings))
+
+	// Encrypted in SecureSettings.
+	secureSecret := savedIntegration.SecureSettings["integrationKey"]
+	assert.NotEmpty(t, secureSecret)
+	encryptedSecret, err := base64.StdEncoding.DecodeString(secureSecret)
+	require.NoError(t, err)
+
+	// No access to .Decrypt, but we can check that it's not the same as the unencrypted value.
+	assert.NotEqual(t, "unencrypted secure secret", string(encryptedSecret))
+}
+
 func TestAlertmanagerAutogenConfig(t *testing.T) {
 	createSutForAutogen := func(t *testing.T) (AlertmanagerSrv, map[int64]*ngmodels.AlertConfiguration) {
 		sut := createSut(t)

pkg/services/ngalert/api/api_provisioning_test.go

@@ -1429,7 +1429,7 @@ func TestProvisioningApiContactPointExport(t *testing.T) {
 		})
 
 		t.Run("json body content is as expected", func(t *testing.T) {
-			expectedRedactedResponse := `{"apiVersion":1,"contactPoints":[{"orgId":1,"name":"grafana-default-email","receivers":[{"uid":"ad95bd8a-49ed-4adc-bf89-1b444fa1aa5b","type":"email","settings":{"addresses":"\u003cexample@email.com\u003e"},"disableResolveMessage":false}]},{"orgId":1,"name":"multiple integrations","receivers":[{"uid":"c2090fda-f824-4add-b545-5a4d5c2ef082","type":"prometheus-alertmanager","settings":{"basicAuthPassword":"[REDACTED]","basicAuthUser":"test","url":"http://localhost:9093"},"disableResolveMessage":true},{"uid":"c84539ec-f87e-4fc5-9a91-7a687d34bbd1","type":"discord","settings":{"avatar_url":"some avatar","url":"some url","use_discord_username":true},"disableResolveMessage":false}]},{"orgId":1,"name":"pagerduty test","receivers":[{"uid":"b9bf06f8-bde2-4438-9d4a-bba0522dcd4d","type":"pagerduty","settings":{"client":"some client","integrationKey":"[REDACTED]","severity":"criticalish"},"disableResolveMessage":false}]},{"orgId":1,"name":"slack test","receivers":[{"uid":"cbfd0976-8228-4126-b672-4419f30a9e50","type":"slack","settings":{"text":"title body test","title":"title test","url":"[REDACTED]"},"disableResolveMessage":true}]}]}`
+			expectedRedactedResponse := `{"apiVersion":1,"contactPoints":[{"orgId":1,"name":"grafana-default-email","receivers":[{"uid":"ad95bd8a-49ed-4adc-bf89-1b444fa1aa5b","type":"email","settings":{"addresses":"\u003cexample@email.com\u003e"},"disableResolveMessage":false}]},{"orgId":1,"name":"multiple integrations","receivers":[{"uid":"c2090fda-f824-4add-b545-5a4d5c2ef082","type":"prometheus-alertmanager","settings":{"basicAuthPassword":"[REDACTED]","basicAuthUser":"test","url":"http://localhost:9093"},"disableResolveMessage":true},{"uid":"c84539ec-f87e-4fc5-9a91-7a687d34bbd1","type":"discord","settings":{"avatar_url":"some avatar","url":"[REDACTED]","use_discord_username":true},"disableResolveMessage":false}]},{"orgId":1,"name":"pagerduty test","receivers":[{"uid":"b9bf06f8-bde2-4438-9d4a-bba0522dcd4d","type":"pagerduty","settings":{"client":"some client","integrationKey":"[REDACTED]","severity":"criticalish"},"disableResolveMessage":false}]},{"orgId":1,"name":"slack test","receivers":[{"uid":"cbfd0976-8228-4126-b672-4419f30a9e50","type":"slack","settings":{"text":"title body test","title":"title test","url":"[REDACTED]"},"disableResolveMessage":true}]}]}`
 			t.Run("decrypt false", func(t *testing.T) {
 				env := createTestEnv(t, testContactPointConfig)
 				sut := createProvisioningSrvSutFromEnv(t, &env)
@@ -1482,14 +1482,14 @@ func TestProvisioningApiContactPointExport(t *testing.T) {
 
 				response := sut.RouteGetContactPointsExport(&rc)
 
-				expectedResponse := `{"apiVersion":1,"contactPoints":[{"orgId":1,"name":"multiple integrations","receivers":[{"uid":"c2090fda-f824-4add-b545-5a4d5c2ef082","type":"prometheus-alertmanager","settings":{"basicAuthPassword":"[REDACTED]","basicAuthUser":"test","url":"http://localhost:9093"},"disableResolveMessage":true},{"uid":"c84539ec-f87e-4fc5-9a91-7a687d34bbd1","type":"discord","settings":{"avatar_url":"some avatar","url":"some url","use_discord_username":true},"disableResolveMessage":false}]}]}`
+				expectedResponse := `{"apiVersion":1,"contactPoints":[{"orgId":1,"name":"multiple integrations","receivers":[{"uid":"c2090fda-f824-4add-b545-5a4d5c2ef082","type":"prometheus-alertmanager","settings":{"basicAuthPassword":"[REDACTED]","basicAuthUser":"test","url":"http://localhost:9093"},"disableResolveMessage":true},{"uid":"c84539ec-f87e-4fc5-9a91-7a687d34bbd1","type":"discord","settings":{"avatar_url":"some avatar","url":"[REDACTED]","use_discord_username":true},"disableResolveMessage":false}]}]}`
 				require.Equal(t, 200, response.Status())
 				require.Equal(t, expectedResponse, string(response.Body()))
 			})
 		})
 
 		t.Run("yaml body content is as expected", func(t *testing.T) {
-			expectedRedactedResponse := "apiVersion: 1\ncontactPoints:\n    - orgId: 1\n      name: grafana-default-email\n      receivers:\n        - uid: ad95bd8a-49ed-4adc-bf89-1b444fa1aa5b\n          type: email\n          settings:\n            addresses: <example@email.com>\n          disableResolveMessage: false\n    - orgId: 1\n      name: multiple integrations\n      receivers:\n        - uid: c2090fda-f824-4add-b545-5a4d5c2ef082\n          type: prometheus-alertmanager\n          settings:\n            basicAuthPassword: '[REDACTED]'\n            basicAuthUser: test\n            url: http://localhost:9093\n          disableResolveMessage: true\n        - uid: c84539ec-f87e-4fc5-9a91-7a687d34bbd1\n          type: discord\n          settings:\n            avatar_url: some avatar\n            url: some url\n            use_discord_username: true\n          disableResolveMessage: false\n    - orgId: 1\n      name: pagerduty test\n      receivers:\n        - uid: b9bf06f8-bde2-4438-9d4a-bba0522dcd4d\n          type: pagerduty\n          settings:\n            client: some client\n            integrationKey: '[REDACTED]'\n            severity: criticalish\n          disableResolveMessage: false\n    - orgId: 1\n      name: slack test\n      receivers:\n        - uid: cbfd0976-8228-4126-b672-4419f30a9e50\n          type: slack\n          settings:\n            text: title body test\n            title: title test\n            url: '[REDACTED]'\n          disableResolveMessage: true\n"
+			expectedRedactedResponse := "apiVersion: 1\ncontactPoints:\n    - orgId: 1\n      name: grafana-default-email\n      receivers:\n        - uid: ad95bd8a-49ed-4adc-bf89-1b444fa1aa5b\n          type: email\n          settings:\n            addresses: <example@email.com>\n          disableResolveMessage: false\n    - orgId: 1\n      name: multiple integrations\n      receivers:\n        - uid: c2090fda-f824-4add-b545-5a4d5c2ef082\n          type: prometheus-alertmanager\n          settings:\n            basicAuthPassword: '[REDACTED]'\n            basicAuthUser: test\n            url: http://localhost:9093\n          disableResolveMessage: true\n        - uid: c84539ec-f87e-4fc5-9a91-7a687d34bbd1\n          type: discord\n          settings:\n            avatar_url: some avatar\n            url: '[REDACTED]'\n            use_discord_username: true\n          disableResolveMessage: false\n    - orgId: 1\n      name: pagerduty test\n      receivers:\n        - uid: b9bf06f8-bde2-4438-9d4a-bba0522dcd4d\n          type: pagerduty\n          settings:\n            client: some client\n            integrationKey: '[REDACTED]'\n            severity: criticalish\n          disableResolveMessage: false\n    - orgId: 1\n      name: slack test\n      receivers:\n        - uid: cbfd0976-8228-4126-b672-4419f30a9e50\n          type: slack\n          settings:\n            text: title body test\n            title: title test\n            url: '[REDACTED]'\n          disableResolveMessage: true\n"
 			t.Run("decrypt false", func(t *testing.T) {
 				env := createTestEnv(t, testContactPointConfig)
 				sut := createProvisioningSrvSutFromEnv(t, &env)
@@ -1542,7 +1542,7 @@ func TestProvisioningApiContactPointExport(t *testing.T) {
 
 				response := sut.RouteGetContactPointsExport(&rc)
 
-				expectedResponse := "apiVersion: 1\ncontactPoints:\n    - orgId: 1\n      name: multiple integrations\n      receivers:\n        - uid: c2090fda-f824-4add-b545-5a4d5c2ef082\n          type: prometheus-alertmanager\n          settings:\n            basicAuthPassword: '[REDACTED]'\n            basicAuthUser: test\n            url: http://localhost:9093\n          disableResolveMessage: true\n        - uid: c84539ec-f87e-4fc5-9a91-7a687d34bbd1\n          type: discord\n          settings:\n            avatar_url: some avatar\n            url: some url\n            use_discord_username: true\n          disableResolveMessage: false\n"
+				expectedResponse := "apiVersion: 1\ncontactPoints:\n    - orgId: 1\n      name: multiple integrations\n      receivers:\n        - uid: c2090fda-f824-4add-b545-5a4d5c2ef082\n          type: prometheus-alertmanager\n          settings:\n            basicAuthPassword: '[REDACTED]'\n            basicAuthUser: test\n            url: http://localhost:9093\n          disableResolveMessage: true\n        - uid: c84539ec-f87e-4fc5-9a91-7a687d34bbd1\n          type: discord\n          settings:\n            avatar_url: some avatar\n            url: '[REDACTED]'\n            use_discord_username: true\n          disableResolveMessage: false\n"
 				require.Equal(t, 200, response.Status())
 				require.Equal(t, expectedResponse, string(response.Body()))
 			})
@@ -2009,10 +2009,11 @@ var testContactPointConfig = `
             "disableResolveMessage":false,
             "settings":{
                "avatar_url":"some avatar",
-               "url":"some url",
                "use_discord_username":true
             },
-            "secureSettings":{}
+            "secureSettings":{
+     		  "url":"some url"
+            }
          }
       ]
    },

pkg/services/ngalert/notifier/alertmanager_config.go

@@ -12,6 +12,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
+	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/util"
 )
 
@@ -151,6 +152,16 @@ func (moa *MultiOrgAlertmanager) gettableUserConfigFromAMConfigString(ctx contex
 			Config: cfg.AlertmanagerConfig.Config,
 		},
 	}
+
+	// First we encrypt the secure settings.
+	// This is done to ensure that any secure settings incorrectly stored in Settings are encrypted and moved to
+	// SecureSettings. This can happen if an integration definition is updated to make a field secure.
+	if err := EncryptReceiverConfigSettings(cfg.AlertmanagerConfig.Receivers, func(ctx context.Context, payload []byte) ([]byte, error) {
+		return moa.Crypto.Encrypt(ctx, payload, secrets.WithoutScope())
+	}); err != nil {
+		return definitions.GettableUserConfig{}, fmt.Errorf("failed to encrypt receivers: %w", err)
+	}
+
 	for _, recv := range cfg.AlertmanagerConfig.Receivers {
 		receivers := make([]*definitions.GettableGrafanaReceiver, 0, len(recv.PostableGrafanaReceivers.GrafanaManagedReceivers))
 		for _, pr := range recv.PostableGrafanaReceivers.GrafanaManagedReceivers {

pkg/services/ngalert/notifier/crypto.go

@@ -3,11 +3,13 @@ package notifier
 import (
 	"context"
 	"encoding/base64"
+	"encoding/json"
 	"errors"
 	"fmt"
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
+	"github.com/grafana/grafana/pkg/services/ngalert/notifier/channels_config"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
 	"github.com/grafana/grafana/pkg/services/secrets"
 )
@@ -57,17 +59,90 @@ func (c *alertmanagerCrypto) ProcessSecureSettings(ctx context.Context, orgId in
 
 // EncryptReceiverConfigs encrypts all SecureSettings in the given receivers.
 func EncryptReceiverConfigs(c []*definitions.PostableApiReceiver, encrypt definitions.EncryptFn) error {
+	return encryptReceiverConfigs(c, encrypt, true)
+}
+
+func EncryptReceiverConfigSettings(c []*definitions.PostableApiReceiver, encrypt definitions.EncryptFn) error {
+	return encryptReceiverConfigs(c, encrypt, false)
+}
+
+// encryptReceiverConfigs encrypts all SecureSettings in the given receivers.
+// encryptExisting determines whether to encrypt existing secure settings.
+func encryptReceiverConfigs(c []*definitions.PostableApiReceiver, encrypt definitions.EncryptFn, encryptExisting bool) error {
 	// encrypt secure settings for storing them in DB
 	for _, r := range c {
 		switch r.Type() {
 		case definitions.GrafanaReceiverType:
 			for _, gr := range r.PostableGrafanaReceivers.GrafanaManagedReceivers {
-				for k, v := range gr.SecureSettings {
+				if encryptExisting {
-					encryptedData, err := encrypt(context.Background(), []byte(v))
+					for k, v := range gr.SecureSettings {
+						encryptedData, err := encrypt(context.Background(), []byte(v))
+						if err != nil {
+							return fmt.Errorf("failed to encrypt secure settings: %w", err)
+						}
+						gr.SecureSettings[k] = base64.StdEncoding.EncodeToString(encryptedData)
+					}
+				}
+
+				if len(gr.Settings) > 0 {
+					// We need to parse the settings to check for secret keys. If we find any, we encrypt them and
+					// store them in SecureSettings. This can happen from incorrect configuration or when an integration
+					// definition is updated to make a field secure.
+					settings := make(map[string]any)
+					if err := json.Unmarshal(gr.Settings, &settings); err != nil {
+						return fmt.Errorf("integration '%s' of receiver '%s' has settings that cannot be parsed as JSON: %w", gr.Type, gr.Name, err)
+					}
+
+					secretKeys, err := channels_config.GetSecretKeysForContactPointType(gr.Type)
 					if err != nil {
-						return fmt.Errorf("failed to encrypt secure settings: %w", err)
+						return fmt.Errorf("failed to get secret keys for contact point type %s: %w", gr.Type, err)
+					}
+
+					secureSettings := gr.SecureSettings
+					if secureSettings == nil {
+						secureSettings = make(map[string]string)
+					}
+
+					settingsChanged := false
+					secureSettingsChanged := false
+					for _, secretKey := range secretKeys {
+						settingsValue, ok := settings[secretKey]
+						if !ok {
+							continue
+						}
+
+						// Secrets should not be stored in settings regardless.
+						delete(settings, secretKey)
+						settingsChanged = true
+
+						// If the secret is already encrypted, we don't need to encrypt it again.
+						if _, ok := secureSettings[secretKey]; ok {
+							continue
+						}
+
+						if strVal, isString := settingsValue.(string); isString {
+							encrypted, err := encrypt(context.Background(), []byte(strVal))
+							if err != nil {
+								return fmt.Errorf("failed to encrypt secure settings: %w", err)
+							}
+							secureSettings[secretKey] = base64.StdEncoding.EncodeToString(encrypted)
+							secureSettingsChanged = true
+						}
+					}
+
+					// Defensive checks to limit the risk of unintentional edge case changes in this legacy API.
+					if settingsChanged {
+						// If we removed any secret keys from settings, we need to save the updated settings.
+						jsonBytes, err := json.Marshal(settings)
+						if err != nil {
+							return err
+						}
+						gr.Settings = jsonBytes
+					}
+					if secureSettingsChanged {
+						// If we added any secure settings, we need to save the updated secure settings.
+						gr.SecureSettings = secureSettings
 					}
-					gr.SecureSettings[k] = base64.StdEncoding.EncodeToString(encryptedData)
 				}
 			}
 		default:
@@ -94,6 +169,14 @@ func (c *alertmanagerCrypto) LoadSecureSettings(ctx context.Context, orgId int64
 		if err != nil {
 			c.log.Warn("Last known alertmanager configuration was invalid. Overwriting...")
 		} else {
+			// First we encrypt the secure settings in the existing configuration.
+			// This is done to ensure that any secure settings incorrectly stored in Settings are encrypted and moved to
+			// SecureSettings. This can happen if an integration definition is updated to make a field secure.
+			if err := EncryptReceiverConfigSettings(currentConfig.AlertmanagerConfig.Receivers, func(ctx context.Context, payload []byte) ([]byte, error) {
+				return c.Encrypt(ctx, payload, secrets.WithoutScope())
+			}); err != nil {
+				return fmt.Errorf("failed to encrypt receivers: %w", err)
+			}
 			currentReceiverMap = currentConfig.GetGrafanaReceiverMap()
 		}
 	}

pkg/tests/api/alerting/api_notification_channel_test.go

@@ -2168,10 +2168,8 @@ var expAlertmanagerConfigFromAPI = `
             "name": "discord_test",
             "type": "discord",
             "disableResolveMessage": false,
-            "settings": {
+            "settings": {},
-              "url": "http://CHANNEL_ADDR/discord_recv/discord_test"
+            "secureFields": {"url": true}
-            },
-            "secureFields": {}
           }
         ]
       },
@@ -2214,10 +2212,8 @@ var expAlertmanagerConfigFromAPI = `
             "name": "victorops_test",
             "type": "victorops",
             "disableResolveMessage": false,
-            "settings": {
+            "settings": {},
-              "url": "http://CHANNEL_ADDR/victorops_recv/victorops_test"
+            "secureFields": {"url": true}
-            },
-            "secureFields": {}
           }
         ]
       },