Setuid as user for OsFs operations #1225
Conversation
Signed-off-by: Peter Verraedt <[email protected]>
|
Hello, thanks for the patch. I initially considered this approach, but was scared about performance penalty. I never did a real test but the linked issue was enough for me to not use this method. I'm not sure |
Signed-off-by: Peter Verraedt <[email protected]>
Signed-off-by: Peter Verraedt <[email protected]>
|
I'll try to test performance differences for some example code. I was hoping that by putting the LockOSThread and UnlockOSThread only in the functions that call out to the file system, and that themselves do not spawn (too much) goroutines, the go executer would not be confused too much. The only real alternative would be to spawn a whole subprocess to handle an incoming connection of a user, but that is nontrivial as it requires some custom messaging passing between the main process an its children. |
this will give even worse performance. If I understand your use case correctly, you need a way to set a different umask for each user. Did I get it right? |
Codecov Report
@@ Coverage Diff @@
## main #1225 +/- ##
==========================================
- Coverage 98.88% 98.88% -0.01%
==========================================
Files 75 75
Lines 25005 24981 -24
==========================================
- Hits 24727 24703 -24
Misses 225 225
Partials 53 53
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
My usecase is a shared filesystem that is also accessible via other methods, and file permissions are therefore kept on the filesystem itself. There are shared folders with setuid bit set, so new files will inherit the group of the folder. There also can be file acls. Al these kind of permissions should be checked, as if gosftp would act as the user. I was testing performance impact, and it seems to be 7-10 times slower (obviously because of the syscalls). But more problematic, go does not behave deterministic when doing |
|
If I put a global lock for the part where Performance impact varies from 7x for non concurrent to 58x for heavily concurrent operations. |
|
I'll invest some time to create a library that mimicks the os.OpenFile/... behaviour as if a user would call it. Redoing the file system permission checks in user space will be more efficient. |
This seems a good idea. Your use case is quite complex, we can't fix it by just adding a umask to SFTPGo users. Please notify me when you will have something usable. Thanks |
This PR proposes to change the behaviour of setting a uid/gid for a user, if sftpgo is run as root. If a user would create or modify a file, we act as the uid/gid to execute the modification, instead of acting as root and chowning the resulting file later. It is implemented in the virtual file system layer by calling syscall.Seteuid and syscall.Setegid before the operations.
The main benefit is that file permissions on the underlying file system will be honoured, as well as the virtual permissions that are checked by sftpgo itself. Only for creating the top user directory, we'll still act as root to ensure its existence.
The only exception is the ScanQuota function for virtual folders that is ran as root if triggered by an event, but that operation only reads files so it should be fine.
In case you would consider to merge this, I'm willing to invest time in implementing secondary groups as well.