Authorization bypass / improper access control for singleton creation

Pre-fix (attack path exists in environments without this fix): 1) Prerequisites: Grafana alerting operator installed; the per-org Config resource does not exist for org-1, and the sync worker has not seeded it because UID is not configured. 2) As a user with RBAC create permissions but not the service identity, attempt to create the per-org Config resource: curl -X POST https://grafana.example/api/alerting/v0alpha1/configs -H "Authorization: Bearer <admin-token>" -H "Content-Type: application/json" -d '{"apiVersion":"alerting/v0alpha1","kind":"Config","metadata":{"name":"config-org-1"},"spec":{}}' Expected (pre-fix): API accepts the create (HTTP 201/200) and the resource becomes the singleton for that org. The sync seeder may not override or may attempt to seed where it can, depending on timing. Post-fix (after this patch): The same create request fails with a denial (e.g., HTTP 403) and message similar to: "Config is a singleton seeded automatically; it cannot be created via the API". This blocks non-service users from creating the singleton. 3) Demonstrating correct service identity behavior: use the service account token to create the singleton curl -X POST https://grafana.example/api/alerting/v0alpha1/configs -H "Authorization: Bearer <service-token>" -H "Content-Type: application/json" -d '{"apiVersion":"alerting/v0alpha1","kind":"Config","metadata":{"name":"config-org-1"},"spec":{}}' Expected: 201 Created. The seeder will seed/update as necessary on subsequent ticks. 4) Update path (after the singleton exists): A non-service user attempting PUT/PATCH should be allowed to update the already-seeded object only if they have appropriate permissions (the update action is allowed). The key mitigation is that creation is strictly service-identity only, preventing initial seeding by human/GitOps accounts. Notes: - The PoC is intended for a lab environment to verify authorization behavior; do not run against production clusters without authorization.

