quadlet: do not reject RemapUsers=keep-id as root #24306
Conversation
This seems to be a testing gap, we need to test both for full coverage. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
This is simply wrong, as of commit de63ad7 --userns=keep-id is also allowed as root. Signed-off-by: Paul Holzinger <pholzing@redhat.com>
openshift-ci
bot
added
release-note-none
approved
|
/lgtm |
| var args []string | ||
| if isRootless() { | ||
| args = append(args, "--user") | ||
| } | ||
| args = append(args, "--no-kmsg-log", generatedDir) |
There was a problem hiding this comment.
style question: why not args := []string{"--no-kmsg-log", generatedDir} (initialize first) followed by prepending --user if rootless?
There was a problem hiding this comment.
because generatedDir is a argument and quadlet does not seem to accept options/flag after the args
$ bin/quadlet --dryrun --no-kmsg-log --user /tmp
quadlet-generator[162564]: Loading source unit file /home/pholzing/.config/containers/systemd/t1.container
...
$ bin/quadlet --dryrun --no-kmsg-log /tmp --user
quadlet-generator[162630]: No files parsed from [/run/containers/systemd /etc/containers/systemd /usr/share/containers/systemd]
The --user is ignored but I haven't looked into why, the systemd generator has a forced syntax defined by systemd so I just accepted it without thinking to long about it.
There was a problem hiding this comment.
FWIW podman also has assumptions about the order between options and arguments - all options must come first, then the first argument being the image and everything after it goes into the command. So, maybe it's a cobra thing.
Having said that, as @edsantiago wrote, the --user parameter can be prepended. Not that critical I guess
There was a problem hiding this comment.
FWIW podman also has assumptions about the order between options and arguments - all options must come first, then the first argument being the image and everything after it goes into the command. So, maybe it's a cobra thing.
quadlet does not use cobra, it used the std flag package. In cobra you control that via flags.SetInterspersed(true/false) and yes a command like podman run $IMAGE treats all options after $IMAGE as args to the container command but in other commands it is fine to mix them.
There was a problem hiding this comment.
Oh, I didn't remember that. I guess the different package is the reason.
Also, AFAIK systemd does not pass any options only parameters. Quadlet uses argv[0] to set isUser (though it should do that using an environment variable). So, the options are only for debugging and testing
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 740f1d1
into
containers:main
There was a problem hiding this comment.
LGTM
Input validation can really hurt. I ran into one in Quay (see https://issues.redhat.com/browse/PROJQUAY-8115). Maybe we can audit the code some time soon and look for similar cases.
This is simply wrong, as of commit de63ad7 --userns=keep-id is also
allowed as root.
Also enable quadlet test to run as root/rootless
Does this PR introduce a user-facing change?