Add per-agent log retention floor, owner-only to set or override
api/agents.ConfigOverride gains LogRetentionDays: a per-agent setting edited on the same remote-config page as extra_file_paths, but unlike every other field there it's central-policy metadata api/logretention reads, never something the agent process itself sees. Any change to it -- setting, raising, lowering, or clearing -- requires RoleOwner, not just RoleAdmin: the whole point of the field is a floor an admin can't move, so an admin able to freely edit it would defeat that. api/logretention now checks the largest LogRetentionDays configured across any agent (AgentRetentionStore, new) before every preview/delete: a non-owner's request is rejected with a clear 403 if it would reach into that protected window. An owner always bypasses it, matching "make the log retention override any attempts to delete logs by anyone other than owner role." Verified live end-to-end: owner sets a 90-day floor on an agent, admin is blocked deleting anything newer than that (both preview and delete), allowed beyond it, and owner bypasses it entirely -- confirmed against real ClickHouse data, not just the fake-backed unit tests. Also caught and fixed a real pre-existing latent bug while verifying in-browser: a type="number" Input's bind:value becomes an actual JS number once a user types into it (only the initial value is a string), which broke a bare .trim() call on the new field.
This commit is contained in:
@@ -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")
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user