Skip to content

Add option to control ownership of containerd socket#11786

Open
viccie30 wants to merge 2 commits into
containerd:mainfrom
viccie30:socket-ownership
Open

Add option to control ownership of containerd socket#11786
viccie30 wants to merge 2 commits into
containerd:mainfrom
viccie30:socket-ownership

Conversation

@viccie30

@viccie30 viccie30 commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Adds support for setting the ownership (for Unix) or access rights (for Windows) for containerd's sockets using user and group names. Also adds a --group flag to containerd, similar to buildkitd.

Would close #10454.

ACL for Windows named pipe

The named pipe is accessible to Administrators and Local System, in addition to a user or group specified by the configuration or on the command line. This matches what buildkit does and also Windows' default ACL for named pipes. Windows' default also adds read access for all authenticated and anonymous users (see below), but I didn't think that'd be appropriate here.

Discovering default ACL

I got the default ACL for named pipes using the following Go program:

package main

import (
	"fmt"

	"golang.org/x/sys/windows"
)

func main() {
	var err error
	var acl *windows.ACL
	var sd *windows.SECURITY_DESCRIPTOR

	if err = windows.RtlDefaultNpAcl(&acl); err != nil {
		fmt.Errorf("RtlDefaultNpAcl failed: %w", err)
		return
	}

	if sd, err = windows.NewSecurityDescriptor(); err != nil {
		fmt.Errorf("NewSecurityDescriptor failed: %w", err)
		return
	}

	if err = sd.SetDACL(acl, true, true); err != nil {
		fmt.Errorf("SetDACL failed: %w", err)
		return
	}

	fmt.Println(sd)
}

And parsed it using winsddl:

$ python winsddl/sd.py -t namedpipe "D:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;<OWNER>)(A;;GR;;;WD)(A;;GR;;;AN)"
D:(A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;<OWNER>)(A;;GR;;;WD)(A;;GR;;;AN)
    Discretionary ACL: (A;;GA;;;SY)(A;;GA;;;BA)(A;;GA;;;<OWNER>)(A;;GR;;;WD)(A;;GR;;;AN)

        ACE 1: A;;GA;;;SY
            Type: A (ACCESS_ALLOWED_ACE_TYPE)
            Access rights: GA   (0x10000000)
                0x10000000 GENERIC_ALL  (Generic right mapped to every other existing right)
            Trustee: SY (S-1-5-18) (Local System)

        ACE 2: A;;GA;;;BA
            Type: A (ACCESS_ALLOWED_ACE_TYPE)
            Access rights: GA   (0x10000000)
                0x10000000 GENERIC_ALL  (Generic right mapped to every other existing right)
            Trustee: BA (S-1-5-32-544) (Administrators)

        ACE 3: A;;GA;;;<OWNER>
            Type: A (ACCESS_ALLOWED_ACE_TYPE)
            Access rights: GA   (0x10000000)
                0x10000000 GENERIC_ALL  (Generic right mapped to every other existing right)
            Trustee: <OWNER>

        ACE 4: A;;GR;;;WD
            Type: A (ACCESS_ALLOWED_ACE_TYPE)
            Access rights: GR   (0x80000000)
                0x80000000 FILE_GENERIC_READ    (Generic right to read (mapped to FILE_READ_ATTRIBUTES | FILE_READ_DATA | FILE_READ_EA | SYNCHRONIZE | READ_CONTROL))
            Trustee: WD (S-1-1-0) (Everyone) (Authenticated users, Guests, and Anonymous remote users in XP SP1 and earlier, or if EveryoneIncludesAnonymous=1)

        ACE 5: A;;GR;;;AN
            Type: A (ACCESS_ALLOWED_ACE_TYPE)
            Access rights: GR   (0x80000000)
                0x80000000 FILE_GENERIC_READ    (Generic right to read (mapped to FILE_READ_ATTRIBUTES | FILE_READ_DATA | FILE_READ_EA | SYNCHRONIZE | READ_CONTROL))
            Trustee: AN (S-1-5-7) (Anonymous Logon) (Group that includes all users who have logged on anonymously, maintained by the system)

