From 4068349afef27850422917541a9e7a5a3e360bfe Mon Sep 17 00:00:00 2001 From: mrhid6 Date: Thu, 17 Sep 2026 08:56:15 +0000 Subject: [PATCH] feat(monitors): validate metric monitors and confine selectors to token scope --- server/internal/api/monitors.go | 20 ++++++- server/internal/mcp/tools_create.go | 41 ++++++++++++-- server/internal/services/metricrules.go | 17 ++++++ server/internal/services/metricscope_test.go | 27 +++++++++ server/internal/services/monitors.go | 58 ++++++++++++++++++-- 5 files changed, 152 insertions(+), 11 deletions(-) create mode 100644 server/internal/services/metricscope_test.go diff --git a/server/internal/api/monitors.go b/server/internal/api/monitors.go index 67393ce..f711783 100644 --- a/server/internal/api/monitors.go +++ b/server/internal/api/monitors.go @@ -91,6 +91,10 @@ func createMonitor(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + if errors.Is(err, services.ErrMonitorOutOfScope) { + c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -137,7 +141,7 @@ func getMonitor(c *gin.Context) { // @Accept json // @Produce json // @Param id path string true "Monitor ID" -// @Param body body object{name=string,group=string,type=string,target=models.MonitorTarget,interval_sec=int,runner=string,retries=int,enabled=bool,channel_ids=[]string} true "Fields to update" +// @Param body body object{name=string,group=string,type=string,target=models.MonitorTarget,interval_sec=int,runner=string,retries=int,enabled=bool,channel_ids=[]string,for_sec=int} true "Fields to update" // @Success 204 // @Failure 400 {object} ErrorResponse // @Failure 500 {object} ErrorResponse @@ -155,6 +159,7 @@ func updateMonitor(c *gin.Context) { Retries *int `json:"retries"` Enabled *bool `json:"enabled"` ChannelIDs *[]string `json:"channel_ids"` + ForSec *int `json:"for_sec"` } if err := c.ShouldBindJSON(&body); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) @@ -188,6 +193,9 @@ func updateMonitor(c *gin.Context) { if body.ChannelIDs != nil { upd["channel_ids"] = *body.ChannelIDs } + if body.ForSec != nil { + upd["for_sec"] = *body.ForSec + } if len(upd) == 0 { c.JSON(http.StatusBadRequest, gin.H{"error": "no fields to update"}) return @@ -197,6 +205,10 @@ func updateMonitor(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + if errors.Is(err, services.ErrMonitorOutOfScope) { + c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } @@ -214,7 +226,11 @@ func updateMonitor(c *gin.Context) { // @Security bearerAuth // @Router /monitors/{id} [delete] func deleteMonitor(c *gin.Context) { - if err := services.DeleteMonitor(auth.InstanceID(c), c.Param("id")); err != nil { + if err := services.DeleteMonitor(auth.InstanceID(c), c.Param("id"), auth.ServerScope(c)); err != nil { + if errors.Is(err, services.ErrMonitorOutOfScope) { + c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) + return + } c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) return } diff --git a/server/internal/mcp/tools_create.go b/server/internal/mcp/tools_create.go index 3424459..3be8e75 100644 --- a/server/internal/mcp/tools_create.go +++ b/server/internal/mcp/tools_create.go @@ -118,6 +118,25 @@ func buildMonitor(args map[string]any) (models.Monitor, error) { if b, ok := rawTarget["insecure"].(bool); ok { target.Insecure = b } + target.Metric = stringArg(rawTarget, "metric") + target.Mount = stringArg(rawTarget, "mount") + if n, ok := rawTarget["threshold"].(float64); ok { + target.Threshold = n + } + if n, ok := rawTarget["period_sec"].(float64); ok { + target.PeriodSec = int(n) + } + if n, ok := rawTarget["grace_sec"].(float64); ok { + target.GraceSec = int(n) + } + if sel, ok := rawTarget["selector"].(map[string]any); ok { + target.Selector = map[string]string{} + for k, v := range sel { + if s, ok := v.(string); ok { + target.Selector[k] = s + } + } + } switch monitorType { case models.MonitorHTTP, models.MonitorTLS: @@ -131,6 +150,9 @@ func buildMonitor(args map[string]any) (models.Monitor, error) { if monitorType == models.MonitorTCP && target.Port == 0 { return models.Monitor{}, fmt.Errorf("target.port is required for a tcp monitor") } + case models.MonitorHeartbeat, models.MonitorMetric: + // Full validation (validateHeartbeat / validateMetric) runs inside + // services.CreateMonitor; nothing further is required here. default: return models.Monitor{}, fmt.Errorf("unknown monitor type %q", monitorType) } @@ -150,6 +172,10 @@ func buildMonitor(args map[string]any) (models.Monitor, error) { if n, ok := args["interval_sec"].(float64); ok && int(n) > 0 { interval = int(n) } + forSec := 0 + if n, ok := args["for_sec"].(float64); ok && int(n) > 0 { + forSec = int(n) + } return models.Monitor{ Name: name, @@ -157,6 +183,7 @@ func buildMonitor(args map[string]any) (models.Monitor, error) { Type: monitorType, Target: target, IntervalSec: interval, + ForSec: forSec, // Never armed on creation. A monitor that started enabled would begin // alerting real people the moment a model invented it, and creating // must stay a separate decision from acting. @@ -257,10 +284,11 @@ func init() { Name: "create_monitor", Args: []ToolArg{ {Name: "name", Type: ArgString, Description: "Name for the monitor.", Required: true}, - {Name: "type", Type: ArgString, Description: "Check type: http, tcp, icmp or tls.", Required: true}, - {Name: "target", Type: ArgObject, Description: "What to check. http/tls take url; tcp/icmp take host, and tcp also port. Optional: method, keyword, expected_status, tls_warn_days, insecure.", Required: true}, + {Name: "type", Type: ArgString, Description: "Check type: http, tcp, icmp, tls, heartbeat or metric.", Required: true}, + {Name: "target", Type: ArgObject, Description: "What to check. http/tls take url; tcp/icmp take host, and tcp also port. Optional: method, keyword, expected_status, tls_warn_days, insecure. heartbeat takes period_sec and optional grace_sec; metric takes metric (disk_pct, disk_free_gb, mem_pct, load_per_core, unit_failed, container_unhealthy, reboot_pending_days, agent_offline_min), threshold, optional mount and selector (tag map).", Required: true}, {Name: "group", Type: ArgString, Description: "Optional group name to file the monitor under."}, {Name: "interval_sec", Type: ArgInteger, Description: "Seconds between checks; defaults to 60."}, + {Name: "for_sec", Type: ArgInteger, Description: "For a metric monitor, how long the condition must hold before a server counts as down."}, }, Write: true, Scope: "monitors:write", @@ -277,12 +305,17 @@ func init() { return nil, fmt.Errorf("could not create the monitor: %w", err) } LogCreated(c, "monitor", created.MonitorID, created.Name) - return map[string]any{ + out := map[string]any{ "monitor_id": created.MonitorID, "name": created.Name, "enabled": false, "note": "Created disabled. Enable it in Vantage to start checking.", - }, nil + } + if created.HeartbeatToken != "" { + out["heartbeat_token"] = created.HeartbeatToken + out["heartbeat_token_note"] = "Shown once." + } + return out, nil }, }) } diff --git a/server/internal/services/metricrules.go b/server/internal/services/metricrules.go index e8f70a3..5bab55e 100644 --- a/server/internal/services/metricrules.go +++ b/server/internal/services/metricrules.go @@ -1,6 +1,7 @@ package services import ( + "errors" "fmt" "path" "strings" @@ -9,6 +10,22 @@ import ( "gitea.hostxtra.co.uk/mrhid6/vantage/server/internal/models" ) +// ErrMonitorOutOfScope marks a metric monitor whose selector reaches beyond a +// restricted token's tag scope, on create, update or delete. +var ErrMonitorOutOfScope = errors.New("selector is outside this credential's tag scope") + +// selectorWithinScope holds when every server the selector can match is also +// inside the token's scope - true exactly when the selector pins every pair +// the scope does. +func selectorWithinScope(sel, tokenScope map[string]string) bool { + for k, v := range tokenScope { + if sel[k] != v { + return false + } + } + return true +} + const ( MetricDiskPct = "disk_pct" MetricDiskFreeGB = "disk_free_gb" diff --git a/server/internal/services/metricscope_test.go b/server/internal/services/metricscope_test.go new file mode 100644 index 0000000..db37a26 --- /dev/null +++ b/server/internal/services/metricscope_test.go @@ -0,0 +1,27 @@ +package services + +import "testing" + +// A restricted token must not be able to create a rule that watches servers it +// cannot see: the per-server table and incident messages would disclose them. +func TestSelectorWithinScope(t *testing.T) { + scope := map[string]string{"env": "prod"} + cases := []struct { + sel map[string]string + want bool + }{ + {nil, false}, + {map[string]string{"env": "dev"}, false}, + {map[string]string{"role": "web"}, false}, + {map[string]string{"env": "prod"}, true}, + {map[string]string{"env": "prod", "role": "web"}, true}, + } + for _, c := range cases { + if got := selectorWithinScope(c.sel, scope); got != c.want { + t.Errorf("sel %v: got %v want %v", c.sel, got, c.want) + } + } + if !selectorWithinScope(nil, nil) { + t.Error("an unrestricted credential may use any selector, including the whole fleet") + } +} diff --git a/server/internal/services/monitors.go b/server/internal/services/monitors.go index 50aaa24..bd9809f 100644 --- a/server/internal/services/monitors.go +++ b/server/internal/services/monitors.go @@ -202,6 +202,18 @@ func CreateMonitor(instanceID string, m *models.Monitor, tokenScope map[string]s m.Runner = models.RunnerServer m.IntervalSec = 0 } + if m.Type == models.MonitorMetric { + if err := validateMetric(m); err != nil { + return nil, err + } + if !selectorWithinScope(m.Target.Selector, tokenScope) { + return nil, ErrMonitorOutOfScope + } + // A metric monitor is evaluated by sweeping stored server inventory, + // never run against a single named agent. + m.Runner = models.RunnerServer + m.IntervalSec = 0 + } m.State = models.MonitorState{Status: models.StatusPending} if _, err := db.Col("monitors").InsertOne(ctx, m); err != nil { return nil, err @@ -229,13 +241,31 @@ func UpdateMonitor(instanceID, monitorID string, upd bson.M, tokenScope map[stri return fmt.Errorf("type cannot be changed to or from %s", bad) } } - if raw, present := upd["target"]; present && existing.Type == models.MonitorHeartbeat { + // Editing any field of a metric monitor the token could not have created is + // refused before any write, so a restricted token cannot rename or disable + // a fleet-wide rule it cannot fully see. + if existing.Type == models.MonitorMetric && !selectorWithinScope(existing.Target.Selector, tokenScope) { + return ErrMonitorOutOfScope + } + + if raw, present := upd["target"]; present && models.IsPassiveMonitor(existing.Type) { tg, ok := raw.(models.MonitorTarget) if !ok { - return fmt.Errorf("target must be an object") + return fmt.Errorf("%w: target must be an object", ErrInvalidMonitor) } - if err := validateHeartbeat(&tg); err != nil { - return err + switch existing.Type { + case models.MonitorHeartbeat: + if err := validateHeartbeat(&tg); err != nil { + return err + } + case models.MonitorMetric: + probe := models.Monitor{Type: existing.Type, Target: tg, ForSec: existing.ForSec} + if err := validateMetric(&probe); err != nil { + return err + } + if !selectorWithinScope(tg.Selector, tokenScope) { + return ErrMonitorOutOfScope + } } upd["target"] = tg } @@ -277,9 +307,26 @@ func UpdateMonitor(instanceID, monitorID string, upd bson.M, tokenScope map[stri return err } -func DeleteMonitor(instanceID, monitorID string) error { +// DeleteMonitor deletes a monitor and its associated data. For a metric +// monitor, deletion is refused when its selector reaches beyond tokenScope - +// the same out-of-scope check applied on create and update - so a restricted +// token cannot remove a fleet-wide rule it could not have made. Other monitor +// types delete as before, unaffected by tokenScope. +func DeleteMonitor(instanceID, monitorID string, tokenScope map[string]string) error { ctx, cancel := monCtx() defer cancel() + + existing, err := GetMonitor(instanceID, monitorID) + if err != nil { + return err + } + if existing == nil { + return nil + } + if existing.Type == models.MonitorMetric && !selectorWithinScope(existing.Target.Selector, tokenScope) { + return ErrMonitorOutOfScope + } + res, err := db.Col("monitors").DeleteOne(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}) if err != nil { return err @@ -291,6 +338,7 @@ func DeleteMonitor(instanceID, monitorID string) error { db.Col("incidents").DeleteMany(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}) db.Col("monitor_rollups").DeleteMany(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}) db.Col("monitor_samples").DeleteMany(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}) + db.Col("monitor_server_states").DeleteMany(ctx, bson.M{"monitor_id": monitorID, "instance_id": instanceID}) return nil }