fix: use filelock for authentication fallback storage (#427)

crates/rattler_networking/Cargo.toml

@@ -19,17 +19,19 @@ blocking = ['reqwest/blocking']
 [dependencies]
 anyhow = "1.0.75"
 dirs = "5.0.1"
+fslock = "0.2.1"
+itertools = "0.11.0"
 keyring = "2.0.5"
 lazy_static = "1.4.0"
 libc = "0.2.148"
+once_cell = "1.18.0"
 reqwest = { version = "0.11.22", default-features = false }
 retry-policies = { version = "0.2.0", default-features = false }
 serde = "1.0.188"
 serde_json = "1.0.107"
 thiserror = "1.0.49"
 tracing = "0.1.37"
 url = "2.4.1"
-itertools = "0.11.0"
 
 [target.'cfg( target_arch = "wasm32" )'.dependencies]
 getrandom = { version = "0.2.10", features = ["js"] }

crates/rattler_networking/src/authentication_storage/fallback_storage.rs

@@ -1,18 +1,15 @@
 //! Fallback storage for passwords.
-use std::{
-    path::PathBuf,
-    sync::{Arc, Mutex},
-};
+use fslock::LockFile;
+use once_cell::sync::Lazy;
+use std::collections::{HashMap, HashSet};
+use std::{path::PathBuf, sync::Mutex};
 
 /// A struct that implements storage and access of authentication
 /// information backed by a on-disk JSON file
 #[derive(Clone)]
 pub struct FallbackStorage {
     /// The path to the JSON file
     pub path: PathBuf,
-
-    /// A mutex to ensure that only one thread accesses the file at a time
-    mutex: Arc<Mutex<()>>,
 }
 
 /// An error that can occur when accessing the fallback storage
@@ -22,6 +19,10 @@ pub enum FallbackStorageError {
     #[error("IO error: {0}")]
     IOError(#[from] std::io::Error),
 
+    /// Failed to lock the fallback storage file
+    #[error("failed to lock fallback storage file {0}.")]
+    FailedToLock(String, #[source] std::io::Error),
+
     /// An error occurred when (de)serializing the credentials
     #[error("JSON error: {0}")]
     JSONError(#[from] serde_json::Error),
@@ -30,44 +31,68 @@ pub enum FallbackStorageError {
 impl FallbackStorage {
     /// Create a new fallback storage with the given path
     pub fn new(path: PathBuf) -> Self {
-        Self {
-            path,
-            mutex: Arc::new(Mutex::new(())),
+        Self { path }
+    }
+
+    /// Lock the fallback storage file for reading and writing. This will block until the lock is
+    /// acquired.
+    fn lock(&self) -> Result<LockFile, FallbackStorageError> {
+        std::fs::create_dir_all(self.path.parent().unwrap())?;
+        let path = self.path.with_extension("lock");
+        let mut lock = fslock::LockFile::open(&path).map_err(|e| {
+            FallbackStorageError::FailedToLock(path.to_string_lossy().into_owned(), e)
+        })?;
+
+        // First try to lock the file without block. If we can't immediately get the lock we block and issue a debug message.
+        if !lock.try_lock_with_pid().map_err(|e| {
+            FallbackStorageError::FailedToLock(path.to_string_lossy().into_owned(), e)
+        })? {
+            tracing::debug!("waiting for lock on {}", path.to_string_lossy());
+            lock.lock_with_pid().map_err(|e| {
+                FallbackStorageError::FailedToLock(path.to_string_lossy().into_owned(), e)
+            })?;
         }
+
+        Ok(lock)
     }
 
     /// Store the given authentication information for the given host
     pub fn set_password(&self, host: &str, password: &str) -> Result<(), FallbackStorageError> {
-        let _lock = self.mutex.lock().unwrap();
+        let _lock = self.lock()?;
         let mut dict = self.read_json()?;
         dict.insert(host.to_string(), password.to_string());
         self.write_json(&dict)
     }
 
     /// Retrieve the authentication information for the given host
     pub fn get_password(&self, host: &str) -> Result<Option<String>, FallbackStorageError> {
-        let _lock = self.mutex.lock().unwrap();
+        let _lock = self.lock()?;
         let dict = self.read_json()?;
         Ok(dict.get(host).cloned())
     }
 
     /// Delete the authentication information for the given host
     pub fn delete_password(&self, host: &str) -> Result<(), FallbackStorageError> {
-        let _lock = self.mutex.lock().unwrap();
+        let _lock = self.lock()?;
         let mut dict = self.read_json()?;
         dict.remove(host);
         self.write_json(&dict)
     }
 
     /// Read the JSON file and deserialize it into a HashMap, or return an empty HashMap if the file
     /// does not exist
-    fn read_json(&self) -> Result<std::collections::HashMap<String, String>, FallbackStorageError> {
+    fn read_json(&self) -> Result<HashMap<String, String>, FallbackStorageError> {
         if !self.path.exists() {
-            tracing::warn!(
-                "Can't find path for fallback storage on {}",
-                self.path.to_string_lossy()
-            );
-            return Ok(std::collections::HashMap::new());
+            static WARN_GUARD: Lazy<Mutex<HashSet<PathBuf>>> =
+                Lazy::new(|| Mutex::new(HashSet::new()));
+            let mut guard = WARN_GUARD.lock().unwrap();
+            if !guard.insert(self.path.clone()) {
+                tracing::warn!(
+                    "Can't find path for fallback storage on {}",
+                    self.path.to_string_lossy()
+                );
+            }
+            return Ok(HashMap::new());
         }
         let file = std::fs::File::open(&self.path)?;
         let reader = std::io::BufReader::new(file);
@@ -76,13 +101,7 @@ impl FallbackStorage {
     }
 
     /// Serialize the given HashMap and write it to the JSON file
-    fn write_json(
-        &self,
-        dict: &std::collections::HashMap<String, String>,
-    ) -> Result<(), FallbackStorageError> {
-        if !self.path.exists() {
-            std::fs::create_dir_all(self.path.parent().unwrap())?;
-        }
+    fn write_json(&self, dict: &HashMap<String, String>) -> Result<(), FallbackStorageError> {
         let file = std::fs::File::create(&self.path)?;
         let writer = std::io::BufWriter::new(file);
         serde_json::to_writer(writer, dict)?;

crates/rattler_networking/src/lib.rs

@@ -252,6 +252,12 @@ mod tests {
         let tdir = tempdir()?;
         let storage = super::AuthenticationStorage::new("rattler_test", tdir.path());
         let host = "conda.example.com";
+
+        // Make sure the keyring is empty
+        if let Ok(entry) = keyring::Entry::new("rattler_test", host) {
+            let _ = entry.delete_password();
+        }
+
         let retrieved = storage.get(host);
 
         if let Err(e) = retrieved.as_ref() {
@@ -288,6 +294,12 @@ mod tests {
         let tdir = tempdir()?;
         let storage = super::AuthenticationStorage::new("rattler_test", tdir.path());
         let host = "bearer.example.com";
+
+        // Make sure the keyring is empty
+        if let Ok(entry) = keyring::Entry::new("rattler_test", host) {
+            let _ = entry.delete_password();
+        }
+
         let retrieved = storage.get(host);
 
         if let Err(e) = retrieved.as_ref() {
@@ -329,6 +341,12 @@ mod tests {
         let tdir = tempdir()?;
         let storage = super::AuthenticationStorage::new("rattler_test", tdir.path());
         let host = "basic.example.com";
+
+        // Make sure the keyring is empty
+        if let Ok(entry) = keyring::Entry::new("rattler_test", host) {
+            let _ = entry.delete_password();
+        }
+
         let retrieved = storage.get(host);
 
         if let Err(e) = retrieved.as_ref() {