From 6ed613fd47c4a6ad23e0cb15f76a5e97f2258795 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 16:18:55 +0200 Subject: [PATCH 01/12] feat: Split the store into active, departed and purged users in a v2 schema The departed map replaces the archived key of the unreleased v2 draft schema, renaming the tombstone timestamp to departed_at and dropping the home_archive field. A new purged map records tombstones whose expired account was removed after the retention period, so even a member rejoining after a purge gets their old UID and GID back. Fresh ID allocation now also avoids the IDs reserved by purged tombstones. The draft schema was never deployed, so version 2 is redefined in place and no compatibility with it is kept. --- src/octosync.rs | 165 ++++++++--------------- src/store.rs | 342 ++++++++++++++++++++++++++++++------------------ 2 files changed, 273 insertions(+), 234 deletions(-) diff --git a/src/octosync.rs b/src/octosync.rs index c64ed6b..3935f8f 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -96,7 +96,7 @@ impl Octosync { ) -> anyhow::Result { let new_user = match store.data().get(&gh_user.id) { Some(user) => self.manage_existing_user(gh_user, user).await?, - None => self.create_user(gh_user, store.archived()).await?, + None => self.create_user(gh_user, store).await?, }; self.user_manager @@ -126,33 +126,50 @@ impl Octosync { Ok(new_user) } - /// Create the platform user, re-using the archived IDs when the member rejoined. + /// Create the platform user, re-using the tombstoned IDs when the member rejoined. /// /// A rejoin whose stored ID is taken fails loudly and is retried on the next /// sync; falling back to fresh IDs would recreate exactly the ownership drift /// the tombstones exist to prevent. A brand-new member must not be allocated an - /// ID a tombstone reserves either, so the reserved IDs travel with the creation. + /// ID a tombstone (departed or purged) reserves either, so the reserved IDs + /// travel with the creation. async fn create_user( &self, gh_user: &octocrab::models::Author, - archived: &store::ArchivedMap, + store: &store::UserStore, ) -> anyhow::Result { - let ids = match archived.get(&gh_user.id) { - Some(tombstone) => { + // A departed member rejoins into their expired account; a purged one gets a + // fresh account and home directory under their old IDs + let stored_ids = store + .departed() + .get(&gh_user.id) + .map(|tombstone| (tombstone.uid(), tombstone.gid())) + .or_else(|| { + store + .purged() + .get(&gh_user.id) + .map(|tombstone| (tombstone.uid(), tombstone.gid())) + }); + let ids = match stored_ids { + Some((uid, gid)) => { tracing::info!( - uid = tombstone.uid().as_raw(), + uid = uid.as_raw(), "Member rejoined, re-creating the account with its stored IDs" ); - user_manager::AccountIds::Stored { - uid: tombstone.uid(), - gid: tombstone.gid(), - } + user_manager::AccountIds::Stored { uid, gid } } None => user_manager::AccountIds::Fresh { - reserved_uids: archived.values().map(store::ArchivedUser::uid).collect(), - reserved_gids: archived + reserved_uids: store + .departed() + .values() + .map(store::DepartedUser::uid) + .chain(store.purged().values().map(store::PurgedUser::uid)) + .collect(), + reserved_gids: store + .departed() .values() - .filter_map(store::ArchivedUser::gid) + .filter_map(store::DepartedUser::gid) + .chain(store.purged().values().filter_map(store::PurgedUser::gid)) .collect(), }, }; @@ -250,21 +267,15 @@ impl Octosync { store: &mut store::UserStore, leavers: Vec, ) -> anyhow::Result<()> { - let deleted_at = chrono::Utc::now(); + let departed_at = chrono::Utc::now(); for user in leavers { tracing::info!("Archiving user '{}' for deletion", user.name()); - store.archive_user(user, deleted_at); + store.depart_user(user, departed_at); } store.save().await?; - for (id, archive_path) in self.delete_archived_accounts(store.archived()).await { - // An already-gone account reports no archive; keep the path recorded by - // the sync that created it - if let Some(archive_path) = archive_path { - store.record_home_archive(&id, archive_path); - } - } - store.save().await + self.delete_archived_accounts(store.departed()).await; + Ok(()) } /// Process all org members concurrently and collect the successfully processed @@ -283,7 +294,8 @@ impl Octosync { let mut new_store = store::UserStore::new(&self.data_dir).await?; // Tombstones are carried over wholesale before any member is processed, so // their survival never depends on per-user processing succeeding - *new_store.archived_mut() = store.archived().clone(); + *new_store.departed_mut() = store.departed().clone(); + *new_store.purged_mut() = store.purged().clone(); *new_store.data_mut() = stream::iter(org_members) .map(|gh_user| { let groups = assignments.user_groups(gh_user.id); @@ -307,35 +319,23 @@ impl Octosync { Ok(new_store) } - /// Delete the platform accounts of the archived users concurrently, returning the - /// ID and home archive path of every successful deletion. + /// Delete the platform accounts of the archived users concurrently. /// /// Failures are only logged: the tombstone stays in the store, so the deletion is /// retried on every sync until the account is gone. - async fn delete_archived_accounts( - &self, - archived: &store::ArchivedMap, - ) -> Vec<(octocrab::models::UserId, Option)> { - stream::iter(archived.values()) - .map(|archived_user| async move { - self.user_manager - .delete_user(&store::User::from(archived_user)) - .await - .map(|archive_path| (archived_user.id(), archive_path)) - .inspect_err(|e| { - tracing::error!( - "Failed to delete user '{}': {:?}", - archived_user.name(), - e - ); - }) - .ok() - }) + async fn delete_archived_accounts(&self, departed: &store::DepartedMap) { + stream::iter(departed.values()) // Deletion archives the user's home directory, which can take a while for large // home directories, run a few deletions concurrently - .buffer_unordered(MAX_CONCURRENT_DELETES) - .filter_map(|res| async move { res }) - .collect() + .for_each_concurrent(MAX_CONCURRENT_DELETES, |departed_user| async move { + if let Err(e) = self + .user_manager + .delete_user(&store::User::from(departed_user)) + .await + { + tracing::error!("Failed to delete user '{}': {:?}", departed_user.name(), e); + } + }) .await } @@ -612,7 +612,7 @@ mod tests { let received_ids = actor.create_user_ids.clone(); let (octosync, _data_dir) = octosync_with(actor); let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + old_store.depart_user(user(1, "alice"), chrono::Utc::now()); let new_store = octosync .process_members( @@ -632,7 +632,7 @@ mod tests { }] ); assert!(new_store.data().contains_key(&octocrab::models::UserId(1))); - assert!(new_store.archived().is_empty()); + assert!(new_store.departed().is_empty()); } /// A brand-new member's creation carries the IDs reserved by tombstones, so @@ -645,7 +645,7 @@ mod tests { let received_ids = actor.create_user_ids.clone(); let (octosync, _data_dir) = octosync_with(actor); let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + old_store.depart_user(user(1, "alice"), chrono::Utc::now()); octosync .process_members( @@ -676,7 +676,7 @@ mod tests { .push_back(Err(anyhow::anyhow!("UID is taken"))); let (octosync, _data_dir) = octosync_with(actor); let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + old_store.depart_user(user(1, "alice"), chrono::Utc::now()); let new_store = octosync .process_members( @@ -691,7 +691,7 @@ mod tests { assert!(new_store.data().is_empty()); assert!( new_store - .archived() + .departed() .contains_key(&octocrab::models::UserId(1)) ); } @@ -714,64 +714,11 @@ mod tests { .unwrap(); let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); - let tombstone = &saved.archived()[&octocrab::models::UserId(1)]; + let tombstone = &saved.departed()[&octocrab::models::UserId(1)]; assert_eq!(tombstone.name(), "alice"); - assert_eq!(tombstone.home_archive(), None); assert!(saved.data().is_empty()); } - /// A successful deletion records the home archive path on the tombstone. - #[tokio::test] - async fn archive_and_delete_records_the_home_archive_path() { - let home_dir = tempfile::tempdir().unwrap(); - std::fs::write(home_dir.path().join("notes.txt"), "important").unwrap(); - - let mut actor = TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::Prepared { - home_dir: home_dir.path().to_path_buf(), - })); - actor.remove_account.push_back(Ok(())); - let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); - - octosync - .archive_and_delete(&mut store, vec![user(1, "alice")]) - .await - .unwrap(); - - let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); - let tombstone = &saved.archived()[&octocrab::models::UserId(1)]; - let archive = tombstone.home_archive().expect("archive path recorded"); - assert!(archive.exists()); - } - - /// A tombstone whose account is already gone reports no archive; the path - /// recorded by the sync that archived the home directory must survive. - #[tokio::test] - async fn archive_and_delete_keeps_a_recorded_archive_path() { - let mut actor = TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::NothingToDo)); - let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); - store.archive_user(user(1, "alice"), chrono::Utc::now()); - let archive = path::PathBuf::from("/data/home-archive/alice.tar.gz"); - store.record_home_archive(&octocrab::models::UserId(1), archive.clone()); - - octosync - .archive_and_delete(&mut store, vec![]) - .await - .unwrap(); - - assert_eq!( - store.archived()[&octocrab::models::UserId(1)].home_archive(), - Some(archive.as_path()) - ); - } - /// The `delete` command tombstones every user instead of removing the store /// file, so the UID memory survives a full wipe. #[tokio::test] @@ -792,7 +739,7 @@ mod tests { let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); assert!(saved.data().is_empty()); assert_eq!( - saved.archived()[&octocrab::models::UserId(1)].name(), + saved.departed()[&octocrab::models::UserId(1)].name(), "alice" ); } diff --git a/src/store.rs b/src/store.rs index 9feb3b2..1deac13 100644 --- a/src/store.rs +++ b/src/store.rs @@ -101,37 +101,34 @@ impl User { } } -/// Tombstone of a user whose account octosync deleted (or is deleting). It keeps the -/// UID so a rejoining member gets their old UID back and file ownership outside the -/// archived home directory survives the delete and re-create cycle. +/// Tombstone of a member who left the org. Their account stays on the machine expired, +/// and the tombstone keeps the IDs so a rejoining member gets their old UID and GID +/// back and file ownership survives the departure. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] -pub struct ArchivedUser { +pub struct DepartedUser { /// GitHub user ID, used as the primary key for identifying users in the store id: octocrab::models::UserId, - /// GitHub login at the time of deletion + /// GitHub login at the time of departure name: String, - /// Linux user UID the account had, reserved for a rejoin + /// Linux user UID the account has, reserved for a rejoin #[serde(with = "uid_serde")] uid: unistd::Uid, - /// GID of the primary group the account had, reserved for a rejoin. `None` when - /// the user was archived before their GID was backfilled. + /// GID of the primary group the account has, reserved for a rejoin. `None` when + /// the user departed before their GID was backfilled. #[serde(default, with = "gid_serde", skip_serializing_if = "Option::is_none")] gid: Option, - /// When the user was archived for deletion - deleted_at: chrono::DateTime, - /// Path of the archived home directory, so a rejoin can later restore it - #[serde(default, skip_serializing_if = "Option::is_none")] - home_archive: Option, + /// When the member departed and their account was expired + departed_at: chrono::DateTime, } #[allow(unused)] -impl ArchivedUser { +impl DepartedUser { /// Get the GitHub user ID pub fn id(&self) -> octocrab::models::UserId { self.id } - /// Get the login the user had when they were archived + /// Get the login the user had when they departed pub fn name(&self) -> &str { &self.name } @@ -146,31 +143,81 @@ impl ArchivedUser { self.gid } - /// Get the time the user was archived for deletion - pub fn deleted_at(&self) -> chrono::DateTime { - self.deleted_at - } - - /// Get the path of the archived home directory, if one was created - pub fn home_archive(&self) -> Option<&path::Path> { - self.home_archive.as_deref() + /// Get the time the member departed + pub fn departed_at(&self) -> chrono::DateTime { + self.departed_at } } -impl From<&ArchivedUser> for User { +impl From<&DepartedUser> for User { /// Account identity of the tombstone, used to address platform operations - fn from(archived: &ArchivedUser) -> Self { + fn from(departed: &DepartedUser) -> Self { Self { - id: archived.id, - name: archived.name.clone(), - uid: archived.uid, - gid: archived.gid, + id: departed.id, + name: departed.name.clone(), + uid: departed.uid, + gid: departed.gid, } } } +/// Tombstone of a departed user whose expired account and home directory were purged +/// after the retention period. It keeps the IDs so a member rejoining even after the +/// purge gets their old UID and GID back, with an empty home directory. +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] +pub struct PurgedUser { + /// GitHub user ID, used as the primary key for identifying users in the store + id: octocrab::models::UserId, + /// GitHub login at the time of departure + name: String, + /// Linux user UID the account had, reserved for a rejoin + #[serde(with = "uid_serde")] + uid: unistd::Uid, + /// GID of the primary group the account had, reserved for a rejoin + #[serde(default, with = "gid_serde", skip_serializing_if = "Option::is_none")] + gid: Option, + /// When the member departed and their account was expired + departed_at: chrono::DateTime, + /// When the expired account and its home directory were purged + purged_at: chrono::DateTime, +} + +#[allow(unused)] +impl PurgedUser { + /// Get the GitHub user ID + pub fn id(&self) -> octocrab::models::UserId { + self.id + } + + /// Get the login the user had when they departed + pub fn name(&self) -> &str { + &self.name + } + + /// Get the Linux user UID reserved for a rejoin + pub fn uid(&self) -> unistd::Uid { + self.uid + } + + /// Get the GID of the primary group reserved for a rejoin, if it is tracked + pub fn gid(&self) -> Option { + self.gid + } + + /// Get the time the member departed + pub fn departed_at(&self) -> chrono::DateTime { + self.departed_at + } + + /// Get the time the account was purged + pub fn purged_at(&self) -> chrono::DateTime { + self.purged_at + } +} + pub type UserMap = collections::HashMap; -pub type ArchivedMap = collections::HashMap; +pub type DepartedMap = collections::HashMap; +pub type PurgedMap = collections::HashMap; const USERS_FILE_NAME: &str = "users.json"; const USERS_V1_FILE_NAME: &str = "users-v1.json"; @@ -182,7 +229,9 @@ const STORE_VERSION: u64 = 2; struct StoreData { users: UserMap, #[serde(default)] - archived: ArchivedMap, + departed: DepartedMap, + #[serde(default)] + purged: PurgedMap, } /// Borrowing counterpart of [`StoreData`] for serialization @@ -190,7 +239,8 @@ struct StoreData { struct StoreDataRef<'a> { version: u64, users: &'a UserMap, - archived: &'a ArchivedMap, + departed: &'a DepartedMap, + purged: &'a PurgedMap, } #[derive(Debug)] @@ -198,9 +248,14 @@ pub struct UserStore { dir: path::PathBuf, /// In-memory cache of users loaded from the members database, keyed by GitHub user ID users: UserMap, - /// Tombstones of deleted users, keyed by GitHub user ID. Kept separate from the - /// active users so no consumer can process a tombstone as an active user. - archived: ArchivedMap, + /// Tombstones of departed members whose account is expired on the machine, keyed by + /// GitHub user ID. Kept separate from the active users so no consumer can process a + /// tombstone as an active user. + departed: DepartedMap, + /// Tombstones of departed members whose expired account was purged, keyed by GitHub + /// user ID. Kept separate from `departed` so the expiry reconciliation and the + /// purge can never fight over an entry. + purged: PurgedMap, } impl UserStore { @@ -210,7 +265,8 @@ impl UserStore { Ok(Self { dir: dir.to_path_buf(), users: UserMap::new(), - archived: ArchivedMap::new(), + departed: DepartedMap::new(), + purged: PurgedMap::new(), }) } @@ -230,41 +286,42 @@ impl UserStore { &mut self.users } - pub fn archived(&self) -> &ArchivedMap { - &self.archived + pub fn departed(&self) -> &DepartedMap { + &self.departed + } + + pub fn departed_mut(&mut self) -> &mut DepartedMap { + &mut self.departed + } + + pub fn purged(&self) -> &PurgedMap { + &self.purged } - pub fn archived_mut(&mut self) -> &mut ArchivedMap { - &mut self.archived + pub fn purged_mut(&mut self) -> &mut PurgedMap { + &mut self.purged } - /// Turn a user into a tombstone, keeping their UID for a later rejoin - pub fn archive_user(&mut self, user: User, deleted_at: chrono::DateTime) { + /// Turn a user into a departed tombstone, keeping their IDs for a later rejoin + pub fn depart_user(&mut self, user: User, departed_at: chrono::DateTime) { self.users.remove(&user.id); - self.archived.insert( + self.departed.insert( user.id, - ArchivedUser { + DepartedUser { id: user.id, name: user.name, uid: user.uid, gid: user.gid, - deleted_at, - home_archive: None, + departed_at, }, ); } - /// Record the home directory archive of an archived user - pub fn record_home_archive(&mut self, id: &octocrab::models::UserId, archive: path::PathBuf) { - if let Some(archived) = self.archived.get_mut(id) { - archived.home_archive = Some(archive); - } - } - - /// Drop the tombstones of users that are active again, keeping every user either - /// active or archived but never both + /// Drop the tombstones of users that are active again, keeping every user in + /// exactly one of the three maps pub fn prune_rejoined(&mut self) { - self.archived.retain(|id, _| !self.users.contains_key(id)); + self.departed.retain(|id, _| !self.users.contains_key(id)); + self.purged.retain(|id, _| !self.users.contains_key(id)); } /// Get the file path for the users database file @@ -312,7 +369,8 @@ impl UserStore { self.backup_v1_file().await?; StoreData { users, - archived: ArchivedMap::new(), + departed: DepartedMap::new(), + purged: PurgedMap::new(), } } Some(version) if version.as_u64() == Some(STORE_VERSION) => { @@ -328,7 +386,8 @@ impl UserStore { }; self.users = data.users; - self.archived = data.archived; + self.departed = data.departed; + self.purged = data.purged; Ok(()) } @@ -352,7 +411,8 @@ impl UserStore { let content = serde_json::to_string_pretty(&StoreDataRef { version: STORE_VERSION, users: &self.users, - archived: &self.archived, + departed: &self.departed, + purged: &self.purged, })?; let path = self.path(); // Stage in a temporary file that atomically replaces the database on commit, so a @@ -385,22 +445,32 @@ mod tests { } } - fn deleted_at() -> chrono::DateTime { + fn departed_at() -> chrono::DateTime { chrono::DateTime::parse_from_rfc3339("2026-08-14T09:00:00Z") .unwrap() .with_timezone(&chrono::Utc) } - fn archived_user() -> ArchivedUser { - ArchivedUser { + fn departed_user() -> DepartedUser { + DepartedUser { id: octocrab::models::UserId(456), name: "bob".to_string(), uid: unistd::Uid::from_raw(1005), gid: Some(unistd::Gid::from_raw(1005)), - deleted_at: deleted_at(), - home_archive: Some(path::PathBuf::from( - "/var/lib/octosync/home-archive/bob.tar.gz", - )), + departed_at: departed_at(), + } + } + + fn purged_user() -> PurgedUser { + PurgedUser { + id: octocrab::models::UserId(789), + name: "carol".to_string(), + uid: unistd::Uid::from_raw(1006), + gid: Some(unistd::Gid::from_raw(1006)), + departed_at: departed_at(), + purged_at: chrono::DateTime::parse_from_rfc3339("2027-02-14T09:00:00Z") + .unwrap() + .with_timezone(&chrono::Utc), } } @@ -485,46 +555,49 @@ mod tests { } } - mod archived_user { + mod departed_user { use super::*; #[test] fn round_trip_serialization() { - let original = archived_user(); + let original = departed_user(); let serialized = serde_json::to_string(&original).expect("Failed to serialize"); - let deserialized: ArchivedUser = + let deserialized: DepartedUser = serde_json::from_str(&serialized).expect("Failed to deserialize"); assert_eq!(original, deserialized); } #[test] - fn deleted_at_is_serialized_as_rfc3339() { - let serialized = serde_json::to_string(&archived_user()).unwrap(); - assert!(serialized.contains("\"deleted_at\":\"2026-08-14T09:00:00Z\"")); + fn departed_at_is_serialized_as_rfc3339() { + let serialized = serde_json::to_string(&departed_user()).unwrap(); + assert!(serialized.contains("\"departed_at\":\"2026-08-14T09:00:00Z\"")); } #[test] - fn missing_home_archive_is_omitted_and_parses_back() { - let mut archived = archived_user(); - archived.home_archive = None; - - let serialized = serde_json::to_string(&archived).unwrap(); - assert!(!serialized.contains("home_archive")); - - let deserialized: ArchivedUser = serde_json::from_str(&serialized).unwrap(); - assert_eq!(deserialized.home_archive, None); + fn account_identity_conversion() { + let departed = departed_user(); + let user = User::from(&departed); + assert_eq!(user.id, departed.id); + assert_eq!(user.name, departed.name); + assert_eq!(user.uid, departed.uid); + assert_eq!(user.gid, departed.gid); } + } + + mod purged_user { + use super::*; #[test] - fn account_identity_conversion() { - let archived = archived_user(); - let user = User::from(&archived); - assert_eq!(user.id, archived.id); - assert_eq!(user.name, archived.name); - assert_eq!(user.uid, archived.uid); - assert_eq!(user.gid, archived.gid); + fn round_trip_serialization() { + let original = purged_user(); + + let serialized = serde_json::to_string(&original).expect("Failed to serialize"); + let deserialized: PurgedUser = + serde_json::from_str(&serialized).expect("Failed to deserialize"); + + assert_eq!(original, deserialized); } } @@ -557,7 +630,8 @@ mod tests { assert!(store_path.exists()); assert!(store.users.is_empty()); - assert!(store.archived.is_empty()); + assert!(store.departed.is_empty()); + assert!(store.purged.is_empty()); } #[tokio::test] @@ -569,7 +643,7 @@ mod tests { .expect("Failed to create store"); assert!(store.users.is_empty()); - assert!(store.archived.is_empty()); + assert!(store.departed.is_empty()); // No file, nothing to back up assert!(!temp_dir.path().join(USERS_V1_FILE_NAME).exists()); } @@ -588,7 +662,7 @@ mod tests { store.users[&octocrab::models::UserId(12345)].name, "testuser" ); - assert!(store.archived.is_empty()); + assert!(store.departed.is_empty()); // The backup preserves the v1 file byte for byte let backup = fs::read_to_string(temp_dir.path().join(USERS_V1_FILE_NAME)) @@ -611,7 +685,8 @@ mod tests { let value: serde_json::Value = serde_json::from_str(&content).unwrap(); assert_eq!(value["version"], 2); assert!(value["users"]["12345"].is_object()); - assert_eq!(value["archived"], serde_json::json!({})); + assert_eq!(value["departed"], serde_json::json!({})); + assert_eq!(value["purged"], serde_json::json!({})); } #[tokio::test] @@ -624,16 +699,18 @@ mod tests { } #[tokio::test] - async fn round_trip_with_archived_users() { + async fn round_trip_with_departed_and_purged_users() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); let mut store = UserStore::from_dir(temp_dir.path()) .await .expect("Failed to create store"); let active = user(); - let archived = archived_user(); + let departed = departed_user(); + let purged = purged_user(); store.users.insert(active.id, active.clone()); - store.archived.insert(archived.id, archived.clone()); + store.departed.insert(departed.id, departed.clone()); + store.purged.insert(purged.id, purged.clone()); store.save().await.expect("Failed to save store"); @@ -641,68 +718,72 @@ mod tests { .await .expect("Failed to load store"); assert_eq!(loaded.users[&active.id], active); - assert_eq!(loaded.archived[&archived.id], archived); + assert_eq!(loaded.departed[&departed.id], departed); + assert_eq!(loaded.purged[&purged.id], purged); // The file was already v2, no v1 backup is created assert!(!temp_dir.path().join(USERS_V1_FILE_NAME).exists()); } #[tokio::test] - async fn v2_file_without_archived_key_parses() { + async fn v2_file_without_tombstone_keys_parses() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), r#"{ "version": 2, "users": {} }"#).await; let store = UserStore::from_dir(temp_dir.path()).await.unwrap(); assert!(store.users.is_empty()); - assert!(store.archived.is_empty()); + assert!(store.departed.is_empty()); + assert!(store.purged.is_empty()); } #[tokio::test] - async fn archive_user_moves_an_active_user_to_the_tombstones() { + async fn depart_user_moves_an_active_user_to_the_tombstones() { let temp_dir = tempfile::TempDir::new().unwrap(); let mut store = UserStore::new(temp_dir.path()).await.unwrap(); let user = user(); store.users.insert(user.id, user.clone()); - store.archive_user(user.clone(), deleted_at()); + store.depart_user(user.clone(), departed_at()); assert!(store.users.is_empty()); - let archived = &store.archived[&user.id]; - assert_eq!(archived.name, user.name); - assert_eq!(archived.uid, user.uid); - assert_eq!(archived.gid, user.gid); - assert_eq!(archived.deleted_at, deleted_at()); - assert_eq!(archived.home_archive, None); + let departed = &store.departed[&user.id]; + assert_eq!(departed.name, user.name); + assert_eq!(departed.uid, user.uid); + assert_eq!(departed.gid, user.gid); + assert_eq!(departed.departed_at, departed_at()); } #[tokio::test] - async fn record_home_archive_sets_the_path_on_the_tombstone() { + async fn prune_rejoined_drops_tombstones_of_active_users() { let temp_dir = tempfile::TempDir::new().unwrap(); let mut store = UserStore::new(temp_dir.path()).await.unwrap(); - let user = user(); - store.archive_user(user.clone(), deleted_at()); + let rejoined = user(); + let departed = departed_user(); + store + .departed + .insert(rejoined.id, DepartedUser::from_test_user(&rejoined)); + store.departed.insert(departed.id, departed.clone()); + store.users.insert(rejoined.id, rejoined.clone()); - let archive = path::PathBuf::from("/data/home-archive/testuser.tar.gz"); - store.record_home_archive(&user.id, archive.clone()); + store.prune_rejoined(); - assert_eq!(store.archived[&user.id].home_archive, Some(archive)); + assert!(!store.departed.contains_key(&rejoined.id)); + assert_eq!(store.departed[&departed.id], departed); } + /// A member who rejoins after their account was purged spends the purged + /// tombstone, exactly like a departed one #[tokio::test] - async fn prune_rejoined_drops_tombstones_of_active_users() { + async fn prune_rejoined_drops_purged_tombstones_of_active_users() { let temp_dir = tempfile::TempDir::new().unwrap(); let mut store = UserStore::new(temp_dir.path()).await.unwrap(); - let rejoined = user(); - let departed = archived_user(); - store - .archived - .insert(rejoined.id, ArchivedUser::from_test_user(&rejoined)); - store.archived.insert(departed.id, departed.clone()); - store.users.insert(rejoined.id, rejoined.clone()); + let purged = purged_user(); + let rejoined = User::from_test_purged(&purged); + store.purged.insert(purged.id, purged.clone()); + store.users.insert(rejoined.id, rejoined); store.prune_rejoined(); - assert!(!store.archived.contains_key(&rejoined.id)); - assert_eq!(store.archived[&departed.id], departed); + assert!(store.purged.is_empty()); } #[tokio::test] @@ -760,7 +841,7 @@ mod tests { } } - impl ArchivedUser { + impl DepartedUser { /// Tombstone with the identity of `user`, for tests fn from_test_user(user: &User) -> Self { Self { @@ -768,8 +849,19 @@ mod tests { name: user.name.clone(), uid: user.uid, gid: user.gid, - deleted_at: deleted_at(), - home_archive: None, + departed_at: departed_at(), + } + } + } + + impl User { + /// Active user with the identity of a purged tombstone, for tests + fn from_test_purged(purged: &PurgedUser) -> Self { + Self { + id: purged.id, + name: purged.name.clone(), + uid: purged.uid, + gid: purged.gid, } } } From b62189d429293fb9b968901e404abbc22e6dd924 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 16:40:20 +0200 Subject: [PATCH 02/12] feat: Replace account deletion with expiry and purge after a retention period A departed member's account is no longer archived and removed. One ExpireAccount message expires it with usermod --expiredate, ends the running sessions through logind (TerminateUser and disabled linger over D-Bus, with the SIGTERM/SIGKILL sweep as the catch-all) and strips the supplementary groups. The account and home directory stay on the machine as the durable, reversible departure record that the existing rejoin path heals. The message is idempotent and re-sent for every departed tombstone on each sync, so an interrupted expiry converges. Accounts expired for at least the retention period (default 180 days, --purge-after-days) are purged at the end of each sync and through the new explicit purge command: a force-kill sweep, then userdel --remove, no archive. Eligibility requires the tombstone age and the account's own shadow expiry to agree, and the member to be absent from the fetched member list. The tombstone moves to the purged map with a purged_at timestamp, so the IDs stay reserved even after the purge. The archiver and its tar dependencies are removed. The delete command now expires all managed users instead of deleting them. --- Cargo.lock | 80 ---- Cargo.toml | 2 - src/archiver.rs | 632 --------------------------- src/main.rs | 27 +- src/octosync.rs | 432 +++++++++++++----- src/store.rs | 43 +- src/user_manager.rs | 191 +++----- src/user_manager/backends/linux.rs | 230 ++++++---- src/user_manager/backends/mock.rs | 37 +- src/user_manager/backends/testing.rs | 31 +- src/user_manager/messages.rs | 75 ++-- 11 files changed, 686 insertions(+), 1094 deletions(-) delete mode 100644 src/archiver.rs diff --git a/Cargo.lock b/Cargo.lock index cd956fd..5cc653b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -82,22 +82,6 @@ dependencies = [ "rustversion", ] -[[package]] -name = "astral-tokio-tar" -version = "0.6.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b18457efd137254e016bbde5e1d88df61c4e1a5ae2223746e56123bac6af2463" -dependencies = [ - "futures-core", - "libc", - "portable-atomic", - "rustc-hash", - "rustix", - "tokio", - "tokio-stream", - "xattr", -] - [[package]] name = "async-broadcast" version = "0.7.2" @@ -122,18 +106,6 @@ dependencies = [ "pin-project-lite", ] -[[package]] -name = "async-compression" -version = "0.4.43" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3976abdc8fe7d1133d43d304afd42abdf5bc3e1319d263d223bde07b5efc4be8" -dependencies = [ - "compression-codecs", - "compression-core", - "pin-project-lite", - "tokio", -] - [[package]] name = "async-executor" version = "1.14.0" @@ -496,23 +468,6 @@ version = "1.0.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1d07550c9036bf2ae0c684c4297d503f838287c83c53686d05370d0e139ae570" -[[package]] -name = "compression-codecs" -version = "0.4.38" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "ce2548391e9c1929c21bf6aa2680af86fe4c1b33e6cea9ac1cfeec0bd11218cf" -dependencies = [ - "compression-core", - "flate2", - "memchr", -] - -[[package]] -name = "compression-core" -version = "0.4.32" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "cc14f565cf027a105f7a44ccf9e5b424348421a1d8952a8fc9d499d313107789" - [[package]] name = "concurrent-queue" version = "2.5.0" @@ -1613,8 +1568,6 @@ name = "octosync" version = "0.3.0" dependencies = [ "anyhow", - "astral-tokio-tar", - "async-compression", "atomic-write-file", "bon", "chrono", @@ -1843,12 +1796,6 @@ dependencies = [ "windows-sys 0.61.2", ] -[[package]] -name = "portable-atomic" -version = "1.15.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "05c8b63e8d9609db387f0324918f81d68fe27748f084ef092fb35954d0539a85" - [[package]] name = "potential_utf" version = "0.1.5" @@ -2060,12 +2007,6 @@ dependencies = [ "zeroize", ] -[[package]] -name = "rustc-hash" -version = "2.1.3" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6b1e7f9a428571be2dc5bc0505c13fb6bf936822b894ec87abf8a08a4e51742d" - [[package]] name = "rustix" version = "1.1.4" @@ -2628,17 +2569,6 @@ dependencies = [ "tokio", ] -[[package]] -name = "tokio-stream" -version = "0.1.19" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a3d06f0b082ba57c26b79407372e57cf2a1e28124f78e9479fe80322cf53420b" -dependencies = [ - "futures-core", - "pin-project-lite", - "tokio", -] - [[package]] name = "tokio-util" version = "0.7.19" @@ -3126,16 +3056,6 @@ version = "0.6.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "1ffae5123b2d3fc086436f8834ae3ab053a283cfac8fe0a0b8eaae044768a4c4" -[[package]] -name = "xattr" -version = "1.6.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" -dependencies = [ - "libc", - "rustix", -] - [[package]] name = "yoke" version = "0.8.3" diff --git a/Cargo.toml b/Cargo.toml index 3c9c0b7..2356e91 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,8 +6,6 @@ repository = "https://github.com/jonasehrlich/octosync" [dependencies] anyhow = "1" -astral-tokio-tar = "0.6.4" -async-compression = { version = "0.4", features = ["gzip", "tokio"] } atomic-write-file = "0.3.1" bon = { version = "3.9" } chrono = { version = "0.4.44", features = ["serde"] } diff --git a/src/archiver.rs b/src/archiver.rs deleted file mode 100644 index e82c2dc..0000000 --- a/src/archiver.rs +++ /dev/null @@ -1,632 +0,0 @@ -use crate::store; -use anyhow::Context as _; -use std::path; -use tokio::fs; - -const ARCHIVE_DIR_PERMISSIONS: u32 = 0o700; -const ARCHIVE_FILE_PERMISSIONS: u32 = 0o600; -/// Number of archives kept per user, older archives are pruned after a successful archive -const MAX_ARCHIVES_PER_USER: usize = 3; - -/// Proof that a user's home directory was archived, or that there was nothing to archive. -/// It can only be obtained from [`archive_home_dir`], so destructive operations that take a -/// receipt can not be performed without archiving first. -#[must_use] -pub struct ArchiveReceipt { - /// Path of the created archive, `None` if the home directory did not exist - archive_path: Option, -} - -impl ArchiveReceipt { - /// Path of the created archive, `None` if the home directory did not exist - pub fn archive_path(&self) -> Option<&path::Path> { - self.archive_path.as_deref() - } -} - -/// Archive a user's home directory as a gzipped tarball in `archive_dir` before it is -/// deleted. The archive directory and the tarball have permissions 700 and 600 respectively -/// and are owned by root when running as root, so only root can access archived data. -/// -/// Entries that cannot be archived (sockets and other special files) or that disappear -/// while archiving are skipped, a failed archive blocks the user deletion forever. -/// Only the [`MAX_ARCHIVES_PER_USER`] most recent archives are kept per user. -/// -/// Returns an [`ArchiveReceipt`] carrying the path of the created archive, or no path if -/// the home directory does not exist. -#[tracing::instrument(name = "archive_home_dir", skip(archive_dir, user), fields(user = %user.name()))] -pub async fn archive_home_dir( - archive_dir: &path::Path, - user: &store::User, - home_dir: &path::Path, -) -> anyhow::Result { - use std::os::unix::fs::PermissionsExt as _; - - // Use symlink_metadata so a home directory that is itself a symlink is refused instead - // of silently archiving (and later deleting) whatever tree it points to - let metadata = match fs::symlink_metadata(home_dir).await { - Ok(metadata) => metadata, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::warn!( - "Home directory '{}' does not exist, nothing to archive", - home_dir.display() - ); - return Ok(ArchiveReceipt { archive_path: None }); - } - Err(e) => { - return Err(e).with_context(|| { - format!("Failed to read home directory '{}'", home_dir.display()) - }); - } - }; - if metadata.file_type().is_symlink() { - anyhow::bail!( - "Refusing to archive home directory '{}': it is a symlink", - home_dir.display() - ); - } - if !metadata.is_dir() { - anyhow::bail!( - "Refusing to archive home directory '{}': not a directory", - home_dir.display() - ); - } - if archive_dir.starts_with(home_dir) { - anyhow::bail!( - "Refusing to archive home directory '{}': it contains the archive directory '{}'", - home_dir.display(), - archive_dir.display() - ); - } - - let home_name = home_dir.file_name().ok_or_else(|| { - anyhow::anyhow!( - "Refusing to archive home directory '{}'", - home_dir.display() - ) - })?; - - let mut dir_builder = fs::DirBuilder::new(); - dir_builder - .recursive(true) - .mode(ARCHIVE_DIR_PERMISSIONS) - .create(archive_dir) - .await - .with_context(|| { - format!( - "Failed to create archive directory '{}'", - archive_dir.display() - ) - })?; - // The directory may have existed before with different permissions or ownership, - // enforce that only root can access it - fs::set_permissions( - archive_dir, - std::fs::Permissions::from_mode(ARCHIVE_DIR_PERMISSIONS), - ) - .await - .with_context(|| { - format!( - "Failed to set permissions on archive directory '{}'", - archive_dir.display() - ) - })?; - chown_to_root(archive_dir) - .context("Failed to change ownership of archive directory to root")?; - - let archive_path = archive_dir.join(format!( - "{}-{}-{}.tar.gz", - user.name(), - user.uid(), - chrono::Utc::now().format("%Y%m%dT%H%M%SZ") - )); - // Create the archive with permissions 600 from the start, so its content is never - // readable by other users. create_new fails if the archive already exists, so an - // existing archive is never overwritten, and the deletion is retried on the next sync - // with a new timestamp. - let file = fs::OpenOptions::new() - .write(true) - .create_new(true) - .mode(ARCHIVE_FILE_PERMISSIONS) - .open(&archive_path) - .await - .with_context(|| format!("Failed to create archive file '{}'", archive_path.display()))?; - - let result = async { - write_archive(file, home_name.as_ref(), home_dir).await?; - chown_to_root(&archive_path).context("Failed to change ownership of archive to root")?; - // Sync the directory entry as well, the archive must survive a crash once - // userdel removes the home directory - fs::File::open(archive_dir) - .await - .context("Failed to open archive directory for syncing")? - .sync_all() - .await - .context("Failed to sync archive directory")?; - Ok::<_, anyhow::Error>(()) - } - .await; - - if let Err(e) = result { - // Don't leave a partial or unsecured archive behind. The file was created by this - // call (create_new above), so it is safe to remove here. - let _ = fs::remove_file(&archive_path).await; - return Err(e) - .with_context(|| format!("Failed to archive home directory '{}'", home_dir.display())); - } - - prune_old_archives(archive_dir, user).await; - - Ok(ArchiveReceipt { - archive_path: Some(archive_path), - }) -} - -/// Write the gzipped tarball of `home_dir` to `file` and sync it to disk. Flushing alone -/// only reaches the page cache, so the file is fsynced before the caller deletes the -/// archived data. -async fn write_archive( - file: fs::File, - home_name: &path::Path, - home_dir: &path::Path, -) -> anyhow::Result<()> { - use tokio::io::AsyncWriteExt as _; - - // Buffer writes so the encoder output doesn't hit the blocking pool for every small - // chunk, and use the fastest compression level, home directories are dominated by - // already-compressed content - let writer = tokio::io::BufWriter::with_capacity(1 << 20, file); - let encoder = async_compression::tokio::write::GzipEncoder::with_quality( - writer, - async_compression::Level::Fastest, - ); - let mut builder = tokio_tar::Builder::new(encoder); - // Store symlinks as symlinks instead of following them out of the home directory - builder.follow_symlinks(false); - append_dir_recursive(&mut builder, home_name, home_dir).await?; - let mut encoder = builder - .into_inner() - .await - .context("Failed to finish tar archive")?; - encoder - .shutdown() - .await - .context("Failed to flush archive")?; - let file = encoder.into_inner().into_inner(); - file.sync_all() - .await - .context("Failed to sync archive to disk")?; - Ok(()) -} - -/// Append `src_root` recursively, storing entries under `dest_root`. Unlike -/// `Builder::append_dir_all`, entries that cannot be archived (sockets and other special -/// files) or that disappear while archiving are skipped with a warning instead of failing -/// the archive, because a failed archive blocks the user deletion until it succeeds. -async fn append_dir_recursive( - builder: &mut tokio_tar::Builder, - dest_root: &path::Path, - src_root: &path::Path, -) -> anyhow::Result<()> -where - W: tokio::io::AsyncWrite + Unpin + Send, -{ - builder - .append_path_with_name(src_root, dest_root) - .await - .with_context(|| format!("Failed to archive '{}'", src_root.display()))?; - - let mut stack = vec![src_root.to_path_buf()]; - while let Some(dir) = stack.pop() { - let mut entries = match fs::read_dir(&dir).await { - Ok(entries) => entries, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::warn!("Skipping '{}': removed while archiving", dir.display()); - continue; - } - Err(e) => { - return Err(e) - .with_context(|| format!("Failed to read directory '{}'", dir.display())); - } - }; - - while let Some(entry) = entries - .next_entry() - .await - .with_context(|| format!("Failed to read directory '{}'", dir.display()))? - { - let entry_path = entry.path(); - let dest = dest_root.join( - entry_path - .strip_prefix(src_root) - .expect("directory entry is below the source root"), - ); - let file_type = match entry.file_type().await { - Ok(file_type) => file_type, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::warn!( - "Skipping '{}': removed while archiving", - entry_path.display() - ); - continue; - } - Err(e) => { - return Err(e).with_context(|| { - format!("Failed to read file type of '{}'", entry_path.display()) - }); - } - }; - if !(file_type.is_file() || file_type.is_dir() || file_type.is_symlink()) { - tracing::warn!( - "Skipping '{}': sockets and other special files can not be archived", - entry_path.display() - ); - continue; - } - match builder.append_path_with_name(&entry_path, &dest).await { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::warn!( - "Skipping '{}': removed while archiving", - entry_path.display() - ); - continue; - } - Err(e) => { - return Err(e) - .with_context(|| format!("Failed to archive '{}'", entry_path.display())); - } - } - if file_type.is_dir() { - stack.push(entry_path); - } - } - } - Ok(()) -} - -/// Remove the oldest archives of a user, keeping the [`MAX_ARCHIVES_PER_USER`] most recent -/// ones. Pruning is best-effort: failures are logged and never fail the archiving. -async fn prune_old_archives(archive_dir: &path::Path, user: &store::User) { - let prefix = format!("{}-{}-", user.name(), user.uid()); - - let mut entries = match fs::read_dir(archive_dir).await { - Ok(entries) => entries, - Err(e) => { - tracing::warn!( - "Failed to read archive directory '{}' for pruning: {e}", - archive_dir.display() - ); - return; - } - }; - - let mut archives = Vec::new(); - loop { - match entries.next_entry().await { - Ok(Some(entry)) => { - let file_name = entry.file_name(); - let Some(file_name) = file_name.to_str() else { - continue; - }; - let Some(suffix) = file_name - .strip_prefix(&prefix) - .and_then(|rest| rest.strip_suffix(".tar.gz")) - else { - continue; - }; - if !is_archive_timestamp(suffix) { - continue; - } - archives.push(file_name.to_owned()); - } - Ok(None) => break, - Err(e) => { - tracing::warn!( - "Failed to read archive directory '{}' for pruning: {e}", - archive_dir.display() - ); - return; - } - } - } - - if archives.len() <= MAX_ARCHIVES_PER_USER { - return; - } - // Timestamped file names sort chronologically - archives.sort(); - for file_name in &archives[..archives.len() - MAX_ARCHIVES_PER_USER] { - let archive_path = archive_dir.join(file_name); - match fs::remove_file(&archive_path).await { - Ok(()) => tracing::info!("Pruned old archive '{}'", archive_path.display()), - Err(e) => tracing::warn!( - "Failed to prune old archive '{}': {e}", - archive_path.display() - ), - } - } -} - -/// Check whether the suffix of an archive file name is a timestamp formatted as -/// `%Y%m%dT%H%M%SZ`, so archives of a user whose name merely starts with another user's -/// name and uid are never pruned by that user. -fn is_archive_timestamp(suffix: &str) -> bool { - suffix.len() == 16 - && suffix.as_bytes()[8] == b'T' - && suffix.as_bytes()[15] == b'Z' - && suffix[..8].bytes().all(|b| b.is_ascii_digit()) - && suffix[9..15].bytes().all(|b| b.is_ascii_digit()) -} - -/// Change ownership of a path to root:root when running as root. When not running as root -/// (e.g. in tests), the path stays owned by the current user and remains inaccessible to -/// others through its permissions. -fn chown_to_root(path: &path::Path) -> anyhow::Result<()> { - if !nix::unistd::geteuid().is_root() { - tracing::debug!( - "Not running as root, '{}' stays owned by the current user", - path.display() - ); - return Ok(()); - } - nix::unistd::chown( - path, - Some(nix::unistd::Uid::from_raw(0)), - Some(nix::unistd::Gid::from_raw(0)), - )?; - Ok(()) -} - -#[cfg(test)] -mod tests { - use super::*; - use futures::StreamExt as _; - use std::collections; - use std::os::unix::fs::PermissionsExt as _; - - fn user() -> store::User { - store::User::builder() - .id(octocrab::models::UserId(12345)) - .name("testuser".to_string()) - .uid(nix::unistd::Uid::from_raw(1000)) - .build() - } - - async fn create_home(root: &path::Path) -> path::PathBuf { - let home = root.join("testuser"); - fs::create_dir(&home).await.expect("Failed to create home"); - fs::write(home.join("file.txt"), b"hello") - .await - .expect("Failed to write file"); - fs::create_dir(home.join(".ssh")) - .await - .expect("Failed to create .ssh"); - fs::write(home.join(".ssh").join("authorized_keys"), b"key") - .await - .expect("Failed to write authorized_keys"); - fs::symlink("/etc/passwd", home.join("link")) - .await - .expect("Failed to create symlink"); - home - } - - async fn read_entries( - archive_path: &path::Path, - ) -> collections::HashMap)> { - use tokio::io::AsyncReadExt as _; - - let file = fs::File::open(archive_path) - .await - .expect("Failed to open archive"); - let decoder = - async_compression::tokio::bufread::GzipDecoder::new(tokio::io::BufReader::new(file)); - let mut archive = tokio_tar::Archive::new(decoder); - let mut entries = archive.entries().expect("Failed to read entries"); - - let mut result = collections::HashMap::new(); - while let Some(entry) = entries.next().await { - let mut entry = entry.expect("Failed to read entry"); - let path = entry.path().expect("Invalid entry path").to_path_buf(); - let entry_type = entry.header().entry_type(); - let mut content = Vec::new(); - entry - .read_to_end(&mut content) - .await - .expect("Failed to read entry content"); - result.insert(path, (entry_type, content)); - } - result - } - - async fn archive_file_names(archive_dir: &path::Path) -> Vec { - let mut names = Vec::new(); - let mut entries = fs::read_dir(archive_dir) - .await - .expect("Failed to read archive dir"); - while let Some(entry) = entries.next_entry().await.expect("Failed to read entry") { - names.push(entry.file_name().to_string_lossy().into_owned()); - } - names.sort(); - names - } - - #[tokio::test] - async fn archives_home_directory_with_root_only_permissions() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let archive_dir = temp_dir.path().join("archive"); - - let receipt = archive_home_dir(&archive_dir, &user(), &home) - .await - .expect("Archiving failed"); - let archive_path = receipt - .archive_path() - .expect("Expected an archive to be created"); - - let dir_mode = fs::metadata(&archive_dir) - .await - .expect("Failed to read archive dir metadata") - .permissions() - .mode() - & 0o777; - assert_eq!(dir_mode, ARCHIVE_DIR_PERMISSIONS); - - let file_mode = fs::metadata(&archive_path) - .await - .expect("Failed to read archive metadata") - .permissions() - .mode() - & 0o777; - assert_eq!(file_mode, ARCHIVE_FILE_PERMISSIONS); - - let entries = read_entries(archive_path).await; - let (entry_type, content) = &entries[path::Path::new("testuser/file.txt")]; - assert_eq!(*entry_type, tokio_tar::EntryType::Regular); - assert_eq!(content, b"hello"); - - let (entry_type, content) = &entries[path::Path::new("testuser/.ssh/authorized_keys")]; - assert_eq!(*entry_type, tokio_tar::EntryType::Regular); - assert_eq!(content, b"key"); - - // Symlinks are stored as symlinks, their target content is not archived - let (entry_type, content) = &entries[path::Path::new("testuser/link")]; - assert_eq!(*entry_type, tokio_tar::EntryType::Symlink); - assert!(content.is_empty()); - } - - #[tokio::test] - async fn missing_home_dir_returns_none() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let archive_dir = temp_dir.path().join("archive"); - - let receipt = archive_home_dir(&archive_dir, &user(), &temp_dir.path().join("nonexistent")) - .await - .expect("Archiving failed"); - - assert!(receipt.archive_path().is_none()); - // No archive directory is created when there is nothing to archive - assert!(!archive_dir.exists()); - } - - #[tokio::test] - async fn symlinked_home_dir_is_refused() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let link = temp_dir.path().join("home_link"); - fs::symlink(&home, &link) - .await - .expect("Failed to create symlink"); - let archive_dir = temp_dir.path().join("archive"); - - let result = archive_home_dir(&archive_dir, &user(), &link).await; - - assert!(result.is_err()); - assert!(!archive_dir.exists()); - } - - #[tokio::test] - async fn home_dir_containing_archive_dir_is_refused() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let archive_dir = home.join("archive"); - - let result = archive_home_dir(&archive_dir, &user(), &home).await; - - assert!(result.is_err()); - assert!(!archive_dir.exists()); - } - - #[tokio::test] - async fn sockets_are_skipped() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let _listener = std::os::unix::net::UnixListener::bind(home.join("agent.sock")) - .expect("Failed to bind unix socket"); - let archive_dir = temp_dir.path().join("archive"); - - let receipt = archive_home_dir(&archive_dir, &user(), &home) - .await - .expect("Archiving failed"); - let archive_path = receipt - .archive_path() - .expect("Expected an archive to be created"); - - let entries = read_entries(archive_path).await; - assert!(!entries.contains_key(path::Path::new("testuser/agent.sock"))); - assert!(entries.contains_key(path::Path::new("testuser/file.txt"))); - } - - #[tokio::test] - async fn failed_archive_is_removed() { - if nix::unistd::geteuid().is_root() { - // Root can read the unreadable file, the failure can not be provoked - return; - } - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let unreadable = home.join("unreadable.txt"); - fs::write(&unreadable, b"secret") - .await - .expect("Failed to write file"); - fs::set_permissions(&unreadable, std::fs::Permissions::from_mode(0o000)) - .await - .expect("Failed to set permissions"); - let archive_dir = temp_dir.path().join("archive"); - - let result = archive_home_dir(&archive_dir, &user(), &home).await; - - assert!(result.is_err()); - // The partial archive is not left behind - assert!(archive_file_names(&archive_dir).await.is_empty()); - } - - #[tokio::test] - async fn old_archives_are_pruned() { - let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let home = create_home(temp_dir.path()).await; - let archive_dir = temp_dir.path().join("archive"); - fs::create_dir(&archive_dir) - .await - .expect("Failed to create archive dir"); - for name in [ - "testuser-1000-20200101T000000Z.tar.gz", - "testuser-1000-20200102T000000Z.tar.gz", - "testuser-1000-20200103T000000Z.tar.gz", - // Not a timestamp suffix, never pruned - "testuser-1000-keepme.tar.gz", - // Different user whose name and uid merely extend the prefix, never pruned - "testuser-1000-2-20200101T000000Z.tar.gz", - ] { - fs::write(archive_dir.join(name), b"") - .await - .expect("Failed to create old archive"); - } - - let receipt = archive_home_dir(&archive_dir, &user(), &home) - .await - .expect("Archiving failed"); - let archive_path = receipt - .archive_path() - .expect("Expected an archive to be created"); - - let names = archive_file_names(&archive_dir).await; - // The oldest archive is pruned, keeping the most recent MAX_ARCHIVES_PER_USER plus - // the two names that don't belong to this user's archives - assert!(!names.contains(&"testuser-1000-20200101T000000Z.tar.gz".to_string())); - assert!(names.contains(&"testuser-1000-20200102T000000Z.tar.gz".to_string())); - assert!(names.contains(&"testuser-1000-20200103T000000Z.tar.gz".to_string())); - assert!(names.contains(&"testuser-1000-keepme.tar.gz".to_string())); - assert!(names.contains(&"testuser-1000-2-20200101T000000Z.tar.gz".to_string())); - assert!( - names.contains( - &archive_path - .file_name() - .expect("Archive has a file name") - .to_string_lossy() - .into_owned() - ) - ); - assert_eq!(names.len(), 5); - } -} diff --git a/src/main.rs b/src/main.rs index d7d8321..c528cd5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -5,8 +5,6 @@ use clap::Parser as _; use fs2::FileExt as _; use std::{fs, path, sync}; -#[cfg_attr(not(target_os = "linux"), allow(dead_code))] -mod archiver; #[cfg_attr(not(target_os = "linux"), allow(dead_code))] mod authorized_keys; mod groups; @@ -120,6 +118,9 @@ struct Cli { command: Commands, } +/// Number of days an account stays expired before a purge removes it +const DEFAULT_PURGE_AFTER_DAYS: u32 = 180; + /// Arguments for the sync subcommand #[derive(clap::Args, Debug)] struct SyncArgs { @@ -130,14 +131,31 @@ struct SyncArgs { /// To map GitHub teams to Linux user groups use `--group :`. #[arg(long, value_parser = clap::value_parser!(groups::GroupMapping), verbatim_doc_comment)] group: Vec, + /// Days a departed member's account must have been expired before the purge at the + /// end of the sync removes the account and its home directory permanently + #[arg(long, default_value_t = DEFAULT_PURGE_AFTER_DAYS)] + purge_after_days: u32, +} + +/// Arguments for the purge subcommand +#[derive(clap::Args, Debug)] +struct PurgeArgs { + #[command(flatten)] + octocrab: InstallationClientArgs, + /// Days a departed member's account must have been expired before it is purged + #[arg(long, default_value_t = DEFAULT_PURGE_AFTER_DAYS)] + purge_after_days: u32, } #[derive(clap::Subcommand, Debug)] enum Commands { /// Synchronize GitHub organization members with Linux user accounts Sync(SyncArgs), - /// Delete all stored user data and Linux users created by octosync + /// Expire all octosync-managed Linux user accounts and record their departure Delete, + /// Permanently remove the accounts and home directories of members who departed + /// longer than the retention period ago + Purge(PurgeArgs), } #[tokio::main] @@ -187,6 +205,9 @@ async fn main() -> anyhow::Result<()> { Commands::Delete => { app.delete().await?; } + Commands::Purge(a) => { + app.purge(&a).await?; + } } Ok(()) } diff --git a/src/octosync.rs b/src/octosync.rs index 3935f8f..0686121 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -1,16 +1,12 @@ use crate::{ - GlobalArgs, InstallationClientArgs, SyncArgs, groups, public_keys, store, user_manager, + GlobalArgs, InstallationClientArgs, PurgeArgs, SyncArgs, groups, public_keys, store, + user_manager, }; use anyhow::Context as _; use futures::{StreamExt as _, stream}; use std::{collections, path, sync, time}; use tokio::fs; -/// Maximum number of user deletions that run concurrently. Each deletion archives the -/// user's home directory outside the user manager actor, which is disk- and CPU-heavy, -/// so it is not unbounded. Account mutations serialize on the actor. -const MAX_CONCURRENT_DELETES: usize = 4; - /// Maximum number of users processed concurrently during a sync. Bounds the GitHub /// requests in flight at the same time. The platform operations of each user are /// serialized by the user manager actor. @@ -74,7 +70,6 @@ impl Octosync { ) -> anyhow::Result { let user_manager = user_manager::UserManager::builder() .dry_run(global_config.dry_run) - .home_archive_dir(data_dir.join("home-archive")) .build(); Ok(Self { data_dir: data_dir.to_path_buf(), @@ -202,7 +197,7 @@ impl Octosync { if org_members.is_empty() && !old_store.data().is_empty() { anyhow::bail!( "Refusing to sync: org '{}' returned no members while {} users are stored. \ - Run the 'delete' command to remove all users intentionally.", + Run the 'delete' command to expire all users intentionally.", args.octocrab.org, old_store.data().len() ); @@ -223,7 +218,7 @@ impl Octosync { .process_members(&octocrab, &org_members, &old_store, &assignments) .await?; - let (users_to_retry, users_to_delete) = + let (users_to_retry, leavers) = partition_stale_users(old_store.data(), new_store.data(), &org_member_map); for user in users_to_retry { @@ -234,47 +229,124 @@ impl Octosync { new_store.data_mut().insert(user.id(), user.clone()); } - if would_delete_all_users(old_store.data(), &users_to_delete) { - for user in users_to_delete { + if would_expire_all_users(old_store.data(), &leavers) { + for user in leavers { new_store.data_mut().insert(user.id(), user.clone()); } new_store.save().await?; anyhow::bail!( - "Refusing to delete all {} stored users in a single sync. None of them is in \ + "Refusing to expire all {} stored users in a single sync. None of them is in \ the fetched member list of org '{}' ({} members). All users are kept in \ - the store; run the 'delete' command to remove them intentionally.", + the store; run the 'delete' command to expire them intentionally.", old_store.data().len(), args.octocrab.org, org_members.len() ); } - let users_to_delete: Vec = users_to_delete.into_iter().cloned().collect(); - self.archive_and_delete(&mut new_store, users_to_delete) + let leavers: Vec = leavers.into_iter().cloned().collect(); + self.depart_and_expire(&mut new_store, leavers).await?; + + self.purge_expired(&mut new_store, &org_member_map, args.purge_after_days) .await } - /// Archive the given users as tombstones and converge every archived account - /// toward deletion. + /// Purge departed users whose account has been expired for at least + /// `purge_after_days`, removing the account and its home directory permanently + /// and without an archive: the data is abandoned by policy. /// - /// The tombstones are saved before any account is removed, so the UID mapping - /// survives a crash anywhere in the deletion window. Every archived user whose - /// account still exists is re-enqueued here on each sync: a deletion interrupted - /// between the store save and `userdel` is finished by a later sync instead of - /// orphaning a live account, and a failed deletion needs no in-memory retry state. - async fn archive_and_delete( + /// Eligibility requires the store-side and the account-side clock to agree: the + /// tombstone's departure timestamp must be at least the retention period old, and + /// the account's own shadow expiry (checked in the backend, where it is cleared by + /// any reactivation) must be as well. A member present in the fetched member list + /// is never purged. The tombstone survives the purge in the purged map, so a + /// member rejoining even later gets their old IDs back. + async fn purge_expired( + &self, + store: &mut store::UserStore, + org_member_map: &collections::HashMap, + purge_after_days: u32, + ) -> anyhow::Result<()> { + let now = chrono::Utc::now(); + let cutoff = now - chrono::Duration::days(purge_after_days.into()); + // The shadow expiry field counts days since the epoch + let expired_before = cutoff.timestamp() / (24 * 60 * 60); + + let candidates: Vec = store + .departed() + .values() + .filter(|tombstone| { + tombstone.departed_at() <= cutoff && !org_member_map.contains_key(&tombstone.id()) + }) + .map(store::User::from) + .collect(); + + for user in candidates { + match self.user_manager.purge_user(&user, expired_before).await { + Ok(user_manager::PurgeOutcome::Purged) => { + tracing::info!( + user = %user.name(), + uid = user.uid().as_raw(), + "Purged the expired account after the retention period" + ); + store.mark_purged(&user.id(), now); + } + // The account is gone but only the purge may spend the tombstone, so + // it stays departed; an operator resolves how the account disappeared + Ok(user_manager::PurgeOutcome::NoAccount) => tracing::warn!( + "No account for departed user '{}', not marking the tombstone purged", + user.name() + ), + Ok(user_manager::PurgeOutcome::NotExpired) => tracing::warn!( + "Account of '{}' has no shadow expiry older than the retention period, \ + not purging despite the tombstone age", + user.name() + ), + // The tombstone stays departed, so the purge is retried on later syncs + Err(e) => { + tracing::error!("Failed to purge the account of '{}': {:?}", user.name(), e); + } + } + } + store.save().await + } + + /// Record the given users as departed and converge every departed account toward + /// its expiry. + /// + /// The tombstones are saved before any account is expired, so the departure record + /// survives a crash anywhere in the expiry window. Every departed user is + /// re-expired on each sync: [`user_manager::ExpireAccount`] is idempotent, so an + /// expiry interrupted between the store save and the platform call is finished by + /// a later sync and a failed expiry needs no in-memory retry state. + async fn depart_and_expire( &self, store: &mut store::UserStore, leavers: Vec, ) -> anyhow::Result<()> { let departed_at = chrono::Utc::now(); for user in leavers { - tracing::info!("Archiving user '{}' for deletion", user.name()); + tracing::info!(user = %user.name(), "Recording departure, expiring the account"); store.depart_user(user, departed_at); } store.save().await?; - self.delete_archived_accounts(store.departed()).await; + // Failures are only logged: the tombstone stays in the store, so the expiry is + // retried on every sync. Sequential, the platform operations serialize on the + // user manager actor anyway. + for departed_user in store.departed().values() { + if let Err(e) = self + .user_manager + .expire_user(&store::User::from(departed_user)) + .await + { + tracing::error!( + "Failed to expire the account of '{}': {:?}", + departed_user.name(), + e + ); + } + } Ok(()) } @@ -283,7 +355,7 @@ impl Octosync { /// /// A processing failure is logged and leaves the user out of the returned store. /// It never signals that the user left the org; [`partition_stale_users`] alone - /// decides which users are deleted. + /// decides which users depart. async fn process_members( &self, octocrab: &octocrab::Octocrab, @@ -319,26 +391,6 @@ impl Octosync { Ok(new_store) } - /// Delete the platform accounts of the archived users concurrently. - /// - /// Failures are only logged: the tombstone stays in the store, so the deletion is - /// retried on every sync until the account is gone. - async fn delete_archived_accounts(&self, departed: &store::DepartedMap) { - stream::iter(departed.values()) - // Deletion archives the user's home directory, which can take a while for large - // home directories, run a few deletions concurrently - .for_each_concurrent(MAX_CONCURRENT_DELETES, |departed_user| async move { - if let Err(e) = self - .user_manager - .delete_user(&store::User::from(departed_user)) - .await - { - tracing::error!("Failed to delete user '{}': {:?}", departed_user.name(), e); - } - }) - .await - } - #[tracing::instrument(name = "Octosync::delete", skip(self))] pub async fn delete(&self) -> anyhow::Result<()> { let mut store = store::UserStore::from_dir(&self.data_dir).await?; @@ -346,21 +398,45 @@ impl Octosync { let count = users.len(); // The store file is kept: the tombstones preserve every UID so members // re-created by a later sync get their old UID back - self.archive_and_delete(&mut store, users).await?; + self.depart_and_expire(&mut store, users).await?; tracing::info!( - "Archived {count} users for deletion, keeping their tombstones in the store" + "Recorded {count} departures and expired their accounts, keeping the tombstones \ + in the store" ); Ok(()) } + + /// The explicit counterpart of the purge pass at the end of every sync, for + /// running the one deliberately irreversible action with a human in the loop. + /// The member list is fetched so a rejoined member is never purged. + #[tracing::instrument( + name = "Octosync::purge", + skip(self, args), + fields(org = %args.octocrab.org) + )] + pub async fn purge(&self, args: &PurgeArgs) -> anyhow::Result<()> { + let octocrab = org_client(&args.octocrab).await?; + let (org_members, mut store) = tokio::try_join!( + get_all_org_members(&octocrab, &args.octocrab.org), + store::UserStore::from_dir(&self.data_dir), + )?; + let org_member_map: collections::HashMap = + collections::HashMap::from_iter( + org_members.iter().map(|user| (user.id, user.login.clone())), + ); + self.purge_expired(&mut store, &org_member_map, args.purge_after_days) + .await + } } /// Split the users of the previous store that are missing from the new store into -/// users to keep for a retry and users to delete. +/// users to keep for a retry and leavers whose account is expired. /// -/// Deletion is decided solely by absence from the fetched org member list. A user that -/// is still an org member but missing from the new store failed processing; deleting -/// them would turn any correlated per-user failure (e.g. rate-limited key fetches) -/// into a mass deletion, so they are kept unchanged and retried on the next sync. +/// Departure is decided solely by absence from the fetched org member list. A user +/// that is still an org member but missing from the new store failed processing; +/// expiring them would turn any correlated per-user failure (e.g. rate-limited key +/// fetches) into a mass lockout, so they are kept unchanged and retried on the next +/// sync. fn partition_stale_users<'a>( old_users: &'a collections::HashMap, new_users: &collections::HashMap, @@ -372,17 +448,17 @@ fn partition_stale_users<'a>( .partition(|user| org_member_map.contains_key(&user.id())) } -/// Circuit breaker for [`Octosync::sync`]. True when the pending deletions would empty -/// a non-empty store. +/// Circuit breaker for [`Octosync::sync`]. True when the pending departures would +/// empty a non-empty store. /// /// A member list that no longer contains a single stored user is far more likely a /// mis-scoped installation, an org rename or an empty-but-successful API response than -/// every member leaving at once, so such a sync must refuse to delete anyone. -fn would_delete_all_users( +/// every member leaving at once, so such a sync must refuse to expire anyone. +fn would_expire_all_users( old_users: &collections::HashMap, - users_to_delete: &[&store::User], + leavers: &[&store::User], ) -> bool { - !users_to_delete.is_empty() && users_to_delete.len() == old_users.len() + !leavers.is_empty() && leavers.len() == old_users.len() } async fn get_all_org_members( @@ -430,9 +506,7 @@ mod tests { mod orchestration { use super::*; - use crate::user_manager::{ - DeletionPreparation, UserManager, backends::testing::TestingUserManager, - }; + use crate::user_manager::{UserManager, backends::testing::TestingUserManager}; /// A GitHub user as returned by the API. `Author` is non-exhaustive, so it can /// only be built through deserialization. @@ -476,11 +550,11 @@ mod tests { /// An [`Octosync`] backed by the scripted user manager. The [`tempfile::TempDir`] /// is its data directory and must outlive it. - fn octosync_with(actor: TestingUserManager) -> (Octosync, tempfile::TempDir) { + pub(super) fn octosync_with(actor: TestingUserManager) -> (Octosync, tempfile::TempDir) { let data_dir = tempfile::tempdir().unwrap(); let octosync = Octosync { data_dir: data_dir.path().to_path_buf(), - user_manager: UserManager::testing(actor, data_dir.path().join("home-archive")), + user_manager: UserManager::testing(actor), }; (octosync, data_dir) } @@ -696,20 +770,18 @@ mod tests { ); } - /// A leaver is tombstoned and the store saved before the account is removed, - /// so a removal failure leaves a durable tombstone instead of in-memory + /// A leaver is tombstoned and the store saved before the account is expired, + /// so an expiry failure leaves a durable tombstone instead of in-memory /// retry state. #[tokio::test] - async fn archive_and_delete_keeps_the_tombstone_when_removal_fails() { + async fn depart_and_expire_keeps_the_tombstone_when_expiry_fails() { let mut actor = TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Err(anyhow::anyhow!("boom"))); + actor.expire_account.push_back(Err(anyhow::anyhow!("boom"))); let (octosync, data_dir) = octosync_with(actor); let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); octosync - .archive_and_delete(&mut store, vec![user(1, "alice")]) + .depart_and_expire(&mut store, vec![user(1, "alice")]) .await .unwrap(); @@ -719,14 +791,32 @@ mod tests { assert!(saved.data().is_empty()); } + /// A departed user is re-expired on every sync, so an expiry that failed or + /// was interrupted converges without in-memory retry state. + #[tokio::test] + async fn depart_and_expire_re_expires_existing_tombstones() { + let mut actor = TestingUserManager::default(); + actor.expire_account.push_back(Ok(())); + let expired_users = actor.expired_users.clone(); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), chrono::Utc::now()); + + // No leavers this sync, the existing tombstone alone drives the expiry + octosync + .depart_and_expire(&mut store, vec![]) + .await + .unwrap(); + + assert_eq!(*expired_users.lock().unwrap(), ["alice"]); + } + /// The `delete` command tombstones every user instead of removing the store /// file, so the UID memory survives a full wipe. #[tokio::test] async fn delete_command_keeps_the_store_file_with_tombstones() { let mut actor = TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::NothingToDo)); + actor.expire_account.push_back(Ok(())); let (octosync, data_dir) = octosync_with(actor); let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); store @@ -745,54 +835,186 @@ mod tests { } } + mod purge { + use super::orchestration::octosync_with; + use super::*; + use crate::user_manager::{PurgeOutcome, backends::testing::TestingUserManager}; + + const RETENTION_DAYS: u32 = 180; + + /// A departure timestamp safely past the retention period + fn old_departure() -> chrono::DateTime { + chrono::Utc::now() - chrono::Duration::days(RETENTION_DAYS as i64 + 30) + } + + #[tokio::test] + async fn purges_a_tombstone_older_than_the_retention_period() { + let mut actor = TestingUserManager::default(); + actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), old_departure()); + + octosync + .purge_expired(&mut store, &member_map(&[]), RETENTION_DAYS) + .await + .unwrap(); + + assert!(store.departed().is_empty()); + // The tombstone survives the purge and is saved, so the UID stays + // reserved for a rejoin even after the purge + let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); + let purged = &saved.purged()[&octocrab::models::UserId(1)]; + assert_eq!(purged.name(), "alice"); + } + + /// A tombstone younger than the retention period is not purged: the scripted + /// Purged response would move it if the message were sent. + #[tokio::test] + async fn young_tombstone_is_not_purged() { + let mut actor = TestingUserManager::default(); + actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), chrono::Utc::now()); + + octosync + .purge_expired(&mut store, &member_map(&[]), RETENTION_DAYS) + .await + .unwrap(); + + assert!(store.departed().contains_key(&octocrab::models::UserId(1))); + assert!(store.purged().is_empty()); + } + + /// A member present in the fetched member list is never purged, however old + /// their tombstone is. + #[tokio::test] + async fn current_org_member_is_not_purged() { + let mut actor = TestingUserManager::default(); + actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let alice = user(1, "alice"); + store.depart_user(alice.clone(), old_departure()); + + octosync + .purge_expired(&mut store, &member_map(&[alice]), RETENTION_DAYS) + .await + .unwrap(); + + assert!(store.departed().contains_key(&octocrab::models::UserId(1))); + assert!(store.purged().is_empty()); + } + + /// The account-side clock disagrees with the tombstone: the backend reports + /// the account as not expired long enough, the tombstone stays departed. + #[tokio::test] + async fn account_side_disagreement_keeps_the_tombstone_departed() { + let mut actor = TestingUserManager::default(); + actor.purge_account.push_back(Ok(PurgeOutcome::NotExpired)); + actor.purge_account.push_back(Ok(PurgeOutcome::NoAccount)); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), old_departure()); + store.depart_user(user(2, "bob"), old_departure()); + + octosync + .purge_expired(&mut store, &member_map(&[]), RETENTION_DAYS) + .await + .unwrap(); + + assert_eq!(store.departed().len(), 2); + assert!(store.purged().is_empty()); + } + + /// A failed purge keeps the tombstone departed, so it is retried on the next + /// sync. + #[tokio::test] + async fn failed_purge_keeps_the_tombstone_departed() { + let mut actor = TestingUserManager::default(); + actor.purge_account.push_back(Err(anyhow::anyhow!("boom"))); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), old_departure()); + + octosync + .purge_expired(&mut store, &member_map(&[]), RETENTION_DAYS) + .await + .unwrap(); + + assert!(store.departed().contains_key(&octocrab::models::UserId(1))); + assert!(store.purged().is_empty()); + } + + /// A purged tombstone is left alone by the expiry reconciliation: the two + /// rules must not fight over an entry. + #[tokio::test] + async fn purged_tombstone_is_not_re_expired() { + let actor = TestingUserManager::default(); + let expired_users = actor.expired_users.clone(); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + store.depart_user(user(1, "alice"), old_departure()); + store.mark_purged(&octocrab::models::UserId(1), chrono::Utc::now()); + + octosync + .depart_and_expire(&mut store, vec![]) + .await + .unwrap(); + + assert!(expired_users.lock().unwrap().is_empty()); + } + } + mod partition_stale_users { use super::*; /// Regression test for the 2026-08-12 incident: every member is still in the /// org, but all of them failed processing, so the new store is empty. - /// No user may be deleted; all must be retried. + /// No user may be expired; all must be retried. #[test] - fn all_members_failed_processing_deletes_nobody() { + fn all_members_failed_processing_expires_nobody() { let users = [user(1, "a"), user(2, "b"), user(3, "c")]; let old = user_map(&users); let new = user_map(&[]); let members = member_map(&users); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert_eq!(retry.len(), 3); - assert!(delete.is_empty()); + assert!(leavers.is_empty()); } #[test] - fn user_absent_from_member_list_is_deleted() { + fn user_absent_from_member_list_is_expired() { let remaining = user(1, "a"); let left = user(2, "b"); let old = user_map(&[remaining.clone(), left.clone()]); let new = user_map(&[remaining]); let members = member_map(std::slice::from_ref(&old[&octocrab::models::UserId(1)])); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert!(retry.is_empty()); - assert_eq!(delete, vec![&left]); + assert_eq!(leavers, vec![&left]); } #[test] - fn successfully_processed_user_is_neither_retried_nor_deleted() { + fn successfully_processed_user_is_neither_retried_nor_expired() { let processed = user(1, "a"); let old = user_map(std::slice::from_ref(&processed)); let new = user_map(std::slice::from_ref(&processed)); let members = member_map(&[processed]); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert!(retry.is_empty()); - assert!(delete.is_empty()); + assert!(leavers.is_empty()); } #[test] - fn failed_member_is_retried_while_removed_member_is_deleted() { + fn failed_member_is_retried_while_removed_member_is_expired() { let processed = user(1, "a"); let failed = user(2, "b"); let left = user(3, "c"); @@ -800,26 +1022,26 @@ mod tests { let new = user_map(std::slice::from_ref(&processed)); let members = member_map(&[processed, failed.clone()]); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert_eq!(retry, vec![&failed]); - assert_eq!(delete, vec![&left]); + assert_eq!(leavers, vec![&left]); } /// The successful-but-empty member list: `sync()` bails on this before /// processing, but the circuit breaker must still trip as defense in depth. #[test] - fn empty_member_list_puts_all_users_in_delete_bucket_and_trips_guard() { + fn empty_member_list_puts_all_users_in_leaver_bucket_and_trips_guard() { let users = [user(1, "a"), user(2, "b")]; let old = user_map(&users); let new = user_map(&[]); let members = member_map(&[]); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert!(retry.is_empty()); - assert_eq!(delete.len(), 2); - assert!(would_delete_all_users(&old, &delete)); + assert_eq!(leavers.len(), 2); + assert!(would_expire_all_users(&old, &leavers)); } #[test] @@ -829,50 +1051,50 @@ mod tests { let new = user_map(std::slice::from_ref(&joined)); let members = member_map(&[joined]); - let (retry, delete) = partition_stale_users(&old, &new, &members); + let (retry, leavers) = partition_stale_users(&old, &new, &members); assert!(retry.is_empty()); - assert!(delete.is_empty()); + assert!(leavers.is_empty()); } } - mod deletes_entire_store { + mod expires_entire_store { use super::*; #[test] - fn deleting_every_stored_user_trips_the_guard() { + fn expiring_every_stored_user_trips_the_guard() { let users = [user(1, "a"), user(2, "b")]; let old = user_map(&users); - let delete: Vec<&store::User> = old.values().collect(); + let leavers: Vec<&store::User> = old.values().collect(); - assert!(would_delete_all_users(&old, &delete)); + assert!(would_expire_all_users(&old, &leavers)); } #[test] - fn deleting_a_subset_is_allowed() { + fn expiring_a_subset_is_allowed() { let users = [user(1, "a"), user(2, "b")]; let old = user_map(&users); - let delete = vec![&old[&octocrab::models::UserId(1)]]; + let leavers = vec![&old[&octocrab::models::UserId(1)]]; - assert!(!would_delete_all_users(&old, &delete)); + assert!(!would_expire_all_users(&old, &leavers)); } #[test] - fn nothing_to_delete_is_allowed() { + fn nothing_to_expire_is_allowed() { let old = user_map(&[user(1, "a")]); - assert!(!would_delete_all_users(&old, &[])); + assert!(!would_expire_all_users(&old, &[])); } /// A single-user store where that user really left still trips the guard; /// the explicit delete command is the intentional path for this case. #[test] - fn deleting_the_only_stored_user_trips_the_guard() { + fn expiring_the_only_stored_user_trips_the_guard() { let only = user(1, "a"); let old = user_map(std::slice::from_ref(&only)); - let delete: Vec<&store::User> = old.values().collect(); + let leavers: Vec<&store::User> = old.values().collect(); - assert!(would_delete_all_users(&old, &delete)); + assert!(would_expire_all_users(&old, &leavers)); } } } diff --git a/src/store.rs b/src/store.rs index 1deac13..5c71ad9 100644 --- a/src/store.rs +++ b/src/store.rs @@ -317,6 +317,28 @@ impl UserStore { ); } + /// Move a departed tombstone to the purged map, keeping its IDs for a rejoin even + /// after the purge + pub fn mark_purged( + &mut self, + id: &octocrab::models::UserId, + purged_at: chrono::DateTime, + ) { + if let Some(departed) = self.departed.remove(id) { + self.purged.insert( + departed.id, + PurgedUser { + id: departed.id, + name: departed.name, + uid: departed.uid, + gid: departed.gid, + departed_at: departed.departed_at, + purged_at, + }, + ); + } + } + /// Drop the tombstones of users that are active again, keeping every user in /// exactly one of the three maps pub fn prune_rejoined(&mut self) { @@ -330,7 +352,7 @@ impl UserStore { } /// Load the store from the file system, starting empty if the file doesn't exist. - /// A v1 file is backed up once and migrated with an empty archived map. + /// A v1 file is backed up once and migrated with empty tombstone maps. #[tracing::instrument(name = "Store::load", skip(self))] async fn load(&mut self) -> anyhow::Result<()> { let path = self.path(); @@ -752,6 +774,25 @@ mod tests { assert_eq!(departed.departed_at, departed_at()); } + #[tokio::test] + async fn mark_purged_moves_a_departed_tombstone_to_the_purged_map() { + let temp_dir = tempfile::TempDir::new().unwrap(); + let mut store = UserStore::new(temp_dir.path()).await.unwrap(); + let departed = departed_user(); + store.departed.insert(departed.id, departed.clone()); + let purged_at = chrono::Utc::now(); + + store.mark_purged(&departed.id, purged_at); + + assert!(store.departed.is_empty()); + let purged = &store.purged[&departed.id]; + assert_eq!(purged.name, departed.name); + assert_eq!(purged.uid, departed.uid); + assert_eq!(purged.gid, departed.gid); + assert_eq!(purged.departed_at, departed.departed_at); + assert_eq!(purged.purged_at, purged_at); + } + #[tokio::test] async fn prune_rejoined_drops_tombstones_of_active_users() { let temp_dir = tempfile::TempDir::new().unwrap(); diff --git a/src/user_manager.rs b/src/user_manager.rs index 527a3c7..1ac27a3 100644 --- a/src/user_manager.rs +++ b/src/user_manager.rs @@ -8,21 +8,17 @@ //! when another invocation holds the lock, so the concurrent per-user syncs must never //! run them in parallel. //! -//! Deletion is split so the disk-heavy work stays concurrent: the actor prepares the -//! deletion and removes the account, while the home directory archiving between the -//! two runs outside the actor in the caller's task. - pub mod backends; mod messages; pub use messages::{ - AccountIds, CreateUser, DeletionPreparation, EnsureGroupsExist, PrepareUserDeletion, - RemoveAccount, SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, + AccountIds, CreateUser, EnsureGroupsExist, ExpireAccount, PurgeAccount, PurgeOutcome, + SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, }; use crate::{public_keys, store}; use anyhow::Context as _; -use std::{collections, path}; +use std::collections; /// Handle to a spawned platform user manager actor. /// @@ -30,12 +26,10 @@ use std::{collections, path}; /// one-message-at-a-time serialization holds across every user of the handle. #[derive(Clone)] pub struct UserManager { - /// Directory where home directories of deleted users are archived - home_archive_dir: path::PathBuf, create_user: hannibal::Caller, update_user: hannibal::Caller, - prepare_user_deletion: hannibal::Caller, - remove_account: hannibal::Caller, + expire_account: hannibal::Caller, + purge_account: hannibal::Caller, sync_supplementary_groups: hannibal::Caller, ensure_groups_exist: hannibal::Caller, update_authorized_keys: hannibal::Caller, @@ -47,23 +41,21 @@ const ACTOR_ERROR: &str = "User manager actor not available or failed to process impl UserManager { #[builder] pub fn new( - /// Directory where home directories of deleted users are archived - home_archive_dir: path::PathBuf, /// Preview actions without changing users, groups or files on the system dry_run: bool, ) -> Self { if dry_run { - return Self::from_actor(backends::mock::MockUserManager::new(1000), home_archive_dir); + return Self::from_actor(backends::mock::MockUserManager::new(1000)); } #[cfg(target_os = "linux")] { - Self::from_actor(backends::linux::LinuxUserManager::new(), home_archive_dir) + Self::from_actor(backends::linux::LinuxUserManager::new()) } #[cfg(not(target_os = "linux"))] { - Self::from_actor(backends::mock::MockUserManager::new(1000), home_archive_dir) + Self::from_actor(backends::mock::MockUserManager::new(1000)) } } } @@ -71,12 +63,12 @@ impl UserManager { impl UserManager { /// Spawn the actor and keep one caller per message type, erasing the concrete /// actor type from the handle. - fn from_actor(actor: A, home_archive_dir: path::PathBuf) -> Self + fn from_actor(actor: A) -> Self where A: hannibal::Handler + hannibal::Handler - + hannibal::Handler - + hannibal::Handler + + hannibal::Handler + + hannibal::Handler + hannibal::Handler + hannibal::Handler + hannibal::Handler, @@ -84,11 +76,10 @@ impl UserManager { use hannibal::spawnable::Spawnable as _; let addr = actor.spawn(); Self { - home_archive_dir, create_user: addr.caller(), update_user: addr.caller(), - prepare_user_deletion: addr.caller(), - remove_account: addr.caller(), + expire_account: addr.caller(), + purge_account: addr.caller(), sync_supplementary_groups: addr.caller(), ensure_groups_exist: addr.caller(), update_authorized_keys: addr.caller(), @@ -127,50 +118,29 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Deletes the platform user of `user`, returning the path of the created home - /// directory archive, or `None` when there was no account or nothing to archive. - /// - /// The actor prepares the deletion ([`PrepareUserDeletion`]) and removes the - /// account ([`RemoveAccount`]); the disk- and CPU-heavy home directory archiving - /// between the two runs here, outside the actor, so concurrent deletions only - /// serialize on the account mutations themselves. A failed archive aborts the - /// deletion, the caller keeps the user's tombstone and retries on the next sync. - #[tracing::instrument( - name = "UserManager::delete_user", - skip_all, - fields(user = %user.name(), uid = user.uid().as_raw()) - )] - pub async fn delete_user(&self, user: &store::User) -> anyhow::Result> { - let preparation = self - .prepare_user_deletion - .call(PrepareUserDeletion { user: user.clone() }) + /// Sends [`ExpireAccount`] to the actor and awaits the expiry, the reversible + /// replacement for deleting the account. + pub async fn expire_user(&self, user: &store::User) -> anyhow::Result<()> { + self.expire_account + .call(ExpireAccount { user: user.clone() }) .await - .context(ACTOR_ERROR)??; - - let DeletionPreparation::Prepared { home_dir } = preparation else { - return Ok(None); - }; - - let receipt = crate::archiver::archive_home_dir(&self.home_archive_dir, user, &home_dir) - .await - .context("Home directory was not archived, not deleting user")?; - let archive_path = receipt.archive_path().map(path::Path::to_path_buf); - if let Some(archive_path) = &archive_path { - tracing::info!( - home_dir = %home_dir.display(), - archive_path = %archive_path.display(), - "Archived home directory", - ); - } + .context(ACTOR_ERROR)? + } - self.remove_account - .call(RemoveAccount { + /// Sends [`PurgeAccount`] to the actor and awaits the outcome. `expired_before` is + /// the account-side eligibility bound, see [`PurgeAccount`]. + pub async fn purge_user( + &self, + user: &store::User, + expired_before: i64, + ) -> anyhow::Result { + self.purge_account + .call(PurgeAccount { user: user.clone(), - receipt, + expired_before, }) .await - .context(ACTOR_ERROR)??; - Ok(archive_path) + .context(ACTOR_ERROR)? } /// Sends [`SyncSupplementaryGroups`] to the actor and awaits the update. @@ -218,11 +188,8 @@ impl UserManager { impl UserManager { /// Build a manager backed by a [`backends::testing::TestingUserManager`], so /// tests can mock the response of every operation. - pub(crate) fn testing( - actor: backends::testing::TestingUserManager, - home_archive_dir: path::PathBuf, - ) -> Self { - Self::from_actor(actor, home_archive_dir) + pub(crate) fn testing(actor: backends::testing::TestingUserManager) -> Self { + Self::from_actor(actor) } } @@ -266,23 +233,21 @@ mod tests { #[tokio::test] async fn responses_are_returned_in_order() { let mut actor = backends::testing::TestingUserManager::default(); + actor.expire_account.push_back(Ok(())); actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::NothingToDo)); - actor - .prepare_user_deletion + .expire_account .push_back(Err(anyhow::anyhow!("scripted failure"))); - let manager = UserManager::testing(actor, path::PathBuf::new()); + let manager = UserManager::testing(actor); let user = test_user(1, "alice"); - manager.delete_user(&user).await.unwrap(); - let err = manager.delete_user(&user).await.unwrap_err(); + manager.expire_user(&user).await.unwrap(); + let err = manager.expire_user(&user).await.unwrap_err(); assert!(err.to_string().contains("scripted failure")); } #[tokio::test] async fn exhausted_script_fails_with_the_message_name() { - let manager = UserManager::testing(Default::default(), path::PathBuf::new()); + let manager = UserManager::testing(Default::default()); let err = manager .ensure_groups_exist(&["developers".to_string()]) @@ -292,55 +257,6 @@ mod tests { } } - mod deletion_flow { - use super::*; - - #[tokio::test] - async fn archives_the_home_directory_before_removing_the_account() { - let home_dir = tempfile::tempdir().unwrap(); - std::fs::write(home_dir.path().join("notes.txt"), "important").unwrap(); - let archive_dir = tempfile::tempdir().unwrap(); - - let mut actor = backends::testing::TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::Prepared { - home_dir: home_dir.path().to_path_buf(), - })); - actor.remove_account.push_back(Ok(())); - let manager = UserManager::testing(actor, archive_dir.path().to_path_buf()); - - manager.delete_user(&test_user(1, "alice")).await.unwrap(); - - // RemoveAccount was consumed from the script, and the archive exists - let archives = std::fs::read_dir(archive_dir.path()).unwrap().count(); - assert!(archives > 0, "expected an archive to be created"); - } - - #[tokio::test] - async fn failed_archive_aborts_the_deletion() { - // A home directory that is a plain file makes the archiver refuse - let bogus_home = tempfile::NamedTempFile::new().unwrap(); - let archive_dir = tempfile::tempdir().unwrap(); - - let mut actor = backends::testing::TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::Prepared { - home_dir: bogus_home.path().to_path_buf(), - })); - // No RemoveAccount response is scripted: reaching it would fail with - // "No scripted response left", so the assertion below proves it is never sent - let manager = UserManager::testing(actor, archive_dir.path().to_path_buf()); - - let err = manager - .delete_user(&test_user(1, "alice")) - .await - .unwrap_err(); - assert!(err.to_string().contains("not deleting user")); - } - } - mod serialization { use super::*; use std::sync::{self, atomic}; @@ -362,14 +278,14 @@ mod tests { impl hannibal::Actor for OverlapProbe {} - impl hannibal::Handler for OverlapProbe { + impl hannibal::Handler for OverlapProbe { async fn handle( &mut self, _ctx: &mut hannibal::Context, - _msg: PrepareUserDeletion, - ) -> anyhow::Result { + _msg: ExpireAccount, + ) -> anyhow::Result<()> { self.probe().await; - Ok(DeletionPreparation::NothingToDo) + Ok(()) } } @@ -406,13 +322,13 @@ mod tests { } } - impl hannibal::Handler for OverlapProbe { + impl hannibal::Handler for OverlapProbe { async fn handle( &mut self, _ctx: &mut hannibal::Context, - _msg: RemoveAccount, - ) -> anyhow::Result<()> { - Ok(()) + _msg: PurgeAccount, + ) -> anyhow::Result { + Ok(PurgeOutcome::NoAccount) } } @@ -443,13 +359,10 @@ mod tests { async fn concurrent_operations_never_overlap() { let active = sync::Arc::new(atomic::AtomicUsize::new(0)); let max_active = sync::Arc::new(atomic::AtomicUsize::new(0)); - let manager = UserManager::from_actor( - OverlapProbe { - active: active.clone(), - max_active: max_active.clone(), - }, - path::PathBuf::new(), - ); + let manager = UserManager::from_actor(OverlapProbe { + active: active.clone(), + max_active: max_active.clone(), + }); let user = test_user(1, "alice"); let calls = (0..8).map(|i| { @@ -457,7 +370,7 @@ mod tests { let user = &user; async move { if i % 2 == 0 { - manager.delete_user(user).await.map(|_| ()) + manager.expire_user(user).await } else { manager.sync_supplementary_groups(user, &[]).await } diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index 6723fa0..c414e1f 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -5,9 +5,8 @@ use crate::store; use crate::user_manager::{ - AccountIds, CreateUser, DeletionPreparation, EnsureGroupsExist, PrepareUserDeletion, - RemoveAccount, SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, - supplementary_groups_update, + AccountIds, CreateUser, EnsureGroupsExist, ExpireAccount, PurgeAccount, PurgeOutcome, + SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, supplementary_groups_update, }; use anyhow::Context as _; use std::{collections, time}; @@ -48,9 +47,9 @@ impl hannibal::Handler for LinuxUserManager { "User already exists. Skipping creation." ); - // The adopted account may carry the expiry of a failed deletion whose - // store entry is gone, e.g. after the delete command wiped the store - clear_deletion_expiry(&existing_user.name).await?; + // The adopted account is usually a rejoining member's expired account: + // lifting the expiry is what turns the departure back into an active user + clear_departure_expiry(&existing_user.name).await?; return Ok(store::User::builder() .id(user.id) @@ -129,10 +128,10 @@ const FREE_ID_ATTEMPTS: u32 = 10_000; /// Create an account with system-allocated IDs, re-creating it with explicitly chosen /// free IDs when the allocation hands out an ID a tombstone reserves. /// -/// shadow-utils allocates the highest ID in range plus one, so deleting the user with +/// shadow-utils allocates the highest ID in range plus one, so purging the user with /// the highest UID frees exactly the next UID `useradd` hands out: without this check -/// the next member to join would routinely receive the most recently departed user's -/// UID and inherit ownership of their files outside the archived home directory. +/// the next member to join would routinely receive the most recently purged user's +/// UID and inherit ownership of their files outside the removed home directory. async fn add_account_with_fresh_ids( login: &str, reserved_uids: &collections::HashSet, @@ -202,7 +201,7 @@ fn free_id_from( } /// Create a platform account with `useradd` and return it. Re-creating a previously -/// deleted account with its stored UID and private-group GID keeps ownership of files +/// removed account with its stored UID and private-group GID keeps ownership of files /// outside the home directory intact. async fn add_account( login: &str, @@ -249,8 +248,8 @@ async fn add_account( /// Re-create the platform account of a stored user that is gone from the system, using /// the stored name, UID and GID so file ownership and a home directory that survived -/// the deletion stay intact. A changed GitHub login is applied afterwards by the -/// regular rename path. +/// the account's removal stay intact. A changed GitHub login is applied afterwards by +/// the regular rename path. async fn recreate_account( gh_user: &octocrab::models::Author, available_user: &store::User, @@ -348,9 +347,9 @@ impl hannibal::Handler for LinuxUserManager { ); } - // The account may still carry the expiry of a deletion that failed after - // PrepareUserDeletion, e.g. when the user left and rejoined within one window - clear_deletion_expiry(&linux_user.name).await?; + // The account may carry an expiry that should no longer be in effect, e.g. + // one set by an operator: octosync owns the lifecycle of synced users + clear_departure_expiry(&linux_user.name).await?; if gh_user.login == linux_user.name { // Rebuild the entry from the system account, healing a stored name that a @@ -402,83 +401,161 @@ impl hannibal::Handler for LinuxUserManager { } } -impl hannibal::Handler for LinuxUserManager { +impl hannibal::Handler for LinuxUserManager { #[tracing::instrument( - name = "UserManager::prepare_user_deletion", + name = "UserManager::expire_account", skip_all, fields(user = %msg.user.name(), uid = msg.user.uid().as_raw()) )] async fn handle( &mut self, _ctx: &mut hannibal::Context, - msg: PrepareUserDeletion, - ) -> anyhow::Result { + msg: ExpireAccount, + ) -> anyhow::Result<()> { let Some(linux_user) = resolve_account_checked(&msg.user)? else { - tracing::warn!("User not found in system when attempting to delete, nothing to do"); - return Ok(DeletionPreparation::NothingToDo); + tracing::warn!("User not found in system when attempting to expire, nothing to do"); + return Ok(()); }; - // The password is locked, but pubkey SSH keeps working while the home directory - // is archived. Expire the account before the sweep so no new session can start - // anywhere in the deletion window. + // Expire first, so no new session (password or pubkey SSH) can start while + // the running ones are ended expire_account(&linux_user.name).await?; - // Kill all of the user's processes so none can block the deletion or keep - // writing to the home directory while it is archived + // logind first: it logs the user out cleanly, closing the PAM sessions and + // tearing down the session scopes and the user manager instead of only + // signaling PIDs. The sweep afterwards is the catch-all for processes outside + // logind's session scopes, and the sole mechanism on systems without logind. + end_logind_sessions(&linux_user).await; kill_processes_for_user(&linux_user).await?; - Ok(DeletionPreparation::Prepared { - home_dir: linux_user.dir, - }) + // octosync owns the supplementary groups of synced users, and a departed + // member keeps no access granted through them + strip_supplementary_groups(&linux_user).await + } +} + +/// End the user's login sessions through logind, which logs them out cleanly: it +/// closes the PAM sessions and tears down the session scopes and the user manager +/// through the cgroup hierarchy instead of only signaling PIDs, so nothing escapes by +/// forking. Never fatal, the following process sweep is the catch-all. +/// +/// Reaching logind at all is what separates the two failure modes, so they are logged +/// differently: no system bus means no logind and hence no logind sessions on this +/// machine, while a logind that answers and then refuses leaves session scopes and a +/// lingering user manager behind that the sweep cannot clean up. +async fn end_logind_sessions(user: &nix::unistd::User) { + let connection = match zbus::Connection::system().await { + Ok(connection) => connection, + // A container or a machine without systemd: the process sweep is the whole + // mechanism here, so this is an expected state rather than a failure + Err(e) => { + tracing::debug!("No system bus, ending sessions with the process sweep alone: {e}"); + return; + } + }; + if let Err(e) = terminate_logind_user(&connection, user.uid.as_raw()).await { + tracing::error!("logind is available but did not end the user's sessions: {e:#}"); } } -impl hannibal::Handler for LinuxUserManager { +/// Disable linger for the user and terminate their sessions over logind's D-Bus API. +/// +/// `TerminateUser` returns once the stop jobs are queued rather than once the +/// processes are gone, so the caller's sweep is what establishes that they exited. +async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyhow::Result<()> { + let manager = zbus::Proxy::new( + connection, + "org.freedesktop.login1", + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", + ) + .await + .context("Failed to create the logind manager proxy")?; + + // Disabling linger keeps a `user@.service` from restarting user services after the + // termination, but it is the lesser half: a surviving login session matters more, + // so a failure here must not skip the termination below + if let Err(e) = manager + .call::<_, _, ()>("SetUserLinger", &(uid, false, false)) + .await + { + tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e}"); + } + + // GetUser errors when the user neither has a session nor lingers, the usual state + // of a departed account that is re-expired by a later sync: nothing to terminate + if manager + .call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid) + .await + .is_err() + { + return Ok(()); + } + manager + .call::<_, _, ()>("TerminateUser", &uid) + .await + .context("Failed to terminate the user's sessions") +} + +/// Remove the user from all supplementary groups, so a departed member keeps no +/// access granted through them. The usermod is skipped when there is nothing to +/// remove, keeping the re-expiry on every sync cheap. +async fn strip_supplementary_groups(user: &nix::unistd::User) -> anyhow::Result<()> { + let current = current_supplementary_groups(user) + .with_context(|| format!("Failed to read current groups of '{}'", user.name))?; + if current.is_empty() { + return Ok(()); + } + sync_user_supplementary_groups_by_name(&user.name, &[]).await +} + +impl hannibal::Handler for LinuxUserManager { #[tracing::instrument( - name = "UserManager::remove_account", + name = "UserManager::purge_account", skip_all, fields(user = %msg.user.name(), uid = msg.user.uid().as_raw()) )] async fn handle( &mut self, _ctx: &mut hannibal::Context, - msg: RemoveAccount, - ) -> anyhow::Result<()> { - // The actor handles other messages between preparation and removal, so verify - // again that the name still belongs to the stored user before userdel + msg: PurgeAccount, + ) -> anyhow::Result { let Some(linux_user) = resolve_account_checked(&msg.user)? else { - tracing::warn!("User disappeared after deletion was prepared, nothing to do"); - return Ok(()); + return Ok(PurgeOutcome::NoAccount); }; - // A process spawned between the preparation sweep and here (archiving can take - // a while) would make userdel fail, so sweep again right before it. The home - // directory is already archived, so nothing is left for a process to shut down - // cleanly into: kill hard without a grace period. userdel decides - // authoritatively whether the account is busy, so a failed sweep is only logged. + // The account-side eligibility: only an account whose own shadow expiry has + // been in effect for the retention period may be purged. Any reactivation + // clears the expiry, so a live account can never pass this gate. + if !account_expire_days(&linux_user.name)? + .is_some_and(|expire_days| expire_days <= msg.expired_before) + { + return Ok(PurgeOutcome::NotExpired); + } + + // The account has been expired for the whole retention period, so nothing is + // left to shut down cleanly into: kill hard without a grace period. userdel + // decides authoritatively whether the account is busy, so a failed sweep is + // only logged. if let Err(e) = force_kill_processes_for_user(&linux_user).await { tracing::warn!("Failed to sweep processes before userdel: {e:#}"); } - let proc = process::Command::new("/usr/sbin/userdel") + let o = process::Command::new("/usr/sbin/userdel") .arg("--remove") - .arg(msg.user.name()) - .output(); - - let o = proc + .arg(&linux_user.name) + .output() .await .context("Failed to wait for userdel command to finish")?; - - if o.status.success() { - tracing::info!(archive = ?msg.receipt.archive_path(), "Deleted user"); - Ok(()) - } else { - Err(anyhow::anyhow!( - "Failed to delete user '{}': {}", - msg.user.name(), + if !o.status.success() { + anyhow::bail!( + "Failed to purge user '{}': {}", + linux_user.name, String::from_utf8_lossy(&o.stderr) - )) + ); } + tracing::info!("Purged the expired account and its home directory"); + Ok(PurgeOutcome::Purged) } } @@ -579,20 +656,21 @@ impl hannibal::Handler for LinuxUserManager { } } -/// Resolve the platform account of a stored user for deletion, refusing when name and -/// UID do not match. `Ok(None)` when no account with that name exists and the UID is -/// unused. +/// Resolve the platform account of a stored user for its expiry or purge, refusing +/// when name and UID do not match. `Ok(None)` when no account with that name exists +/// and the UID is unused. fn resolve_account_checked(user: &store::User) -> anyhow::Result> { - // userdel operates on the account name, so resolve the account by name to - // guarantee the home directory that is archived is the one userdel --remove deletes + // usermod and userdel operate on the account name, so resolve the account by name + // to guarantee the account acted on is the one the name addresses let Some(linux_user) = nix::unistd::User::from_name(user.name())? else { // The account may exist under a different name, e.g. when a usermod rename // succeeded but the store update was lost. Whether that is this user or an // unrelated account that reuses the UID can not be decided here, refuse - // instead of orphaning the account or deleting a wrong one. + // instead of acting on a possibly wrong account. if let Some(other_user) = nix::unistd::User::from_uid(user.uid())? { anyhow::bail!( - "No user named '{}' in the system, but UID {} belongs to '{}', refusing to delete", + "No user named '{}' in the system, but UID {} belongs to '{}', refusing to act \ + on the account", user.name(), user.uid(), other_user.name @@ -602,7 +680,8 @@ fn resolve_account_checked(user: &store::User) -> anyhow::Result anyhow::Result { Ok(days as i64) } -/// Expire the account so no new session (password or pubkey SSH) can start while its -/// deletion is in progress. Sessions already running are handled by the process sweep. +/// Expire the account so no new session (password or pubkey SSH) can start. Sessions +/// already running are handled by the session teardown and the process sweep. /// -/// The expiry is set to the day before the deletion: the shadow field has day +/// The expiry is set to the day before the departure: the shadow field has day /// granularity and some login paths treat an account as expired only strictly after -/// its date, so the deletion day itself could keep logins open until midnight. +/// its date, so the departure day itself could keep logins open until midnight. async fn expire_account(name: &str) -> anyhow::Result<()> { let expire_days = days_since_epoch()? - 1; + // A departed account is re-expired on every sync; skip the usermod when the + // expiry is already in effect + if account_expire_days(name)?.is_some_and(|days| days <= expire_days) { + return Ok(()); + } let output = process::Command::new("/usr/sbin/usermod") .arg("--expiredate") // shadow parses a plain number as days since the epoch, which sidesteps the @@ -702,18 +786,18 @@ async fn expire_account(name: &str) -> anyhow::Result<()> { String::from_utf8_lossy(&output.stderr) ); } - tracing::info!("Expired account so no new session can start during deletion"); + tracing::info!("Expired account so no new session can start"); Ok(()) } -/// Lift an expiry that is already in effect, so a user whose account survived a failed -/// deletion and who is synced again is not locked out silently. An expiry in the -/// future is an operator's scheduled offboarding and stays. +/// Lift an expiry that is already in effect, reactivating the expired account of a +/// departed member who rejoined. An expiry in the future is an operator's scheduled +/// offboarding and stays. /// /// octosync owns the account lifecycle of synced users, so a past expiry set by an /// operator does not survive a sync either. Suspending a member is done by removing /// them from the org. -async fn clear_deletion_expiry(name: &str) -> anyhow::Result<()> { +async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { let Some(expire_days) = account_expire_days(name)? else { return Ok(()); }; diff --git a/src/user_manager/backends/mock.rs b/src/user_manager/backends/mock.rs index 33b21da..5259d88 100644 --- a/src/user_manager/backends/mock.rs +++ b/src/user_manager/backends/mock.rs @@ -3,8 +3,8 @@ use crate::store; use crate::user_manager::{ - AccountIds, CreateUser, DeletionPreparation, EnsureGroupsExist, PrepareUserDeletion, - RemoveAccount, SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, + AccountIds, CreateUser, EnsureGroupsExist, ExpireAccount, PurgeAccount, PurgeOutcome, + SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, }; #[derive(Debug)] @@ -103,40 +103,39 @@ impl hannibal::Handler for MockUserManager { } } -impl hannibal::Handler for MockUserManager { +impl hannibal::Handler for MockUserManager { #[tracing::instrument( - name = "UserManager::prepare_user_deletion", + name = "UserManager::expire_account", skip_all, fields(user = %msg.user.name(), uid = msg.user.uid().as_raw()) )] async fn handle( &mut self, _ctx: &mut hannibal::Context, - msg: PrepareUserDeletion, - ) -> anyhow::Result { - tracing::info!("Would expire the account, archive the home directory and delete the user"); - Ok(DeletionPreparation::NothingToDo) + msg: ExpireAccount, + ) -> anyhow::Result<()> { + tracing::info!( + "Would expire the account, end its sessions and strip its supplementary groups" + ); + Ok(()) } } -impl hannibal::Handler for MockUserManager { - // Unreachable through the mock flow, since the preparation never reports an - // account to remove, but the contract requires handling the full message set +impl hannibal::Handler for MockUserManager { #[tracing::instrument( - name = "UserManager::remove_account", + name = "UserManager::purge_account", skip_all, fields(user = %msg.user.name(), uid = msg.user.uid().as_raw()) )] async fn handle( &mut self, _ctx: &mut hannibal::Context, - msg: RemoveAccount, - ) -> anyhow::Result<()> { - tracing::info!( - archive = ?msg.receipt.archive_path(), - "Mock removing account (not actually deleting users on non-Linux OS)" - ); - Ok(()) + msg: PurgeAccount, + ) -> anyhow::Result { + // The account-side shadow-expiry check needs a real account, so the preview + // reports the purge the store-side eligibility selected + tracing::info!("Would purge the expired account and its home directory"); + Ok(PurgeOutcome::Purged) } } diff --git a/src/user_manager/backends/testing.rs b/src/user_manager/backends/testing.rs index cf8d01d..5242986 100644 --- a/src/user_manager/backends/testing.rs +++ b/src/user_manager/backends/testing.rs @@ -3,8 +3,8 @@ use crate::store; use crate::user_manager::{ - AccountIds, CreateUser, DeletionPreparation, EnsureGroupsExist, PrepareUserDeletion, - RemoveAccount, SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, + AccountIds, CreateUser, EnsureGroupsExist, ExpireAccount, PurgeAccount, PurgeOutcome, + SyncSupplementaryGroups, UpdateAuthorizedKeys, UpdateUser, }; use std::{collections, sync}; @@ -15,8 +15,11 @@ pub struct TestingUserManager { /// or reserved IDs are passed through. Clone the handle before spawning the actor. pub create_user_ids: sync::Arc>>, pub update_user: collections::VecDeque>, - pub prepare_user_deletion: collections::VecDeque>, - pub remove_account: collections::VecDeque>, + pub expire_account: collections::VecDeque>, + /// The user names received with [`ExpireAccount`], so tests can assert which + /// accounts were expired. Clone the handle before spawning the actor. + pub expired_users: sync::Arc>>, + pub purge_account: collections::VecDeque>, pub sync_supplementary_groups: collections::VecDeque>, pub ensure_groups_exist: collections::VecDeque>, pub update_authorized_keys: collections::VecDeque>, @@ -54,23 +57,27 @@ impl hannibal::Handler for TestingUserManager { } } -impl hannibal::Handler for TestingUserManager { +impl hannibal::Handler for TestingUserManager { async fn handle( &mut self, _ctx: &mut hannibal::Context, - _msg: PrepareUserDeletion, - ) -> anyhow::Result { - next_response(&mut self.prepare_user_deletion, "PrepareUserDeletion") + msg: ExpireAccount, + ) -> anyhow::Result<()> { + self.expired_users + .lock() + .unwrap() + .push(msg.user.name().to_string()); + next_response(&mut self.expire_account, "ExpireAccount") } } -impl hannibal::Handler for TestingUserManager { +impl hannibal::Handler for TestingUserManager { async fn handle( &mut self, _ctx: &mut hannibal::Context, - _msg: RemoveAccount, - ) -> anyhow::Result<()> { - next_response(&mut self.remove_account, "RemoveAccount") + _msg: PurgeAccount, + ) -> anyhow::Result { + next_response(&mut self.purge_account, "PurgeAccount") } } diff --git a/src/user_manager/messages.rs b/src/user_manager/messages.rs index f4050a7..6dd5dbc 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -3,8 +3,8 @@ //! A platform backend is an actor handling the full message set. The messages carry //! owned data so they can cross the actor boundary. -use crate::{archiver, public_keys, store}; -use std::{collections, path}; +use crate::{public_keys, store}; +use std::collections; /// Creates a platform user for the given GitHub user without a password. #[hannibal::message(response = anyhow::Result)] @@ -16,18 +16,19 @@ pub struct CreateUser { /// UID and GID of the account [`CreateUser`] creates. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AccountIds { - /// The stored IDs of a rejoining member: the account is created with exactly this - /// UID and its private group with exactly this GID, so file ownership survives the - /// delete and re-create cycle. When either ID is taken, creation fails instead of - /// falling back to a fresh one. A tombstone migrated from v1 carries no GID and - /// leaves the private group to `useradd`. + /// The stored IDs of a rejoining member whose account is gone from the system + /// (purged, or removed by hand): the account is created with exactly this UID and + /// its private group with exactly this GID, so file ownership survives the removal + /// and re-create cycle. When either ID is taken, creation fails instead of falling + /// back to a fresh one. A tombstone migrated from v1 carries no GID and leaves the + /// private group to `useradd`. Stored { uid: nix::unistd::Uid, gid: Option, }, /// System-allocated IDs for a brand-new member, avoiding the IDs reserved by /// tombstones: shadow-utils allocates the highest ID in range plus one, so - /// deleting the user with the highest UID frees exactly the next UID to be + /// purging the user with the highest UID frees exactly the next UID to be /// allocated. Fresh { reserved_uids: collections::HashSet, @@ -45,32 +46,50 @@ pub struct UpdateUser { pub available_user: store::User, } -/// Prepares the deletion of a platform user: verifies that the stored user still -/// matches the platform account, expires the account so no new session can start, -/// and stops everything that could block the deletion or keep writing to the home -/// directory while it is archived. -#[hannibal::message(response = anyhow::Result)] -pub struct PrepareUserDeletion { +/// Expires the platform account of a departed user, replacing its deletion: verifies +/// that the stored user still matches the platform account, expires it so no new +/// session (password or pubkey SSH) can start, ends the running sessions and strips +/// the supplementary groups. The account, its home directory and its primary group +/// stay on the machine as the durable departure record, so the departure is a +/// reversible lockout that a rejoin heals, until the purge removes the account after +/// the retention period. +/// +/// Idempotent, so the sync can send it for every departed user on every run and an +/// expiry interrupted at any point converges. +#[hannibal::message(response = anyhow::Result<()>)] +pub struct ExpireAccount { pub user: store::User, } -/// Outcome of [`PrepareUserDeletion`]. -#[derive(Debug)] -pub enum DeletionPreparation { - /// No platform account for the user exists, there is nothing to delete. - NothingToDo, - /// The account can be removed once its home directory is archived. +/// Permanently removes the expired platform account of a departed user with +/// `userdel --remove`, deleting the home directory without an archive: the one +/// deliberately irreversible operation left now that expiry replaced deletion. +/// +/// The account-side half of the purge eligibility is verified here: the shadow expiry +/// must be set and at most `expired_before`. It is the evidence on the machine itself +/// that survives store damage and is cleared on any reactivation, so a wrongly +/// resurrected tombstone can never purge a live account. +#[hannibal::message(response = anyhow::Result)] +pub struct PurgeAccount { + pub user: store::User, + /// Latest shadow expiry, in days since the epoch, an account may have to be purged #[cfg_attr(not(target_os = "linux"), allow(dead_code))] - Prepared { home_dir: path::PathBuf }, + pub expired_before: i64, } -/// Removes the platform account of a prepared user. Taking an -/// [`archiver::ArchiveReceipt`] forces the home directory to be archived before the -/// account and its files can be deleted. -#[hannibal::message(response = anyhow::Result<()>)] -pub struct RemoveAccount { - pub user: store::User, - pub receipt: archiver::ArchiveReceipt, +/// Outcome of [`PurgeAccount`]. Only the Linux backend can verify the account-side +/// eligibility, the mock previews every purge as performed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PurgeOutcome { + /// The account and its home directory were removed + Purged, + /// No account for the user exists on the system, nothing was done + #[cfg_attr(not(target_os = "linux"), allow(dead_code))] + NoAccount, + /// The account's shadow expiry is missing or newer than `expired_before`: the + /// account-side clock does not agree with the store's, nothing was done + #[cfg_attr(not(target_os = "linux"), allow(dead_code))] + NotExpired, } /// Synchronizes the supplementary groups of a user. From 3c497d2fedd35d872f29e9590f4fbac0cc22c541 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 16:41:26 +0200 Subject: [PATCH 03/12] feat: Refuse to create a user whose login a departed tombstone reserves With expiry, departed accounts stay on the machine as adoptable by name. A leaver's GitHub login can be released and claimed by a different person, and adopting the expired account would hand the new member the previous owner's home directory and UID. A tombstone whose GitHub ID matches the joining member stays the rejoin path, and a pre-existing account without a tombstone stays adoptable by decision. --- src/octosync.rs | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/octosync.rs b/src/octosync.rs index 0686121..3d21e98 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -133,6 +133,26 @@ impl Octosync { gh_user: &octocrab::models::Author, store: &store::UserStore, ) -> anyhow::Result { + // With expiry, departed accounts stay on the machine as adoptable-by-name. A + // leaver's GitHub login can be released and claimed by a different person; + // adopting the expired account would hand that person the previous owner's + // home directory and UID, so the collision is refused and left to an operator. + if let Some(tombstone) = store + .departed() + .values() + .find(|tombstone| tombstone.name() == gh_user.login) + && tombstone.id() != gh_user.id + { + anyhow::bail!( + "Login '{}' belonged to the departed member with GitHub ID {}, but the joining \ + member has GitHub ID {}: the login was recycled by a different person, refusing \ + to create the user", + gh_user.login, + tombstone.id(), + gh_user.id + ); + } + // A departed member rejoins into their expired account; a purged one gets a // fresh account and home directory under their old IDs let stored_ids = store @@ -740,6 +760,23 @@ mod tests { ); } + /// A departed member's login claimed by a different person must not adopt the + /// expired account: no user manager response is scripted, so the test also + /// proves the guard refuses before any platform operation runs. + #[tokio::test] + async fn recycled_login_of_a_departed_member_is_refused() { + let (octosync, _data_dir) = octosync_with(TestingUserManager::default()); + let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + old_store.depart_user(user(1, "alice"), chrono::Utc::now()); + + let err = octosync + .create_user(&author(2, "alice"), &old_store) + .await + .unwrap_err(); + + assert!(err.to_string().contains("recycled")); + } + /// Tombstones survive member processing structurally: a failed re-creation /// leaves the tombstone in place for a retry on the next sync. #[tokio::test] From 1bf8577448182454c4f7100a473f26b1db67d7fb Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 16:42:49 +0200 Subject: [PATCH 04/12] fix: Never write the users database in a dry run --dry-run only swapped in the mock backend, while sync and delete still saved the rebuilt store into the real data directory: new members were persisted with invented mock UIDs and leavers were tombstoned for real, so a later real run acted on preview data. The store itself now carries the dry-run mode and its save method skips the write, so no caller can persist preview data. This also makes the purge preview side-effect free. --- src/octosync.rs | 107 +++++++++++++++++++++++++++++++++++++----------- src/store.rs | 67 +++++++++++++++++++++--------- 2 files changed, 129 insertions(+), 45 deletions(-) diff --git a/src/octosync.rs b/src/octosync.rs index 3d21e98..6bb1f7a 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -60,6 +60,9 @@ async fn org_client(args: &InstallationClientArgs) -> anyhow::Result anyhow::Result { - let mut new_store = store::UserStore::new(&self.data_dir).await?; + let mut new_store = store::UserStore::new(&self.data_dir, self.dry_run).await?; // Tombstones are carried over wholesale before any member is processed, so // their survival never depends on per-user processing succeeding *new_store.departed_mut() = store.departed().clone(); @@ -413,7 +417,7 @@ impl Octosync { #[tracing::instrument(name = "Octosync::delete", skip(self))] pub async fn delete(&self) -> anyhow::Result<()> { - let mut store = store::UserStore::from_dir(&self.data_dir).await?; + let mut store = store::UserStore::from_dir(&self.data_dir, self.dry_run).await?; let users: Vec = store.data().values().cloned().collect(); let count = users.len(); // The store file is kept: the tombstones preserve every UID so members @@ -438,7 +442,7 @@ impl Octosync { let octocrab = org_client(&args.octocrab).await?; let (org_members, mut store) = tokio::try_join!( get_all_org_members(&octocrab, &args.octocrab.org), - store::UserStore::from_dir(&self.data_dir), + store::UserStore::from_dir(&self.data_dir, self.dry_run), )?; let org_member_map: collections::HashMap = collections::HashMap::from_iter( @@ -574,11 +578,40 @@ mod tests { let data_dir = tempfile::tempdir().unwrap(); let octosync = Octosync { data_dir: data_dir.path().to_path_buf(), + dry_run: false, user_manager: UserManager::testing(actor), }; (octosync, data_dir) } + /// A dry run must not write the users database: new members would be + /// persisted with invented mock IDs and tombstone changes would be acted on + /// by a later real run. + #[tokio::test] + async fn dry_run_does_not_write_the_store() { + let mut actor = TestingUserManager::default(); + actor.expire_account.push_back(Ok(())); + let data_dir = tempfile::tempdir().unwrap(); + let octosync = Octosync { + data_dir: data_dir.path().to_path_buf(), + dry_run: true, + user_manager: UserManager::testing(actor), + }; + let mut store = store::UserStore::new(data_dir.path(), true).await.unwrap(); + + octosync + .depart_and_expire(&mut store, vec![user(1, "alice")]) + .await + .unwrap(); + + // The departure exists in memory but nothing was persisted + assert!(store.departed().contains_key(&octocrab::models::UserId(1))); + let on_disk = store::UserStore::from_dir(data_dir.path(), false) + .await + .unwrap(); + assert!(on_disk.departed().is_empty()); + } + #[tokio::test] async fn process_user_creates_missing_user_and_tolerates_failed_key_fetch() { let mut actor = TestingUserManager::default(); @@ -587,7 +620,9 @@ mod tests { // No UpdateAuthorizedKeys response is scripted: the key fetch against the // unreachable client fails, so the keys must be skipped, not synced let (octosync, _data_dir) = octosync_with(actor); - let empty_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let empty_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); let processed = octosync .process_user( @@ -608,7 +643,9 @@ mod tests { actor.update_user.push_back(Ok(user(1, "alice-renamed"))); actor.sync_supplementary_groups.push_back(Ok(())); let (octosync, _data_dir) = octosync_with(actor); - let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); old_store .data_mut() .insert(octocrab::models::UserId(1), user(1, "alice")); @@ -634,7 +671,9 @@ mod tests { .sync_supplementary_groups .push_back(Err(anyhow::anyhow!("boom"))); let (octosync, _data_dir) = octosync_with(actor); - let empty_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let empty_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); let err = octosync .process_user( @@ -659,7 +698,9 @@ mod tests { let mut actor = TestingUserManager::default(); actor.create_user.push_back(Err(anyhow::anyhow!("boom"))); let (octosync, _data_dir) = octosync_with(actor); - let empty_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let empty_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); let new_store = octosync .process_members( @@ -680,7 +721,9 @@ mod tests { actor.create_user.push_back(Ok(user(1, "alice"))); actor.sync_supplementary_groups.push_back(Ok(())); let (octosync, _data_dir) = octosync_with(actor); - let empty_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let empty_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); let new_store = octosync .process_members( @@ -705,7 +748,9 @@ mod tests { actor.sync_supplementary_groups.push_back(Ok(())); let received_ids = actor.create_user_ids.clone(); let (octosync, _data_dir) = octosync_with(actor); - let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); old_store.depart_user(user(1, "alice"), chrono::Utc::now()); let new_store = octosync @@ -738,7 +783,9 @@ mod tests { actor.sync_supplementary_groups.push_back(Ok(())); let received_ids = actor.create_user_ids.clone(); let (octosync, _data_dir) = octosync_with(actor); - let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); old_store.depart_user(user(1, "alice"), chrono::Utc::now()); octosync @@ -766,7 +813,9 @@ mod tests { #[tokio::test] async fn recycled_login_of_a_departed_member_is_refused() { let (octosync, _data_dir) = octosync_with(TestingUserManager::default()); - let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); old_store.depart_user(user(1, "alice"), chrono::Utc::now()); let err = octosync @@ -786,7 +835,9 @@ mod tests { .create_user .push_back(Err(anyhow::anyhow!("UID is taken"))); let (octosync, _data_dir) = octosync_with(actor); - let mut old_store = store::UserStore::new(_data_dir.path()).await.unwrap(); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); old_store.depart_user(user(1, "alice"), chrono::Utc::now()); let new_store = octosync @@ -815,14 +866,16 @@ mod tests { let mut actor = TestingUserManager::default(); actor.expire_account.push_back(Err(anyhow::anyhow!("boom"))); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); octosync .depart_and_expire(&mut store, vec![user(1, "alice")]) .await .unwrap(); - let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); + let saved = store::UserStore::from_dir(data_dir.path(), false) + .await + .unwrap(); let tombstone = &saved.departed()[&octocrab::models::UserId(1)]; assert_eq!(tombstone.name(), "alice"); assert!(saved.data().is_empty()); @@ -836,7 +889,7 @@ mod tests { actor.expire_account.push_back(Ok(())); let expired_users = actor.expired_users.clone(); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), chrono::Utc::now()); // No leavers this sync, the existing tombstone alone drives the expiry @@ -855,7 +908,7 @@ mod tests { let mut actor = TestingUserManager::default(); actor.expire_account.push_back(Ok(())); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store .data_mut() .insert(octocrab::models::UserId(1), user(1, "alice")); @@ -863,7 +916,9 @@ mod tests { octosync.delete().await.unwrap(); - let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); + let saved = store::UserStore::from_dir(data_dir.path(), false) + .await + .unwrap(); assert!(saved.data().is_empty()); assert_eq!( saved.departed()[&octocrab::models::UserId(1)].name(), @@ -889,7 +944,7 @@ mod tests { let mut actor = TestingUserManager::default(); actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), old_departure()); octosync @@ -900,7 +955,9 @@ mod tests { assert!(store.departed().is_empty()); // The tombstone survives the purge and is saved, so the UID stays // reserved for a rejoin even after the purge - let saved = store::UserStore::from_dir(data_dir.path()).await.unwrap(); + let saved = store::UserStore::from_dir(data_dir.path(), false) + .await + .unwrap(); let purged = &saved.purged()[&octocrab::models::UserId(1)]; assert_eq!(purged.name(), "alice"); } @@ -912,7 +969,7 @@ mod tests { let mut actor = TestingUserManager::default(); actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), chrono::Utc::now()); octosync @@ -931,7 +988,7 @@ mod tests { let mut actor = TestingUserManager::default(); actor.purge_account.push_back(Ok(PurgeOutcome::Purged)); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); let alice = user(1, "alice"); store.depart_user(alice.clone(), old_departure()); @@ -952,7 +1009,7 @@ mod tests { actor.purge_account.push_back(Ok(PurgeOutcome::NotExpired)); actor.purge_account.push_back(Ok(PurgeOutcome::NoAccount)); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), old_departure()); store.depart_user(user(2, "bob"), old_departure()); @@ -972,7 +1029,7 @@ mod tests { let mut actor = TestingUserManager::default(); actor.purge_account.push_back(Err(anyhow::anyhow!("boom"))); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), old_departure()); octosync @@ -991,7 +1048,7 @@ mod tests { let actor = TestingUserManager::default(); let expired_users = actor.expired_users.clone(); let (octosync, data_dir) = octosync_with(actor); - let mut store = store::UserStore::new(data_dir.path()).await.unwrap(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), old_departure()); store.mark_purged(&octocrab::models::UserId(1), chrono::Utc::now()); diff --git a/src/store.rs b/src/store.rs index 5c71ad9..cc626bb 100644 --- a/src/store.rs +++ b/src/store.rs @@ -246,6 +246,9 @@ struct StoreDataRef<'a> { #[derive(Debug)] pub struct UserStore { dir: path::PathBuf, + /// Preview mode: [`UserStore::save`] never writes, so a dry run cannot persist + /// invented mock IDs or tombstone changes into the real users database + dry_run: bool, /// In-memory cache of users loaded from the members database, keyed by GitHub user ID users: UserMap, /// Tombstones of departed members whose account is expired on the machine, keyed by @@ -259,11 +262,13 @@ pub struct UserStore { } impl UserStore { - /// Create a new store instance with the given directory, without loading any data - pub async fn new(dir: &path::Path) -> anyhow::Result { + /// Create a new store instance with the given directory, without loading any data. + /// A store created with `dry_run` never writes on save. + pub async fn new(dir: &path::Path, dry_run: bool) -> anyhow::Result { fs::create_dir_all(&dir).await?; Ok(Self { dir: dir.to_path_buf(), + dry_run, users: UserMap::new(), departed: DepartedMap::new(), purged: PurgedMap::new(), @@ -272,8 +277,8 @@ impl UserStore { /// Create a new store loading data from the directory #[tracing::instrument(name = "Store::from_dir")] - pub async fn from_dir(dir: &path::Path) -> anyhow::Result { - let mut s = Self::new(dir).await?; + pub async fn from_dir(dir: &path::Path, dry_run: bool) -> anyhow::Result { + let mut s = Self::new(dir, dry_run).await?; s.load().await?; Ok(s) } @@ -429,7 +434,14 @@ impl UserStore { Ok(()) } + /// Write the store to the users database file. A dry run never writes: the + /// preview must not persist invented mock IDs or tombstone changes that a later + /// real run would act on. pub async fn save(&self) -> anyhow::Result<()> { + if self.dry_run { + tracing::info!("Dry run: not writing the users database"); + return Ok(()); + } let content = serde_json::to_string_pretty(&StoreDataRef { version: STORE_VERSION, users: &self.users, @@ -646,7 +658,7 @@ mod tests { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); let store_path = temp_dir.path().join("store"); - let store = UserStore::from_dir(&store_path) + let store = UserStore::from_dir(&store_path, false) .await .expect("Failed to create store"); @@ -660,7 +672,7 @@ mod tests { async fn load_nonexistent_file() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let store = UserStore::from_dir(temp_dir.path()) + let store = UserStore::from_dir(temp_dir.path(), false) .await .expect("Failed to create store"); @@ -675,7 +687,7 @@ mod tests { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), &v1_content()).await; - let store = UserStore::from_dir(temp_dir.path()) + let store = UserStore::from_dir(temp_dir.path(), false) .await .expect("Failed to load store"); @@ -698,7 +710,7 @@ mod tests { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), &v1_content()).await; - let store = UserStore::from_dir(temp_dir.path()).await.unwrap(); + let store = UserStore::from_dir(temp_dir.path(), false).await.unwrap(); store.save().await.expect("Failed to save store"); let content = fs::read_to_string(temp_dir.path().join(USERS_FILE_NAME)) @@ -716,7 +728,9 @@ mod tests { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), r#"{ "version": 3, "users": {} }"#).await; - let err = UserStore::from_dir(temp_dir.path()).await.unwrap_err(); + let err = UserStore::from_dir(temp_dir.path(), false) + .await + .unwrap_err(); assert!(err.to_string().contains("version 3")); } @@ -724,7 +738,7 @@ mod tests { async fn round_trip_with_departed_and_purged_users() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let mut store = UserStore::from_dir(temp_dir.path()) + let mut store = UserStore::from_dir(temp_dir.path(), false) .await .expect("Failed to create store"); let active = user(); @@ -736,7 +750,7 @@ mod tests { store.save().await.expect("Failed to save store"); - let loaded = UserStore::from_dir(temp_dir.path()) + let loaded = UserStore::from_dir(temp_dir.path(), false) .await .expect("Failed to load store"); assert_eq!(loaded.users[&active.id], active); @@ -751,7 +765,7 @@ mod tests { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), r#"{ "version": 2, "users": {} }"#).await; - let store = UserStore::from_dir(temp_dir.path()).await.unwrap(); + let store = UserStore::from_dir(temp_dir.path(), false).await.unwrap(); assert!(store.users.is_empty()); assert!(store.departed.is_empty()); assert!(store.purged.is_empty()); @@ -760,7 +774,7 @@ mod tests { #[tokio::test] async fn depart_user_moves_an_active_user_to_the_tombstones() { let temp_dir = tempfile::TempDir::new().unwrap(); - let mut store = UserStore::new(temp_dir.path()).await.unwrap(); + let mut store = UserStore::new(temp_dir.path(), false).await.unwrap(); let user = user(); store.users.insert(user.id, user.clone()); @@ -777,7 +791,7 @@ mod tests { #[tokio::test] async fn mark_purged_moves_a_departed_tombstone_to_the_purged_map() { let temp_dir = tempfile::TempDir::new().unwrap(); - let mut store = UserStore::new(temp_dir.path()).await.unwrap(); + let mut store = UserStore::new(temp_dir.path(), false).await.unwrap(); let departed = departed_user(); store.departed.insert(departed.id, departed.clone()); let purged_at = chrono::Utc::now(); @@ -796,7 +810,7 @@ mod tests { #[tokio::test] async fn prune_rejoined_drops_tombstones_of_active_users() { let temp_dir = tempfile::TempDir::new().unwrap(); - let mut store = UserStore::new(temp_dir.path()).await.unwrap(); + let mut store = UserStore::new(temp_dir.path(), false).await.unwrap(); let rejoined = user(); let departed = departed_user(); store @@ -816,7 +830,7 @@ mod tests { #[tokio::test] async fn prune_rejoined_drops_purged_tombstones_of_active_users() { let temp_dir = tempfile::TempDir::new().unwrap(); - let mut store = UserStore::new(temp_dir.path()).await.unwrap(); + let mut store = UserStore::new(temp_dir.path(), false).await.unwrap(); let purged = purged_user(); let rejoined = User::from_test_purged(&purged); store.purged.insert(purged.id, purged.clone()); @@ -827,12 +841,25 @@ mod tests { assert!(store.purged.is_empty()); } + /// A store in dry-run mode previews everything in memory but never persists + #[tokio::test] + async fn dry_run_save_writes_nothing() { + let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); + let mut store = UserStore::new(temp_dir.path(), true).await.unwrap(); + let user = user(); + store.users.insert(user.id, user); + + store.save().await.expect("Dry-run save must succeed"); + + assert!(!temp_dir.path().join(USERS_FILE_NAME).exists()); + } + #[tokio::test] async fn load_invalid_json() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); write_users_file(temp_dir.path(), "{ invalid json content").await; - let result = UserStore::from_dir(temp_dir.path()).await; + let result = UserStore::from_dir(temp_dir.path(), false).await; assert!(result.is_err()); } @@ -840,7 +867,7 @@ mod tests { async fn user_path() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let store = UserStore::new(temp_dir.path()) + let store = UserStore::new(temp_dir.path(), false) .await .expect("Failed to create store"); @@ -852,7 +879,7 @@ mod tests { async fn multiple_users() { let temp_dir = tempfile::TempDir::new().expect("Failed to create temp dir"); - let mut store = UserStore::new(temp_dir.path()) + let mut store = UserStore::new(temp_dir.path(), false) .await .expect("Failed to create store"); @@ -868,7 +895,7 @@ mod tests { store.save().await.expect("Failed to save store"); - let loaded_store = UserStore::from_dir(temp_dir.path()) + let loaded_store = UserStore::from_dir(temp_dir.path(), false) .await .expect("Failed to load store"); From dbe8900747ecbc25c36d87d078bf4ecb99cb0b6b Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 16:44:48 +0200 Subject: [PATCH 05/12] docs: Describe expiry and purge in the README and prune stale changelog entries The home directory archive section is replaced by the departure lifecycle. Unreleased changelog entries describing removed behavior (home archiving, the second sweep before userdel, the deletion-window expiry) are pruned or folded into the new entries, since none of it was ever released. --- CHANGELOG.md | 63 ++++++++++++++++++++++++++++++---------------------- README.md | 26 +++++++++++++++------- 2 files changed, 54 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a25b35..68d13b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,12 +2,10 @@ ## Unreleased -- Archive home directories of deleted users in the data directory - ([#9](https://github.com/jonasehrlich/octosync/pull/9)) -- Only delete users whose GitHub ID is missing from the fetched org member list. Users whose +- Only expire users whose GitHub ID is missing from the fetched org member list. Users whose processing failed stay in the store unchanged. ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- A failed authorized keys fetch no longer fails user processing which could lead to the deletion of +- A failed authorized keys fetch no longer fails user processing which could lead to the removal of users whose keys could not be fetched. ([#15](https://github.com/jonasehrlich/octosync/pull/15)) - Fetch public keys through the authenticated GitHub client instead of the anonymous, rate-limited endpoint ([#15](https://github.com/jonasehrlich/octosync/pull/15)) @@ -15,45 +13,56 @@ ([#15](https://github.com/jonasehrlich/octosync/pull/15)) - Write `authorized_keys` atomically, never follow symlinks and enforce exact permissions on `.ssh` and `authorized_keys` ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- Refuse a sync that would delete every stored user +- Refuse a sync that would expire every stored user ([#17](https://github.com/jonasehrlich/octosync/pull/17)) - Implement mapping GitHub teams to Linux groups with `--group :` ([#20](https://github.com/jonasehrlich/octosync/pull/20)) - Re-create stored users whose account disappeared from the system, passing the stored name and UID - to `useradd` so file ownership survives a delete and re-create cycle + to `useradd` so file ownership survives a removal and re-create cycle ([#19](https://github.com/jonasehrlich/octosync/pull/19)) - Refuse to update a user whose stored UID or name belongs to a different account, instead of renaming that account and handing it the GitHub user's SSH keys ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Kill a user's processes a second time right before `userdel`, so a process spawned while the home - directory was archived cannot fail the deletion - ([#19](https://github.com/jonasehrlich/octosync/pull/19)) - Stop user processes gracefully: send SIGTERM first and SIGKILL only the processes still running after a grace period, then wait for the killed processes to leave the process table ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Expire an account at the day before its deletion starts, so no new SSH session can begin while the - home directory is archived. Syncing a user whose account survived a failed deletion lifts an - expiry that is already in effect. A future expiry date set by an operator stays. - ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Store `users.json` in schema v2: users deleted by octosync are kept as tombstones under an - `archived` key, so a member who leaves and rejoins gets their old UID back. The v1 file is backed - up to `users-v1.json` once before the first v2 save. - ([#22](https://github.com/jonasehrlich/octosync/pull/22)) -- Save the tombstone before `userdel` runs and re-enqueue archived users whose account still exists - on every sync, so a deletion interrupted at any point is finished by a later sync instead of - orphaning a live account ([#22](https://github.com/jonasehrlich/octosync/pull/22)) -- Record the home directory archive path on the tombstone, so a rejoin can later restore it - ([#22](https://github.com/jonasehrlich/octosync/pull/22)) -- The `delete` command keeps the store file and writes tombstones instead of removing `users.json`, - preserving the UID memory of a full wipe ([#22](https://github.com/jonasehrlich/octosync/pull/22)) +- Replace account deletion with expiry: a departed member's account is expired with + `usermod --expiredate`, which blocks password and pubkey SSH logins, their sessions are ended + through logind (with the process sweep as the catch-all) and their supplementary groups are + removed. The account and home directory stay on the machine as the durable departure record, so a + wrong departure decision is a reversible lockout that the next sync of a rejoining member heals, + with files, UID and GID intact. Expiry is re-applied on every sync, so an interrupted departure + converges. A future expiry date set by an operator stays. + ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Purge accounts that have been expired for longer than the retention period (180 days by default, + `--purge-after-days`) at the end of each sync and through the new `purge` command: the account and + home directory are removed permanently and without an archive. A purge requires the tombstone's + departure timestamp and the account's own shadow expiry to agree on the age, and the member to be + absent from the fetched member list. The tombstone survives the purge, so even a member rejoining + later gets their old UID and GID back. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Refuse to create a user whose GitHub login a departed member's tombstone reserves under a + different GitHub ID: a recycled login must not adopt the previous owner's expired account and home + directory. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Store `users.json` in a versioned v2 schema: members who left are kept as tombstones under a + `departed` key while their account is expired, and under a `purged` key after the purge, so a + member who leaves and rejoins gets their old UID back. The v1 file is backed up to `users-v1.json` + once before the first v2 save. ([#22](https://github.com/jonasehrlich/octosync/pull/22), + [#25](https://github.com/jonasehrlich/octosync/pull/25)) +- The `delete` command expires all managed users and keeps the store file with tombstones instead of + removing `users.json`, preserving the UID memory of a full wipe + ([#22](https://github.com/jonasehrlich/octosync/pull/22), + [#25](https://github.com/jonasehrlich/octosync/pull/25)) +- A dry run no longer writes the users database: previously new members were persisted with invented + mock UIDs and leavers were tombstoned for real, so a later real run acted on preview data. + ([#25](https://github.com/jonasehrlich/octosync/pull/25)) - Track the primary group GID in the store and re-create a rejoining member's private group with `groupadd --gid ` before `useradd --gid `, so group ownership of files outside the - archived home directory also survives the delete and re-create cycle. A GID that meanwhile belongs - to another group fails the re-creation loudly instead of falling back to a fresh one. Entries + home directory also survives a removal and re-create cycle. A GID that meanwhile belongs to + another group fails the re-creation loudly instead of falling back to a fresh one. Entries migrated from a v1 store carry no GID and are backfilled on their next update. ([#22](https://github.com/jonasehrlich/octosync/pull/22)) - Never allocate a departed user's UID or GID to a brand-new member: `useradd` hands out the highest - ID in range plus one, which is exactly what deleting the highest-UID user frees. A new account + ID in range plus one, which is exactly what purging the highest-UID user frees. A new account whose auto-allocated IDs collide with a tombstone is removed while still empty and re-created with explicitly chosen free IDs. ([#22](https://github.com/jonasehrlich/octosync/pull/22)) diff --git a/README.md b/README.md index 094166f..bd9db55 100644 --- a/README.md +++ b/README.md @@ -49,16 +49,26 @@ octosync fully manages the supplementary groups of synced users. On every sync t with the groups derived from the `--group` arguments, so memberships added through other channels are removed. -### Home directory archives +### Departures: expiry instead of deletion -Before a user is deleted, their home directory is archived to -`/home-archive/--.tar.gz`. The archive directory and the archives -are owned by root with permissions `700` and `600` respectively, so only root can access them. If -archiving fails, the user is not deleted and the deletion is retried on the next sync. +When a member leaves the org, their account is not deleted. It is expired instead, which blocks both +password and pubkey SSH logins, running sessions are ended and the supplementary groups are removed. +The account and home directory stay on the machine as the durable departure record, so a wrong +departure decision is a reversible lockout: a member who rejoins is synced back into their account +with their files, UID and GID intact. -Sockets and other special files cannot be stored in a tar archive and are skipped with a warning. -Only the three most recent archives are kept per user, older ones are pruned after a successful -archive. In dry-run mode, archiving is neither performed nor validated. +Accounts that have been expired for longer than the retention period (180 days by default, +configurable with `--purge-after-days`) are purged at the end of each sync: the account and its home +directory are removed permanently and without an archive. A purge only happens when the tombstone in +the users database and the account's own shadow expiry agree on the age of the departure and the +member is absent from the fetched member list. The purge can also be run explicitly: + +```sh +octosync purge --org --app-id --private-key +``` + +Even after a purge the users database remembers the member's UID and GID, so a member rejoining +later still gets their old IDs back, with an empty home directory. ## Development From 70761e15420ae5c8c062c44fa81eb91dc819b1b6 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 17:08:16 +0200 Subject: [PATCH 06/12] feat: Route every account mutation through one shared name and UID cross-check SyncSupplementaryGroups resolved the account by the stored UID alone and replaced that account's supplementary groups. Since octosync owns the supplementary groups of synced users, a stale UID stripped e.g. sudo from an unrelated account that reuses it. The resolver returns missing or matches and treats a store that disagrees with the system as an error, and UpdateUser, ExpireAccount, PurgeAccount and the group sync all route through it. UpdateUser passes the GitHub login as the accepted alternate name, so a rename whose store update was lost is still picked up instead of refused. --- CHANGELOG.md | 4 ++ src/user_manager/backends/linux.rs | 83 +++++++++++++++++------------- 2 files changed, 50 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 68d13b3..e88cbae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,10 @@ - A dry run no longer writes the users database: previously new members were persisted with invented mock UIDs and leavers were tombstoned for real, so a later real run acted on preview data. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Every mutating operation resolves the account through one shared name and UID cross-check. Syncing + supplementary groups previously trusted the stored UID alone, so a stale UID could strip the + supplementary groups (e.g. `sudo`) of an unrelated account that reuses it. + ([#25](https://github.com/jonasehrlich/octosync/pull/25)) - Track the primary group GID in the store and re-create a rejoining member's private group with `groupadd --gid ` before `useradd --gid `, so group ownership of files outside the home directory also survives a removal and re-create cycle. A GID that meanwhile belongs to diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index c414e1f..cdccbbc 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -328,25 +328,15 @@ impl hannibal::Handler for LinuxUserManager { msg: UpdateUser, ) -> anyhow::Result { let (gh_user, available_user) = (&msg.gh_user, &msg.available_user); - let linux_user = match nix::unistd::User::from_uid(available_user.uid())? { - Some(linux_user) => linux_user, + // The GitHub login is accepted as an alternate name so a rename whose store + // update was lost is picked up instead of refused + let linux_user = match resolve_account(available_user, Some(&gh_user.login))? { + AccountResolution::Matches(linux_user) => linux_user, // The account is known in the store but gone from the system, e.g. deleted // by hand - None => recreate_account(gh_user, available_user).await?, + AccountResolution::Missing => recreate_account(gh_user, available_user).await?, }; - // The stored UID may have been freed and recycled by an unrelated account. Only - // the stored name (the normal case) or the GitHub login (a rename whose store - // update was lost) prove this is the managed account. - if linux_user.name != available_user.name() && linux_user.name != gh_user.login { - anyhow::bail!( - "UID {} belongs to '{}', not to stored user '{}', refusing to update", - available_user.uid(), - linux_user.name, - available_user.name() - ); - } - // The account may carry an expiry that should no longer be in effect, e.g. // one set by an operator: octosync owns the lifecycle of synced users clear_departure_expiry(&linux_user.name).await?; @@ -412,7 +402,7 @@ impl hannibal::Handler for LinuxUserManager { _ctx: &mut hannibal::Context, msg: ExpireAccount, ) -> anyhow::Result<()> { - let Some(linux_user) = resolve_account_checked(&msg.user)? else { + let AccountResolution::Matches(linux_user) = resolve_account(&msg.user, None)? else { tracing::warn!("User not found in system when attempting to expire, nothing to do"); return Ok(()); }; @@ -520,7 +510,7 @@ impl hannibal::Handler for LinuxUserManager { _ctx: &mut hannibal::Context, msg: PurgeAccount, ) -> anyhow::Result { - let Some(linux_user) = resolve_account_checked(&msg.user)? else { + let AccountResolution::Matches(linux_user) = resolve_account(&msg.user, None)? else { return Ok(PurgeOutcome::NoAccount); }; @@ -571,14 +561,15 @@ impl hannibal::Handler for LinuxUserManager { msg: SyncSupplementaryGroups, ) -> anyhow::Result<()> { let user = &msg.user; - let linux_user = nix::unistd::User::from_uid(user.uid()) - .context("Failed to read user before syncing groups")? - .ok_or_else(|| { - anyhow::anyhow!( - "User '{}' was not found while syncing supplementary groups", - user.name() - ) - })?; + // octosync owns the supplementary groups of synced users, so acting on a + // stale UID would strip the groups of an unrelated account + let linux_user = match resolve_account(user, None)? { + AccountResolution::Matches(linux_user) => linux_user, + AccountResolution::Missing => anyhow::bail!( + "User '{}' was not found while syncing supplementary groups", + user.name() + ), + }; let primary_group_name = nix::unistd::Group::from_gid(linux_user.gid) .context("Failed to read primary group while syncing groups")? @@ -656,18 +647,36 @@ impl hannibal::Handler for LinuxUserManager { } } -/// Resolve the platform account of a stored user for its expiry or purge, refusing -/// when name and UID do not match. `Ok(None)` when no account with that name exists -/// and the UID is unused. -fn resolve_account_checked(user: &store::User) -> anyhow::Result> { - // usermod and userdel operate on the account name, so resolve the account by name - // to guarantee the account acted on is the one the name addresses +/// Result of [`resolve_account`]: the platform account of a stored user, or proof +/// that none exists. A store that disagrees with the system is an error, never a +/// variant, so no caller can act on a possibly wrong account. +enum AccountResolution { + /// No account with the stored name exists and the stored UID is unused + Missing, + /// The account whose name and UID agree with the store + Matches(nix::unistd::User), +} + +/// Resolve the platform account of a stored user, the one cross-check every mutating +/// handler routes through: shadow-utils commands operate on the account name, so the +/// name is authoritative and the stored UID must agree with it. Acting on a name +/// whose UID moved on, or a UID whose name moved on, would mutate an unrelated +/// account. +/// +/// `renamed_to` accepts one alternate name for the account holding the stored UID: +/// a usermod rename whose store update was lost leaves the account only findable by +/// UID, and the caller knows which new name proves it is still the managed account. +fn resolve_account( + user: &store::User, + renamed_to: Option<&str>, +) -> anyhow::Result { let Some(linux_user) = nix::unistd::User::from_name(user.name())? else { - // The account may exist under a different name, e.g. when a usermod rename - // succeeded but the store update was lost. Whether that is this user or an - // unrelated account that reuses the UID can not be decided here, refuse - // instead of acting on a possibly wrong account. if let Some(other_user) = nix::unistd::User::from_uid(user.uid())? { + if renamed_to.is_some_and(|name| name == other_user.name) { + return Ok(AccountResolution::Matches(other_user)); + } + // Whether this is the managed account under an unexpected name or an + // unrelated account that reuses the UID can not be decided here anyhow::bail!( "No user named '{}' in the system, but UID {} belongs to '{}', refusing to act \ on the account", @@ -676,7 +685,7 @@ fn resolve_account_checked(user: &store::User) -> anyhow::Result anyhow::Result Date: Fri, 14 Aug 2026 17:43:10 +0200 Subject: [PATCH 07/12] feat: Remove a departed member's scheduled work along with their sessions cron and at run jobs without a login, so ending the sessions left the departed account still executing code. Departure now deletes the user's crontab and their queued at jobs. Expiring the account already blocks the PAM account check both perform, so this is the second half of that guarantee and it also covers a machine whose cron does not consult PAM. The removal runs before the process sweep, so no job can spawn processes into the teardown. A machine without cron or at installed has no scheduled work to remove rather than a failing departure. --- CHANGELOG.md | 4 ++ README.md | 12 ++-- src/user_manager/backends/linux.rs | 111 +++++++++++++++++++++++++++-- src/user_manager/backends/mock.rs | 3 +- src/user_manager/messages.rs | 10 +-- 5 files changed, 126 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e88cbae..b7597a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,10 @@ with files, UID and GID intact. Expiry is re-applied on every sync, so an interrupted departure converges. A future expiry date set by an operator stays. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Remove a departed member's scheduled work along with their sessions: their crontab and queued `at` + jobs are deleted, since cron and at run without a login and would otherwise keep executing code + under the departed account. A machine without cron or at installed is not a failure. + ([#25](https://github.com/jonasehrlich/octosync/pull/25)) - Purge accounts that have been expired for longer than the retention period (180 days by default, `--purge-after-days`) at the end of each sync and through the new `purge` command: the account and home directory are removed permanently and without an archive. A purge requires the tombstone's diff --git a/README.md b/README.md index bd9db55..8d2296a 100644 --- a/README.md +++ b/README.md @@ -52,10 +52,14 @@ are removed. ### Departures: expiry instead of deletion When a member leaves the org, their account is not deleted. It is expired instead, which blocks both -password and pubkey SSH logins, running sessions are ended and the supplementary groups are removed. -The account and home directory stay on the machine as the durable departure record, so a wrong -departure decision is a reversible lockout: a member who rejoins is synced back into their account -with their files, UID and GID intact. +password and pubkey SSH logins, running sessions are ended, their crontab and queued `at` jobs are +removed and the supplementary groups are stripped. The account and home directory stay on the +machine as the durable departure record, so a wrong departure decision is a reversible lockout: a +member who rejoins is synced back into their account with their files, UID and GID intact. + +Sessions are ended through logind, which logs the user out cleanly and tears down their session +scopes. On a machine without logind, such as a container, the SIGTERM/SIGKILL process sweep is the +whole mechanism and the missing system bus is not reported as an error. Accounts that have been expired for longer than the retention period (180 days by default, configurable with `--purge-after-days`) are purged at the end of each sync: the account and its home diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index cdccbbc..88fa8ed 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -411,10 +411,11 @@ impl hannibal::Handler for LinuxUserManager { // the running ones are ended expire_account(&linux_user.name).await?; - // logind first: it logs the user out cleanly, closing the PAM sessions and - // tearing down the session scopes and the user manager instead of only - // signaling PIDs. The sweep afterwards is the catch-all for processes outside - // logind's session scopes, and the sole mechanism on systems without logind. + // Before the sweep, so no scheduled job can spawn processes into the teardown + remove_scheduled_jobs(&linux_user).await?; + + // logind first, then the sweep as the catch-all for processes outside logind's + // session scopes and as the sole mechanism on systems without logind end_logind_sessions(&linux_user).await; kill_processes_for_user(&linux_user).await?; @@ -487,6 +488,108 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho .context("Failed to terminate the user's sessions") } +/// Remove the departed user's scheduled work, which outlives their sessions: cron and +/// at run jobs without a login, so ending the sessions alone would leave the account +/// executing code after its departure. +/// +/// Expiring the account already blocks the PAM account check cron and at perform, so +/// this is the second half of that guarantee rather than the only one, and it also +/// covers a machine whose cron does not consult PAM. +async fn remove_scheduled_jobs(user: &nix::unistd::User) -> anyhow::Result<()> { + remove_crontab(&user.name).await?; + remove_at_jobs(&user.name).await +} + +/// Drop the user's crontab with `crontab -r`, which removes the spool file and makes +/// cron pick up the change. +/// +/// Listing first keeps this quiet in the common case of a user without a crontab: +/// `crontab -r` reports that as a failure, and telling it apart from a real one would +/// mean matching its message. +async fn remove_crontab(name: &str) -> anyhow::Result<()> { + let Some(list) = optional_command("/usr/bin/crontab", &["-l", "-u", name]).await? else { + return Ok(()); + }; + if !list.status.success() { + return Ok(()); + } + + let output = process::Command::new("/usr/bin/crontab") + .args(["-r", "-u", name]) + .output() + .await + .context("Failed to wait for crontab command to finish")?; + if !output.status.success() { + anyhow::bail!( + "Failed to remove the crontab of '{name}': {}", + String::from_utf8_lossy(&output.stderr) + ); + } + tracing::info!("Removed the crontab of the departed user"); + Ok(()) +} + +/// Remove the user's queued `at` jobs. Running as root, `atq` lists the jobs of every +/// user with the owner in the last field, which is the only way to select one user's +/// jobs for `atrm`. +async fn remove_at_jobs(name: &str) -> anyhow::Result<()> { + let Some(queue) = optional_command("/usr/bin/atq", &[]).await? else { + return Ok(()); + }; + if !queue.status.success() { + anyhow::bail!( + "Failed to list the at jobs of '{name}': {}", + String::from_utf8_lossy(&queue.stderr) + ); + } + + let job_ids: Vec = String::from_utf8_lossy(&queue.stdout) + .lines() + .filter_map(|line| { + let mut fields = line.split_whitespace(); + let job_id = fields.next()?; + (fields.last()? == name).then(|| job_id.to_string()) + }) + .collect(); + if job_ids.is_empty() { + return Ok(()); + } + + let output = process::Command::new("/usr/bin/atrm") + .args(&job_ids) + .output() + .await + .context("Failed to wait for atrm command to finish")?; + if !output.status.success() { + anyhow::bail!( + "Failed to remove the at jobs of '{name}': {}", + String::from_utf8_lossy(&output.stderr) + ); + } + tracing::info!( + jobs = job_ids.len(), + "Removed the at jobs of the departed user" + ); + Ok(()) +} + +/// Run a command that only exists when the service behind it is installed. `Ok(None)` +/// when the binary is missing, so a machine without cron or at has no scheduled work +/// to remove rather than a failing departure. +async fn optional_command( + program: &str, + args: &[&str], +) -> anyhow::Result> { + match process::Command::new(program).args(args).output().await { + Ok(output) => Ok(Some(output)), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => { + tracing::debug!("'{program}' is not installed, no scheduled work to remove with it"); + Ok(None) + } + Err(e) => Err(e).with_context(|| format!("Failed to run '{program}'")), + } +} + /// Remove the user from all supplementary groups, so a departed member keeps no /// access granted through them. The usermod is skipped when there is nothing to /// remove, keeping the re-expiry on every sync cheap. diff --git a/src/user_manager/backends/mock.rs b/src/user_manager/backends/mock.rs index 5259d88..d291410 100644 --- a/src/user_manager/backends/mock.rs +++ b/src/user_manager/backends/mock.rs @@ -115,7 +115,8 @@ impl hannibal::Handler for MockUserManager { msg: ExpireAccount, ) -> anyhow::Result<()> { tracing::info!( - "Would expire the account, end its sessions and strip its supplementary groups" + "Would expire the account, remove its scheduled work, end its sessions and strip \ + its supplementary groups" ); Ok(()) } diff --git a/src/user_manager/messages.rs b/src/user_manager/messages.rs index 6dd5dbc..2a90165 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -48,11 +48,11 @@ pub struct UpdateUser { /// Expires the platform account of a departed user, replacing its deletion: verifies /// that the stored user still matches the platform account, expires it so no new -/// session (password or pubkey SSH) can start, ends the running sessions and strips -/// the supplementary groups. The account, its home directory and its primary group -/// stay on the machine as the durable departure record, so the departure is a -/// reversible lockout that a rejoin heals, until the purge removes the account after -/// the retention period. +/// session (password or pubkey SSH) can start, removes its scheduled work, ends the +/// running sessions and strips the supplementary groups. The account, its home +/// directory and its primary group stay on the machine as the durable departure +/// record, so the departure is a reversible lockout that a rejoin heals, until the +/// purge removes the account after the retention period. /// /// Idempotent, so the sync can send it for every departed user on every run and an /// expiry interrupted at any point converges. From 8a755ccf640f413a462f8345f3eb58f30b69cf11 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 18:34:32 +0200 Subject: [PATCH 08/12] chore: Cleanup changelog --- CHANGELOG.md | 93 +++++++++++++++------------------------------------- 1 file changed, 26 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b7597a0..48a230b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,77 +2,36 @@ ## Unreleased -- Only expire users whose GitHub ID is missing from the fetched org member list. Users whose - processing failed stay in the store unchanged. - ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- A failed authorized keys fetch no longer fails user processing which could lead to the removal of - users whose keys could not be fetched. ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- Fetch public keys through the authenticated GitHub client instead of the anonymous, rate-limited - endpoint ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- Manage SSH fetched keys in a marked, octosync-managed block inside `authorized_keys` - ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- Write `authorized_keys` atomically, never follow symlinks and enforce exact permissions on `.ssh` - and `authorized_keys` ([#15](https://github.com/jonasehrlich/octosync/pull/15)) -- Refuse a sync that would expire every stored user - ([#17](https://github.com/jonasehrlich/octosync/pull/17)) -- Implement mapping GitHub teams to Linux groups with `--group :` - ([#20](https://github.com/jonasehrlich/octosync/pull/20)) -- Re-create stored users whose account disappeared from the system, passing the stored name and UID - to `useradd` so file ownership survives a removal and re-create cycle - ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Refuse to update a user whose stored UID or name belongs to a different account, instead of - renaming that account and handing it the GitHub user's SSH keys - ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Stop user processes gracefully: send SIGTERM first and SIGKILL only the processes still running - after a grace period, then wait for the killed processes to leave the process table - ([#19](https://github.com/jonasehrlich/octosync/pull/19)) -- Replace account deletion with expiry: a departed member's account is expired with - `usermod --expiredate`, which blocks password and pubkey SSH logins, their sessions are ended - through logind (with the process sweep as the catch-all) and their supplementary groups are - removed. The account and home directory stay on the machine as the durable departure record, so a - wrong departure decision is a reversible lockout that the next sync of a rejoining member heals, - with files, UID and GID intact. Expiry is re-applied on every sync, so an interrupted departure - converges. A future expiry date set by an operator stays. +- Departed members are now locked out instead of immediately deleted: octosync expires their + account, ends their sessions, removes their crontab and queued `at` jobs, and strips supplementary + groups while keeping the account and home directory intact. Rejoining reverses the expiry and + restores access to the same files and IDs. The `delete` command now follows this reversible + departure flow as well. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Expired accounts are permanently purged after a configurable retention period (180 days by + default), either during sync or with the new `purge` command. Purging removes the home directory + without an archive, but reserves the departed member's UID and GID for a later rejoin. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Remove a departed member's scheduled work along with their sessions: their crontab and queued `at` - jobs are deleted, since cron and at run without a login and would otherwise keep executing code - under the departed account. A machine without cron or at installed is not a failure. - ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Purge accounts that have been expired for longer than the retention period (180 days by default, - `--purge-after-days`) at the end of each sync and through the new `purge` command: the account and - home directory are removed permanently and without an archive. A purge requires the tombstone's - departure timestamp and the account's own shadow expiry to agree on the age, and the member to be - absent from the fetched member list. The tombstone survives the purge, so even a member rejoining - later gets their old UID and GID back. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Refuse to create a user whose GitHub login a departed member's tombstone reserves under a - different GitHub ID: a recycled login must not adopt the previous owner's expired account and home - directory. ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Store `users.json` in a versioned v2 schema: members who left are kept as tombstones under a - `departed` key while their account is expired, and under a `purged` key after the purge, so a - member who leaves and rejoins gets their old UID back. The v1 file is backed up to `users-v1.json` - once before the first v2 save. ([#22](https://github.com/jonasehrlich/octosync/pull/22), +- Sync no longer offboards members because their processing or key fetch failed, and refuses a sync + that would expire every stored user. Dry runs no longer write preview state to `users.json`. + ([#15](https://github.com/jonasehrlich/octosync/pull/15), + [#17](https://github.com/jonasehrlich/octosync/pull/17), + [#25](https://github.com/jonasehrlich/octosync/pull/25)) +- Account identity is preserved across departures, manual account removal and rejoins. octosync + restores stored UIDs and GIDs, prevents their reuse by other members, and refuses unsafe login or + ID collisions rather than modifying the wrong account. + ([#19](https://github.com/jonasehrlich/octosync/pull/19), + [#22](https://github.com/jonasehrlich/octosync/pull/22), [#25](https://github.com/jonasehrlich/octosync/pull/25)) -- The `delete` command expires all managed users and keeps the store file with tombstones instead of - removing `users.json`, preserving the UID memory of a full wipe +- GitHub SSH keys are now fetched through the authenticated client and updated inside an + octosync-managed block, preserving manually managed keys. Existing keys survive fetch failures, + and updates are atomic and protected against unsafe permissions and symlinks. + ([#15](https://github.com/jonasehrlich/octosync/pull/15)) +- GitHub teams can now be mapped to Linux groups with `--group :`. + ([#20](https://github.com/jonasehrlich/octosync/pull/20)) +- `users.json` is automatically migrated to a versioned v2 format that records departed and purged + members. The original v1 file is backed up once as `users-v1.json`. ([#22](https://github.com/jonasehrlich/octosync/pull/22), [#25](https://github.com/jonasehrlich/octosync/pull/25)) -- A dry run no longer writes the users database: previously new members were persisted with invented - mock UIDs and leavers were tombstoned for real, so a later real run acted on preview data. - ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Every mutating operation resolves the account through one shared name and UID cross-check. Syncing - supplementary groups previously trusted the stored UID alone, so a stale UID could strip the - supplementary groups (e.g. `sudo`) of an unrelated account that reuses it. - ([#25](https://github.com/jonasehrlich/octosync/pull/25)) -- Track the primary group GID in the store and re-create a rejoining member's private group with - `groupadd --gid ` before `useradd --gid `, so group ownership of files outside the - home directory also survives a removal and re-create cycle. A GID that meanwhile belongs to - another group fails the re-creation loudly instead of falling back to a fresh one. Entries - migrated from a v1 store carry no GID and are backfilled on their next update. - ([#22](https://github.com/jonasehrlich/octosync/pull/22)) -- Never allocate a departed user's UID or GID to a brand-new member: `useradd` hands out the highest - ID in range plus one, which is exactly what purging the highest-UID user frees. A new account - whose auto-allocated IDs collide with a tombstone is removed while still empty and re-created with - explicitly chosen free IDs. ([#22](https://github.com/jonasehrlich/octosync/pull/22)) ## v0.3.0 From 33f41ad1c89e4b71ccd6331ed41e8edffc8bc58f Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 18:58:29 +0200 Subject: [PATCH 09/12] perf: Retry a departure teardown only until it completed The sync re-sent ExpireAccount for every departed tombstone on every run, so the full teardown -- a /proc walk, a system bus connection with its round trips and the crontab and atq spawns -- was paid for every tombstone on every sync, for the whole 180 day retention period. The cost grew with the number of departures the machine had ever seen. A departed tombstone now carries the timestamp of the teardown that completed, and only a tombstone without it is torn down. A teardown that failed or was interrupted leaves the timestamp empty and is retried by the next sync, so convergence stays store-driven instead of relying on re-sending the message blindly. The account of a departed member is no longer reconciled continuously as a consequence: an expiry lifted by hand stays lifted until the member is synced again. --- src/octosync.rs | 88 ++++++++++++++------- src/store.rs | 27 +++++++ src/user_manager/backends/linux.rs | 119 ++++++++++++++++++++++------- src/user_manager/messages.rs | 5 +- 4 files changed, 183 insertions(+), 56 deletions(-) diff --git a/src/octosync.rs b/src/octosync.rs index 6bb1f7a..a2760a8 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -335,14 +335,18 @@ impl Octosync { store.save().await } - /// Record the given users as departed and converge every departed account toward - /// its expiry. + /// Record the given users as departed and tear down every departed account whose + /// teardown has not completed yet. /// - /// The tombstones are saved before any account is expired, so the departure record - /// survives a crash anywhere in the expiry window. Every departed user is - /// re-expired on each sync: [`user_manager::ExpireAccount`] is idempotent, so an - /// expiry interrupted between the store save and the platform call is finished by - /// a later sync and a failed expiry needs no in-memory retry state. + /// The tombstones are saved before any account is torn down, so the departure + /// record survives a crash anywhere in the expiry window. A tombstone only carries + /// the completion timestamp once [`user_manager::ExpireAccount`] succeeded, so a + /// teardown that failed or was interrupted is retried by the next sync without + /// in-memory retry state, while a completed one is left alone instead of paying the + /// full teardown again on every sync of the retention period. + /// + /// The account of a departed member is therefore not reconciled continuously: an + /// expiry lifted by hand stays lifted until the member is synced again. async fn depart_and_expire( &self, store: &mut store::UserStore, @@ -355,23 +359,25 @@ impl Octosync { } store.save().await?; - // Failures are only logged: the tombstone stays in the store, so the expiry is - // retried on every sync. Sequential, the platform operations serialize on the - // user manager actor anyway. - for departed_user in store.departed().values() { - if let Err(e) = self - .user_manager - .expire_user(&store::User::from(departed_user)) - .await - { - tracing::error!( - "Failed to expire the account of '{}': {:?}", - departed_user.name(), - e - ); + let pending: Vec = store + .departed() + .values() + .filter(|tombstone| tombstone.expired_at().is_none()) + .map(store::User::from) + .collect(); + + // Failures are only logged: the tombstone keeps its empty completion timestamp, + // so the teardown is retried on the next sync. Sequential, the platform + // operations serialize on the user manager actor anyway. + for user in pending { + match self.user_manager.expire_user(&user).await { + Ok(()) => store.mark_expired(&user.id(), chrono::Utc::now()), + Err(e) => { + tracing::error!("Failed to expire the account of '{}': {:?}", user.name(), e) + } } } - Ok(()) + store.save().await } /// Process all org members concurrently and collect the successfully processed @@ -878,13 +884,15 @@ mod tests { .unwrap(); let tombstone = &saved.departed()[&octocrab::models::UserId(1)]; assert_eq!(tombstone.name(), "alice"); + // No completion timestamp, so the next sync retries the teardown + assert!(tombstone.expired_at().is_none()); assert!(saved.data().is_empty()); } - /// A departed user is re-expired on every sync, so an expiry that failed or - /// was interrupted converges without in-memory retry state. + /// A tombstone whose teardown never completed is retried by a later sync, so a + /// failed or interrupted expiry converges without in-memory retry state. #[tokio::test] - async fn depart_and_expire_re_expires_existing_tombstones() { + async fn depart_and_expire_retries_an_unfinished_tombstone() { let mut actor = TestingUserManager::default(); actor.expire_account.push_back(Ok(())); let expired_users = actor.expired_users.clone(); @@ -892,13 +900,41 @@ mod tests { let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store.depart_user(user(1, "alice"), chrono::Utc::now()); - // No leavers this sync, the existing tombstone alone drives the expiry + // No leavers this sync, the unfinished tombstone alone drives the teardown octosync .depart_and_expire(&mut store, vec![]) .await .unwrap(); assert_eq!(*expired_users.lock().unwrap(), ["alice"]); + assert!( + store.departed()[&octocrab::models::UserId(1)] + .expired_at() + .is_some() + ); + } + + /// A tombstone whose teardown completed is left alone for the rest of the + /// retention period, instead of paying the full teardown on every sync. + #[tokio::test] + async fn depart_and_expire_skips_a_finished_tombstone() { + // No scripted response: reaching the actor would fail with "No scripted + // response left", which the loop only logs, so the assertion below is what + // proves the message is never sent + let actor = TestingUserManager::default(); + let expired_users = actor.expired_users.clone(); + let (octosync, data_dir) = octosync_with(actor); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); + let now = chrono::Utc::now(); + store.depart_user(user(1, "alice"), now); + store.mark_expired(&octocrab::models::UserId(1), now); + + octosync + .depart_and_expire(&mut store, vec![]) + .await + .unwrap(); + + assert!(expired_users.lock().unwrap().is_empty()); } /// The `delete` command tombstones every user instead of removing the store diff --git a/src/store.rs b/src/store.rs index cc626bb..06d6573 100644 --- a/src/store.rs +++ b/src/store.rs @@ -119,6 +119,13 @@ pub struct DepartedUser { gid: Option, /// When the member departed and their account was expired departed_at: chrono::DateTime, + /// When the account teardown completed: the account expired, its scheduled work + /// removed, its sessions ended and its supplementary groups stripped. `None` while + /// the teardown has not run or did not finish, which is what makes the sync retry + /// it; a tombstone that carries the timestamp is left alone for the rest of the + /// retention period. + #[serde(default, skip_serializing_if = "Option::is_none")] + expired_at: Option>, } #[allow(unused)] @@ -147,6 +154,11 @@ impl DepartedUser { pub fn departed_at(&self) -> chrono::DateTime { self.departed_at } + + /// Get the time the account teardown completed, `None` while it still has to run + pub fn expired_at(&self) -> Option> { + self.expired_at + } } impl From<&DepartedUser> for User { @@ -318,10 +330,23 @@ impl UserStore { uid: user.uid, gid: user.gid, departed_at, + expired_at: None, }, ); } + /// Record that the account teardown of a departed user completed, so later syncs + /// leave the tombstone alone instead of tearing the account down again + pub fn mark_expired( + &mut self, + id: &octocrab::models::UserId, + expired_at: chrono::DateTime, + ) { + if let Some(departed) = self.departed.get_mut(id) { + departed.expired_at = Some(expired_at); + } + } + /// Move a departed tombstone to the purged map, keeping its IDs for a rejoin even /// after the purge pub fn mark_purged( @@ -492,6 +517,7 @@ mod tests { uid: unistd::Uid::from_raw(1005), gid: Some(unistd::Gid::from_raw(1005)), departed_at: departed_at(), + expired_at: Some(departed_at()), } } @@ -918,6 +944,7 @@ mod tests { uid: user.uid, gid: user.gid, departed_at: departed_at(), + expired_at: None, } } } diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index 88fa8ed..ad6172b 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -408,20 +408,43 @@ impl hannibal::Handler for LinuxUserManager { }; // Expire first, so no new session (password or pubkey SSH) can start while - // the running ones are ended + // the running ones are ended. The one step whose failure aborts the teardown: + // without the expiry the account stays reachable and the rest is cosmetic. expire_account(&linux_user.name).await?; + // The steps below revoke different kinds of access and none of them is a + // precondition of another, so a failing one must not skip the others: a + // process stuck in uninterruptible I/O may not keep the departed member in + // their supplementary groups for the whole retention period. The first error + // is returned once all of them ran, so the tombstone-driven retry on the next + // sync still converges. + let mut error = None; + // Before the sweep, so no scheduled job can spawn processes into the teardown - remove_scheduled_jobs(&linux_user).await?; + keep_first_error(&mut error, remove_scheduled_jobs(&linux_user).await); // logind first, then the sweep as the catch-all for processes outside logind's // session scopes and as the sole mechanism on systems without logind end_logind_sessions(&linux_user).await; - kill_processes_for_user(&linux_user).await?; + keep_first_error(&mut error, kill_processes_for_user(&linux_user).await); // octosync owns the supplementary groups of synced users, and a departed // member keeps no access granted through them - strip_supplementary_groups(&linux_user).await + keep_first_error(&mut error, strip_supplementary_groups(&linux_user).await); + + error.map_or(Ok(()), Err) + } +} + +/// Keep the first error of the independent teardown steps, so one failing step does +/// not skip the following ones. Only the first is returned, so a later one is logged +/// here instead of being lost. +fn keep_first_error(first: &mut Option, result: anyhow::Result<()>) { + if let Err(e) = result { + match first { + None => *first = Some(e), + Some(_) => tracing::error!("Another teardown step of the account failed: {e:#}"), + } } } @@ -435,30 +458,60 @@ impl hannibal::Handler for LinuxUserManager { /// machine, while a logind that answers and then refuses leaves session scopes and a /// lingering user manager behind that the sweep cannot clean up. async fn end_logind_sessions(user: &nix::unistd::User) { - let connection = match zbus::Connection::system().await { - Ok(connection) => connection, + let connection = match tokio::time::timeout(LOGIND_TIMEOUT, zbus::Connection::system()).await { + Ok(Ok(connection)) => connection, // A container or a machine without systemd: the process sweep is the whole // mechanism here, so this is an expected state rather than a failure - Err(e) => { + Ok(Err(e)) => { tracing::debug!("No system bus, ending sessions with the process sweep alone: {e}"); return; } + Err(_) => { + tracing::error!( + "Connecting to the system bus timed out, ending sessions with the process \ + sweep alone" + ); + return; + } }; if let Err(e) = terminate_logind_user(&connection, user.uid.as_raw()).await { tracing::error!("logind is available but did not end the user's sessions: {e:#}"); } } +/// Deadline for a single logind D-Bus operation. +/// +/// zbus applies no reply timeout of its own, and the bus does not enforce one on the +/// method calls it forwards, so a wedged logind would never answer. The teardown runs +/// inside the actor, which would leave every following platform operation waiting +/// behind it for as long as logind stays wedged. +const LOGIND_TIMEOUT: time::Duration = time::Duration::from_secs(5); + +/// Await a logind D-Bus operation, failing after [`LOGIND_TIMEOUT`] instead of waiting +/// for a reply that may never come. +async fn with_logind_timeout( + operation: &str, + call: impl Future>, +) -> anyhow::Result { + tokio::time::timeout(LOGIND_TIMEOUT, call) + .await + .with_context(|| format!("logind did not answer '{operation}' in time"))? + .with_context(|| format!("logind call '{operation}' failed")) +} + /// Disable linger for the user and terminate their sessions over logind's D-Bus API. /// /// `TerminateUser` returns once the stop jobs are queued rather than once the /// processes are gone, so the caller's sweep is what establishes that they exited. async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyhow::Result<()> { - let manager = zbus::Proxy::new( - connection, - "org.freedesktop.login1", - "/org/freedesktop/login1", - "org.freedesktop.login1.Manager", + let manager = with_logind_timeout( + "Manager proxy", + zbus::Proxy::new( + connection, + "org.freedesktop.login1", + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", + ), ) .await .context("Failed to create the logind manager proxy")?; @@ -466,26 +519,36 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho // Disabling linger keeps a `user@.service` from restarting user services after the // termination, but it is the lesser half: a surviving login session matters more, // so a failure here must not skip the termination below - if let Err(e) = manager - .call::<_, _, ()>("SetUserLinger", &(uid, false, false)) - .await + if let Err(e) = with_logind_timeout( + "SetUserLinger", + manager.call::<_, _, ()>("SetUserLinger", &(uid, false, false)), + ) + .await { - tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e}"); + tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e:#}"); } // GetUser errors when the user neither has a session nor lingers, the usual state - // of a departed account that is re-expired by a later sync: nothing to terminate - if manager - .call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid) - .await - .is_err() + // of a departed account whose teardown is retried by a later sync: nothing to + // terminate. + // A timed-out GetUser lands here too, which is the right move: a logind that is + // not answering will not terminate anything either, so the sweep takes over + // instead of the teardown waiting out a second deadline. + if let Err(e) = with_logind_timeout( + "GetUser", + manager.call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid), + ) + .await { + tracing::debug!("logind reports no user to terminate: {e:#}"); return Ok(()); } - manager - .call::<_, _, ()>("TerminateUser", &uid) - .await - .context("Failed to terminate the user's sessions") + with_logind_timeout( + "TerminateUser", + manager.call::<_, _, ()>("TerminateUser", &uid), + ) + .await + .context("Failed to terminate the user's sessions") } /// Remove the departed user's scheduled work, which outlives their sessions: cron and @@ -592,7 +655,7 @@ async fn optional_command( /// Remove the user from all supplementary groups, so a departed member keeps no /// access granted through them. The usermod is skipped when there is nothing to -/// remove, keeping the re-expiry on every sync cheap. +/// remove. async fn strip_supplementary_groups(user: &nix::unistd::User) -> anyhow::Result<()> { let current = current_supplementary_groups(user) .with_context(|| format!("Failed to read current groups of '{}'", user.name))?; @@ -876,8 +939,8 @@ fn days_since_epoch() -> anyhow::Result { /// its date, so the departure day itself could keep logins open until midnight. async fn expire_account(name: &str) -> anyhow::Result<()> { let expire_days = days_since_epoch()? - 1; - // A departed account is re-expired on every sync; skip the usermod when the - // expiry is already in effect + // A teardown that is retried meets an account that is already expired; skip the + // usermod when the expiry is in effect if account_expire_days(name)?.is_some_and(|days| days <= expire_days) { return Ok(()); } diff --git a/src/user_manager/messages.rs b/src/user_manager/messages.rs index 2a90165..ad54544 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -54,8 +54,9 @@ pub struct UpdateUser { /// record, so the departure is a reversible lockout that a rejoin heals, until the /// purge removes the account after the retention period. /// -/// Idempotent, so the sync can send it for every departed user on every run and an -/// expiry interrupted at any point converges. +/// Idempotent, so an expiry interrupted at any point converges when a later sync +/// retries it. The sync only sends it for a tombstone whose teardown has not completed +/// yet, so the retry is driven by the store rather than by re-sending it every run. #[hannibal::message(response = anyhow::Result<()>)] pub struct ExpireAccount { pub user: store::User, From 045b66d1ea4c3bcba3672af0c328eea06366508e Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 18:58:50 +0200 Subject: [PATCH 10/12] fix: Keep a wedged logind or one failed teardown step from stalling departures Two ways the departure teardown could stop making progress. zbus applies no reply timeout of its own and the bus does not enforce one on the method calls it forwards, so a logind that accepts the call and never answers left the teardown waiting forever. It runs inside the user manager actor, so that stalled every following platform operation while the process kept holding the lockfile, and the sync stopped without failing. The logind conversation now has a deadline and falls back to the process sweep, which the design already treats as the catch-all. The teardown steps also ran as one fallible sequence, so a process stuck in uninterruptible I/O or an unhealthy at queue skipped the steps behind it. The supplementary groups of a departed member could stay in place for the whole retention period because of a stuck process, failing the same way on every sync. Only the expiry is a precondition of the rest and stays fatal; the remaining steps now all run before the first error is returned, so the retry still converges. --- src/user_manager/backends/linux.rs | 117 ++++++++++------------------- 1 file changed, 40 insertions(+), 77 deletions(-) diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index ad6172b..076d215 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -412,39 +412,24 @@ impl hannibal::Handler for LinuxUserManager { // without the expiry the account stays reachable and the rest is cosmetic. expire_account(&linux_user.name).await?; - // The steps below revoke different kinds of access and none of them is a - // precondition of another, so a failing one must not skip the others: a - // process stuck in uninterruptible I/O may not keep the departed member in - // their supplementary groups for the whole retention period. The first error - // is returned once all of them ran, so the tombstone-driven retry on the next - // sync still converges. - let mut error = None; + // None of the steps below is a precondition of another, so they are all run + // before the first error is returned: a process stuck in uninterruptible I/O + // may not keep the departed member in their supplementary groups for the whole + // retention period. Returning an error is what makes the next sync retry. // Before the sweep, so no scheduled job can spawn processes into the teardown - keep_first_error(&mut error, remove_scheduled_jobs(&linux_user).await); + let scheduled_jobs = remove_scheduled_jobs(&linux_user).await; // logind first, then the sweep as the catch-all for processes outside logind's // session scopes and as the sole mechanism on systems without logind end_logind_sessions(&linux_user).await; - keep_first_error(&mut error, kill_processes_for_user(&linux_user).await); + let sweep = kill_processes_for_user(&linux_user).await; // octosync owns the supplementary groups of synced users, and a departed // member keeps no access granted through them - keep_first_error(&mut error, strip_supplementary_groups(&linux_user).await); + let groups = strip_supplementary_groups(&linux_user).await; - error.map_or(Ok(()), Err) - } -} - -/// Keep the first error of the independent teardown steps, so one failing step does -/// not skip the following ones. Only the first is returned, so a later one is logged -/// here instead of being lost. -fn keep_first_error(first: &mut Option, result: anyhow::Result<()>) { - if let Err(e) = result { - match first { - None => *first = Some(e), - Some(_) => tracing::error!("Another teardown step of the account failed: {e:#}"), - } + scheduled_jobs.and(sweep).and(groups) } } @@ -467,51 +452,38 @@ async fn end_logind_sessions(user: &nix::unistd::User) { return; } Err(_) => { - tracing::error!( - "Connecting to the system bus timed out, ending sessions with the process \ - sweep alone" - ); + tracing::error!("System bus did not answer in time, leaving the sessions to the sweep"); return; } }; - if let Err(e) = terminate_logind_user(&connection, user.uid.as_raw()).await { - tracing::error!("logind is available but did not end the user's sessions: {e:#}"); + + // zbus applies no reply timeout of its own and the bus does not enforce one on the + // method calls it forwards, so a wedged logind never answers. The teardown runs + // inside the actor, where waiting for it would stall every following platform + // operation, so the conversation gets one deadline and the sweep takes over. + let uid = user.uid.as_raw(); + match tokio::time::timeout(LOGIND_TIMEOUT, terminate_logind_user(&connection, uid)).await { + Ok(Ok(())) => {} + Ok(Err(e)) => tracing::error!("logind is available but did not end the sessions: {e:#}"), + Err(_) => { + tracing::error!("logind did not answer in time, leaving the sessions to the sweep") + } } } -/// Deadline for a single logind D-Bus operation. -/// -/// zbus applies no reply timeout of its own, and the bus does not enforce one on the -/// method calls it forwards, so a wedged logind would never answer. The teardown runs -/// inside the actor, which would leave every following platform operation waiting -/// behind it for as long as logind stays wedged. +/// Deadline for the logind conversation, see [`end_logind_sessions`] const LOGIND_TIMEOUT: time::Duration = time::Duration::from_secs(5); -/// Await a logind D-Bus operation, failing after [`LOGIND_TIMEOUT`] instead of waiting -/// for a reply that may never come. -async fn with_logind_timeout( - operation: &str, - call: impl Future>, -) -> anyhow::Result { - tokio::time::timeout(LOGIND_TIMEOUT, call) - .await - .with_context(|| format!("logind did not answer '{operation}' in time"))? - .with_context(|| format!("logind call '{operation}' failed")) -} - /// Disable linger for the user and terminate their sessions over logind's D-Bus API. /// /// `TerminateUser` returns once the stop jobs are queued rather than once the /// processes are gone, so the caller's sweep is what establishes that they exited. async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyhow::Result<()> { - let manager = with_logind_timeout( - "Manager proxy", - zbus::Proxy::new( - connection, - "org.freedesktop.login1", - "/org/freedesktop/login1", - "org.freedesktop.login1.Manager", - ), + let manager = zbus::Proxy::new( + connection, + "org.freedesktop.login1", + "/org/freedesktop/login1", + "org.freedesktop.login1.Manager", ) .await .context("Failed to create the logind manager proxy")?; @@ -519,36 +491,27 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho // Disabling linger keeps a `user@.service` from restarting user services after the // termination, but it is the lesser half: a surviving login session matters more, // so a failure here must not skip the termination below - if let Err(e) = with_logind_timeout( - "SetUserLinger", - manager.call::<_, _, ()>("SetUserLinger", &(uid, false, false)), - ) - .await + if let Err(e) = manager + .call::<_, _, ()>("SetUserLinger", &(uid, false, false)) + .await { - tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e:#}"); + tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e}"); } // GetUser errors when the user neither has a session nor lingers, the usual state // of a departed account whose teardown is retried by a later sync: nothing to - // terminate. - // A timed-out GetUser lands here too, which is the right move: a logind that is - // not answering will not terminate anything either, so the sweep takes over - // instead of the teardown waiting out a second deadline. - if let Err(e) = with_logind_timeout( - "GetUser", - manager.call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid), - ) - .await + // terminate + if manager + .call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid) + .await + .is_err() { - tracing::debug!("logind reports no user to terminate: {e:#}"); return Ok(()); } - with_logind_timeout( - "TerminateUser", - manager.call::<_, _, ()>("TerminateUser", &uid), - ) - .await - .context("Failed to terminate the user's sessions") + manager + .call::<_, _, ()>("TerminateUser", &uid) + .await + .context("Failed to terminate the user's sessions") } /// Remove the departed user's scheduled work, which outlives their sessions: cron and From 2ebe64c58cfdcb3a7ba9afbb811f04254de20831 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 20:53:00 +0200 Subject: [PATCH 11/12] refactor: Simplify account lifecycle code --- src/authorized_keys.rs | 80 +++------ src/groups.rs | 25 +-- src/main.rs | 4 +- src/octosync.rs | 143 +++++---------- src/store.rs | 69 ++------ src/user_manager.rs | 33 +--- src/user_manager/backends/linux.rs | 270 +++++++++-------------------- src/user_manager/messages.rs | 73 +++----- 8 files changed, 201 insertions(+), 496 deletions(-) diff --git a/src/authorized_keys.rs b/src/authorized_keys.rs index 2c5a55f..04e1f82 100644 --- a/src/authorized_keys.rs +++ b/src/authorized_keys.rs @@ -1,15 +1,6 @@ -//! Management of authorized_keys files for system users. -//! -//! Works on any Unix platform. Fetched keys live in a marked, octosync-managed block that is -//! replaced in place on every sync, so a key revoked on GitHub is removed from the block. -//! Lines outside the block are preserved byte-for-byte at their position and never removed, -//! so keys installed through other channels (or manually) stay intact. -//! -//! All operations run as root inside user-owned directories, so nothing here follows -//! symlinks: files are opened with O_NOFOLLOW, replacements are staged by -//! [`atomic_write_file`], and permissions and ownership are set through the opened file -//! descriptor. Otherwise a user could, for example, symlink their authorized_keys to an -//! arbitrary file and have root truncate, chown or disclose it. +//! Maintains an octosync-managed block in each user's authorized_keys file while +//! preserving all manual entries. Files are opened without following symlinks and +//! replaced atomically because these operations run as root in user-owned directories. use crate::{public_keys, store}; use anyhow::Context as _; @@ -21,52 +12,37 @@ const SSH_DIR_MODE: u32 = 0o700; const AUTHORIZED_KEYS_MODE: u32 = 0o600; const AUTHORIZED_KEYS_FILE_NAME: &str = "authorized_keys"; -/// Marker line opening the octosync-managed block in an authorized_keys file const MANAGED_BLOCK_START: &str = "# >>> octosync managed keys - do not edit this block >>>"; -/// Marker line closing the octosync-managed block in an authorized_keys file const MANAGED_BLOCK_END: &str = "# <<< octosync managed keys - do not edit this block <<<"; -/// Manages the authorized_keys files of system users, resolving each account's actual home -/// directory from the user database. -#[derive(Clone, Debug, Default)] -pub struct AuthorizedKeysManager; - -impl AuthorizedKeysManager { - /// Replaces the octosync-managed key block in the user's authorized_keys file with - /// the given keys. Lines outside the managed block are never touched. - #[tracing::instrument( - name = "AuthorizedKeysManager::update_authorized_keys", - skip_all, - fields(user = %user.name()) - )] - pub async fn update_authorized_keys( - &self, - user: &store::User, - keys: &public_keys::PublicKeys, - ) -> anyhow::Result<()> { - let system_user = nix::unistd::User::from_uid(user.uid()) - .context("Failed to look up user before updating authorized_keys")? - .ok_or_else(|| { - anyhow::anyhow!( - "User '{}' not found in system when updating authorized_keys", - user.name() - ) - })?; +#[tracing::instrument( + name = "authorized_keys::update_authorized_keys", + skip_all, + fields(user = %user.name()) +)] +pub async fn update_authorized_keys( + user: &store::User, + keys: &public_keys::PublicKeys, +) -> anyhow::Result<()> { + let system_user = nix::unistd::User::from_uid(user.uid()) + .context("Failed to look up user before updating authorized_keys")? + .ok_or_else(|| { + anyhow::anyhow!( + "User '{}' not found in system when updating authorized_keys", + user.name() + ) + })?; - sync_authorized_keys( - &system_user.dir.join(".ssh"), - keys, - system_user.uid, - system_user.gid, - ) - .await - } + sync_authorized_keys( + &system_user.dir.join(".ssh"), + keys, + system_user.uid, + system_user.gid, + ) + .await } -/// Replace the octosync-managed block in `/authorized_keys` with `keys`, creating the -/// directory and file with the required permissions if they don't exist. Lines outside the -/// managed block are preserved byte-for-byte. The file is replaced atomically, so an -/// interrupted update cannot leave a truncated authorized_keys behind. +/// Replace the managed block while preserving content outside it. async fn sync_authorized_keys( ssh_dir: &path::Path, keys: &public_keys::PublicKeys, diff --git a/src/groups.rs b/src/groups.rs index 4dfbbce..b3b1ea8 100644 --- a/src/groups.rs +++ b/src/groups.rs @@ -1,14 +1,12 @@ -//! Resolution of `--group` CLI arguments into per-user Linux group assignments +//! Resolves `--group` arguments into per-user Linux group assignments. use anyhow::Context as _; use futures::{StreamExt as _, TryStreamExt as _, stream}; use std::{collections, str}; -/// Maximum number of team member fetches in flight at the same time const MAX_CONCURRENT_TEAM_FETCHES: usize = 8; -/// A single `--group` argument. Either a Linux group for every synced user or a -/// mapping from a GitHub team to a Linux group. +/// A global Linux group or a GitHub-team-to-Linux-group mapping. #[derive(Debug, Clone, PartialEq, Eq)] pub enum GroupMapping { AddGroup(String), @@ -94,12 +92,7 @@ pub struct GroupAssignments { } impl GroupAssignments { - /// Resolve the CLI mappings by fetching the members of every mapped GitHub team. - /// - /// Mapped teams are checked against the org's team list, so a missing team is a - /// confirmed fact rather than a guess from an ambiguous response. Mappings whose - /// team does not exist are skipped with a warning: the Linux group is not created - /// and no longer assigned to any user. Any API error fails the resolution. + /// Resolve mappings after checking mapped teams against the org's team list. pub async fn resolve( octocrab: &octocrab::Octocrab, org: &str, @@ -109,8 +102,6 @@ impl GroupAssignments { Ok(Self::from_mappings(mappings, &team_members)) } - /// Combine the CLI mappings with the fetched team memberships, skipping mappings - /// whose GitHub team is missing from `team_members` (see [`Self::resolve`]) fn from_mappings( mappings: &[GroupMapping], team_members: &collections::HashMap>, @@ -153,8 +144,8 @@ impl GroupAssignments { groups.into_iter().cloned().collect() } - /// The supplementary groups for a single user: the global groups plus the groups - /// of the GitHub teams the user is a member of + /// The supplementary groups for a single user. This consists of the global groups plus the + /// groups of the GitHub teams the user is a member of pub fn user_groups(&self, id: octocrab::models::UserId) -> Vec { let mut groups: collections::BTreeSet<&String> = self.global.iter().collect(); if let Some(team_groups) = self.per_user.get(&id) { @@ -165,11 +156,6 @@ impl GroupAssignments { } /// Fetch the member IDs of every GitHub team referenced by a team mapping. -/// -/// The org's team list is the source of truth for which teams exist: mapped teams -/// missing from it are left out of the returned map, and members are only fetched for -/// teams it confirms. This keeps a transient member-fetch failure from ever being -/// mistaken for a missing team; any API error is returned instead. async fn get_members_of_mapped_teams( octocrab: &octocrab::Octocrab, org: &str, @@ -222,7 +208,6 @@ async fn get_org_team_slugs( Ok(teams.into_iter().map(|team| team.slug).collect()) } -/// List the member IDs of a team that is known to exist in the org async fn get_team_members( octocrab: &octocrab::Octocrab, org: &str, diff --git a/src/main.rs b/src/main.rs index c528cd5..cebed70 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,7 +3,7 @@ use anyhow::Context as _; use clap::Parser as _; use fs2::FileExt as _; -use std::{fs, path, sync}; +use std::{fs, path}; #[cfg_attr(not(target_os = "linux"), allow(dead_code))] mod authorized_keys; @@ -196,7 +196,7 @@ async fn main() -> anyhow::Result<()> { .to_path_buf(); let _process_lock = ProcessLock::acquire(&data_dir)?; - let app = octosync::Octosync::new(sync::Arc::new(args.global), &data_dir).await?; + let app = octosync::Octosync::new(args.global.dry_run, &data_dir); match args.command { Commands::Sync(a) => { diff --git a/src/octosync.rs b/src/octosync.rs index a2760a8..1c4e5d1 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -1,24 +1,16 @@ use crate::{ - GlobalArgs, InstallationClientArgs, PurgeArgs, SyncArgs, groups, public_keys, store, - user_manager, + InstallationClientArgs, PurgeArgs, SyncArgs, groups, public_keys, store, user_manager, }; use anyhow::Context as _; use futures::{StreamExt as _, stream}; -use std::{collections, path, sync, time}; +use std::{collections, path, time}; use tokio::fs; -/// Maximum number of users processed concurrently during a sync. Bounds the GitHub -/// requests in flight at the same time. The platform operations of each user are -/// serialized by the user manager actor. +/// Bounds concurrency of GitHub requests const MAX_CONCURRENT_USER_SYNCS: usize = 8; - -/// Maximum number of items per page supported by the GitHub API pub(crate) const GITHUB_MAX_PER_PAGE: u8 = 100; - -/// Timeout for establishing a connection to the GitHub API -const CONNECT_TIMEOUT: time::Duration = time::Duration::from_secs(10); -/// Timeout for individual socket reads/writes, so a hung request cannot stall the sync forever -const READ_WRITE_TIMEOUT: time::Duration = time::Duration::from_secs(30); +const GITHUB_CONNECT_TIMEOUT: time::Duration = time::Duration::from_secs(10); +const SOCKET_RW_TIMEOUT: time::Duration = time::Duration::from_secs(30); async fn org_client(args: &InstallationClientArgs) -> anyhow::Result { let private_key = fs::read(args.private_key.as_path()) @@ -33,9 +25,9 @@ async fn org_client(args: &InstallationClientArgs) -> anyhow::Result anyhow::Result, - data_dir: &path::Path, - ) -> anyhow::Result { + pub fn new(dry_run: bool, data_dir: &path::Path) -> Self { let user_manager = user_manager::UserManager::builder() - .dry_run(global_config.dry_run) + .dry_run(dry_run) .build(); - Ok(Self { + Self { data_dir: data_dir.to_path_buf(), - dry_run: global_config.dry_run, + dry_run, user_manager, - }) + } } #[tracing::instrument( @@ -94,7 +82,10 @@ impl Octosync { groups: &[String], ) -> anyhow::Result { let new_user = match store.data().get(&gh_user.id) { - Some(user) => self.manage_existing_user(gh_user, user).await?, + Some(user) => { + tracing::debug!("User exists in store"); + self.user_manager.update_user(gh_user, user).await? + } None => self.create_user(gh_user, store).await?, }; @@ -103,8 +94,7 @@ impl Octosync { .await .context("Failed to sync supplementary groups")?; - // A failed fetch must not fail processing the user: the authorized_keys file keeps its - // current keys and is refreshed on the next sync + // Keep existing keys when GitHub cannot refresh them. match public_keys::PublicKeys::fetch(octocrab, new_user.name()).await { Ok(keys) => { if keys.is_empty() { @@ -127,20 +117,18 @@ impl Octosync { /// Create the platform user, re-using the tombstoned IDs when the member rejoined. /// - /// A rejoin whose stored ID is taken fails loudly and is retried on the next - /// sync; falling back to fresh IDs would recreate exactly the ownership drift - /// the tombstones exist to prevent. A brand-new member must not be allocated an - /// ID a tombstone (departed or purged) reserves either, so the reserved IDs - /// travel with the creation. + /// A rejoin whose stored ID is taken fails and is retried on the next sync. + /// + /// Falling back to fresh IDs would create an ownership drift that should be prevented by the + /// tombstones. A brand-new member must not be allocated an ID that a tombstone (departed or + /// purged) reserves either, so the reserved IDs are propagated to the creation. async fn create_user( &self, gh_user: &octocrab::models::Author, store: &store::UserStore, ) -> anyhow::Result { - // With expiry, departed accounts stay on the machine as adoptable-by-name. A - // leaver's GitHub login can be released and claimed by a different person; - // adopting the expired account would hand that person the previous owner's - // home directory and UID, so the collision is refused and left to an operator. + // Refuse a recycled login for a different GitHub account, rather than expose the departed + // member's account. This is something that needs to be solved by an operator. if let Some(tombstone) = store .departed() .values() @@ -157,8 +145,7 @@ impl Octosync { ); } - // A departed member rejoins into their expired account; a purged one gets a - // fresh account and home directory under their old IDs + // Departed members reuse their account, purged members reuse only its IDs. let stored_ids = store .departed() .get(&gh_user.id) @@ -195,15 +182,6 @@ impl Octosync { self.user_manager.create_user(gh_user, ids).await } - async fn manage_existing_user( - &self, - gh_user: &octocrab::models::Author, - user: &store::User, - ) -> anyhow::Result { - tracing::debug!("User exists in store"); - self.user_manager.update_user(gh_user, user).await - } - #[tracing::instrument( name = "Octosync::sync", skip(self, args), @@ -275,16 +253,8 @@ impl Octosync { .await } - /// Purge departed users whose account has been expired for at least - /// `purge_after_days`, removing the account and its home directory permanently - /// and without an archive: the data is abandoned by policy. - /// - /// Eligibility requires the store-side and the account-side clock to agree: the - /// tombstone's departure timestamp must be at least the retention period old, and - /// the account's own shadow expiry (checked in the backend, where it is cleared by - /// any reactivation) must be as well. A member present in the fetched member list - /// is never purged. The tombstone survives the purge in the purged map, so a - /// member rejoining even later gets their old IDs back. + /// Permanently remove departed accounts once both the tombstone and account expiry + /// exceed the retention period. Current members are never purged. async fn purge_expired( &self, store: &mut store::UserStore, @@ -293,7 +263,6 @@ impl Octosync { ) -> anyhow::Result<()> { let now = chrono::Utc::now(); let cutoff = now - chrono::Duration::days(purge_after_days.into()); - // The shadow expiry field counts days since the epoch let expired_before = cutoff.timestamp() / (24 * 60 * 60); let candidates: Vec = store @@ -315,8 +284,7 @@ impl Octosync { ); store.mark_purged(&user.id(), now); } - // The account is gone but only the purge may spend the tombstone, so - // it stays departed; an operator resolves how the account disappeared + // Keep the tombstone until a purge succeeds. Ok(user_manager::PurgeOutcome::NoAccount) => tracing::warn!( "No account for departed user '{}', not marking the tombstone purged", user.name() @@ -335,18 +303,10 @@ impl Octosync { store.save().await } - /// Record the given users as departed and tear down every departed account whose - /// teardown has not completed yet. - /// - /// The tombstones are saved before any account is torn down, so the departure - /// record survives a crash anywhere in the expiry window. A tombstone only carries - /// the completion timestamp once [`user_manager::ExpireAccount`] succeeded, so a + /// Persist new departures, then retry every unfinished account teardown. A tombstone only + /// carries the completion timestamp once [`user_manager::ExpireAccount`] succeeded, so a /// teardown that failed or was interrupted is retried by the next sync without - /// in-memory retry state, while a completed one is left alone instead of paying the - /// full teardown again on every sync of the retention period. - /// - /// The account of a departed member is therefore not reconciled continuously: an - /// expiry lifted by hand stays lifted until the member is synced again. + /// in-memory retry state async fn depart_and_expire( &self, store: &mut store::UserStore, @@ -366,9 +326,7 @@ impl Octosync { .map(store::User::from) .collect(); - // Failures are only logged: the tombstone keeps its empty completion timestamp, - // so the teardown is retried on the next sync. Sequential, the platform - // operations serialize on the user manager actor anyway. + // A failed teardown keeps its empty completion timestamp for the next sync. for user in pending { match self.user_manager.expire_user(&user).await { Ok(()) => store.mark_expired(&user.id(), chrono::Utc::now()), @@ -380,12 +338,7 @@ impl Octosync { store.save().await } - /// Process all org members concurrently and collect the successfully processed - /// users into a new store. - /// - /// A processing failure is logged and leaves the user out of the returned store. - /// It never signals that the user left the org; [`partition_stale_users`] alone - /// decides which users depart. + /// Process members concurrently and collect successful results into a new store. async fn process_members( &self, octocrab: &octocrab::Octocrab, @@ -394,8 +347,6 @@ impl Octosync { assignments: &groups::GroupAssignments, ) -> anyhow::Result { let mut new_store = store::UserStore::new(&self.data_dir, self.dry_run).await?; - // Tombstones are carried over wholesale before any member is processed, so - // their survival never depends on per-user processing succeeding *new_store.departed_mut() = store.departed().clone(); *new_store.purged_mut() = store.purged().clone(); *new_store.data_mut() = stream::iter(org_members) @@ -415,8 +366,7 @@ impl Octosync { .map(|user| (user.id(), user)) .collect() .await; - // A tombstoned member that was processed has rejoined and their account is - // re-created, so the tombstone is spent + // Successful processing spends any tombstone for a rejoining member. new_store.prune_rejoined(); Ok(new_store) } @@ -436,9 +386,7 @@ impl Octosync { Ok(()) } - /// The explicit counterpart of the purge pass at the end of every sync, for - /// running the one deliberately irreversible action with a human in the loop. - /// The member list is fetched so a rejoined member is never purged. + /// Explicitly purge eligible departed users, excluding current org members. #[tracing::instrument( name = "Octosync::purge", skip(self, args), @@ -459,14 +407,8 @@ impl Octosync { } } -/// Split the users of the previous store that are missing from the new store into -/// users to keep for a retry and leavers whose account is expired. -/// -/// Departure is decided solely by absence from the fetched org member list. A user -/// that is still an org member but missing from the new store failed processing; -/// expiring them would turn any correlated per-user failure (e.g. rate-limited key -/// fetches) into a mass lockout, so they are kept unchanged and retried on the next -/// sync. +/// Separate failed current members, which remain for retry, from actual leavers based on the +/// fetched org member list. fn partition_stale_users<'a>( old_users: &'a collections::HashMap, new_users: &collections::HashMap, @@ -478,12 +420,7 @@ fn partition_stale_users<'a>( .partition(|user| org_member_map.contains_key(&user.id())) } -/// Circuit breaker for [`Octosync::sync`]. True when the pending departures would -/// empty a non-empty store. -/// -/// A member list that no longer contains a single stored user is far more likely a -/// mis-scoped installation, an org rename or an empty-but-successful API response than -/// every member leaving at once, so such a sync must refuse to expire anyone. +/// Refuse a sync that would expire every previously stored user. fn would_expire_all_users( old_users: &collections::HashMap, leavers: &[&store::User], diff --git a/src/store.rs b/src/store.rs index 06d6573..e4048b7 100644 --- a/src/store.rs +++ b/src/store.rs @@ -11,8 +11,7 @@ mod uid_serde { where S: Serializer, { - // Use the public getter provided by the crate - // Cast to u32 to keep the JSON format stable across OSs + // Keep the JSON format stable across platforms. #[allow(clippy::unnecessary_cast)] (uid.as_raw() as u32).serialize(serializer) } @@ -23,8 +22,6 @@ mod uid_serde { { let val = u32::deserialize(deserializer)?; - // Use the public constructor/factory provided by the crate - // The 'as _' handles the platform-specific uid_t conversion Ok(nix::unistd::Uid::from_raw(val as _)) } } @@ -36,7 +33,6 @@ mod gid_serde { where S: Serializer, { - // Cast to u32 to keep the JSON format stable across OSs #[allow(clippy::unnecessary_cast)] gid.map(|gid| gid.as_raw() as u32).serialize(serializer) } @@ -50,8 +46,7 @@ mod gid_serde { } } -/// Canonical representation of a user that exists both on GitHub and as a Linux user, -/// with the necessary information to manage their Linux account and SSH keys. +/// A GitHub member and their Linux account identity. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] pub struct User { /// GitHub user ID, used as the primary key for identifying users in the store @@ -68,18 +63,7 @@ pub struct User { gid: Option, } -#[allow(unused)] impl User { - /// Get the home directory path for this user, typically "/home/{name}" - pub fn home_dir(&self) -> path::PathBuf { - path::PathBuf::from(format!("/home/{}", self.name)) - } - - /// Get the SSH directory path for this user, typically "/home/{name}/.ssh" - pub fn ssh_dir(&self) -> path::PathBuf { - self.home_dir().join(".ssh") - } - /// Get the GitHub user ID pub fn id(&self) -> octocrab::models::UserId { self.id @@ -95,7 +79,7 @@ impl User { self.uid } - /// Get the GID of the user's primary group, if it is tracked + /// Get the Linux user GID of the primary group pub fn gid(&self) -> Option { self.gid } @@ -173,9 +157,7 @@ impl From<&DepartedUser> for User { } } -/// Tombstone of a departed user whose expired account and home directory were purged -/// after the retention period. It keeps the IDs so a member rejoining even after the -/// purge gets their old UID and GID back, with an empty home directory. +/// A purged member whose IDs remain reserved for a rejoin. #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, bon::Builder)] pub struct PurgedUser { /// GitHub user ID, used as the primary key for identifying users in the store @@ -246,7 +228,6 @@ struct StoreData { purged: PurgedMap, } -/// Borrowing counterpart of [`StoreData`] for serialization #[derive(Serialize)] struct StoreDataRef<'a> { version: u64, @@ -274,8 +255,7 @@ pub struct UserStore { } impl UserStore { - /// Create a new store instance with the given directory, without loading any data. - /// A store created with `dry_run` never writes on save. + /// Create an empty store without loading data. pub async fn new(dir: &path::Path, dry_run: bool) -> anyhow::Result { fs::create_dir_all(&dir).await?; Ok(Self { @@ -287,7 +267,7 @@ impl UserStore { }) } - /// Create a new store loading data from the directory + /// Load a store from a directory. #[tracing::instrument(name = "Store::from_dir")] pub async fn from_dir(dir: &path::Path, dry_run: bool) -> anyhow::Result { let mut s = Self::new(dir, dry_run).await?; @@ -335,8 +315,7 @@ impl UserStore { ); } - /// Record that the account teardown of a departed user completed, so later syncs - /// leave the tombstone alone instead of tearing the account down again + /// Mark a departed user's account teardown as complete. pub fn mark_expired( &mut self, id: &octocrab::models::UserId, @@ -347,8 +326,7 @@ impl UserStore { } } - /// Move a departed tombstone to the purged map, keeping its IDs for a rejoin even - /// after the purge + /// Move a departed tombstone to the purged map. pub fn mark_purged( &mut self, id: &octocrab::models::UserId, @@ -369,20 +347,17 @@ impl UserStore { } } - /// Drop the tombstones of users that are active again, keeping every user in - /// exactly one of the three maps + /// Drop tombstones for active users. pub fn prune_rejoined(&mut self) { self.departed.retain(|id, _| !self.users.contains_key(id)); self.purged.retain(|id, _| !self.users.contains_key(id)); } - /// Get the file path for the users database file fn path(&self) -> path::PathBuf { self.dir.join(USERS_FILE_NAME) } - /// Load the store from the file system, starting empty if the file doesn't exist. - /// A v1 file is backed up once and migrated with empty tombstone maps. + /// Load the store, starting empty when no database exists. #[tracing::instrument(name = "Store::load", skip(self))] async fn load(&mut self) -> anyhow::Result<()> { let path = self.path(); @@ -408,7 +383,6 @@ impl UserStore { .with_context(|| format!("Failed to parse users database from '{}'", path.display()))?; let data = match value.get("version") { - // A v1 file is a bare user map without a version key None => { let users: UserMap = serde_json::from_value(value).with_context(|| { format!( @@ -416,8 +390,7 @@ impl UserStore { path.display() ) })?; - // save() overwrites the file with v2, back it up first so rolling back - // to an older binary keeps working + // Preserve rollback compatibility before the next v2 save. self.backup_v1_file().await?; StoreData { users, @@ -430,7 +403,6 @@ impl UserStore { format!("Failed to parse users database from '{}'", path.display()) })? } - // Refuse an unknown newer version instead of silently dropping its data Some(version) => anyhow::bail!( "Unsupported users database version {version} in '{}', this binary supports version {STORE_VERSION}", path.display() @@ -459,9 +431,7 @@ impl UserStore { Ok(()) } - /// Write the store to the users database file. A dry run never writes: the - /// preview must not persist invented mock IDs or tombstone changes that a later - /// real run would act on. + /// Persist the store unless this is a dry run. pub async fn save(&self) -> anyhow::Result<()> { if self.dry_run { tracing::info!("Dry run: not writing the users database"); @@ -474,8 +444,7 @@ impl UserStore { purged: &self.purged, })?; let path = self.path(); - // Stage in a temporary file that atomically replaces the database on commit, so a - // failed write (e.g. on a full disk) can not truncate it + // Avoid truncating the database if the write fails. let dest = path.clone(); tokio::task::spawn_blocking(move || { use std::io::Write as _; @@ -537,18 +506,6 @@ mod tests { mod user { use super::*; - #[test] - fn home_dir() { - let user = user(); - assert_eq!(user.home_dir(), path::PathBuf::from("/home/testuser")); - } - - #[test] - fn ssh_dir() { - let user = user(); - assert_eq!(user.ssh_dir(), path::PathBuf::from("/home/testuser/.ssh")); - } - #[test] fn serialization() { let user = user(); diff --git a/src/user_manager.rs b/src/user_manager.rs index 1ac27a3..50090d9 100644 --- a/src/user_manager.rs +++ b/src/user_manager.rs @@ -1,13 +1,10 @@ -//! Platform user management behind a message-passing actor. +//! Serializes platform user management through a message-passing actor. This keeps +//! concurrent syncs from racing shadow-utils' passwd and group file locks. //! -//! The [`messages`] are the platform-neutral contract: every backend in [`backends`] +//! The [`messages`] are the platform-neutral contract. Every backend in [`backends`] //! is an actor handling the full message set, and [`UserManager`] is a -//! type-erased handle to one spawned backend. The actor processes one message at a -//! time, which serializes all account mutations: shadow-utils commands (useradd, -//! usermod, userdel, groupadd) fail with "cannot lock /etc/passwd" instead of waiting -//! when another invocation holds the lock, so the concurrent per-user syncs must never -//! run them in parallel. -//! +//! type-erased handle to one spawned backend. + pub mod backends; mod messages; @@ -20,10 +17,7 @@ use crate::{public_keys, store}; use anyhow::Context as _; use std::collections; -/// Handle to a spawned platform user manager actor. -/// -/// Cloning the handle clones the callers; all clones address the same actor, so the -/// one-message-at-a-time serialization holds across every user of the handle. +/// Type-erased handle to a platform user manager actor. #[derive(Clone)] pub struct UserManager { create_user: hannibal::Caller, @@ -50,7 +44,7 @@ impl UserManager { #[cfg(target_os = "linux")] { - Self::from_actor(backends::linux::LinuxUserManager::new()) + Self::from_actor(backends::linux::LinuxUserManager) } #[cfg(not(target_os = "linux"))] @@ -61,8 +55,6 @@ impl UserManager { } impl UserManager { - /// Spawn the actor and keep one caller per message type, erasing the concrete - /// actor type from the handle. fn from_actor(actor: A) -> Self where A: hannibal::Handler @@ -86,9 +78,6 @@ impl UserManager { } } - /// Sends [`CreateUser`] to the actor and awaits the created user. `ids` chooses - /// between the stored IDs of a rejoining member and fresh allocation, see - /// [`AccountIds`]. pub async fn create_user( &self, gh_user: &octocrab::models::Author, @@ -103,7 +92,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`UpdateUser`] to the actor and awaits the updated user. pub async fn update_user( &self, gh_user: &octocrab::models::Author, @@ -118,8 +106,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`ExpireAccount`] to the actor and awaits the expiry, the reversible - /// replacement for deleting the account. pub async fn expire_user(&self, user: &store::User) -> anyhow::Result<()> { self.expire_account .call(ExpireAccount { user: user.clone() }) @@ -127,8 +113,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`PurgeAccount`] to the actor and awaits the outcome. `expired_before` is - /// the account-side eligibility bound, see [`PurgeAccount`]. pub async fn purge_user( &self, user: &store::User, @@ -143,7 +127,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`SyncSupplementaryGroups`] to the actor and awaits the update. pub async fn sync_supplementary_groups( &self, user: &store::User, @@ -158,7 +141,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`EnsureGroupsExist`] to the actor and awaits the group creation. pub async fn ensure_groups_exist(&self, groups: &[String]) -> anyhow::Result<()> { self.ensure_groups_exist .call(EnsureGroupsExist { @@ -168,7 +150,6 @@ impl UserManager { .context(ACTOR_ERROR)? } - /// Sends [`UpdateAuthorizedKeys`] to the actor and awaits the key update. pub async fn update_authorized_keys( &self, user: &store::User, diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index 076d215..1f39766 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -1,7 +1,4 @@ -//! Linux backend running the shadow-utils commands (useradd, usermod, userdel, -//! groupadd). It only ever runs as an actor: the commands fail instead of waiting when -//! another invocation holds the lock on /etc/passwd or /etc/group, so the actor's -//! one-message-at-a-time processing is what keeps them from racing each other. +//! Actor-serialized Linux user management through shadow-utils. use crate::store; use crate::user_manager::{ @@ -13,17 +10,7 @@ use std::{collections, time}; use tokio::process; #[derive(Debug)] -pub struct LinuxUserManager { - authorized_keys: crate::authorized_keys::AuthorizedKeysManager, -} - -impl LinuxUserManager { - pub fn new() -> Self { - Self { - authorized_keys: crate::authorized_keys::AuthorizedKeysManager, - } - } -} +pub struct LinuxUserManager; impl hannibal::Actor for LinuxUserManager {} @@ -47,8 +34,7 @@ impl hannibal::Handler for LinuxUserManager { "User already exists. Skipping creation." ); - // The adopted account is usually a rejoining member's expired account: - // lifting the expiry is what turns the departure back into an active user + // Reactivate an adopted account for a rejoining member. clear_departure_expiry(&existing_user.name).await?; return Ok(store::User::builder() @@ -78,8 +64,7 @@ impl hannibal::Handler for LinuxUserManager { /// Check that a pre-existing account with the GitHub login may be adopted. /// /// Adopting an account whose IDs differ from a rejoining member's stored ones, or -/// whose IDs another user's tombstone reserves, would silently accept exactly the -/// ownership drift the stored IDs exist to prevent. +/// whose IDs another user's tombstone reserves, would create an ownership drift between the users. fn verify_adoption(existing: &nix::unistd::User, ids: &AccountIds) -> anyhow::Result<()> { match ids { AccountIds::Stored { uid, gid } => { @@ -125,13 +110,11 @@ fn verify_adoption(existing: &nix::unistd::User, ids: &AccountIds) -> anyhow::Re /// Number of candidates to probe before giving up the search for a free ID const FREE_ID_ATTEMPTS: u32 = 10_000; -/// Create an account with system-allocated IDs, re-creating it with explicitly chosen -/// free IDs when the allocation hands out an ID a tombstone reserves. +/// Create an account without re-using IDs reserved by tombstones. /// -/// shadow-utils allocates the highest ID in range plus one, so purging the user with -/// the highest UID frees exactly the next UID `useradd` hands out: without this check -/// the next member to join would routinely receive the most recently purged user's -/// UID and inherit ownership of their files outside the removed home directory. +/// If an account is created with a UID or GID that a departed user's tombstone reserves, the +/// new account may own files and directories that the departed user should still control. +/// Thus the account is deleted and the function searches for a free UID / GID pair. async fn add_account_with_fresh_ids( login: &str, reserved_uids: &collections::HashSet, @@ -147,8 +130,7 @@ async fn add_account_with_fresh_ids( gid = linux_user.gid.as_raw(), "Allocated IDs are reserved for a departed user, re-creating with free IDs" ); - // The account was created moments ago and has never been returned to a caller, so - // nothing can have used it yet; remove it together with its fresh home directory + // The new account has not escaped, so it is safe to recreate with explicit IDs. let o = process::Command::new("/usr/sbin/userdel") .arg("--remove") .arg(login) @@ -175,9 +157,7 @@ async fn add_account_with_fresh_ids( .await } -/// First ID at or above `start` that no tombstone reserves and no account or group -/// uses. The value serves as both UID and GID, mirroring the user-private group of a -/// regular `useradd`. +/// Find a free ID usable as both UID and private-group GID. fn free_id_from( start: u32, reserved_uids: &collections::HashSet, @@ -200,9 +180,7 @@ fn free_id_from( anyhow::bail!("No free UID/GID found in [{start}, {end})") } -/// Create a platform account with `useradd` and return it. Re-creating a previously -/// removed account with its stored UID and private-group GID keeps ownership of files -/// outside the home directory intact. +/// Create a platform account with `useradd` and return it. async fn add_account( login: &str, uid: Option, @@ -246,17 +224,12 @@ async fn add_account( }) } -/// Re-create the platform account of a stored user that is gone from the system, using -/// the stored name, UID and GID so file ownership and a home directory that survived -/// the account's removal stay intact. A changed GitHub login is applied afterwards by -/// the regular rename path. +/// Recreate a missing stored account with its original identity. async fn recreate_account( gh_user: &octocrab::models::Author, available_user: &store::User, ) -> anyhow::Result { - // The account may have been re-created by hand under a new UID. Adopting it would - // silently accept the ownership drift the stored UID exists to prevent, so refuse - // and leave the resolution to an operator. + // Refuse a manually recreated account with a different identity. let colliding = match nix::unistd::User::from_name(available_user.name())? { Some(existing) => Some(existing), None => nix::unistd::User::from_name(&gh_user.login)?, @@ -280,16 +253,13 @@ async fn recreate_account( .await } -/// Ensure the user-private group of a re-created account exists as `login` with the -/// stored GID, refusing when the GID meanwhile belongs to another group. Falling back -/// to a fresh GID would leave files with the old GID group-owned by whichever group is -/// allocated it next. +/// Recreate the private group, refusing conflicting names or GIDs. async fn ensure_private_group(login: &str, gid: nix::unistd::Gid) -> anyhow::Result<()> { if let Some(group) = nix::unistd::Group::from_gid(gid) .with_context(|| format!("Failed to check whether GID {gid} is in use"))? { if group.name == login { - // Already re-created, e.g. by an attempt that failed after groupadd + // A previous attempt may have failed after groupadd. return Ok(()); } anyhow::bail!( @@ -328,23 +298,18 @@ impl hannibal::Handler for LinuxUserManager { msg: UpdateUser, ) -> anyhow::Result { let (gh_user, available_user) = (&msg.gh_user, &msg.available_user); - // The GitHub login is accepted as an alternate name so a rename whose store - // update was lost is picked up instead of refused + // Accept the GitHub login after a rename whose store update was lost. let linux_user = match resolve_account(available_user, Some(&gh_user.login))? { AccountResolution::Matches(linux_user) => linux_user, - // The account is known in the store but gone from the system, e.g. deleted - // by hand + // Recreate an account removed outside octosync. AccountResolution::Missing => recreate_account(gh_user, available_user).await?, }; - // The account may carry an expiry that should no longer be in effect, e.g. - // one set by an operator: octosync owns the lifecycle of synced users + // Active org members must not retain an effective expiry. clear_departure_expiry(&linux_user.name).await?; if gh_user.login == linux_user.name { - // Rebuild the entry from the system account, healing a stored name that a - // lost store update after a rename left stale and backfilling the primary - // GID into entries migrated from a v1 store + // Heal stale names and backfill GIDs missing from v1 stores. return Ok(store::User::builder() .id(available_user.id()) .uid(linux_user.uid) @@ -407,46 +372,33 @@ impl hannibal::Handler for LinuxUserManager { return Ok(()); }; - // Expire first, so no new session (password or pubkey SSH) can start while - // the running ones are ended. The one step whose failure aborts the teardown: - // without the expiry the account stays reachable and the rest is cosmetic. + // Block new sessions before tearing down existing work. expire_account(&linux_user.name).await?; - // None of the steps below is a precondition of another, so they are all run - // before the first error is returned: a process stuck in uninterruptible I/O - // may not keep the departed member in their supplementary groups for the whole - // retention period. Returning an error is what makes the next sync retry. + // Run independent cleanup steps before returning the first error. - // Before the sweep, so no scheduled job can spawn processes into the teardown + // Remove schedulers before sweeping processes. let scheduled_jobs = remove_scheduled_jobs(&linux_user).await; - // logind first, then the sweep as the catch-all for processes outside logind's - // session scopes and as the sole mechanism on systems without logind + // Sweep anything outside logind, or everything on systems without it. end_logind_sessions(&linux_user).await; let sweep = kill_processes_for_user(&linux_user).await; - // octosync owns the supplementary groups of synced users, and a departed - // member keeps no access granted through them + // Remove access granted through supplementary groups. let groups = strip_supplementary_groups(&linux_user).await; scheduled_jobs.and(sweep).and(groups) } } -/// End the user's login sessions through logind, which logs them out cleanly: it +/// End the user's login sessions through logind, which logs them out cleanly. It /// closes the PAM sessions and tears down the session scopes and the user manager /// through the cgroup hierarchy instead of only signaling PIDs, so nothing escapes by -/// forking. Never fatal, the following process sweep is the catch-all. -/// -/// Reaching logind at all is what separates the two failure modes, so they are logged -/// differently: no system bus means no logind and hence no logind sessions on this -/// machine, while a logind that answers and then refuses leaves session scopes and a -/// lingering user manager behind that the sweep cannot clean up. +/// forking. async fn end_logind_sessions(user: &nix::unistd::User) { let connection = match tokio::time::timeout(LOGIND_TIMEOUT, zbus::Connection::system()).await { Ok(Ok(connection)) => connection, - // A container or a machine without systemd: the process sweep is the whole - // mechanism here, so this is an expected state rather than a failure + // Containers and non-systemd hosts rely on the process sweep. Ok(Err(e)) => { tracing::debug!("No system bus, ending sessions with the process sweep alone: {e}"); return; @@ -457,10 +409,7 @@ async fn end_logind_sessions(user: &nix::unistd::User) { } }; - // zbus applies no reply timeout of its own and the bus does not enforce one on the - // method calls it forwards, so a wedged logind never answers. The teardown runs - // inside the actor, where waiting for it would stall every following platform - // operation, so the conversation gets one deadline and the sweep takes over. + // Bound D-Bus calls so a stuck logind cannot stall the actor. let uid = user.uid.as_raw(); match tokio::time::timeout(LOGIND_TIMEOUT, terminate_logind_user(&connection, uid)).await { Ok(Ok(())) => {} @@ -471,13 +420,9 @@ async fn end_logind_sessions(user: &nix::unistd::User) { } } -/// Deadline for the logind conversation, see [`end_logind_sessions`] const LOGIND_TIMEOUT: time::Duration = time::Duration::from_secs(5); -/// Disable linger for the user and terminate their sessions over logind's D-Bus API. -/// -/// `TerminateUser` returns once the stop jobs are queued rather than once the -/// processes are gone, so the caller's sweep is what establishes that they exited. +/// Disable linger and ask logind to terminate the user's sessions. async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyhow::Result<()> { let manager = zbus::Proxy::new( connection, @@ -489,8 +434,8 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho .context("Failed to create the logind manager proxy")?; // Disabling linger keeps a `user@.service` from restarting user services after the - // termination, but it is the lesser half: a surviving login session matters more, - // so a failure here must not skip the termination below + // termination, but a surviving login session matters more, so a failure here must not skip the + // termination below if let Err(e) = manager .call::<_, _, ()>("SetUserLinger", &(uid, false, false)) .await @@ -498,9 +443,7 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho tracing::warn!("Failed to disable linger, terminating the sessions anyway: {e}"); } - // GetUser errors when the user neither has a session nor lingers, the usual state - // of a departed account whose teardown is retried by a later sync: nothing to - // terminate + // GetUser fails when there is no session or lingering user manager. if manager .call::<_, _, zbus::zvariant::OwnedObjectPath>("GetUser", &uid) .await @@ -514,24 +457,19 @@ async fn terminate_logind_user(connection: &zbus::Connection, uid: u32) -> anyho .context("Failed to terminate the user's sessions") } -/// Remove the departed user's scheduled work, which outlives their sessions: cron and -/// at run jobs without a login, so ending the sessions alone would leave the account -/// executing code after its departure. +/// Remove `cron` and `at` jobs that can outlive login sessions. /// -/// Expiring the account already blocks the PAM account check cron and at perform, so +/// Expiring the account already blocks the PAM account check `cron` and `at` perform, so /// this is the second half of that guarantee rather than the only one, and it also -/// covers a machine whose cron does not consult PAM. +/// covers a machine whose `cron` does not consult PAM. async fn remove_scheduled_jobs(user: &nix::unistd::User) -> anyhow::Result<()> { remove_crontab(&user.name).await?; remove_at_jobs(&user.name).await } -/// Drop the user's crontab with `crontab -r`, which removes the spool file and makes -/// cron pick up the change. +/// Remove the user's `crontab` /// -/// Listing first keeps this quiet in the common case of a user without a crontab: -/// `crontab -r` reports that as a failure, and telling it apart from a real one would -/// mean matching its message. +/// List the `crontab` first to avoid treating absence as an error. async fn remove_crontab(name: &str) -> anyhow::Result<()> { let Some(list) = optional_command("/usr/bin/crontab", &["-l", "-u", name]).await? else { return Ok(()); @@ -555,9 +493,10 @@ async fn remove_crontab(name: &str) -> anyhow::Result<()> { Ok(()) } -/// Remove the user's queued `at` jobs. Running as root, `atq` lists the jobs of every -/// user with the owner in the last field, which is the only way to select one user's -/// jobs for `atrm`. +/// Remove the user's queued `at` jobs from root's global listing. +/// +/// `atq` lists the jobs of every user with the owner in the last field, which is the only way to +/// select one user's jobs for `atrm`. async fn remove_at_jobs(name: &str) -> anyhow::Result<()> { let Some(queue) = optional_command("/usr/bin/atq", &[]).await? else { return Ok(()); @@ -599,9 +538,7 @@ async fn remove_at_jobs(name: &str) -> anyhow::Result<()> { Ok(()) } -/// Run a command that only exists when the service behind it is installed. `Ok(None)` -/// when the binary is missing, so a machine without cron or at has no scheduled work -/// to remove rather than a failing departure. +/// Treat a missing optional command as no work to do. async fn optional_command( program: &str, args: &[&str], @@ -609,16 +546,14 @@ async fn optional_command( match process::Command::new(program).args(args).output().await { Ok(output) => Ok(Some(output)), Err(e) if e.kind() == std::io::ErrorKind::NotFound => { - tracing::debug!("'{program}' is not installed, no scheduled work to remove with it"); + tracing::debug!("'{program}' is not installed"); Ok(None) } Err(e) => Err(e).with_context(|| format!("Failed to run '{program}'")), } } -/// Remove the user from all supplementary groups, so a departed member keeps no -/// access granted through them. The usermod is skipped when there is nothing to -/// remove. +/// Remove all supplementary groups, skipping `usermod` when none exist. async fn strip_supplementary_groups(user: &nix::unistd::User) -> anyhow::Result<()> { let current = current_supplementary_groups(user) .with_context(|| format!("Failed to read current groups of '{}'", user.name))?; @@ -643,19 +578,14 @@ impl hannibal::Handler for LinuxUserManager { return Ok(PurgeOutcome::NoAccount); }; - // The account-side eligibility: only an account whose own shadow expiry has - // been in effect for the retention period may be purged. Any reactivation - // clears the expiry, so a live account can never pass this gate. - if !account_expire_days(&linux_user.name)? - .is_some_and(|expire_days| expire_days <= msg.expired_before) - { + // Reactivation clears the shadow expiry, preventing a live account from being purged. + if !account_is_expired(&linux_user.name, msg.expired_before)? { return Ok(PurgeOutcome::NotExpired); } // The account has been expired for the whole retention period, so nothing is - // left to shut down cleanly into: kill hard without a grace period. userdel - // decides authoritatively whether the account is busy, so a failed sweep is - // only logged. + // left to shut down cleanly into. userdel decides whether the account is busy, + // so a failed sweep is only logged. if let Err(e) = force_kill_processes_for_user(&linux_user).await { tracing::warn!("Failed to sweep processes before userdel: {e:#}"); } @@ -690,8 +620,6 @@ impl hannibal::Handler for LinuxUserManager { msg: SyncSupplementaryGroups, ) -> anyhow::Result<()> { let user = &msg.user; - // octosync owns the supplementary groups of synced users, so acting on a - // stale UID would strip the groups of an unrelated account let linux_user = match resolve_account(user, None)? { AccountResolution::Matches(linux_user) => linux_user, AccountResolution::Missing => anyhow::bail!( @@ -770,15 +698,11 @@ impl hannibal::Handler for LinuxUserManager { _ctx: &mut hannibal::Context, msg: UpdateAuthorizedKeys, ) -> anyhow::Result<()> { - self.authorized_keys - .update_authorized_keys(&msg.user, &msg.keys) - .await + crate::authorized_keys::update_authorized_keys(&msg.user, &msg.keys).await } } -/// Result of [`resolve_account`]: the platform account of a stored user, or proof -/// that none exists. A store that disagrees with the system is an error, never a -/// variant, so no caller can act on a possibly wrong account. +/// A matching platform account or proof that neither its name nor UID exists. enum AccountResolution { /// No account with the stored name exists and the stored UID is unused Missing, @@ -786,14 +710,16 @@ enum AccountResolution { Matches(nix::unistd::User), } -/// Resolve the platform account of a stored user, the one cross-check every mutating -/// handler routes through: shadow-utils commands operate on the account name, so the -/// name is authoritative and the stored UID must agree with it. Acting on a name -/// whose UID moved on, or a UID whose name moved on, would mutate an unrelated -/// account. +/// Resolve the platform account of a stored user +/// +/// This is a crucial check used by every mutating handler, because `shadow-utils` commands operate +/// on the account name, so the name is authoritative and the stored UID must agree with it. +/// +/// Acting on a name whose UID changed on the system, or a UID whose name moved on the system, would +/// mutate an unrelated account. /// -/// `renamed_to` accepts one alternate name for the account holding the stored UID: -/// a usermod rename whose store update was lost leaves the account only findable by +/// `renamed_to` accepts one alternate name for the account holding the stored UID: a usermod +/// rename whose store update was lost leaves the account only findable by /// UID, and the caller knows which new name proves it is still the managed account. fn resolve_account( user: &store::User, @@ -804,8 +730,7 @@ fn resolve_account( if renamed_to.is_some_and(|name| name == other_user.name) { return Ok(AccountResolution::Matches(other_user)); } - // Whether this is the managed account under an unexpected name or an - // unrelated account that reuses the UID can not be decided here + // An unexpected name cannot safely be distinguished from UID reuse. anyhow::bail!( "No user named '{}' in the system, but UID {} belongs to '{}', refusing to act \ on the account", @@ -828,8 +753,7 @@ fn resolve_account( Ok(AccountResolution::Matches(linux_user)) } -/// The names of the supplementary groups the user is currently a member of, -/// excluding the primary group +/// Supplementary group names, excluding the primary group. fn current_supplementary_groups( user: &nix::unistd::User, ) -> anyhow::Result> { @@ -843,8 +767,7 @@ fn current_supplementary_groups( if gid == user.gid { continue; } - // A group deleted since getgrouplist has no name to compare or pass to - // usermod, skip it + // Skip groups deleted after getgrouplist. if let Some(group) = nix::unistd::Group::from_gid(gid) .with_context(|| format!("Failed to resolve group with GID {gid}"))? { @@ -882,9 +805,8 @@ async fn sync_user_supplementary_groups_by_name( } } -const SECONDS_PER_DAY: u64 = 86_400; +const SECONDS_PER_DAY: u64 = 24 * 60 * 60; -/// Today as the day number since the epoch, the unit of the shadow expiry field fn days_since_epoch() -> anyhow::Result { let days = time::SystemTime::now() .duration_since(time::UNIX_EPOCH) @@ -894,23 +816,15 @@ fn days_since_epoch() -> anyhow::Result { Ok(days as i64) } -/// Expire the account so no new session (password or pubkey SSH) can start. Sessions -/// already running are handled by the session teardown and the process sweep. -/// -/// The expiry is set to the day before the departure: the shadow field has day -/// granularity and some login paths treat an account as expired only strictly after -/// its date, so the departure day itself could keep logins open until midnight. +/// Expire the account as of yesterday; existing sessions are handled separately. async fn expire_account(name: &str) -> anyhow::Result<()> { let expire_days = days_since_epoch()? - 1; - // A teardown that is retried meets an account that is already expired; skip the - // usermod when the expiry is in effect - if account_expire_days(name)?.is_some_and(|days| days <= expire_days) { + if account_is_expired(name, expire_days)? { return Ok(()); } let output = process::Command::new("/usr/sbin/usermod") .arg("--expiredate") - // shadow parses a plain number as days since the epoch, which sidesteps the - // timezone interpretation of a YYYY-MM-DD date. chage displays it as a date. + // Use epoch days to avoid YYYY-MM-DD timezone interpretation. .arg(expire_days.to_string()) .arg(name) .output() @@ -928,18 +842,9 @@ async fn expire_account(name: &str) -> anyhow::Result<()> { Ok(()) } -/// Lift an expiry that is already in effect, reactivating the expired account of a -/// departed member who rejoined. An expiry in the future is an operator's scheduled -/// offboarding and stays. -/// -/// octosync owns the account lifecycle of synced users, so a past expiry set by an -/// operator does not survive a sync either. Suspending a member is done by removing -/// them from the org. +/// Clear an effective expiry while preserving a future scheduled expiry. async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { - let Some(expire_days) = account_expire_days(name)? else { - return Ok(()); - }; - if expire_days > days_since_epoch()? { + if !account_is_expired(name, days_since_epoch()?)? { return Ok(()); } @@ -958,12 +863,16 @@ async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { String::from_utf8_lossy(&output.stderr) ); } - tracing::info!("Cleared the expiry of the re-activated account"); + tracing::info!("Cleared the expiry of the reactivated account"); Ok(()) } -/// The account's expiry as days since the epoch, `None` when no expiry is set, read -/// through the same NSS lookup the passwd queries of this module use. +/// Whether the shadow expiry is on or before `as_of_days`. +fn account_is_expired(name: &str, as_of_days: i64) -> anyhow::Result { + Ok(account_expire_days(name)?.is_some_and(|expire_days| expire_days <= as_of_days)) +} + +/// Read the shadow expiry as days since the epoch through libc's `getspnam`. fn account_expire_days(name: &str) -> anyhow::Result> { let c_name = std::ffi::CString::new(name).context("User name contains an interior NUL byte")?; // SAFETY: getspnam returns a pointer into static storage, which is only unsound @@ -971,7 +880,6 @@ fn account_expire_days(name: &str) -> anyhow::Result> { // manager actor, which processes one message at a time. let entry = unsafe { libc::getspnam(c_name.as_ptr()) }; if entry.is_null() { - // No shadow entry means no expiry to consider return Ok(None); } // SAFETY: checked to be non-null above, and the field is copied out before any @@ -981,20 +889,14 @@ fn account_expire_days(name: &str) -> anyhow::Result> { Ok((expire_days >= 0).then_some(expire_days)) } -/// Grace period a SIGTERM'd process gets to exit before it is killed const KILL_GRACE_PERIOD: time::Duration = time::Duration::from_secs(3); -/// Poll interval while waiting for terminated processes to exit const KILL_POLL_INTERVAL: time::Duration = time::Duration::from_millis(200); -/// Time SIGKILL'd processes get to disappear from the process table, so a following -/// userdel cannot race a process that is still being torn down const SIGKILL_WAIT: time::Duration = time::Duration::from_secs(1); -/// Stop all processes of the user: SIGTERM first so they can shut down cleanly, then -/// SIGKILL whatever is still running after [`KILL_GRACE_PERIOD`]. Errors when -/// processes survive the SIGKILL, e.g. stuck in uninterruptible I/O. +/// Stop all processes for a user with SIGTERM, so they can shutdown cleanly. +/// After [`KILL_GRACE_PERIOD`], `SIGKILL` is sent to the remaining processes. /// -/// Runs inside the actor, so the grace period stalls other platform operations. It is -/// only paid when the user actually has running processes. +/// Errors are returned if any processes remain after [`SIGKILL_WAIT`]. #[tracing::instrument(name = "kill_processes", skip(user), fields(user = %user.name))] async fn kill_processes_for_user(user: &nix::unistd::User) -> anyhow::Result<()> { let uid = user.uid.as_raw(); @@ -1018,7 +920,7 @@ async fn kill_processes_for_user(user: &nix::unistd::User) -> anyhow::Result<()> ensure_processes_are_gone(uid, &user.name).await } -/// SIGKILL all processes of the user without a grace period +/// Kill all user processes without a grace period. #[tracing::instrument(name = "force_kill_processes", skip(user), fields(user = %user.name))] async fn force_kill_processes_for_user(user: &nix::unistd::User) -> anyhow::Result<()> { let uid = user.uid.as_raw(); @@ -1030,8 +932,7 @@ async fn force_kill_processes_for_user(user: &nix::unistd::User) -> anyhow::Resu ensure_processes_are_gone(uid, &user.name).await } -/// Confirm that the SIGKILL'd processes are gone from the process table within -/// [`SIGKILL_WAIT`] +/// Confirm killed processes leave the process table. async fn ensure_processes_are_gone(uid: u32, name: &str) -> anyhow::Result<()> { let survivors = wait_for_processes_to_exit(uid, SIGKILL_WAIT).await?; if survivors.is_empty() { @@ -1043,8 +944,7 @@ async fn ensure_processes_are_gone(uid: u32, name: &str) -> anyhow::Result<()> { ) } -/// Poll until no process of `uid` is left or `timeout` elapses, returning the -/// processes still running +/// Wait for a UID's processes to exit, returning any survivors. async fn wait_for_processes_to_exit( uid: u32, timeout: time::Duration, @@ -1059,13 +959,11 @@ async fn wait_for_processes_to_exit( } } -/// PIDs of all live processes whose real UID is `uid`. Zombies are excluded: no signal -/// removes them and only their parent reaping them does, so counting them would stall -/// every wait for the full timeout. +/// Live non-zombie processes whose real UID matches `uid`. async fn processes_of_uid(uid: u32) -> anyhow::Result> { tokio::task::spawn_blocking(move || -> anyhow::Result> { let procs = procfs::process::all_processes().context("Failed to list processes")?; - // Processes that exit while being inspected are skipped + // Ignore processes that exit during inspection. Ok(procs .flatten() .filter(|proc| { @@ -1079,7 +977,7 @@ async fn processes_of_uid(uid: u32) -> anyhow::Result> { .context("Process listing task failed")? } -/// Send `signal` to all `pids`. A process that is already gone is not an error. +/// Signal every PID, ignoring processes that already exited. fn signal_processes(pids: &[nix::unistd::Pid], signal: nix::sys::signal::Signal) { for &pid in pids { let _ = nix::sys::signal::kill(pid, signal); diff --git a/src/user_manager/messages.rs b/src/user_manager/messages.rs index ad54544..fc5c5c6 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -1,7 +1,4 @@ -//! The messages of the platform user manager contract. -//! -//! A platform backend is an actor handling the full message set. The messages carry -//! owned data so they can cross the actor boundary. +//! Owned messages forming the platform user manager contract. use crate::{public_keys, store}; use std::collections; @@ -16,88 +13,64 @@ pub struct CreateUser { /// UID and GID of the account [`CreateUser`] creates. #[derive(Debug, Clone, PartialEq, Eq)] pub enum AccountIds { - /// The stored IDs of a rejoining member whose account is gone from the system - /// (purged, or removed by hand): the account is created with exactly this UID and - /// its private group with exactly this GID, so file ownership survives the removal - /// and re-create cycle. When either ID is taken, creation fails instead of falling - /// back to a fresh one. A tombstone migrated from v1 carries no GID and leaves the - /// private group to `useradd`. + /// Reuse a rejoining member's IDs. Fail if either is unavailable. Stored { uid: nix::unistd::Uid, gid: Option, }, - /// System-allocated IDs for a brand-new member, avoiding the IDs reserved by - /// tombstones: shadow-utils allocates the highest ID in range plus one, so - /// purging the user with the highest UID frees exactly the next UID to be - /// allocated. + /// Allocate fresh IDs without spending those reserved by tombstones. Fresh { reserved_uids: collections::HashSet, reserved_gids: collections::HashSet, }, } -/// Renames the platform user of `available_user` (login and home directory) to the -/// GitHub login of `gh_user`, re-creating the account with the stored name, UID and -/// GID first when it no longer exists on the system. Refuses when the stored UID or -/// name belongs to a different account. +/// Reconcile a stored platform account with the current GitHub user. +/// +/// This re-creates the account with the stored name, UID and GID first when the user no longer +/// exists on the system. Refuses when the stored UID or name belongs to a different account. #[hannibal::message(response = anyhow::Result)] pub struct UpdateUser { pub gh_user: octocrab::models::Author, pub available_user: store::User, } -/// Expires the platform account of a departed user, replacing its deletion: verifies -/// that the stored user still matches the platform account, expires it so no new -/// session (password or pubkey SSH) can start, removes its scheduled work, ends the -/// running sessions and strips the supplementary groups. The account, its home -/// directory and its primary group stay on the machine as the durable departure -/// record, so the departure is a reversible lockout that a rejoin heals, until the -/// purge removes the account after the retention period. +/// Reversibly expire a departed account and remove its active access. /// -/// Idempotent, so an expiry interrupted at any point converges when a later sync -/// retries it. The sync only sends it for a tombstone whose teardown has not completed -/// yet, so the retry is driven by the store rather than by re-sending it every run. +/// Verifies that the stored user matches the platform account and expires the account, which +/// prevents logins and SSH key access. The account remains on the system and can be restored later. #[hannibal::message(response = anyhow::Result<()>)] pub struct ExpireAccount { pub user: store::User, } -/// Permanently removes the expired platform account of a departed user with -/// `userdel --remove`, deleting the home directory without an archive: the one -/// deliberately irreversible operation left now that expiry replaced deletion. -/// -/// The account-side half of the purge eligibility is verified here: the shadow expiry -/// must be set and at most `expired_before`. It is the evidence on the machine itself -/// that survives store damage and is cleared on any reactivation, so a wrongly -/// resurrected tombstone can never purge a live account. +/// Permanently remove a departed user's account and home directory once its shadow +/// expiry meets the cutoff. #[hannibal::message(response = anyhow::Result)] pub struct PurgeAccount { pub user: store::User, - /// Latest shadow expiry, in days since the epoch, an account may have to be purged + /// Latest shadow-expiry day an account may have to be purged. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] pub expired_before: i64, } -/// Outcome of [`PurgeAccount`]. Only the Linux backend can verify the account-side -/// eligibility, the mock previews every purge as performed. +/// Result of checking and applying a [`PurgeAccount`] operation. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum PurgeOutcome { - /// The account and its home directory were removed + /// The account and home directory were removed. Purged, - /// No account for the user exists on the system, nothing was done + /// No matching account exists. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] NoAccount, - /// The account's shadow expiry is missing or newer than `expired_before`: the - /// account-side clock does not agree with the store's, nothing was done + /// The account expiry does not meet the cutoff. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] NotExpired, } -/// Synchronizes the supplementary groups of a user. +/// Replace a user's supplementary groups, excluding their primary group. /// -/// octosync owns the supplementary groups of synced users: the user's memberships are -/// replaced with `groups`, keeping only the primary group. Groups assigned through -/// other channels are removed. +/// All supplementary groups of synced users are managed by octosync. Any group not in the given +/// list is removed from the user. #[hannibal::message(response = anyhow::Result<()>)] pub struct SyncSupplementaryGroups { pub user: store::User, @@ -110,10 +83,8 @@ pub struct EnsureGroupsExist { pub groups: Vec, } -/// Replaces the octosync-managed key block in the user's authorized_keys file with the -/// given keys, so a key revoked on GitHub is removed on the next sync. Lines outside -/// the managed block are never touched, so keys installed through other channels stay -/// intact. +/// Replace the octosync-managed authorized_keys block with the current public keys of the given +/// user. #[hannibal::message(response = anyhow::Result<()>)] pub struct UpdateAuthorizedKeys { pub user: store::User, From 8c450ca3f6ffa95b4d88abba17c1ee8456548cba Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Fri, 14 Aug 2026 20:54:14 +0200 Subject: [PATCH 12/12] refactor: Carry purge cutoffs as UTC timestamps --- src/octosync.rs | 3 +-- src/user_manager.rs | 2 +- src/user_manager/backends/linux.rs | 29 ++++++++++++----------------- src/user_manager/messages.rs | 4 ++-- 4 files changed, 16 insertions(+), 22 deletions(-) diff --git a/src/octosync.rs b/src/octosync.rs index 1c4e5d1..65ff229 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -263,7 +263,6 @@ impl Octosync { ) -> anyhow::Result<()> { let now = chrono::Utc::now(); let cutoff = now - chrono::Duration::days(purge_after_days.into()); - let expired_before = cutoff.timestamp() / (24 * 60 * 60); let candidates: Vec = store .departed() @@ -275,7 +274,7 @@ impl Octosync { .collect(); for user in candidates { - match self.user_manager.purge_user(&user, expired_before).await { + match self.user_manager.purge_user(&user, cutoff).await { Ok(user_manager::PurgeOutcome::Purged) => { tracing::info!( user = %user.name(), diff --git a/src/user_manager.rs b/src/user_manager.rs index 50090d9..c128184 100644 --- a/src/user_manager.rs +++ b/src/user_manager.rs @@ -116,7 +116,7 @@ impl UserManager { pub async fn purge_user( &self, user: &store::User, - expired_before: i64, + expired_before: chrono::DateTime, ) -> anyhow::Result { self.purge_account .call(PurgeAccount { diff --git a/src/user_manager/backends/linux.rs b/src/user_manager/backends/linux.rs index 1f39766..50d54e3 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -805,27 +805,18 @@ async fn sync_user_supplementary_groups_by_name( } } -const SECONDS_PER_DAY: u64 = 24 * 60 * 60; - -fn days_since_epoch() -> anyhow::Result { - let days = time::SystemTime::now() - .duration_since(time::UNIX_EPOCH) - .context("System time is before the epoch")? - .as_secs() - / SECONDS_PER_DAY; - Ok(days as i64) -} +const SECONDS_PER_DAY: i64 = 24 * 60 * 60; /// Expire the account as of yesterday; existing sessions are handled separately. async fn expire_account(name: &str) -> anyhow::Result<()> { - let expire_days = days_since_epoch()? - 1; - if account_is_expired(name, expire_days)? { + let expire_at = chrono::Utc::now() - chrono::Duration::days(1); + if account_is_expired(name, expire_at)? { return Ok(()); } let output = process::Command::new("/usr/sbin/usermod") .arg("--expiredate") // Use epoch days to avoid YYYY-MM-DD timezone interpretation. - .arg(expire_days.to_string()) + .arg(shadow_days(expire_at).to_string()) .arg(name) .output() .await @@ -844,7 +835,7 @@ async fn expire_account(name: &str) -> anyhow::Result<()> { /// Clear an effective expiry while preserving a future scheduled expiry. async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { - if !account_is_expired(name, days_since_epoch()?)? { + if !account_is_expired(name, chrono::Utc::now())? { return Ok(()); } @@ -867,9 +858,13 @@ async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { Ok(()) } -/// Whether the shadow expiry is on or before `as_of_days`. -fn account_is_expired(name: &str, as_of_days: i64) -> anyhow::Result { - Ok(account_expire_days(name)?.is_some_and(|expire_days| expire_days <= as_of_days)) +/// Whether the shadow expiry is on or before `as_of`. +fn account_is_expired(name: &str, as_of: chrono::DateTime) -> anyhow::Result { + Ok(account_expire_days(name)?.is_some_and(|expire_days| expire_days <= shadow_days(as_of))) +} + +fn shadow_days(timestamp: chrono::DateTime) -> i64 { + timestamp.timestamp().div_euclid(SECONDS_PER_DAY) } /// Read the shadow expiry as days since the epoch through libc's `getspnam`. diff --git a/src/user_manager/messages.rs b/src/user_manager/messages.rs index fc5c5c6..384e447 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -49,9 +49,9 @@ pub struct ExpireAccount { #[hannibal::message(response = anyhow::Result)] pub struct PurgeAccount { pub user: store::User, - /// Latest shadow-expiry day an account may have to be purged. + /// Purge the account if it has been expired since before this timestamp. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] - pub expired_before: i64, + pub expired_before: chrono::DateTime, } /// Result of checking and applying a [`PurgeAccount`] operation.