diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 39d46e20fd6..0ef3a17f0f0 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -96,10 +96,11 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) { api.DatasourceCache, NewLotexAM(proxy, logger), &AlertmanagerSrv{ - crypto: api.MultiOrgAlertmanager.Crypto, - log: logger, - ac: api.AccessControl, - mam: api.MultiOrgAlertmanager, + crypto: api.MultiOrgAlertmanager.Crypto, + log: logger, + ac: api.AccessControl, + mam: api.MultiOrgAlertmanager, + featureManager: api.FeatureManager, silenceSvc: notifier.NewSilenceService( accesscontrol.NewSilenceService(api.AccessControl, api.RuleStore), api.TransactionManager, diff --git a/pkg/services/ngalert/api/api_alertmanager.go b/pkg/services/ngalert/api/api_alertmanager.go index 4d9f3bec5a5..ff2cd001821 100644 --- a/pkg/services/ngalert/api/api_alertmanager.go +++ b/pkg/services/ngalert/api/api_alertmanager.go @@ -15,6 +15,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/featuremgmt" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/notifier" "github.com/grafana/grafana/pkg/services/ngalert/store" @@ -28,11 +29,12 @@ const ( ) type AlertmanagerSrv struct { - log log.Logger - ac accesscontrol.AccessControl - mam *notifier.MultiOrgAlertmanager - crypto notifier.Crypto - silenceSvc SilenceService + log log.Logger + ac accesscontrol.AccessControl + mam *notifier.MultiOrgAlertmanager + crypto notifier.Crypto + silenceSvc SilenceService + featureManager featuremgmt.FeatureToggles } type UnknownReceiverError struct { @@ -195,6 +197,18 @@ func (srv AlertmanagerSrv) RoutePostAlertingConfig(c *contextmodel.ReqContext, b return ErrResp(http.StatusBadRequest, err, "") } } + if srv.featureManager.IsEnabled(c.Req.Context(), featuremgmt.FlagAlertingApiServer) { + if err != nil { + // Unclear if returning an error here is the right thing to do, preventing the user from posting a new config + // when the current one is legitimately invalid is not optimal, but we need to ensure receiver + // permissions are maintained and prevent potential access control bypasses. The workaround is to use the + // various new k8s API endpoints to fix the configuration. + return ErrResp(http.StatusInternalServerError, err, "") + } + if err := srv.k8sApiServiceGuard(currentConfig, body); err != nil { + return ErrResp(http.StatusBadRequest, err, "") + } + } err = srv.mam.SaveAndApplyAlertmanagerConfiguration(c.Req.Context(), c.SignedInUser.GetOrgID(), body) if err == nil { return response.JSON(http.StatusAccepted, util.DynMap{"message": "configuration created"}) diff --git a/pkg/services/ngalert/api/api_alertmanager_guards.go b/pkg/services/ngalert/api/api_alertmanager_guards.go index bc32c3f5291..1c130e0fac6 100644 --- a/pkg/services/ngalert/api/api_alertmanager_guards.go +++ b/pkg/services/ngalert/api/api_alertmanager_guards.go @@ -32,6 +32,27 @@ func (srv AlertmanagerSrv) provenanceGuard(currentConfig apimodels.GettableUserC return nil } +func (srv AlertmanagerSrv) k8sApiServiceGuard(currentConfig apimodels.GettableUserConfig, newConfig apimodels.PostableUserConfig) error { + // Modifications to receivers via this API is tricky with new per-receiver RBAC. Assuming we restrict the API to only + // those users with global edit permissions, we would still need to consider the following: + // - Since the UIDs stored in the database for the purposes of per-receiver RBAC are generated based on the receiver + // name, we would need to ensure continuity of permissions when a receiver is renamed. This would, preferably, + // require detecting renames and updating the permissions UID in the database. + // - It would need to determine newly created and deleted receivers so it can populate per-receiver access control defaults. + + // Neither of these are insurmountable, but considering this endpoint will be removed once FlagAlertingApiServer + // becomes GA, the complexity may not be worthwhile. To that end, for now we reject any request that attempts to + // modify receivers. + delta, err := calculateReceiversDelta(currentConfig.AlertmanagerConfig.Receivers, newConfig.AlertmanagerConfig.Receivers) + if err != nil { + return err + } + if !delta.IsEmpty() { + return fmt.Errorf("cannot modify receivers using this API while per-receiver RBAC is enabled; either disable the `alertingApiServer` feature flag or use an API that supports per-receiver RBAC (e.g. provisioning or receivers API)") + } + return nil +} + func checkRoutes(currentConfig apimodels.GettableUserConfig, newConfig apimodels.PostableUserConfig) error { reporter := cmputil.DiffReporter{} options := []cmp.Option{cmp.Reporter(&reporter), cmpopts.EquateEmpty(), cmpopts.IgnoreUnexported(labels.Matcher{})} diff --git a/pkg/services/ngalert/api/api_alertmanager_test.go b/pkg/services/ngalert/api/api_alertmanager_test.go index 14676789294..ed064df3ff5 100644 --- a/pkg/services/ngalert/api/api_alertmanager_test.go +++ b/pkg/services/ngalert/api/api_alertmanager_test.go @@ -569,11 +569,12 @@ func createSut(t *testing.T) AlertmanagerSrv { ruleStore := ngfakes.NewRuleStore(t) ruleAuthzService := accesscontrol.NewRuleService(acimpl.ProvideAccessControl(featuremgmt.WithFeatures(), zanzana.NewNoopClient())) return AlertmanagerSrv{ - mam: mam, - crypto: mam.Crypto, - ac: ac, - log: log, - silenceSvc: notifier.NewSilenceService(accesscontrol.NewSilenceService(ac, ruleStore), ruleStore, log, mam, ruleStore, ruleAuthzService), + mam: mam, + crypto: mam.Crypto, + ac: ac, + log: log, + featureManager: featuremgmt.WithFeatures(), + silenceSvc: notifier.NewSilenceService(accesscontrol.NewSilenceService(ac, ruleStore), ruleStore, log, mam, ruleStore, ruleAuthzService), } } diff --git a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go index 03d849056e4..4a2bd218020 100644 --- a/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go +++ b/pkg/tests/apis/alerting/notifications/receivers/receiver_test.go @@ -24,6 +24,7 @@ import ( common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" "github.com/grafana/grafana/pkg/apis/alerting_notifications/v0alpha1" "github.com/grafana/grafana/pkg/generated/clientset/versioned" + notificationsv0alpha1 "github.com/grafana/grafana/pkg/generated/clientset/versioned/typed/alerting_notifications/v0alpha1" "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/accesscontrol/acimpl" "github.com/grafana/grafana/pkg/services/accesscontrol/resourcepermissions" @@ -426,6 +427,10 @@ func TestIntegrationInUseMetadata(t *testing.T) { cliCfg := helper.Org1.Admin.NewRestConfig() legacyCli := alerting.NewAlertingLegacyAPIClient(helper.GetEnv().Server.HTTPServer.Listener.Addr().String(), cliCfg.Username, cliCfg.Password) + adminK8sClient, err := versioned.NewForConfig(cliCfg) + require.NoError(t, err) + adminClient := adminK8sClient.NotificationsV0alpha1().Receivers("default") + // Prepare environment and create notification policy and rule that use receiver alertmanagerRaw, err := testData.ReadFile(path.Join("test-data", "notification-settings.json")) require.NoError(t, err) @@ -440,8 +445,7 @@ func TestIntegrationInUseMetadata(t *testing.T) { parentRoute.Routes = []*definitions.Route{&route1, &route2} amConfig.AlertmanagerConfig.Route.Routes = append(amConfig.AlertmanagerConfig.Route.Routes, &parentRoute) - success, err := legacyCli.PostConfiguration(t, amConfig) - require.Truef(t, success, "Failed to post Alertmanager configuration: %s", err) + persistInitialConfig(t, amConfig, adminClient, legacyCli) postGroupRaw, err := testData.ReadFile(path.Join("test-data", "rulegroup-1.json")) require.NoError(t, err) @@ -471,10 +475,6 @@ func TestIntegrationInUseMetadata(t *testing.T) { _, status, data := legacyCli.PostRulesGroupWithStatus(t, folderUID, &ruleGroup) require.Equalf(t, http.StatusAccepted, status, "Failed to post Rule: %s", data) - adminK8sClient, err := versioned.NewForConfig(cliCfg) - require.NoError(t, err) - adminClient := adminK8sClient.NotificationsV0alpha1().Receivers("default") - requestReceivers := func(t *testing.T, title string) (v0alpha1.Receiver, v0alpha1.Receiver) { t.Helper() receivers, err := adminClient.List(ctx, v1.ListOptions{}) @@ -508,7 +508,7 @@ func TestIntegrationInUseMetadata(t *testing.T) { // Removing the new extra route should leave only 1. amConfig.AlertmanagerConfig.Route.Routes = amConfig.AlertmanagerConfig.Route.Routes[:1] - success, err = legacyCli.PostConfiguration(t, amConfig) + success, err := legacyCli.PostConfiguration(t, amConfig) require.Truef(t, success, "Failed to post Alertmanager configuration: %s", err) receiverListed, receiverGet = requestReceivers(t, "user-defined") @@ -786,6 +786,26 @@ func TestIntegrationPatch(t *testing.T) { }) } +func TestIntegrationRejectConfigApiReceiverModification(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test") + } + + helper := getTestHelper(t) + + cliCfg := helper.Org1.Admin.NewRestConfig() + legacyCli := alerting.NewAlertingLegacyAPIClient(helper.GetEnv().Server.HTTPServer.Listener.Addr().String(), cliCfg.Username, cliCfg.Password) + + // This config has new and modified receivers. + alertmanagerRaw, err := testData.ReadFile(path.Join("test-data", "notification-settings.json")) + require.NoError(t, err) + var amConfig definitions.PostableUserConfig + require.NoError(t, json.Unmarshal(alertmanagerRaw, &amConfig)) + + success, err := legacyCli.PostConfiguration(t, amConfig) + require.Falsef(t, success, "Expected receiver modification to be rejected, but got %s", err) +} + func TestIntegrationReferentialIntegrity(t *testing.T) { if testing.Short() { t.Skip("skipping integration test") @@ -802,14 +822,17 @@ func TestIntegrationReferentialIntegrity(t *testing.T) { cliCfg := helper.Org1.Admin.NewRestConfig() legacyCli := alerting.NewAlertingLegacyAPIClient(helper.GetEnv().Server.HTTPServer.Listener.Addr().String(), cliCfg.Username, cliCfg.Password) + adminK8sClient, err := versioned.NewForConfig(cliCfg) + require.NoError(t, err) + adminClient := adminK8sClient.NotificationsV0alpha1().Receivers("default") + // Prepare environment and create notification policy and rule that use time receiver alertmanagerRaw, err := testData.ReadFile(path.Join("test-data", "notification-settings.json")) require.NoError(t, err) var amConfig definitions.PostableUserConfig require.NoError(t, json.Unmarshal(alertmanagerRaw, &amConfig)) - success, err := legacyCli.PostConfiguration(t, amConfig) - require.Truef(t, success, "Failed to post Alertmanager configuration: %s", err) + persistInitialConfig(t, amConfig, adminClient, legacyCli) postGroupRaw, err := testData.ReadFile(path.Join("test-data", "rulegroup-1.json")) require.NoError(t, err) @@ -821,10 +844,6 @@ func TestIntegrationReferentialIntegrity(t *testing.T) { _, status, data := legacyCli.PostRulesGroupWithStatus(t, folderUID, &ruleGroup) require.Equalf(t, http.StatusAccepted, status, "Failed to post Rule: %s", data) - adminK8sClient, err := versioned.NewForConfig(cliCfg) - require.NoError(t, err) - adminClient := adminK8sClient.NotificationsV0alpha1().Receivers("default") - receivers, err := adminClient.List(ctx, v1.ListOptions{}) require.NoError(t, err) require.Len(t, receivers.Items, 2) @@ -1171,14 +1190,72 @@ func TestIntegrationReceiverListSelector(t *testing.T) { }) } +// persistInitialConfig helps create an initial config with new receivers using legacy json. Config API blocks receiver +// modifications, so we need to use k8s API to create new receivers before posting the config. +func persistInitialConfig(t *testing.T, amConfig definitions.PostableUserConfig, adminClient notificationsv0alpha1.ReceiverInterface, legacyCli alerting.LegacyApiClient) { + ctx := context.Background() + + var defaultReceiver *definitions.PostableApiReceiver + for _, receiver := range amConfig.AlertmanagerConfig.Receivers { + if receiver.Name == "grafana-default-email" { + defaultReceiver = receiver + continue + } + + toCreate := v0alpha1.Receiver{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "default", + }, + Spec: v0alpha1.ReceiverSpec{ + Title: receiver.Name, + Integrations: []v0alpha1.Integration{}, + }, + } + + for _, integration := range receiver.GrafanaManagedReceivers { + settings := common.Unstructured{} + require.NoError(t, settings.UnmarshalJSON(integration.Settings)) + toCreate.Spec.Integrations = append(toCreate.Spec.Integrations, v0alpha1.Integration{ + Settings: settings, + Type: integration.Type, + DisableResolveMessage: util.Pointer(false), + }) + } + + created, err := adminClient.Create(ctx, &toCreate, v1.CreateOptions{}) + require.NoError(t, err) + + for i, integration := range created.Spec.Integrations { + receiver.GrafanaManagedReceivers[i].UID = *integration.Uid + } + } + + success, err := legacyCli.PostConfiguration(t, amConfig) + require.Truef(t, success, "Failed to post Alertmanager configuration: %s", err) + + gettable, status, body := legacyCli.GetAlertmanagerConfigWithStatus(t) + require.Equalf(t, http.StatusOK, status, body) + + idx := slices.IndexFunc(gettable.AlertmanagerConfig.Receivers, func(recv *definitions.GettableApiReceiver) bool { + return recv.Name == "grafana-default-email" + }) + gettableDefault := gettable.AlertmanagerConfig.Receivers[idx] + + // Assign uid of default receiver as well. + defaultReceiver.GrafanaManagedReceivers[0].UID = gettableDefault.GrafanaManagedReceivers[0].UID +} + func createIntegration(t *testing.T, integrationType string) v0alpha1.Integration { cfg, ok := notify.AllKnownConfigsForTesting[integrationType] require.Truef(t, ok, "no known config for integration type %s", integrationType) + return createIntegrationWithSettings(t, integrationType, cfg.Config) +} +func createIntegrationWithSettings(t *testing.T, integrationType string, settingsJson string) v0alpha1.Integration { settings := common.Unstructured{} - require.NoError(t, settings.UnmarshalJSON([]byte(cfg.Config))) + require.NoError(t, settings.UnmarshalJSON([]byte(settingsJson))) return v0alpha1.Integration{ Settings: settings, - Type: cfg.NotifierType, + Type: integrationType, DisableResolveMessage: util.Pointer(false), } } diff --git a/pkg/tests/apis/alerting/notifications/receivers/test-data/notification-settings.json b/pkg/tests/apis/alerting/notifications/receivers/test-data/notification-settings.json index 5c816ed4687..1228ef875e9 100644 --- a/pkg/tests/apis/alerting/notifications/receivers/test-data/notification-settings.json +++ b/pkg/tests/apis/alerting/notifications/receivers/test-data/notification-settings.json @@ -23,8 +23,9 @@ "grafana_managed_receiver_configs": [ { "type": "email", + "name": "email receiver", "settings": { - "addresses": "example@email.com" + "addresses": "" } } ] @@ -34,6 +35,7 @@ "grafana_managed_receiver_configs": [ { "type": "email", + "name": "user-defined", "settings": { "addresses": "example@email.com" } @@ -42,4 +44,4 @@ } ] } -} \ No newline at end of file +} diff --git a/pkg/tests/apis/alerting/notifications/timeinterval/test-data/notification-settings.json b/pkg/tests/apis/alerting/notifications/timeinterval/test-data/notification-settings.json index 5a8e90d4d39..80f1b0438a0 100644 --- a/pkg/tests/apis/alerting/notifications/timeinterval/test-data/notification-settings.json +++ b/pkg/tests/apis/alerting/notifications/timeinterval/test-data/notification-settings.json @@ -45,6 +45,7 @@ "grafana_managed_receiver_configs": [ { "type": "email", + "name": "email receiver", "settings": { "addresses": "" } @@ -53,4 +54,4 @@ } ] } -} \ No newline at end of file +}