fix(agent-installer): secure policy migration

[Benoît Cortier (CBenoit)]

Creates and verifies the managed PackageBroker directory, then transactionally migrates eligible legacy JSON policies with identity- and digest-bound rollback and commit cleanup. Unsafe or ambiguous legacy policies remain untouched for manual remediation, and legacy YAML is never parsed or converted.

Registers the Agent Event Log source through native MSI registry lifecycle handling. Adds 35 focused Windows tests and a required CI job; the stacked change covers 8 paths with 1,738 insertions and 2 deletions.

Extracted from #1963 and stacked on #1978.

[Copilot]

🟡 Changes recommended

The review found concrete build/installer correctness issues (security descriptor comparison scope and a likely-broken MSBuild copy path) that should be addressed before approval.

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

Pull request overview

This PR hardens the Windows Agent installer by adding a secured %ProgramData%\Devolutions\PackageBroker directory and implementing transactional migration of eligible legacy JSON package-broker policies, with rollback/commit support and strict security verification. It also registers the Windows Event Log source via MSI-managed registry values, and adds a dedicated Windows CI job plus focused installer tests to validate the security and migration behavior.

Changes:

• Add strict SDDL definitions and custom actions to create/verify the dedicated PackageBroker policy directory and migrate legacy JSON policies transactionally (with rollback/commit cleanup).
• Register the Agent Event Log source through MSI registry lifecycle handling.
• Add a new .NET Framework test project with Windows-focused tests, and wire it into CI as a required job.

File summaries

File	Description
package/AgentWindowsManaged/Resources/Includes.cs	Adds protected SDDL strings for the PackageBroker directory and policy files.
package/AgentWindowsManaged/Program.cs	Adds MSI registry value creation for the Event Log source message file.
package/AgentWindowsManaged/Actions/WinAPI.cs	Extends Win32 interop and constants needed for pinned-handle security and deletion logic.
package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs	Implements secure directory creation/verification and transactional legacy policy migration with marker-bound rollback/commit.
package/AgentWindowsManaged/Actions/AgentActions.cs	Wires new deferred/rollback/commit custom actions into the MSI execute sequence.
package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs	Adds tests covering ACL contracts, strict JSON config parsing, pinning semantics, and action sequencing.
package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj	Introduces the new net48 xUnit test project and build wiring for installer references.
.github/workflows/ci.yml	Adds a Windows CI job to run the new agent installer tests and makes it required via the success job.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

NULL DACL handling and delete-access assumptions can undermine migration safety or fail otherwise valid installations.

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.

package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs:247

• Requesting DELETE while first pinning the source makes the commit fail before reaching the best-effort cleanup. For example, a SYSTEM-owned source that grants SYSTEM read access but not delete access passes TryVerifyLegacyPolicySourceSecurity and is migrated, but this open then fails with access denied and the outer handler returns Failure, aborting an otherwise successful install instead of preserving both copies. Inspect the source with read-only access first, then acquire and reverify an identity/digest-bound delete handle only inside the best-effort cleanup path (and preserve the source if that access is unavailable).

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

[Copilot]

🔵 Needs a closer look

A commit-phase custom action can currently fail the install late in the transaction, which risks leaving the machine in an installed-but-failed state for a cleanup-only error.

Review details

Suppressed comments (1)

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

package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs:297

• CommitLegacyPackageBrokerPolicyMigration returns ActionResult.Failure on any exception. Because this action is scheduled as an MSI commit custom action, a late failure here can mark the install as failed even after the policy file was successfully migrated, leaving the machine in an installed-but-failed state. Consider treating commit-time cleanup as best-effort (log the error, but return Success) since rollback already handles failed migrations and the worst-case outcome here is leaving the marker/source for manual remediation.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

It introduces security-sensitive installer migration logic with multiple failure/rollback paths that warrants final human validation beyond automated review.

Review details

Suppressed comments (1)

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

package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs:140

• File.Move(temporary, destination) is a TOCTOU point: destination is checked earlier, but can be created between the check and the move. In that case File.Move throws and the custom action returns Failure, aborting the install even though the safe behavior would be to abandon migration and leave the legacy policy untouched.

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

Temporary migration cleanup pins the temp file without read access but still computes a content digest, which can fail and leave secure temp files behind.

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

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The installer action code contains a compile-blocking uint-sized array allocation and also performs an elevated File.Exists probe that can follow reparse points, which should be corrected before merge.

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.

package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs:855

• The access-mask checks rely on raw 0x40000000 / 0x10000000 values (generic rights). As-written this is hard to audit and easy to regress; introduce named FileSystemRights constants with a short comment and use them in the bitmask.

• Files reviewed: 8/8 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The configured PackageBroker.PolicyPath parsing currently allows UNC / drive-relative rooted paths, which can trigger elevated network or unintended local path access during commit cleanup.

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

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The changes introduce security-critical installer filesystem/ACL and migration behavior that warrants final human review despite strong test coverage.

Review details

• Files reviewed: 8/8 changed files
• Comments generated: 0 new
• Review effort level: Lite