libct/init_linux: reorder chdir to fix EPERM #2712
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Commit 5e0e67d ("fix permission denied") modified some code but did not provide a test case. This is a test case that was tested to fail before and succeed after the above commit. For more details, see opencontainers#2086 Signed-off-by: Kir Kolyshkin <[email protected]>
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 <[email protected]>
The test setup is complicated, as we want to run it as non-root, but we need to create a directory owned by a particular user, and root is needed for chown. To do that, we hijack enable_idmap. Now, there is no ideal place to clean up AUX_DIR. A function cleanup is called after the test to do so. Note that it won't be called if the test fails. This was verified to fail before and pass after the previous commit. Signed-off-by: Kir Kolyshkin <[email protected]>
|
LGTM, thanks for picking this up 👍 |
AkihiroSuda
approved these changes
Dec 19, 2020
mrunalp
approved these changes
Jan 4, 2021
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc-1
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers/runc@5e0e67d was released and when the workaround opencontainers/runc#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 6, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 7, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc-1
that referenced
this pull request
Apr 7, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers/runc@5e0e67d was released and when the workaround opencontainers/runc#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc-1
that referenced
this pull request
Apr 7, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers/runc@5e0e67d was released and when the workaround opencontainers/runc#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
This was referenced Apr 7, 2021
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 15, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
haircommander
added a commit
to haircommander/runc
that referenced
this pull request
Apr 15, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
AdamKorcz
pushed a commit
to AdamKorcz/runc
that referenced
this pull request
May 22, 2021
Alas, the EPERM on chdir saga continues... Unfortunately, the there were two releases between when opencontainers@5e0e67d was released and when the workaround opencontainers#2712 was added. Between this, folks started relying on the ability to have a workdir that the container user doesn't have access to. Since this case was previously valid, we should continue support for it. Now, we retry the chdir: Once at the top of the function (to catch cases where the runc user has access, but container user does not) and once after we setup user (to catch cases where the container user has access, and the runc user does not) Add a test case for this as well. Signed-off-by: Peter Hunt <[email protected]>
This was referenced Jun 24, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a carry of #2685, adding test cases for it, as well for #2086 to make sure there is no regression. Most work was done by @haircommander, I just tinkered with the tests. Original description from Peter follows.
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)
Closes: #2685