switch podman image scp from depending on machinectl to just os/exec #12867

cdoern · 2022-01-14T18:01:13Z

switch podman image scp from depending on machinectl to just os/exec

machinectl does not propogate error messages and adds extra lines in the output, exec.Cmd is able to clear the env besides PATH and TERM, and use the given UID and GID to execute the command properly.

machinectl is still used to create a user session. Ubuntu support is limited by this.

Signed-off-by: cdoern [email protected]

Luap99

This does not work because systemd-run does not create the required login session.

I think you have to use both, first run machinectl ... sleep inf to make sure that the login session is created (just start the process), then run your systemd-run command, when that is done kill the machinectl process.

cdoern · 2022-01-14T19:04:38Z

@Luap99 think I got all of the necessary changes in there, just made a utils function to start the machinectl sleep.

eriksjolund · 2022-01-15T07:12:15Z

This does not work because systemd-run does not create the required login session.

I think you have to use both, first run machinectl ... sleep inf to make sure that the login session is created (just start the process), then run your systemd-run command, when that is done kill the machinectl process.

I wonder if machinectl ... sleep inf is really needed.
It seems the command
systemd-run --machine=test11@ --quiet --user --collect --pipe --wait ...
starts a login session.

[root@laptop ~]# useradd test11
[root@laptop ~]# systemd-run --machine=test11@ --quiet --user --collect --pipe --wait loginctl
SESSION  UID USER     SEAT  TTY
      2 1000 esjolund seat0 tty2
     32 1019 test11         

2 sessions listed.
[root@laptop ~]# systemd-run --machine=test11@ --quiet --user --collect --pipe --wait systemctl --user status
● laptop
    State: running
     Jobs: 0 queued
   Failed: 0 units
    Since: Sat 2022-01-15 08:10:23 CET; 131ms ago
   CGroup: /user.slice/user-1019.slice/[email protected]
           ├─session.slice 
           │ └─dbus-broker.service 
           │   ├─6371 /usr/bin/dbus-broker-launch --scope user
           │   └─6374 dbus-broker --log 4 --controller 9 --machine-id c341780e88a04800982a41eea47639d3 --max-bytes 100000000000000 --max-fds 25000000000000 --max-matches 5000000000
           ├─app.slice 
           │ └─run-u0.service 
           │   ├─6376 systemctl --user status
           │   └─6378 less
           └─init.scope 
             ├─6346 /usr/lib/systemd/systemd --user
             └─6350 (sd-pam)
[root@laptop ~]# cat /etc/fedora-release 
Fedora release 35 (Thirty Five)
[root@laptop ~]#

cmd/podman/images/scp.go

pkg/domain/infra/abi/images.go

cdoern · 2022-01-17T15:26:17Z

This does not work because systemd-run does not create the required login session.
I think you have to use both, first run machinectl ... sleep inf to make sure that the login session is created (just start the process), then run your systemd-run command, when that is done kill the machinectl process.

I wonder if machinectl ... sleep inf is really needed. It seems the command systemd-run --machine=test11@ --quiet --user --collect --pipe --wait ... starts a login session.

[root@laptop ~]# useradd test11
[root@laptop ~]# systemd-run --machine=test11@ --quiet --user --collect --pipe --wait loginctl
SESSION  UID USER     SEAT  TTY
      2 1000 esjolund seat0 tty2
     32 1019 test11         

2 sessions listed.
[root@laptop ~]# systemd-run --machine=test11@ --quiet --user --collect --pipe --wait systemctl --user status
● laptop
    State: running
     Jobs: 0 queued
   Failed: 0 units
    Since: Sat 2022-01-15 08:10:23 CET; 131ms ago
   CGroup: /user.slice/user-1019.slice/[email protected]
           ├─session.slice 
           │ └─dbus-broker.service 
           │   ├─6371 /usr/bin/dbus-broker-launch --scope user
           │   └─6374 dbus-broker --log 4 --controller 9 --machine-id c341780e88a04800982a41eea47639d3 --max-bytes 100000000000000 --max-fds 25000000000000 --max-matches 5000000000
           ├─app.slice 
           │ └─run-u0.service 
           │   ├─6376 systemctl --user status
           │   └─6378 less
           └─init.scope 
             ├─6346 /usr/lib/systemd/systemd --user
             └─6350 (sd-pam)
