Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

kubelet: set terminationMessagePath perms to 0660 #108076

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
Loading
from
Open

kubelet: set terminationMessagePath perms to 0660 #108076

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6619c9
kubelet: set terminationMessagePath perms to 0660
skrobul Jan 18, 2022
b6beb97
node runtime test: use non-root group
skrobul Apr 4, 2023
fd5a394
kuberuntime: change ownership of the termination-log file
skrobul Apr 4, 2023
9cbab5b
kuberuntime: add nil check for SecurityContext
skrobul Apr 28, 2023
c4a75ea
kuberuntime: linter updates
skrobul Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions 29 pkg/kubelet/kuberuntime/kuberuntime_container.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ func (m *kubeGenericRuntimeManager) generateContainerConfig(ctx context.Context,
Annotations: newContainerAnnotations(container, pod, restartCount, opts),
Devices: makeDevices(opts),
CDIDevices: makeCDIDevices(opts),
Mounts: m.makeMounts(opts, container),
Mounts: m.makeMounts(opts, container, pod),
LogPath: containerLogsPath,
Stdin: container.Stdin,
StdinOnce: container.StdinOnce,
Expand Down Expand Up @@ -405,7 +405,7 @@ func makeCDIDevices(opts *kubecontainer.RunContainerOptions) []*runtimeapi.CDIDe
}

