feat(hermes-attestation-guardian): harden attestation verification and drift controls#192
Conversation
![baz-reviewer[bot]](https://avatars.githubusercontent.com/in/933528?s=60&v=4){kind=small}
| function bool(value, defaultValue = false) { | ||
| if (value === undefined || value === null) { | ||
| return defaultValue; | ||
| } | ||
| return !!value; |
There was a problem hiding this comment.
Since bool() coerces non-null config.json strings to true in buildAttestation() for gateways.*.enabled and security.*, should we normalize/validate config booleans like readEnvBool() before attesting so values like "false" don’t get treated as enabled?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 171-176 (`bool`) and
lines 209-229 (`buildAttestation()` where `gateways.*.enabled` and `security.*` are
computed), fix the logical bug where `bool()` treats any non-null string as truthy (so
config values like "false" become enabled). Refactor by either (a) updating `bool()` to
normalize boolean-like strings using the same rules as `readEnvBool()` (trim +
lowercase; accept 1/true/yes/on/enabled and 0/false/no/off/disabled) before coercing, or
(b) validating that the config fields are actual booleans and otherwise ignoring them in
favor of the env fallback/default. Ensure malformed/incorrect config types in
detectHermesConfig() do not flip posture; if the config type is invalid, default to the
provided env fallback.
| if (!isPlainObject(attestation.generator)) { | ||
| errors.push("generator object is required"); | ||
| } |
There was a problem hiding this comment.
generator is only validated as an object, but diffAttestations() reads generator.version and defaults to "unknown", so malformed attestations can slip through and skew diff output—should we require generator.version as a string in the schema?
Finding type: Type Inconsistency | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 342-344 inside
`validateAttestationSchema()`, the code only checks that `attestation.generator` is a
plain object and does not validate `attestation.generator.version`. Refactor the
validation to additionally require `generator.version` to be a string (and ideally
non-empty/trimmed) and push a clear schema error if it’s missing or not a string. This
will prevent malformed attestations from passing schema checks and producing misleading
diff results that hide version changes behind the `
| if (!isPlainObject(attestation.posture.runtime)) { | ||
| errors.push("posture.runtime object is required"); | ||
| } |
There was a problem hiding this comment.
posture.runtime only checks object-ness, so diffAttestations() can misread posture.runtime.gateways.* and posture.runtime.risky_toggles.* with !! on malformed shapes; can we validate both sub-objects and their leaf boolean values here?
Finding type: Type Inconsistency | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 352-354 inside
`validateAttestationSchema`, extend validation for `attestation.posture.runtime`:
currently it only checks that `posture.runtime` is a plain object, but it never
validates `posture.runtime.gateways` and `posture.runtime.risky_toggles`. Refactor this
block to also assert both `gateways` and `risky_toggles` are plain objects and that each
expected leaf key exists (telegram/matrix/discord and
allow_unsigned_mode/bypass_verification) and each corresponding value is a boolean; if
values are missing or not booleans, add descriptive errors. Keep the existing `errors`
array behavior and ensure any malformed shapes that could later affect
`diffAttestations()` are rejected at schema validation time.
| if (!Array.isArray(integrity.watched_files)) { | ||
| errors.push("posture.integrity.watched_files must be an array"); | ||
| } | ||
| if (!Array.isArray(integrity.trust_anchors)) { | ||
| errors.push("posture.integrity.trust_anchors must be an array"); |
There was a problem hiding this comment.
posture.integrity.watched_files and trust_anchors only get array checks in validateAttestationSchema(), so objects missing a string path can slip through and be dropped later by mapByPath() in diffAttestations()—should we validate each integrity item shape here (e.g. path string, sha256 string|null, and required fields like exists) before accepting the attestation?
Finding type: Logical Bugs | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 323-375 inside
validateAttestationSchema(), tighten validation of posture.integrity.watched_files and
posture.integrity.trust_anchors: after confirming they are arrays, also iterate each
element and ensure it has the expected object shape (path must be a non-empty string;
sha256 must be either null or a 64-char hex string; and any required fields your
verifier/diff logic expects, e.g. exists boolean). If any element fails, push a specific
error message and return the accumulated errors. This prevents malformed integrity
entries from passing schema validation and then being silently dropped later in
diffAttestations().
| const schemaErrors = validateAttestationSchema(attestation); | ||
| for (const message of schemaErrors) { | ||
| verificationFindings.push({ severity: "critical", code: "SCHEMA_INVALID", message }); | ||
| failures.push(message); | ||
| } |
There was a problem hiding this comment.
The schema/digest validation flow (including SCHEMA_INVALID/CANONICAL_DIGEST_MISMATCH and BASELINE_SCHEMA_INVALID/BASELINE_CANONICAL_DIGEST_MISMATCH) is duplicated later, so changes could drift—should we extract something like validateAttestationDocument({ bytes, findings, failureList, codePrefix }) and reuse it for both flows before computing the diff?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
abutbul
force-pushed
the
skill/hermes-attestation-guardian-hardening-v0.0.1
branch
from
59eef65 to
aa30e7f
Compare
abutbul
force-pushed
the
skill/hermes-attestation-guardian-hardening-v0.0.1
branch
from
aa30e7f to
1f1dde4
Compare
| const outPath = resolveHermesScopedOutputPath(args.output); | ||
| fs.mkdirSync(path.dirname(outPath), { recursive: true }); | ||
| const body = stableStringify(attestation, args.compact ? 0 : 2); | ||
| fs.writeFileSync(outPath, `${body}\n`, "utf8"); | ||
|
|
There was a problem hiding this comment.
generate_attestation should write current.json atomically instead of directly to outPath, so a crash during fs.writeFileSync doesn’t leave partial invalid JSON — can we write to a temp file in the same dir and rename it into place?
Finding type: Architecture soundness design | Severity: 🟠 Medium
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/scripts/generate_attestation.mjs around lines 113-117
inside the run() logic that computes outPath and calls fs.writeFileSync, change the
output write to be atomic. Refactor to write the JSON to a temporary file in the same
directory (e.g., prefix/suffix the filename), flush it to disk (fsync) after writing,
and then replace the target outPath via a rename/move that is atomic on the same
filesystem. Also ensure the .sha256 file is written only after the rename succeeds so
watchers never observe a digest paired with a corrupted/partial current.json.
| if (typeof value === "string") { | ||
| const norm = value.trim().toLowerCase(); | ||
| if (["1", "true", "yes", "on", "enabled"].includes(norm)) return true; | ||
| if (["0", "false", "no", "off", "disabled"].includes(norm)) return false; | ||
| return defaultValue; | ||
| } | ||
| return defaultValue; |
There was a problem hiding this comment.
bool() silently falls back to readEnvBool(...) for malformed values like {} or unknown strings, and also duplicates readEnvBool's normalization logic — should we refactor bool() to explicitly accept only booleans, 0/1, and a fixed set of strings, returning false for anything else, and have readEnvBool reuse it as the single normalization source?
Finding types: Logical Bugs Code Dedup and Conventions | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
In skills/hermes-attestation-guardian/lib/attestation.mjs around lines 175-189, refactor
`bool()` to explicitly accept only boolean/number (0/1) and a fixed set of normalized
strings; for anything else, return false or throw an error — do not fall back to
reading `HERMES_*` env vars. Then have `readEnvBool` reuse `bool()` as the single
normalization source of truth. Adjust all call sites (especially the fail-closed
attestation path) so that a present-but-malformed config value is rejected/forced false
rather than triggering an env fallback.
| try { | ||
| if (fs.lstatSync(resolvedOutput).isSymbolicLink()) { | ||
| throw new Error(`output path must not be a symlink: ${resolvedOutput}`); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
Since ENOENT is swallowed on the preflight lstat, should we avoid the TOCTOU gap by opening/creating resolvedOutput atomically with no-follow semantics (or rechecking right before fs.writeFileSync)?
Finding type: Basic Security Patterns | Severity: 🔴 High
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
skills/hermes-attestation-guardian/lib/attestation.mjs around lines 116-120, the catch
block that ignores error.code === "ENOENT" causes a TOCTOU issue: a missing path is
treated as safe, but later fs.writeFileSync will follow a symlink if another process
creates one between the check and the write. Refactor this logic so the file is
created/opened in a single atomic step with no-follow semantics (e.g., open with
O_NOFOLLOW and use the resulting file descriptor for writing), or alternatively re-check
for symlink immediately before the write and fail closed if it appears. Ensure the final
write path enforcement is applied on the handle you write to, not only on a prior
lstatSync preflight.
| function isSymlinkPath(filePath) { | ||
| try { | ||
| return fs.lstatSync(filePath).isSymbolicLink(); | ||
| } catch (error) { | ||
| if (error?.code === "ENOENT") { | ||
| return false; | ||
| } | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| function writeAtomically(outPath, body) { |
There was a problem hiding this comment.
writeAtomically duplicates the symlink guard from resolveHermesScopedOutputPath (both lstatSync().isSymbolicLink() and throw the same error), should we consolidate to a shared helper or rely only on the resolveHermesScopedOutputPath export?
Finding type: Code Dedup and Conventions | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
| INSTALL_OUT=\$(hermes skills install \"well-known:http://127.0.0.1:$WELL_KNOWN_PORT/.well-known/skills/hermes-attestation-guardian\" --yes 2>&1) | ||
| echo \"\$INSTALL_OUT\" | ||
|
|
||
| echo \"\$INSTALL_OUT\" | grep -q \"Verdict: SAFE\" | ||
| echo \"\$INSTALL_OUT\" | grep -q \"Decision: ALLOWED\" |
There was a problem hiding this comment.
hermes skills install success currently depends on grepping logs for Verdict: SAFE and Decision: ALLOWED (not documented as part of the contract), so can we assert a stable machine-readable signal from INSTALL_OUT instead of parsing prose?
Finding type: Logical Bugs | Severity: 🟢 Low
Want Baz to fix this for you? Activate Fixer
Other fix methods
Prompt for AI Agents:
Before applying, verify this suggestion against the current code. In
scripts/hermes_attestation_sandbox_regression.sh around lines 73-77 (inside the docker
run bash -lc block), the sandbox determines install success by grepping INSTALL_OUT for
the literal strings “Verdict: SAFE” and “Decision: ALLOWED”. Refactor this to
use a stable, machine-readable success signal: first check the hermes CLI for an
existing option/contract (e.g., a JSON output or a documented flag) and assert on that;
if none exists, change the logic to assert on the hermes skills install command exit
code and then verify the skill was actually installed by checking for the expected
installed directory/files under the configured HERMES_HOME. Remove the brittle prose
greps so the test doesn’t fail when wording changes.
2 participants
User description
This PR introduces the new
hermes-attestation-guardianskill and hardens Hermes runtime attestation verification and drift detection controls.Initial release note
v0.0.1.skills/hermes-attestation-guardian/CHANGELOG.mdis populated with a0.0.1entry dated 2026-04-15.Security fixes and hardening summary
Validation run (all passing)
python utils/validate_skill.py skills/hermes-attestation-guardianValidation PASSED - all checks passednode skills/hermes-attestation-guardian/test/attestation_schema.test.mjsattestation_schema.test.mjs: oknode skills/hermes-attestation-guardian/test/attestation_diff.test.mjsattestation_diff.test.mjs: oknode skills/hermes-attestation-guardian/test/attestation_cli.test.mjsattestation_cli.test.mjs: oknode skills/hermes-attestation-guardian/test/setup_attestation_cron.test.mjssetup_attestation_cron.test.mjs: okOverlap check findings
Searched upstream issues and PRs for:
hermes-attestation-guardianbaseline authenticityattestation cron markeroutput scope guarddigest bindingResult: no matching open/closed upstream issues or PRs found for these keywords at search time.
Generated description
Below is a concise technical summary of the changes proposed in this PR:
Introduce the Hermes attestation guardian skill that generates canonicalized Hermes posture attestations, verifies them with fail-closed schema/digest/signature checks, and diffing-driven drift severity logic plus cron scheduling guardrails. Document the manifest, docs, and quickstart guidance for the Hermes-only flow so operators can run or schedule the skill confidently.
Modified files (9)
Latest Contributors(2)
lib/attestation.mjs/lib/diff.mjshelpers, CLI scripts, regression sandbox, and supporting tests.Modified files (10)
Latest Contributors(0)