fix(agent): systematic failure recovery for agent tunnel enrollment (#1802)

## Summary

Agent tunnel enrollment spans two non-transactional write phases — the
agent's `up` (Rust: client cert/key + the fixed-name `gateway-ca.pem` +
`agent.json`) and the MSI custom action (advertise subnets/domains +
rollback bookkeeping). A failure in either phase left the machine in a
partial state: orphaned cert files, a clobbered `gateway-ca.pem`, or a
half-written `agent.json` that even a rollback couldn't recover from.

This makes the whole enrollment **recoverable end to end** — every
failure path leaves the machine exactly as it was before enroll.

## Agent (Rust)

- `persist_enrollment_response` is now transactional: load/validate the
config **before** any write (a corrupt `agent.json` fails before
touching disk), back up the fixed-name `gateway-ca.pem`, and roll back
partial cert/CA writes on any failure.
- `save_config` creates its parent directory (fixes fresh standalone
`agent.exe up` on a clean machine) and writes **atomically** (temp +
rename) so a mid-write failure never truncates `agent.json`.

## Installer (C#)

- `EnrollAgentTunnel` snapshots the pre-enrollment `Tunnel` section and
`gateway-ca.pem` into a per-install **rollback marker**
(`%TEMP%\{installId}-tunnel-rollback.json`), written atomically. The
marker is **required**: if it can't be recorded, the enrollment is
undone inline and the CA fails.
- New marker-driven `RollbackEnrollAgentTunnel` (`Execute.rollback`)
only cleans up / restores when *this* install recorded a marker — so it
never touches pre-existing or partial state. It restores the original
`Tunnel` section and `gateway-ca.pem`, and deletes the certs this
install wrote.
- All `agent.json` writes go through an atomic temp-replace helper, so
the rollback can always re-parse it.
- Drains stdout/stderr concurrently with `WaitForExit` (fixes a
pipe-buffer deadlock that could kill a healthy `up`) and fails loudly
when operator-supplied advertise subnets/domains can't be persisted
(instead of silently dropping them).

## Failure-recovery matrix

| Failure point | Recovery |
|---|---|
| Empty / pre-write failure | No marker → rollback no-op; nothing was
written |
| `up` fails mid-write (bad json / save error) | Rust self-rolls-back
its partial writes → non-zero exit → no marker |
| `up` ok, marker write fails | Inline cleanup + CA fails |
| `up` ok, advertisements write fails | Marker present → rollback
restores |
| Later MSI action fails after success | Marker present → rollback
restores |

## Test

- `cargo check -p devolutions-agent` — clean.
- `dotnet build DevolutionsAgent.csproj -c Debug` — 0 errors (8
pre-existing WiX CNDL warnings), MSI builds.

Changelog: ignore

Author: irvingoujAtDevolution

devolutions-agent/src/config.rs

