feat(agent): add package policy management

[Benoît Cortier (CBenoit)]

Adds JSON-only package policy management using the official policy crates, including management snapshots, authoritative validation, and Create, Update, ReplaceIdentity, and Repair operations.

Uses exact store tokens to serialize API concurrency, keeps monitoring failures sticky, and fails closed for reparse-backed policy paths. The core change adds approximately 3,300 lines across 13 paths.

Privileged external filesystem compare-and-swap and crash recovery are deferred. Authorization, storage hardening, installer integration, audit events, and broader end-to-end coverage will follow in separate layers.

Issue: #1937
Source: #1963

[Benoît Cortier (CBenoit)]

Implementation notes:

• Policy management uses the official now-policy, now-policy-api, and now-policy-server-template crates with JSON-only authoritative validation and HMAC-bound validation receipts.
• Writers are serialized and require the exact observed store token; monitoring failure is sticky and advertises ReadOnly with ManagementDisabled.
• Policy activation and write capability validate every ordinary ancestor and reject any reparse ancestor or leaf before following it. The opened file's final path must also match the validated path.
• Publication uses same-directory temporary files, durable writes, security verification, atomic replacement, and authoritative reload.
• Focused security/store/watcher tests, dev broker tests, i686/x64/ARM64 checks, formatting, warnings-denied workspace Clippy, and the locked workspace suite passed with only the documented local WindowsApps ACL baseline excluded.
• The existing LocalSystem Agent policy harness now covers ordinary protected paths, nested-junction redirection, leaf symlink redirection, unavailable policy, reload snapshots, and rejected writes. Local execution was blocked because PsExec could not install PSEXESVC, so the existing CI LocalSystem job is the authoritative run.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🟡 Changes recommended

The Windows policy end-to-end check fails because the new test directory is classified as an invalid policy path.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds the package-policy management layer extracted from #1963.

Changes:

• Adds authenticated management, validation, and replacement endpoints.
• Introduces serialized storage, HMAC receipts, monitoring, and path security checks.
• Expands Windows end-to-end coverage.

File summaries

File	Description
Cargo.lock	Locks the HMAC dependency.
crates/agent-policy-tester/src/windows.rs	Expands policy-management integration tests.
crates/now-package-broker/Cargo.toml	Adds HMAC and Win32 globalization support.
crates/now-package-broker/src/auth.rs	Captures client elevation and administrator membership.
crates/now-package-broker/src/lib.rs	Exposes the policy-store module.
crates/now-package-broker/src/policy_security.rs	Adds policy-path and ACL validation.
crates/now-package-broker/src/policy_store/mod.rs	Implements policy state, concurrency, and persistence.
crates/now-package-broker/src/policy_store/receipt.rs	Implements validation receipts and store tests.
crates/now-package-broker/src/policy_store/validation.rs	Implements authoritative draft validation.
crates/now-package-broker/src/policy_watcher.rs	Integrates fail-closed store reloading.
crates/now-package-broker/src/server/mod.rs	Exposes management API routes and authorization.
crates/now-package-broker/src/task.rs	Initializes and monitors the policy store.
crates/win-api-wrappers/src/token.rs	Adds token group-membership checks.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Policy writes currently authorize the process primary token instead of the connected named-pipe client token.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Validation truncation incorrectly rejects warnings-only policies, and several management diagnostics diverge from the shared API contract.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

crates/now-package-broker/src/policy_store/validation.rs:721

• These generated finding paths can name fields that do not exist in the submitted draft. For example, an explicit Constraints object produces /Constraints/SkipHashCheck and /Constraints/PreRelease, although the actual fields are AllowSkipHashCheck and AllowPreRelease; omitted defaulted fields are also addressed as if present. The API defines Path as a JSON Pointer into the submitted draft, and the shared fixture points an explicit skip-hash warning at /Rules/0/Match/SkipHashCheck. Build the path from the actual match/constraint member that triggered the warning, or use the containing rule when no concrete member exists.

            let path = if rule.constraints.is_some() {
                format!("/Rules/{index}/Constraints/{option}")
            } else {
                format!("/Rules/{index}")
            };

• Files reviewed: 12/13 changed files
• Comments generated: 3
• Review effort level: Balanced

[Benoît Cortier (CBenoit)]

Review follow-up for the suppressed sensitive-warning pointer observation:

• Warning paths now come from raw input presence: an explicit Match member, the exact explicit Allow* constraint member, or the containing rule when behavior comes only from defaults.
• Parameterized coverage checks all seven sensitive options across explicit match, explicit constraint, and omitted/default forms and asserts that every emitted pointer exists in the submitted JSON.
• Receipt authorization intentionally excludes UX-only diagnostic path/message fields while retaining the full canonical draft, validator version 5, and semantic finding identity (severity, code, rule ID, arguments).
• End-to-end store coverage serializes the returned canonical draft and successfully submits it with the original receipt for every option/form; a meaningful option change still invalidates the receipt.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🔵 Needs a closer look

Equal validity timestamps are incorrectly accepted despite the shared contract requiring ValidUntil to be later than ValidFrom.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/now-package-broker/src/policy_store/validation.rs:397