// makeMounts generates container volume mounts for kubelet runtime v1.
func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerOptions, container *v1.Container) []*runtimeapi.Mount {
func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerOptions, container *v1.Container, pod *v1.Pod) []*runtimeapi.Mount {
volumeMounts := []*runtimeapi.Mount{}

for idx := range opts.Mounts {
Expand Down Expand Up @@ -441,10 +441,33 @@ func (m *kubeGenericRuntimeManager) makeMounts(opts *kubecontainer.RunContainerO
// open(2) to create the file, so the final mode used is "mode &
// ~umask". But we want to make sure the specified mode is used
// in the file no matter what the umask is.
if err := m.osInterface.Chmod(containerLogPath, 0666); err != nil {
if err := m.osInterface.Chmod(containerLogPath, 0660); err != nil {
Copy link
Member

@rata rata Dec 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR mentions the terminationMessagePath that is defined a few lines below: https://github.com/kubernetes/kubernetes/pull/108076/files#diff-d86ffc24d8e4c9deafd3e39e8f4ee3546ee46a31dcea8ea6526443b3f4bffa28R420

How is it that changing the containerLogPath affects the terminationMessagePath?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so my goal is to get rid of the world writeable file regardless of how is it called. The error message on line 406 suggests that termination-log file is the same as containerLogPath - is it not?

The terminationMessagePath as I understand it is an absolute path that is used when mounting the volume and
it has not much to do with what is known as the concept of termination message despite the name. In fact, even the error message on 415 refers to containerLogPath rather than terminationMessagePath.

At this point I am bit confused about this code - if that's not the line responsible for creating the empty world writable files like this:

# ls -la /var/lib/kubelet/pods/fa852268-2261-4ba4-9aaf-a4eaa98d1dfe/containers/kube-proxy/a62b249a
-rw-rw-rw- 1 root root 0 Mar 20 12:30 /var/lib/kubelet/pods/fa852268-2261-4ba4-9aaf-a4eaa98d1dfe/containers/kube-proxy/a62b249a

then what is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might very well be the same, I'm unsure. But some explanation on the commit msg would be helpful so it is easier to review, you probably figured that out when writing the PR :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh, I see it now. It seems a bind mount is created a few lines below where the source is the host path (containerLogPath) and the destination is the terminationMessagePath.

See here: https://github.com/kubernetes/kubernetes/pull/108076/files#diff-d86ffc24d8e4c9deafd3e39e8f4ee3546ee46a31dcea8ea6526443b3f4bffa28L422-L426

So this is solved :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I didn't want to make any statements I was not sure about - initially I also had thought that containerLogPath referred to a log with whatever is printed out to container's standard output, but that doesn't seem to be the case.

Either way, big thank you for the review and looking into this. Apologies it took me so long to respond, I have missed the notification email.

utilruntime.HandleError(fmt.Errorf("unable to set termination-log file permissions %q: %v", containerLogPath, err))
}

var containerUid, containerGid int
if container.SecurityContext != nil && container.SecurityContext.RunAsUser != nil {
containerUid = int(*container.SecurityContext.RunAsUser)
} else if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.RunAsUser != nil {
containerUid = int(*pod.Spec.SecurityContext.RunAsUser)
} else {
containerUid = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also consider SecurityContext.RunAsNonRoot here and below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I understand the RunAsNonRoot does not influence the UID selection, it merely enables a validation that it's non-root, is that not the case?

Copy link
Contributor

@bart0sh bart0sh Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true, but this change would set UID to 0 (root) even if RunAsNonRoot is true. It's may be ok, but looked suspicious to me. What would happen if container runs as non-root, but containerLogPath owner is root?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The container would not run at all if the RunAsNonRoot was set to true, but the pod.Spec.SecurityContext.RunAsUser was set unset. In other words, as far as I understand there is no way for the container to start as non-root without changing the RunAsUser either on container or pod level. What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about it. I thought that RunAsNonRoot and RunAsUser are independent options. By default user UID and/or username is taken from the image.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the code I'm talking about: https://github.com/kubernetes/kubernetes/blob/release-1.29/pkg/kubelet/kuberuntime/kuberuntime_container.go#L326

}

if container.SecurityContext != nil && container.SecurityContext.RunAsGroup != nil {
containerGid = int(*container.SecurityContext.RunAsGroup)
} else if pod.Spec.SecurityContext != nil && pod.Spec.SecurityContext.RunAsGroup != nil {
containerGid = int(*pod.Spec.SecurityContext.RunAsGroup)
} else {
containerGid = 0
}

if goruntime.GOOS != "windows" {
if err := os.Chown(containerLogPath, containerUid, containerGid); err != nil {
utilruntime.HandleError(fmt.Errorf("unable to chown termination-log file: %q: %v", containerLogPath, err))
}
}

// Volume Mounts fail on Windows if it is not of the form C:/
containerLogPath = volumeutil.MakeAbsolutePath(goruntime.GOOS, containerLogPath)
terminationMessagePath := volumeutil.MakeAbsolutePath(goruntime.GOOS, container.TerminationMessagePath)
Expand Down
2 changes: 1 addition & 1 deletion 2 pkg/kubelet/kuberuntime/kuberuntime_container_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func makeExpectedConfig(m *kubeGenericRuntimeManager, pod *v1.Pod, containerInde
Labels: newContainerLabels(container, pod),
Annotations: newContainerAnnotations(container, pod, restartCount, opts),
Devices: makeDevices(opts),
Mounts: m.makeMounts(opts, container),
Mounts: m.makeMounts(opts, container, pod),
LogPath: containerLogsPath,
Stdin: container.Stdin,
StdinOnce: container.StdinOnce,
Expand Down
2 changes: 2 additions & 0 deletions 2 test/e2e/common/node/runtime.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ while true; do sleep 1; done
ginkgo.Context("on terminated container", func() {
rootUser := int64(0)
nonRootUser := int64(10000)
nonRootGroup := int64(10000)
adminUserName := "ContainerAdministrator"
nonAdminUserName := "ContainerUser"

Expand Down Expand Up @@ -204,6 +205,7 @@ while true; do sleep 1; done
container.SecurityContext.WindowsOptions = &v1.WindowsSecurityContextOptions{RunAsUserName: &nonAdminUserName}
} else {
container.SecurityContext.RunAsUser = &nonRootUser
container.SecurityContext.RunAsGroup = &nonRootGroup
}
matchTerminationMessage(ctx, container, v1.PodSucceeded, gomega.Equal("DONE"))
})
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.