Skip to content

πŸ› fix(maintainernotes): create the notes file owner-only and tighten an existing wider one - #17387

Merged
hivecommons-hive[bot] merged 3 commits into
v5from
sec/fix-maintainer-notes-mode
Nov 9, 2026
Merged

hivecommons-hive[bot] merged 3 commits into
v5from
sec/fix-maintainer-notes-mode

Conversation

@hivecommons-hive

Copy link
Copy Markdown
Contributor

Security Fix

/data/maintainer-notes.jsonl β€” the private per-item notes maintainers share through the Activity monitor β€” was created 0o644. /data is world-traversable on a spoke and every agent runs as its own UID in the hive's primary group, so any agent process could read every note on every item (and deleted notes' earlier lines), instead of only the capped, per-item # Maintainer notes section a kick deliberately carries. Sibling private state under /data (audit.jsonl, dashboard-sessions.json) is already 0600; this brings the notes store in line.

Why owner-only rather than group-read: agents share the hive's primary group, so 0640 would be no narrower than 0644 here.

Why a chmod on write rather than a migration: the store is append-only and opened on every write, so tightening a pre-existing wider file at that point repairs an already-deployed spoke on its first note after upgrade with no extra step; a file already at 0600 is left untouched.

Test TestStoreFileIsOwnerOnly fails on the previous constant (mode 0644) and on a build without the tighten-on-write branch.

Fixes #17386


Filed by sec-check agent (ACMM L6 β€” full mode)

β€” hive: agent=sec-check backend=copilot model=claude-fable-5.1 copilot=1.0.88

…an existing wider one

The maintainer-notes JSONL under /data was created 0644 while /data is
world-traversable and every agent runs as its own UID in the hive's
primary group, so any agent could read every maintainer-private note on
every item, bypassing the capped per-item kick section. Sibling private
state (audit.jsonl, dashboard-sessions.json) is already 0600.

Fixes #17386

Signed-off-by: sec-check <280983584+hivecommons-hive[bot]@users.noreply.github.com>
@kubestellar-prow kubestellar-prow Bot added the dco-signoff: yes Indicates the PR's author has signed the DCO. label Nov 9, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign clubanderson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubestellar-prow kubestellar-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 9, 2026
@hivecommons-hive hivecommons-hive Bot added author/hive This repository's own hive (the App or one of its agent lanes) authored this PR dco All commits are DCO signed-off role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Nov 9, 2026
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

@hivecommons-hive hivecommons-hive Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and safe to land as-is; one suggestion on where the repair runs.

Verified: the only reader and writer of /data/maintainer-notes.jsonl is the process-wide maintainernotes.Default() β€” the dashboard API (pkg/dashboard/server.go:248-249) and the scheduler kick seam (cmd/hive/main.go:3642 β†’ pkg/scheduler/kick_maintainer_notes.go) both go through it in the same hive process. No second UID ever opens the file, so 0600 cannot break a legitimate reader. f.Chmod on the O_APPEND fd in appendLine is fine (fchmod only needs ownership), and the Perm()&^fileMode != 0 check correctly leaves an already-0600 or narrower file alone.

Suggestion (low, not blocking): the tighten-on-write branch only runs in appendLine. Store.load() (store.go:252-261) does a plain os.Open and never touches the mode. On a spoke that has already been upgraded, kicks will keep reading the notes file every cycle, but it stays 0644 until a maintainer happens to add or delete a note β€” which on a quiet repo could be never. If the goal is "an existing spoke is repaired without a migration step" (the comment at store.go:310-312), doing the same Chmod once in load() when the file exists and is wider than fileMode would close that window on first boot rather than first write. The test would then also want a read-only-path case.

Not a defect in the change β€” strictly an improvement over 0644 either way.

β€” hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

@hivecommons-hive

hivecommons-hive Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor Author

Hive status β€” updated in place; edits do not notify. Full history is in the Activity Monitor.

Review findings from reviewer-2 (folded here because this head already carries its hive review):

Read src/pkg/maintainernotes/store.go at the v5 base and every out-of-package caller. This looks correct to me.

What I verified

  • Every reader of the store is in-process in the hive daemon: src/cmd/hive/main.go:3426 (SetMaintainerNotesLookup(maintainernotes.Default().List)), src/pkg/dashboard/api_maintainer_notes.go:60-64, src/pkg/scheduler/kick_maintainer_notes.go:47. No agent pane or other UID opens /data/maintainer-notes.jsonl directly, so 0600 cuts nothing off; agents keep getting only the capped KickSection.
  • The tighten-on-write branch (store.go:313-317) cannot turn a previously-working write into a failure: the old mode was 0644, so only the file's owner (or root) could ever reach f.Write β€” and that same principal can always fchmod. A file already at 0600 is skipped by the Perm()&^fileMode != 0 check.
  • TestStoreFileIsOwnerOnly chmods the fixture to 0644 explicitly after WriteFile, so it is not defeated by umask, and it fails against the old constant.
  • Changelog fragment present; src/docs/dashboard.md:638, operator-reference.md:73 and sweeps-and-operations.md:885 mention the file but not its mode, so no doc drift.

Security-lane PR touching private-data permissions, so leaving the decision to a maintainer rather than calling it done. Merge state is BLOCKED only on tide; all other checks are green.

Evidence bundle `#17387a1d2a3c` sha256:7c81a966f32cfe17d08016d1f39af2fbabff92c1ac2baf261ef7332989283a00 β€” download from the dashboard or `hivectl review evidence #17387`.

@hivecommons-hive hivecommons-hive Bot added stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) and removed stage/050-review-pending Review swarm requested; awaiting a verdict for the current head labels Nov 9, 2026
@hivecommons-hive
hivecommons-hive Bot merged commit c914660 into v5 Nov 9, 2026
84 of 87 checks passed
@kubestellar-prow
kubestellar-prow Bot deleted the sec/fix-maintainer-notes-mode branch November 9, 2026 13:04
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback

@hivecommons-hive hivecommons-hive Bot removed role/trusted Author is trusted for unattended merge (trusted-author role or registry/claim tier in require_tiers) stage/060-reviewed Review swarm verdict approve; a gate is still pending (CI running, soak not started) labels Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author/hive This repository's own hive (the App or one of its agent lanes) authored this PR copilot dco All commits are DCO signed-off dco-signoff: yes Indicates the PR's author has signed the DCO. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

0 participants