specconv: temporarily allow userns path and mapping if they match #4134
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
rata
reviewed
Dec 8, 2023
It turns out that the error added in commit 09822c3 ("configs: disallow ambiguous userns and timens configurations") causes issues with containerd and CRIO because they pass both userns mappings and a userns path. These configurations are broken, but to avoid the regression in this one case, output a warning to tell the user that the configuration is incorrect but we will continue to use it if and only if the configured mappings are identical to the mappings of the provided namespace. Fixes: 09822c3 ("configs: disallow ambiguous userns and timens configurations") Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
lifubang
reviewed
Dec 10, 2023
rata
approved these changes
Dec 11, 2023
lifubang
added
the
backport/todo/1.1
AkihiroSuda
reviewed
Dec 13, 2023
| // want to produce broken behaviour if the mapping doesn't match | ||
| // the userns. So (for now) we output a warning if the actual | ||
| // userns mappings match the configuration, otherwise we return an | ||
| // error. |
There was a problem hiding this comment.
Could you add links to the containerd issue and the crio issue tickets?
There was a problem hiding this comment.
AFAIK there are no bug reports for crio and containerd. @rata was right that they were forced to do this, and I suspect that it will be somewhat difficult for them to change this because they might want to work with older runc versions.
AkihiroSuda
reviewed
Dec 13, 2023
| // IsSameMapping returns whether or not the two id mappings are the same. Note | ||
| // that if the order of the mappings is different, or a mapping has been split, | ||
| // the mappings will be considered different. | ||
| func IsSameMapping(a, b []configs.IDMap) bool { |
There was a problem hiding this comment.
Can we just use reflect.DeepEqual ?
There was a problem hiding this comment.
Reflection is slower and importing reflect IIRC means the compiler can't do certain optimisations. We do use reflect elsewhere in runc, but it seems best to not add more uses that aren't actually necessary.
Once we switch to Go 1.21, we can use slices.Equal. I would prefer that over either option.
AkihiroSuda
reviewed
Dec 13, 2023
AkihiroSuda
approved these changes
Dec 13, 2023
This was referenced Dec 14, 2023
Using ints for all of our mapping structures means that a 32-bit binary errors out when trying to parse /proc/self/*id_map: failed to cache mappings for userns: failed to parse uid_map of userns /proc/1/ns/user: parsing id map failed: invalid format in line " 0 0 4294967295": integer overflow on token 4294967295 This issue was unearthed by commit 1912d59 ("*: actually support joining a userns with a new container") but the underlying issue has been present since the docker/libcontainer days. In theory, switching to uint32 (to match the spec) instead of int64 would also work, but keeping everything signed seems much less error-prone. It's also important to note that a mapping might be too large for an int on 32-bit, so we detect this during the mapping. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
kolyshkin
added
backport/done/1.1
|
1.1 backport: #4144 |
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.
It turns out that the error added in commit 09822c3 ("configs: disallow ambiguous userns and timens configurations") causes issues with containerd and CRIO because they pass both userns mappings and a userns path.
These configurations are broken, but to avoid the regression in this one case, output a warning to tell the user that the configuration is incorrect but we will continue to use it if and only if the configured mappings are identical to the mappings of the provided namespace.
/cc @rata
Fixes: 09822c3 ("configs: disallow ambiguous userns and timens configurations")
Fixes #4133
Signed-off-by: Aleksa Sarai cyphar@cyphar.com