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

libctr/init_linux: reorder chdir #2685

Closed
wants to merge 2 commits into from

Conversation

haircommander
Copy link
Contributor

@haircommander haircommander commented Nov 19, 2020

commit 5e0e67d moved the chdir to be one of the
first steps of finalizing the namespace of the container.

However, this causes issues when the cwd is not accessible by the user running runc, but rather
as the container user.

Thus, setupUser has to happen before we call chdir. setupUser still happens before setting the caps,
so the user should be privileged enough to mitigate the issues fixed in 5e0e67d (I've tested it on my machine, so I believe it does not regress)

Signed-off-by: Peter Hunt pehunt@redhat.com

@thaJeztah
Copy link
Member

I see there was some discussion around the original change in #2086 (comment) as well

@cyphar @justincormack @crosbymichael ptal

@haircommander
Copy link
Contributor Author

Warning: apt-key output should not be parsed (stdout is not a terminal)

https://download.opensuse.org/repositories/devel:/kubic:/libcontainers:/stable/Debian_Unstable/Release.key:

2020-11-19 18:37:41 ERROR 404: Not Found.

gpg: no valid OpenPGP data found.

The command '/bin/sh -c echo 'deb http://download.opensuse.org/repositories/devel:/kubic:/libcontainers:/stable/Debian_Unstable/ /' > /etc/apt/sources.list.d/devel:kubic:libcontainers:stable.list     && wget -nv https://download.opensuse.org/repositories/devel:kubic:libcontainers:stable/Debian_Unstable/Release.key -O- | sudo apt-key add -     && apt-get update     && apt-get install -y --no-install-recommends skopeo     && rm -rf /etc/apt/sources.list.d/devel:kubic:libcontainers:stable.list     && apt-get clean     && rm -rf /var/cache/apt /var/lib/apt/lists/*;' returned a non-zero code: 2

this doesn't seem related. can someon retrigger the tests?

also, PTAL @kolyshkin @mrunalp @AkihiroSuda

@thaJeztah
Copy link
Member

been running into that as well; /cc @cyphar

@kolyshkin
Copy link
Contributor

Proposed CI fix is in #2686. Once merged, you'll need to rebase to fix CI.

@haircommander
Copy link
Contributor Author

rebased, thanks @kolyshkin

libcontainer/init_linux.go Outdated Show resolved Hide resolved
mrunalp
mrunalp previously approved these changes Nov 20, 2020
@kolyshkin
Copy link
Contributor

@haircommander this seems like a regression. Can we have a test case?

@haircommander
Copy link
Contributor Author

I am having trouble writing a test (I personally have not created a situation where this bug comes up, only had it come up in someone else's node in a pretty specific scenario). I'll keep trying, but ideally we could go forward with this without it for now

@haircommander
Copy link
Contributor Author

ok the test is written--it's not pretty but it exists. PTAL @kolyshkin

@kolyshkin
Copy link
Contributor

CI is failing on rootless tests

....
ok 21 runc create --pid-file with new CWD
[sudo] password for rootless: 
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Maybe sudo needs -n?

@haircommander
Copy link
Contributor Author

commit 5e0e67d moved the chdir to be one of the
first steps of finalizing the namespace of the container.

However, this causes issues when the cwd is not accessible by the user running runc, but rather
as the container user.

Thus, setupUser has to happen before we call chdir. setupUser still happens before setting the caps,
so the user should be privileged enough to mitigate the issues fixed in 5e0e67d

Signed-off-by: Peter Hunt <pehunt@redhat.com>
Signed-off-by: Peter Hunt <pehunt@redhat.com>
@kolyshkin
Copy link
Contributor

CI went south :(

GitHub Actions has encountered an internal error when running your job.

# This test must be particular with how it's run. The user that runs it must have the privileges
# to chown a directory it creates away from itself, but not have CAP_DAC_OVERRIDE to override
# the fact it was chowned away. However, it must also be able to clean up.
# Thus, this test must be skipped if the UID running the test is root, or the user doesn't have sudo privileges.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't quite get it. Can this test be run as root, and then drop the privs to a non-root user when it needs to?

Asking because the way it is right now, our CI never runs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately, I wasn't able to figure out how to run the test as a non privileged user

@kolyshkin
Copy link
Contributor

Also related: containerd/containerd#4669

@kolyshkin
Copy link
Contributor

Carrying this over in #2712

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.

None yet

4 participants