fix(agent-mesh): fail closed when a policy file cannot be loaded - #3660
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
github-actions
Bot
added
tests
agent-mesh
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
🔴 Contributor Check: HIGH
Automated check by AGT Contributor Check. |
github-actions
Bot
added
the
needs-review:HIGH
The sidecar and policy-server directory loaders wrapped each `load_yaml`/`load_json` in `except Exception: logger.warning("Skipped ..."); continue`. A policy file that failed to load (malformed YAML, bad apiVersion, invalid rule) was silently dropped and the server started and served decisions without it; if the dropped file carried a deny and a broader allow also loaded, the effective outcome flipped to allow (issue microsoft#3538).
Both loaders now fail closed by default: an unloadable file raises and the server refuses to start. Setting `AGT_POLICY_STRICT` / `AGENTMESH_POLICY_STRICT` to `0`, `false`, `no`, or `off` restores best-effort loading, which logs each unloadable file at error level and records a skipped count exposed via `/api/v1/policies`. Loading now builds into a local engine and commits only once every file has loaded, so a strict-mode failure on reload leaves the previously loaded policy set intact instead of swapping in a partially loaded, weaker one.
The policy-server trust-policy fallback passed file content to `TrustPolicy.from_yaml`, which opens a path, so a valid trust policy failed the fallback and, under the new strict default, would wrongly refuse to start; pass the file path instead. The `agentmesh.governance.policy.PolicyRegistry` loader is intentionally best-effort and covered by its own tests, so it is left unchanged.
Add tests: a bad file raises under the strict default in both loaders; non-strict skips and counts it; the sidecar API exposes the count; a failed strict reload preserves the previously loaded engine; and a trust policy loads through the fixed path-based fallback.
Signed-off-by: AlgoVoi <chopmob@gmail.com>
…d on blank strict The policy-server directory loader tracked a skipped count but never surfaced it: its /api/v1/policies handler returned only total_loaded, trust_policies and policy_dir, so an operator running in best-effort mode had no API view of how many files were dropped, even though the sidecar already exposed the count. The policy-server listing now returns "skipped" too, so the count the loader records is observable from both servers. Both loaders also treated a blank strict toggle as an opt-out: AGENTMESH_POLICY_STRICT= or AGT_POLICY_STRICT= (set but empty) silently disabled the fail-closed default. A security toggle should fail closed when it is misconfigured, so a blank or unrecognised value now stays strict; only 0, false, no or off select best-effort loading. Adds regression tests: an unloadable JSON file raises under the strict default in both loaders (the JSON branch has no trust fallback and was previously untested), a blank toggle stays strict, and the policy-server API surfaces the skipped count. Signed-off-by: AlgoVoi <chopmob@gmail.com>
…ed reload When the policy-server directory loader tries a YAML file as a governance policy and then falls back to a trust policy, a file that both parsers reject previously surfaced only the trust parser's error, which misattributes the failure whenever the file was meant to be a governance policy. The load failure now names both reasons, so an operator sees why each parser rejected it. The /api/v1/policy/reload endpoints raised the strict-mode RuntimeError uncaught, so a reload that hit an unloadable file returned a bare 500 that reads as a server bug. The loader already keeps the previously loaded policy set intact on a strict failure, so each reload endpoint now catches that failure, logs it, and returns 409 with the reason; the caller can tell the reload was rejected and the prior policies still serve. Strict mode raises without logging, so catching it here is also what records the reason on the reload path. Adds tests: a file rejected by both parsers names governance and trust in the raised error, and a strict reload against a bad file returns 409 while the previously loaded engine keeps serving, on both the sidecar and the policy-server. Signed-off-by: AlgoVoi <chopmob@gmail.com>
…test The new fail-closed policy-loader test shipped without the standard MIT header, and its added lines use the coined words unloadable and nonstrict, so the changed-files license gate and the cspell gate both rejected it. Add the canonical Microsoft MIT header and register both words in the repo cspell dictionary. Signed-off-by: AlgoVoi <chopmob@gmail.com>
49ca6d9 to
fc70dbe
Compare
Imran Siddique (imran-siddique)
left a comment
Imran Siddique (imran-siddique)
left a comment
There was a problem hiding this comment.
Same class as your #3659 and the same quality. Reviewed the parts that decide whether a fail-closed default is actually closed.
The failure is worse than "a file was skipped". except Exception: logger.warning("Skipped ..."); continue means a malformed policy is dropped and the server starts and serves decisions without it. If the dropped file carried a deny and a broader allow still loaded, the effective decision flips from deny to allow. The operator sees a warning in a startup log and a running server, which is indistinguishable from a healthy one. Fail-closed is right here.
The escape hatch is parsed in the safe direction, which is the thing to get wrong. os.getenv("AGT_POLICY_STRICT", "1").strip().lower() not in {"0", "false", "no", "off"} defaults to strict and treats every unrecognised value as strict, so a typo in a deployment manifest cannot silently reopen best-effort loading. test_blank_strict_env_stays_strict pins the empty-string case, which is the one a templated env var actually produces. An allowlist of enable-values would have inverted that and it would have looked equally reasonable in review.
test_strict_failure_preserves_previous_engine and the 409 on a rejected reload are the other half of getting this right: refusing a bad reload while continuing to serve the last good policy is better than either accepting it or failing the process.
One non-blocking point, for operators rather than for the code. This changes a default such that a deployment which previously started degraded now refuses to start. That is the correct behaviour and it is still a behaviour change someone will meet during an upgrade, at startup, in production. Breaking Changes passes and the docstrings explain the opt-out, but there is no CHANGELOG entry, so an operator upgrading has nothing to read beforehand. Worth a line under Changed naming the two env vars. I would not hold the PR for it if a maintainer would rather add it separately.
MohammadHaroonAbuomar this is part 1 of #3538, which you filed, and it has been green and unreviewed for two days.
No approve bit here, so this is a comment, but I would merge it.
… changelog Add an Unreleased Changed entry naming AGT_POLICY_STRICT and AGENTMESH_POLICY_STRICT so an operator upgrading can read the new fail-closed loader default and its opt-out before it changes startup behaviour, addressing the review note on PR microsoft#3660 (issue microsoft#3538). Signed-off-by: AlgoVoi <chopmob@gmail.com>
github-actions
Bot
added
the
documentation
2 participants
Addresses #3538 (part 1: the two directory loaders).
The sidecar and policy-server directory loaders wrapped each
load_yaml/load_jsoninexcept Exception: logger.warning("Skipped ..."); continue. A policy file that failed to load (malformed YAML, bad apiVersion, invalid rule) was silently dropped and the server started and served decisions without it; if the dropped file carried a deny and a broader allow also loaded, the effective outcome flipped to allow.Fix
AGT_POLICY_STRICT/AGENTMESH_POLICY_STRICTto0,false,no, oroffrestores best-effort loading, which logs each unloadable file at error level and records a skipped count exposed via/api/v1/policies.TrustPolicy.from_yaml, which opens a path, so a valid trust policy failed the fallback and (under the new strict default) would wrongly refuse to start; it now passes the file path.The
agentmesh.governance.policy.PolicyRegistryloader is intentionally best-effort and covered by its own tests, so it is left unchanged.Scope
Part 2 of the issue (
validate_policy_schemadoes not validatescope) is intentionally left out of this PR. Its wording assumes the model-level scope check from #3537, which did not merge, so on currentmainthere is nothing to delegate to and an unparseable scope is still demoted to global at evaluation rather than rejected at load. Whether the lint/save path should reject a scope that the engine still accepts is a maintainer call. Happy to follow up once that direction is settled.Testing
tests/test_policy_loader_failclosed.py: a bad file raises under the strict default in both loaders; non-strict skips and counts it; the sidecar API exposes the count; a failed strict reload preserves the previously loaded engine; and a trust policy loads through the fixed path-based fallback.ruff check src/ --select E,F,W --ignore E501is clean.