From 2a94a85cfe88b794c27b29ae195ba21cb2ba290d Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 3 Aug 2026 13:11:27 +0300 Subject: [PATCH 1/4] fix(agent): probe package manager availability instead of hardcoding capabilities The broker capabilities endpoint statically claimed all supported package managers (including PowerShell 7) without checking whether they exist on the machine. UniGetUI preflights against capabilities, so wrong capabilities turned into runtime failures instead of clean unsupported errors. Manager executables are now probed against the target user's environment using the same resolution rules the execution path applies, and the advertised capabilities are filtered to the managers actually available. Probe results are cached per user and refreshed after a short TTL. Issue: DGW-435 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/broker/command_builder/dotnet.rs | 2 +- devolutions-agent/src/broker/executor/mod.rs | 27 +++- .../src/broker/executor/windows/mod.rs | 97 +++++++++++- devolutions-agent/src/broker/server/mod.rs | 142 +++++++++++++++++- .../src/broker/server/responses.rs | 16 +- devolutions-agent/src/broker/task.rs | 1 + 6 files changed, 272 insertions(+), 13 deletions(-) diff --git a/devolutions-agent/src/broker/command_builder/dotnet.rs b/devolutions-agent/src/broker/command_builder/dotnet.rs index 8e199893d..0128fc1eb 100644 --- a/devolutions-agent/src/broker/command_builder/dotnet.rs +++ b/devolutions-agent/src/broker/command_builder/dotnet.rs @@ -120,7 +120,7 @@ fn dotnet_source(request: &PackageRequest) -> anyhow::Result<&str> { Ok(NUGET_ORG_V3_SOURCE) } -fn trusted_dotnet_executable() -> String { +pub(crate) fn trusted_dotnet_executable() -> String { let program_files = std::env::var("ProgramFiles") .ok() .filter(|path| trusted_program_files_path(path)) diff --git a/devolutions-agent/src/broker/executor/mod.rs b/devolutions-agent/src/broker/executor/mod.rs index ff2d70dfd..68f9b01f6 100644 --- a/devolutions-agent/src/broker/executor/mod.rs +++ b/devolutions-agent/src/broker/executor/mod.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; -use now_policy_api::{Elevation, Scope}; +use now_policy_api::{Elevation, ManagerName, Scope}; use tracing::info; use win_api_wrappers::identity::sid::Sid; @@ -57,6 +57,21 @@ pub struct ExecutionContext { pub type ProcessStartedCallback = std::sync::Arc) + Send + Sync>; +/// All package managers the broker knows how to drive. +pub const BROKER_SUPPORTED_MANAGERS: [ManagerName; 11] = [ + ManagerName::Winget, + ManagerName::Chocolatey, + ManagerName::Bun, + ManagerName::Cargo, + ManagerName::Dotnet, + ManagerName::Pip, + ManagerName::Npm, + ManagerName::PowerShell, + ManagerName::PowerShell7, + ManagerName::Scoop, + ManagerName::Vcpkg, +]; + /// Trait for command execution strategies. #[async_trait] pub trait CommandExecutor: Send + Sync { @@ -69,6 +84,16 @@ pub trait CommandExecutor: Send + Sync { ctx: &ExecutionContext, process_started: Option, ) -> anyhow::Result; + + /// Probe which package managers are actually available for the target user. + /// + /// The default implementation optimistically reports every broker-supported manager; + /// platform executors override this to resolve each manager executable using the same + /// logic the execution path uses, so advertised capabilities match what would run. + async fn probe_managers(&self, user_sid: &Sid) -> Vec { + let _ = user_sid; + BROKER_SUPPORTED_MANAGERS.to_vec() + } } /// Dry-run executor that only logs commands without running them. diff --git a/devolutions-agent/src/broker/executor/windows/mod.rs b/devolutions-agent/src/broker/executor/windows/mod.rs index 7f0b023a0..ff65da32c 100644 --- a/devolutions-agent/src/broker/executor/windows/mod.rs +++ b/devolutions-agent/src/broker/executor/windows/mod.rs @@ -9,15 +9,16 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, bail}; use async_trait::async_trait; use devolutions_agent_shared::temp_file::{BATCH_UTF8_PREAMBLE, POWERSHELL_UTF8_ENCODING_PREAMBLE, TmpFileGuard}; -use now_policy_api::{Elevation, Scope}; +use now_policy_api::{Elevation, ManagerName, Scope}; use tracing::{debug, info, warn}; +use win_api_wrappers::identity::sid::Sid; use win_api_wrappers::process::Process; use win_api_wrappers::security::privilege::{self, ScopedPrivileges}; use win_api_wrappers::token::Token; use win_api_wrappers::utils::WideString; use windows::Win32::Security::{TOKEN_ADJUST_PRIVILEGES, TOKEN_ALL_ACCESS, TOKEN_QUERY}; -use super::{CommandExecutor, ExecutionContext, ExecutionOutput, ProcessStartedCallback}; +use super::{BROKER_SUPPORTED_MANAGERS, CommandExecutor, ExecutionContext, ExecutionOutput, ProcessStartedCallback}; mod process; mod token; @@ -90,6 +91,98 @@ impl CommandExecutor for WindowsExecutor { .await .context("blocking task panicked")? } + + async fn probe_managers(&self, user_sid: &Sid) -> Vec { + let is_system = self.is_system; + let user_sid = user_sid.clone(); + + // All Win32 calls are blocking — run in a blocking thread. + let probed = tokio::task::spawn_blocking(move || -> anyhow::Result> { + let user_env = probe_user_environment(is_system, &user_sid)?; + Ok(BROKER_SUPPORTED_MANAGERS + .into_iter() + .filter(|manager| manager_is_available(*manager, &user_env)) + .collect()) + }) + .await; + + match probed { + Ok(Ok(managers)) => managers, + Ok(Err(error)) => { + // Fail open: advertising the full set preserves the previous behavior when the + // target user environment cannot be inspected (e.g., no active session). + warn!( + error = format!("{error:#}"), + "Failed to probe package manager availability; advertising all supported managers" + ); + BROKER_SUPPORTED_MANAGERS.to_vec() + } + Err(error) => { + warn!(%error, "Package manager availability probe panicked; advertising all supported managers"); + BROKER_SUPPORTED_MANAGERS.to_vec() + } + } + } +} + +/// Load the environment block of the target user identified by `user_sid`. +/// +/// In SYSTEM (service) mode the token comes from the user's active session, matching the +/// token later used for execution. In user (development) mode the current process token is used. +fn probe_user_environment(is_system: bool, user_sid: &Sid) -> anyhow::Result> { + let token = if is_system { + // WTSQueryUserToken (used by find_user_session) requires the SeTcb privilege. + let mut process_token = Process::current_process() + .token(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY) + .context("failed to open process token for privilege adjustment")?; + let _priv_tcb = + ScopedPrivileges::enter(&mut process_token, &[privilege::SE_TCB_NAME]).context("failed to enable SeTcb")?; + + let (_session_id, user_token) = + find_user_session(user_sid).context("failed to find active session for user")?; + user_token + } else { + Process::current_process() + .token(TOKEN_ALL_ACCESS) + .context("failed to open current process token")? + }; + + win_api_wrappers::utils::environment_block(Some(&token), false).context("failed to load user environment block") +} + +/// Check whether a package manager is usable for the target user, mirroring the executable +/// resolution rules the execution path applies for that manager. +fn manager_is_available(manager: ManagerName, user_env: &HashMap) -> bool { + match manager { + ManagerName::Winget => resolve_winget_executable(user_env).is_ok(), + ManagerName::Chocolatey => default_chocolatey_install_dir() + .and_then(|root| resolve_trusted_chocolatey_executable(Some(user_env), &root)) + .is_ok(), + ManagerName::Bun => resolve_bun_executable("bun", user_env).is_ok(), + ManagerName::Cargo => resolve_cargo_executable(user_env).is_ok(), + ManagerName::Dotnet => { + Path::new(&crate::broker::command_builder::dotnet::trusted_dotnet_executable()).is_file() + } + ManagerName::Pip => resolve_python_executable("python.exe", user_env).is_ok(), + // npm runs through the user PATH `npm` shim inside the trusted PowerShell wrapper. + ManagerName::Npm => path_contains_executable(user_env, &["npm.cmd", "npm.exe"]), + ManagerName::PowerShell => Path::new(&trusted_windows_powershell_executable()).is_file(), + ManagerName::PowerShell7 => Path::new(&trusted_powershell7_executable()).is_file(), + // Scoop is driven through the user's `scoop.ps1` shim resolved from PATH. + ManagerName::Scoop => path_contains_executable(user_env, &["scoop.ps1", "scoop.cmd"]), + ManagerName::Vcpkg => resolve_vcpkg_executable(user_env).is_ok(), + _ => false, + } +} + +/// Return true when any of `names` exists as a file in a directory listed in the environment PATH. +fn path_contains_executable(env: &HashMap, names: &[&str]) -> bool { + let path_var = env_value_ignore_case(env, "PATH").unwrap_or_default(); + path_var + .split(';') + .map(str::trim) + .filter(|dir| !dir.is_empty()) + .any(|dir| names.iter().any(|name| PathBuf::from(dir).join(name).is_file())) } /// Execute a command in the context of the target user's session (SYSTEM mode). diff --git a/devolutions-agent/src/broker/server/mod.rs b/devolutions-agent/src/broker/server/mod.rs index 78a63a16c..815082d60 100644 --- a/devolutions-agent/src/broker/server/mod.rs +++ b/devolutions-agent/src/broker/server/mod.rs @@ -1,6 +1,8 @@ //! Runtime implementation of the shared NOW package broker server facade. +use std::collections::HashMap; use std::sync::{Arc, RwLock}; +use std::time::{Duration, Instant}; use async_trait::async_trait; use base64::Engine as _; @@ -9,8 +11,8 @@ use now_policy::PolicyDocument; use now_policy_api::{ Base64Utf8Data, CapabilitiesResponse, CapabilitiesResponseKind, Decision, DecisionInfo, Elevation, ErrorCode, ErrorResponse, EvaluationResponse, EvaluationResponseKind, ExecutionResponse, ExecutionResponseKind, - HealthResponse, HealthResponseKind, HealthStatus, OperationStatus, OperationSubmission, PackageRequest, Scope, - StatusRequest, StatusResponse, StatusResponseKind, Transport, + HealthResponse, HealthResponseKind, HealthStatus, ManagerCapability, ManagerName, OperationStatus, + OperationSubmission, PackageRequest, Scope, StatusRequest, StatusResponse, StatusResponseKind, Transport, }; use now_policy_server_template::{MAX_REQUEST_BODY_BYTES, PackageBrokerServer, SharedPackageBrokerServer}; use tracing::{info, trace, warn}; @@ -28,10 +30,35 @@ mod responses; pub use connection::serve_connection; use responses::{ - api_version, default_manager_capabilities, diagnostics, error_response, new_operation_id, parse_rule_id, - policy_info, policy_validity_failure, request_summary, server_context, + api_version, diagnostics, error_response, filter_manager_capabilities, new_operation_id, parse_rule_id, + policy_info, policy_validity_failure, request_summary, server_context, supported_manager_capabilities, }; +/// How long a per-user manager availability probe stays fresh before it is re-run. +const MANAGER_PROBE_TTL: Duration = Duration::from_secs(60); + +/// Per-user cache of probed manager availability. +/// +/// Probing walks the target user's environment (PATH lookups and file checks); the cache +/// keeps capability requests cheap while still picking up newly (un)installed managers +/// after [`MANAGER_PROBE_TTL`]. +#[derive(Default)] +pub struct ManagerProbeCache(parking_lot::Mutex)>>); + +impl ManagerProbeCache { + fn get_fresh(&self, user_key: &str) -> Option> { + let cache = self.0.lock(); + cache + .get(user_key) + .filter(|(probed_at, _)| probed_at.elapsed() < MANAGER_PROBE_TTL) + .map(|(_, managers)| managers.clone()) + } + + fn insert(&self, user_key: String, managers: Vec) { + self.0.lock().insert(user_key, (Instant::now(), managers)); + } +} + /// Shared server state. pub struct BrokerState { /// Current policy. `None` means the broker is paused (policy file missing or corrupted). @@ -40,6 +67,7 @@ pub struct BrokerState { pub pipe_name: String, pub tracker: OperationTracker, pub skip_signature_validation: bool, + pub manager_probe_cache: ManagerProbeCache, } struct EvaluatedRequest { @@ -69,7 +97,7 @@ impl PackageBrokerServer for BrokerConnection { } async fn capabilities(&self) -> CapabilitiesResponse { - self.state.capabilities().await + self.state.capabilities(self.client.user_sid()).await } async fn evaluate(&self, request: PackageRequest) -> Result { @@ -124,17 +152,41 @@ impl BrokerState { } } - async fn capabilities(&self) -> CapabilitiesResponse { + async fn capabilities(&self, user_sid: &Sid) -> CapabilitiesResponse { CapabilitiesResponse { response_kind: CapabilitiesResponseKind, response_version: api_version(), server: server_context(), transports: vec![Transport::HttpNamedPipe], - managers: default_manager_capabilities(), + managers: self.probed_manager_capabilities(user_sid).await, max_request_body_bytes: MAX_REQUEST_BODY_BYTES as u64, } } + /// Capabilities for the managers actually available to the target user. + /// + /// Availability is probed through the executor (mirroring execution-time resolution) + /// and cached per user for [`MANAGER_PROBE_TTL`]. + async fn probed_manager_capabilities(&self, user_sid: &Sid) -> Vec { + let user_key = user_sid.to_string(); + + let available = match self.manager_probe_cache.get_fresh(&user_key) { + Some(available) => available, + None => { + let available = self.executor.probe_managers(user_sid).await; + info!( + user_sid = %user_key, + available_managers = ?available, + "Probed package manager availability" + ); + self.manager_probe_cache.insert(user_key, available.clone()); + available + } + }; + + filter_manager_capabilities(supported_manager_capabilities(), &available) + } + async fn evaluate(&self, request: PackageRequest) -> Result { let evaluated = self.evaluate_request(&request)?; @@ -374,6 +426,8 @@ impl PackageRequestClientOwner for PackageRequest { mod tests { #![allow(clippy::unwrap_used)] + use std::sync::atomic::{AtomicUsize, Ordering}; + use chrono::Utc; use now_policy::{ PackageBrokerPolicy, PolicyEnforcement, PolicyMetadata, PolicySchemaUri, ResourceId, RulePrecedence, @@ -397,6 +451,27 @@ mod tests { } } + struct FakeExecutor { + available: Vec, + probe_count: AtomicUsize, + } + + #[async_trait] + impl CommandExecutor for FakeExecutor { + async fn execute( + &self, + _ctx: &ExecutionContext, + _process_started: Option, + ) -> anyhow::Result { + anyhow::bail!("not used in this test") + } + + async fn probe_managers(&self, _user_sid: &Sid) -> Vec { + self.probe_count.fetch_add(1, Ordering::SeqCst); + self.available.clone() + } + } + /// The most permissive policy possible: default Allow, no rules, audit mode on. fn permissive_policy() -> PolicyDocument { PolicyDocument { @@ -429,6 +504,7 @@ mod tests { pipe_name: "test-pipe".to_owned(), tracker: OperationTracker::new(), skip_signature_validation: true, + manager_probe_cache: Default::default(), } } @@ -439,7 +515,7 @@ mod tests { request_id: api::ResourceId::from("req-1"), created_at: Utc::now(), operation: api::Operation::Install, - manager: api::ManagerName::Winget, + manager: ManagerName::Winget, source: api::RequestSource { name: "winget".to_owned(), url: None, @@ -535,4 +611,54 @@ mod tests { }; assert!(evaluated.would_execute); } + + fn make_state(available: Vec) -> (Arc, Arc) { + let executor = Arc::new(FakeExecutor { + available, + probe_count: AtomicUsize::new(0), + }); + let state = Arc::new(BrokerState { + policy: RwLock::new(None), + executor: Arc::clone(&executor) as Arc, + pipe_name: "test-pipe".to_owned(), + tracker: OperationTracker::new(), + skip_signature_validation: true, + manager_probe_cache: Default::default(), + }); + (state, executor) + } + + fn test_sid() -> Sid { + Sid::from_well_known(windows::Win32::Security::WinLocalSystemSid, None).unwrap() + } + + #[tokio::test] + async fn capabilities_only_advertise_probed_managers() { + let (state, _) = make_state(vec![ManagerName::Winget, ManagerName::PowerShell]); + + let response = state.capabilities(&test_sid()).await; + + let managers: Vec = response.managers.iter().map(|capability| capability.manager).collect(); + assert_eq!(managers, vec![ManagerName::Winget, ManagerName::PowerShell]); + } + + #[tokio::test] + async fn capabilities_empty_when_no_manager_available() { + let (state, _) = make_state(Vec::new()); + + let response = state.capabilities(&test_sid()).await; + + assert!(response.managers.is_empty()); + } + + #[tokio::test] + async fn manager_probe_is_cached_per_user() { + let (state, executor) = make_state(vec![ManagerName::Winget]); + let sid = test_sid(); + + state.capabilities(&sid).await; + state.capabilities(&sid).await; + + assert_eq!(executor.probe_count.load(Ordering::SeqCst), 1); + } } diff --git a/devolutions-agent/src/broker/server/responses.rs b/devolutions-agent/src/broker/server/responses.rs index 1d87eb1b3..74b27a7b1 100644 --- a/devolutions-agent/src/broker/server/responses.rs +++ b/devolutions-agent/src/broker/server/responses.rs @@ -21,7 +21,11 @@ pub(super) fn server_context() -> ServerContext { } } -pub(super) fn default_manager_capabilities() -> Vec { +/// Capability descriptions for every manager the broker can drive. +/// +/// This is the full supported set; the server filters it down to the managers actually +/// available for the requesting user (see [`crate::broker::server::BrokerState`]). +pub(super) fn supported_manager_capabilities() -> Vec { vec![ ManagerCapability { manager: ManagerName::Winget, @@ -157,6 +161,16 @@ pub(super) fn default_manager_capabilities() -> Vec { ] } +/// Keep only the capabilities of managers reported available by the probe. +pub(super) fn filter_manager_capabilities( + all: Vec, + available: &[ManagerName], +) -> Vec { + all.into_iter() + .filter(|capability| available.contains(&capability.manager)) + .collect() +} + pub(super) fn request_summary(request: &PackageRequest) -> RequestSummary { RequestSummary { manager: Some(request.manager), diff --git a/devolutions-agent/src/broker/task.rs b/devolutions-agent/src/broker/task.rs index 6aa75b0a3..100009266 100644 --- a/devolutions-agent/src/broker/task.rs +++ b/devolutions-agent/src/broker/task.rs @@ -104,6 +104,7 @@ impl Task for BrokerTask { pipe_name: self.config.pipe_name.clone(), tracker: crate::broker::operation_tracker::OperationTracker::new(), skip_signature_validation: self.config.skip_signature_validation, + manager_probe_cache: Default::default(), }); // Bridge the agent's ShutdownSignal to the cancellation token used by subsystems. From aef5f7f746275328cef9eb6268522456fd33b6bb Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 3 Aug 2026 13:19:00 +0300 Subject: [PATCH 2/4] fix(agent): default probe_managers implementation advertises no managers Availability must be positively established by a platform executor rather than assumed for unknown platforms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- devolutions-agent/src/broker/executor/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/devolutions-agent/src/broker/executor/mod.rs b/devolutions-agent/src/broker/executor/mod.rs index 68f9b01f6..2cd5cd745 100644 --- a/devolutions-agent/src/broker/executor/mod.rs +++ b/devolutions-agent/src/broker/executor/mod.rs @@ -87,12 +87,12 @@ pub trait CommandExecutor: Send + Sync { /// Probe which package managers are actually available for the target user. /// - /// The default implementation optimistically reports every broker-supported manager; - /// platform executors override this to resolve each manager executable using the same - /// logic the execution path uses, so advertised capabilities match what would run. + /// The default implementation reports no managers at all: availability must be + /// positively established by a platform executor, which overrides this to resolve + /// each manager executable using the same logic the execution path uses. async fn probe_managers(&self, user_sid: &Sid) -> Vec { let _ = user_sid; - BROKER_SUPPORTED_MANAGERS.to_vec() + Vec::new() } } From 04db88aadb49a1bf4b43f28ce40e51caa4a65ea2 Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 3 Aug 2026 16:14:17 +0300 Subject: [PATCH 3/4] fix(agent): address review comments on manager availability probing - Fail closed when the availability probe fails: advertise no managers instead of the full static list, since execution requires the same session/environment lookup and would fail anyway. - Require the trusted Windows PowerShell host for npm and Scoop availability, and only accept the scoop.ps1 shim (matching build_scoop_command resolution). - Replace ScopedPrivileges with a process-wide reference-counted SharedPrivileges guard in both SYSTEM-mode execution and probing, so concurrent requests cannot disable SeTcb (and friends) out from under one another. - Make the probe cache TTL injectable and add a TTL-expiry refresh test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/broker/executor/windows/mod.rs | 66 ++++----- .../src/broker/executor/windows/privileges.rs | 138 ++++++++++++++++++ devolutions-agent/src/broker/server/mod.rs | 50 ++++++- 3 files changed, 213 insertions(+), 41 deletions(-) create mode 100644 devolutions-agent/src/broker/executor/windows/privileges.rs diff --git a/devolutions-agent/src/broker/executor/windows/mod.rs b/devolutions-agent/src/broker/executor/windows/mod.rs index ff65da32c..3288c064c 100644 --- a/devolutions-agent/src/broker/executor/windows/mod.rs +++ b/devolutions-agent/src/broker/executor/windows/mod.rs @@ -13,16 +13,18 @@ use now_policy_api::{Elevation, ManagerName, Scope}; use tracing::{debug, info, warn}; use win_api_wrappers::identity::sid::Sid; use win_api_wrappers::process::Process; -use win_api_wrappers::security::privilege::{self, ScopedPrivileges}; +use win_api_wrappers::security::privilege; use win_api_wrappers::token::Token; use win_api_wrappers::utils::WideString; -use windows::Win32::Security::{TOKEN_ADJUST_PRIVILEGES, TOKEN_ALL_ACCESS, TOKEN_QUERY}; +use windows::Win32::Security::TOKEN_ALL_ACCESS; use super::{BROKER_SUPPORTED_MANAGERS, CommandExecutor, ExecutionContext, ExecutionOutput, ProcessStartedCallback}; +mod privileges; mod process; mod token; +use privileges::SharedPrivileges; use process::create_process; use token::{detect_running_as_system, find_user_session, get_elevated_token}; @@ -109,17 +111,17 @@ impl CommandExecutor for WindowsExecutor { match probed { Ok(Ok(managers)) => managers, Ok(Err(error)) => { - // Fail open: advertising the full set preserves the previous behavior when the - // target user environment cannot be inspected (e.g., no active session). + // Fail closed: the same session/environment lookup is required for execution, + // so a manager that cannot be verified cannot run either. warn!( error = format!("{error:#}"), - "Failed to probe package manager availability; advertising all supported managers" + "Failed to probe package manager availability; advertising no managers" ); - BROKER_SUPPORTED_MANAGERS.to_vec() + Vec::new() } Err(error) => { - warn!(%error, "Package manager availability probe panicked; advertising all supported managers"); - BROKER_SUPPORTED_MANAGERS.to_vec() + warn!(%error, "Package manager availability probe panicked; advertising no managers"); + Vec::new() } } } @@ -132,11 +134,7 @@ impl CommandExecutor for WindowsExecutor { fn probe_user_environment(is_system: bool, user_sid: &Sid) -> anyhow::Result> { let token = if is_system { // WTSQueryUserToken (used by find_user_session) requires the SeTcb privilege. - let mut process_token = Process::current_process() - .token(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY) - .context("failed to open process token for privilege adjustment")?; - let _priv_tcb = - ScopedPrivileges::enter(&mut process_token, &[privilege::SE_TCB_NAME]).context("failed to enable SeTcb")?; + let _privileges = SharedPrivileges::acquire(&[privilege::SE_TCB_NAME]).context("failed to enable SeTcb")?; let (_session_id, user_token) = find_user_session(user_sid).context("failed to find active session for user")?; @@ -164,12 +162,20 @@ fn manager_is_available(manager: ManagerName, user_env: &HashMap Path::new(&crate::broker::command_builder::dotnet::trusted_dotnet_executable()).is_file() } ManagerName::Pip => resolve_python_executable("python.exe", user_env).is_ok(), - // npm runs through the user PATH `npm` shim inside the trusted PowerShell wrapper. - ManagerName::Npm => path_contains_executable(user_env, &["npm.cmd", "npm.exe"]), + // npm runs through the user PATH `npm` shim inside the trusted Windows PowerShell wrapper, + // so both the shim and the host must exist. + ManagerName::Npm => { + Path::new(&trusted_windows_powershell_executable()).is_file() + && path_contains_executable(user_env, &["npm.cmd", "npm.exe"]) + } ManagerName::PowerShell => Path::new(&trusted_windows_powershell_executable()).is_file(), ManagerName::PowerShell7 => Path::new(&trusted_powershell7_executable()).is_file(), - // Scoop is driven through the user's `scoop.ps1` shim resolved from PATH. - ManagerName::Scoop => path_contains_executable(user_env, &["scoop.ps1", "scoop.cmd"]), + // Scoop is driven through the user's `scoop.ps1` shim (resolved via `Get-Command + // -CommandType ExternalScript`) inside the trusted Windows PowerShell wrapper. + ManagerName::Scoop => { + Path::new(&trusted_windows_powershell_executable()).is_file() + && path_contains_executable(user_env, &["scoop.ps1"]) + } ManagerName::Vcpkg => resolve_vcpkg_executable(user_env).is_ok(), _ => false, } @@ -205,22 +211,16 @@ fn execute_as_system( ); // Enable privileges required by CreateProcessAsUserW when running as SYSTEM. - // These are held by SYSTEM but not enabled by default. - let mut process_token = Process::current_process() - .token(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY) - .context("failed to open process token for privilege adjustment")?; - - debug!("Enabling SeTcb privilege"); - let mut _priv_tcb = - ScopedPrivileges::enter(&mut process_token, &[privilege::SE_TCB_NAME]).context("failed to enable SeTcb")?; - - debug!("Enabling SeAssignPrimaryToken privilege"); - let mut _priv_primary = ScopedPrivileges::enter(_priv_tcb.token_mut(), &[privilege::SE_ASSIGNPRIMARYTOKEN_NAME]) - .context("failed to enable SeAssignPrimaryToken")?; - - debug!("Enabling SeIncreaseQuota privilege"); - let _priv_quota = ScopedPrivileges::enter(_priv_primary.token_mut(), &[privilege::SE_INCREASE_QUOTA_NAME]) - .context("failed to enable SeIncreaseQuota")?; + // These are held by SYSTEM but not enabled by default. The guard is reference-counted + // process-wide so concurrent requests (and availability probes) cannot disable a + // privilege out from under one another. + debug!("Enabling SeTcb, SeAssignPrimaryToken and SeIncreaseQuota privileges"); + let _privileges = SharedPrivileges::acquire(&[ + privilege::SE_TCB_NAME, + privilege::SE_ASSIGNPRIMARYTOKEN_NAME, + privilege::SE_INCREASE_QUOTA_NAME, + ]) + .context("failed to enable privileges required for SYSTEM-mode execution")?; debug!("All privileges enabled, finding user session"); diff --git a/devolutions-agent/src/broker/executor/windows/privileges.rs b/devolutions-agent/src/broker/executor/windows/privileges.rs new file mode 100644 index 000000000..8175a9d0c --- /dev/null +++ b/devolutions-agent/src/broker/executor/windows/privileges.rs @@ -0,0 +1,138 @@ +//! Process-wide, reference-counted privilege enabling. +//! +//! `ScopedPrivileges` unconditionally disables privileges on drop, which is unsafe with +//! concurrent broker requests: dropping one scope would disable a privilege another +//! in-flight request still relies on (privileges are process-token state, not per-handle). +//! [`SharedPrivileges`] keeps a per-privilege reference count and only enables a privilege +//! on the first acquisition and disables it again when the last guard is dropped. + +use std::collections::HashMap; +use std::sync::Mutex; + +use anyhow::Context as _; +use tracing::error; +use win_api_wrappers::process::Process; +use win_api_wrappers::security::privilege; +use win_api_wrappers::str::U16CStr; +use win_api_wrappers::token::TokenPrivilegesAdjustment; +use windows::Win32::Security::{TOKEN_ADJUST_PRIVILEGES, TOKEN_QUERY}; + +static PRIVILEGE_REFCOUNTS: Mutex>> = Mutex::new(None); + +/// Guard keeping a set of process token privileges enabled. +/// +/// Privileges are disabled on drop only when no other live guard still requires them. +pub(super) struct SharedPrivileges { + names: Vec<&'static U16CStr>, +} + +impl SharedPrivileges { + /// Enable `names` on the current process token, reference-counted across all guards. + pub(super) fn acquire(names: &[&'static U16CStr]) -> anyhow::Result { + let mut guard = PRIVILEGE_REFCOUNTS.lock().expect("privilege refcount lock poisoned"); + let refcounts = guard.get_or_insert_with(HashMap::new); + + let mut to_enable = Vec::new(); + for name in names { + let count = refcounts.entry(name.to_string_lossy()).or_insert(0); + if *count == 0 { + to_enable.push(*name); + } + *count += 1; + } + + if let Err(error) = adjust(&to_enable, true) { + // Roll back the reference counts taken above before reporting the failure. + for name in names { + if let Some(count) = refcounts.get_mut(&name.to_string_lossy()) { + *count = count.saturating_sub(1); + } + } + return Err(error); + } + + Ok(Self { names: names.to_vec() }) + } +} + +impl Drop for SharedPrivileges { + fn drop(&mut self) { + let mut guard = PRIVILEGE_REFCOUNTS.lock().expect("privilege refcount lock poisoned"); + let Some(refcounts) = guard.as_mut() else { + return; + }; + + let mut to_disable = Vec::new(); + for name in &self.names { + if let Some(count) = refcounts.get_mut(&name.to_string_lossy()) { + *count = count.saturating_sub(1); + if *count == 0 { + to_disable.push(*name); + } + } + } + + if let Err(error) = adjust(&to_disable, false) { + error!(error = format!("{error:#}"), "Failed to disable shared privileges"); + } + } +} + +/// Enable or disable `names` on the current process token. +/// +/// Must be called with the refcount lock held so enable/disable operations are serialized. +fn adjust(names: &[&U16CStr], enable: bool) -> anyhow::Result<()> { + if names.is_empty() { + return Ok(()); + } + + let mut luids = Vec::with_capacity(names.len()); + for name in names { + luids.push(privilege::lookup_privilege_value(None, name).context("failed to look up privilege value")?); + } + + let adjustment = if enable { + TokenPrivilegesAdjustment::Enable(luids) + } else { + TokenPrivilegesAdjustment::Disable(luids) + }; + + let mut token = Process::current_process() + .token(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY) + .context("failed to open process token for privilege adjustment")?; + token + .adjust_privileges(&adjustment) + .context("failed to adjust privileges") +} + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used)] + + use super::*; + + #[test] + fn nested_guards_keep_privilege_enabled_until_last_drop() { + // SeChangeNotify is available to any token, making the test runnable without elevation. + let name = privilege::SE_CHANGE_NOTIFY_NAME; + + let first = SharedPrivileges::acquire(&[name]).unwrap(); + let second = SharedPrivileges::acquire(&[name]).unwrap(); + + drop(first); + + { + let guard = PRIVILEGE_REFCOUNTS.lock().unwrap(); + let refcounts = guard.as_ref().unwrap(); + assert_eq!(refcounts.get(&name.to_string_lossy()), Some(&1)); + } + + drop(second); + + { + let guard = PRIVILEGE_REFCOUNTS.lock().unwrap(); + let refcounts = guard.as_ref().unwrap(); + assert_eq!(refcounts.get(&name.to_string_lossy()), Some(&0)); + } + } +} diff --git a/devolutions-agent/src/broker/server/mod.rs b/devolutions-agent/src/broker/server/mod.rs index 815082d60..abf5b45de 100644 --- a/devolutions-agent/src/broker/server/mod.rs +++ b/devolutions-agent/src/broker/server/mod.rs @@ -41,21 +41,36 @@ const MANAGER_PROBE_TTL: Duration = Duration::from_secs(60); /// /// Probing walks the target user's environment (PATH lookups and file checks); the cache /// keeps capability requests cheap while still picking up newly (un)installed managers -/// after [`MANAGER_PROBE_TTL`]. -#[derive(Default)] -pub struct ManagerProbeCache(parking_lot::Mutex)>>); +/// after the TTL elapses. +pub struct ManagerProbeCache { + ttl: Duration, + entries: parking_lot::Mutex)>>, +} + +impl Default for ManagerProbeCache { + fn default() -> Self { + Self::with_ttl(MANAGER_PROBE_TTL) + } +} impl ManagerProbeCache { + fn with_ttl(ttl: Duration) -> Self { + Self { + ttl, + entries: parking_lot::Mutex::new(HashMap::new()), + } + } + fn get_fresh(&self, user_key: &str) -> Option> { - let cache = self.0.lock(); - cache + let entries = self.entries.lock(); + entries .get(user_key) - .filter(|(probed_at, _)| probed_at.elapsed() < MANAGER_PROBE_TTL) + .filter(|(probed_at, _)| probed_at.elapsed() < self.ttl) .map(|(_, managers)| managers.clone()) } fn insert(&self, user_key: String, managers: Vec) { - self.0.lock().insert(user_key, (Instant::now(), managers)); + self.entries.lock().insert(user_key, (Instant::now(), managers)); } } @@ -613,6 +628,13 @@ mod tests { } fn make_state(available: Vec) -> (Arc, Arc) { + make_state_with_cache(available, Default::default()) + } + + fn make_state_with_cache( + available: Vec, + manager_probe_cache: ManagerProbeCache, + ) -> (Arc, Arc) { let executor = Arc::new(FakeExecutor { available, probe_count: AtomicUsize::new(0), @@ -623,7 +645,7 @@ mod tests { pipe_name: "test-pipe".to_owned(), tracker: OperationTracker::new(), skip_signature_validation: true, - manager_probe_cache: Default::default(), + manager_probe_cache, }); (state, executor) } @@ -661,4 +683,16 @@ mod tests { assert_eq!(executor.probe_count.load(Ordering::SeqCst), 1); } + + #[tokio::test] + async fn manager_probe_is_refreshed_after_ttl_expiry() { + let (state, executor) = + make_state_with_cache(vec![ManagerName::Winget], ManagerProbeCache::with_ttl(Duration::ZERO)); + let sid = test_sid(); + + state.capabilities(&sid).await; + state.capabilities(&sid).await; + + assert_eq!(executor.probe_count.load(Ordering::SeqCst), 2); + } } From 0b0b6ce2f97b9064f7ef5e5e238edae5657a0707 Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 3 Aug 2026 16:17:43 +0300 Subject: [PATCH 4/4] refactor(agent): flatten probe blocking-task result handling Convert the JoinError into the same anyhow error path instead of matching on the nested result, mirroring the execute() pattern. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- devolutions-agent/src/broker/executor/windows/mod.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/devolutions-agent/src/broker/executor/windows/mod.rs b/devolutions-agent/src/broker/executor/windows/mod.rs index 3288c064c..96bfc9406 100644 --- a/devolutions-agent/src/broker/executor/windows/mod.rs +++ b/devolutions-agent/src/broker/executor/windows/mod.rs @@ -106,11 +106,13 @@ impl CommandExecutor for WindowsExecutor { .filter(|manager| manager_is_available(*manager, &user_env)) .collect()) }) - .await; + .await + .context("blocking task panicked") + .and_then(std::convert::identity); match probed { - Ok(Ok(managers)) => managers, - Ok(Err(error)) => { + Ok(managers) => managers, + Err(error) => { // Fail closed: the same session/environment lookup is required for execution, // so a manager that cannot be verified cannot run either. warn!( @@ -119,10 +121,6 @@ impl CommandExecutor for WindowsExecutor { ); Vec::new() } - Err(error) => { - warn!(%error, "Package manager availability probe panicked; advertising no managers"); - Vec::new() - } } } }