From d7840f9f22f330dd67e0a1638f66591ae221df36 Mon Sep 17 00:00:00 2001 From: zm-oai Date: Thu, 12 Nov 2026 00:13:31 +0000 Subject: [PATCH] Reconcile Windows runtime registration after service privilege errors (#60944) ## Why Windows can report a service privilege error after another deployment has registered the same runtime package version for the target account, causing provisioning to fail despite that registration. ## What changed Accept `ERROR_PACKAGED_SERVICE_REQUIRES_ADMIN_PRIVILEGES` from a registration operation with `AsyncStatus::Error` only when both checks pass: - The execution alias exists as a reparse point when queried as the target user. - `FindUsers` reports the exact package as installed for the target SID. ## Testing Add a regression test covering successful reconciliation, missing target registration, cancellation, and unrelated access-denied errors. Verify that the registration check runs only for the matching status and deployment error. GitOrigin-RevId: 5ccf0186046b1d681777d7caa083695b86fda1ca --- .../src/registered_runtime.rs | 33 ++++++++++++++++--- .../registered_runtime/registration_result.rs | 14 +++++++- .../registration_result_tests.rs | 31 +++++++++++++++-- 3 files changed, 69 insertions(+), 9 deletions(-) diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime.rs b/codex-rs/windows-sandbox-service/src/registered_runtime.rs index 5bb8cfdb7f..e46eca7f0f 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime.rs @@ -2,6 +2,7 @@ //! Lifecycle and admission decisions stay with callers. Persist ownership before deployment; //! retain profiles until Windows completes, or pending resources until service exit on shutdown. +use std::os::windows::fs::MetadataExt; use std::os::windows::io::AsRawHandle; use std::os::windows::io::OwnedHandle; use std::sync::atomic::AtomicBool; @@ -143,13 +144,13 @@ pub(crate) fn provision( cleanup_logon_pending: false, account, user_sid: profile.user_sid.clone(), - alias_path: Some(alias_path), + alias_path: Some(alias_path.clone()), }); - profiles.push((profile, installed.contains(full_name))); + profiles.push((profile, installed.contains(full_name), alias_path)); } let receipt_current = record.runtime()?.ready_for_package(&full_name.to_string()) && record.runtime()?.accounts == accounts - && profiles.iter().all(|(_, installed)| *installed); + && profiles.iter().all(|(_, installed, _)| *installed); // Both exact SIDs are durable before either package registration or metadata grant. // An idempotent setup check must not temporarily revoke a valid launch receipt. record.runtime_mut()?.accounts = accounts; @@ -158,7 +159,7 @@ pub(crate) fn provision( crate::installation_record::save_runtime(record) .context("persist managed runtime account registration")?; } - for (mut profile, installed) in profiles { + for (mut profile, installed, alias_path) in profiles { if !installed { let operation = with_owner_impersonation(profile.token.as_raw_handle() as _, || { Ok(PackageManager::new()?.RegisterPackageByFullNameAsync( @@ -201,7 +202,29 @@ pub(crate) fn provision( } std::thread::sleep(Duration::from_millis(50)); }; - registration_result::RegistrationResult::read(&operation, status).check()?; + registration_result::RegistrationResult::read(&operation, status).check(|| { + // Look up the generated execution alias as its intended user. The + // command path independently verifies the spawned image and identity. + let alias_present = with_owner_impersonation(profile.token.as_raw_handle(), || { + Ok(std::fs::symlink_metadata(&alias_path)?.file_attributes() + & windows_sys::Win32::Storage::FileSystem::FILE_ATTRIBUTE_REPARSE_POINT + != 0) + }); + if !matches!(alias_present, Ok(true)) { + return false; + } + // FindUsers distinguishes a staged package from a registration for this SID. + PackageManager::new() + .and_then(|manager| manager.FindUsers(full_name)) + .is_ok_and(|users| { + users.into_iter().any(|user| { + user.UserSecurityId().is_ok_and(|sid| sid == profile.user_sid.as_str()) + && user.InstallState().is_ok_and(|state| { + state == windows::Management::Deployment::PackageInstallState::Installed + }) + }) + }) + })?; ensure!( registered_packages(&profile.user_sid, &family) .context("verify managed runtime package registration")? diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result.rs b/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result.rs index e186b4b34a..3f077f3244 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result.rs @@ -39,7 +39,7 @@ impl RegistrationResult { } } - pub(super) fn check(self) -> Result<()> { + pub(super) fn check(self, registered_for_target: impl FnOnce() -> bool) -> Result<()> { let Self { status, async_error, @@ -69,6 +69,18 @@ impl RegistrationResult { ); anyhow::Error::new(error).context(summary) })?; + // Windows can report the service privilege error after another deployment + // registered this same version for the target account. Trust that exact + // postcondition only for this error. The runner still verifies its image + // and package identity before accepting a command. + if status == AsyncStatus::Error + && extended_error == HRESULT::from_win32( + windows_sys::Win32::Foundation::ERROR_PACKAGED_SERVICE_REQUIRES_ADMIN_PRIVILEGES, + ) + && registered_for_target() + { + return Ok(()); + } extended_error.ok().with_context(|| { format!("{context} phase=deployment extended_error={extended_error:?}") }) diff --git a/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result_tests.rs b/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result_tests.rs index 5586b1fe6c..8d1158e712 100644 --- a/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result_tests.rs +++ b/codex-rs/windows-sandbox-service/src/registered_runtime/registration_result_tests.rs @@ -19,7 +19,7 @@ fn retains_different_async_and_deployment_errors_with_activity() { activity_id: Ok(activity_id), }), } - .check() + .check(|| false) .unwrap_err(); assert_eq!( error.to_string(), @@ -44,7 +44,7 @@ fn get_results_errors_keep_original_hresult_and_phase() { async_error: Ok(code), result: Err(windows::core::Error::from_hresult(code)), } - .check() + .check(|| false) .unwrap_err(); assert_eq!( error.to_string(), @@ -72,7 +72,7 @@ fn diagnostic_getter_failures_do_not_mask_completion_or_leak_text() { activity_id: Err(private_error()), }), } - .check(); + .check(|| false); if code.is_err() { let error = result.unwrap_err(); assert_eq!( @@ -91,3 +91,28 @@ fn diagnostic_getter_failures_do_not_mask_completion_or_leak_text() { } } } + +#[test] +fn only_reconciles_a_terminal_service_privilege_error_for_the_registered_target() { + for (status, code, registered, expected) in [ + (AsyncStatus::Error, DEPLOYMENT_FAILURE, true, true), + (AsyncStatus::Error, DEPLOYMENT_FAILURE, false, false), + (AsyncStatus::Canceled, DEPLOYMENT_FAILURE, true, false), + (AsyncStatus::Error, ACCESS_DENIED, true, false), + ] { + let completion = RegistrationResult { + status, + async_error: Ok(ASYNC_FAILURE), + result: Ok(DeploymentDetails { + extended_error: Ok(code), + activity_id: Ok(GUID::zeroed()), + }), + }; + let result = completion.check(|| { + assert_eq!(status, AsyncStatus::Error); + assert_eq!(code, DEPLOYMENT_FAILURE); + registered + }); + assert_eq!(result.is_ok(), expected); + } +}