Skip to content

harden(daemon): reject symlink at artifact-dir boot verify; retry healthz probe#35

Merged
thebenignhacker merged 1 commit into
mainfrom
fix/daemon-boot-symlink-reject-healthz
Jun 15, 2026
Merged

harden(daemon): reject symlink at artifact-dir boot verify; retry healthz probe#35
thebenignhacker merged 1 commit into
mainfrom
fix/daemon-boot-symlink-reject-healthz

Conversation

@thebenignhacker

Copy link
Copy Markdown
Contributor

What

Two hardening fixes from the nanomind-analyst 0.1.3 release-test + round-3 review (todo/2026-06-11-daemon-boot-verify-symlink-reject.md):

  • Symlink reject at boot verify. verify_artifact_integrity now os.lstat+S_ISLNK-rejects a symlink at classifier.joblib / meta.json before hashing, so the SHA is read from a real file rather than followed through a link. Mirrors the installer-side reject in artifacts._verify. Defense in depth, same-uid only, no privilege boundary crossed.
  • healthz retry. _healthz now retries a few quick probes with short backoff before declaring no-response, so a daemon busy on a long in-flight NLM generation isn't falsely reported dead (status vs status --json could disagree under load).

Honest scope (Phase 4.5 adversarial review)

The symlink reject does NOT close the full check-to-load TOCTOU: verify_artifact_integrity and joblib.load open the path independently (predictor.py reopens by name), so a same-uid process could still rename() a SHA-matching real file in between. This change is genuine defense-in-depth against a leftover/accidental (or planted) symlink and does not regress anything; the residual load-time window is pre-existing and is now documented in-code + tracked in todo/2026-06-15-daemon-artifact-load-toctou-fd.md (proper fix: open-once O_NOFOLLOW fd shared by hash and load). Comments, commit message, and test docstrings were corrected to not overstate the guarantee.

Vendored pair

The same verify_artifact_integrity lives in nanomind-training/serving/nanomind_guard_daemon.py — kept byte-identical (separate PR on that repo).

Testing

96 non-smoke tests pass; ruff clean; status/status --json smoke agree (ready, exit 0, valid JSON); HMA scan of the daemon dir 98/100 (only a subdir-.gitignore LOW FP). New test_daemon_integrity.py (symlink/mismatch/missing/match) + healthz retry tests.

…lthz probe

Two hardening fixes from the 2026-06-11 nanomind-analyst 0.1.3 release-test
and round-3 adversarial review:

- verify_artifact_integrity (boot, before joblib.load) now lstat-rejects a
  symlink at classifier.joblib / meta.json so the SHA is read from a real
  file, not followed through a link. Mirrors the installer-side reject in
  artifacts._verify. Defense in depth, same-uid only, no privilege boundary
  crossed. It does NOT close the full check-to-load TOCTOU (verify and
  joblib.load open the path independently); closing that needs an open-once
  O_NOFOLLOW fd shared by hash and load — tracked as a follow-up.
- _healthz now retries a few quick probes with short backoff before
  declaring no-response, so a daemon busy on a long in-flight NLM generation
  is not falsely reported dead ('status' vs 'status --json' could disagree
  under load).

Adds test_daemon_integrity.py (symlink/mismatch/missing/match, check-time
behaviour only) and healthz retry tests.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

Security Review: PR #35 - Daemon Boot Verification & Healthz Retry

Verification Checklist Completion

1. Symlink Rejection (nanomind_guard_daemon.py:186-206)

Pattern identified: os.lstat() + stat.S_ISLNK() check before hashing
Mitigation search:

  • ✅ Check occurs BEFORE sha256_file(path) is called (line 207)
  • ✅ Raises IntegrityError on symlink detection (line 202)
  • ✅ Test coverage confirms rejection behavior (test_daemon_integrity.py:64-97)
  • ✅ PR description explicitly acknowledges TOCTOU limitation (honest scoping)

Verdict: Implementation is sound for stated scope.

2. Healthz Retry Logic (lifecycle.py:53-74)

Pattern identified: Retry loop with backoff
Mitigation search:

  • ✅ Loop bounds checked: for attempt in range(attempts) with default attempts=3 (line 68)
  • ✅ Early return on success prevents unnecessary retries (line 71)
  • ✅ Backoff only applied between attempts: if attempt + 1 < attempts (line 72)
  • ✅ Test coverage validates retry behavior (test_lifecycle.py:587-641)

Verdict: No unbounded loops or resource exhaustion risks.

3. TOCTOU Window Documentation

Assessment: The PR description and inline comments (lines 192-199) correctly document that this does NOT close the check-to-load TOCTOU window. The symlink reject is defense-in-depth only, same-uid threat model, no privilege boundary crossed. This is honest security engineering.

4. Test Coverage Review

New tests added:

  • test_daemon_integrity.py: 4 tests covering pass/reject paths for symlink detection
  • test_lifecycle.py::TestHealthzRetry: 4 tests covering retry logic

All tests use proper isolation (tmp_path, monkeypatch) and assert expected behavior.

Findings

None. After completing the verification checklist:

  1. Symlink rejection has adequate mitigation: the lstat() check occurs before the hash is computed, raises on detection, and is covered by tests that verify both symlink rejection AND that rejection occurs even when the symlink target has a matching SHA.

  2. Healthz retry has no resource exhaustion risk: bounded retry count (default 3), early return on success, backoff only between attempts.

  3. TOCTOU limitation is properly documented in code and PR description—no false claims about closing the full window.

  4. Test coverage is comprehensive and uses proper test isolation.


VERDICT: APPROVE

SUMMARY: This PR introduces two defensive hardening measures for the nanomind-analyst daemon. The symlink rejection at boot-time artifact verification (verify_artifact_integrity) uses os.lstat() to detect and reject symlinks before hashing, mirroring the installer-side check. The healthz retry logic adds resilience against false no-response reports when the daemon is momentarily busy serving long-running NLM requests. Both changes have adequate mitigations: the symlink check occurs before the hash is read (preventing follow-through), and the retry loop is bounded with early-exit on success. The PR description honestly scopes the TOCTOU limitation (check-to-load window remains, tracked separately). Test coverage is comprehensive with proper isolation. No security or correctness issues introduced.


Reviewed 4 files changed (11040 bytes)

@thebenignhacker thebenignhacker merged commit e95ae2b into main Jun 15, 2026
8 checks passed
@thebenignhacker thebenignhacker deleted the fix/daemon-boot-symlink-reject-healthz branch June 15, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant