Review follow-ups: durable alert acceptance, credential epochs, fatal privilege drop by yellowman · Pull Request #77 · yellowman/sysmon

yellowman · 2026-08-22T00:01:03Z

Fixes from the post-merge review rounds on the alerters feature (PR #76). The acceptance contract is now: 333 accepted means the alert is durably committed to the history store; push notification is a best-effort delivery path layered on top, never a precondition.

Alerter acceptance and ordering

History-first acceptance: 333 is only sent after hist.Append commits to bbolt. A storage failure returns 444 try later and leaves alerter state (lastStatus, rate bucket bookkeeping) untouched, so a retry after failure behaves as a first attempt.
Per-identity alert queue with a single dispatcher goroutine, so a reconnect cannot reorder or drop alerts queued under the previous socket.
Overlong greeting/alert lines are bounded reads with explicit refusal instead of silent truncation.
Simple token-bucket rate limit per alerter (burst 30, 1/s refill) refusing with 444 rate limited before commit.

Credential lifecycle

MintAgentToken(site, label, kind, replace) is one bolt transaction: exists-check, hash, label, kind, and a fresh random credential epoch commit together. Two racing mints cannot both hand out a token; the loser gets ErrTokenExists (409 with details in the API, clear message in the CLI).
Kind (sysmond vs alerter) is chosen at mint time by the admin; the greeting verb must match. Legacy pre-kind records take the kind of their first successful greeting, first claim wins, inside one transaction.
Credential epochs close the revoke/re-mint vs in-flight handshake races: a connection remembers the epoch it authenticated under, registerAlerter/adoptAgent pre-check it under lock, and a post-registration recheck catches a revoke that lands mid-handshake. DisconnectSite linearizes with acceptance via the same lock, so revocation cuts live connections without racing a concurrent replacement.
Store errors fail closed everywhere on the auth path: CheckAgentToken, ClaimAgentKind, and SetAgentLabel surface errors instead of guessing, and the auth callback refuses on store failure.

sysmond (C)

A failed privilege drop is fatal: revoke_root_if_necessary reports failure at every step (getpwnam, setgroups, setgid, setuid, verification), and the caller logs CRITICAL, unlinks the pidfile, and exits rather than running as root.
A paused daemon now honors SIGTERM: the pause loop also watches stop_daemon.
Ping-helper detection uses stat instead of spawning the helper to probe it.

Startup files and service hardening

New misc/sysmond.service and misc/rc.d/sysmond startup files; removed a dead -sysmon flag from the shipped sysmon-web.service and rc.d script that would have killed the process at startup.
sysmon-web.service prepares its runtime/backup/audit paths in ExecStartPre and narrows ReadWritePaths to the audit file.

UI, docs, API

History (web, Android, iOS) splits the source box into its own visually minimized column instead of overloading names as source:object, with read-side backfill for old rows; an alerter's first sighting of an object renders without a bogus previous state.
Agents page is now "Agents & alerters" with a kind selector at mint, kind column, and an alerter greeting panel.
docs/ALERTERS.md documents the real contract: what 333 guarantees, the 444 catalog, line-length limits, and revocation behavior, with a hardened example client.
api/openapi.yaml now tells the truth: real auth schemes, real rate limits, agents endpoints with kind.

Tests cover the contested interleavings (reconnect ordering, revoke during handshake, stale-producer commit attempts, concurrent mint, storage failure fail-closed paths) using two test-only barrier hooks that are nil in production.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

Generated by Claude Code

Note

Overview
Hardens the alerter path so 333 ok means the event is already in the history store; push is best-effort after that commit. Overlong lines are refused, ingestion is rate-limited, and a full push queue no longer rejects an already-recorded alert.

Credentials are typed at mint (sysmond vs alerter) in one store transaction, with a per-mint epoch so revoke/re-mint cuts live sockets and cannot race an in-flight handshake. Store failures fail closed. History rows now carry a stable id; mobile history shows alerter text and skips empty previous-state badges.

sysmond treats a failed privilege drop as fatal and lets SIGTERM break a pause so graceful stop still saves state. Foreground sysmon-web logs to stderr; OpenAPI and alerter docs match the real session auth and 333/444 contract.

^{Reviewed by Cursor Bugbot for commit 041390b. Bugbot is set up for automated code reviews on this repo. Configure here.}

An external review of the merged alerter work found five blocking problems and a set of operability defects. This addresses all of them. Alert delivery is now serialized per identity, not per socket: the queue and its dispatcher belong to the alerter's name, created at first sight and fed by every connection that ever speaks for it. Two per-connection dispatchers could race a reconnect - an OK accepted on the new socket overtaking a CRITICAL still queued behind the old one, with the shared collapse key leaving the phones stuck on the stale CRITICAL - and every abandoned connection could strand another 64 queued alerts behind a blocked worker. A test reproduces exactly that sequence and asserts WARNING, CRITICAL, OK arrive in the order they were accepted across the reconnect. 333 ok no longer promises what the server cannot do. An alert with no delivery path - no push service, or push disabled - is refused with a 444 naming the reason, instead of being acknowledged and dropped where only the server log would ever know; the sender is the one party that can page some other way. ALERTERS.md now states the contract exactly: 333 means accepted for immediate, in-order delivery, not a durable receipt; the server keeps no alert on disk. Overlong protocol lines are likewise refused (444 line too long) rather than silently processed as a truncation of what the peer actually sent, on the handshake and every line after it. Revoking or re-minting a token now cuts the live connection, daemon or alerter, through the new DisconnectSite - authentication happens once at the greeting and the sockets are long-lived by design, so "revoked at the next connection" used to mean never for a peer that kept its socket up. The credential's type is also chosen at mint time now (the Add-a-box form has a Monitoring box / External alerter selector, the token table shows it, and the fresh-token panel shows an ALERTER greeting instead of sysmon.conf lines for alerter credentials), so a mistaken first greeting can no longer claim a token's kind forever. The credential store fails closed: SetAgentLabel, ClaimAgentKind and CheckAgentToken return errors instead of swallowing bolt failures, the authenticator refuses when its store cannot answer, the kind claim refuses rather than admitting a peer whose binding never stuck, and the nickname endpoint reports a failed write instead of confirming it. A paused sysmond now honors SIGTERM: the pause loop tested only paused and gotsighup, so a service stop during a pause hung until the supervisor escalated to SIGKILL - skipping the graceful state save. The drop path also clears supplementary groups (setgroups before setgid/setuid), which the helper probe's group check already assumed. The systemd unit prepares every path the www-data process cannot create - the backup directory and the audit log alongside the socket directory - since the binary's own root-time preparation never runs under User=www-data; the ReadWritePaths carve-out narrows from all of /var/log to the one audit file. And -foreground now logs to stderr, so the production unit gets warnings and errors in the journal instead of io.Discard; -debug is extra detail, not the price of having logs. Smaller corrections from the same review: the push log records alerter events under source:object so two alerters reporting the same object name stay distinguishable; the Fleet alerter card shows since when or how long gone, strips the ephemeral source port from the address (full value in the tooltip), and hides the rename pencil from non-admins who would only 403; the navigation collapses to the hamburger up to the lg breakpoint instead of hiding overflow links behind an invisible scrollbar; the iOS host row ellipsizes long names with layout priority so the site tag survives; and the ALERTERS.md Python example verifies the certificate hostname and reads the token from the environment instead of source. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

The setgroups call added for the supplementary-groups fix copied the surrounding pattern of warn-and-return on failure - but returning there skips setgid and setuid entirely, leaving the daemon fully root. That failure path did not exist before the call was added, and it is strictly worse than what it replaced: a uid/gid drop with stale supplementary groups is a partial drop, staying root is none at all. Warn and carry on instead. The one side effect of continuing - a retained group could keep the ping helper usable after the probe called it unusable - errs in the conservative direction, and only on this rare failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

…h gate The previous commit made the alerter protocol refuse alerts whenever push was disabled, on the reviewer's premise that a phone is the only place an accepted alert can go. It is not. The web UI's alert history was sitting right there - durable, bbolt-backed, already rendered on the History page - and an alert that shows up there has been delivered to something an operator looks at. So the contract inverts. An accepted ALERT is now recorded in the same history store the hosts' transitions land in, keyed source:object with the halves split out like everything else, the alert text as the description, and - once this server has seen the object before - the status it changed from, so the History page shows "CRITICAL -> OK, was critical for 20m" for alerter objects exactly as it does for hosts. The write commits before the 333 goes out, which quietly gives the acknowledgment the durability the review wanted an outbox for: what was accepted survives a server restart, in the UI if not in the phones' queue. Push stays exactly what it was - the extra channel when enabled - and the "push delivery is disabled" refusal is gone along with the whole gate mechanism; the only server that still refuses is one with neither a history store nor a push service, where an accepted alert genuinely goes nowhere. The 444 busy refusal is checked before the history write, so a client's retry cannot duplicate a history row. History.Append also stops swallowing its transaction error - the alerter path acks against it, the poller logs it - and the History page hides the previous-status badge on first-sighting rows instead of rendering an empty pill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

… lying Round two of the external review, against what it verified and what had already moved on. The mint-time credential type was not actually an invariant: the API minted the token in one transaction and recorded the kind in a second whose failure was silently swallowed, so it could hand out a plaintext token claiming a type the store never learned - and the provisioning CLI never recorded a kind at all, leaving every CLI-minted token first-greeting-wins. NewAgentToken now takes the kind and commits hash, label and kind in one transaction, validating it first; the API passes the chosen kind, the CLI mints sysmond credentials by definition (it prints a sysmon.conf block), and SetAgentKind survives only as an error-returning migration/test helper. ClaimAgentKind remains what migrates records minted before kinds existed, and the tests that exercise that path now blank the kind explicitly to simulate such a record. A root-started daemon that cannot completely drop root now refuses to run: revoke_root_if_necessary returns a verdict, every failure in the chain - no drop identity, setgroups, setgid, setuid, or IDs still zero afterwards - is CRITICAL, and the caller removes the pidfile and exits instead of entering the monitoring loop as root. The previous warn-and-continue paths meant a failed setgid left the daemon running with privileges nobody chose, on the strength of a log line nobody was reading yet. Not running as root remains the ordinary non-drop path. The served OpenAPI document described an API that does not exist: a 60-per-minute limiter and X-Auth-Key header auth. It now documents the real contract - session auth via Bearer token or the sysmon_session cookie, 10-per-minute login and 300-per-minute general limits with logout never limited - and gains the agent-credential endpoints (mint with kind, list, revoke-with-disconnect) that were missing entirely. The credentials page stops calling everything a box now that it mints alerter credentials too: retitled Agents & alerters, Add credential, kind-aware re-mint and revoke confirmations (revoke gets the red button), and the empty-state row spans all seven columns instead of six. References elsewhere follow. ALERTERS.md clarifies that the overlong-line refusal keeps the connection only after authentication - an overlong greeting is a failed handshake and closes - and that mint-time typing applies to new credentials while legacy blank-kind records still take the kind of their first greeting. Not changed, deliberately: the reviewer's DeliveryGate finding was against a mechanism the previous commit already removed - since the alert history became a delivery path in its own right, acceptance no longer depends on the state of the push providers at all, and the no-provider cases it lists are exactly what the history record is for. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

… atomic Round three of the external review: the three remaining blockers, and the trailing defects. The 333 now means exactly what the protocol document says it means. handleAlertLine requires a history store (a server without one refuses every ALERT - push alone cannot back a durable acknowledgment), commits the history event BEFORE replying, and only then attempts the push enqueue. A failed history write refuses with a retryable 444 regardless of push, and no longer advances the object's remembered previous status, so the retry records the same transition the failed attempt tried to. A saturated push queue can no longer refuse an accepted alert - the retry it invited would duplicate the durable record - so the skipped phone delivery becomes a logged delivery failure instead. HistoryStore.Append stops advancing its in-memory duration clock inside the transaction (a rolled-back append no longer moves it) and surfaces marshal and sequence errors instead of skipping rows. Revocation and re-minting can no longer lose to an in-flight handshake. Every mint stamps a random credential epoch into the record; authentication returns it, and after the connection registers - for daemons and alerters both - the credential is checked again. Register-then-recheck against the admin's write-then-sweep means one side always sees the other: a revoke that lands mid-handshake fails the recheck, and a handshake that finishes first is found by the sweep. A re-mint changes the epoch, so a connection holding the old token dies even though the record is not revoked. A replaced connection can no longer commit a stale event. Acceptance takes a per-identity lock, verifies the line's connection is still the record's current one, and commits under that lock; the reconnect swap takes the same lock. The only two outcomes are the documented ones: the old alert commits before the replacement, or it is refused. Minting is one transaction end to end: MintAgentToken folds the does-a-live-token-exist check into the write, so two racing first mints produce exactly one token (the loser gets ErrTokenExists and the API's 409), replace stays explicit, and revoked records may be re-minted freely. The OpenAPI document gains the /api/auth/login and logout paths its own introduction points at, a global security requirement so protected reads stop looking public in Swagger, and loses the four obsolete auth_key request fields. The History page's small column becomes Source and the name column Object, which is true for host and alerter rows alike, and the filter placeholder stops promising hosts only. ALERTERS.md drops the 444 busy reply, documents the two history refusals, and states the mint-time/legacy typing rule precisely. New tests pin each blocker: push-but-no-history refuses, history failure with a live sink refuses without counting or pushing, a retry after a failed write records a first sighting, a saturated push queue still records and accepts, a parsed event from a replaced connection is rejected, revoke and re-mint landing mid-handshake both keep the connection out, and concurrent first mints yield exactly one winner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

…gistry Round four: the two remaining credential races, the flood and read-failure gaps behind them, and the mobile history stragglers. DisconnectSite now does what a reconnect replacement does: it takes the identity's acceptance lock and invalidates the connection UNDER it - a.conn cleared, the record marked disconnected - before closing the socket. That gives revocation a real ordering against an alert the connection had already read: either the alert's acceptance held the lock first and its commit completes before the revoke API returns, or the revoke wins and the stalled event finds its connection gone and is refused. Merely closing the socket left a.conn intact, and one already-parsed event could commit after revocation had reported success. Daemon connections are detached (nilled), not just closed, for the same reason. A stale-epoch handshake can no longer evict the connection that replaced it. The credential epoch is checked INSIDE the registry lock - registerAlerter and adoptAgent both - before the current connection is touched, and a stale epoch is refused with no mutation at all; previously the stale handshake would close the legitimate new-epoch peer, wipe a daemon's sequence state and host cache, and only then notice it was stale. The post-registration recheck remains for a store write landing after the in-lock check, and its failure detaches only the checking connection, never one that has since replaced it. Live records now carry their epoch. The race tests actually pin the claimed interleavings now: two test barriers (after parse / before acceptance, and after registration / before the epoch recheck) let tests hold a goroutine exactly inside the contested window. New coverage: a revocation completing while a parsed alert is stalled cannot see that alert commit afterwards; a revoke landing between registration and recheck is caught; a stale alerter or daemon handshake leaves the current-epoch connection registered and answering. Alert ingestion is rate limited per identity - a burst of 30 refilling at one per second, refused with 444 rate limited BEFORE anything commits - so one looping cron script or a compromised credential cannot churn the fleet's shared, bounded history store. Recent() returns its read error and the history API answers 503 instead of presenting an unreadable store as an empty healthy one. History events carry the store's sequence number as an immutable id. Mobile catches up with events that are now possible: both apps hide the previous-status badge and arrow on first-sighting rows instead of rendering a blank label and dangling arrow, show the alert's own message under the object name, rename the Downs filter to Problems (warnings and alerter events are included), and key history rows by the new event id - iOS previously collided rows for same-status alerts within one second. Comments and ALERTERS.md stop describing deleted behavior (444 busy, the accept-time gate, first-handshake kinds, the Push Log claim for queue-full skips) and document the rate limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NpxQtrW7f2YaKiYL6evoTZ

cursor

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 041390b. Configure here.}

cursor · 2026-08-22T00:03:19Z

+		// this 444 asks for must see the same prev status and must not
+		// duplicate anything.
+		log.Printf("agents: alerter %s: recording %s %s to history failed: %v", name, status, object, err)
+		return "could not record the alert - try again"


Rate limit spent before commit

Medium Severity

handleAlertLine decrements the ingestion token bucket before hist.Append succeeds. On a storage failure it returns 444 could not record without restoring the token, so a transient write error both refuses the alert and burns rate budget. That contradicts the acceptance contract that a failed commit leaves rate-bucket bookkeeping untouched so a retry behaves like a first attempt.

^{Reviewed by Cursor Bugbot for commit 041390b. Configure here.}

cursor · 2026-08-22T00:03:19Z

+		a.svc.detachDaemonConn(site, conn)
+		conn.Close()
+		log.Printf("agents: site %s (%s): credential revoked or replaced during handshake - dropping", site, remote)
+		return


Welcome before epoch refusal

Medium Severity

The handshake writes 333 welcome before registerAlerter / adoptAgent and the post-registration credential recheck. Those new epoch checks can still refuse and close the socket, so a peer can see success and then get a silent drop with no 444. Docs define 333 welcome as authenticated and ready to send; that promise is broken in the revoke/re-mint mid-handshake window this change adds.

Additional Locations (1)

web-ui/backend/internal/monitoring/alerters.go#L245-L283

^{Reviewed by Cursor Bugbot for commit 041390b. Configure here.}

claude added 6 commits August 21, 2026 19:15

yellowman merged commit cc05963 into main Aug 22, 2026
11 checks passed

cursor Bot reviewed Aug 22, 2026

View reviewed changes

Navigation Menu

Review follow-ups: durable alert acceptance, credential epochs, fatal privilege drop - #77

Review follow-ups: durable alert acceptance, credential epochs, fatal privilege drop#77
yellowman merged 6 commits into
mainfrom
claude/fcm-token-expiration-renewal-q15761

yellowman commented Aug 22, 2026 •

edited by cursor Bot

Loading

Uh oh!

Uh oh!

cursor Bot left a comment

Uh oh!

cursor Bot Aug 22, 2026

Uh oh!

cursor Bot Aug 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

yellowman commented Aug 22, 2026 • edited by cursor Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Alerter acceptance and ordering

Credential lifecycle

sysmond (C)

Startup files and service hardening

UI, docs, API

Uh oh!

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

cursor Bot Aug 22, 2026

Choose a reason for hiding this comment

Rate limit spent before commit

Uh oh!

cursor Bot Aug 22, 2026

Choose a reason for hiding this comment

Welcome before epoch refusal

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yellowman commented Aug 22, 2026 •

edited by cursor Bot

Loading