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

filestore: add dirs(0775 def) and files(0664 def) chmod(2) perms cmdline options #1155

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

avreg
Copy link

@avreg avreg commented Jul 12, 2024

Hi.

This pool request allows flexible use of DAC access attributes to access intermediate directories, if used.

@avreg avreg changed the title filestore: add dirs(0775) and files(0644) chmod(2) perms options filestore: add dirs(0775 def) and files(0664 def) chmod(2) perms cmdline options Jul 13, 2024
Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! I had a brief look at this, but I was wondering what other file permissions you want to use? Should we change the default file permission?

@@ -6,4 +6,8 @@ set -o pipefail

. /usr/local/share/load-env.sh

if [ -n "$UMASK" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unrelated to the PR itself. Can you please remove it if that's the case?

Copy link
Author

@avreg avreg Jul 15, 2024

Choose a reason for hiding this comment

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

Sure, it can be removed, but ...

On many UNIX systems, the default umask is 022.
This significantly limits the usefulness of the new options in the context of "group" and "other" permissions
If we don't use the UMASK environment variable, we have to call umask(2) directly in the go application (hard-coded zero or add new option like -fs-umask=umask).

Copy link
Member

Choose a reason for hiding this comment

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

I don't know how umask works. Can you provide some hints or links so I can understand what this is about?

Copy link
Author

Choose a reason for hiding this comment

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

Here is an example using Go code
https://stackoverflow.com/a/61645606/23561452

I needed this patch to get group write access to the intermediate directories of the downloaded file.

// pre-create hook:
return {
            ChangeFileInfo: {
                ID: "subdir1/subdir2/.../basename",
            },
        };

Copy link
Member

@Acconut Acconut Jul 16, 2024

Choose a reason for hiding this comment

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

Sorry, I meant that I don't know what umask is in general. Maybe you can provide a bit more detailed explanation showing the motivation/necessity behind your PR.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, now you exactly understand the mechanics of how umask(2) works.

diff --git a/docker/entrypoint.sh b/docker/entrypoint.sh
index 88a70ef..f1934bb 100644
--- a/docker/entrypoint.sh
+++ b/docker/entrypoint.sh
@@ -6,4 +6,18 @@ set -o pipefail
 
 . /usr/local/share/load-env.sh
 
+if printenv UMASK >/dev/null; then
+   umask "$UMASK"
+fi

With this and the other above patches and without explicitly setting env UMASK for the docker container, the tusd daemon will create new directories/files with 0755/0644 permissions (since usually linux distributions set umask 022 for users by default).

If a user needs something special, like "wx" access for a group, they will have to pass env UMASK=002 to the container. And then the permissions of newly created directories/files will become 0775/0664.

This changes is lightweight and should satisfy most users. Unless the user wants different permissions for files and directories, e.g. 0755 (g=rx) for directories but 0660 (g=rwx) for files ;)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the patience. I'm slowly getting it. Do you know how other Docker images handle this? Do they expose a UMASK environment variable as well? Or are there other patterns to control file permissions from containers?

Copy link
Author

@avreg avreg Sep 19, 2024

Choose a reason for hiding this comment

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

The docker ecosystem has "Isolate containers with a user namespace", which would be interesting to use with a TUSD container, but it won't solve the UMASK issue.

Copy link
Member

Choose a reason for hiding this comment

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

This doesn't really ask my question, so let me try to rephrase it: Do other Docker images also use the UMASK environment variable for configuring the umask of the process running in the Docker container? Does the Docker community have other defacto ways of configuring the umask?

Copy link
Author

@avreg avreg Sep 20, 2024

Choose a reason for hiding this comment

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

I have not used any other projects where I need to change the UMASK. So my answer to your question is that I don't know and I don't know any de facto solutions.

However, if you search on google, you can find many posts where people are looking for similar solutions in other projects.

Generally, regarding umask there are not so many solutions:

  1. pam_umask(8) (systemwide only),
  2. RC, like bashrc (systemwide and user),
  3. umask (CLI).

}

// New creates a new file based storage backend. The directory specified will
// be used as the only storage entry. This method does not check
// whether the path exists, use os.MkdirAll to ensure.
func New(path string) FileStore {
return FileStore{path}
func New(path string, dirPerms uint32, filePerms uint32) FileStore {
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't change the signature of exported functions as this would be a breaking change. Instead, we can keep New unchanged and users can set the DirPerm and FilePerm fields after they called New.

Copy link
Author

Choose a reason for hiding this comment

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

I corrected the code, please take a look.

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.

2 participants