fix(web): make expiration lifecycle updates atomic

Author: wiresock

amneziawg-web/docs/ARCHITECTURE.md

@@ -72,7 +72,8 @@ list of disabled public keys; the helper derives and filters the trusted
 root-owned config internally before invoking `awg syncconf`.
 Peer additions and removals are semantic operations: the helper holds a stable
 per-interface lock, reconstructs an approved peer block or removes one exact
-managed-client block, and atomically replaces the config.  Arbitrary config
+managed-client block, optionally after checking its expected public key under
+the same lock, and atomically replaces the config.  Arbitrary config
 content, raw file reads, arbitrary `syncconf` stdin, and unknown operations are
 rejected rather than forwarded.
 

amneziawg-web/docs/INSTALL.md

@@ -175,6 +175,8 @@ allow-listed operations provide:
   lifecycle identity checks
 - `append-peer` – validate and atomically append one reconstructed managed-peer block
 - `remove-client` – atomically remove one exact, validated managed-client block
+- `remove-client-if-key` – atomically validate a managed client's public-key
+  identity and remove its exact block
 
 Every operation has a fixed argument shape.  Unknown subcommands, malformed
 interface/client names, keys or AllowedIPs, unsafe configuration paths,

amneziawg-web/scripts/amneziawg-web-privileged

