Grant permissions to RCC files to all users instead of specific users

We grant all users * read and execute access to the RCC binary * read access to the custom RCC profile (if configured) By not granting access to specific users, we don't have to revoke this access if the configuration changes.
elabit · Nov 27, 2024 · d4bd533 · d4bd533
1 parent bb4cec4
commit d4bd533
Show file tree

Hide file tree

Showing 2 changed files with 72 additions and 82 deletions.
diff --git a/src/bin/scheduler/setup/steps/rcc.rs b/src/bin/scheduler/setup/steps/rcc.rs
@@ -24,36 +24,27 @@ use tokio_util::sync::CancellationToken;
 #[cfg(windows)]
 struct StepFilePermissions {
     target: Utf8PathBuf,
-    session: Session,
+    sid: String,
     icacls_permissions: String,
 }
 
 #[cfg(windows)]
 impl SetupStep for StepFilePermissions {
     fn setup(&self) -> Result<(), api::Error> {
-        if let Session::User(user_session) = &self.session {
-            log::info!(
-                "Granting user `{user}` {permissions} access to {target}.",
-                user = &user_session.user_name,
-                permissions = &self.icacls_permissions,
-                target = &self.target,
-            );
-            run_icacls_command([
-                self.target.as_str(),
-                "/grant",
-                &format!("{}:{}", &user_session.user_name, self.icacls_permissions),
-            ])
-            .map_err(|err| {
-                api::Error::new(
-                    format!(
-                        "Adjusting permissions of {} for user `{}` failed",
-                        self.target, &user_session.user_name
-                    ),
-                    err,
-                )
-            })?;
-        }
-        Ok(())
+        run_icacls_command([
+            self.target.as_str(),
+            "/grant",
+            &format!("{}:{}", &self.sid, self.icacls_permissions),
+        ])
+        .map_err(|err| {
+            api::Error::new(
+                format!(
+                    "Adjusting permissions of {} for SID `{}` failed",
+                    self.target, &self.sid
+                ),
+                err,
+            )
+        })
     }
 }
 
@@ -253,46 +244,26 @@ pub fn gather_rcc_binary_permissions(
     config: &GlobalConfig,
     plans: Vec<Plan>,
 ) -> Vec<StepWithPlans> {
-    let (rcc_plans, system_plans): (Vec<Plan>, Vec<Plan>) =
-        partition_into_rcc_and_system_plans(plans);
-    let mut steps: Vec<StepWithPlans> = vec![skip(system_plans)];
-    for (session, plans_in_session) in plans_by_sessions(rcc_plans) {
-        steps.push((
-            Box::new(StepFilePermissions {
-                target: config.rcc_config.binary_path.clone(),
-                session,
-                icacls_permissions: "(RX)".to_string(),
-            }),
-            plans_in_session,
-        ));
-    }
-    steps
+    gather_file_permissions_for_users_group(
+        config.rcc_config.binary_path.clone(),
+        "(RX)".into(),
+        plans,
+    )
 }
 
 #[cfg(windows)]
 pub fn gather_rcc_profile_permissions(
     config: &GlobalConfig,
     plans: Vec<Plan>,
 ) -> Vec<StepWithPlans> {
-    let (rcc_plans, system_plans): (Vec<Plan>, Vec<Plan>) =
-        partition_into_rcc_and_system_plans(plans);
-    let mut steps: Vec<StepWithPlans> = vec![skip(system_plans)];
     match &config.rcc_config.profile_config {
-        RCCProfileConfig::Default => steps.push(skip(rcc_plans)),
-        RCCProfileConfig::Custom(custom_profile) => {
-            for (session, plans_in_session) in plans_by_sessions(rcc_plans) {
-                steps.push((
-                    Box::new(StepFilePermissions {
-                        target: custom_profile.path.clone(),
-                        session,
-                        icacls_permissions: "(R)".to_string(),
-                    }),
-                    plans_in_session,
-                ));
-            }
-        }
+        RCCProfileConfig::Default => vec![skip(plans)],
+        RCCProfileConfig::Custom(custom_profile) => gather_file_permissions_for_users_group(
+            custom_profile.path.clone(),
+            "(R)".into(),
+            plans,
+        ),
     }
-    steps
 }
 
 pub fn gather_disable_rcc_telemetry(config: &GlobalConfig, plans: Vec<Plan>) -> Vec<StepWithPlans> {
@@ -445,3 +416,33 @@ pub fn gather_disable_rcc_shared_holotree(
     }
     steps
 }
+
+#[cfg(windows)]
+fn gather_file_permissions_for_users_group(
+    target: Utf8PathBuf,
+    icacls_permissions: String,
+    plans: Vec<Plan>,
+) -> Vec<StepWithPlans> {
+    let (rcc_plans, system_plans): (Vec<Plan>, Vec<Plan>) =
+        partition_into_rcc_and_system_plans(plans);
+    let mut rcc_plans_in_current_session = vec![];
+    let mut rcc_plans_in_other_session = vec![];
+    for plan in rcc_plans {
+        match plan.session {
+            Session::Current(_) => rcc_plans_in_current_session.push(plan),
+            _ => rcc_plans_in_other_session.push(plan),
+        }
+    }
+    vec![
+        skip(system_plans),
+        skip(rcc_plans_in_current_session),
+        (
+            Box::new(StepFilePermissions {
+                target,
+                sid: "*S-1-5-32-545".to_string(), // Users (https://learn.microsoft.com/en-us/windows-server/identity/ad-ds/manage/understand-security-identifiers)
+                icacls_permissions,
+            }),
+            rcc_plans_in_other_session,
+        ),
+    ]
+}
diff --git a/tests/test_scheduler.rs b/tests/test_scheduler.rs
@@ -97,12 +97,7 @@ async fn test_scheduler() -> AnyhowResult<()> {
         &current_user_name,
     )
     .await?;
-    assert_rcc(
-        &config.rcc_config,
-        #[cfg(windows)]
-        &current_user_name,
-    )
-    .await?;
+    assert_rcc(&config.rcc_config).await?;
     #[cfg(windows)]
     assert_tasks().await?;
     assert_sequentiality(
@@ -547,6 +542,14 @@ async fn assert_permissions(path: impl AsRef<OsStr>, permissions: &str) -> Anyho
     Ok(())
 }
 
+#[cfg(windows)]
+async fn dacl_exists_for_sid(path: &Utf8Path, sid: &str) -> AnyhowResult<bool> {
+    Ok(run_icacls([path.as_str(), "/findsid", sid])
+        .await?
+        .status
+        .success())
+}
+
 fn assert_results_directory(results_directory: &Utf8Path) {
     assert!(results_directory.is_dir());
     assert_eq!(
@@ -580,12 +583,9 @@ async fn assert_managed_directory(
     Ok(())
 }
 
-async fn assert_rcc(
-    rcc_config: &RCCConfig,
-    #[cfg(windows)] headed_user_name: &str,
-) -> AnyhowResult<()> {
+async fn assert_rcc(rcc_config: &RCCConfig) -> AnyhowResult<()> {
     #[cfg(windows)]
-    assert_rcc_files_permissions(rcc_config, headed_user_name).await?;
+    assert_rcc_files_permissions(rcc_config).await?;
     assert_rcc_configuration(
         rcc_config,
         rcc_config
@@ -601,19 +601,13 @@ async fn assert_rcc(
 }
 
 #[cfg(windows)]
-async fn assert_rcc_files_permissions(
-    rcc_config: &RCCConfig,
-    headed_user_name: &str,
-) -> AnyhowResult<()> {
-    assert_permissions(&rcc_config.binary_path, &format!("{headed_user_name}:(RX)")).await?;
+async fn assert_rcc_files_permissions(rcc_config: &RCCConfig) -> AnyhowResult<()> {
+    assert!(dacl_exists_for_sid(&rcc_config.binary_path, "*S-1-5-32-544").await?);
     let RCCProfileConfig::Custom(custom_rcc_profile_config) = &rcc_config.profile_config else {
         return Ok(());
     };
-    assert_permissions(
-        &custom_rcc_profile_config.path,
-        &format!("{headed_user_name}:(R)"),
-    )
-    .await
+    assert!(dacl_exists_for_sid(&custom_rcc_profile_config.path, "*S-1-5-32-544").await?);
+    Ok(())
 }
 
 async fn assert_rcc_configuration(rcc_config: &RCCConfig, robocorp_home: &str) -> AnyhowResult<()> {
@@ -700,12 +694,7 @@ async fn assert_robocorp_home(
                 .len(),
             3 // Administrator group + empty line + success message (suppressing the latter with /q does not seem to work)
         );
-        assert!(
-            run_icacls([robocorp_home_base.as_str(), "/findsid", "*S-1-5-32-544"])
-                .await?
-                .status
-                .success()
-        );
+        assert!(dacl_exists_for_sid(robocorp_home_base, "*S-1-5-32-544").await?);
         assert!(
             get_permissions(robocorp_home_base.join(format!("user_{headed_user_name}")))
                 .await?