-
Notifications
You must be signed in to change notification settings - Fork 488
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,4 +6,8 @@ set -o pipefail | |
|
||
. /usr/local/share/load-env.sh | ||
|
||
if [ -n "$UMASK" ]; then | ||
umask "$UMASK" | ||
fi | ||
|
||
exec tusd "$@" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,29 +18,30 @@ import ( | |
"errors" | ||
"fmt" | ||
"io" | ||
"io/fs" | ||
"os" | ||
"path/filepath" | ||
|
||
"github.com/tus/tusd/v2/internal/uid" | ||
"github.com/tus/tusd/v2/pkg/handler" | ||
) | ||
|
||
var defaultFilePerm = os.FileMode(0664) | ||
var defaultDirectoryPerm = os.FileMode(0754) | ||
|
||
// See the handler.DataStore interface for documentation about the different | ||
// methods. | ||
type FileStore struct { | ||
// Relative or absolute path to store files in. FileStore does not check | ||
// whether the path exists, use os.MkdirAll in this case on your own. | ||
Path string | ||
|
||
DirPerm fs.FileMode | ||
FilePerm fs.FileMode | ||
} | ||
|
||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I corrected the code, please take a look. |
||
return FileStore{path, os.FileMode(dirPerms), os.FileMode(filePerms)} | ||
} | ||
|
||
// UseIn sets this store as the core data store in the passed composer and adds | ||
|
@@ -73,14 +74,16 @@ func (store FileStore) NewUpload(ctx context.Context, info handler.FileInfo) (ha | |
} | ||
|
||
// Create binary file with no content | ||
if err := createFile(binPath, nil); err != nil { | ||
if err := createFile(binPath, store.DirPerm, store.FilePerm, nil); err != nil { | ||
return nil, err | ||
} | ||
|
||
upload := &fileUpload{ | ||
info: info, | ||
infoPath: infoPath, | ||
binPath: binPath, | ||
dirPerm: store.DirPerm, | ||
filePerm: store.FilePerm, | ||
} | ||
|
||
// writeInfo creates the file by itself if necessary | ||
|
@@ -133,6 +136,8 @@ func (store FileStore) GetUpload(ctx context.Context, id string) (handler.Upload | |
info: info, | ||
binPath: binPath, | ||
infoPath: infoPath, | ||
dirPerm: store.DirPerm, | ||
filePerm: store.FilePerm, | ||
}, nil | ||
} | ||
|
||
|
@@ -166,14 +171,17 @@ type fileUpload struct { | |
infoPath string | ||
// binPath is the path to the binary file (which has no extension) | ||
binPath string | ||
|
||
dirPerm fs.FileMode | ||
filePerm fs.FileMode | ||
} | ||
|
||
func (upload *fileUpload) GetInfo(ctx context.Context) (handler.FileInfo, error) { | ||
return upload.info, nil | ||
} | ||
|
||
func (upload *fileUpload) WriteChunk(ctx context.Context, offset int64, src io.Reader) (int64, error) { | ||
file, err := os.OpenFile(upload.binPath, os.O_WRONLY|os.O_APPEND, defaultFilePerm) | ||
file, err := os.OpenFile(upload.binPath, os.O_WRONLY|os.O_APPEND, upload.filePerm) | ||
if err != nil { | ||
return 0, err | ||
} | ||
|
@@ -212,7 +220,7 @@ func (upload *fileUpload) Terminate(ctx context.Context) error { | |
} | ||
|
||
func (upload *fileUpload) ConcatUploads(ctx context.Context, uploads []handler.Upload) (err error) { | ||
file, err := os.OpenFile(upload.binPath, os.O_WRONLY|os.O_APPEND, defaultFilePerm) | ||
file, err := os.OpenFile(upload.binPath, os.O_WRONLY|os.O_APPEND, upload.filePerm) | ||
if err != nil { | ||
return err | ||
} | ||
|
@@ -253,7 +261,7 @@ func (upload *fileUpload) writeInfo() error { | |
if err != nil { | ||
return err | ||
} | ||
return createFile(upload.infoPath, data) | ||
return createFile(upload.infoPath, upload.dirPerm, upload.filePerm, data) | ||
} | ||
|
||
func (upload *fileUpload) FinishUpload(ctx context.Context) error { | ||
|
@@ -262,19 +270,19 @@ func (upload *fileUpload) FinishUpload(ctx context.Context) error { | |
|
||
// createFile creates the file with the content. If the corresponding directory does not exist, | ||
// it is created. If the file already exists, its content is removed. | ||
func createFile(path string, content []byte) error { | ||
file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, defaultFilePerm) | ||
func createFile(path string, dirPerm fs.FileMode, filePerm fs.FileMode, content []byte) error { | ||
file, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, filePerm) | ||
if err != nil { | ||
if os.IsNotExist(err) { | ||
// An upload ID containing slashes is mapped onto different directories on disk, | ||
// for example, `myproject/uploadA` should be put into a folder called `myproject`. | ||
// If we get an error indicating that a directory is missing, we try to create it. | ||
if err := os.MkdirAll(filepath.Dir(path), defaultDirectoryPerm); err != nil { | ||
if err := os.MkdirAll(filepath.Dir(path), dirPerm); err != nil { | ||
return fmt.Errorf("failed to create directory for %s: %s", path, err) | ||
} | ||
|
||
// Try creating the file again. | ||
file, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, defaultFilePerm) | ||
file, err = os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, filePerm) | ||
if err != nil { | ||
// If that still doesn't work, error out. | ||
return err | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
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 ;)
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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: