feat(agent): implement package broker#1847
feat(agent): implement package broker#1847Vladyslav Nikonov (vnikonov-devolutions) merged 4 commits into
Conversation
Let maintainers know that an action is required on their side
|
2158cf9 to
83fa0b1
Compare
Vladyslav Nikonov (vnikonov-devolutions)
changed the title
There was a problem hiding this comment.
Pull request overview
Implements an early Windows package broker for Devolutions Agent.
Changes:
- Adds policy loading, evaluation, live reload, and sample scenarios.
- Adds authenticated named-pipe APIs, operation tracking, and status polling.
- Adds WinGet/PowerShell command builders and elevated execution.
Reviewed changes
Copilot reviewed 47 out of 48 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
.gitattributes |
Adds asset and YAML attributes. |
Cargo.lock |
Locks new broker dependencies. |
devolutions-agent/Cargo.toml |
Adds broker dependencies and Windows features. |
devolutions-agent/src/lib.rs |
Exposes Windows broker modules. |
devolutions-agent/src/service.rs |
Starts the configured broker task. |
devolutions-agent/src/config.rs |
Adds broker and debug configuration. |
devolutions-agent/src/code_signing.rs |
Centralizes signature validation. |
devolutions-agent/src/updater/package.rs |
Reuses signature helpers. |
devolutions-agent/src/broker/mod.rs |
Defines broker modules. |
devolutions-agent/src/broker/auth.rs |
Authenticates pipe clients. |
devolutions-agent/src/broker/task.rs |
Coordinates broker subsystems. |
devolutions-agent/src/broker/pipe.rs |
Implements named-pipe transport. |
devolutions-agent/src/broker/policy_loader.rs |
Loads JSON/YAML policies. |
devolutions-agent/src/broker/policy_watcher.rs |
Implements policy hot reload. |
devolutions-agent/src/broker/operation_tracker.rs |
Tracks asynchronous operations. |
devolutions-agent/src/broker/server/mod.rs |
Implements broker API behavior. |
devolutions-agent/src/broker/server/connection.rs |
Serves HTTP pipe connections. |
devolutions-agent/src/broker/server/execution.rs |
Runs background operations. |
devolutions-agent/src/broker/server/responses.rs |
Builds API responses. |
devolutions-agent/src/broker/executor/mod.rs |
Defines execution abstractions. |
devolutions-agent/src/broker/executor/output.rs |
Handles output and exit codes. |
devolutions-agent/src/broker/executor/windows/mod.rs |
Implements Windows execution plans. |
devolutions-agent/src/broker/executor/windows/process.rs |
Creates user-token processes. |
devolutions-agent/src/broker/executor/windows/token.rs |
Resolves sessions and tokens. |
devolutions-agent/src/broker/command_builder/mod.rs |
Dispatches command construction. |
devolutions-agent/src/broker/command_builder/winget.rs |
Builds WinGet commands. |
devolutions-agent/src/broker/command_builder/powershell.rs |
Builds PowerShell commands. |
devolutions-agent/src/broker/evaluator/mod.rs |
Implements policy precedence. |
devolutions-agent/src/broker/evaluator/matching.rs |
Matches request attributes. |
devolutions-agent/src/broker/evaluator/constraints.rs |
Enforces rule constraints. |
devolutions-agent/src/broker/evaluator/version.rs |
Matches version ranges. |
devolutions-agent/src/broker/evaluator/wildcard.rs |
Implements wildcard matching. |
devolutions-agent/src/broker/evaluator/tests.rs |
Tests policy evaluation. |
devolutions-agent/src/broker/scenario_tests.rs |
Runs sample scenarios. |
devolutions-agent/src/broker/assets/samples/corporate-allowlist.policy.json |
Adds allowlist policy sample. |
devolutions-agent/src/broker/assets/samples/deny-risky-options.policy.json |
Adds deny-list policy sample. |
devolutions-agent/src/broker/assets/samples/scenarios/baseline.scenarios.json |
Defines baseline scenarios. |
devolutions-agent/src/broker/assets/samples/requests/winget-vscode-install.request.json |
Adds allowed request sample. |
devolutions-agent/src/broker/assets/samples/requests/winget-vscode-skiphash.request.json |
Adds risky request sample. |
devolutions-agent/src/broker/assets/samples/requests/winget-unknown-install.request.json |
Adds denied request sample. |
crates/devolutions-agent-shared/Cargo.toml |
Adds temporary-file dependencies. |
crates/devolutions-agent-shared/src/lib.rs |
Exposes temporary-file utilities. |
crates/devolutions-agent-shared/src/temp_file.rs |
Adds shared temporary-script handling. |
devolutions-session/Cargo.toml |
Removes direct tempfile dependency. |
devolutions-session/src/main.rs |
Removes obsolete dependency import. |
devolutions-session/src/dvc/fs.rs |
Re-exports the shared guard. |
devolutions-session/src/dvc/task.rs |
Uses shared script utilities. |
devolutions-session/src/dvc/process.rs |
Converts I/O execution errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Current improvements list (generated by Copilot):
Suggested parallel split:
|
|
Given the diff size, I’ll honestly not be able to review as well as I wish I could. For the sake of merging quickly, do you think you could flag me some areas that you think are worth spending extra time reviewing on my end? Can you double check that nothing that is stable today is affected by this? The idea is that it’s okay to merge a POC in a somewhat unfinished state for improved velocity, so it’s easier to iterate together with follow up PRs against master, but it should not impact negatively any other stable component. Anything that is affecting other stable modules should be extracted in a dedicated PR so I can perform the usual hardened review. Otherwise, I’ll just conduct a minimal review and approve possibly with follow up suggestions. How does that sound? |
I think mostly windows executor need some review for critical security issues, and the code from devolutions-agent/updater which was moved to shared code but other than that, I think everything stable is untouched |
Benoît Cortier (CBenoit)
left a comment
Benoît Cortier (CBenoit)
left a comment
There was a problem hiding this comment.
Thank you, I double checked the overall architecture + the security sensitive code (to the best of my knowledge + using a AI-based rubber duck), and here are my findings.
Extra low/hardening issues found by Claude, in no specific order:
Temp script DACL is applied after creation (executor/windows/mod.rs:406-475). The file is created with the inherited ACL, content written, then SetNamedSecurityInfoW tightens it. In SYSTEM's C:\Windows\Temp this is mostly safe (new files there aren't Users-readable during the window, and tempfile uses CREATE_NEW random names so no pre-creation hijack), but creating the file atomically with the target SDDL (SECURITY_ATTRIBUTES at creation) removes the window entirely and is cleaner. Also worth an integration test confirming a non-admin user token can actually read the script from the broker's temp dir in real SYSTEM mode.
winget batch escaping (executor/windows/mod.rs:552-593) correctly follows CommandLineToArgvW + %-doubling + /V:OFF conventions, but the cmd.exe-layer handling of embedded " in a batch line is subtle and only unit-tested at the string level. An execution-level test (actually invoking through cmd) would harden this, especially for allow-rules that permit custom parameters.
skip_broker_signature_validation (config.rs, auth.rs:80-84) fully disables client auth. It's debug-scoped and logged at warn! (good) — just ensure it can never be set in shipped configs.
connection setup is unauthenticated work (pipe.rs:40-56): every connection does OpenProcess/exe_path/lookup_account before any signature gate; a connection flood is cheap-ish DoS. Low impact; note only.
The new code is gated and does not impact existing stable features. As discussed on Slack, I think at this point we should merge, and iterate from there 🙂
There was a problem hiding this comment.
nitpick: This is very close to tempfile::TempPath:
A path to a named temporary file without an open file handle... When dropped, the temporary file is deleted.
Instead of keep() + storing a PathBuf + a hand-rolled Drop, we could hold the TempPath from NamedTempFile::into_temp_path() and get deletion-on-drop for free. If we want to keep logging cleanup failures, we can retain a small Drop that calls TempPath::close() and logs the error.
Not a big win either way. Happy to leave as-is if you prefer the explicit version.
There was a problem hiding this comment.
nitpick (non-blocking, but worth deciding now): Placement of the TmpFileGuard + the script preamble constants are generic "write a temp script and clean it up" helpers. Nothing here is agent-specific. Landing them in devolutions-agent-shared reads as convenience placement (it's just the common dep) rather than a domain fit.
We already favor small focused utility crates (http-client-proxy, win-api-wrappers, secure-memory, …), a tiny dedicated crate (e.g. tmp-script-file or tmp-file) where we wrap tempfile with higher level functionalities feels like a better home. Maybe change that in a follow-up.
| notify = { version = "7", default-features = false, features = ["macos_kqueue"] } | ||
| now-policy = "0.1" | ||
| now-policy-api = { version = "0.1", features = ["policy-compat"] } | ||
| now-policy-server-template = { version = "0.1", features = ["policy-compat"] } |
There was a problem hiding this comment.
question: I suppose we’ll remove the policy-compat feature in the feature? What is the purpose of enabling it today, we never had such a feature until now?
There was a problem hiding this comment.
This is feature for gating conversion code between now-policy & now-policy-api types as now-policy-api deliberately was designed in such way, that direct use of now-policy-api crate is not required unless explicitly requested via policy-compat feature.
| fn recognizes_devolutions_certificate_thumbprints() { | ||
| for thumbprint in DEVOLUTIONS_CERT_THUMBPRINTS { | ||
| let mut thumbprint_bytes = [0u8; 20]; | ||
| hex::decode_to_slice(thumbprint, &mut thumbprint_bytes) | ||
| .expect("test thumbprint should be valid hexadecimal"); | ||
| assert!(is_devolutions_certificate_thumbprint(&thumbprint_bytes)); | ||
| } | ||
| } |
There was a problem hiding this comment.
praise: Exhaustive testing for a small N (= number of test cases), is a good practice.
| #[cfg(windows)] | ||
| pub mod broker; | ||
| #[cfg(windows)] | ||
| pub(crate) mod code_signing; |
There was a problem hiding this comment.
suggestion: Extract the package broker into its own crate.
The broker is shaping up nicely, but it's landing as a large, self-contained subsystem inside the devolutions-agent crate. I suggest pulling it out into a dedicated crate (e.g. crates/dwa-package-broker or crates/now-package-broker; to decide) as part of this work, before it grows further. This aligns with the repo's existing philosophy of splitting concerns into focused sub-crates.
rationale:
- Size and cohesion: the broker already spans ~15 modules with a clear boundary:
auth,command_builder/{winget,powershell},evaluator/*,executor/*, etc. It's a typical leaf-crate candidate. - Dependency surface: This PR adds a large, broker-only dependency set directly to the
devolutions-agentbinary:axum,hyper,hyper-util,tower-service,notify,now-policy,now-policy-api,now-policy-server-template,serde_yaml,semver,regex,chrono,tokio-util. Moving these behind a crate keeps the agent binary's manifest lean. - Feature-gating: A separate crate also gives a natural place to hang a Cargo feature so the whole subsystem and its dep tree can be compiled out entirely when not needed (typically during development, when working on a separate module).
On the shared code_signing module: as part of this migration, I think we should also extract code_signing (currently moved into devolutions-agent/src/code_signing.rs) so it can be shared between the updater (updater/package.rs, which historically owned DEVOLUTIONS_CERT_THUMBPRINTS) and the broker (broker/auth.rs, which now consumes validate_devolutions_authenticode_signature). Right now the broker reaches back into the agent crate for it via use crate::code_signing::..., which won't work once the broker is its own crate.
I’ll be happy to help with the mechanical move. Not blocking, good follow up.
| }; | ||
|
|
||
| expected_domain.eq_ignore_ascii_case(&actual.domain) && expected_name.eq_ignore_ascii_case(&actual.name) | ||
| } |
There was a problem hiding this comment.
issue: We are matching identity by by display name instead of SID.
This compares DOMAIN\name strings, and when the request's effective_user is unqualified ("alice") it ignores the domain entirely. find_user_session() likewise matches by username only when unqualified. On a host with both MACHINE\alice and DOMAIN\alice, the authenticated caller and the session actually launched into can differ, and the broker can run the operation in the wrong user's session. The token-side domain fix is only helping partially.
suggestion: Capture the caller's SID at connect (we already have read sid_and_attributes()), then thread that SID through ExecutionContext, and both authenticate and select the WTS session by SID rather than by name string.
| fn canonicalize_for_comparison(path: &Path) -> anyhow::Result<PathBuf> { | ||
| Ok(std::fs::canonicalize(path)?) | ||
| } |
There was a problem hiding this comment.
nitpick: Arguably non necessary helper function on top of canonicalize.
| fn same_windows_path(left: &Path, right: &Path) -> bool { | ||
| left.as_os_str() | ||
| .to_string_lossy() | ||
| .eq_ignore_ascii_case(&right.as_os_str().to_string_lossy()) | ||
| } |
There was a problem hiding this comment.
issue: to_string_lossy() looking wrong to me as a security-relevant identity check.
suggestion: Use GetFileInformationByHandleEx with FILE_ID_INFO via win-api-wrappers / windows, and compare the file identity directly. This is immune to all the edge cases I can think of. (Unicode casing, invalid UTF-8, etc)
| /// The main package-manager command line as separate arguments (exe + args). | ||
| pub command: Vec<String>, | ||
| /// Optional shell command to run after the main command (`cmd.exe /S /C`). | ||
| pub post_command: Option<String>, |
There was a problem hiding this comment.
issue: pre_operation_command/post_operation_command are raw strings written to a .bat and run via cmd.exe with the possibly elevated token.
Claude’s analysis:
The only gate is the boolean allow_pre_post_commands, and constraints are permissive when absent (let Some(c) = constraints else { return true }). So any allow-rule that omits constraints, or sets that one flag, yields arbitrary elevated code execution regardless of the package allowlist, the packages allowlist becomes moot. The command content is never restricted or previewed to the policy.
Recommendation: at minimum document this loudly and make it opt-in per rule with a content allowlist; consider defaulting constraints to deny-all when a rule has none, so constraints: None isn't a silent "allow every risky option."
| /// Load a policy document from a file path. | ||
| /// | ||
| /// The file format is detected from the extension: | ||
| /// - `.json` — parsed as JSON | ||
| /// - `.yaml` or `.yml` — parsed as YAML | ||
| /// | ||
| /// Deserialization performs all validation (structure, types, length constraints, patterns). | ||
| pub fn load_policy(path: &Path) -> anyhow::Result<PolicyDocument> { | ||
| let content = std::fs::read_to_string(path) | ||
| .map_err(|e| anyhow::anyhow!("failed to read policy file at {}: {e}", path.display()))?; | ||
|
|
||
| let policy = deserialize_policy(&content, path)?; | ||
|
|
||
| info!( | ||
| policy_id = %policy.metadata.id, | ||
| revision = policy.metadata.revision, | ||
| rules_count = policy.rules.len(), | ||
| "Loaded policy" | ||
| ); | ||
|
|
||
| Ok(policy) | ||
| } |
There was a problem hiding this comment.
issue: Policy file integrity is trusted without an ownership/ACL check.
Claude’s analysis:
The policy is the entire authorization control, but it's read from %PROGRAMDATA%\Devolutions\Agent\ (or a configured path) with no check that the file/directory is writable only by Administrators/SYSTEM. C:\ProgramData subtrees are a known spot for over-permissive inherited ACEs; if a standard user can write the policy, they self-authorize arbitrary elevated installs.
Recommendation: before trusting a loaded policy, verify the file's owner and DACL restrict write to SYSTEM/Administrators (fail-closed to Paused otherwise). Defense-in-depth even if the installer sets ACLs correctly.
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>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
87d48ab to
26094bd
Compare
66c8377
into
master
Implements now package broker (Also see UniGetUI WIP branch Devolutions/UniGetUI#4826)
Implemented
Not implemented yet
Steps to run this branch:
"PackageBroker":{"Enabled":true, "PolicyPath": "C:/path/to/policy.json"}"__debug__": { "skip_broker_signature_validation": true }UseAgentBrokerfile at%LOCALAPPDATA%/UniGetUI/Configuration(UniGetUI uses file-based config)Do not merge yet, testing in progress