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

pkg/services/ngalert/api/api_alertmanager_test.go

@@ -3,6 +3,7 @@ package api
 import (
 	"context"
 	"crypto/md5"
+	"encoding/base64"
 	"encoding/json"
 	"net/http"
 	"testing"
@@ -12,6 +13,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"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/services/ngalert/accesscontrol"
@@ -259,6 +261,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/notifier/alertmanager_config.go

@@ -16,6 +16,7 @@ import (
 	"github.com/grafana/grafana/pkg/services/ngalert/models"
 	"github.com/grafana/grafana/pkg/services/ngalert/notifier/legacy_storage"
 	"github.com/grafana/grafana/pkg/services/ngalert/store"
+	"github.com/grafana/grafana/pkg/services/secrets"
 	"github.com/grafana/grafana/pkg/util"
 )
 
@@ -223,6 +224,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 {
-					if err != nil {
+						encryptedData, err := encrypt(context.Background(), []byte(v))
-						return fmt.Errorf("failed to encrypt secure settings: %w", err)
+						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 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

@@ -2161,10 +2161,8 @@ var expAlertmanagerConfigFromAPI = `
             "name": "discord_test",
             "type": "discord",
             "disableResolveMessage": false,
-            "settings": {
+            "settings": {},
-              "url": "http://CHANNEL_ADDR/discord_recv/discord_test"
+            "secureFields": {"url": true}
-            },
-            "secureFields": {}
           }
         ]
       },
@@ -2176,10 +2174,8 @@ var expAlertmanagerConfigFromAPI = `
             "name": "googlechat_test",
             "type": "googlechat",
             "disableResolveMessage": false,
-            "settings": {
+            "settings": {},
-              "url": "http://CHANNEL_ADDR/googlechat_recv/googlechat_test"
+            "secureFields": {"url": true}
-            },
-            "secureFields": {}
           }
         ]
       },
@@ -2207,10 +2203,8 @@ var expAlertmanagerConfigFromAPI = `
             "name": "victorops_test",
             "type": "victorops",
             "disableResolveMessage": false,
-            "settings": {
+            "settings": {},
-              "url": "http://CHANNEL_ADDR/victorops_recv/victorops_test"
+            "secureFields": {"url": true}
-            },
-            "secureFields": {}
           }
         ]
       },