From f2860e74cd35681747a4c677c18776b5bd476021 Mon Sep 17 00:00:00 2001 From: Daniel Carl Jones Date: Tue, 28 Nov 2023 10:10:10 +0000 Subject: [PATCH] Update cache directory to create content with MP owner access only (#637) Signed-off-by: Daniel Carl Jones --- .../src/data_cache/cache_directory.rs | 108 ++++++++++++------ .../src/data_cache/disk_data_cache.rs | 15 ++- 2 files changed, 84 insertions(+), 39 deletions(-) diff --git a/mountpoint-s3/src/data_cache/cache_directory.rs b/mountpoint-s3/src/data_cache/cache_directory.rs index 1c589ce4c..0c9da2f0e 100644 --- a/mountpoint-s3/src/data_cache/cache_directory.rs +++ b/mountpoint-s3/src/data_cache/cache_directory.rs @@ -8,6 +8,7 @@ use std::fs; use std::io; +use std::os::unix::fs::DirBuilderExt; use std::path::{Path, PathBuf}; use thiserror::Error; @@ -27,36 +28,39 @@ pub enum ManagedCacheDirError { } impl ManagedCacheDir { - /// Create a new directory inside the provided parent path, cleaning it of any contents if it already exists. + /// Create a new directory inside the provided parent path. + /// If the directory already exists, it will be deleted before being recreated. pub fn new_from_parent>(parent_path: P) -> Result { - let managed_path = parent_path.as_ref().join("mountpoint-cache"); + let managed_cache_dir = Self { + managed_path: parent_path.as_ref().join("mountpoint-cache"), + }; - if let Err(mkdir_err) = fs::create_dir(&managed_path) { + managed_cache_dir.remove()?; + + let mkdir_result = fs::DirBuilder::new().mode(0o700).create(managed_cache_dir.as_path()); + if let Err(mkdir_err) = mkdir_result { match mkdir_err.kind() { - io::ErrorKind::AlreadyExists => (), + io::ErrorKind::AlreadyExists => tracing::warn!( + cache_dir = ?managed_cache_dir.as_path(), + "cache sub-directory already existed immediately after removal", + ), _kind => return Err(ManagedCacheDirError::CreationFailure(mkdir_err)), } } - let managed_cache_dir = Self { managed_path }; - managed_cache_dir.clean()?; - Ok(managed_cache_dir) } - /// Clear the cache sub-directory - fn clean(&self) -> Result<(), ManagedCacheDirError> { - tracing::debug!(cache_subdirectory = ?self.managed_path, "cleaning up contents of cache sub-directory"); - let read_dir = fs::read_dir(self.managed_path.as_path()).map_err(ManagedCacheDirError::CleanupFailure)?; - for entry in read_dir { - let path = entry.map_err(ManagedCacheDirError::CleanupFailure)?.path(); - if path.is_dir() { - fs::remove_dir_all(path).map_err(ManagedCacheDirError::CleanupFailure)?; - } else { - fs::remove_file(path).map_err(ManagedCacheDirError::CleanupFailure)?; + /// Remove the cache sub-directory, along with its contents if any + fn remove(&self) -> Result<(), ManagedCacheDirError> { + tracing::debug!(cache_subdirectory = ?self.managed_path, "removing the cache sub-directory and any contents"); + if let Err(remove_dir_err) = fs::remove_dir_all(&self.managed_path) { + match remove_dir_err.kind() { + io::ErrorKind::NotFound => (), + _kind => return Err(ManagedCacheDirError::CleanupFailure(remove_dir_err)), } } - tracing::trace!(cache_subdirectory = ?self.managed_path, "cleanup complete"); + tracing::trace!(cache_subdirectory = ?self.managed_path, "cache sub-directory removal complete"); Ok(()) } @@ -73,8 +77,8 @@ impl ManagedCacheDir { impl Drop for ManagedCacheDir { fn drop(&mut self) { - if let Err(err) = self.clean() { - tracing::error!(managed_cache_path = ?self.managed_path, "failed to clean cache directory: {err}"); + if let Err(err) = self.remove() { + tracing::error!(cache_subdirectory = ?self.managed_path, "failed to remove cache sub-directory: {err}"); } } } @@ -84,6 +88,9 @@ mod tests { use super::ManagedCacheDir; use std::fs; + use std::os::unix::fs::{DirBuilderExt, PermissionsExt}; + + const EXPECTED_DIR_MODE: u32 = 0o700; #[test] fn test_unused() { @@ -92,13 +99,21 @@ mod tests { let managed_dir = ManagedCacheDir::new_from_parent(temp_dir.path()).expect("creating managed dir should succeed"); - assert!(expected_path.try_exists().unwrap(), "{expected_path:?} should exist"); + let dir_mode = fs::metadata(&expected_path) + .expect("path should exist") + .permissions() + .mode(); + assert_eq!( + dir_mode & 0o777, + EXPECTED_DIR_MODE, + "path should have {EXPECTED_DIR_MODE:#o} permission mode but had {dir_mode:#o}", + ); drop(managed_dir); - let dir_entries = fs::read_dir(&expected_path) - .expect("cache dir should still exist") - .count(); - assert!(dir_entries == 0, "directory should be empty"); + assert!( + !expected_path.try_exists().unwrap(), + "{expected_path:?} should not exist" + ); temp_dir.close().unwrap(); } @@ -110,7 +125,15 @@ mod tests { let managed_dir = ManagedCacheDir::new_from_parent(temp_dir.path()).expect("creating managed dir should succeed"); - assert!(expected_path.try_exists().unwrap(), "{expected_path:?} should exist"); + let dir_mode = fs::metadata(&expected_path) + .expect("path should exist") + .permissions() + .mode(); + assert_eq!( + dir_mode & 0o777, + EXPECTED_DIR_MODE, + "path should have {EXPECTED_DIR_MODE:#o} permission mode but had {dir_mode:#o}", + ); fs::File::create(expected_path.join("file.txt")) .expect("should be able to create file within managed directory"); @@ -119,10 +142,10 @@ mod tests { .expect("should be able to create file within subdirectory"); drop(managed_dir); - let dir_entries = fs::read_dir(&expected_path) - .expect("cache dir should still exist") - .count(); - assert!(dir_entries == 0, "directory should be empty"); + assert!( + !expected_path.try_exists().unwrap(), + "{expected_path:?} should not exist" + ); temp_dir.close().unwrap(); } @@ -132,21 +155,34 @@ mod tests { let temp_dir = tempfile::tempdir().unwrap(); let expected_path = temp_dir.path().join("mountpoint-cache"); - fs::create_dir_all(expected_path.join("dir")).unwrap(); + fs::DirBuilder::new() + .recursive(true) + .mode(0o775) // something that isn't the expected `0o700` + .create(expected_path.join("dir")) + .unwrap(); fs::File::create(expected_path.join("dir/file.txt")).unwrap(); let managed_dir = ManagedCacheDir::new_from_parent(temp_dir.path()).expect("creating managed dir should succeed"); - assert!(expected_path.try_exists().unwrap(), "{expected_path:?} should exist"); + + let dir_mode = fs::metadata(&expected_path) + .expect("path should exist") + .permissions() + .mode(); + assert_eq!( + dir_mode & 0o777, + EXPECTED_DIR_MODE, + "path should have {EXPECTED_DIR_MODE:#o} permission mode but had {dir_mode:#o}", + ); let dir_entries = fs::read_dir(&expected_path).unwrap().count(); assert!(dir_entries == 0, "directory should be empty"); drop(managed_dir); - let dir_entries = fs::read_dir(&expected_path) - .expect("cache dir should still exist") - .count(); - assert!(dir_entries == 0, "directory should be empty"); + assert!( + !expected_path.try_exists().unwrap(), + "{expected_path:?} should not exist" + ); temp_dir.close().unwrap(); } diff --git a/mountpoint-s3/src/data_cache/disk_data_cache.rs b/mountpoint-s3/src/data_cache/disk_data_cache.rs index 6423e5984..4d76b0127 100644 --- a/mountpoint-s3/src/data_cache/disk_data_cache.rs +++ b/mountpoint-s3/src/data_cache/disk_data_cache.rs @@ -2,6 +2,7 @@ use std::fs; use std::io::{ErrorKind, Read, Seek, Write}; +use std::os::unix::fs::{DirBuilderExt, OpenOptionsExt}; use std::path::{Path, PathBuf}; use std::time::Instant; @@ -269,7 +270,10 @@ impl DiskDataCache { .as_ref() .parent() .expect("path should include cache key in directory name"); - fs::create_dir_all(cache_path_for_key)?; + fs::DirBuilder::new() + .mode(0o700) + .recursive(true) + .create(cache_path_for_key)?; trace!( key = block.header.s3_key, @@ -277,7 +281,12 @@ impl DiskDataCache { "writing block at {}", path.as_ref().display() ); - let mut file = fs::File::create(path.as_ref())?; + let mut file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path.as_ref())?; file.write_all(CACHE_VERSION.as_bytes())?; let serialize_result = bincode::serialize_into(&mut file, &block); if let Err(err) = serialize_result { @@ -439,7 +448,7 @@ impl DataCache for DiskDataCache { /// Key to identify a block in the disk cache, composed of a hash of the S3 key and Etag, and the block index. /// An S3 key may be up to 1024 UTF-8 bytes long, which exceeds the maximum UNIX file name length. /// Instead, this key contains a hash of the S3 key and ETag to avoid the limit when used in paths. -/// The risk of collisions is mitigated as we ignore blocks read that contain the wrong S3 key, etc.. +/// The risk of collisions is mitigated as we ignore blocks read that contain the wrong S3 key, etc.. #[derive(Debug, Hash, PartialEq, Eq, Clone, Copy)] struct DiskBlockKey { hashed_key: [u8; 32],