mirror of
https://github.com/openai/codex.git
synced 2026-11-12 21:20:34 +00:00
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
This commit is contained in:
@@ -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")?
|
||||
|
||||
@@ -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:?}")
|
||||
})
|
||||
|
||||
+28
-3
@@ -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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user