fix(agent): stop identities only on a failed check

.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

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::<std::io::Error>() {
+        return error.kind() == ErrorKind::PermissionDenied;
+    }
+    #[cfg(windows)]
+    if let Some(error) = cause.downcast_ref::<windows::core::Error>() {
+        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<bool> {
-    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<File>) -> anyhow::Result<Option<File>> {
     match opened {
         Ok(file) => Ok(Some(file)),
@@ -226,10 +254,7 @@ fn classify_identity_file(path: &Utf8Path, opened: anyhow::Result<File>) -> 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::<UntrustedStoredIdentity>()),
+            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() {

devolutions-agent/src/identity/mod.rs

@@ -313,12 +313,38 @@ async fn run_pending_enrollments(
         ..
     } = shared;
     let mut retries: HashMap<Utf8PathBuf, RetryState> = 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::<UntrustedStoredIdentity>())
+            {
+                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<Arc<Shared>> {
+        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<Token> {
+        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::<UntrustedStoredIdentity>())),
+            "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