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