• Equal ValidFrom/ValidUntil timestamps currently pass validation, even though the shared contract defines this finding as requiring ValidUntil to be after ValidFrom. A zero-length validity interval can therefore receive a receipt and be committed. Reject equality as well and add it to the interval regression test.

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Benoît Cortier (CBenoit)]

Disposition for the suppressed validity-interval observation in review 5136001769:

Fixed in 8580f081. Validation now requires ValidFrom < ValidUntil; exact equality, timezone-equivalent equality, and reversed intervals produce InvalidValidityInterval at /Metadata/ValidUntil with the contract wording ValidUntil must be after ValidFrom. Either bound may still be omitted, and a strictly increasing interval remains valid. The validator version advances to 6 so receipts issued under the prior accepted semantics are invalidated.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🔵 Needs a closer look

The exported receipt-less validator can produce a validation result that the shared API contract refuses to serialize.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/now-package-broker/src/policy_store/validation.rs:71

• This public function returns a PolicyValidationResult with IsValid = true but no ValidationReceipt. The shared API type rejects serialization of that combination, so callers of the exported policy_store::validation::validate_draft API receive a value they cannot put on the wire. Keep this receipt-less validator internal and expose only PolicyStore::validate_draft, which adds the receipt.

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Benoît Cortier (CBenoit)]

Disposition for the suppressed receipt-less validator observation in review 5136149030:

Fixed in 1ca50831. The policy_store::validation module is now private, and its raw draft validator, committed-policy validator, and validator-version constant are visible only to the containing store. Public callers can use only PolicyStore::validate_draft, which adds the keyed receipt before returning any valid result. The complete-result serializer and canonical-original-receipt regressions remain green.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🔵 Needs a closer look

Draft validation currently accepts duplicate values that violate the official policy schema, and management requests perform redundant signature verification.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/now-package-broker/src/policy_store/validation.rs:282

• The raw bounds pass does not enforce the draft schema's uniqueItems: true requirement for the set-backed Match arrays (Operations, Managers, Sources, PackageIdentifiers, PackageNames, Versions, Scopes, Architectures, and Elevation). Serde then collects these arrays into BTreeSets, silently removing duplicates, so an invalid draft is reported valid and committed in a different canonical form. Detect duplicate raw values here before typed deserialization and return a schema/value finding for the affected JSON pointer.
  crates/now-package-broker/src/server/mod.rs:124
• Every protected route performs Authenticode validation twice: this middleware calls validate_connection, then each of policy_management, validate_policy, and replace_policy calls it again. Since validation invokes the full signature check on disk, each request pays that cost twice. Keep the pre-body middleware check and remove the redundant handler checks (or pass an authenticated marker to the handlers).

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Benoît Cortier (CBenoit)]

Disposition for the two suppressed observations in review 5136250964:

• Fixed set-backed draft uniqueness in 02acb828. The bounded raw pass now rejects exact duplicates in all nine Match arrays with schema uniqueItems: true, before BTreeSet deserialization can normalize them. Checks are case-sensitive, use borrowed string keys only after existing length bounds, leave type failures to strict parsing, and do not apply to constraint vectors.
• Removed duplicate protected-route signature work in 02acb828. GET/HEAD management, POST validation, and PUT replacement authenticate once in pre-body middleware, which scopes a private task-local marker around the handler future. Each handler requires that marker, direct handler invocation fails Unauthorized, and PUT retains its elevated-Administrator authorization check.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🔵 Needs a closer look

Inconsistent path normalization can rotate a newly returned store token after the write-triggered watcher reload.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

crates/now-package-broker/src/policy_store/mod.rs:784

• The authoritative post-write reload fingerprints path, which is observation.configured_path (the canonical display path), while normal loads/reloads fingerprint self.configured_path (the original configured path). On Windows, canonicalize() commonly adds the \\?\ prefix, so these fingerprints differ even for the same file. The watcher event caused by this write will then rotate the just-returned store token, making a follow-up request unexpectedly stale. Normalize the path before every fingerprint calculation, or perform the post-write observation using the store's original configured path.

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Benoît Cortier (CBenoit)]

Disposition for the suppressed post-write fingerprint observation in review 5136419956:

Fixed in fbbddb3c. Publication still targets the validated canonical observation path, but every authoritative observation—including the post-write reload—now receives the store's original configured path. Startup, readiness, API writes, and watcher reloads therefore hash one stable configured identity while retaining the full lexical/canonical/reparse validation chain.

The LocalSystem harness now exercises real Create → watcher reload → Update flows for both ordinary and \\?\ verbatim configured paths, asserting that each returned token survives its own watcher event. The existing external file replacement scenario additionally asserts that a genuine out-of-band content change still rotates the token.

Note

Human-tuned, LLM-assisted content.

[Copilot]

🔵 Needs a closer look

It introduces security-sensitive Windows authorization and persistence while explicitly deferring crash recovery and external-write CAS protections.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The validator accepts an empty prerelease-only range, and the security-sensitive Windows persistence surface warrants final human review.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 1
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

The change combines authorization, Windows filesystem security, persistence, concurrency, and public API behavior and needs final human validation.

Review details

• Files reviewed: 12/13 changed files
• Comments generated: 0 new
• Review effort level: Balanced