Skip to content
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.

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

Make permissions and ownership? configurable while persisting #137

Open
xaverdh opened this issue Jan 7, 2021 · 7 comments
Open

Make permissions and ownership? configurable while persisting #137

xaverdh opened this issue Jan 7, 2021 · 7 comments

Comments

@xaverdh
Copy link

xaverdh commented Jan 7, 2021

Related issues: #30, #114

It might not be possible to support on all platforms, but it would still be nice to have it at least on unix. This issue is specifically about permissions / ownership of the persisted file rather than the temporary file, but I would also be quite happy with an option to configure permissions on the temporary, if they get carried over to the persisted file.

My use case is to atomically write files with given permissions / ownership by first writing to a temporary file and then persisting it (relying on rename likely being atomic), so just changing these after the fact will slightly defeat the purpose..

@xaverdh
Copy link
Author

xaverdh commented Jan 7, 2021

Implementation wise, I think the most realistic approach is to make permissions on the temporary file configurable and then make persist keep those. Ownership could be configured through the Builder, but would only come into effect when persisting.

On Linux this would correspond to passing O_TMPFILE, but without O_EXCL and creating it with given mode. Then just before persisting it with linkat you would call fchmod to adjust ownership.
I don't know how well this fits with other platforms though..

@Stebalien
Copy link
Owner

As far as I know, persist should keep the permissions. So you should be able to:

  1. Create a named temporary file.
  2. Chmod the temporary file.
  3. Call persist.

On Linux this would correspond to passing O_TMPFILE, but without O_EXCL and creating it with given mode. Then just before persisting it with linkat you would call fchmod to adjust ownership.

This library does not support persisting temporary files created with O_TMPFILE, unfortunately. I'd like to implement this, but I have a policy of only providing features that are truly cross-platform (linux, windows, macos, freebsd), including sharing equivalent security guarantees.

@D1plo1d
Copy link
Contributor

D1plo1d commented May 8, 2021

This library does not support persisting temporary files created with O_TMPFILE, unfortunately. I'd like to implement this, but I have a policy of only providing features that are truly cross-platform (linux, windows, macos, freebsd), including sharing equivalent security guarantees.

Would it be possible to remove the O_EXCL flag entirely? Is there a security difference from eg. windows or some other OS if it is removed? It appears that according to the gnu libc docs O_EXCL | O_TMPFILE only serve to prevent persisting tempfiles and it's removal would allow developers to persist tempfiles externally to tempfile.rs using nix.

For example without O_EXCL the following works:

tmpfile::tmpfile();
let tmp_file_path = format!("/proc/self/fd/{}", tmp_file.as_raw_fd());

nix::unistd::linkat(
    None,
    &tmp_file_path[..],
    None,
    &file_path[..],
    nix::unistd::LinkatFlags::SymlinkFollow,
)?;

@Stebalien
Copy link
Owner

Stebalien commented May 10, 2021 via email

@D1plo1d
Copy link
Contributor

D1plo1d commented May 10, 2021

Ok yeah, so I'm wondering if removing the flag preventing the file from being copied on platforms where it can be - is that a feature or a secruity concern?

I don't know if the absense of a flag is a feature - maybe this is pedantic but it feels like a move to be more inline with the OS defaults IMHO.

I'm trying to imagine the malicious use of this but I haven't been able to come up with anything yet.

So I don't know: If we only removed that flag what would we be potentially disrupting for users of tmpfile?

@Stebalien
Copy link
Owner

Stebalien commented May 10, 2021

Ok yeah, so I'm wondering if removing the flag preventing the file from being copied on platforms where it can be - is that a feature or a secruity concern?

Ah, no. I'm happy to accept a patch to remove that (assuming it works cross-platform... linux only so it should be fine).

yuja added a commit to yuja/qmluic that referenced this issue May 23, 2022
Since getting umask is hard, it just copies a file mode from the known good
source.

Stebalien/tempfile#137
@Byron
Copy link
Contributor

Byron commented Apr 20, 2023

For gitoxide it would be most beneficial if the mode could be set for the temporary file at creation time to avoid a follow-up chmod call for increased performance by reducing the amount of syscalls.

Git does the same by default.

As per the policy of providing cross-platform features only, implementing this would be out of scope though. Would you be open to a contribution if this was an exceptional case?

Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants