Information disclosure (secret handling/case-insensitive key processing)

PoC (conceptual): - Scenario: A receiver integration has secret settings defined with two keys that differ only by case, both containing sensitive values, e.g. settings = {"apiKey": "secret1", "APIKEY": "secret2"}. - Before this fix, the encryption path would locate one of the keys (depending on map iteration order), move that value to secure settings (encrypted), and delete the matched key from plain settings, leaving the other key still present in plain text in the settings. An attacker or an observer with access to the unencrypted settings could observe the remaining plaintext secret. - Steps to reproduce (conceptual): 1) Build a PostableGrafanaReceiver with Settings containing both "apiKey" and "APIKEY" with secret values. 2) Run EncryptReceiverConfigSettings with a placeholder encryptor (that returns the payload or a base64-encoded fake ciphertext). 3) Inspect the result: one of the keys is moved to SecureSettings (encrypted), but the other key remains in Settings as plaintext, exposing a secret. - Expected after the fix: The system rejects duplicates that differ only by casing (validation detects the mixed-case keys) and prevents the insecure configuration from being accepted at provisioning/update time. - What changes this demonstrates: The vulnerability is information disclosure risk due to case-sensitive handling allowing mixed-case secret keys to be processed inconsistently. The fix enforces case-insensitive uniqueness and proper redaction/encryption of secret fields. Note: The PoC demonstrates the vulnerability path and the resulting plaintext exposure prior to the new validation enforcing case-insensitive uniqueness; the production fix would reject such configurations altogether.