[root@laptop ~]# cat /etc/fedora-release 
Fedora release 35 (Thirty Five)
[root@laptop ~]#

not sure if executing on a container is what we are looking for here, this functionality is similar to that of machinectl and I am unsure if this would propagate errors the way we want to.

--machine operates much differently than --uid if I am not mistaken, @Luap99 @edsantiago WDYT?

edsantiago · 2022-01-17T15:36:05Z

I'm old and crotchety, and I really, really do not want to learn about systemd-run and all this newfangled crap. Can someone ELI5 why sudo or su -l -c "%s" USER aren't enough? (preferably documenting this in the code itself, so noone else has to ask again)

Luap99 · 2022-01-17T15:36:26Z

I think --machine is what we want because it seems to create the login session. I just was never able to make it work on my test vm. You should still get the correct exit code according to the man page.

Luap99 · 2022-01-17T15:38:29Z

I'm old and crotchety, and I really, really do not want to learn about systemd-run and all this newfangled crap. Can someone ELI5 why sudo or su -l -c "%s" USER aren't enough? (preferably documenting this in the code itself, so noone else has to ask again)

Because both sudo and su do not create a login session they just change the uid/gid. Podman needs the systemd session to function properly. The only way su/sudo will work when you set enable-linger for that user set.

edsantiago · 2022-01-17T15:39:37Z

The only way su/sudo will work when you set enable-linger for that user set.

But podman is completely unusable without linger, so in what possible situation would anyone use podman image scp on a user that doesn't have the magic linger?

Luap99 · 2022-01-17T15:44:59Z

You do not need linger!!!! If you login in you systemd as a user systemd will create the user session for you and you can use podman just fine. If you logout the session is destroyed so all containers are stopped.
Linger is only needed when you want to run containers at boot or after logout.

cdoern · 2022-01-17T15:46:05Z

also @Luap99 I am having the same issue as you, --machine does not work on my machine and reports: Failed to get machine PTY: No machine 'charliedoern@' same fails with [email protected]

even when using --collect --pipe:

Failed to connect to bus: $DBUS_SESSION_BUS_ADDRESS and $XDG_RUNTIME_DIR not defined (consider using --machine=<user>@.host --user to connect to bus of other user)

those env variables are in fact set too...

Luap99 · 2022-01-17T15:56:09Z

@cdoern On which os did you test this? I tested on f34 and it failed but I just tried with f35 and @eriksjolund commands worked there.

cdoern · 2022-01-17T15:56:27Z

@Luap99 ubuntu and f34

eriksjolund · 2022-01-17T16:07:21Z

I tested a few examples on Fedora 35 with a brand new user. (linger is off). It seems to work.

[root@laptop ~]# cat /etc/fedora-release 
Fedora release 35 (Thirty Five)
[root@laptop ~]# useradd test26
[root@laptop ~]# systemd-run --machine=test26@ --quiet --user --collect --pipe --wait podman run --rm docker.io/library/alpine echo hello
Trying to pull docker.io/library/alpine:latest...
Getting image source signatures
Copying blob 59bf1c3509f3 done  
Copying config c059bfaa84 done  
Writing manifest to image destination
Storing signatures
hello
[root@laptop ~]# systemd-run --machine=test26@ --quiet --user --collect --pipe --wait /bin/false ; echo $?
1
[root@laptop ~]# systemd-run --machine=test26@ --quiet --user --collect --pipe --wait /bin/true ; echo $?
0
[root@laptop ~]# systemd-run --machine=test26@ --quiet --user --collect --pipe --wait ls -l /non-existent ; echo $?
ls: cannot access '/non-existent': No such file or directory
2
[root@laptop ~]# systemd-run --machine=test26@ --quiet --user --collect --pipe --wait /bin/bash -c "exit 34" ; echo $?
34
[root@laptop ~]#

