[release/1.7] shim: Create pid-file and address with 0644 permissions #9548
Conversation
|
Hi @Dzejrou. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Dzejrou
force-pushed
the
1.7_fix_ignoring_umask
branch
from
f900226
to
088d5cf
Compare
|
Can we fix this on |
I'm not sure, one half of the patch (WritePidFile) works on main, the second half (WriteAddress) does not as that functionality has been moved to bootstrap.json (the address file no longer exists as far as I know). I can open a PR for main, but a backport to 1.7 would still be needed instead of cherry-picking. |
fuweid
changed the title
Makes sense. Can you do the main PR first, and then we'll get this one in? For WritePidFile you can do a normal cherry-pick ( |
Opened #9571 for pid-file, but as far as I can see no action w.r.t. the address file is needed for main (it no longer exists and its former contents are now stored in boostrap.json). Did you perhaps mean you'd like me to remove modifications to WritePidFile from this PR and leave only the WriteAddress change so that the WritePidFile change can be taken from main? If so, let me know and I'll do that. |
Fixes ae70213 In ae70213 the WritePidFile and WriteAddress functions were changed to use AtomicFile instead of os.CreateFile. However, AtomicFile creates a temporary file and then changes its permissions with os.Chmod which alters the previously observed behavior of os.CreateFile which takes the system's umask into account. This means that on Linux-based systems these files suddenly became world writable (containerd#9363). The address file has since been removed, but pid-file was still created as world writable. This commit explicitly requests 0644 permissions as even on systems without default umask of 0022 there is no reason to have these two files world writable. Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com> (cherry picked from commit 9d32841) Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
Fixes ae70213 In ae70213 the WritePidFile and WriteAddress functions were changed to use AtomicFile instead of os.CreateFile. However, AtomicFile creates a temporary file and then changes its permissions with os.Chmod which alters the previously observed behavior of os.CreateFile which takes the system's umask into account. This means that on Linux-based systems these files suddenly became world writable (containerd#9363). Signed-off-by: Jaroslav Jindrak <dzejrou@gmail.com>
Dzejrou
force-pushed
the
1.7_fix_ignoring_umask
branch
from
088d5cf
to
8d82242
Compare
Fixes ae70213
In ae70213 the WritePidFile and WriteAddress functions were changed to use AtomicFile instead of os.CreateFile. However, AtomicFile creates a temporary file and then changes its permissions with os.Chmod which alters the previously observed behavior of os.CreateFile which takes the system's umask into account.
This means that on Linux-based systems these files suddenly became world writable (#9363). This commit explicitly requests 0644 permissions as even on systems without default umask of 0022 there is no reason to have these two files world writable.
This should be affecting only 1.6 (introduced in 1.6.22) and 1.7 as 2.0+ uses boostrap.json to store the address file (not sure about the pid file, but WritePidFile also uses 0666 in release/2.0 and master) and 1.5 does not have ae70213. We're using 1.7.7 and the bug was reported to me against the address file, so I opened a PR for release/1.7. If you'd like a PR for release/1.6 (or even release/2.0 and main) let me know.
Note: I do not know how this (0666 -> 0644) will affect other systems such as Windows or macOS, but given that prior to ae70213 both of these files effectively had the 0644 permissions on Linux-based systems without any issues, I thought that explicitly requesting them wouldn't cause problems.