Skip to content

dirext: rework permissions handling in atomic_* #73

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

dirext: rework permissions handling in atomic_* #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 58 additions & 38 deletions src/dirext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
//! [`cap_std::fs::Dir`]: https://docs.rs/cap-std/latest/cap_std/fs/struct.Dir.html

use cap_primitives::fs::FileType;
#[cfg(unix)]
use cap_primitives::fs::MetadataExt;
use cap_std::fs::{Dir, File, Metadata};
use cap_tempfile::cap_std;
use cap_tempfile::cap_std::fs::DirEntry;
Expand Down Expand Up @@ -172,25 +174,26 @@ pub trait CapStdExtDirExt {
/// require a higher level wrapper which queries the existing file and gathers such metadata
/// before replacement.
///
/// # Example, including setting permissions
/// # Security
///
/// The closure may also perform other file operations beyond writing, such as changing
/// file permissions:
/// On filesystems that do not support `O_TMPFILE` (e.g. `vfat`), the TempFile is first created
/// with default permissions (mode 0o666 & ~umask) that can be readeable by everyone.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

readable

/// If this is a concern, you must set a more restrictive umask for your program.
///
/// # Example
///
/// ```rust
/// # use std::io;
/// # use std::io::Write;
/// # use cap_tempfile::cap_std;
/// # use cap_std::fs::PermissionsExt;
/// # fn main() -> io::Result<()> {
/// # let somedir = cap_tempfile::tempdir(cap_std::ambient_authority())?;
/// use cap_std_ext::prelude::*;
/// let contents = b"hello world\n";
/// somedir.atomic_replace_with("somefilename", |f| -> io::Result<_> {
/// let perms = cap_std::fs::Permissions::from_mode(0o600);
/// somedir.atomic_replace_with("somefilename", Some(perms), |f| -> io::Result<_> {
/// f.write_all(contents)?;
/// f.flush()?;
/// use cap_std::fs::PermissionsExt;
/// let perms = cap_std::fs::Permissions::from_mode(0o600);
/// f.get_mut().as_file_mut().set_permissions(perms)?;
/// Ok(())
/// })
/// # }
Expand All @@ -200,6 +203,7 @@ pub trait CapStdExtDirExt {
fn atomic_replace_with<F, T, E>(
&self,
destname: impl AsRef<Path>,
perms: Option<cap_std::fs::Permissions>,
f: F,
) -> std::result::Result<T, E>
where
Expand Down Expand Up @@ -318,26 +322,27 @@ pub trait CapStdExtDirExtUtf8 {
/// require a higher level wrapper which queries the existing file and gathers such metadata
/// before replacement.
///
/// # Example, including setting permissions
/// # Security
///
/// On filesystems that do not support `O_TMPFILE` (e.g. `vfat`), the TempFile is first created
/// with default permissions (mode 0o666 & ~umask) that can be readeable by everyone.
/// If this is a concern, you must set a more restrictive umask for your program.
///
/// The closure may also perform other file operations beyond writing, such as changing
/// file permissions:
/// # Example
///
/// ```rust
/// # use std::io;
/// # use std::io::Write;
/// # use cap_tempfile::cap_std;
/// # use cap_std::fs::PermissionsExt;
/// # fn main() -> io::Result<()> {
/// # let somedir = cap_tempfile::tempdir(cap_std::ambient_authority())?;
/// # let somedir = cap_std::fs_utf8::Dir::from_cap_std((&*somedir).try_clone()?);
/// use cap_std_ext::prelude::*;
/// let contents = b"hello world\n";
/// somedir.atomic_replace_with("somefilename", |f| -> io::Result<_> {
/// let perms = cap_std::fs::Permissions::from_mode(0o600);
/// somedir.atomic_replace_with("somefilename", Some(perms), |f| -> io::Result<_> {
/// f.write_all(contents)?;
/// f.flush()?;
/// use cap_std::fs::PermissionsExt;
/// let perms = cap_std::fs::Permissions::from_mode(0o600);
/// f.get_mut().as_file_mut().set_permissions(perms)?;
/// Ok(())
/// })
/// # }
Expand All @@ -347,6 +352,7 @@ pub trait CapStdExtDirExtUtf8 {
fn atomic_replace_with<F, T, E>(
&self,
destname: impl AsRef<Utf8Path>,
perms: Option<cap_std::fs::Permissions>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would require a semver bump, which is not a problem exactly but I'd prefer new APIs.

It feels to me like there's a bit of a conflict with the existing API promising to preserve permissions on existing files.

For the non-existing file case, a calling process can set the permissions in their write function which is how this was intended to be used.

The use case for overriding the perms on an existing file feels...somewhat obscure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now atomic_replace_with copy the existing perms, but maybe not the security bits.
It's easy not to know you must flush before setting security bits (not sure it's in a man page)
Also I want to ensure named temporary file are secure, and with the current API i don't see how to do it without leaving some small corner cases.
Overriding the perms of an existing file might not be common, but it could be that you now want to store secrets in your currently world readable config file.

f: F,
) -> std::result::Result<T, E>
where
Expand Down Expand Up @@ -710,6 +716,7 @@ impl CapStdExtDirExt for Dir {
fn atomic_replace_with<F, T, E>(
&self,
destname: impl AsRef<Path>,
perms: Option<cap_std::fs::Permissions>,
f: F,
) -> std::result::Result<T, E>
where
Expand All @@ -718,23 +725,47 @@ impl CapStdExtDirExt for Dir {
{
let destname = destname.as_ref();
let (d, name) = subdir_of(self, destname)?;
let existing_metadata = d.symlink_metadata_optional(destname)?;
// If the target is already a file, then acquire its mode, which we will preserve by default.
// We don't follow symlinks here for replacement, and so we definitely don't want to pick up its mode.
let existing_perms = existing_metadata
.filter(|m| m.is_file())
.map(|m| m.permissions());

// cap_tempfile doesn't support passing permissions on creation.
// https://github.com/bytecodealliance/cap-std/pull/390
let mut t = cap_tempfile::TempFile::new(&d)?;
// Apply the permissions, if we have them
if let Some(existing_perms) = existing_perms {
t.as_file_mut().set_permissions(existing_perms)?;
#[cfg(unix)]
let t_metadata = t.as_file().metadata()?;
#[cfg(unix)]
if t_metadata.nlink() > 0 {
let zeroperms = cap_std::fs::PermissionsExt::from_mode(0o000);
t.as_file().set_permissions(zeroperms)?;
}
Comment on lines +732 to 738
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it doesn't hurt to do so, why not unconditionally set zero mode even with O_TMPFILE?

Also, since this is already underneath #[cfg(unix)] I think we could just

#[cfg(unix)]
rustix::fs::fchmod(t.as_file(), Mode::from_raw(0))

or so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to convert to draft
This is going to change once we have bytecodealliance/cap-std#390
The idea is that O_TMPFILE is safe, and to get the default permissions we need to create another file (or to depend on /proc), so for O_TMPFILE (the happy path) we create with 0o666 & !umask, and if nobody wants to change the permissions, we already have the right ones, only in the case of named temp files we need some extra steps


// We always operate in terms of buffered writes
let mut bufw = std::io::BufWriter::new(t);
// Call the provided closure to generate the file content
let r = f(&mut bufw)?;
// Flush the buffer, get the TempFile
t = bufw.into_inner().map_err(From::from)?;
// Set the file permissions
// This must be done after flushing the application buffer else Linux clears the security related bits:
// https://github.com/torvalds/linux/blob/94305e83eccb3120c921cd3a015cd74731140bac/fs/inode.c#L2360
if let Some(perms) = perms {
t.as_file().set_permissions(perms)?;
} else {
let existing_metadata = d.symlink_metadata_optional(destname)?;
// If the target is already a file, then acquire its mode, which we will preserve by default.
// We don't follow symlinks here for replacement, and so we definitely don't want to pick up its mode.
let existing_perms = existing_metadata
.filter(|m| m.is_file())
.map(|m| m.permissions());
if let Some(existing_perms) = existing_perms {
// Apply the existing permissions, if we have them
t.as_file().set_permissions(existing_perms)?;
} else {
// Else restore default permissions
#[cfg(unix)]
if t_metadata.nlink() > 0 {
t.as_file().set_permissions(t_metadata.permissions())?;
}
}
}
// fsync the TempFile
t.as_file().sync_all()?;
// rename the TempFile
Expand All @@ -745,7 +776,7 @@ impl CapStdExtDirExt for Dir {
}

fn atomic_write(&self, destname: impl AsRef<Path>, contents: impl AsRef<[u8]>) -> Result<()> {
self.atomic_replace_with(destname, |f| f.write_all(contents.as_ref()))
self.atomic_replace_with(destname, None, |f| f.write_all(contents.as_ref()))
}

fn atomic_write_with_perms(
Expand All @@ -754,19 +785,8 @@ impl CapStdExtDirExt for Dir {
contents: impl AsRef<[u8]>,
perms: cap_std::fs::Permissions,
) -> Result<()> {
self.atomic_replace_with(destname, |f| -> io::Result<_> {
// If the user is overriding the permissions, let's make the default be
// writable by us but not readable by anyone else, in case it has
// secret data.
#[cfg(unix)]
{
use cap_std::fs::PermissionsExt;
let perms = cap_std::fs::Permissions::from_mode(0o600);
f.get_mut().as_file_mut().set_permissions(perms)?;
}
self.atomic_replace_with(destname, Some(perms), |f| -> io::Result<_> {
f.write_all(contents.as_ref())?;
f.flush()?;
f.get_mut().as_file_mut().set_permissions(perms)?;
Ok(())
})
}
Expand Down
18 changes: 14 additions & 4 deletions tests/it/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,21 +164,21 @@ fn default_mode(d: &Dir) -> Result<Permissions> {
fn link_tempfile_with() -> Result<()> {
let td = cap_tempfile::tempdir(cap_std::ambient_authority())?;
let p = Path::new("foo");
td.atomic_replace_with(p, |f| writeln!(f, "hello world"))
td.atomic_replace_with(p, None, |f| writeln!(f, "hello world"))
.unwrap();
assert_eq!(td.read_to_string(p).unwrap().as_str(), "hello world\n");
let default_perms = default_mode(&td)?;
assert_eq!(td.metadata(p)?.permissions(), default_perms);

td.atomic_replace_with(p, |f| writeln!(f, "atomic replacement"))
td.atomic_replace_with(p, None, |f| writeln!(f, "atomic replacement"))
.unwrap();
assert_eq!(
td.read_to_string(p).unwrap().as_str(),
"atomic replacement\n"
);

let e = td
.atomic_replace_with(p, |f| {
.atomic_replace_with(p, None, |f| {
writeln!(f, "should not be written")?;
Err::<(), _>(std::io::Error::new(std::io::ErrorKind::Other, "oops"))
})
Expand Down Expand Up @@ -238,14 +238,24 @@ fn link_tempfile_with() -> Result<()> {
td.atomic_write_with_perms(p, "self-only file v3", Permissions::from_mode(0o640))
.unwrap();
assert_eq!(td.metadata(p).unwrap().permissions().mode() & 0o777, 0o640);
// Write a file with mode 000
td.atomic_write_with_perms(p, "no permissions", Permissions::from_mode(0o000))
.unwrap();
let metadata = td.metadata(p).unwrap();
assert_eq!(metadata.permissions().mode() & 0o777, 0o000);
assert_eq!(metadata.len(), 14);
// Replace a file with mode 000
td.atomic_write_with_perms(p, "600", Permissions::from_mode(0o600))
.unwrap();
assert_eq!(td.metadata(p).unwrap().permissions().mode() & 0o777, 0o600);
Ok(())
}

#[test]
fn test_timestamps() -> Result<()> {
let td = cap_tempfile::tempdir(cap_std::ambient_authority())?;
let p = Path::new("foo");
td.atomic_replace_with(p, |f| writeln!(f, "hello world"))
td.atomic_replace_with(p, None, |f| writeln!(f, "hello world"))
.unwrap();
let ts0 = td.metadata(p)?.modified()?;
// This test assumes at least second granularity on filesystem timestamps, and
Expand Down