@github-project-automation github-project-automation Bot moved this to Needs Triage in Pull Request Review May 2, 2025
@k8s-ci-robot

k8s-ci-robot commented May 2, 2025

Copy link
Copy Markdown

Hi @viccie30. 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.

Details

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-sigs/prow repository.

@dosubot dosubot Bot added the platform/windows Windows label May 2, 2025
AkihiroSuda
AkihiroSuda requested changes May 2, 2025
View reviewed changes

@AkihiroSuda AkihiroSuda left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is insecure and should not be implemented, at least for Linux.

This matches what buildkit does and also Windows' default ACL for named pipes.

BuildKit is not comparable here, as BuiltKit does not allow privileged operations by default

@github-project-automation github-project-automation Bot moved this from Needs Triage to Needs Update in Pull Request Review May 2, 2025
@viccie30 viccie30 force-pushed the socket-ownership branch from 3e35ead to a2b04af Compare May 2, 2025 09:14
@viccie30

viccie30 commented May 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

This is insecure and should not be implemented, at least for Linux.

What is insecure about it? For Linux the only change is that besides accepting numeric uids and gids, the code now also accepts and parses user and group names?

This matches what buildkit does and also Windows' default ACL for named pipes.

BuildKit is not comparable here, as BuiltKit does not allow privileged operations by default

With this change containerd won't either. Default access is restricted to Administrators and Local System as well, unless explicitly changed by the configuration or command line.

@AkihiroSuda

AkihiroSuda commented May 2, 2025

Copy link
Copy Markdown
Member

What is insecure about it? For Linux the only change is that besides accepting numeric uids and gids, the code now also accepts and parses user and group names?

Sorry, I forgot we have been already supporting uids and gids. This probably should be deprecated though.

@viccie30

viccie30 commented May 2, 2025

Copy link
Copy Markdown
Author

Sorry, I forgot we have been already supporting uids and gids. This probably should be deprecated though.

Can I ask why? Giving selected users access to use containerd without requiring them to have root access otherwise seems useful.

@AkihiroSuda

AkihiroSuda commented May 2, 2025

Copy link
Copy Markdown
Member

Sorry, I forgot we have been already supporting uids and gids. This probably should be deprecated though.

Can I ask why? Giving selected users access to use containerd without requiring them to have root access otherwise seems useful.

Because an access to the socket equates to the root, and some users do not know this fact.

Why not just use sudo explicitly?

@viccie30 viccie30 force-pushed the socket-ownership branch from a2b04af to ce34a1f Compare May 2, 2025 09:49
@viccie30 viccie30 force-pushed the socket-ownership branch from ce34a1f to 257076d Compare May 4, 2025 10:25
@samuelkarp

samuelkarp commented May 27, 2025

Copy link
Copy Markdown
Member

Because an access to the socket equates to the root, and some users do not know this fact.

This is correct. Access to the socket means a user can create a container that has no restrictions, at least on Linux.

@samuelkarp samuelkarp added the status/needs-discussion Needs discussion and decision from maintainers label May 27, 2025
@rabindra-harlalka rabindra-harlalka mentioned this pull request Oct 3, 2025
Open
@apurv15

apurv15 commented Dec 24, 2025

Copy link
Copy Markdown
Contributor

I agree that this PR will add first class support for restricting access to certain users on Windows.
As an alternative, protected prefix paths for namedpipes can be used as is described here - https://www.tiraniddo.dev/2017/11/named-pipe-secure-prefixes.html
But i could not find any official documentation around this.

@k8s-ci-robot

k8s-ci-robot commented Apr 23, 2026

Copy link
Copy Markdown

PR needs rebase.

Details

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-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

needs-ok-to-test needs-rebase platform/windows Windows size/L status/needs-discussion Needs discussion and decision from maintainers

Projects

Pull Request Review
Status: Needs Update

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add option to control group ownership of containerd socket file

5 participants

@viccie30 @k8s-ci-robot @AkihiroSuda @samuelkarp @apurv15