Skip to content

shim: Handle apparmor_restrict_unprivileged_userns#134

Merged
dmcgowan merged 4 commits intocontainerd:mainfrom
vvoland:test-shimconn
Mar 24, 2026
Merged

shim: Handle apparmor_restrict_unprivileged_userns#134
dmcgowan merged 4 commits intocontainerd:mainfrom
vvoland:test-shimconn

Conversation

@vvoland
Copy link
Copy Markdown
Contributor

@vvoland vvoland commented Mar 23, 2026

  • Add a test that exercises the full shim connection lifecycle.
  • Fix the test failure by disabling apparmor_restrict_unprivileged_userns
  • Add better handling of apparmor_restrict_unprivileged_userns=1 and return human readable error

Copilot AI review requested due to automatic review settings March 23, 2026 15:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an integration test that verifies the shim’s TTRPC socket is actually reachable after start returns, aiming to reproduce/guard against CI flakes where the returned socket path is not connectable yet.

Changes:

  • Refactors shim startup logic into a startShim helper returning parsed bootstrap params.
  • Adds TestShimConnect that polls for socket readiness, dials the Unix socket, and pings the TTRPC server via internal/ttrpcutil.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread integration/shim_start_test.go
Comment thread integration/shim_start_test.go
Comment thread integration/shim_start_test.go
Copilot AI review requested due to automatic review settings March 23, 2026 16:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot AI review requested due to automatic review settings March 23, 2026 17:15
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/mount_linux.go Outdated
Comment thread internal/shim/manager/manager_unix.go Outdated
Comment thread internal/shim/manager/manager_unix.go Outdated
@vvoland vvoland changed the title integration: Add TestShimConnect to verify TTRPC connection shim/manager: Temporarily disable user namespace cloning on Linux Mar 23, 2026
Copilot AI review requested due to automatic review settings March 23, 2026 17:53
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/mount_linux.go Outdated
Comment thread integration/shim_start_test.go Outdated
Comment thread internal/shim/manager/mount_linux.go Outdated
Comment thread internal/shim/manager/mount_linux.go Outdated
@vvoland vvoland force-pushed the test-shimconn branch 3 times, most recently from 3275cbc to d1a3840 Compare March 24, 2026 13:32
Copilot AI review requested due to automatic review settings March 24, 2026 13:32
@vvoland vvoland changed the title shim/manager: Temporarily disable user namespace cloning on Linux shim: Detect restricted userns and add test for ttrpc connection Mar 24, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread script/userns-check/main.go Outdated
Comment thread script/userns-check/main.go Outdated
Comment thread script/userns-check/main.go
Comment thread script/userns-check/main.go
Comment thread internal/shim/manager/mount_linux.go
Comment thread .github/workflows/ci.yml Outdated
Copilot AI review requested due to automatic review settings March 24, 2026 15:22
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

internal/shim/manager/mount_linux.go:62

  • This PR’s description says the CLONE_NEWUSER|CLONE_NEWNS logic in cloneMntNs is being commented out/disabled, but this implementation still unconditionally enables CLONE_NEWUSER|CLONE_NEWNS (and now errors if AppArmor restricts it). Please reconcile the PR description with the actual behavior, or adjust the implementation if the intent really is to disable userns cloning temporarily.
func cloneMntNs(cmd *exec.Cmd) error {
	if restricted, err := apparmorRestrictsUserns(); err != nil {
		return fmt.Errorf("checking apparmor userns restriction: %w", err)
	} else if restricted {
		return fmt.Errorf("kernel.apparmor_restrict_unprivileged_userns=1 prevents creating user namespaces; either disable this sysctl or configure an AppArmor profile that allows userns creation for the containerd process")
	}

	uid := os.Getuid()
	gid := os.Getgid()
	cmd.SysProcAttr.Cloneflags |= syscall.CLONE_NEWUSER | syscall.CLONE_NEWNS
	cmd.SysProcAttr.UidMappings = []syscall.SysProcIDMap{

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread script/userns-check/main.go
Comment thread script/userns-check/main.go
Comment thread script/userns-check/main.go
Ubuntu 24+ enables kernel.apparmor_restrict_unprivileged_userns=1 by
default, which cripples the user namespaces.

Set it to 0 before running integration tests.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Copilot AI review requested due to automatic review settings March 24, 2026 16:22
@vvoland vvoland marked this pull request as ready for review March 24, 2026 16:26
@vvoland vvoland changed the title shim: Detect restricted userns and add test for ttrpc connection shim: Handle apparmor_restrict_unprivileged_userns Mar 24, 2026
@vvoland vvoland changed the title shim: Handle apparmor_restrict_unprivileged_userns shim: Handle apparmor_restrict_unprivileged_userns Mar 24, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/shim/manager/mount_linux.go
Comment thread internal/shim/manager/mount_linux.go
Comment thread .github/workflows/ci.yml
vvoland added 3 commits March 24, 2026 17:29
Check the kernel.apparmor_restrict_unprivileged_userns sysctl before
attempting to create user namespaces.

When set to 1, AppArmor cripples unprivileged user namespaces, causing
the shim to fail when accessing the parent socket. Return an actionable
error message instead.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
…m start

Add a test that exercises the full shim connection lifecycle: start the
shim binary, parse the returned socket address, then dial and ping the
TTRPC server. This reproduces the "failed to create TTRPC connection:
dial unix …: connect: no such file or directory" error seen in
docker-next CI when the shim socket is unreachable after Start returns.

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
Build and run cmd/repro to verify that user namespaces are not
restricted before running integration tests. This catches environment
issues (e.g. apparmor_restrict_unprivileged_userns still enabled)

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
@dmcgowan dmcgowan merged commit f8710d7 into containerd:main Mar 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants