diff --git a/pkg/services/ngalert/api/api_alertmanager_test.go b/pkg/services/ngalert/api/api_alertmanager_test.go index 302dc468788..68a3be8fb89 100644 --- a/pkg/services/ngalert/api/api_alertmanager_test.go +++ b/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) diff --git a/pkg/services/ngalert/notifier/alertmanager_config.go b/pkg/services/ngalert/notifier/alertmanager_config.go index c2248fe91c6..2109a975b1b 100644 --- a/pkg/services/ngalert/notifier/alertmanager_config.go +++ b/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 { diff --git a/pkg/services/ngalert/notifier/crypto.go b/pkg/services/ngalert/notifier/crypto.go index f710181f45c..221e9094943 100644 --- a/pkg/services/ngalert/notifier/crypto.go +++ b/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 { - encryptedData, err := encrypt(context.Background(), []byte(v)) + if encryptExisting { + 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() } } diff --git a/pkg/tests/api/alerting/api_notification_channel_test.go b/pkg/tests/api/alerting/api_notification_channel_test.go index 4753d915446..b653b3a8184 100644 --- a/pkg/tests/api/alerting/api_notification_channel_test.go +++ b/pkg/tests/api/alerting/api_notification_channel_test.go @@ -2161,10 +2161,8 @@ var expAlertmanagerConfigFromAPI = ` "name": "discord_test", "type": "discord", "disableResolveMessage": false, - "settings": { - "url": "http://CHANNEL_ADDR/discord_recv/discord_test" - }, - "secureFields": {} + "settings": {}, + "secureFields": {"url": true} } ] }, @@ -2176,10 +2174,8 @@ var expAlertmanagerConfigFromAPI = ` "name": "googlechat_test", "type": "googlechat", "disableResolveMessage": false, - "settings": { - "url": "http://CHANNEL_ADDR/googlechat_recv/googlechat_test" - }, - "secureFields": {} + "settings": {}, + "secureFields": {"url": true} } ] }, @@ -2207,10 +2203,8 @@ var expAlertmanagerConfigFromAPI = ` "name": "victorops_test", "type": "victorops", "disableResolveMessage": false, - "settings": { - "url": "http://CHANNEL_ADDR/victorops_recv/victorops_test" - }, - "secureFields": {} + "settings": {}, + "secureFields": {"url": true} } ] },