@@ -237,7 +237,25 @@ impl ConfHandle {
 pub fn save_config(conf: &dto::ConfFile) -> anyhow::Result<()> {
     let conf_file_path = get_conf_file_path();
     let json = serde_json::to_string_pretty(conf).context("failed JSON serialization of configuration")?;
-    std::fs::write(&conf_file_path, json).with_context(|| format!("failed to write file at {conf_file_path}"))?;
+
+    // Ensure the parent directory exists — a fresh machine (standalone `agent.exe up`, no MSI) may
+    // not have the data directory yet, and a bare write would fail before any config is created.
+    if let Some(parent) = conf_file_path.parent().filter(|parent| !parent.as_str().is_empty()) {
+        std::fs::create_dir_all(parent)
+            .with_context(|| format!("failed to create configuration directory: {parent}"))?;
+    }
+
+    // Write atomically: serialize to a sibling temp file, then rename over the target. A failure
+    // mid-write therefore never truncates or corrupts an existing agent.json — the original stays
+    // intact until the rename atomically replaces it (std::fs::rename replaces on Windows too).
+    let tmp_path = Utf8PathBuf::from(format!("{conf_file_path}.tmp"));
+    std::fs::write(&tmp_path, json).with_context(|| format!("failed to write temporary config at {tmp_path}"))?;
+    if let Err(e) = std::fs::rename(&tmp_path, &conf_file_path) {
+        // Rename failed: don't leave the temp file lingering next to agent.json.
+        let _ = std::fs::remove_file(&tmp_path);
+        return Err(anyhow::Error::new(e).context(format!("failed to replace config at {conf_file_path}")));
+    }
+
     Ok(())
 }
 

devolutions-agent/src/enrollment.rs

@@ -181,59 +181,106 @@ fn persist_enrollment_response(
         .unwrap_or_else(|| Utf8PathBuf::from("."));
     let cert_dir = config_dir.join("certs");
 
+    // Load (and validate) the existing config BEFORE writing anything to disk. A corrupt
+    // agent.json must fail the enrollment here — while nothing has been written yet — rather than
+    // after we've already overwritten the fixed-name gateway-ca.pem and dropped the new cert/key.
+    // This preserves other settings (Updater, Session, PEDM, etc.) configured by the MSI installer
+    // or admin.
+    let mut conf_file = config::load_conf_file_or_generate_new().context("failed to load existing configuration")?;
+
     std::fs::create_dir_all(&cert_dir)
         .with_context(|| format!("failed to create certificate directory: {}", cert_dir))?;
 
     let client_cert_path = cert_dir.join(format!("{agent_id}-cert.pem"));
     let client_key_path = cert_dir.join(format!("{agent_id}-key.pem"));
     let gateway_ca_path = cert_dir.join("gateway-ca.pem");
 
-    // Write the locally-generated private key first (before cert/CA from the network).
-    std::fs::write(&client_key_path, key_pem)
-        .with_context(|| format!("failed to write client private key: {client_key_path}"))?;
+    // gateway-ca.pem is a fixed filename we are about to overwrite. Back up any existing copy so a
+    // later failure can restore it. The {agent_id}-prefixed cert/key files are uniquely named, so
+    // a rollback just removes them.
+    let gateway_ca_backup = if gateway_ca_path.exists() {
+        Some(
+            std::fs::read(&gateway_ca_path)
+                .with_context(|| format!("failed to back up existing gateway CA certificate: {gateway_ca_path}"))?,
+        )
+    } else {
+        None
+    };
 
-    std::fs::write(&client_cert_path, &client_cert_pem)
-        .with_context(|| format!("failed to write client certificate: {client_cert_path}"))?;
+    // All the destructive writes happen inside this single fallible step. `up` is otherwise
+    // non-transactional: if it wrote the cert files (and clobbered gateway-ca.pem) and then failed
+    // at config save, a non-zero exit would leave partial, orphaned state behind. On any failure we
+    // roll that partial state back below so the machine is left exactly as it was before enroll.
+    let persist = || -> Result<()> {
+        // Write the locally-generated private key first (before cert/CA from the network).
+        std::fs::write(&client_key_path, key_pem)
+            .with_context(|| format!("failed to write client private key: {client_key_path}"))?;
+
+        std::fs::write(&client_cert_path, &client_cert_pem)
+            .with_context(|| format!("failed to write client certificate: {client_cert_path}"))?;
+
+        std::fs::write(&gateway_ca_path, &gateway_ca_cert_pem)
+            .with_context(|| format!("failed to write gateway CA certificate: {gateway_ca_path}"))?;
+
+        // Restrict permissions on cert/key files (owner-only on Unix).
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::PermissionsExt as _;
+            let restricted = std::fs::Permissions::from_mode(0o600);
+            for path in [&client_cert_path, &client_key_path, &gateway_ca_path] {
+                std::fs::set_permissions(path, restricted.clone())
+                    .with_context(|| format!("failed to set permissions on {path}"))?;
+            }
+        }
 
-    std::fs::write(&gateway_ca_path, &gateway_ca_cert_pem)
-        .with_context(|| format!("failed to write gateway CA certificate: {gateway_ca_path}"))?;
+        // Preserve existing domain config from previous enrollment/manual configuration.
+        let existing_tunnel = conf_file.tunnel.as_ref();
 
-    // Restrict permissions on cert/key files (owner-only on Unix).
-    #[cfg(unix)]
-    {
-        use std::os::unix::fs::PermissionsExt as _;
-        let restricted = std::fs::Permissions::from_mode(0o600);
-        for path in [&client_cert_path, &client_key_path, &gateway_ca_path] {
-            std::fs::set_permissions(path, restricted.clone())
-                .with_context(|| format!("failed to set permissions on {path}"))?;
-        }
-    }
+        let tunnel_conf = config::dto::TunnelConf {
+            enabled: true,
+            gateway_endpoint: quic_endpoint.clone(),
+            client_cert_path: Some(client_cert_path.clone()),
+            client_key_path: Some(client_key_path.clone()),
+            gateway_ca_cert_path: Some(gateway_ca_path.clone()),
+            advertise_subnets,
+            advertise_domains: existing_tunnel.map(|t| t.advertise_domains.clone()).unwrap_or_default(),
+            auto_detect_domain: existing_tunnel.map(|t| t.auto_detect_domain).unwrap_or(true),
+            heartbeat_interval_secs: Some(60),
+            route_advertise_interval_secs: Some(30),
+            server_spki_sha256: Some(server_spki_sha256),
+        };
 
-    // Load existing config and update only the Tunnel section.
-    // This preserves other settings (Updater, Session, PEDM, etc.) that may have been
-    // configured by the MSI installer or admin.
-    let mut conf_file = config::load_conf_file_or_generate_new().context("failed to load existing configuration")?;
+        conf_file.tunnel = Some(tunnel_conf);
 
-    // Preserve existing domain config from previous enrollment/manual configuration.
-    let existing_tunnel = conf_file.tunnel.as_ref();
-
-    let tunnel_conf = config::dto::TunnelConf {
-        enabled: true,
-        gateway_endpoint: quic_endpoint.clone(),
-        client_cert_path: Some(client_cert_path.clone()),
-        client_key_path: Some(client_key_path.clone()),
-        gateway_ca_cert_path: Some(gateway_ca_path.clone()),
-        advertise_subnets,
-        advertise_domains: existing_tunnel.map(|t| t.advertise_domains.clone()).unwrap_or_default(),
-        auto_detect_domain: existing_tunnel.map(|t| t.auto_detect_domain).unwrap_or(true),
-        heartbeat_interval_secs: Some(60),
-        route_advertise_interval_secs: Some(30),
-        server_spki_sha256: Some(server_spki_sha256),
-    };
+        config::save_config(&conf_file)?;
 
-    conf_file.tunnel = Some(tunnel_conf);
+        Ok(())
+    };
 
-    config::save_config(&conf_file)?;
+    if let Err(error) = persist() {
+        // Roll back the partial writes: remove the uniquely-named cert/key, and restore (or remove)
+        // the overwritten fixed-name gateway-ca.pem so a failed enroll never destroys or orphans state.
+        let _ = std::fs::remove_file(&client_key_path);
+        let _ = std::fs::remove_file(&client_cert_path);
+        match &gateway_ca_backup {
+            Some(original) => {
+                // Atomic restore: write to a sibling temp then rename over the target so a failed
+                // restore mid-write can't truncate the previous CA. Best-effort like the rest of
+                // this rollback block; clean up the temp if either step fails.
+                let tmp = Utf8PathBuf::from(format!("{gateway_ca_path}.tmp"));
+                if std::fs::write(&tmp, original)
+                    .and_then(|_| std::fs::rename(&tmp, &gateway_ca_path))
+                    .is_err()
+                {
+                    let _ = std::fs::remove_file(&tmp);
+                }
+            }
+            None => {
+                let _ = std::fs::remove_file(&gateway_ca_path);
+            }
+        }
+        return Err(error);
+    }
 
     Ok(PersistedEnrollment {
         agent_id,

package/AgentWindowsManaged/Actions/AgentActions.cs

@@ -298,9 +298,38 @@ internal static class AgentActions
             AgentProperties.AgentTunnelAdvertiseSubnets,
             AgentProperties.AgentTunnelAdvertiseDomains,
             AgentProperties.InstallDir,
+            // installId is a typed WixProperty<Guid>, so reference its string Id; the rollback CA
+            // reads it to locate the per-install tunnel rollback marker.
+            AgentProperties.installId.Id,
         }.Select(p => $"{p}=[{p}]")),
     };
 