diff --git a/pkg/registry/apps/alerting/notifications/config/authorize.go b/pkg/registry/apps/alerting/notifications/config/authorize.go index d4c4f1103482f..0f24a1b5908c6 100644 --- a/pkg/registry/apps/alerting/notifications/config/authorize.go +++ b/pkg/registry/apps/alerting/notifications/config/authorize.go @@ -13,11 +13,17 @@ import ( // Authorize maps k8s verbs on Config (and its /status subresource) to RBAC // actions. Config is a per-org singleton: // - get/list/watch → read (Viewer) -// - create/patch/update → update (Admin). create is gated by the update -// permission (not rejected) because the singleton must be creatable via the -// API — and an upsert (PUT/apply to a missing object) is authorized by the -// apiserver as create. The singleton-name check lives in the admission -// validator, not here. +// - patch/update → update (Admin). +// - create → service identity only. The singleton is seeded automatically by +// the sync worker, so humans never create it — they only read/update the +// already-seeded object. Gated on IsServiceIdentity rather than RBAC because +// both humans and the service identity hold configs:update; we can't simply +// deny create (that would block the seeder, which creates through this same +// authorizer) nor drop it (it would fall through to the org-role authorizer, +// which allows Admins). This also covers GitOps create-on-update: a PUT/apply +// to a *missing* object is re-authorized by the apiserver as create, so it's +// denied here too; once the object exists a PUT/apply is an update and is +// allowed. The singleton-name check lives in the admission validator, not here. // - delete/deletecollection → always rejected (deleting the singleton would // nuke every admin setting it carries; reset individual fields via update) // - status writes → service identity only (sync worker owns it; see @@ -50,7 +56,14 @@ func Authorize(ctx context.Context, ac accesscontrol.AccessControl, attr authori switch attr.GetVerb() { case "get", "list", "watch": action = accesscontrol.EvalPermission(accesscontrol.ActionAlertingConfigRead, scope) - case "create", "patch", "update": + case "create": + // The singleton is seeded automatically by the sync worker (service + // identity); humans/GitOps update the seeded object, never create it. + if !identity.IsServiceIdentity(ctx) { + return authorizer.DecisionDeny, "Config is a singleton seeded automatically; it cannot be created via the API", nil + } + action = accesscontrol.EvalPermission(accesscontrol.ActionAlertingConfigUpdate, scope) + case "patch", "update": action = accesscontrol.EvalPermission(accesscontrol.ActionAlertingConfigUpdate, scope) case "delete", "deletecollection": return authorizer.DecisionDeny, "Config is a singleton and cannot be deleted", nil diff --git a/pkg/registry/apps/alerting/notifications/config/authorize_test.go b/pkg/registry/apps/alerting/notifications/config/authorize_test.go index e51b1f40e57a2..f6ad3825c4517 100644 --- a/pkg/registry/apps/alerting/notifications/config/authorize_test.go +++ b/pkg/registry/apps/alerting/notifications/config/authorize_test.go @@ -90,10 +90,13 @@ func TestAuthorize(t *testing.T) { {name: "status patch permitted", id: idHuman, verb: "patch", subresource: "status", rbac: true, wantAction: accesscontrol.ActionAlertingConfigStatusUpdate, want: authorizer.DecisionAllow}, {name: "status create permitted", id: idHuman, verb: "create", subresource: "status", rbac: true, wantAction: accesscontrol.ActionAlertingConfigStatusUpdate, want: authorizer.DecisionAllow}, - // create is gated by the update permission: the singleton must be creatable - // via the API, and an upsert (PUT to a missing object) is authorized as create. - {name: "create permitted with update", id: idHuman, verb: "create", rbac: true, wantAction: accesscontrol.ActionAlertingConfigUpdate, want: authorizer.DecisionAllow}, - {name: "create denied without update", id: idHuman, verb: "create", rbac: false, wantAction: accesscontrol.ActionAlertingConfigUpdate, want: authorizer.DecisionDeny}, + // create is service-identity only: the singleton is seeded automatically, so + // humans/GitOps are denied (without consulting RBAC) and update the seeded + // object instead. The service identity (the seeder) is gated on the update + // permission like any other write. + {name: "human create denied even with update", id: idHuman, verb: "create", rbac: true, want: authorizer.DecisionDeny, wantReason: "Config is a singleton seeded automatically; it cannot be created via the API"}, + {name: "service create permitted with update", id: idService, verb: "create", rbac: true, wantAction: accesscontrol.ActionAlertingConfigUpdate, want: authorizer.DecisionAllow}, + {name: "service create denied without update", id: idService, verb: "create", rbac: false, wantAction: accesscontrol.ActionAlertingConfigUpdate, want: authorizer.DecisionDeny}, // delete/deletecollection are always rejected (destructive on a singleton). {name: "delete denied", id: idService, verb: "delete", rbac: true, want: authorizer.DecisionDeny}, diff --git a/pkg/services/ngalert/notifier/external_am_syncer.go b/pkg/services/ngalert/notifier/external_am_syncer.go index 3d41fc4504600..1cc9bf3303fb1 100644 --- a/pkg/services/ngalert/notifier/external_am_syncer.go +++ b/pkg/services/ngalert/notifier/external_am_syncer.go @@ -56,13 +56,13 @@ type mimirConfigResponse struct { // status.conditions[] without collision. const conditionTypeExternalAlertmanagerSynced = "ExternalAlertmanagerSynced" -// conditionReasonSyncSucceeded is the success-branch reason. Failure -// reasons come from SyncReason.ConditionReason(). -const conditionReasonSyncSucceeded = "SyncSucceeded" - -// conditionReasonMergeCommitted is the terminal reason once the config was -// imported as a managed route: sync stops and the admin owns it from here. -const conditionReasonMergeCommitted = "MergeCommitted" +// ExternalAlertmanagerSynced condition reasons (failure reasons come from +// SyncReason.ConditionReason()). +const ( + conditionReasonSyncSucceeded = "SyncSucceeded" // a sync attempt succeeded + conditionReasonMergeCommitted = "MergeCommitted" // imported as a managed route; sync stops + conditionReasonNotConfigured = "NotConfigured" // flag on, but no UID configured → Synced=Unknown +) const mergeCommittedMessage = "The external Alertmanager configuration has already been merged into Grafana; automatic sync from the datasource has stopped." @@ -296,6 +296,10 @@ func (s *ExternalAMSyncer) FetchExtraConfig(ctx context.Context, orgID int64) (* return nil, 0 } if uid == "" { + // Flag on but sync isn't configured here (no ini override, no spec UID): + // record Unknown/NotConfigured, which also seeds the singleton (writeStatus + // creates on missing) so it reliably exists without a manual create. + s.recordNotConfigured(ctx, orgID) return nil, 0 } @@ -412,6 +416,15 @@ func (s *ExternalAMSyncer) recordMergeCommitted(ctx context.Context, orgID int64 }) } +// recordNotConfigured records Synced=Unknown/NotConfigured for the org, seeding +// the singleton if absent (writeStatus creates on missing). Best-effort. +func (s *ExternalAMSyncer) recordNotConfigured(ctx context.Context, orgID int64) { + now := time.Now() + s.writeStatus(ctx, orgID, func(prev *alertingnotifv0alpha1.ConfigStatus) alertingnotifv0alpha1.ConfigStatus { + return computeNotConfiguredStatus(prev, now) + }) +} + // writeStatus upserts the org's Config.status using compute(prev), creating the // resource if absent. Optimistic via RetryOnConflict; best-effort (failures are // logged). Unchanged status produces no physical write (unified storage dedup). @@ -473,6 +486,40 @@ func computeCommittedStatus(prev *alertingnotifv0alpha1.ConfigStatus, uid string return buildSyncStatus(prev, uid, origin, alertingnotifv0alpha1.ConfigConditionStatusTrue, conditionReasonMergeCommitted, mergeCommittedMessage, now) } +// computeNotConfiguredStatus returns prev with only the ExternalAlertmanagerSynced +// condition updated to Unknown/NotConfigured (used when the flag is on but no UID +// is configured). Everything else rides through unchanged: observedGeneration, +// externalAlertmanagerSync (kept as the last-attempt context, its documented +// meaning) and any sibling conditions. The Synced condition is a current-state +// snapshot — no preserved MergeCommitted/SyncSucceeded — and its lastTransitionTime +// advances only on a flip to Unknown, so consecutive not-configured ticks produce +// an identical status that dedups to no write. +func computeNotConfiguredStatus(prev *alertingnotifv0alpha1.ConfigStatus, now time.Time) alertingnotifv0alpha1.ConfigStatus { + st := alertingnotifv0alpha1.ConfigStatus{} + if prev != nil { + st = *prev + st.Conditions = append([]alertingnotifv0alpha1.ConfigCondition(nil), prev.Conditions...) + } + + synced := alertingnotifv0alpha1.ConfigCondition{ + Type: conditionTypeExternalAlertmanagerSynced, + Status: alertingnotifv0alpha1.ConfigConditionStatusUnknown, + LastTransitionTime: now.UTC().Format(time.RFC3339), + Reason: conditionReasonNotConfigured, + } + for i, c := range st.Conditions { + if c.Type == conditionTypeExternalAlertmanagerSynced { + if c.Status == alertingnotifv0alpha1.ConfigConditionStatusUnknown { + synced.LastTransitionTime = c.LastTransitionTime // no flip → keep the timestamp + } + st.Conditions[i] = synced + return st + } + } + st.Conditions = append(st.Conditions, synced) + return st +} + // buildSyncStatus folds an ExternalAlertmanagerSynced condition into prev. k8s // condition FSM: lastTransitionTime advances only on status flip. Preserves // other condition types so future controllers aren't clobbered. diff --git a/pkg/services/ngalert/notifier/external_am_syncer_status_test.go b/pkg/services/ngalert/notifier/external_am_syncer_status_test.go index dcfb20d4cf946..8be9ee6be9a7e 100644 --- a/pkg/services/ngalert/notifier/external_am_syncer_status_test.go +++ b/pkg/services/ngalert/notifier/external_am_syncer_status_test.go @@ -307,3 +307,134 @@ func TestComputeCommittedStatus(t *testing.T) { assert.Equal(t, nowRFC, c.LastTransitionTime, "flip False->True advances the timestamp") }) } + +func TestComputeNotConfiguredStatus(t *testing.T) { + now := time.Date(2026, 5, 20, 12, 0, 0, 0, time.UTC) + earlier := time.Date(2026, 5, 19, 9, 0, 0, 0, time.UTC) + nowRFC := now.UTC().Format(time.RFC3339) + earlRFC := earlier.UTC().Format(time.RFC3339) + + findSynced := func(t *testing.T, st alertingnotifv0alpha1.ConfigStatus) alertingnotifv0alpha1.ConfigCondition { + t.Helper() + for _, c := range st.Conditions { + if c.Type == conditionTypeExternalAlertmanagerSynced { + return c + } + } + t.Fatalf("expected Synced condition, got: %+v", st.Conditions) + return alertingnotifv0alpha1.ConfigCondition{} + } + + t.Run("from clean state emits Synced=Unknown/NotConfigured with current timestamp", func(t *testing.T) { + got := computeNotConfiguredStatus(nil, now) + + synced := findSynced(t, got) + assert.Equal(t, alertingnotifv0alpha1.ConfigConditionStatusUnknown, synced.Status) + assert.Equal(t, conditionReasonNotConfigured, synced.Reason) + assert.Equal(t, nowRFC, synced.LastTransitionTime) + assert.Nil(t, synced.Message) + assert.Nil(t, got.ExternalAlertmanagerSync, "no datasource context when unconfigured") + }) + + t.Run("after a prior success flips to Unknown and advances lastTransitionTime", func(t *testing.T) { + prev := &alertingnotifv0alpha1.ConfigStatus{ + Conditions: []alertingnotifv0alpha1.ConfigCondition{{ + Type: conditionTypeExternalAlertmanagerSynced, + Status: alertingnotifv0alpha1.ConfigConditionStatusTrue, + LastTransitionTime: earlRFC, + Reason: conditionReasonSyncSucceeded, + }}, + } + + synced := findSynced(t, computeNotConfiguredStatus(prev, now)) + assert.Equal(t, alertingnotifv0alpha1.ConfigConditionStatusUnknown, synced.Status) + assert.Equal(t, conditionReasonNotConfigured, synced.Reason) + assert.Equal(t, nowRFC, synced.LastTransitionTime, "flip True->Unknown advances the timestamp") + }) + + t.Run("consecutive not-configured ticks preserve lastTransitionTime", func(t *testing.T) { + prev := &alertingnotifv0alpha1.ConfigStatus{ + Conditions: []alertingnotifv0alpha1.ConfigCondition{{ + Type: conditionTypeExternalAlertmanagerSynced, + Status: alertingnotifv0alpha1.ConfigConditionStatusUnknown, + LastTransitionTime: earlRFC, + Reason: conditionReasonNotConfigured, + }}, + } + + synced := findSynced(t, computeNotConfiguredStatus(prev, now)) + assert.Equal(t, earlRFC, synced.LastTransitionTime, "status stayed Unknown, so the timestamp is preserved") + }) + + t.Run("a prior MergeCommitted flips to Unknown when the UID is removed", func(t *testing.T) { + // No special-case: when sync is no longer configured the Synced condition is + // a current-state snapshot, so even a terminal MergeCommitted flips. The merge + // artifact still lives in the AM config; this condition only tracks sync. + prev := &alertingnotifv0alpha1.ConfigStatus{ + Conditions: []alertingnotifv0alpha1.ConfigCondition{{ + Type: conditionTypeExternalAlertmanagerSynced, + Status: alertingnotifv0alpha1.ConfigConditionStatusTrue, + LastTransitionTime: earlRFC, + Reason: conditionReasonMergeCommitted, + }}, + } + + synced := findSynced(t, computeNotConfiguredStatus(prev, now)) + assert.Equal(t, alertingnotifv0alpha1.ConfigConditionStatusUnknown, synced.Status) + assert.Equal(t, conditionReasonNotConfigured, synced.Reason) + assert.Equal(t, nowRFC, synced.LastTransitionTime, "flip True->Unknown advances the timestamp") + }) + + t.Run("root fields ride through unchanged", func(t *testing.T) { + gen := int64(7) + uidLast := "uid-last" + originLast := originAPI + prev := &alertingnotifv0alpha1.ConfigStatus{ + ObservedGeneration: &gen, + ExternalAlertmanagerSync: &alertingnotifv0alpha1.ConfigV0alpha1StatusExternalAlertmanagerSync{ + DatasourceUid: &uidLast, + Origin: &originLast, + }, + Conditions: []alertingnotifv0alpha1.ConfigCondition{{ + Type: conditionTypeExternalAlertmanagerSynced, + Status: alertingnotifv0alpha1.ConfigConditionStatusTrue, + LastTransitionTime: earlRFC, + Reason: conditionReasonSyncSucceeded, + }}, + } + + got := computeNotConfiguredStatus(prev, now) + // observedGeneration and the last-attempt externalAlertmanagerSync context are + // preserved; only the Synced condition flips. + require.NotNil(t, got.ObservedGeneration) + assert.Equal(t, gen, *got.ObservedGeneration) + require.NotNil(t, got.ExternalAlertma ... [truncated]