Relevant PR
systemd/systemd#17967
hence systemd v248

cdoern · 2022-01-17T16:36:59Z

@eriksjolund I think we are going to stick with the current approach as a fix for the moment, but I am going to look into if there is a better way to start the login session. Your approach is preferable but does not work on f34 or ubuntu.

edsantiago · 2022-01-17T17:23:20Z

It does not seem to be working when run as root (on my f35 laptop):

$ sudo bin/podman image scp foo.bar/nonesuch/c_zr5sx1le5n:mytag esm@localhost::
Running as unit: run-re976e3d02b7c4edea1205642220f85b8.service
Running as unit: run-r04f0570cc1cd4e778ed6368f137c8032.service
$ bin/podman images
[ does not show foo.bar/nonesuch/anything]

Note also that the expected output of image scp is 'Copying blob...Writing manifest...Storing signatures...' but none of that appears, only Running as unit. Could you test this as root?

cdoern · 2022-01-17T17:39:03Z

sorry @edsantiago left an extra argument in execSystemd works now as expected.

edsantiago · 2022-01-17T17:55:14Z

Now I get:

$ sudo bin/podman image scp foo.bar/nonesuch/c_zr5sx1le5n:mytag esm@localhost::
Error: exit status 208

Adding --log-level=debug yields:

DEBU[0000] Executing via systemd: "/usr/bin/sudo /usr/bin/systemd-run --pty -q --wait --uid 0 /home/esm/src/atomic/2018-02.podman/libpod/bin/podman --log-level=debug save --output /var/tmp/podman997723794 foo.bar/nonesuch/c_zr5sx1le5n:mytag"

cdoern · 2022-01-17T18:07:54Z

@edsantiago this seems to be an selinux issue, https://bugzilla.redhat.com/show_bug.cgi?id=1559409 not sure how to get around this one...

Luap99 · 2022-01-17T18:10:54Z

/usr/bin/sudo /usr/bin/systemd-run --pty -q --wait --uid 0 /home/esm/src/atomic/2018-02.podman/libpod/bin/podman --log-level=debug save --output /var/tmp/podman997723794 foo.bar/nonesuch/c_zr5sx1le5n:mytag The command looks wrong should't it be --uid <uid from esm> instead of --uid 0 which is pointless since it should load the image as user.

Luap99 · 2022-01-17T18:11:13Z

Also why do you use sudo when you are already root

cdoern · 2022-01-17T18:15:03Z

@Luap99 in the example @edsantiago is giving the save is --uid 0 and the load is --uid <edsantiago>

issue here is exit code 208 from systemd-run this works on ubuntu meaning the syntax is correct, the incorrect part is the selinux permssions on f35

cdoern · 2022-01-17T18:21:59Z

--collect --pipe works on fedora rather than --pty, let me try that

SeLinux blocks the executing of the podman binary....... via exit code 203

cdoern · 2022-01-24T14:07:00Z

@edsantiago @Luap99 thoughts on this? I am not sure if runuser creates a login session so I left the machinectl login as is.

edsantiago · 2022-01-24T14:11:20Z

I'm sorry, I can't help. "User session" is not a UNIX concept, it's a systemd concept, and one that I've never been able to understand. @giuseppe might know.

rhatdan · 2022-01-24T18:00:23Z

runuser will NOT create a user session it is just setting the UID and perhaps running the bash init scripts.
User Sessions require pam_systemd to run, and runuser specifically does not execute the PAM stack.

cdoern · 2022-01-24T23:34:07Z

Ok, that makes sense @rhatdan I think this is all set then @containers/podman-maintainers PTAL

pkg/domain/infra/abi/images.go

giuseppe · 2022-01-25T09:33:27Z

pkg/domain/infra/abi/images.go

+	verb := runUser
+	args := []string{"-l", execUser.Username, "-c"}
+	cmd := utils.CreateSCPCommand(exec.Command(verb, args...), []string{strings.Join(command, " ")})