@@ -539,11 +539,16 @@ append_peer_block() {
 remove_client_block() {
     local path="$1"
     local client_name="$2"
+    local expected_public_key="${3:-}"
+    local actual_public_key=""
     local marker_index=-1 marker_count=0 start end index
     local interface_seen=false marker_after_interface=false
     local -a config_lines=()
 
     validate_client_name "${client_name}"
+    if [[ -n "${expected_public_key}" ]]; then
+        validate_public_key "${expected_public_key}"
+    fi
     mapfile -t config_lines < "${path}"
 
     for index in "${!config_lines[@]}"; do
@@ -572,11 +577,17 @@ remove_client_block() {
 
     if (( marker_index + 4 >= ${#config_lines[@]} )) || \
             [[ ! "${config_lines[marker_index + 1]}" =~ ^[[:space:]]*\[Peer\][[:space:]]*$ ]] || \
-            [[ ! "${config_lines[marker_index + 2]}" =~ ^[[:space:]]*PublicKey[[:space:]]*= ]] || \
             [[ ! "${config_lines[marker_index + 3]}" =~ ^[[:space:]]*PresharedKey[[:space:]]*= ]] || \
             [[ ! "${config_lines[marker_index + 4]}" =~ ^[[:space:]]*AllowedIPs[[:space:]]*= ]]; then
         fail "managed client block has an unexpected shape: ${client_name}"
     fi
+    if [[ ! "${config_lines[marker_index + 2]}" =~ ^[[:space:]]*PublicKey[[:space:]]*=[[:space:]]*([A-Za-z0-9+/]{43}=)[[:space:]]*$ ]]; then
+        fail "managed client block has an invalid public key: ${client_name}"
+    fi
+    actual_public_key="${BASH_REMATCH[1]}"
+    if [[ -n "${expected_public_key}" && "${actual_public_key}" != "${expected_public_key}" ]]; then
+        fail "managed client public key does not match the expected identity: ${client_name}"
+    fi
 
     # The web panel creates an exact five-line managed block.  Refuse to
     # remove only its prefix if an operator has added another field or comment;
@@ -686,6 +697,18 @@ main() {
             remove_client_block "${remove_path}" "$2"
             return 0
             ;;
+        remove-client-if-key)
+            require_arg_count "${subcommand}" 3 "$#"
+            validate_interface "$1"
+            validate_client_name "$2"
+            validate_public_key "$3"
+            local conditional_remove_path="${AWG_CONFIG_ROOT}/$1.conf"
+            validate_server_config_path "${conditional_remove_path}"
+            acquire_server_config_lock "$1"
+            validate_server_config_path "${conditional_remove_path}"
+            remove_client_block "${conditional_remove_path}" "$2" "$3"
+            return 0
+            ;;
         *)
             fail "unsupported subcommand: ${subcommand}"
             ;;

amneziawg-web/src/admin/client_manager.rs

@@ -15,7 +15,7 @@
 //! | Read safe params | `…-privileged read-params` |
 //! | Read sanitized server state | `…-privileged read-server-state <iface>` |
 //! | Append peer to server config | `…-privileged append-peer <iface> <name>` |
-//! | Remove managed peer block | `…-privileged remove-client <iface> <name>` |
+//! | Remove managed peer block | `…-privileged remove-client[-if-key] …` |
 //! | Reconcile live interface | `…-privileged reconcile-interface <iface>` |
 //!
 //! Key generation (`awg genkey`, `awg pubkey`, `awg genpsk`) does **not**
@@ -950,26 +950,6 @@ fn server_config_contains_public_key(server_config: &str, expected_public_key: &
     })
 }
 
-fn named_client_has_public_key(
-    server_config: &str,
-    name: &str,
-    expected_public_key: &str,
-) -> Option<bool> {
-    let marker = format!("### Client {name}");
-    let mut lines = server_config.lines();
-    lines.find(|line| line.trim() == marker)?;
-
-    Some(
-        lines
-            .take_while(|line| !line.trim_start().starts_with("### Client "))
-            .any(|line| {
-                line.split_once('=')
-                    .filter(|(key, _)| key.trim().eq_ignore_ascii_case("PublicKey"))
-                    .is_some_and(|(_, value)| value.trim() == expected_public_key)
-            }),
-    )
-}
-
 fn live_interfaces_contain_public_key(
     interfaces: &[awg::AwgInterface],
     expected_public_key: &str,
@@ -1394,13 +1374,15 @@ fn remove_client_inner(
         .map_err(|e| RemoveClientError::ParamsRead(format!("failed to read server config: {e}")))?;
 
     if remove_client_block(&server_config, name).is_some() {
-        if expected_public_key.is_some_and(|public_key| {
-            named_client_has_public_key(&server_config, name, public_key) != Some(true)
-        }) {
-            return Err(RemoveClientError::IdentityMismatch(name.to_string()));
+        match expected_public_key {
+            Some(public_key) => {
+                // The helper revalidates this identity while holding the
+                // server-config lock, closing the read/delete TOCTOU window.
+                awg::remove_client_if_key_via_sudo(&params.server_awg_nic, name, public_key)
+            }
+            None => awg::remove_client_via_sudo(&params.server_awg_nic, name),
         }
-        awg::remove_client_via_sudo(&params.server_awg_nic, name)
-            .map_err(|e| RemoveClientError::FileWrite(format!("remove from server config: {e}")))?;
+        .map_err(|e| RemoveClientError::FileWrite(format!("remove from server config: {e}")))?;
     } else if !allow_missing_server_block {
         return Err(RemoveClientError::ClientNotFound(name.to_string()));
     } else if expected_public_key
@@ -1525,21 +1507,9 @@ mod tests {
     }
 
     #[test]
-    fn resumable_removal_identity_helpers_detect_renamed_and_mismatched_peers() {
+    fn resumable_removal_identity_helpers_detect_keys_across_config_and_live_state() {
         let server = "[Interface]\nPrivateKey = S\n\n### Client renamed\n[Peer]\nPublicKey = ALICE_KEY=\n\n### Client bob\n[Peer]\nPublicKey = BOB_KEY=\n";
 
-        assert_eq!(
-            named_client_has_public_key(server, "renamed", "ALICE_KEY="),
-            Some(true)
-        );
-        assert_eq!(
-            named_client_has_public_key(server, "renamed", "BOB_KEY="),
-            Some(false)
-        );
-        assert_eq!(
-            named_client_has_public_key(server, "alice", "ALICE_KEY="),
-            None
-        );
         assert!(server_config_contains_public_key(server, "ALICE_KEY="));
         assert!(!server_config_contains_public_key(server, "MISSING_KEY="));
 

amneziawg-web/src/admin/mod.rs

@@ -21,10 +21,10 @@ use crate::domain::{normalize_comment, PublicKey};
 
 use self::client_manager::{acquire_lifecycle_lock, RemoveClientError};
 
-/// Serializes expiration edits with automated expiration cleanup. The native
-/// lifecycle lock still protects server/config rewrites; this lock closes the
-/// smaller in-process race between revalidating a deadline and starting that
-/// rewrite.
+/// Serializes expiration edits with creation metadata persistence and
+/// automated expiration cleanup. The native lifecycle lock still protects
+/// server/config rewrites; this lock closes the smaller in-process races at
+/// the database/lifecycle boundary.
 static EXPIRATION_STATE_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(());
 
 fn map_lock_error(err: std::io::Error) -> RemoveClientError {
@@ -184,8 +184,8 @@ pub async fn execute_suggest_ips(
 /// 4. Logs `user_created` or `user_create_failed`.
 ///
 /// The caller is responsible for triggering a config rescan after success.
-// The test-only native-result injection adds an eighth parameter; production
-// retains the existing seven-argument lifecycle boundary.
+// Test-only native-result and lock-acquisition injections extend this
+// boundary; production retains the existing seven parameters.
 #[cfg_attr(test, allow(clippy::too_many_arguments))]
 pub async fn execute_create_user(
     db: &Database,
@@ -196,6 +196,7 @@ pub async fn execute_create_user(
     actor: &str,
     ip_override: &client_manager::IpOverride,
     #[cfg(test)] create_result_override: Option<client_manager::CreateClientResult>,
+    #[cfg(test)] expiration_lock_acquired: Option<tokio::sync::oneshot::Sender<()>>,
 ) -> Result<CreateUserResult, client_manager::CreateClientError> {
     // Pre-validate name (fail fast for the UI).
     script_bridge::validate_client_name(name)?;
@@ -241,7 +242,16 @@ pub async fn execute_create_user(
         }
     };
 
-    // Keep a single lifecycle lock held across native creation and expiration
+    // Expiration state always precedes the native lifecycle lock. Keep both
+    // held through metadata persistence so an edit cannot land on a
+    // poller-created row and then be overwritten by the creation upsert.
+    let _expiration_guard = EXPIRATION_STATE_LOCK.lock().await;
+    #[cfg(test)]
+    if let Some(acquired) = expiration_lock_acquired {
+        let _ = acquired.send(());
+    }
+
+    // Keep a single lifecycle lock held across native creation and metadata
     // persistence. If the database write fails, rollback runs under the same
     // lock, so no concurrent add/remove can strand or replace this client.
     #[cfg(test)]
@@ -918,6 +928,7 @@ mod tests {
             "test-admin",
             &client_manager::IpOverride::default(),
             Some(created_client_result(true)),
+            None,
         )
         .await
         .expect("durable creation must remain a success");
@@ -978,6 +989,7 @@ mod tests {
             "test-admin",
             &client_manager::IpOverride::default(),
             Some(created_client_result(true)),
+            None,
         )
         .await;
 
@@ -1095,4 +1107,83 @@ mod tests {
         assert_eq!(persisted.has_config, 1);
         assert_eq!(persisted.sync_pending, 1);
     }
+
+    #[tokio::test]
+    async fn expiration_edit_waits_for_creation_metadata_persistence() {
+        let db = Database::connect_for_test().await.expect("connect");
+        sqlx::query(
+            "INSERT INTO peers (public_key, allowed_ips)
+             VALUES ('CREATED_PUBLIC_KEY=', '10.66.66.2/32')",
+        )
+        .execute(&db.pool)
+        .await
+        .expect("seed poller-created peer");
+        let peer = find_by_public_key(&db.pool, "CREATED_PUBLIC_KEY=")
+            .await
+            .expect("query peer")
+            .expect("seeded peer");
+
+        // Hold the config-mapping lock so creation pauses after taking the
+        // expiration lock but before its upsert can overwrite the seeded row.
+        let mapping_guard = crate::poller::acquire_config_mapping_lock().await;
+        let task_db = db.clone();
+        let dir = tempfile::tempdir().expect("tempdir");
+        let dir_path = dir.path().to_path_buf();
+        let (lock_acquired_tx, lock_acquired_rx) = tokio::sync::oneshot::channel();
+        let create_task = tokio::spawn(async move {
+            let ip_override = client_manager::IpOverride::default();
+            execute_create_user(
+                &task_db,
+                &dir_path,
+                "alice",
+                None,
+                None,
+                "test-admin",
+                &ip_override,
+                Some(created_client_result(false)),
+                Some(lock_acquired_tx),
+            )
+            .await
+        });
+
+        lock_acquired_rx
+            .await
+            .expect("creation must acquire the expiration lock before persistence");
+
+        let update_db = db.clone();
+        let update_task = tokio::spawn(async move {
+            execute_update_peer_expiration(
+                &update_db,
+                peer.id,
+                Some("2026-08-18T12:00:00Z"),
+                Some("alice"),
+            )
+            .await
+        });
+        tokio::task::yield_now().await;
+        assert!(
+            !update_task.is_finished(),
+            "expiration edit must wait until creation metadata is durable"
+        );
+
+        drop(mapping_guard);
+        create_task
+            .await
+            .expect("creation task")
+            .expect("create user");
+        update_task
+            .await
+            .expect("expiration task")
+            .expect("update expiration")
+            .expect("updated peer");
+
+        let persisted = find_by_public_key(&db.pool, "CREATED_PUBLIC_KEY=")
+            .await
+            .expect("query peer")
+            .expect("persisted peer");
+        assert_eq!(
+            persisted.expires_at.as_deref(),
+            Some("2026-08-18T12:00:00Z")
+        );
+    }
 }

amneziawg-web/src/awg/mod.rs

@@ -393,6 +393,37 @@ pub fn remove_client_via_sudo(interface: &str, name: &str) -> Result<(), AwgErro
     Ok(())
 }
 
+/// Remove one installer-managed client block only if its public key still
+/// matches `expected_public_key`. The helper performs the identity check and
+/// config rewrite atomically while holding the per-interface lock.
+#[cfg_attr(not(unix), allow(dead_code))]
+pub fn remove_client_if_key_via_sudo(
+    interface: &str,
+    name: &str,
+    expected_public_key: &str,
+) -> Result<(), AwgError> {
+    let output = Command::new(SUDO_BIN)
+        .args([
+            "-n",
+            PRIVILEGED_HELPER_BIN,
+            "remove-client-if-key",
+            interface,
+            name,
+            expected_public_key,
+        ])
+        .output()?;
+
+    if !output.status.success() {
+        let stderr = String::from_utf8_lossy(&output.stderr).into_owned();
+        return Err(AwgError::NonZeroExit {
+            status: output.status.code().unwrap_or(-1),
+            stderr,
+        });
+    }
+
+    Ok(())
+}
+
 /// Remove `[Peer]` sections from a WireGuard stripped config whose
 /// `PublicKey` appears in `disabled_keys`.
 ///
@@ -827,6 +858,31 @@ awg0\tCLIENT2_PUB_KEY=\t(none)\t(none)\t10.8.0.3/32\t0\t0\t0\toff\n\
                 "alice",
             ]
         );
+
+        let mut conditional_remove_cmd = Command::new(SUDO_BIN);
+        conditional_remove_cmd.args([
+            "-n",
+            PRIVILEGED_HELPER_BIN,
+            "remove-client-if-key",
+            "awg0",
+            "alice",
+            "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
+        ]);
+        let conditional_remove_args: Vec<_> = conditional_remove_cmd
+            .get_args()
+            .map(|a| a.to_string_lossy().to_string())
+            .collect();
+        assert_eq!(
+            conditional_remove_args,
+            vec![
+                "-n",
+                PRIVILEGED_HELPER_BIN,
+                "remove-client-if-key",
+                "awg0",
+                "alice",
+                "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=",
+            ]
+        );
     }
 
     // ── filter_disabled_peers tests ─────────────────────────────────

amneziawg-web/src/web/mod.rs

@@ -2807,6 +2807,8 @@ async fn api_create_user(
         &ip_override,
         #[cfg(test)]
         injected_create_result(&headers, &state, &name, &ip_override),
+        #[cfg(test)]
+        None,
     )
     .await
     {
@@ -3033,6 +3035,8 @@ async fn post_add_user_form(
         &ip_override,
         #[cfg(test)]
         injected_create_result(&headers, &state, &name, &ip_override),
+        #[cfg(test)]
+        None,
     )
     .await
     {