diff --git a/CHANGELOG.md b/CHANGELOG.md index 5a25b35..48a230b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,60 +2,36 @@ ## 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 - processing failed stay in the store unchanged. +- 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)) +- 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)) +- 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)) -- A failed authorized keys fetch no longer fails user processing which could lead to the deletion 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 delete every stored user - ([#17](https://github.com/jonasehrlich/octosync/pull/17)) -- Implement mapping GitHub teams to Linux groups with `--group :` +- GitHub teams can now be mapped 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 - ([#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)) -- 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 - 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 - 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)) +- `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)) ## v0.3.0 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/README.md b/README.md index 094166f..8d2296a 100644 --- a/README.md +++ b/README.md @@ -49,16 +49,30 @@ 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, 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. -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. +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 +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 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/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 d7d8321..cebed70 100644 --- a/src/main.rs +++ b/src/main.rs @@ -3,10 +3,8 @@ 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 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] @@ -178,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) => { @@ -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 c64ed6b..65ff229 100644 --- a/src/octosync.rs +++ b/src/octosync.rs @@ -1,28 +1,16 @@ use crate::{ - GlobalArgs, InstallationClientArgs, 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 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. +/// 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()) @@ -37,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) - .home_archive_dir(data_dir.join("home-archive")) + .dry_run(dry_run) .build(); - Ok(Self { + Self { data_dir: data_dir.to_path_buf(), + dry_run, user_manager, - }) + } } #[tracing::instrument( @@ -95,8 +82,11 @@ 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?, - None => self.create_user(gh_user, store.archived()).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?, }; self.user_manager @@ -104,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() { @@ -126,48 +115,73 @@ 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 and is retried on the next sync. /// - /// 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. + /// 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, - archived: &store::ArchivedMap, + store: &store::UserStore, ) -> anyhow::Result { - let ids = match archived.get(&gh_user.id) { - Some(tombstone) => { + // 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() + .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 + ); + } + + // Departed members reuse their account, purged members reuse only its 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() - .filter_map(store::ArchivedUser::gid) + .map(store::DepartedUser::uid) + .chain(store.purged().values().map(store::PurgedUser::uid)) + .collect(), + reserved_gids: store + .departed() + .values() + .filter_map(store::DepartedUser::gid) + .chain(store.purged().values().filter_map(store::PurgedUser::gid)) .collect(), }, }; 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), @@ -177,7 +191,7 @@ impl Octosync { let octocrab = org_client(&args.octocrab).await?; let (org_members, old_store, assignments) = 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), groups::GroupAssignments::resolve(&octocrab, &args.octocrab.org, &args.group) )?; tracing::info!("Successfully retrieved {} members", org_members.len()); @@ -185,7 +199,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() ); @@ -206,7 +220,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 { @@ -217,62 +231,113 @@ 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. - /// - /// 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( + /// 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, + 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()); + + 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, cutoff).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); + } + // 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() + ), + 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 + } + + /// 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 + async fn depart_and_expire( &self, 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); + tracing::info!(user = %user.name(), "Recording departure, expiring the account"); + 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); + let pending: Vec = store + .departed() + .values() + .filter(|tombstone| tombstone.expired_at().is_none()) + .map(store::User::from) + .collect(); + + // 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()), + Err(e) => { + tracing::error!("Failed to expire the account of '{}': {:?}", user.name(), e) + } } } 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 are deleted. + /// Process members concurrently and collect successful results into a new store. async fn process_members( &self, octocrab: &octocrab::Octocrab, @@ -280,10 +345,9 @@ impl Octosync { store: &store::UserStore, assignments: &groups::GroupAssignments, ) -> anyhow::Result { - 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(); + let mut new_store = store::UserStore::new(&self.data_dir, self.dry_run).await?; + *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); @@ -301,66 +365,49 @@ 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) } - /// Delete the platform accounts of the archived users concurrently, returning the - /// ID and home archive path of every successful deletion. - /// - /// 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() - }) - // 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() - .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?; + 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 // 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(()) } + + /// Explicitly purge eligible departed users, excluding current org members. + #[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, self.dry_run), + )?; + 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. -/// -/// 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. +/// 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, @@ -372,17 +419,12 @@ 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. -/// -/// 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( +/// Refuse a sync that would expire every previously stored user. +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 +472,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,15 +516,44 @@ 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")), + 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(); @@ -493,7 +562,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( @@ -514,7 +585,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")); @@ -540,7 +613,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( @@ -565,7 +640,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( @@ -586,7 +663,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( @@ -611,8 +690,10 @@ 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(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + 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 .process_members( @@ -632,7 +713,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 @@ -644,8 +725,10 @@ 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(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + let mut old_store = store::UserStore::new(_data_dir.path(), false) + .await + .unwrap(); + old_store.depart_user(user(1, "alice"), chrono::Utc::now()); octosync .process_members( @@ -666,6 +749,25 @@ 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(), false) + .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] @@ -675,8 +777,10 @@ 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(); - old_store.archive_user(user(1, "alice"), chrono::Utc::now()); + 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 .process_members( @@ -691,85 +795,82 @@ mod tests { assert!(new_store.data().is_empty()); assert!( new_store - .archived() + .departed() .contains_key(&octocrab::models::UserId(1)) ); } - /// 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(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); octosync - .archive_and_delete(&mut store, vec![user(1, "alice")]) + .depart_and_expire(&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 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_eq!(tombstone.home_archive(), None); + // No completion timestamp, so the next sync retries the teardown + assert!(tombstone.expired_at().is_none()); assert!(saved.data().is_empty()); } - /// A successful deletion records the home archive path on the tombstone. + /// 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 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(); - + async fn depart_and_expire_retries_an_unfinished_tombstone() { 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(())); + 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 unfinished tombstone alone drives the teardown octosync - .archive_and_delete(&mut store, vec![user(1, "alice")]) + .depart_and_expire(&mut store, vec![]) .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()); + assert_eq!(*expired_users.lock().unwrap(), ["alice"]); + assert!( + store.departed()[&octocrab::models::UserId(1)] + .expired_at() + .is_some() + ); } - /// A tombstone whose account is already gone reports no archive; the path - /// recorded by the sync that archived the home directory must survive. + /// 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 archive_and_delete_keeps_a_recorded_archive_path() { - let mut actor = TestingUserManager::default(); - actor - .prepare_user_deletion - .push_back(Ok(DeletionPreparation::NothingToDo)); + 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()).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()); + 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 - .archive_and_delete(&mut store, vec![]) + .depart_and_expire(&mut store, vec![]) .await .unwrap(); - assert_eq!( - store.archived()[&octocrab::models::UserId(1)].home_archive(), - Some(archive.as_path()) - ); + assert!(expired_users.lock().unwrap().is_empty()); } /// The `delete` command tombstones every user instead of removing the store @@ -777,11 +878,9 @@ mod tests { #[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(); + let mut store = store::UserStore::new(data_dir.path(), false).await.unwrap(); store .data_mut() .insert(octocrab::models::UserId(1), user(1, "alice")); @@ -789,63 +888,199 @@ 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.archived()[&octocrab::models::UserId(1)].name(), + saved.departed()[&octocrab::models::UserId(1)].name(), "alice" ); } } + 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(), false).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(), false) + .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(), false).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(), false).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(), false).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(), false).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(), false).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"); @@ -853,26 +1088,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] @@ -882,50 +1117,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 9feb3b2..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,43 +79,47 @@ 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 } } -/// 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 + /// 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")] - home_archive: Option, + expired_at: Option>, } #[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 +134,84 @@ 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 time the member departed + pub fn departed_at(&self) -> chrono::DateTime { + self.departed_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 account teardown completed, `None` while it still has to run + pub fn expired_at(&self) -> Option> { + self.expired_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, } } } +/// 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 + 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,42 +223,54 @@ 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 #[derive(Serialize)] struct StoreDataRef<'a> { version: u64, users: &'a UserMap, - archived: &'a ArchivedMap, + departed: &'a DepartedMap, + purged: &'a PurgedMap, } #[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 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 { - /// Create a new store instance with the given directory, without loading any data - pub async fn new(dir: &path::Path) -> anyhow::Result { + /// 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 { dir: dir.to_path_buf(), + dry_run, users: UserMap::new(), - archived: ArchivedMap::new(), + departed: DepartedMap::new(), + purged: PurgedMap::new(), }) } - /// 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) -> 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) } @@ -230,50 +283,81 @@ 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, + expired_at: None, }, ); } - /// 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); + /// Mark a departed user's account teardown as complete. + 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); } } - /// Drop the tombstones of users that are active again, keeping every user either - /// active or archived but never both + /// Move a departed tombstone to the purged map. + 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 tombstones for active users. 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 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 an empty archived map. + /// 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(); @@ -299,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!( @@ -307,12 +390,12 @@ 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, - archived: ArchivedMap::new(), + departed: DepartedMap::new(), + purged: PurgedMap::new(), } } Some(version) if version.as_u64() == Some(STORE_VERSION) => { @@ -320,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() @@ -328,7 +410,8 @@ impl UserStore { }; self.users = data.users; - self.archived = data.archived; + self.departed = data.departed; + self.purged = data.purged; Ok(()) } @@ -348,15 +431,20 @@ impl UserStore { Ok(()) } + /// 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"); + return Ok(()); + } 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 - // 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 _; @@ -385,39 +473,38 @@ 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(), + expired_at: Some(departed_at()), } } - mod user { - use super::*; - - #[test] - fn home_dir() { - let user = user(); - assert_eq!(user.home_dir(), path::PathBuf::from("/home/testuser")); + 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), } + } - #[test] - fn ssh_dir() { - let user = user(); - assert_eq!(user.ssh_dir(), path::PathBuf::from("/home/testuser/.ssh")); - } + mod user { + use super::*; #[test] fn serialization() { @@ -485,46 +572,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); } } @@ -551,25 +641,26 @@ 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"); 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] 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"); 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()); } @@ -579,7 +670,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"); @@ -588,7 +679,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)) @@ -602,7 +693,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)) @@ -611,7 +702,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] @@ -619,90 +711,130 @@ 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")); } #[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()) + let mut store = UserStore::from_dir(temp_dir.path(), false) .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"); - 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); - 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(); + let store = UserStore::from_dir(temp_dir.path(), false).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 mut store = UserStore::new(temp_dir.path(), false).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 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 user = user(); - store.archive_user(user.clone(), deleted_at()); - - let archive = path::PathBuf::from("/data/home-archive/testuser.tar.gz"); - store.record_home_archive(&user.id, archive.clone()); - - assert_eq!(store.archived[&user.id].home_archive, Some(archive)); + 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(); + + 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(); - 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 = archived_user(); + let departed = departed_user(); store - .archived - .insert(rejoined.id, ArchivedUser::from_test_user(&rejoined)); - store.archived.insert(departed.id, departed.clone()); + .departed + .insert(rejoined.id, DepartedUser::from_test_user(&rejoined)); + store.departed.insert(departed.id, departed.clone()); store.users.insert(rejoined.id, rejoined.clone()); store.prune_rejoined(); - assert!(!store.archived.contains_key(&rejoined.id)); - assert_eq!(store.archived[&departed.id], departed); + 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_purged_tombstones_of_active_users() { + let temp_dir = tempfile::TempDir::new().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()); + store.users.insert(rejoined.id, rejoined); + + store.prune_rejoined(); + + 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] @@ -710,7 +842,7 @@ mod tests { 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()); } @@ -718,7 +850,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"); @@ -730,7 +862,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"); @@ -746,7 +878,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"); @@ -760,7 +892,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 +900,20 @@ mod tests { name: user.name.clone(), uid: user.uid, gid: user.gid, - deleted_at: deleted_at(), - home_archive: None, + departed_at: departed_at(), + expired_at: None, + } + } + } + + 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, } } } diff --git a/src/user_manager.rs b/src/user_manager.rs index 527a3c7..c128184 100644 --- a/src/user_manager.rs +++ b/src/user_manager.rs @@ -1,41 +1,29 @@ -//! 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. -//! -//! 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. +//! type-erased handle to one spawned backend. 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. -/// -/// 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 { - /// 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,36 +35,32 @@ 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) } #[cfg(not(target_os = "linux"))] { - Self::from_actor(backends::mock::MockUserManager::new(1000), home_archive_dir) + Self::from_actor(backends::mock::MockUserManager::new(1000)) } } } 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,20 +68,16 @@ 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(), } } - /// 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, @@ -112,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, @@ -127,53 +106,27 @@ 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() }) - .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) + pub async fn expire_user(&self, user: &store::User) -> anyhow::Result<()> { + self.expire_account + .call(ExpireAccount { user: user.clone() }) .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 { + pub async fn purge_user( + &self, + user: &store::User, + expired_before: chrono::DateTime, + ) -> 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. pub async fn sync_supplementary_groups( &self, user: &store::User, @@ -188,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 { @@ -198,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, @@ -218,11 +169,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 +214,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 +238,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 +259,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 +303,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 +340,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 +351,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..50d54e3 100644 --- a/src/user_manager/backends/linux.rs +++ b/src/user_manager/backends/linux.rs @@ -1,30 +1,16 @@ -//! 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::{ - 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}; 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 {} @@ -48,9 +34,8 @@ 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?; + // Reactivate an adopted account for a rejoining member. + clear_departure_expiry(&existing_user.name).await?; return Ok(store::User::builder() .id(user.id) @@ -79,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 } => { @@ -126,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 deleting 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. +/// 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, @@ -148,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) @@ -176,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, @@ -201,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 -/// deleted 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, @@ -247,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 deletion 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)?, @@ -281,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!( @@ -329,33 +298,18 @@ 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 account is known in the store but gone from the system, e.g. deleted - // by hand - None => recreate_account(gh_user, available_user).await?, + // 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, + // Recreate an account removed outside octosync. + 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 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?; + // 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) @@ -402,83 +356,255 @@ 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 { - 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); + msg: ExpireAccount, + ) -> anyhow::Result<()> { + 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(()); }; - // 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. + // Block new sessions before tearing down existing work. 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 - kill_processes_for_user(&linux_user).await?; + // Run independent cleanup steps before returning the first error. + + // Remove schedulers before sweeping processes. + let scheduled_jobs = remove_scheduled_jobs(&linux_user).await; + + // 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; + + // 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 +/// 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. +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, + // 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; + } + Err(_) => { + tracing::error!("System bus did not answer in time, leaving the sessions to the sweep"); + return; + } + }; + + // 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(())) => {} + 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") + } + } +} + +const LOGIND_TIMEOUT: time::Duration = time::Duration::from_secs(5); + +/// 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, + "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 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 fails when there is no session or lingering user manager. + 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 `cron` and `at` jobs that can outlive login sessions. +/// +/// 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 +} + +/// Remove the user's `crontab` +/// +/// 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(()); + }; + 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 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(()); + }; + if !queue.status.success() { + anyhow::bail!( + "Failed to list the at jobs of '{name}': {}", + String::from_utf8_lossy(&queue.stderr) + ); + } - Ok(DeletionPreparation::Prepared { - home_dir: linux_user.dir, + 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(()) +} + +/// Treat a missing optional command as no work to do. +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"); + Ok(None) + } + Err(e) => Err(e).with_context(|| format!("Failed to run '{program}'")), + } +} + +/// 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))?; + if current.is_empty() { + return Ok(()); } + sync_user_supplementary_groups_by_name(&user.name, &[]).await } -impl hannibal::Handler for LinuxUserManager { +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 - let Some(linux_user) = resolve_account_checked(&msg.user)? else { - tracing::warn!("User disappeared after deletion was prepared, nothing to do"); - return Ok(()); + msg: PurgeAccount, + ) -> anyhow::Result { + let AccountResolution::Matches(linux_user) = resolve_account(&msg.user, None)? else { + 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. + // 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. 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:#}"); } - 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) } } @@ -494,14 +620,13 @@ 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() - ) - })?; + 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")? @@ -573,46 +698,62 @@ 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 } } -/// 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. -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 +/// 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, + /// The account whose name and UID agree with the store + Matches(nix::unistd::User), +} + +/// 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 +/// 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 orphaning the account or deleting a wrong one. 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)); + } + // An unexpected name cannot safely be distinguished from UID reuse. 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 ); } - return Ok(None); + return Ok(AccountResolution::Missing); }; if linux_user.uid != user.uid() { anyhow::bail!( - "User '{}' has UID {} in the system but UID {} in the store, refusing to delete", + "User '{}' has UID {} in the system but UID {} in the store, refusing to act on \ + the account", user.name(), linux_user.uid, user.uid() ); } - Ok(Some(linux_user)) + 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> { @@ -626,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}"))? { @@ -665,31 +805,18 @@ async fn sync_user_supplementary_groups_by_name( } } -const SECONDS_PER_DAY: u64 = 86_400; +const SECONDS_PER_DAY: i64 = 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) - .context("System time is before the epoch")? - .as_secs() - / SECONDS_PER_DAY; - 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. -/// -/// The expiry is set to the day before the deletion: 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. +/// 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; + 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") - // 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. - .arg(expire_days.to_string()) + // Use epoch days to avoid YYYY-MM-DD timezone interpretation. + .arg(shadow_days(expire_at).to_string()) .arg(name) .output() .await @@ -702,22 +829,13 @@ 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. -/// -/// 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<()> { - let Some(expire_days) = account_expire_days(name)? else { - return Ok(()); - }; - if expire_days > days_since_epoch()? { +/// Clear an effective expiry while preserving a future scheduled expiry. +async fn clear_departure_expiry(name: &str) -> anyhow::Result<()> { + if !account_is_expired(name, chrono::Utc::now())? { return Ok(()); } @@ -736,12 +854,20 @@ async fn clear_deletion_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`. +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`. 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 @@ -749,7 +875,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 @@ -759,20 +884,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(); @@ -796,7 +915,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(); @@ -808,8 +927,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() { @@ -821,8 +939,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, @@ -837,13 +954,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| { @@ -857,7 +972,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/backends/mock.rs b/src/user_manager/backends/mock.rs index 33b21da..d291410 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,40 @@ 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, remove its scheduled work, 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..384e447 100644 --- a/src/user_manager/messages.rs +++ b/src/user_manager/messages.rs @@ -1,10 +1,7 @@ -//! 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::{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,68 +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: 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`. + /// 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 - /// deleting 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, } -/// 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 { +/// Reversibly expire a departed account and remove its active access. +/// +/// 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, } -/// 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 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, + /// Purge the account if it has been expired since before this timestamp. #[cfg_attr(not(target_os = "linux"), allow(dead_code))] - Prepared { home_dir: path::PathBuf }, + pub expired_before: chrono::DateTime, } -/// 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, +/// Result of checking and applying a [`PurgeAccount`] operation. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum PurgeOutcome { + /// The account and home directory were removed. + Purged, + /// No matching account exists. + #[cfg_attr(not(target_os = "linux"), allow(dead_code))] + NoAccount, + /// 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, @@ -90,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,