Skip to content

Commit 1e603e6

Browse files
authored
fix: owner-only vantage bundle, NTP clear watermark, reload warm-start of redefined targets (#135)
* fix(vantage): write the vantage add -out bundle owner-only (0600) The onboarding tar.gz embeds the vantage's client private key, but os.Create gave it 0666 minus umask, so 0644 under the usual 022 umask. Create it 0600 and tighten an existing file it overwrites before any key bytes are written. * fix(api): keep the NTP stat's freshness watermark when a round clears it Clearing a remote vantage's NTP clock stat deleted the entry, timestamp included, so a store-and-forward replay of an older synchronized round was accepted as the first reading and the dashboard showed an obsolete offset next to the newer unsynchronized round. Leave a timestamped tombstone that reads as absent but still gates older sets. * fix(alert): don't warm-start a redefined target from its old measurement On a reload, swapRuntime seeded every alerted target's window from stored history matched only by host/probe/metric/step. A target whose measurement fingerprint changed without changing those (a TCPConnect port 443 -> 22) got the old port's rounds, so one new-port failure fired a CheckLoss(x=3) alert. Skip the seed for targets whose fingerprint changed between the old and new runtime; same-identity targets, including one that just gained an alert, are still seeded.
1 parent e23181c commit 1e603e6

7 files changed

Lines changed: 252 additions & 32 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,21 @@ All notable changes to **Heliograph** are recorded here. The format follows
1313
once a newer revision was published (`libcrypto3`/`libssl3` 3.5.9-r0, `curl` 8.22.0-r0), the pinned ones
1414
disappeared and every fresh build failed in the `apk` step. Those pins are now `>=` minimum
1515
versions: the CVE fixes still can't regress, and newer revisions install without breaking the build.
16+
- **A delayed NTP round no longer brings back an obsolete clock offset after it was cleared.** When a
17+
remote vantage reports an unsynchronized round, the hub clears that vantage's clock stat for the
18+
target, but the clear also dropped the stat's freshness timestamp. A store-and-forward replay of an
19+
*older* synchronized round could then re-set the offset, so `/api/targets?vantage=…` and the
20+
dashboard showed an out-of-date offset next to the newer round. The clear now keeps its timestamp,
21+
so only a round newer than it can set the stat again.
22+
- **A config reload no longer pre-fills a redefined target's alert window with its old measurement.**
23+
On a reload (SIGHUP or a config apply), alert windows are warm-started from stored history, which
24+
was matched only by host, probe, metric and step. Changing what a target measures without changing
25+
those, such as a `TCPConnect` port from `443` to `22`, re-imported the old port's rounds, so a
26+
`CheckLoss(x=3)` alert fired on the *first* failure of the new port. A target whose measurement
27+
identity changed in a reload is no longer warm-started: its alerts wait for `x` rounds of the new
28+
measurement. A target that only gains an alert on reload is still warm-started. On a cold start,
29+
smoked has no record of a target's previous definition, so a target redefined while smoked was
30+
stopped can still be warm-started from its recent old rounds.
1631

1732
### Security
1833
- **Revoking and re-adding a vantage now retires its earlier certificates.** Agent authorization
@@ -35,6 +50,11 @@ All notable changes to **Heliograph** are recorded here. The format follows
3550
SCRAM password handling. That path runs the operator-supplied database password through a
3651
different precis profile from the one the advisory names, so no remote trigger is known; the bump
3752
clears the scan. Caddy's `--with golang.org/x/text` floor is raised to match.
53+
- **`smoked vantage add -out` writes the onboarding bundle owner-only.** The `.tar.gz` embeds the
54+
vantage's client private key in `agent.yaml`, but was created with the default `0666`-minus-umask
55+
mode, which is world-readable (`0644`) under the usual `022` umask. It is now created `0600`
56+
regardless of umask, and an existing file that `-out` overwrites is tightened to `0600` before any
57+
key material is written to it.
3858

3959
## [2.2.0] - 2026-09-05
4060

‎cmd/smoked/main.go‎

Lines changed: 80 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -310,11 +310,13 @@ func main() {
310310
// evalMu (inside swapRuntime) only guards the swap; applyMu is strictly outside it.
311311
var applyMu sync.Mutex
312312
// seedFn seeds a freshly built engine's alert windows from durable history on a reload,
313-
// so a target that is newly alerted or redefined isn't left dark until X fresh samples
314-
// arrive (CODE_REVIEW #4). Assigned once the store exists (below); the SIGHUP goroutine and
315-
// the API apply both pass it into swapRuntime. It reads the store, so swapRuntime runs it
316-
// before taking evalMu. nil until assigned — an early reload simply skips seeding.
317-
var seedFn func(*runtime)
313+
// so a target with no live window to inherit (e.g. one that just gained an alert) isn't
314+
// left dark until X fresh samples arrive (CODE_REVIEW #4). swapRuntime picks which targets
315+
// it may seed (reloadSeedTargets). Assigned once the store exists (below); the SIGHUP
316+
// goroutine and the API apply both pass it into swapRuntime. It reads the store, so
317+
// swapRuntime runs it before taking evalMu. nil until assigned — an early reload simply
318+
// skips seeding.
319+
var seedFn func(*runtime, []model.Monitor)
318320
if *configPath != "" {
319321
fmt.Printf("config: %d targets from %s\n", len(rt.jobs), *configPath)
320322
}
@@ -347,11 +349,11 @@ func main() {
347349
}
348350

349351
// Now that the store exists, wire the reload seed: on a config reload, swapRuntime seeds
350-
// the new engine from durable history for targets it can't inherit (redefined or newly
351-
// alerted), then InheritStateFrom overwrites the seed for same-identity targets (#4).
352-
seedFn = func(nrt *runtime) {
353-
warmStartAlerts(ctx, nrt.engine, nrt.monitors, nrt.metricByName, st, time.Now())
354-
}
352+
// the new engine from durable history for every target whose measurement identity is
353+
// unchanged, then InheritStateFrom overwrites the seed with the live window where one
354+
// exists, so the seed only fills windows the running engine doesn't have, e.g. a newly
355+
// alerted target's (#4).
356+
seedFn = historySeed(ctx, st)
355357

356358
// SIGHUP reloads the config; on error the running config is kept (a bad edit can't take the
357359
// collector down). Alert firing state and sample windows carry over from the running engine
@@ -385,7 +387,10 @@ func main() {
385387
// that is already breaching at startup fires on its first new round instead of
386388
// waiting X fresh samples (the durable-store replacement for SmokePing's S sentinel).
387389
// Boot uses this directly; a SIGHUP/API reload runs the same seed via seedFn inside
388-
// swapRuntime and then carries unchanged windows/state via InheritStateFrom.
390+
// swapRuntime and then carries unchanged windows/state via InheritStateFrom. Unlike a
391+
// reload, boot has no previous fingerprint to compare (the store keeps none), so a target
392+
// redefined while smoked was down can still be seeded from its old measurement's rounds
393+
// if they are recent enough to pass recentContiguous.
389394
if rt := current.Load(); rt.engine != nil {
390395
warmStartAlerts(ctx, rt.engine, rt.monitors, rt.metricByName, st, time.Now())
391396
}
@@ -822,6 +827,27 @@ func main() {
822827
}
823828
}
824829

830+
// createOwnerOnly opens path for writing (created or truncated) readable by its owner only, for a
831+
// file that will hold a secret. A new file is created 0600 whatever the umask; an existing regular
832+
// file with any group/other permission bits is tightened to 0600 before the caller writes to it, so
833+
// overwriting a world-readable file can't leave the secret world-readable. A non-regular path (a
834+
// FIFO, /dev/stdout) is left alone: its mode isn't this file's to change.
835+
func createOwnerOnly(path string) (*os.File, error) {
836+
f, err := os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0o600)
837+
if err != nil {
838+
return nil, err
839+
}
840+
fi, err := f.Stat()
841+
if err == nil && fi.Mode().IsRegular() && fi.Mode().Perm()&0o077 != 0 {
842+
err = f.Chmod(0o600)
843+
}
844+
if err != nil {
845+
f.Close()
846+
return nil, err
847+
}
848+
return f, nil
849+
}
850+
825851
// vantageCmd implements `smoked vantage <add NAME|ls|revoke NAME> [-dsn DSN]` — provisioning
826852
// against the same TimescaleDB the daemon uses. The subcommand and the vantage NAME are
827853
// positional (git-style), so flags may follow them: `vantage add nyc -dsn X`. -dsn defaults
@@ -886,7 +912,8 @@ func vantageCmd(args []string) int {
886912

887913
switch {
888914
case *out != "":
889-
f, err := os.Create(*out)
915+
// The bundle embeds the client private key: owner-only, never os.Create's 0666-umask.
916+
f, err := createOwnerOnly(*out)
890917
if err != nil {
891918
fmt.Fprintf(os.Stderr, "vantage add: %v\n", err)
892919
return 1
@@ -1257,16 +1284,17 @@ func (rt *runtime) commitRemote(ctx context.Context, ing store.ResultIngester, o
12571284
// state + sample windows over from the running engine (so a reload/apply doesn't
12581285
// re-fire alerts already firing or drop hysteresis history). Serialized against a
12591286
// round's alert eval via evalMu. Shared by the SIGHUP reload and the config-apply API.
1260-
func swapRuntime(current *atomic.Pointer[runtime], evalMu *sync.Mutex, nrt *runtime, seed func(*runtime)) {
1287+
func swapRuntime(current *atomic.Pointer[runtime], evalMu *sync.Mutex, nrt *runtime, seed func(nrt *runtime, monitors []model.Monitor)) {
12611288
// Seed the new engine's windows from durable history BEFORE the swap and OUTSIDE evalMu.
1262-
// A target whose measurement identity changed, or one that just gained its first alert,
1263-
// won't inherit a window below — without seeding it would start dark and fire late
1264-
// (CODE_REVIEW #4). recentContiguous filters the seed to the current host/probe, so a
1265-
// redefined target pulls nothing stale. nrt.engine isn't live yet, so this needs no lock,
1266-
// keeping the store read off the evalMu critical path. Inherit then overwrites the seed for
1267-
// same-identity targets with their live window.
1289+
// A target that just gained its first alert has no window to inherit below — without
1290+
// seeding it would start dark and fire late (CODE_REVIEW #4). A target whose measurement
1291+
// identity changed is not seeded at all (reloadSeedTargets): its stored rounds measured the
1292+
// old definition. nrt.engine isn't live yet, so this needs no lock, keeping the store read
1293+
// off the evalMu critical path. Inherit then overwrites the seed for same-identity targets
1294+
// with their live window. Reading current here, outside evalMu, is safe because every caller
1295+
// holds applyMu, so no other swap can land before the locked Load below.
12681296
if seed != nil && nrt.engine != nil {
1269-
seed(nrt)
1297+
seed(nrt, reloadSeedTargets(current.Load(), nrt))
12701298
}
12711299
evalMu.Lock()
12721300
old := current.Load()
@@ -1277,6 +1305,26 @@ func swapRuntime(current *atomic.Pointer[runtime], evalMu *sync.Mutex, nrt *runt
12771305
evalMu.Unlock()
12781306
}
12791307

1308+
// reloadSeedTargets returns the monitors a reload may warm-start from durable history: all of
1309+
// nrt's except a target whose measurement identity (targetFP) changed since old. The store keeps
1310+
// no fingerprint and warmStartAlerts matches history only on host/probe/metric/cadence, so the
1311+
// recent rounds it would find for a redefined target were measured under the old definition:
1312+
// moving a TCPConnect target from port 443 to 22 would count port-443 failures toward the port-22
1313+
// consecutive-loss matcher and fire it early. A redefined target starts with an empty window and
1314+
// waits for X rounds of its new measurement. A same-identity target stays seedable (it inherits
1315+
// its live window over the seed, or needs the seed if it just gained an alert), and so does a
1316+
// target absent from old, which has no previous identity to compare (as at boot).
1317+
func reloadSeedTargets(old, nrt *runtime) []model.Monitor {
1318+
out := make([]model.Monitor, 0, len(nrt.monitors))
1319+
for _, m := range nrt.monitors {
1320+
if was, ok := old.targetFP[m.ID]; ok && was != nrt.targetFP[m.ID] {
1321+
continue
1322+
}
1323+
out = append(out, m)
1324+
}
1325+
return out
1326+
}
1327+
12801328
// reloadIdentity builds the per-target/per-alert identity InheritStateFrom uses to decide what
12811329
// survives a reload: which targets exist (ValidTarget) with an unchanged measurement identity
12821330
// (SameTarget), which alerts are attached to each target now (Attached), and whether each
@@ -1325,8 +1373,9 @@ func sameStringSet(x, y []string) bool {
13251373
// the old and new runtime, using the same federation.Fingerprint (probe/host/params/pings/
13261374
// probe-config) the ingest and in-flight-drop paths key on — so the reload inherit-gate can't
13271375
// disagree with them. A target that is new, removed, or redefined in ANY of those fields is
1328-
// absent (false), so InheritStateFrom carries neither its window nor its firing/visible state;
1329-
// it is seeded fresh from durable history instead (CODE_REVIEW #4).
1376+
// absent (false), so InheritStateFrom carries neither its window nor its firing/visible state
1377+
// (CODE_REVIEW #4). A new target is seeded from durable history; a redefined one is not
1378+
// (reloadSeedTargets), since that history measured its old definition.
13301379
func sameTargetIdentity(old, nrt *runtime) map[string]bool {
13311380
same := make(map[string]bool, len(nrt.targetFP))
13321381
for name, fp := range nrt.targetFP {
@@ -1342,7 +1391,7 @@ func sameTargetIdentity(old, nrt *runtime) map[string]bool {
13421391
// runtime in. Validate → persist → swap: an invalid doc never persists, a stale
13431392
// version never swaps. Returns api.ErrConfigInvalid / api.ErrConfigConflict.
13441393
func applyConfig(cfgStore *configstore.Store, current *atomic.Pointer[runtime], evalMu *sync.Mutex,
1345-
build func(dbFragment func() ([]byte, error)) (*runtime, error), doc json.RawMessage, expectedVersion int, seed func(*runtime)) error {
1394+
build func(dbFragment func() ([]byte, error)) (*runtime, error), doc json.RawMessage, expectedVersion int, seed func(*runtime, []model.Monitor)) error {
13461395
nrt, berr := build(func() ([]byte, error) { return doc, nil })
13471396
if berr != nil {
13481397
return fmt.Errorf("%w: %v", api.ErrConfigInvalid, berr)
@@ -1378,6 +1427,14 @@ func alertRTT(o scheduler.Outcome) float64 {
13781427
return o.Computed.Median
13791428
}
13801429

1430+
// historySeed returns the reload seed swapRuntime runs: it warm-starts the given monitors' alert
1431+
// windows in nrt's not-yet-live engine from st's durable history, as of when it runs.
1432+
func historySeed(ctx context.Context, st store.Store) func(nrt *runtime, monitors []model.Monitor) {
1433+
return func(nrt *runtime, monitors []model.Monitor) {
1434+
warmStartAlerts(ctx, nrt.engine, monitors, nrt.metricByName, st, time.Now())
1435+
}
1436+
}
1437+
13811438
// warmStartLookback bounds how far back a remote vantage's warm-start history read goes.
13821439
// SeedWindow trims to the deepest matcher window anyway; this just caps the query.
13831440
const warmStartLookback = 24 * time.Hour

‎cmd/smoked/main_test.go‎

Lines changed: 92 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"strings"
1212
"sync"
1313
"sync/atomic"
14+
"syscall"
1415
"testing"
1516
"time"
1617

@@ -566,8 +567,7 @@ func TestSwapRuntimeSeedsNewlyAlertedTarget(t *testing.T) {
566567
engine: alert.NewEngine(alertDef, map[string]alert.Notifier{"cap": cap}),
567568
monitors: []model.Monitor{monAlerted}, alertsByTarget: map[string][]string{"t": {"loss"}}, targetFP: map[string]string{"t": "fp"},
568569
}
569-
seed := func(r *runtime) { warmStartAlerts(context.Background(), r.engine, r.monitors, r.metricByName, st, now) }
570-
swapRuntime(&current, &mu, nrt, seed)
570+
swapRuntime(&current, &mu, nrt, historySeed(context.Background(), st))
571571

572572
// Window seeded with the 1 prior breaching round; one more now meets X=2 → fires immediately.
573573
current.Load().eval([]scheduler.Outcome{{
@@ -579,6 +579,56 @@ func TestSwapRuntimeSeedsNewlyAlertedTarget(t *testing.T) {
579579
}
580580
}
581581

582+
// A reload that redefines what a target measures must not warm-start its alert window from rounds
583+
// of the old measurement. Moving a TCPConnect target from port 443 to 22 on the same host, with two
584+
// recent failing port-443 rounds in the store, a three-round loss alert must still need three
585+
// failing port-22 rounds, not fire on the first.
586+
func TestReloadDoesNotSeedRedefinedTargetFromOldMeasurement(t *testing.T) {
587+
st := store.NewMem(16)
588+
cap := &capNotify{}
589+
alertDef := map[string]*alert.Alert{"loss": {Name: "loss", Matcher: alert.CheckLoss{L: 50, X: 3}, To: []string{"cap"}}}
590+
newRT := func(m model.Monitor) *runtime {
591+
return &runtime{
592+
engine: alert.NewEngine(alertDef, map[string]alert.Notifier{"cap": cap}),
593+
monitors: []model.Monitor{m},
594+
alertsByTarget: map[string][]string{"t": {"loss"}},
595+
targetFP: map[string]string{"t": federation.Fingerprint(m, nil)},
596+
}
597+
}
598+
failedRound := func(m model.Monitor, when time.Time) scheduler.Outcome {
599+
return scheduler.Outcome{
600+
Target: probe.Target{ID: m.ID, Name: m.Name, Host: m.Host, Params: m.Params},
601+
ProbeName: m.ProbeKind, Fingerprint: federation.Fingerprint(m, nil),
602+
Computed: sample.Compute(1, nil), When: when,
603+
}
604+
}
605+
606+
port443 := model.Monitor{ID: "t", Name: "t", Host: "h", ProbeKind: "TCPConnect", Params: map[string]string{"port": "443"},
607+
Pings: 1, Step: time.Minute, Alerts: []string{"loss"}, Vantages: []string{store.DefaultVantage}}
608+
port22 := port443
609+
port22.Params = map[string]string{"port": "22"}
610+
611+
var current atomic.Pointer[runtime]
612+
current.Store(newRT(port443))
613+
var mu sync.Mutex
614+
now := time.Now()
615+
for _, ago := range []time.Duration{90 * time.Second, 30 * time.Second} {
616+
current.Load().storeLocal(st, []scheduler.Outcome{failedRound(port443, now.Add(-ago))})
617+
}
618+
if cap.n != 0 {
619+
t.Fatalf("two failures must not fire a three-round alert, got %d events", cap.n)
620+
}
621+
622+
swapRuntime(&current, &mu, newRT(port22), historySeed(context.Background(), st))
623+
624+
for i := 1; i <= 3; i++ {
625+
current.Load().storeLocal(st, []scheduler.Outcome{failedRound(port22, now.Add(time.Duration(i)*time.Minute))})
626+
if want := map[bool]int{true: 1, false: 0}[i == 3]; cap.n != want {
627+
t.Fatalf("after %d failing port-22 round(s): %d notification(s), want %d", i, cap.n, want)
628+
}
629+
}
630+
}
631+
582632
// sameTargetIdentity underpins Bug A: only a target whose measurement fingerprint is unchanged
583633
// keeps its inherited window/firing state across a reload. It compares the runtimes' targetFP,
584634
// so any identity-changing field (host, probe, params, pings, probe-config) flips it.
@@ -1079,3 +1129,43 @@ func TestVantageAddEmitsOnboardingBundle(t *testing.T) {
10791129
t.Fatalf("bundle file is not gzip (first bytes %x)", data[:min(2, len(data))])
10801130
}
10811131
}
1132+
1133+
// The `vantage add -out` bundle embeds the agent's client private key, so it must be readable by
1134+
// its owner only: under the common 022 umask (where a default-created file is 0644), and also when
1135+
// -out overwrites an existing file that was created world-readable.
1136+
func TestVantageAddBundleIsOwnerOnly(t *testing.T) {
1137+
dsn := os.Getenv("SMOKE_TEST_DSN")
1138+
if dsn == "" {
1139+
t.Skip("set SMOKE_TEST_DSN to run vantage add test")
1140+
}
1141+
defer syscall.Umask(syscall.Umask(0o022))
1142+
1143+
dir := t.TempDir()
1144+
existing := filepath.Join(dir, "existing.tar.gz")
1145+
if err := os.WriteFile(existing, []byte("old bundle"), 0o644); err != nil {
1146+
t.Fatal(err)
1147+
}
1148+
for _, c := range []struct{ vantage, path string }{
1149+
{"perm-new", filepath.Join(dir, "new.tar.gz")},
1150+
{"perm-existing", existing},
1151+
} {
1152+
t.Cleanup(func() { vantageCmd([]string{"revoke", c.vantage, "-dsn", dsn}) })
1153+
if rc := vantageCmd([]string{"add", c.vantage, "-dsn", dsn, "-hub", "https://hub.example:8443", "-out", c.path}); rc != 0 {
1154+
t.Fatalf("%s: vantage add -out rc=%d", c.vantage, rc)
1155+
}
1156+
fi, err := os.Stat(c.path)
1157+
if err != nil {
1158+
t.Fatal(err)
1159+
}
1160+
if perm := fi.Mode().Perm(); perm != 0o600 {
1161+
t.Errorf("%s: bundle holding the client private key has mode %04o, want 0600", c.vantage, perm)
1162+
}
1163+
data, err := os.ReadFile(c.path)
1164+
if err != nil {
1165+
t.Fatal(err)
1166+
}
1167+
if len(data) < 2 || data[0] != 0x1f || data[1] != 0x8b {
1168+
t.Errorf("%s: bundle file is not gzip (first bytes %x)", c.vantage, data[:min(2, len(data))])
1169+
}
1170+
}
1171+
}

‎docs/federation.md‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -130,7 +130,10 @@ $ smoked vantage add nyc -dsn "$SMOKED_DSN" -hub https://your-hub.example:8443 -
130130
wrote bundle nyc-vantage.tar.gz for vantage "nyc"
131131
```
132132

133-
That tar.gz is the same onboarding bundle the GUI downloads. Omit `-out` to
133+
That tar.gz is the same onboarding bundle the GUI downloads. It holds the
134+
vantage's private key, so `-out` writes it readable by its owner only (mode
135+
`0600`, whatever your umask), and tightens an existing file it overwrites to
136+
`0600` too. Omit `-out` to
134137
print the rendered `agent.yaml` to stdout instead (handy for `scp`-ing just the
135138
file), or pass `-json` to emit the raw PEMs as JSON. `-dsn` defaults to
136139
`$SMOKED_DSN`. Running `vantage add nyc` again for an **already-registered**

‎internal/alert/engine.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ type ReloadIdentity struct {
119119
// here has its window and all alert state dropped.
120120
ValidTarget map[string]bool
121121
// SameTarget[t]: t's measurement identity (host/probe/params/...) is unchanged. If it
122-
// changed, t's window and alert state are not inherited (t is seeded from history instead).
122+
// changed, t's window and alert state are not inherited.
123123
SameTarget map[string]bool
124124
// Attached[t][name]: alert `name` is attached to target t in the NEW config. State for an
125125
// alert not currently attached to t is never inherited — so detaching an alert and later

0 commit comments

Comments
 (0)