Fix permissions for resolv.conf and hosts #3708
Conversation
|
Note: we should yank out every and all filesystem direct access away. The Store interface did a lot of that already, and did solve atomicity and concurrency, but this is yet another proof that letting people use the filesystem directly is almost always a mistake and we should provide safer abstractions instead. |
|
Looks like @AkihiroSuda can you restart Thanks. |
| @@ -114,6 +114,9 @@ func (x *hostsStore) Acquire(meta Meta) (err error) { | |||
| if err = os.WriteFile(loc, []byte{}, 0o644); err != nil { | |||
| return errors.Join(store.ErrSystemFailure, err) | |||
| } | |||
| if err = os.Chmod(loc, 0o644); err != nil { | |||
There was a problem hiding this comment.
Could you add a comment to explain why 0o644 in the arg of WriteFile doesn't take an expected effect?
| @@ -175,6 +178,9 @@ func (x *hostsStore) AllocHostsFile(id string, content []byte) (location string, | |||
| if err != nil { | |||
| err = errors.Join(store.ErrSystemFailure, err) | |||
| } | |||
| if err = os.Chmod(loc, 0o644); err != nil { | |||
| return nil, err | ||
| } | ||
|
|
||
| return &File{Content: content.Bytes(), Hash: hash}, os.Chmod(path, 0o644) |
apostasie
force-pushed
the
fix-permission
branch
from
f02707c to
750d400
Compare
pkg/dnsutil/hostsstore/hostsstore.go
Outdated
| @@ -114,6 +114,10 @@ func (x *hostsStore) Acquire(meta Meta) (err error) { | |||
| if err = os.WriteFile(loc, []byte{}, 0o644); err != nil { | |||
| return errors.Join(store.ErrSystemFailure, err) | |||
| } | |||
| // See https://www.man7.org/linux/man-pages/man2/open.2.html | |||
There was a problem hiding this comment.
This doesn't explain how the umask rule differs across Go's os.WriteFile and os.Chmod
There was a problem hiding this comment.
No chmod implementation (system or language) would ever change the provided mode to account for umask - that would not make sense.
As for WriteFile, it calls syscall.Open underneath - and Open behavior wrt umask is covered in the man page.
Anyhow, changed the comment.
WriteFile uses syscall.Open, so permissions are modified by umask, if set. For people using agressive umasks (0077), /etc/resolv.conf will end-up unreadable for non root processes. See containerd#3704 Signed-off-by: apostasie <[email protected]>
apostasie
force-pushed
the
fix-permission
branch
from
750d400 to
442b01d
Compare
WriteFile permissions are before umask is applied. For people using aggressive umasks (0077), /etc/resolv.conf will end-up unreadable for non root processes.
Fix #3704