completely untested, but wouldn't something like the following:

cmd := exec.Command(command[0], command[1:]...) cmd.Env = os.Environ() cmd.Stderr = os.Stderr cmd.Stdout = os.Stdout uid, err := strconv.ParseInt(execUser.Uid, 10, 32) if err != nil { return err } gid, err := strconv.ParseInt(execUser.Gid, 10, 32) if err != nil { return err } cmd.SysProcAttr = &syscall.SysProcAttr{ Credential: &syscall.Credential{ Uid: uint32(uid), Gid: uint32(gid), Groups: nil, NoSetGroups: false, }, }

run the command with the specified UID/GID without requiring runuser?

I think this leaks the rootful environment variables such as XDG_RUNTIME_DIR and thus make the rootless podman fail.

Wasn't that leak already in CreateSCPCommand?

AFAICS runuser doesn't clean up the environment:

$ sudo XDG_RUNTIME_DIR=wrong runuser -u gscrivano printenv XDG_RUNTIME_DIR wrong

In any case, you are right, and I think we need to drop the Environ block.

I agree we should block the environment.

What do you mean the environment is leaked? Doesnt -l clear all env variables except for those specified? @Luap99 clears all the environment variables except for TERM and variables specified by --whitelist-environment

ah okay, sorry I was looking at this on mobile. From what i have tried over the iterations, directly calling a n exec.Cmd via a different user using the sysprocattr basically never works and always leaves something behind from the previous session.

can you specify some Env not nil? From the doc:

// If Env is nil, the new process uses the current process's // environment.

hmm maybe I should try to keep $TERM like runuser does and see if that works?

I would set $TERM and $PATH

seems to work, I am getting a storage error regarding the graph driver: ERRO[0000] User-selected graph driver "vfs" overwritten by graph driver "overlay" from database - delete libpod local files to resolve but the command finishes successfully. This could be my machine so I will push here and let the tests run

cdoern · 2022-01-25T15:30:34Z

I think it might be possible to consolidate transferRootless and transferRootful now but on my first try it failed due to the uid/gid aspect of transferRootful, I think after this merges I'll open a separate PR to work on that.

cdoern · 2022-01-26T01:45:22Z

@giuseppe any idea why this is failing? This error has happened to me before in my initial iterations of scp, do I need to UID/GID map as well in the SysProcAttr?

this works on f34 and f35 for me as is when using runc as the runtime

giuseppe · 2022-01-26T08:31:34Z

@giuseppe any idea why this is failing? This error has happened to me before in my initial iterations of scp, do I need to UID/GID map as well in the SysProcAttr?

no we don't neet to specify the map, it has simply to run with the specified UID/GID

pkg/domain/infra/abi/images.go

giuseppe · 2022-01-26T08:34:41Z

the tests are failing because it doesn't find the newuidmap executable. Please propagate also PATH in addition to TERM

cdoern · 2022-01-26T16:37:38Z

I initially tried to do this with PATH as well and got the same error, could be because I was not doing PATH=... I will try that

machinectl does not propogate error messages and adds extra lines in the output, exec.Cmd is able to clear the env besides PATH and TERM, and use the given UID and GID to execute the command properly. machinectl is still used to create a user session. Ubuntu support is limited by this. Signed-off-by: cdoern <[email protected]>

rhatdan · 2022-01-27T12:48:31Z

/approve
/lgtm

I am going to merge, since I believe this is an improvement over what we currently have, and we should get it into podman 4.0.
@cdoern continue working on improving it. and fixing other bugs in podman image scp.

openshift-ci · 2022-01-27T12:48:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, rhatdan

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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cdoern force-pushed the scp branch from 30b7f48 to 688267a Compare January 14, 2022 18:12

cdoern changed the title ~~[NO TESTS NEEDED] switch podman image scp from depending on machinectl to systemd-run~~ [NO NEW TESTS NEEDED] switch podman image scp from depending on machinectl to systemd-run Jan 14, 2022

