chore(agent): drop dev-skip-broker-signature cargo feature

.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

crates/now-package-broker/Cargo.toml

@@ -7,13 +7,6 @@ authors = ["Devolutions Inc. <infos@devolutions.net>"]
 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
 

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<Self> {
         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<u32> {
     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:#}"
+        );
     }
 }

crates/now-package-broker/src/policy_security.rs

@@ -590,6 +590,16 @@ pub(crate) struct RetainedExecutableSecurity {
     _ancestor_handles: Vec<File>,
 }
 
+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,

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<BrokerState>,
         client: PipeClient,
@@ -1170,23 +1168,17 @@ 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;
             assert_eq!(response.status(), expected_status, "{uri}");
         }
     }
 
-    #[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");

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
 

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,
     }

devolutions-agent/src/service.rs

@@ -220,23 +220,14 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result<TasksCtx> {
         }
 
         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
                     .pipe_name
                     .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));
         }