From 06c5a3461d379a40ae61e58810947fcb5e2162f2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 04:04:45 +0900 Subject: [PATCH] fix(agent): stop identities only on a failed check The pending-enrollment poll stopped every identity runner and its agent channel streams whenever the stored-identity check returned any error, and logged nothing. An I/O error that only kept the check from running therefore restarted all identity processing, for example in the middle of a renewal, which can reorder its confirm, new channel authentication and old stream close. The check now reports an authority directory or identity.json as untrusted only when it fails the check itself or denies the agent access; any other I/O error stays an ordinary error. The poll stops every identity only for an untrusted stored identity and otherwise retries at the next poll, logging the error in both cases. The check still runs without the state lock, since it changes nothing and every state change repeats it under the lock. When the tests job fails, CI now uploads the Agent Identity conformance directories the suite keeps, so that a failure seen once leaves its agent data directories and logs. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 29 +++++ crates/agent-identity/src/private_file.rs | 86 +++++++++++--- devolutions-agent/src/identity/mod.rs | 134 +++++++++++++++++++--- 3 files changed, 218 insertions(+), 31 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8013a3fdc..d0db87cb3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -330,6 +330,35 @@ jobs: env: AWS_LC_SYS_NO_ASM: true + # A failed Agent Identity conformance run keeps its work directory, whose agent data directories and logs are the + # only record of the state that made it fail. + # The filter matches that directory under `target`, the temporary directory or `ProgramData`, and a file the runner + # cannot read, such as one only SYSTEM can open, is reported and skipped. + # UNTESTED: these steps run only after a failure; when changing them, check by hand with a failing conformance test + # that the artifact holds the kept agent data directories. + - name: Collect kept Agent Identity conformance directories + if: ${{ failure() }} + shell: pwsh + run: | + $Destination = Join-Path $Env:RUNNER_TEMP 'agent-identity-conformance' + New-Item -ItemType Directory -Force $Destination | Out-Null + $Parents = @('target', [IO.Path]::GetTempPath(), $Env:ProgramData) | Where-Object { $_ } + $Index = 0 + Get-ChildItem -Path $Parents -Directory -Filter 'agent-identity-integration*' -ErrorAction SilentlyContinue | + ForEach-Object { + $Index++ + $Copy = Join-Path $Destination "$Index-$($_.Name)" + Copy-Item -LiteralPath $_.FullName -Destination $Copy -Recurse -ErrorAction Continue + } + + - name: Upload kept Agent Identity conformance directories + if: ${{ failure() }} + uses: actions/upload-artifact@v7 + with: + name: agent-identity-conformance-${{ matrix.os }}-${{ matrix.arch }}-attempt-${{ github.run_attempt }} + path: ${{ runner.temp }}/agent-identity-conformance + if-no-files-found: ignore + - name: Timing-sensitive tests run: | cargo test --locked --target ${{ matrix.target }} -p testsuite --test integration_tests timing:: -- --ignored --test-threads=1 diff --git a/crates/agent-identity/src/private_file.rs b/crates/agent-identity/src/private_file.rs index a5a24eae5..6c75a6304 100644 --- a/crates/agent-identity/src/private_file.rs +++ b/crates/agent-identity/src/private_file.rs @@ -52,6 +52,39 @@ fn untrusted() -> anyhow::Error { anyhow::Error::new(UntrustedStateFile) } +/// Turns a failed check of the authority directory or `identity.json` at `path` into an [`UntrustedStoredIdentity`] when +/// it fails the trust check, and leaves any other failure, which only kept the check from running, an ordinary error. +/// +/// [`crates/agent-identity/INTENT.md`] ("Local state"): only a stored identity that fails the check stops all identity +/// processing. +/// +/// [`crates/agent-identity/INTENT.md`]: ../INTENT.md +fn check_failure(path: &Utf8Path, error: anyhow::Error) -> anyhow::Error { + if fails_trust_check(&error) { + anyhow::Error::new(UntrustedStoredIdentity { + path: path.to_owned(), + source: error, + }) + } else { + error.context(format!("check stored identity {path}")) + } +} + +/// Reports whether a failed check found a stored identity untrusted, rather than an I/O error keeping it from running. +/// +/// Denied access counts as untrusted, because the agent never closes its own state to itself. +fn fails_trust_check(error: &anyhow::Error) -> bool { + let cause = error.root_cause(); + if let Some(error) = cause.downcast_ref::() { + return error.kind() == ErrorKind::PermissionDenied; + } + #[cfg(windows)] + if let Some(error) = cause.downcast_ref::() { + return error.code() == windows::Win32::Foundation::ERROR_ACCESS_DENIED.to_hresult(); + } + true +} + struct TemporaryFile(Utf8PathBuf); impl Drop for TemporaryFile { @@ -138,22 +171,17 @@ pub(crate) fn check_stored_identities(data_dir: &Utf8Path, grant_current_user: b /// Checks, without changing it, that an authority directory is owned by the agent's account and protected from other /// users, and returns whether it exists. pub(crate) fn check_authority_directory(path: &Utf8Path, grant_current_user: bool) -> anyhow::Result { - let untrusted = |source| { - anyhow::Error::new(UntrustedStoredIdentity { - path: path.to_owned(), - source, - }) - }; let metadata = match fs::symlink_metadata(path) { Ok(metadata) => metadata, Err(error) if error.kind() == ErrorKind::NotFound => return Ok(false), Err(error) => { - return Err(untrusted( + return Err(check_failure( + path, anyhow::Error::new(error).context("inspect authority directory"), )); } }; - verify_authority_directory(path, &metadata, grant_current_user).map_err(untrusted)?; + verify_authority_directory(path, &metadata, grant_current_user).map_err(|error| check_failure(path, error))?; Ok(true) } @@ -215,7 +243,7 @@ pub(crate) fn check_identity_file(path: &Utf8Path, grant_current_user: bool) -> classify_identity_file(path, open_trusted_identity_file(path, grant_current_user, false)).map(drop) } -/// Turns a failure to open a trusted `identity.json` into an absent file, or else into an [`UntrustedStoredIdentity`]. +/// Turns a failure to open a trusted `identity.json` into an absent file, or else as [`check_failure`] does. fn classify_identity_file(path: &Utf8Path, opened: anyhow::Result) -> anyhow::Result> { match opened { Ok(file) => Ok(Some(file)), @@ -226,10 +254,7 @@ fn classify_identity_file(path: &Utf8Path, opened: anyhow::Result) -> anyh { Ok(None) } - Err(source) => Err(anyhow::Error::new(UntrustedStoredIdentity { - path: path.to_owned(), - source, - })), + Err(error) => Err(check_failure(path, error)), } } @@ -618,6 +643,9 @@ fn sync_directory(path: &Utf8Path) -> anyhow::Result<()> { #[cfg(test)] mod tests { + #[cfg(windows)] + use windows::Win32::Foundation::{ERROR_ACCESS_DENIED, ERROR_NOT_ENOUGH_MEMORY, WIN32_ERROR}; + use super::*; // oracle: intent crates/agent-identity/INTENT.md §Pending-enrollment files @@ -654,6 +682,38 @@ mod tests { assert!(verify_unix_directory_access(0o700, agent, agent).is_ok()); } + // oracle: intent crates/agent-identity/INTENT.md §Local state + #[rstest::rstest] + #[case::check_finding(anyhow::anyhow!("authority directory is open to other users"), true)] + #[case::denied_access(failed_open(ErrorKind::PermissionDenied), true)] + #[case::file_in_use(failed_open(ErrorKind::ResourceBusy), false)] + #[case::interrupted_call(failed_open(ErrorKind::Interrupted), false)] + #[cfg_attr(windows, case::denied_security_read(failed_security_read(ERROR_ACCESS_DENIED), true))] + #[cfg_attr(windows, case::out_of_memory(failed_security_read(ERROR_NOT_ENOUGH_MEMORY), false))] + fn only_a_failed_check_makes_an_untrusted_stored_identity(#[case] error: anyhow::Error, #[case] untrusted: bool) { + // Only an authority directory or `identity.json` that fails the check stops all identity processing, so an I/O + // error that only kept the check from running is not an untrusted stored identity. + // Denied access is one, because the state the agent provisions never denies the agent's account. + let path = Utf8Path::new("identity/authorities/00000000-0000-0000-0000-000000000000"); + + let error = check_failure(path, error); + + assert_eq!( + error.chain().any(|cause| cause.is::()), + untrusted, + "{error:#}" + ); + } + + fn failed_open(kind: ErrorKind) -> anyhow::Error { + anyhow::Error::new(std::io::Error::from(kind)).context("open authority directory") + } + + #[cfg(windows)] + fn failed_security_read(code: WIN32_ERROR) -> anyhow::Error { + anyhow::Error::new(windows::core::Error::from_hresult(code.to_hresult())).context("inspect identity file DACL") + } + // oracle: intent crates/agent-identity/INTENT.md §Local state #[test] fn recognizes_only_own_temporary_file_names() { diff --git a/devolutions-agent/src/identity/mod.rs b/devolutions-agent/src/identity/mod.rs index 7da5cfe00..d175aef20 100644 --- a/devolutions-agent/src/identity/mod.rs +++ b/devolutions-agent/src/identity/mod.rs @@ -313,12 +313,38 @@ async fn run_pending_enrollments( .. } = shared; let mut retries: HashMap = HashMap::new(); + let mut last_check_error = None; let mut last_list_error = None; loop { // `crates/agent-identity/INTENT.md` ("Local state"): an authority directory or `identity.json` that fails its // check stops all identity processing; start-up reports it and waits until it is fixed. - if store.check_stored_identities().is_err() { - return Stopped::Untrusted; + // Any other error only kept the check from running, so the poll retries it rather than stop every runner and its + // streams. + // UNTESTED: both outcomes are logged with the error, because stopping every identity is otherwise unexplained + // and a check that keeps failing otherwise holds pending enrollment silently. + // No test captures service logs; when changing this, keep the error in both logs. + if let Err(error) = store.check_stored_identities() { + if let Some(untrusted) = error + .chain() + .find_map(|cause| cause.downcast_ref::()) + { + warn!( + error = format!("{error:#}"), + path = %untrusted.path, + "Stopping every Agent Identity because a stored identity failed its check" + ); + return Stopped::Untrusted; + } + if log_is_due(&mut last_check_error) { + warn!( + error = format!("{error:#}"), + "Cannot check the stored Agent Identities; retrying at the next poll" + ); + } + if wait_or_stop(shutdown, poll_interval).await { + return Stopped::Shutdown; + } + continue; } match pending::list(data_dir) { Ok(paths) => { @@ -630,22 +656,37 @@ mod tests { }))?) } + fn test_shared(data_dir: &TestDataDir) -> anyhow::Result> { + let conf = test_conf()?; + let settings = conf.debug.identity.clone().unwrap_or_default(); + Ok(Arc::new(Shared { + store: data_dir.store()?, + client: Client::new(conf.proxy.to_proxy_config(), None)?, + channels: channel_client(&conf, &settings)?, + settings, + })) + } + + /// Returns a token for a loopback authority that refuses connections, which keeps an enrollment local. + fn loopback_token() -> anyhow::Result { + use base64::Engine as _; + + Token::parse(&format!( + "dvaet1.{}.{}", + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(r#"{"u":"https://127.0.0.1:9"}"#), + "A".repeat(43) + )) + } + #[tokio::test] async fn an_untrusted_authority_directory_holds_pending_enrollment_until_it_is_fixed() -> anyhow::Result<()> { // oracle: intent crates/agent-identity/INTENT.md §Local state // An authority directory that fails the check stops all identity processing until it is fixed, so no pending // file is processed meanwhile, and processing resumes without a restart. - use base64::Engine as _; - let data_dir = TestDataDir::new()?; drop(data_dir.store()?); let untrusted = data_dir.put_untrusted_authority_directory()?; - // A loopback authority that refuses connections keeps the enrollment local. - let token = Token::parse(&format!( - "dvaet1.{}.{}", - base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(r#"{"u":"https://127.0.0.1:9"}"#), - "A".repeat(43) - ))?; + let token = loopback_token()?; pending::write(&data_dir.0, &token, true)?; let in_progress = pending::in_progress_path(&data_dir.0, &token.sha256_hex())?; let (shutdown_handle, shutdown) = devolutions_gateway_task::ShutdownHandle::new(); @@ -686,14 +727,7 @@ mod tests { // A stored identity that fails the check after start-up stops all identity processing too; the caller then // stops every runner and its streams. let data_dir = TestDataDir::new()?; - let conf = test_conf()?; - let settings = conf.debug.identity.clone().unwrap_or_default(); - let shared = Arc::new(Shared { - store: data_dir.store()?, - client: Client::new(conf.proxy.to_proxy_config(), None)?, - channels: channel_client(&conf, &settings)?, - settings, - }); + let shared = test_shared(&data_dir)?; let (_shutdown_handle, mut shutdown) = devolutions_gateway_task::ShutdownHandle::new(); let mut runners = Runners::new(Arc::clone(&shared), shutdown.clone()); put_untrusted(&data_dir)?; @@ -715,6 +749,70 @@ mod tests { Ok(()) } + #[tokio::test] + async fn a_check_that_cannot_run_neither_stops_the_pending_poll_nor_holds_enrollment_once_it_runs_again() + -> anyhow::Result<()> { + // oracle: intent crates/agent-identity/INTENT.md §Local state + // Only an authority directory or `identity.json` that fails the check stops all identity processing. + // An error that keeps the check from running, here listing the authority directories, is not such a failure, + // so the poll keeps running, which keeps its caller from stopping every runner and its streams, and processes + // pending files again once the check runs. + let data_dir = TestDataDir::new()?; + let shared = test_shared(&data_dir)?; + let (_shutdown_handle, mut shutdown) = devolutions_gateway_task::ShutdownHandle::new(); + let mut runners = Runners::new(Arc::clone(&shared), shutdown.clone()); + let token = loopback_token()?; + pending::write(&data_dir.0, &token, true)?; + let in_progress = pending::in_progress_path(&data_dir.0, &token.sha256_hex())?; + let authorities = data_dir.0.join("identity").join("authorities"); + std::fs::remove_dir(&authorities)?; + std::fs::write(&authorities, b"")?; + let check = shared.store.check_stored_identities(); + assert!( + check + .as_ref() + .is_err_and(|error| !error.chain().any(|cause| cause.is::())), + "listing a file as the authority directories must fail without an untrusted identity: {check:?}" + ); + + let poll = run_pending_enrollments( + &shared, + &mut runners, + &data_dir.0, + Duration::from_millis(10), + &mut shutdown, + ); + tokio::pin!(poll); + let stopped_while_failing = tokio::select! { + _ = &mut poll => true, + _ = sleep(Duration::from_millis(500)) => false, + }; + // A poll that returned cannot be polled again, so this is checked before the check runs again. + assert!( + !stopped_while_failing, + "the pending poll stopped while the check could not run" + ); + std::fs::remove_file(&authorities)?; + let resumed = tokio::time::timeout(Duration::from_secs(10), async { + tokio::select! { + _ = &mut poll => false, + _ = async { + while !in_progress.exists() { + sleep(Duration::from_millis(10)).await; + } + } => true, + } + }) + .await; + + assert_eq!( + resumed.ok(), + Some(true), + "the pending poll did not process its pending file once the check ran again" + ); + Ok(()) + } + #[test] fn the_n_th_consecutive_failure_waits_2_to_the_n_seconds_times_jitter_then_at_most_the_maximum() { // oracle: intent crates/agent-identity/INTENT.md §Schedules