Skip to content
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

shim: Create pid-file with 0644 permissions #9571

Merged
merged 1 commit into from
Dec 25, 2023

Conversation

Dzejrou
Copy link
Contributor

@Dzejrou Dzejrou commented Dec 21, 2023

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). 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.

(cherry picked from commit 088d5cf)

@k8s-ci-robot
Copy link

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@AkihiroSuda
Copy link
Member

cherry picked from commit 088d5cf

This is confusing, as the commit does not exist in the upstream

@thaJeztah thaJeztah added ok-to-test cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch and removed needs-ok-to-test labels Dec 23, 2023
@thaJeztah
Copy link
Member

original PR was backported to 1.6 as well, so we'll also need a cherry pick for the 1.6 branch

@Dzejrou
Copy link
Contributor Author

Dzejrou commented Dec 23, 2023

cherry picked from commit 088d5cf

This is confusing, as the commit does not exist in the upstream

This is a result of me following Samuel's recommendation at #9548 (comment), would you like me to amend the message to leave this bit out? (I'm sorry about all this confusion, I'm not used to contribute to multiple maintained branches.)

@AkihiroSuda
Copy link
Member

cherry picked from commit 088d5cf

This is confusing, as the commit does not exist in the upstream

This is a result of me following Samuel's recommendation at #9548 (comment), would you like me to amend the message to leave this bit out? (I'm sorry about all this confusion, I'm not used to contribute to multiple maintained branches.)

The main PR has to be merged first, and then the commit should be cherry-picked to the v1.7 branch.
The current PR is cherry-picked in the opposite way.

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>
@Dzejrou
Copy link
Contributor Author

Dzejrou commented Dec 23, 2023

The main PR has to be merged first, and then the commit should be cherry-picked to the v1.7 branch. The current PR is cherry-picked in the opposite way.

Thank you for the clarification, hopefully I did it correctly this time:

main PR: Commit that fixes pid-file.
release/1.7 PR: Cherry-pick of the commit from main + a commit that fixes the address file.

@AkihiroSuda AkihiroSuda added this pull request to the merge queue Dec 25, 2023
Merged via the queue into containerd:main with commit b7b808c Dec 25, 2023
45 checks passed
@thaJeztah thaJeztah added cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch and removed cherry-pick/1.6.x Change to be cherry picked to release/1.6 branch labels Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-picked/1.6.x PR commits are cherry-picked into release/1.6 branch cherry-picked/1.7.x PR commits are cherry-picked into release/1.7 branch ok-to-test size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants