[23.0 backport] Do not drop effective&permitted set #46222
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
Switch to using t.TempDir() instead of rolling our own. Clean up mounts leaked by the tests as otherwise the tests fail due to the leaked mounts because unlike the old cleanup code, t.TempDir() cleanup does not ignore errors from os.RemoveAll. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 9ff169c) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Many of the fields in LinuxResources struct are pointers to scalars for some reason, presumably to differentiate between set-to-zero and unset when unmarshaling from JSON, despite zero being outside the acceptable range for the corresponding kernel tunables. When creating the OCI spec for a container, the daemon sets the container's OCI spec CPUShares and BlkioWeight parameters to zero when the corresponding Docker container configuration values are zero, signifying unset, despite the minimum acceptable value for CPUShares being two, and BlkioWeight ten. This has gone unnoticed as runC does not distingiush set-to-zero from unset as it also uses zero internally to represent unset for those fields. However, kata-containers v3.2.0-alpha.3 tries to apply the explicit-zero resource parameters to the container, exactly as instructed, and fails loudly. The OCI runtime-spec is silent on how the runtime should handle the case when those parameters are explicitly set to out-of-range values and kata's behaviour is not unreasonable, so the daemon must therefore be in the wrong. Translate unset values in the Docker container's resources HostConfig to omit the corresponding fields in the container's OCI spec when starting and updating a container in order to maximize compatibility with runtimes. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit dea870f) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Audit the OCI spec options used for Linux containers to ensure they are less order-dependent. Ensure they don't assume that any pointer fields are non-nil and that they don't unintentionally clobber mutations to the spec applied by other options. Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 8a094fe) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Currently moby drops ep sets before the entrypoint is executed. This does mean that with combination of no-new-privileges the file capabilities stops working with non-root containers. This is undesired as the usability of such containers is harmed comparing to running root containers. This commit therefore sets the effective/permitted set in order to allow use of file capabilities or libcap(3)/prctl(2) respectively with combination of no-new-privileges and without respectively. For no-new-privileges the container will be able to obtain capabilities that are requested. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com> Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com> (cherry picked from commit 3aef732) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Verify non-root containers are able to use file capabilities. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com> Co-authored-by: Cory Snider <csnider@mirantis.com> Signed-off-by: Cory Snider <csnider@mirantis.com> (cherry picked from commit 42fa7a1) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah
added
area/runtime
status/2-code-review
impact/changelog
area/daemon
kind/bugfix
corhere
approved these changes
Aug 29, 2023
|
Alright; let's bring this one in 👍 |
This was referenced Feb 4, 2024
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.
--security-opt=no-new-privileges#45491- What I did
daemon: stop setting container resources to zero
Many of the fields in LinuxResources struct are pointers to scalars for some reason, presumably to differentiate between set-to-zero and unset when unmarshaling from JSON, despite zero being outside the acceptable range for the corresponding kernel tunables. When creating the OCI spec for a container, the daemon sets the container's OCI spec CPUShares and BlkioWeight parameters to zero when the corresponding Docker container configuration values are zero, signifying unset, despite the minimum acceptable value for CPUShares being two, and BlkioWeight ten. This has gone unnoticed as runC does not distingiush set-to-zero from unset as it also uses zero internally to represent unset for those fields. However, kata-containers v3.2.0-alpha.3 tries to apply the explicit-zero resource parameters to the container, exactly as instructed, and fails loudly. The OCI runtime-spec is silent on how the runtime should handle the case when those parameters are explicitly set to out-of-range values and kata's behaviour is not unreasonable, so the daemon must therefore be in the wrong.
Translate unset values in the Docker container's resources HostConfig to omit the corresponding fields in the container's OCI spec when starting and updating a container in order to maximize compatibility with runtimes.
Do not drop effective&permitted set
Currently moby drops ep sets before the entrypoint is executed. This does mean that with combination of no-new-privileges the file capabilities stops working with non-root containers. This is undesired as the usability of such containers is harmed comparing to running root containers.
This commit therefore sets the effective/permitted set in order to allow use of file capabilities or libcap(3)/prctl(2) respectively with combination of no-new-privileges and without respectively.
For no-new-privileges the container will be able to obtain capabilities that are requested.
- How I did it
- How to verify it
Use the below as Dockerfile
docker run --security-opt=no-new-privileges --user=100 --cap-add sys_admin <tag of the build Dockerfile> capsh --printYou should see
Current: = cap_sys_admin+eprather thanCurrent:- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)