feat(agent,agent-installer): add transactional policy store - #1963
feat(agent,agent-installer): add transactional policy store#1963Benoît Cortier (CBenoit) wants to merge 45 commits into
Conversation
Expose the validated active package-broker policy through the shared authenticated GET /v1/policy route. Return a structured unavailable error without leaking policy source or file-security details. This requires now-policy-api and now-policy-server-template 0.4.0 from Devolutions/now-libraries#93 before the change can ship. Issue: Devolutions/now-libraries#93 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Return a generic policy-unavailable message so clients cannot infer whether the active policy is file-backed, missing, or corrupt. Issue: Devolutions/now-libraries#93 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adopt the final shared server trait and keep policy-domain conversions owned by the broker after the compatibility feature removal. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Record the registry graph after removing the obsolete policy compatibility features so locked CI can resolve the manifest consistently. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exercise the policy HTTP contract from the repository integration tests while keeping authentication and snapshot-locking invariants beside the broker implementation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Launch the Agent through its CLI and issue HTTP requests over a real Tokio named-pipe client. Build the test Agent with the development signature bypass while requiring the matching debug configuration opt-in. Remove the in-process broker test harness and its testsuite dependencies; retain authentication and snapshot-locking invariants as broker unit tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Move privileged policy endpoint coverage into a dedicated tester so the active-policy path cannot silently skip in normal test runs. Run it as LocalSystem in CI with a development-only signature bypass. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Implementation notes:
Note Human-tuned, LLM-assisted content. |
|
CI repair update (
Focused validation passed: 37 policy-store Windows tests, 3 token-wrapper tests, agent-policy-tester check, direct unelevated E2E with token assertion, rustfmt, diff check, and targeted Clippy with warnings denied. Workflow assertions for restricted launch and single message-compiler selection also passed; Note Human-tuned, LLM-assisted content. |
|
Follow-up CI repair ( Local script parsing, workflow assertions, diff/rustfmt, agent-policy-tester check, and targeted Clippy passed. Note Human-tuned, LLM-assisted content. |
|
Final CI status: run 33254105745 passed at Note Human-tuned, LLM-assisted content. |
|
Shared-contract refresh provenance for
All six local artifacts were rehashed and matched Local validation: rustfmt and diff checks passed; locked broker suites passed 409/413 tests respectively with only the documented host Winget AppExecAlias DACL baseline; locked targeted check and Clippy with warnings denied passed; Agent and tester builds passed; unelevated Agent policy E2E passed. Note Human-tuned, LLM-assisted content. |
|
Authoritative shared-contract refresh at
All six unpublished artifacts were rehashed and match Local validation: rustfmt and diff checks passed; locked broker suites passed 409/413 tests respectively with only the documented host Winget AppExecAlias DACL baseline; locked targeted check and Clippy with warnings denied passed; Agent and tester builds passed; unelevated Agent policy E2E passed. Exact-head CI: 33764438462. Note Human-tuned, LLM-assisted content. |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved upgrade, filesystem-concurrency, authorization, and installer-security issues undermine the transactional guarantees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds authenticated transactional JSON policy management to Devolutions Agent, including secure persistence, validation, auditing, and end-to-end coverage.
Changes:
- Replaces the legacy loader/watcher with a transactional policy store.
- Adds elevation, ACL, concurrency-token, and Windows Event Log protections.
- Expands Windows CI and privileged/unelevated testing.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds SDK discovery and policy test modes. |
Cargo.lock |
Locks new contract and security dependencies. |
Cargo.toml |
Pins Phase 2 policy crates. |
crates/agent-policy-tester/Cargo.toml |
Adds Windows policy-test dependencies. |
crates/agent-policy-tester/run-as-system.ps1 |
Selects elevated test mode. |
crates/agent-policy-tester/run-unelevated.ps1 |
Adds restricted-token test runner. |
crates/agent-policy-tester/src/windows.rs |
Tests management lifecycle and authorization. |
crates/now-package-broker/Cargo.toml |
Adds receipts and audit dependencies. |
crates/now-package-broker/src/audit.rs |
Implements structured policy auditing. |
crates/now-package-broker/src/auth.rs |
Captures elevation and administrator membership. |
crates/now-package-broker/src/evaluator/mod.rs |
Exposes wildcard validation internally. |
crates/now-package-broker/src/evaluator/wildcard.rs |
Shares wildcard compilation checks. |
crates/now-package-broker/src/lib.rs |
Registers policy-store modules. |
crates/now-package-broker/src/policy_loader.rs |
Removes the legacy loader. |
crates/now-package-broker/src/policy_security.rs |
Strengthens policy filesystem security. |
crates/now-package-broker/src/policy_store/mod.rs |
Implements transactional store coordination. |
crates/now-package-broker/src/policy_store/receipt.rs |
Adds keyed validation receipts. |
crates/now-package-broker/src/policy_store/validation.rs |
Implements deterministic policy validation. |
crates/now-package-broker/src/policy_store/windows.rs |
Implements secure Windows persistence. |
crates/now-package-broker/src/policy_watcher.rs |
Removes the legacy watcher. |
crates/now-package-broker/src/scenario_tests.rs |
Makes scenarios JSON-only. |
crates/now-package-broker/src/server/mod.rs |
Adds policy-management routes and authorization. |
crates/now-package-broker/src/server/responses.rs |
Maps management errors and snapshots. |
crates/now-package-broker/src/task.rs |
Integrates the transactional store. |
crates/now-package-broker/src/test_support.rs |
Adds shared SID test support. |
crates/sysevent-codes/src/lib.rs |
Defines policy audit events. |
crates/sysevent-codes/tests/message_catalog_parity.rs |
Checks event-catalog parity. |
crates/sysevent-winevent/src/lib.rs |
Corrects null-handle detection. |
crates/win-api-wrappers/src/token.rs |
Adds token group-membership checks. |
devolutions-agent/build.rs |
Embeds the Agent message catalog. |
devolutions-agent/devolutions-agent.mc |
Adds the Agent event catalog. |
devolutions-gateway/devolutions-gateway.mc |
Mirrors policy audit messages. |
package/AgentWindowsManaged/Actions/AgentActions.cs |
Schedules policy-directory actions. |
package/AgentWindowsManaged/Actions/CustomActions.cs |
Creates and secures the policy directory. |
package/AgentWindowsManaged/Program.cs |
Registers the Agent event source. |
package/AgentWindowsManaged/Resources/Includes.cs |
Defines the policy-directory SDDL. |
Review details
- Files reviewed: 34/36 changed files
- Comments generated: 10
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Migration cleanup uses an incorrect Win32 field ABI, and the dependency-publication gate remains unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 38/40 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Production resource embedding and legacy ACL migration are defective, and registry dependency publication remains incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
devolutions-agent/build.rs:128
- Master builds and packages the Agent with the custom
productionprofile (.github/workflows/ci.yml:135-144, 1011, 1036), but this gate embeds the message catalog only whenPROFILEis literallyrelease. The shipped production binary will therefore lack the message-table resource that the installer registers asEventMessageFile, so Event Viewer cannot format these audit events. Include the production profile in this gate (or detect release-like profiles without relying on the profile name).
- Files reviewed: 38/40 changed files
- Comments generated: 1
- Review effort level: Balanced
Keep Phase 1's function-level large-error expectations and remove the now-redundant module-level expectation after Phase 2 route integration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
953d964 to
04d6e47
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Default-directory creation can follow an attacker-controlled junction before ancestor verification.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Verify the complete existing ancestor chain before the runtime fallback creates the protected policy directory, preventing privileged side effects through a pre-created junction. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Create transactions lack retained-directory anchoring, and capability-only updates produce false policy-change audits.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
crates/now-package-broker/src/policy_security.rs:861
- This paragraph describes ancestor verification, but it is attached to
final_path_from_handle, which only resolves a path. Remove the stale paragraph so the function's documentation matches its behavior.
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Create default policy components under retained verified parents, permit compatible shared-vendor ACLs while keeping PackageBroker strict, and use tested handle-relative rename transitions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Create default policy components under retained verified parents, support compatible shared-vendor ACLs, and use tested handle-relative file transitions. Avoid policy-change audit events for capability-only snapshot updates. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Legacy migration can delete a policy file that remains explicitly configured as the Agent’s active path.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Client-token impersonation occurs before any pipe message is read, causing real connections to fail authentication.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 39/42 changed files
- Comments generated: 1
- Review effort level: Balanced
Capture authorization from the retained connector process token without premature pipe impersonation, and reject inherited connections after the original process exits. Preserve an explicitly configured legacy policy during installer migration using strict config parsing and pinned file identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Exposes the validated active package-broker policy through the authenticated `GET /v1/policy` route. Clients receive a stable snapshot, while an unavailable policy returns the shared structured 404 response without exposing policy source or file-security details. Uses the official `now-policy` 0.3.0, `now-policy-api` 0.4.0, and `now-policy-server-template` 0.4.0 releases. Phase 1 remains read-only: policy management, validation, and replacement routes are not exposed. Package policy files are now JSON-only. Deployments using `.yaml` or `.yml` policy files must convert them to `.json`; unsupported extensions and YAML content are rejected rather than silently parsed. Coordinated stack: Devolutions/now-libraries#93 defines the shared contract, this PR implements the Agent endpoint, and Devolutions/UniGetUI#5292 consumes it. Keep this PR open while Phase 2 #1963 is restacked and the full stack is finalized. BREAKING CHANGE: Package broker policy files must use JSON; YAML and YML are no longer discovered or parsed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Adds authenticated transactional package-policy management to Devolutions Agent, including deterministic validation, optimistic concurrency, crash-safe JSON persistence, secure path and ACL enforcement, and stable old-or-new runtime snapshots.
Policy writes require the actual named-pipe client token to be elevated with the Administrators group enabled. Read, management, and validation remain available to an authenticated standard user. Structured tracing, Windows Event Log auditing, fail-closed watcher reconciliation, and split unelevated/LocalSystem end-to-end coverage make policy changes observable and recoverable.
Caller authentication binds a retained executable candidate to the process main-image section with
NtQueryInformationProcess(ProcessImageFileMapping), rejects network/provider image paths, verifies Devolutions Authenticode through the retained handle, and permits only trusted writers to tamper with the executable or its pinned, reparse-safe ancestor path. The same-stream regression proves that class 44 and a current valid signature are insufficient alone: a user-writable rewritten stream is rejected by the retained file and ancestor security policy.This authenticates approved image and file provenance at connection time, not runtime memory integrity or historical bytes. Same-integrity injection or hollowing of an approved non-PPL process is outside signed-image authentication. Such control grants no additional read, management, or validation authority beyond running signed UniGetUI, and a standard or medium-integrity user still cannot replace policy because the pipe token must independently be elevated and Administrators-enabled. SYSTEM and elevated-Administrator injection are inside the write trust boundary; stronger runtime integrity requires an appropriate WDAC policy or compatible PPL protection levels.
Current ACL verification cannot reconstruct historical write access. Trust assumes the approved binary and each ancestor were secure when created and were never untrusted-writable. Operators must reinstall or remediate after any ACL or path compromise before trusting the deployment again; installer and package verification establish secure deployment, while runtime checks fail closed on present insecurity.
Non-Create replacement retains the exact token-observed file and directory path, writes a durable secure transaction marker and replacement, moves the observed target to a tombstone by handle, and publishes the replacement by a no-replace handle-relative rename. Concurrent external content is never overwritten. Startup recovery restores the exact verified tombstone when the final leaf is absent, preserves any raced-in or published final leaf, and fails closed on malformed, ambiguous, or insecure remnants.
The default policy tree is traversed under retained no-delete-share handles from ProgramData. Existing shared
Devolutionsdirectories may retain compatible create-only sibling grants, but path-tampering grants are rejected; the dedicatedPackageBrokerdirectory always requires strict SYSTEM/Administrators-only security. Missing components receive protected security at creation, and race winners are reopened without following reparses and verified before traversal continues.The store preserves structured API failures, binds
ConfirmOverwriteto the exact observed store token, restricts committed revisions and rule priorities to1..=2,147,483,647where applicable, and migrates eligible legacy policy files transactionally. Migration commit and rollback retain verified marker handles through deletion so path replacement or in-place marker rewrites cannot delete newer content.Uses official crates.io releases
now-policy0.3.0,now-policy-api0.4.0, andnow-policy-server-template0.4.0 with no now-libraries git source. Committed documents usehttps://devolutions.net/schemas/now-policy.schema.1.0.json; editable drafts usehttps://devolutions.net/schemas/now-policy-draft.schema.1.0.json.Local validation includes Rust formatting, locked default and development broker suites, warnings-denied workspace Clippy, the full locked workspace suite, the i686 Windows broker check, Agent and policy-tester builds, the unelevated Agent policy end-to-end suite, direct Win32/native handle-relative rename tests, transaction crash/race recovery tests, and hostile directory creation tests. Exact-head CI provides the LocalSystem Repair and full end-to-end legs.
Stacked on #1937.
Shared contract: Devolutions/now-libraries#99 with the corrective follow-up in Devolutions/now-libraries#101.
Registry publication: Devolutions/now-libraries#98.