diff --git a/api/agents/handler.go b/api/agents/handler.go index dd7bb16..ebac861 100644 --- a/api/agents/handler.go +++ b/api/agents/handler.go @@ -155,6 +155,20 @@ func (h *Handler) handleSetConfig(w http.ResponseWriter, r *http.Request) { } } + // log_retention_days requires RoleOwner specifically, not just + // RoleAdmin -- unlike extra_file_paths (an asymmetric "only granting + // more capability needs the stricter role" check), *any* change here + // needs the top role, including lowering or clearing an existing + // value. The whole point of this field is a floor only an owner can + // override (see api/logretention's doc comment); an admin able to + // freely lower or remove it would make that floor meaningless. + if identity, ok := authz.IdentityFromContext(r.Context()); ok && !identity.Role.Satisfies(authz.RoleOwner) { + if changesLogRetentionDays(h.currentLogRetentionDays(r.Context(), h.tenantID(r), r.PathValue("host")), override.LogRetentionDays) { + writeError(w, http.StatusForbidden, "log_retention_days requires the owner role") + return + } + } + a, err := h.store.SetOverride(r.Context(), h.tenantID(r), r.PathValue("host"), override, h.updatedBy(r)) if err != nil { h.writeStoreErr(w, err, "setting agent config") @@ -237,6 +251,14 @@ func validateOverride(o ConfigOverride) error { return err } } + // 3650 days (10 years) matches api/logretention's own + // maxOlderThanHours bound -- a floor further out than the deletion + // feature can even reach is meaningless, and rejecting an obviously- + // wrong input (a stray extra digit) here is cheaper than debugging it + // later as "why can no one ever delete logs." + if o.LogRetentionDays != nil && (*o.LogRetentionDays < 1 || *o.LogRetentionDays > 3650) { + return errors.New("log_retention_days must be between 1 and 3650") + } return nil } @@ -320,6 +342,31 @@ func changesExtraFilePaths(current, desired []string) bool { return false } +// currentLogRetentionDays mirrors currentExtraFilePaths exactly, for +// the same reason: handleSetConfig needs the stored value to tell +// whether this request actually changes it. +func (h *Handler) currentLogRetentionDays(ctx context.Context, tenantID, host string) *int { + a, err := h.store.Get(ctx, tenantID, host) + if err != nil || a.DesiredOverride == nil { + return nil + } + return a.DesiredOverride.LogRetentionDays +} + +// changesLogRetentionDays reports whether desired differs from current +// at all -- unlike changesExtraFilePaths, there is no safe direction +// here (see handleSetConfig's log_retention_days comment for why even +// lowering or clearing needs the same gate as raising it). +func changesLogRetentionDays(current, desired *int) bool { + if current == nil && desired == nil { + return false + } + if current == nil || desired == nil { + return true + } + return *current != *desired +} + func (h *Handler) writeStoreErr(w http.ResponseWriter, err error, action string) { if errors.Is(err, ErrNotFound) { writeError(w, http.StatusNotFound, "agent not found") diff --git a/api/agents/handler_test.go b/api/agents/handler_test.go index 913b48b..ea47049 100644 --- a/api/agents/handler_test.go +++ b/api/agents/handler_test.go @@ -309,6 +309,73 @@ func TestHandleSetConfigExtraFilePathsRequiresAdminToAdd(t *testing.T) { } } +// TestHandleSetConfigLogRetentionDaysRequiresOwner is the analogous +// regression test for log_retention_days -- but unlike extra_file_paths, +// there is no safe direction an Admin is allowed to move it in: setting, +// raising, lowering, and clearing all require Owner (see +// changesLogRetentionDays's doc comment for why). +func TestHandleSetConfigLogRetentionDaysRequiresOwner(t *testing.T) { + s := newFakeStore() + s.put(Agent{TenantID: "default", Host: "web-01"}) + editor := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleEditor}, nil) + admin := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleAdmin}, nil) + owner := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleOwner}, nil) + + days90 := 90 + rec := doRequest(t, admin, "PUT", "/agents/web-01/config", ConfigOverride{LogRetentionDays: &days90}) + if rec.Code != http.StatusForbidden { + t.Fatalf("admin setting log_retention_days: status = %d, want 403", rec.Code) + } + + rec = doRequest(t, owner, "PUT", "/agents/web-01/config", ConfigOverride{LogRetentionDays: &days90}) + if rec.Code != http.StatusOK { + t.Fatalf("owner setting log_retention_days: status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var got Agent + if err := json.Unmarshal(rec.Body.Bytes(), &got); err != nil { + t.Fatalf("decoding response: %v", err) + } + if got.DesiredOverride == nil || got.DesiredOverride.LogRetentionDays == nil || *got.DesiredOverride.LogRetentionDays != 90 { + t.Fatalf("stored override = %+v, want log_retention_days=90", got.DesiredOverride) + } + + // Lowering an existing value is exactly as gated as raising it. + days30 := 30 + rec = doRequest(t, admin, "PUT", "/agents/web-01/config", ConfigOverride{LogRetentionDays: &days30}) + if rec.Code != http.StatusForbidden { + t.Fatalf("admin lowering log_retention_days: status = %d, want 403", rec.Code) + } + + // Clearing it (omitting the field entirely) is also gated -- an + // admin resending the rest of the override without this field must + // not silently drop an owner-set floor. + rec = doRequest(t, admin, "PUT", "/agents/web-01/config", ConfigOverride{}) + if rec.Code != http.StatusForbidden { + t.Fatalf("admin clearing log_retention_days: status = %d, want 403", rec.Code) + } + + // An editor is blocked the same way an admin is -- this floor is + // Owner-only, not Admin-or-above like extra_file_paths. + rec = doRequest(t, editor, "PUT", "/agents/web-01/config", ConfigOverride{LogRetentionDays: &days30}) + if rec.Code != http.StatusForbidden { + t.Fatalf("editor setting log_retention_days: status = %d, want 403", rec.Code) + } +} + +func TestHandleSetConfigRejectsInvalidLogRetentionDays(t *testing.T) { + s := newFakeStore() + s.put(Agent{TenantID: "default", Host: "web-01"}) + owner := NewHandler(discardLogger(), s, fakeAuthorizer{role: authz.RoleOwner}, nil) + + for _, days := range []int{0, -1, 3651} { + d := days + rec := doRequest(t, owner, "PUT", "/agents/web-01/config", ConfigOverride{LogRetentionDays: &d}) + if rec.Code != http.StatusBadRequest { + t.Errorf("log_retention_days=%d: status = %d, want 400", days, rec.Code) + } + } +} + func TestHandleSetConfigUnknownHostIsNotFound(t *testing.T) { h := newTestHandler(newFakeStore()) interval := int64(30000) diff --git a/api/agents/store.go b/api/agents/store.go index 4eb4ec6..4a45211 100644 --- a/api/agents/store.go +++ b/api/agents/store.go @@ -41,6 +41,18 @@ type ConfigOverride struct { HeartbeatIntervalMS *int64 `json:"heartbeat_interval_ms,omitempty"` JournaldUnit *string `json:"journald_unit,omitempty"` ExtraFilePaths []string `json:"extra_file_paths,omitempty"` + // LogRetentionDays is unlike every other field above: it configures + // nothing about the agent's own runtime behavior (agent_control.proto + // has no equivalent field, and the Rust agent never reads this) -- + // it's a central policy tag read only by api/logretention, which + // treats the largest LogRetentionDays configured across any agent as + // a protective floor a non-owner's age-based log deletion request + // must not reach into (see logretention.AgentRetentionStore). Stored + // here anyway, in the same desired_override JSONB column and edited + // on the same per-agent config page, because "a setting attached to + // an agent" is exactly what it conceptually is, even though nothing + // ever ships it to the agent process itself. + LogRetentionDays *int `json:"log_retention_days,omitempty"` } type Agent struct { diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index 8de7922..295376b 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -163,8 +163,11 @@ func main() { // Same conn sqlRunner above already wraps -- logretention issues its // own purpose-built statements against the `logs` table directly // rather than going through sqlRunner's SELECT-only RunSQL (see - // logretention.Store's doc comment). - logRetentionHandler := logretention.NewHandler(logger, logretention.NewStore(conn), authorizer) + // logretention.Store's doc comment). Same pgPool as dashboards/agents + // above for the owner-only retention floor (logretention. + // AgentRetentionStore reads agents.ConfigOverride.LogRetentionDays + // out of the same `agents` table agentsHandler manages). + logRetentionHandler := logretention.NewHandler(logger, logretention.NewStore(conn), logretention.NewAgentRetentionStore(pgPool), authorizer) // One shared mux, CORS applied once around the whole thing -- see // httpserver's doc comment for why this changed from each diff --git a/api/logretention/agent_floor.go b/api/logretention/agent_floor.go new file mode 100644 index 0000000..0f78efc --- /dev/null +++ b/api/logretention/agent_floor.go @@ -0,0 +1,46 @@ +package logretention + +import ( + "context" + + "github.com/jackc/pgx/v5/pgxpool" +) + +// AgentRetentionStore reads the protective retention floor set on +// agents.ConfigOverride.LogRetentionDays -- a separate, Postgres-backed +// concern from Store's ClickHouse access above, so it lives in its own +// file. Deliberately its own narrow query against the same `agents` +// table api/agents.Store manages, rather than importing api/agents for +// a shared type, matching this codebase's "each package owns direct +// SQL access to what it needs" convention (e.g. alerting and api both +// read dashboards-adjacent tables independently rather than sharing a +// store type across a package boundary). +type AgentRetentionStore struct { + pool *pgxpool.Pool +} + +func NewAgentRetentionStore(pool *pgxpool.Pool) *AgentRetentionStore { + return &AgentRetentionStore{pool: pool} +} + +// MaxRetentionDays reports the largest log_retention_days configured +// across any agent's desired_override, if any are set at all -- this is +// the floor a non-owner's deletion request must not reach into (see +// Handler.checkRetentionFloor). The second return value is false when +// no agent has this field configured, distinct from a configured floor +// of 0 (which validateOverride never allows to be stored in the first +// place). +func (s *AgentRetentionStore) MaxRetentionDays(ctx context.Context) (int, bool, error) { + var days *int + err := s.pool.QueryRow(ctx, ` + SELECT max((desired_override->>'log_retention_days')::int) + FROM agents + WHERE desired_override->>'log_retention_days' IS NOT NULL`).Scan(&days) + if err != nil { + return 0, false, err + } + if days == nil { + return 0, false, nil + } + return *days, true, nil +} diff --git a/api/logretention/handler.go b/api/logretention/handler.go index 7fd04c0..57987cf 100644 --- a/api/logretention/handler.go +++ b/api/logretention/handler.go @@ -3,6 +3,7 @@ package logretention import ( "context" "encoding/json" + "fmt" "log/slog" "net/http" "strconv" @@ -19,6 +20,13 @@ type store interface { DeleteOlderThan(ctx context.Context, cutoff time.Time) error } +// retentionFloor is the narrow interface backing the owner-only +// override check -- *AgentRetentionStore (agent_floor.go) is the +// production implementation. +type retentionFloor interface { + MaxRetentionDays(ctx context.Context) (int, bool, error) +} + // maxOlderThanHours bounds the age a caller can specify -- 10 years is // far beyond any real retention window this feature exists for, and // exists only to reject an obviously-wrong input (e.g. a stray extra @@ -28,11 +36,12 @@ const maxOlderThanHours = 10 * 365 * 24 type Handler struct { logger *slog.Logger store store + floor retentionFloor authorizer authz.Authorizer } -func NewHandler(logger *slog.Logger, store store, authorizer authz.Authorizer) *Handler { - return &Handler{logger: logger, store: store, authorizer: authorizer} +func NewHandler(logger *slog.Logger, store store, floor retentionFloor, authorizer authz.Authorizer) *Handler { + return &Handler{logger: logger, store: store, floor: floor, authorizer: authorizer} } // RegisterRoutes: both routes are RoleAdmin -- RoleOwner satisfies it @@ -58,6 +67,37 @@ func parseOlderThanHours(r *http.Request) (int, bool) { return hours, true } +// checkRetentionFloor enforces api/agents.ConfigOverride.LogRetentionDays +// as a hard floor against anyone but an owner: if any agent has a +// configured retention, the largest one across all agents is the +// earliest boundary a non-owner may delete up to. An owner always +// bypasses this (identity.Role == RoleOwner short-circuits before ever +// querying the floor) -- "owner and admin" gates the routes themselves +// (RegisterRoutes), but this narrows what admin specifically can do +// once inside them, the same shape handleSetConfig's own +// log_retention_days gate uses on the agents side. A nil identity (no +// authorizer configured at all, Phase 0-3 default-open) skips this +// too, consistent with every other RBAC check in this codebase being a +// no-op when there's no RBAC to begin with. +func (h *Handler) checkRetentionFloor(ctx context.Context, cutoff time.Time) (string, error) { + identity, ok := authz.IdentityFromContext(ctx) + if !ok || identity.Role == authz.RoleOwner { + return "", nil + } + maxDays, hasFloor, err := h.floor.MaxRetentionDays(ctx) + if err != nil { + return "", err + } + if !hasFloor { + return "", nil + } + protectedBoundary := time.Now().UTC().Add(-time.Duration(maxDays) * 24 * time.Hour) + if cutoff.After(protectedBoundary) { + return fmt.Sprintf("a configured log retention policy protects logs newer than %d days; only an owner can override this", maxDays), nil + } + return "", nil +} + type previewResponse struct { Count uint64 `json:"count"` Cutoff time.Time `json:"cutoff"` @@ -71,6 +111,15 @@ func (h *Handler) handlePreview(w http.ResponseWriter, r *http.Request) { } cutoff := time.Now().UTC().Add(-time.Duration(hours) * time.Hour) + if msg, err := h.checkRetentionFloor(r.Context(), cutoff); err != nil { + h.logger.Error("checking log retention floor", "error", err) + writeError(w, http.StatusInternalServerError, "checking retention policy failed") + return + } else if msg != "" { + writeError(w, http.StatusForbidden, msg) + return + } + count, err := h.store.CountOlderThan(r.Context(), cutoff) if err != nil { h.logger.Error("counting logs for retention preview", "error", err) @@ -101,6 +150,15 @@ func (h *Handler) handleDelete(w http.ResponseWriter, r *http.Request) { } cutoff := time.Now().UTC().Add(-time.Duration(hours) * time.Hour) + if msg, err := h.checkRetentionFloor(r.Context(), cutoff); err != nil { + h.logger.Error("checking log retention floor", "error", err) + writeError(w, http.StatusInternalServerError, "checking retention policy failed") + return + } else if msg != "" { + writeError(w, http.StatusForbidden, msg) + return + } + count, err := h.store.CountOlderThan(r.Context(), cutoff) if err != nil { h.logger.Error("counting logs before retention delete", "error", err) diff --git a/api/logretention/handler_test.go b/api/logretention/handler_test.go index 4f1fd04..f767463 100644 --- a/api/logretention/handler_test.go +++ b/api/logretention/handler_test.go @@ -8,6 +8,7 @@ import ( "log/slog" "net/http" "net/http/httptest" + "strconv" "testing" "time" @@ -50,8 +51,21 @@ func (f fakeAuthorizer) Authorize(*http.Request) (authz.Identity, error) { return authz.Identity{TenantID: "default", UserID: "u1", Role: f.role}, nil } +// fakeFloor stands in for AgentRetentionStore -- hasFloor false (the +// zero value) means no agent has log_retention_days configured, same +// as every existing test in this file assumed before the floor existed. +type fakeFloor struct { + days int + hasFloor bool + err error +} + +func (f fakeFloor) MaxRetentionDays(context.Context) (int, bool, error) { + return f.days, f.hasFloor, f.err +} + func newTestHandler(s *fakeStore, role authz.Role) *Handler { - return NewHandler(discardLogger(), s, fakeAuthorizer{role: role}) + return NewHandler(discardLogger(), s, fakeFloor{}, fakeAuthorizer{role: role}) } func doRequest(t *testing.T, h *Handler, method, path string) *httptest.ResponseRecorder { @@ -193,7 +207,7 @@ func TestViewerAndEditorAreForbiddenFromRetentionRoutes(t *testing.T) { func TestRetentionRoutesRequireAuth(t *testing.T) { s := &fakeStore{} - h := NewHandler(discardLogger(), s, nil) + h := NewHandler(discardLogger(), s, fakeFloor{}, nil) // A nil authorizer is Phase 0-3's default-open behavior (see // authz.RequireRole's doc comment) -- confirm that posture applies @@ -205,3 +219,81 @@ func TestRetentionRoutesRequireAuth(t *testing.T) { t.Fatalf("status with nil authorizer = %d, want 200 (default-open, matches RequireRole elsewhere)", rec.Code) } } + +// TestAdminBlockedByRetentionFloor is the core regression test for the +// owner-only override: an agent configured with a 90-day retention +// floor must block an admin's attempt to delete anything newer than +// that, on both preview and delete. +func TestAdminBlockedByRetentionFloor(t *testing.T) { + s := &fakeStore{count: 100} + h := NewHandler(discardLogger(), s, fakeFloor{days: 90, hasFloor: true}, fakeAuthorizer{role: authz.RoleAdmin}) + + // 30 days is newer than the 90-day floor -- must be blocked. + preview := doRequest(t, h, "GET", "/logs/retention/preview?older_than_hours="+hoursForDays(30)) + if preview.Code != http.StatusForbidden { + t.Fatalf("preview at 30d against a 90d floor: status = %d, want 403, body=%s", preview.Code, preview.Body.String()) + } + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours="+hoursForDays(30)) + if del.Code != http.StatusForbidden { + t.Fatalf("delete at 30d against a 90d floor: status = %d, want 403, body=%s", del.Code, del.Body.String()) + } + if len(s.countedWith) != 0 || len(s.deletedWith) != 0 { + t.Error("a blocked request must never reach the store at all") + } +} + +// TestAdminAllowedBeyondRetentionFloor confirms the floor only blocks +// requests that would actually reach into the protected window -- a +// request older than the floor itself is unaffected by it. +func TestAdminAllowedBeyondRetentionFloor(t *testing.T) { + s := &fakeStore{count: 5} + h := NewHandler(discardLogger(), s, fakeFloor{days: 90, hasFloor: true}, fakeAuthorizer{role: authz.RoleAdmin}) + + // 120 days is older than the 90-day floor -- must be allowed. + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours="+hoursForDays(120)) + if del.Code != http.StatusOK { + t.Fatalf("delete at 120d against a 90d floor: status = %d, want 200, body=%s", del.Code, del.Body.String()) + } +} + +// TestOwnerBypassesRetentionFloor confirms the whole point of the +// feature: an owner can still delete within a configured retention +// window that blocks everyone else. +func TestOwnerBypassesRetentionFloor(t *testing.T) { + s := &fakeStore{count: 100} + h := NewHandler(discardLogger(), s, fakeFloor{days: 90, hasFloor: true}, fakeAuthorizer{role: authz.RoleOwner}) + + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours="+hoursForDays(1)) + if del.Code != http.StatusOK { + t.Fatalf("owner deleting within the floor: status = %d, want 200, body=%s", del.Code, del.Body.String()) + } +} + +// TestNoConfiguredFloorNeverBlocksAdmin confirms the default, common +// case (no agent has log_retention_days set) behaves exactly as before +// this feature existed -- fakeFloor{} (hasFloor: false) is what every +// other test in this file already relies on, this just makes the +// no-floor-configured case explicit. +func TestNoConfiguredFloorNeverBlocksAdmin(t *testing.T) { + s := &fakeStore{count: 9} + h := NewHandler(discardLogger(), s, fakeFloor{hasFloor: false}, fakeAuthorizer{role: authz.RoleAdmin}) + + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=1") + if del.Code != http.StatusOK { + t.Fatalf("status = %d, want 200 with no configured floor", del.Code) + } +} + +func TestRetentionFloorCheckPropagatesStoreErrors(t *testing.T) { + s := &fakeStore{} + h := NewHandler(discardLogger(), s, fakeFloor{err: errors.New("postgres unreachable")}, fakeAuthorizer{role: authz.RoleAdmin}) + + rec := doRequest(t, h, "GET", "/logs/retention/preview?older_than_hours=24") + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } +} + +func hoursForDays(days int) string { + return strconv.Itoa(days * 24) +} diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index b60b6d5..72ed0e4 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -590,6 +590,11 @@ export type ConfigOverride = { heartbeat_interval_ms?: number; journald_unit?: string; extra_file_paths?: string[]; + // log_retention_days is owner-only to change (see api/agents/handler.go's + // changesLogRetentionDays) -- it's a central policy tag api/logretention + // reads as a protective floor, not something the agent process itself + // ever sees or applies. + log_retention_days?: number; }; export type Agent = { diff --git a/web/src/routes/agents/[host]/+page.svelte b/web/src/routes/agents/[host]/+page.svelte index 457c9e5..9438171 100644 --- a/web/src/routes/agents/[host]/+page.svelte +++ b/web/src/routes/agents/[host]/+page.svelte @@ -33,6 +33,11 @@ // an agent with no override starts with an empty list, not // something derived from `agent`. let extraFilePaths = $state([]); + // Same "no effective value to fall back to" shape as extra_file_paths + // -- and like that field, the agent process itself never reads this + // one either. Kept as a string ('' = no override) so the input can be + // empty rather than defaulting to some arbitrary number of days. + let logRetentionDays = $state(''); function resetForm(a: Agent) { const o = a.desired_override; @@ -42,6 +47,7 @@ heartbeatIntervalMs = String(o?.heartbeat_interval_ms ?? a.heartbeat_interval_ms); journaldUnit = o?.journald_unit ?? ''; extraFilePaths = o?.extra_file_paths ? [...o.extra_file_paths] : []; + logRetentionDays = o?.log_retention_days != null ? String(o.log_retention_days) : ''; } function addExtraFilePath() { @@ -76,7 +82,16 @@ heartbeat_enabled: heartbeatEnabled, heartbeat_interval_ms: Number(heartbeatIntervalMs), ...(agent?.source_kind === 'journald' ? { journald_unit: journaldUnit } : {}), - extra_file_paths: extraFilePaths.map((p) => p.trim()).filter((p) => p !== '') + extra_file_paths: extraFilePaths.map((p) => p.trim()).filter((p) => p !== ''), + // String(...) first, deliberately -- Input's `value` prop is + // declared string, but a type="number" input's bind:value + // actually hands back a real JS number once the user has + // typed into it (only the initial resetForm-assigned value is + // guaranteed to be a string), so a bare .trim() here throws + // the moment someone edits this field. batch_max_size etc. + // above never hit this because Number(x) doesn't care + // whether x is already a number. + ...(String(logRetentionDays).trim() !== '' ? { log_retention_days: Number(logRetentionDays) } : {}) }); } catch (e) { saveError = e instanceof Error ? e.message : String(e); @@ -211,6 +226,22 @@ +
+ + +

+ Owner only. Once set, this host's logs can't be deleted by age (Settings → Log retention) by anyone + other than an owner until they're at least this many days old. +

+
+ {#if saveError}

Error: {saveError}

{/if}