Luap99 requested changes Jan 14, 2022

View reviewed changes

cdoern force-pushed the scp branch from 688267a to 350639a Compare January 14, 2022 19:04

edsantiago reviewed Jan 17, 2022

View reviewed changes

cmd/podman/images/scp.go Outdated Show resolved Hide resolved

edsantiago reviewed Jan 17, 2022

View reviewed changes

pkg/domain/infra/abi/images.go Outdated Show resolved Hide resolved

cdoern force-pushed the scp branch 2 times, most recently from 9f4ed80 to e6d0958 Compare January 17, 2022 16:49

cdoern force-pushed the scp branch from e6d0958 to 0e8f2c0 Compare January 17, 2022 17:38

cdoern marked this pull request as ready for review January 20, 2022 03:51

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2022

cdoern removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2022

giuseppe reviewed Jan 25, 2022

View reviewed changes

cdoern force-pushed the scp branch 2 times, most recently from 3d7c8ab to 74d4186 Compare January 25, 2022 15:29

cdoern changed the title ~~switch podman image scp from depending on machinectl to runuser~~ switch podman image scp from depending on machinectl to just os/exec Jan 26, 2022

cdoern force-pushed the scp branch from 74d4186 to c7e7056 Compare January 26, 2022 03:01

giuseppe reviewed Jan 26, 2022

View reviewed changes

pkg/domain/infra/abi/images.go Outdated Show resolved Hide resolved

cdoern force-pushed the scp branch 2 times, most recently from 95b7619 to 30271bc Compare January 26, 2022 16:33

cdoern force-pushed the scp branch from 30271bc to 77cd38d Compare January 26, 2022 19:40

openshift-ci bot assigned rhatdan Jan 27, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2022

openshift-merge-robot merged commit 5659b07 into containers:main Jan 27, 2022

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023

switch podman image scp from depending on machinectl to just os/exec #12867

switch podman image scp from depending on machinectl to just os/exec #12867

Conversation

cdoern commented Jan 14, 2022 • edited Loading

Luap99 left a comment

Choose a reason for hiding this comment

cdoern commented Jan 14, 2022

eriksjolund commented Jan 15, 2022

cdoern commented Jan 17, 2022

edsantiago commented Jan 17, 2022

Luap99 commented Jan 17, 2022

Luap99 commented Jan 17, 2022

edsantiago commented Jan 17, 2022

Luap99 commented Jan 17, 2022

cdoern commented Jan 17, 2022 • edited Loading

Luap99 commented Jan 17, 2022

cdoern commented Jan 17, 2022

eriksjolund commented Jan 17, 2022

cdoern commented Jan 17, 2022

edsantiago commented Jan 17, 2022

cdoern commented Jan 17, 2022

edsantiago commented Jan 17, 2022

cdoern commented Jan 17, 2022

Luap99 commented Jan 17, 2022

Luap99 commented Jan 17, 2022

cdoern commented Jan 17, 2022 • edited Loading

cdoern commented Jan 17, 2022 • edited Loading

cdoern commented Jan 24, 2022

edsantiago commented Jan 24, 2022

rhatdan commented Jan 24, 2022

cdoern commented Jan 24, 2022

giuseppe Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdoern Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdoern commented Jan 25, 2022

cdoern commented Jan 26, 2022 • edited Loading

giuseppe commented Jan 26, 2022

giuseppe commented Jan 26, 2022

cdoern commented Jan 26, 2022

rhatdan commented Jan 27, 2022

openshift-ci bot commented Jan 27, 2022

cdoern commented Jan 14, 2022 •

edited

Loading

cdoern commented Jan 17, 2022 •

edited

Loading

cdoern commented Jan 17, 2022 •

edited

Loading

cdoern commented Jan 17, 2022 •

edited

Loading

giuseppe Jan 25, 2022 •

edited

Loading

cdoern Jan 25, 2022 •

edited

Loading

cdoern commented Jan 26, 2022 •

edited

Loading