From 608e7e465724ee67fb124e965158eb54ca816da2 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 6 Feb 2025 14:49:54 +0100 Subject: [PATCH 1/2] fix: use atomic tempfile to persist file credentials instead of locking --- crates/rattler_networking/Cargo.toml | 5 +- .../authentication_storage/backends/file.rs | 104 +++++++++--------- ...gration_test.rs => s3_integration_test.rs} | 3 + 3 files changed, 57 insertions(+), 55 deletions(-) rename crates/rattler_networking/tests/{integration_test.rs => s3_integration_test.rs} (99%) diff --git a/crates/rattler_networking/Cargo.toml b/crates/rattler_networking/Cargo.toml index e25cf1caa..03bcf422b 100644 --- a/crates/rattler_networking/Cargo.toml +++ b/crates/rattler_networking/Cargo.toml @@ -19,11 +19,10 @@ s3 = ["aws-config", "aws-sdk-s3"] [dependencies] anyhow = { workspace = true } -async-fd-lock = { workspace = true } async-trait = { workspace = true } base64 = { workspace = true } -chrono = { workspace = true } dirs = { workspace = true } +fs-err = { workspace = true } google-cloud-auth = { workspace = true, optional = true } google-cloud-token = { workspace = true, optional = true } aws-config = { workspace = true, optional = true } @@ -43,6 +42,7 @@ reqwest-middleware = { workspace = true } retry-policies = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } +tempfile = { workspace = true } thiserror = { workspace = true } tracing = { workspace = true } url = { workspace = true } @@ -60,4 +60,3 @@ reqwest-retry = { workspace = true } sha2 = { workspace = true } temp-env = { workspace = true } rstest = { workspace = true } -rand = { workspace = true } diff --git a/crates/rattler_networking/src/authentication_storage/backends/file.rs b/crates/rattler_networking/src/authentication_storage/backends/file.rs index a58748e6a..d2f41100c 100644 --- a/crates/rattler_networking/src/authentication_storage/backends/file.rs +++ b/crates/rattler_networking/src/authentication_storage/backends/file.rs @@ -1,22 +1,20 @@ //! file storage for passwords. -use async_fd_lock::{ - blocking::{LockRead, LockWrite}, - RwLockWriteGuard, +use std::{ + collections::BTreeMap, + ffi::OsStr, + io::BufWriter, + path::{Path, PathBuf}, + sync::{Arc, RwLock}, }; -use std::collections::BTreeMap; -use std::fs::File; -use std::io::BufWriter; -use std::path::Path; -use std::path::PathBuf; -use std::sync::{Arc, RwLock}; -use crate::authentication_storage::{AuthenticationStorageError, StorageBackend}; -use crate::Authentication; +use crate::{ + authentication_storage::{AuthenticationStorageError, StorageBackend}, + Authentication, +}; #[derive(Clone, Debug)] struct FileStorageCache { content: BTreeMap, - file_exists: bool, } /// A struct that implements storage and access of authentication @@ -36,36 +34,27 @@ pub struct FileStorage { #[derive(thiserror::Error, Debug)] pub enum FileStorageError { /// An IO error occurred when accessing the file storage - #[error("IO error: {0}")] + #[error(transparent)] IOError(#[from] std::io::Error), - /// Failed to lock the file storage file - #[error("failed to lock file storage file: {0:?}")] - FailedToLock(async_fd_lock::LockError), - /// An error occurred when (de)serializing the credentials - #[error("JSON error: {0}")] - JSONError(#[from] serde_json::Error), + #[error("failed to parse {0}: {1}")] + JSONError(PathBuf, serde_json::Error), } impl FileStorageCache { pub fn from_path(path: &Path) -> Result { - let file_exists = path.exists(); - let content = if file_exists { - let read_guard = File::options() - .read(true) - .open(path)? - .lock_read() - .map_err(FileStorageError::FailedToLock)?; - serde_json::from_reader(read_guard)? - } else { - BTreeMap::new() - }; - - Ok(Self { - content, - file_exists, - }) + match fs_err::read_to_string(path) { + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Self { + content: BTreeMap::new(), + }), + Err(e) => return Err(FileStorageError::IOError(e)), + Ok(content) => { + let content = serde_json::from_str(&content) + .map_err(|e| FileStorageError::JSONError(path.to_path_buf(), e))?; + Ok(Self { content }) + } + } } } @@ -87,13 +76,12 @@ impl FileStorage { Self::from_path(path) } - /// Updates the cache by reading the JSON file and deserializing it into a `BTreeMap`, or return an empty `BTreeMap` if the - /// file does not exist + /// Updates the cache by reading the JSON file and deserializing it into a + /// `BTreeMap`, or return an empty `BTreeMap` if the file does not exist fn read_json(&self) -> Result, FileStorageError> { let new_cache = FileStorageCache::from_path(&self.path)?; let mut cache = self.cache.write().unwrap(); cache.content = new_cache.content; - cache.file_exists = new_cache.file_exists; Ok(cache.content.clone()) } @@ -107,22 +95,32 @@ impl FileStorage { "Parent directory not found", )))?; std::fs::create_dir_all(parent)?; - let write_guard: std::result::Result< - RwLockWriteGuard, - async_fd_lock::LockError, - > = File::options() - .create(true) - .write(true) - .truncate(true) - .open(&self.path)? - .lock_write(); - let write_guard = write_guard.map_err(FileStorageError::FailedToLock)?; - serde_json::to_writer(BufWriter::new(write_guard), dict)?; + + let prefix = self + .path + .file_stem() + .unwrap_or_else(|| OsStr::new("credentials")); + let extension = self + .path + .extension() + .and_then(OsStr::to_str) + .unwrap_or("json"); + + // Write the contents to a temporary file and then atomically move it to the + // final location. + let mut temp_file = tempfile::Builder::new() + .prefix(prefix) + .suffix(&format!(".{extension}")) + .tempfile_in(parent)?; + serde_json::to_writer(BufWriter::new(&mut temp_file), dict) + .map_err(std::io::Error::from)?; + temp_file + .persist(&self.path) + .map_err(std::io::Error::from)?; // Store the new data in the cache let mut cache = self.cache.write().unwrap(); cache.content = dict.clone(); - cache.file_exists = true; Ok(()) } @@ -156,11 +154,13 @@ impl StorageBackend for FileStorage { #[cfg(test)] mod tests { - use super::*; - use insta::assert_snapshot; use std::{fs, io::Write}; + + use insta::assert_snapshot; use tempfile::tempdir; + use super::*; + #[test] fn test_file_storage() { let file = tempdir().unwrap(); diff --git a/crates/rattler_networking/tests/integration_test.rs b/crates/rattler_networking/tests/s3_integration_test.rs similarity index 99% rename from crates/rattler_networking/tests/integration_test.rs rename to crates/rattler_networking/tests/s3_integration_test.rs index 464439e2b..e5782e6de 100644 --- a/crates/rattler_networking/tests/integration_test.rs +++ b/crates/rattler_networking/tests/s3_integration_test.rs @@ -1,9 +1,12 @@ +#![cfg(feature = "s3")] + use std::{collections::HashMap, sync::Arc}; use rattler_networking::{ authentication_storage::backends::file::FileStorage, s3_middleware::S3Config, AuthenticationMiddleware, AuthenticationStorage, S3Middleware, }; + use reqwest::Client; use rstest::*; use temp_env::async_with_vars; From 9f83ec598ca1a0e6522e44a78b5949c35bce4af7 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 6 Feb 2025 14:59:01 +0100 Subject: [PATCH 2/2] fix: clippy --- .../src/authentication_storage/backends/file.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rattler_networking/src/authentication_storage/backends/file.rs b/crates/rattler_networking/src/authentication_storage/backends/file.rs index d2f41100c..8962f6d9f 100644 --- a/crates/rattler_networking/src/authentication_storage/backends/file.rs +++ b/crates/rattler_networking/src/authentication_storage/backends/file.rs @@ -48,7 +48,7 @@ impl FileStorageCache { Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(Self { content: BTreeMap::new(), }), - Err(e) => return Err(FileStorageError::IOError(e)), + Err(e) => Err(FileStorageError::IOError(e)), Ok(content) => { let content = serde_json::from_str(&content) .map_err(|e| FileStorageError::JSONError(path.to_path_buf(), e))?;