+    /// <summary>
+    /// Undo a successful agent tunnel enrollment if a later install action triggers an MSI rollback.
+    /// </summary>
+    /// <remarks>
+    /// The enrollment writes cert/key files and a Tunnel section into agent.json that aren't
+    /// MSI-managed components, so MSI's own rollback leaves them orphaned. The condition mirrors
+    /// <see cref="enrollAgentTunnel"/> exactly (the feature being installed) so the rollback covers
+    /// every path the forward action can run — including adding the feature during a maintenance or
+    /// upgrade transition, not just first install. The rollback is marker-driven: it only deletes
+    /// and restores when EnrollAgentTunnel recorded a per-install marker, so it never touches a
+    /// pre-existing Tunnel section or certs left by a failed/partial enrollment.
+    /// </remarks>
+    private static readonly ElevatedManagedAction rollbackEnrollAgentTunnel = new(
+        new Id($"CA.{nameof(rollbackEnrollAgentTunnel)}"),
+        CustomActions.RollbackEnrollAgentTunnel,
+        Return.ignore,
+        When.Before, new Step(enrollAgentTunnel.Id),
+        Features.AGENT_TUNNEL_FEATURE.BeingInstall(),
+        Sequence.InstallExecuteSequence)
+    {
+        Execute = Execute.rollback,
+        Impersonate = false,
+        // installId locates the per-install tunnel rollback marker EnrollAgentTunnel writes.
+        UsesProperties = UseProperties(new[] { AgentProperties.installId }),
+    };
+
     private static readonly ElevatedManagedAction registerExplorerCommand = new(
         CustomActions.RegisterExplorerCommand
     )
@@ -375,6 +404,7 @@ private static string UseProperties(IEnumerable<IWixProperty> properties)
         setFeaturesToConfigure,
         configureFeatures,
         enrollAgentTunnel,
+        rollbackEnrollAgentTunnel,
         createProgramDataDirectory,
         setProgramDataDirectoryPermissions,
         createProgramDataPedmDirectories,