diff --git a/pkg/services/ngalert/models/receivers.go b/pkg/services/ngalert/models/receivers.go index 3d8c2d173b5cc..20cb98a9e9cfe 100644 --- a/pkg/services/ngalert/models/receivers.go +++ b/pkg/services/ngalert/models/receivers.go @@ -10,6 +10,7 @@ import ( "math" "slices" "sort" + "strings" "github.com/grafana/alerting/models" alertingNotify "github.com/grafana/alerting/notify" @@ -217,7 +218,7 @@ func (integration *Integration) Encrypt(encryptFn EncryptFn) error { } var errs []error for _, path := range secretFieldPaths { - unencryptedSecureValue, ok, err := extractField(integration.Settings, path) + unencryptedSecureValue, ok, err := extractFieldCaseInsensitive(integration.Settings, path) if err != nil { errs = append(errs, fmt.Errorf("failed to extract secret field by path '%s': %w", path, err)) } @@ -236,8 +237,21 @@ func (integration *Integration) Encrypt(encryptFn EncryptFn) error { return errors.Join(errs...) } -func extractField(settings map[string]any, path schema.IntegrationFieldPath) (string, bool, error) { - val, ok := settings[path.Head()] +func MapGetCaseInsensitive(settings map[string]any, key string) (string, any, bool) { + if val, ok := settings[key]; ok { + return key, val, true + } + + for k := range settings { + if strings.EqualFold(k, key) { + return k, settings[k], true + } + } + return "", nil, false +} + +func extractFieldCaseInsensitive(settings map[string]any, path schema.IntegrationFieldPath) (string, bool, error) { + key, val, ok := MapGetCaseInsensitive(settings, path.Head()) if !ok { return "", false, nil } @@ -246,14 +260,14 @@ func extractField(settings map[string]any, path schema.IntegrationFieldPath) (st if !ok { return "", false, fmt.Errorf("expected string but got %T", val) } - delete(settings, path.Head()) + delete(settings, key) return secret, true, nil } sub, ok := val.(map[string]any) if !ok { return "", false, fmt.Errorf("expected nested object but got %T", val) } - return extractField(sub, path.Tail()) + return extractFieldCaseInsensitive(sub, path.Tail()) } func getFieldValue(settings map[string]any, path schema.IntegrationFieldPath) (any, bool) { diff --git a/pkg/services/ngalert/models/receivers_test.go b/pkg/services/ngalert/models/receivers_test.go index 3dcd5a2507bcc..48d2284e5e7ad 100644 --- a/pkg/services/ngalert/models/receivers_test.go +++ b/pkg/services/ngalert/models/receivers_test.go @@ -47,7 +47,7 @@ func TestReceiver_EncryptDecrypt(t *testing.T) { typeVersion, ok := alertingNotify.GetSchemaVersionForIntegration(integrationType, schema.V1) require.True(t, ok) for _, key := range typeVersion.GetSecretFieldsPaths() { - val, ok, err := extractField(encrypted.Settings, key) + val, ok, err := extractFieldCaseInsensitive(encrypted.Settings, key) assert.NoError(t, err) if ok { encryptedVal, err := encryptFn(val) @@ -275,7 +275,7 @@ func TestIntegration_SecureFields(t *testing.T) { if validIntegration.Config.IsSecureField(path) { expected[path.String()] = true validIntegration.SecureSettings[path.String()] = "test" - _, _, err := extractField(validIntegration.Settings, path) + _, _, err := extractFieldCaseInsensitive(validIntegration.Settings, path) require.NoError(t, err) continue } @@ -413,3 +413,72 @@ func TestReceiver_Fingerprint(t *testing.T) { } }) } + +func TestExtractFieldCaseInsensitive(t *testing.T) { + t.Run("exact match", func(t *testing.T) { + settings := map[string]any{ + "apiKey": "test", + } + + field, ok, err := extractFieldCaseInsensitive(settings, schema.ParseIntegrationPath("apiKey")) + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "test", field) + + _, ok = settings["apiKey"] + assert.False(t, ok) + }) + + t.Run("case-insensitive match", func(t *testing.T) { + settings := map[string]any{ + "APIKEY": "test", + } + + field, ok, err := extractFieldCaseInsensitive(settings, schema.ParseIntegrationPath("apiKey")) + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "test", field) + + _, ok = settings["APIKEY"] + assert.False(t, ok) + }) + + t.Run("no match", func(t *testing.T) { + settings := map[string]any{ + "someKey": "test", + } + + field, ok, err := extractFieldCaseInsensitive(settings, schema.ParseIntegrationPath("apiKey")) + require.NoError(t, err) + assert.False(t, ok) + assert.Equal(t, "", field) + + _, ok = settings["someKey"] + assert.True(t, ok) + }) + + t.Run("nested match", func(t *testing.T) { + settings := map[string]any{ + "nested": map[string]any{ + "apiKey": "test", + "auth_key": "test2", + }, + } + + field, ok, err := extractFieldCaseInsensitive(settings, schema.ParseIntegrationPath("nested.apiKey")) + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "test", field) + + _, ok = settings["nested"].(map[string]any)["apiKey"] + assert.False(t, ok) + + field, ok, err = extractFieldCaseInsensitive(settings, schema.ParseIntegrationPath("nested.AUTH_KEY")) + require.NoError(t, err) + assert.True(t, ok) + assert.Equal(t, "test2", field) + + _, ok = settings["nested"].(map[string]any)["auth_key"] + assert.False(t, ok) + }) +} diff --git a/pkg/services/ngalert/notifier/crypto.go b/pkg/services/ngalert/notifier/crypto.go index 50df44100e517..da7f3d4c1acc9 100644 --- a/pkg/services/ngalert/notifier/crypto.go +++ b/pkg/services/ngalert/notifier/crypto.go @@ -107,8 +107,8 @@ func encryptReceiverConfigs(c []*v1.PostableApiReceiver, encrypt definitions.Enc settingsChanged := false secureSettingsChanged := false for _, secretPath := range secretPaths { - secretKey := secretPath.String() - settingsValue, ok := settings[secretKey] + schemaKey := secretPath.String() + secretKey, settingsValue, ok := models.MapGetCaseInsensitive(settings, schemaKey) if !ok { continue } diff --git a/pkg/services/ngalert/notifier/crypto_test.go b/pkg/services/ngalert/notifier/crypto_test.go index 898feacabf341..387013982dfaa 100644 --- a/pkg/services/ngalert/notifier/crypto_test.go +++ b/pkg/services/ngalert/notifier/crypto_test.go @@ -3,10 +3,12 @@ package notifier import ( "context" "encoding/base64" + "encoding/json" "testing" "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" v1 "github.com/grafana/grafana/pkg/services/ngalert/notifier/legacy_storage/v1" "github.com/grafana/grafana/pkg/services/secrets/fakes" ) @@ -55,6 +57,81 @@ func TestEncryptExtraConfigs(t *testing.T) { } } +func TestEncryptReceiverConfigSettings_MovesSecretsFromSettings(t *testing.T) { + // Identity "encryption" so we can assert on the stored value (base64 of the plaintext). + encrypt := func(_ context.Context, payload []byte) ([]byte, error) { + return payload, nil + } + + const secretValue = "my-integration-key" + encryptedSecret := base64.StdEncoding.EncodeToString([]byte(secretValue)) + + tests := []struct { + name string + settings string + secureSettings map[string]string + expectedSecureSettings map[string]string + expectedSettingsKeys []string + removedSettingsKeys []string + }{ + { + name: "secret with exact schema casing is moved to secure settings", + settings: `{"integrationKey":"my-integration-key","severity":"critical"}`, + expectedSecureSettings: map[string]string{"integrationKey": encryptedSecret}, + expectedSettingsKeys: []string{"severity"}, + removedSettingsKeys: []string{"integrationKey"}, + }, + { + name: "secret with non-canonical casing is matched case-insensitively and moved", + settings: `{"integrationkey":"my-integration-key","severity":"critical"}`, + expectedSecureSettings: map[string]string{"integrationkey": encryptedSecret}, + expectedSettingsKeys: []string{"severity"}, + removedSettingsKeys: []string{"integrationkey"}, + }, + { + name: "secret already present in secure settings is removed from settings but not re-encrypted", + settings: `{"INTEGRATIONKEY":"my-integration-key"}`, + secureSettings: map[string]string{"INTEGRATIONKEY": "already-encrypted"}, + expectedSecureSettings: map[string]string{"INTEGRATIONKEY": "already-encrypted"}, + removedSettingsKeys: []string{"INTEGRATIONKEY"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gr := &v1.PostableGrafanaReceiver{ + UID: "uid", + Name: "pd", + Type: "pagerduty", + Settings: definitions.RawMessage(tt.settings), + SecureSettings: tt.secureSettings, + } + receivers := []*v1.PostableApiReceiver{ + { + Receiver: definitions.Receiver{Name: "pd"}, + PostableGrafanaReceivers: v1.PostableGrafanaReceivers{ + GrafanaManagedReceivers: []*v1.PostableGrafanaReceiver{gr}, + }, + }, + } + + err := EncryptReceiverConfigSettings(receivers, encrypt) + require.NoError(t, err) + + require.Equal(t, tt.expectedSecureSettings, gr.SecureSettings) + + parsed := map[string]any{} + require.NoError(t, json.Unmarshal(gr.Settings, &parsed)) + for _, k := range tt.expectedSettingsKeys { + require.Contains(t, parsed, k) + } + for _, k := range tt.removedSettingsKeys { + require.NotContains(t, parsed, k) + } + }) + } +} + func TestDecryptExtraConfigs(t *testing.T) { decryptedData := "derypted-data" decryptedDataBase64 := base64.StdEncoding.EncodeToString([]byte(decryptedData)) diff --git a/pkg/services/ngalert/notifier/receiver_svc.go b/pkg/services/ngalert/notifier/receiver_svc.go index cd3ae56a95ae1..e2dcd95b17b14 100644 --- a/pkg/services/ngalert/notifier/receiver_svc.go +++ b/pkg/services/ngalert/notifier/receiver_svc.go @@ -9,6 +9,7 @@ import ( "strings" "github.com/grafana/alerting/receivers/schema" + "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -393,6 +394,11 @@ func (rs *ReceiverService) CreateReceiver(ctx context.Context, r *models.Receive span.AddEvent("Loaded Alertmanager configuration", trace.WithAttributes(attribute.String("concurrency_token", revision.ConcurrencyToken))) createdReceiver := r.Clone() + for _, integration := range createdReceiver.Integrations { + if err := rs.validateNoDuplicateSecretFields(integration.Config, integration.Settings); err != nil { + return nil, models.ErrReceiverInvalid(err) + } + } err = createdReceiver.Encrypt(rs.encryptor(ctx)) if err != nil { return nil, err @@ -514,11 +520,17 @@ func (rs *ReceiverService) UpdateReceiver(ctx context.Context, r *models.Receive } } - // We need to perform two important steps to process settings on an updated integration: - // 1. Encrypt new or updated secret fields as they will arrive in plain text. - // 2. For updates, callers do not re-send unchanged secure settings and instead mark them in SecureFields. We need - // to load these secure settings from the existing integration. + // We need to perform three important steps to process settings on an updated integration: + // 1. Validate no duplicate settings field exists (could happen if the user uses different casing) + // 2. Encrypt new or updated secret fields as they will arrive in plain text. + // 3. For updates, callers do not re-send unchanged secure settings and instead mark them in SecureFields. We need + // to load these secure settings from the existing integration. updatedReceiver := r.Clone() + for _, integration := range updatedReceiver.Integrations { + if err := rs.validateNoDuplicateSecretFields(integration.Config, integration.Settings); err != nil { + return nil, models.ErrReceiverInvalid(err) + } + } err = updatedReceiver.Encrypt(rs.encryptor(ctx)) if err != nil { return nil, err @@ -881,3 +893,29 @@ func (rs *ReceiverService) validateReceiver(ctx context.Context, orgID int64, re } return nil } + +func (rs *ReceiverService) validateNoDuplicateSecretFields(typeSchema schema.IntegrationSchemaVersion, settings map[string]any) error { + for _, secretPath := range typeSchema.GetSecretFieldsPaths() { + node := settings + for _, segment := range secretPath { + var matches []string + for k := range node { + if strings.EqualFold(k, segment) { + matches = append(matches, k) + } + } + if len(matches) == 0 { + break + } + if len(matches) > 1 { + return fmt.Errorf("duplicate keys found for secret field %s", secretPath.String()) + } + next, ok := node[matches[0]].(map[string]any) + if !ok { + break + } + node = next + } + } + return nil +} diff --git a/pkg/services/ngalert/notifier/receiver_svc_test.go b/pkg/services/ngalert/notifier/receiver_svc_test.go index 437697d3b8a30..22628a76ef8de 100644 --- a/pkg/services/ngalert/notifier/receiver_svc_test.go +++ b/pkg/services/ngalert/notifier/receiver_svc_test.go @@ -512,6 +512,14 @@ func TestReceiverService_Create(t *testing.T) { receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithInvalidIntegration("slack")), expectedErr: models.ErrReceiverInvalidBase, }, + { + name: "create with case-only duplicate of secret field fails", + user: writer, + receiver: models.CopyReceiverWith(baseReceiver, models.ReceiverMuts.WithIntegrations( + models.CopyIntegrationWith(slackIntegration, models.IntegrationMuts.AddSetting("TOKEN", "duplicate")), + )), + expectedErr: models.ErrReceiverInvalidBase, + }, { name: "create integration with no normal settings should not store nil settings", user: writer, @@ -869,6 +877,15 @@ func TestReceiverService_Update(t *testing.T) { existing: new(baseReceiver.Clone()), expectedErr: models.ErrReceiverInvalidBase, }, + { + name: "update with case-only duplicate of secret field fails", + user: writer, + receiver: models.CopyReceiverWith(baseReceiver, rm.WithIntegrations( + models.CopyIntegrationWith(slackIntegration, im.AddSetting("TOKEN", "duplicate")), + )), + existing: new(baseReceiver.Clone()), + expectedErr: models.ErrReceiverInvalidBase, + }, { name: "receivers with non-Grafana origin are not accepted", user: writer, @@ -1129,6 +1146,95 @@ func TestReceiverService_UpdateReceiverName(t *testing.T) { }) } +func TestReceiverService_validateNoDuplicateSecretFields(t *testing.T) { + // The method does not depend on any ReceiverService state, only on the + // integration schema lookup, so a zero value receiver is sufficient. + rs := &ReceiverService{} + + cases := []struct { + name string + integration string + settings map[string]any + wantErr string // empty means no error expected + }{ + { + name: "canonical secret key passes", + integration: "slack", + settings: map[string]any{"recipient": "#chan", "token": "secret" ... [truncated]