diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d8526760..211be235e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1422,7 +1422,7 @@ jobs: if ($LASTEXITCODE -ne 0 -or $actualCommit -ne $env:AGENT_POLICY_TEST_SHA) { throw "Agent policy tests must build the requested commit $env:AGENT_POLICY_TEST_SHA, got $actualCommit" } - cargo build --locked -p devolutions-agent --features dev-skip-broker-signature + cargo build --locked -p devolutions-agent if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } @@ -1453,10 +1453,6 @@ jobs: exit $exitCode } - - name: Run policy route authorization tests - shell: pwsh - run: cargo test --locked -p now-package-broker --features dev-skip-broker-signature - - name: Show sccache stats if: ${{ needs.preflight.outputs.sccache == 'true' && !cancelled() }} shell: pwsh diff --git a/crates/now-package-broker/Cargo.toml b/crates/now-package-broker/Cargo.toml index 337463431..036cd33e3 100644 --- a/crates/now-package-broker/Cargo.toml +++ b/crates/now-package-broker/Cargo.toml @@ -7,13 +7,6 @@ authors = ["Devolutions Inc. "] description = "Package broker for the Devolutions Agent" publish = false -[features] -default = [] -# Development-only feature allowing broker client signature validation to be skipped. -# Must never be enabled for shipped builds: without it, broker client signature validation is -# unconditionally enforced regardless of the configuration file contents. -dev-skip-broker-signature = [] - [lints] workspace = true diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index d647e750c..f9319b74f 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -209,7 +209,7 @@ impl PipeClient { Self::from_process_id(std::process::id()) } - #[cfg(all(test, feature = "dev-skip-broker-signature"))] + #[cfg(test)] pub(crate) fn test_with_authority(is_elevated: bool, is_administrator: bool) -> anyhow::Result { let mut client = Self::from_current_process()?; client.is_elevated = is_elevated; @@ -264,7 +264,7 @@ impl PipeClient { pub(crate) fn validate_connection(&self, skip_signature_validation: bool) -> anyhow::Result<()> { self.validate_process_instance()?; - if signature_validation_skipped(skip_signature_validation) { + if skip_signature_validation { warn!("DEBUG MODE: Skipping package broker client signature validation"); return Ok(()); } @@ -372,16 +372,6 @@ impl PipeClient { } } -/// Returns whether broker client signature validation is skipped. -/// -/// The bypass is compile-time gated behind the development-only `dev-skip-broker-signature` -/// cargo feature, which must never be enabled for shipped builds. Without the feature, this -/// always returns `false` and signature validation is unconditionally enforced, no matter -/// what the configuration says. -fn signature_validation_skipped(skip_signature_validation: bool) -> bool { - cfg!(feature = "dev-skip-broker-signature") && skip_signature_validation -} - fn connected_pipe_client_process_id(server: &NamedPipeServer) -> anyhow::Result { use std::os::windows::io::AsRawHandle as _; @@ -574,7 +564,6 @@ mod tests { .expect_err("an inherited pipe cannot outlive the authenticated process"); } - #[cfg(not(feature = "dev-skip-broker-signature"))] fn client_user_sid() -> Sid { system_client().user_sid } @@ -990,54 +979,58 @@ mod tests { ); } - #[cfg(not(feature = "dev-skip-broker-signature"))] - mod shipping_build { - use super::*; - - #[test] - fn signature_validation_is_never_skipped() { - assert!(!signature_validation_skipped(true)); - assert!(!signature_validation_skipped(false)); - } + #[test] + fn validate_connection_rejects_unsigned_client_by_default() { + // Retain the executable handle and a security guard so validation reaches the Authenticode + // check, which must reject the test binary because it is not Devolutions-signed. + let executable_path = std::env::current_exe().expect("current test executable path"); + let executable_file = File::open(&executable_path).expect("open current test executable"); + let client = PipeClient { + process_id: std::process::id(), + process_creation_time: SystemTime::UNIX_EPOCH, + process: None, + executable_path, + executable_file: Some(Arc::new(executable_file)), + executable_security: Some(Arc::new(RetainedExecutableSecurity::unchecked_for_tests())), + user_sid: client_user_sid(), + is_elevated: false, + is_administrator: false, + }; - #[test] - fn validate_signature_is_attempted_even_when_skip_is_requested() { - // The test binary is not Devolutions-signed, so validation must be attempted and fail - // even though the configuration requests skipping it. - let client = PipeClient { - process_id: std::process::id(), - process_creation_time: SystemTime::UNIX_EPOCH, - process: None, - executable_path: std::env::current_exe().expect("current test executable path"), - executable_file: None, - executable_security: None, - user_sid: client_user_sid(), - is_elevated: false, - is_administrator: false, - }; - - assert!(client.validate_connection(true).is_err()); - } + let error = client + .validate_connection(false) + .expect_err("an unsigned client must be rejected when the bypass is not requested"); + assert!( + format!("{error:#}").contains("executable signature is not valid"), + "unexpected validation error: {error:#}" + ); } - #[cfg(feature = "dev-skip-broker-signature")] - mod dev_build { - use super::*; - - #[test] - fn signature_bypass_does_not_disable_trusted_writer_security() { - let error = PipeClient::from_process_id_with_security(std::process::id()) - .expect_err("the user-writable test executable path must be rejected"); - assert!( - error.to_string().contains("trusted-writer security validation"), - "unexpected security error: {error:#}" - ); - } + #[test] + fn validate_connection_skips_signature_only_when_requested() { + // Without a retained handle or security guard, only the bypass lets validation succeed. + let client = PipeClient { + process_id: std::process::id(), + process_creation_time: SystemTime::UNIX_EPOCH, + process: None, + executable_path: std::env::current_exe().expect("current test executable path"), + executable_file: None, + executable_security: None, + user_sid: client_user_sid(), + is_elevated: false, + is_administrator: false, + }; - #[test] - fn signature_validation_is_skipped_only_when_requested() { - assert!(signature_validation_skipped(true)); - assert!(!signature_validation_skipped(false)); - } + assert!(client.validate_connection(true).is_ok()); + } + + #[test] + fn signature_bypass_does_not_disable_trusted_writer_security() { + let error = PipeClient::from_process_id_with_security(std::process::id()) + .expect_err("the user-writable test executable path must be rejected"); + assert!( + error.to_string().contains("trusted-writer security validation"), + "unexpected security error: {error:#}" + ); } } diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index f0e6140a7..4d4ba36bf 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -590,6 +590,16 @@ pub(crate) struct RetainedExecutableSecurity { _ancestor_handles: Vec, } +impl RetainedExecutableSecurity { + /// Guard that pins nothing, letting tests exercise checks performed after trusted-writer validation. + #[cfg(test)] + pub(crate) fn unchecked_for_tests() -> Self { + Self { + _ancestor_handles: Vec::new(), + } + } +} + /// Verify trusted-writer security for an already-retained executable and pin its ancestors. pub(crate) fn verify_retained_executable_security( file: &File, diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 6ace82de8..b2340e602 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -987,7 +987,6 @@ mod tests { assert!(!is_json_content_type(&headers)); } - #[cfg(feature = "dev-skip-broker-signature")] #[tokio::test] async fn policy_write_routes_reject_duplicate_members_before_draft_conversion() { let client = PipeClient::test_with_authority(true, true).expect("create elevated test client"); @@ -1070,7 +1069,6 @@ mod tests { } } - #[cfg(feature = "dev-skip-broker-signature")] async fn route_json( state: Arc, client: PipeClient, @@ -1170,15 +1168,10 @@ mod tests { #[tokio::test] async fn shared_router_exposes_policy_management_routes() { - let (management_status, body_status) = if cfg!(feature = "dev-skip-broker-signature") { - (StatusCode::OK, StatusCode::UNSUPPORTED_MEDIA_TYPE) - } else { - (StatusCode::UNAUTHORIZED, StatusCode::UNAUTHORIZED) - }; for (method, uri, expected_status) in [ - (Method::GET, "/v1/policy/management", management_status), - (Method::POST, "/v1/policy/validate", body_status), - (Method::PUT, "/v1/policy", body_status), + (Method::GET, "/v1/policy/management", StatusCode::OK), + (Method::POST, "/v1/policy/validate", StatusCode::UNSUPPORTED_MEDIA_TYPE), + (Method::PUT, "/v1/policy", StatusCode::UNSUPPORTED_MEDIA_TYPE), (Method::DELETE, "/v1/policy", StatusCode::METHOD_NOT_ALLOWED), ] { let response = route_request(shared_state(Some(permissive_policy())), method, uri).await; @@ -1186,7 +1179,6 @@ mod tests { } } - #[cfg(feature = "dev-skip-broker-signature")] #[tokio::test] async fn management_is_authenticated_but_only_elevated_administrators_can_write() { let unelevated = PipeClient::test_with_authority(false, false).expect("test client"); diff --git a/devolutions-agent/Cargo.toml b/devolutions-agent/Cargo.toml index bf7ed414e..a419217e5 100644 --- a/devolutions-agent/Cargo.toml +++ b/devolutions-agent/Cargo.toml @@ -8,13 +8,6 @@ description = "Agent companion service for Devolutions Gateway" build = "build.rs" publish = false -[features] -default = [] -# Development-only feature allowing the SkipBrokerSignatureValidation debug option to take effect. -# Must never be enabled for shipped builds: without it, broker client signature validation is -# unconditionally enforced regardless of the configuration file contents. -dev-skip-broker-signature = ["now-package-broker/dev-skip-broker-signature"] - [lints] workspace = true diff --git a/devolutions-agent/src/config.rs b/devolutions-agent/src/config.rs index fb04d203b..c4ea8eaa9 100644 --- a/devolutions-agent/src/config.rs +++ b/devolutions-agent/src/config.rs @@ -952,8 +952,6 @@ pub mod dto { /// Skip package broker client executable signature validation /// /// Useful for testing with unsigned or test-signed broker clients. - /// Only honored in builds with the development-only `dev-skip-broker-signature` - /// cargo feature; shipped builds always validate signatures and ignore this option. #[serde(default)] pub skip_broker_signature_validation: bool, } diff --git a/devolutions-agent/src/service.rs b/devolutions-agent/src/service.rs index 6739ea391..222fbfeae 100644 --- a/devolutions-agent/src/service.rs +++ b/devolutions-agent/src/service.rs @@ -220,12 +220,6 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { } if conf.package_broker.enabled { - if !cfg!(feature = "dev-skip-broker-signature") && conf.debug.skip_broker_signature_validation { - error!( - "SkipBrokerSignatureValidation is set but ignored: broker client signature validation cannot be disabled in this build" - ); - } - let broker_config = BrokerTaskConfig { pipe_name: conf .package_broker @@ -233,10 +227,7 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { .clone() .unwrap_or_else(|| DEFAULT_PIPE_NAME.to_owned()), policy_path: conf.package_broker.policy_path.clone(), - // The bypass only takes effect in builds with the development-only - // `dev-skip-broker-signature` cargo feature, never in shipped builds. - skip_signature_validation: cfg!(feature = "dev-skip-broker-signature") - && conf.debug.skip_broker_signature_validation, + skip_signature_validation: conf.debug.skip_broker_signature_validation, }; tasks.register(BrokerTask::new(broker_config)); }