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

Add functions for working with file permissions. #170

Merged
merged 7 commits into from
Dec 21, 2019
Merged

Conversation

sunfishcode
Copy link
Member

@sunfishcode sunfishcode commented Dec 4, 2019

Add functions and types to query and set the filesystem permissions of
a file: read, write, execute (for files), and search (for directories).

These permissions aren't the only thing which determines whether a given
file or directory can be accessed; hosts may impose additional arbitrary
security restrictions not reflected here.

WASI itself does not currently have the ability to execute files, so the
meaning of "execute" here is entirely up to the host filesystem.

These are similar to fchmod/fchmodat/fstat/fstatat in POSIX, though
there are some differences:

  • This doesn't surface POSIX's setuid, setgid, or sticky bits. These can
    be added in the future if needed, though they'll require additional
    security and portability considerations.

  • This uses separate flags for "execute" on files vs "search" on
    directories. WASI libc can provide a POSIX-compatible C API.

  • This doesn't expose POSIX's user/group/other concepts. These can be
    added in the future if needed, though they'll require additional
    security and portability considerations. Implementations in POSIX
    environments should follow the umask when setting permissions flags.

[Edit - added discussion about user/group/other].

@bjorn3
Copy link

bjorn3 commented Dec 4, 2019

Maybe just split user and everyone permissions? Those should be supported on both UNIX and Windows systems. Relying on umask doesn't seem very nice, as not everyone knows about the existence of umask. Also you may want to prevent reading private keys by everyone, while the default umask on most systems enables read for everyone by default.

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

It not clear to me why we would need a separate permission_get. Why not just have the permission be part of the path_filestat_get result? It there some platform where getting permission is more expensive than, say, the mtime of a file?

Otherwise lgtm.

phases/ephemeral/witx/typenames.witx Outdated Show resolved Hide resolved
@sunfishcode
Copy link
Member Author

@sbc100

It not clear to me why we would need a separate permission_get. Why not just have the permission be part of the path_filestat_get result? It there some platform where getting permission is more expensive than, say, the mtime of a file?

Not a super strong opinion here. I've now changed it to work as you suggest.

I do have a feeling that filestat is too monolithic, but if we do split it up, that can be a separate conversation.

@bjorn3

Maybe just split user and everyone permissions? Those should be supported on both UNIX and Windows systems. Relying on umask doesn't seem very nice, as not everyone knows about the existence of umask. Also you may want to prevent reading private keys by everyone, while the default umask on most systems enables read for everyone by default.

That's a good point. I want to keep this as simple as possible, but maybe what we can do is add a $private permission flag, and say that path_open interprets the flag as:

  • if $private is unset, follow umask
  • if $private is set, follow umask | 0077

I'm imagining filestat_get on POSIX hosts could do this:

read    = (st_mode & (S_IRUSR | S_IRGRP | S_IROTH)) != 0 ||
          (S_ISDIR(st_mode) && (st_mode & (S_IXUSR | S_IXGRP | S_IXOTH)) != 0);
write   = (st_mode & (S_IWUSR | S_IWGRP | S_IWOTH)) != 0;
execute = (st_mode & (S_IXUSR | S_IXGRP | S_IXOTH)) != 0 &&
          !S_ISDIR(st_mode);
private = (st_mode & (S_IRGRP | S_IWGRP | S_IXGRP | S_IROTH | S_IWOTH | S_IXOTH)) == 0;

I've now add $private to the PR so we can iterate from here.

Add functions and types to query and set the filesystem permissions of
a file: read, write, execute (for files), and search (for directories).

These permissions aren't the only thing which determines whether a given
file or directory can be accessed; hosts may impose additional arbitrary
security restrictions not reflected here.

WASI itself does not currently have the ability to execute files, so the
meaning of "execute" here is entirely up to the host filesystem.

These are similar to fchmod/fchmodat/fstat/fstatat in POSIX, though
there are some differences:

 - This doesn't surface POSIX's setuid, setgid, or sticky bits. These can
   be added in the future if needed, though they'll require additional
   security and portability considerations.

 - This uses separate flags for "execute" on files vs "search" on
   directories. WASI libc can provide a POSIX-compatible C API.

 - This doesn't expose POSIX's user/group/other concepts. These can be
   added in the future if needed, though they'll require additional
   security and portability considerations. Implementations in POSIX
   environments should follow the umask when setting permissions flags.
This is instead of dedicated `permissions_get` functions.
Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm, although perhaps the initial version does't need the "private" concept? It seems rather limited, and could be added later after more discussion perhaps?

@sunfishcode
Copy link
Member Author

With the snapshot development model, we don't need to commit to this right now. The private concept should address @bjorn3's use case above of writing private keys, so unless there are specific concerns, I'm inclined to keep it until we get some real-world experience with it.

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

Successfully merging this pull request may close these issues.

3 participants