fix(cmd/gluetun): directory mode on /tmp/gluetun should be 0755 - #3471
evan314159 wants to merge 1 commit into
Conversation
|
why ? |
|
The original 0644 is missing the execute bit so non-root users cannot change into the directory. 0755 adds the execute bit. |
|
Also, I checked the rest of the code base and this is the only function that creates directories with the wrong permissions. The rest use 0755 or 0751. I renamed the constant to dirPermissions as per ./internal/storage/flush.go to avoid confusion in future. |
|
why do you need the execute bit for all users ? also if that's not done already the directory should be owned by what the user specifies with PUID and PGID |
|
0644 is not a valid directory mode. A 0644 directory can't be traversed even by root, except via DAC_OVERRIDE capability that allows root to bypass permission checks, which is why 0644 appears to work enough with default capabilities, at least until one tries to share the directory with the non-root user. I hit this running with all capabilities dropped. Gluetun also chowns the port mapping files in /tmp/gluetun to PUID:PGID. This further needs DAC_OVERRIDE to update them because with file mode 0644 once they have been chownd to PUID:PGID root no longer has permissions to update them. In the PR I set 0755 on the directories assuming world-read was intended, since the files themselves are created 0644, and that directory mode 0644 was likely not intended. Can you confirm the intent? Two alternatives depending on the answer: If /tmp/gluetun is private by default (admin overrides to share):
If /tmp/gluetun is meant to be shared with the non-root user by default:
Controlling access by PGID is better than by PUID both because it does not depend on DAC_OVERRIDE and because Kubernetes securityContext.fsGroup can be used to fix the volume permissions and assign a secondary GID where no such capability exists for controlling access by PUID. For /gluetun, if it's meant to be private then 0700 would be appropriate. Please let me know which model was intended and I'll revise the PR accordingly. |
qdm12
left a comment
qdm12
left a comment
There was a problem hiding this comment.
Makes sense, my bad for misunderstanding!
|
|
||
| const permission = fs.FileMode(0o644) | ||
| err = os.MkdirAll("/tmp/gluetun", permission) | ||
| const dirPermission = fs.FileMode(0o755) |
|
Actually, ideally, I think we should:
So we have the same permissions for both PUID and PGID. Now to avoid breaking compatibility, let's keep directories to 0o774 and files to 0o664 (if the world permission was |
If /tmp/gluetun does not exist, create with permissions 0774 (instead of 0644). Create files in /tmp/gluetun with permissions 0664 (instead of 0644). If /gluetun does not exist, create with permissions 0755 (instead of 0644).
d89b28e to
fd47a1f
Compare
|
Updated to your specifications. With default umask 022 to get file modes like 664 we need to use chmod. |
| if err != nil { | ||
| return err | ||
| } | ||
| // chmod explicitly since MkdirAll is subject to the umask |
| _, err = os.Stat("/tmp/gluetun") | ||
| switch { | ||
| case errors.Is(err, os.ErrNotExist): | ||
| // TODO v4: remove world permission (0770 or 2770) | ||
| // TODO v4: chown root:PGID for sharing with nonrootuser | ||
| const tmpPermission = fs.FileMode(0o774) | ||
| err = os.MkdirAll("/tmp/gluetun", tmpPermission) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // chmod explicitly since MkdirAll is subject to the umask | ||
| err = os.Chmod("/tmp/gluetun", tmpPermission) | ||
| if err != nil { | ||
| return fmt.Errorf("setting /tmp/gluetun permissions: %w", err) | ||
| } | ||
| case err != nil: | ||
| return fmt.Errorf("checking /tmp/gluetun: %w", err) | ||
| } | ||
| err = os.MkdirAll("/gluetun", permission) | ||
|
|
||
| const gluetunPermission = fs.FileMode(0o755) | ||
| err = os.MkdirAll("/gluetun", gluetunPermission) |
There was a problem hiding this comment.
let's move this to a local createDirectories function, since it's getting pretty big (and _main is already pretty too-big)
| const gluetunPermission = fs.FileMode(0o755) | ||
| err = os.MkdirAll("/gluetun", gluetunPermission) |
There was a problem hiding this comment.
should /gluetun get the same treatment as /tmp/gluetun? (same checks as switch above)
| // TODO v4: set UID to -1 (root:PGID)? | ||
| // files chowned to PUID require DAC_OVERRIDE to update |
There was a problem hiding this comment.
I'm not sure I understand, 0 is for root. But why
files chowned to PUID require DAC_OVERRIDE to update
If you match PUID to your user id? Sorry if I'm missing something!
This branch has not been deployed
2 participants
Type
Please tick which one the following applies to your pull request:
Description
Correct directory mode on /tmp/gluetun to be 0755 (from 0644).
Issue (optional)
Gluetun wiki associated pull request (optional)