Only allow Rootless runs of Podman Machine #14706

ashley-cui · 2022-06-22T21:49:28Z

Podman Machine crashes if run as root, as we need the UID of the core user to match the UID of the user on the host. If the user on the host is root, the core UID and the Root UID collide, causing a the VM not to boot.

[NO NEW TESTS NEEDED]

Signed-off-by: Ashley Cui [email protected]

Does this PR introduce a user-facing change?

Allow `podman machine` to only be run rootless, as the VM refuses to boot when run as root.

openshift-ci · 2022-06-22T21:49:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui

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~~ [ashley-cui]

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

ashley-cui · 2022-06-22T21:51:26Z

No tests because this just disables running podman machine as root. Also, wasn't sure if it was a better idea to do this with annotations or by just copying and pasting if !rootless.IsRootless() || os.Getuid() == 0 into every machine file, annotations felt cleaner so I went with that. Let me know if this is the right way.

Luap99 · 2022-06-23T07:25:07Z

I think it would be much less code if you just add it to the PersitentPreRunE function for the machine command:

podman/cmd/podman/machine/machine.go

Line 37 in 8e88abd

PersistentPreRunE: validate.NoOp,

giuseppe · 2022-06-23T09:02:48Z

Podman Machine crashes if run as root, as we need the UID of the core user to match the UID of the user on the host. If the user on the host is root, the core UID and the Root UID collide, causing a the VM not to boot.

does it mean it crashes with any UID != UID in the VM? e.g. what happens if the UID on the local system is 1001?

ashley-cui · 2022-06-24T13:54:13Z

@giuseppe We create the root user and the core user in the machine by writing the UID to the ignition file. Podman machine checks your current UID on the host and sets the core user to that UID. If we were to use podman machine as root, we end up trying to create the core user and the root user with UID 0, which causes the machine not to boot. So, the root UID != core UID in order for the machine to properly boot.

It shouldn't crash if user UID != core UID in the VM (I think..)

ashley-cui · 2022-06-24T14:25:20Z

Thanks @Luap99 ! Great idea, updated

Luap99 · 2022-06-24T14:31:19Z

cmd/podman/machine/machine.go

@@ -162,3 +164,10 @@ func closeMachineEvents(cmd *cobra.Command, _ []string) error {
 	}
 	return nil
 }
+
+func rootlessOnly(cmd *cobra.Command, _ []string) error {
+	if !rootless.IsRootless() || os.Getuid() == 0 {


The getuid() check should not be needed sine it is already done in IsRootless()

Luap99

LGTM

giuseppe · 2022-06-24T17:29:15Z

@giuseppe We create the root user and the core user in the machine by writing the UID to the ignition file. Podman machine checks your current UID on the host and sets the core user to that UID. If we were to use podman machine as root, we end up trying to create the core user and the root user with UID 0, which causes the machine not to boot. So, the root UID != core UID in order for the machine to properly boot.

It shouldn't crash if user UID != core UID in the VM (I think..)

it makes sense. Thanks for the explanation

mheon · 2022-06-24T17:32:08Z

System tests are angry

ashley-cui · 2022-06-24T19:44:21Z

Hmm, okay. Looks like putting the rootless check in PersistentPreRunE for podman machine makes it so that running just podman machine without a subcommand fails with podman-machine can only be run as root, whereas the test expects the help output to be printed. I can't think of a way to get this working atm, other than putting the check back into every machine subcommand, @Luap99 Any thoughts?

Luap99 · 2022-06-28T13:06:57Z

Hmm, okay. Looks like putting the rootless check in PersistentPreRunE for podman machine makes it so that running just podman machine without a subcommand fails with podman-machine can only be run as root, whereas the test expects the help output to be printed. I can't think of a way to get this working atm, other than putting the check back into every machine subcommand, @Luap99 Any thoughts?

Mhh, I don't think there is a way to avoid that. Code LGTM but you need to update the man pages.

ashley-cui · 2022-06-28T18:41:21Z

Updated, @containers/podman-maintainers PTAL

cdoern

Do you need to modify the machine tests in pkg/machine/e2e to make sure they only run as rootless?

ashley-cui · 2022-06-28T20:58:24Z

@cdoern Those currently don't run in CI at all :)

Podman Machine crashes if run as root. When creating the machine, we write the ignition so that the UID of the core user matches the UID of the user on the host. We by default, create the root user on the machine with UID 0. If the user on the host is root, the core UID and the Root UID collide, causing a the VM not to boot. [NO NEW TESTS NEEDED] Signed-off-by: Ashley Cui <[email protected]>

rhatdan · 2022-06-29T19:24:23Z

/lgtm

gbraad · 2022-07-01T02:44:57Z

Unfortunately this broke podman machine on Windows. I discussed this with @n1hility and he provided a fix. Will test #14794

openshift-ci bot added the release-note label Jun 22, 2022

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

ashley-cui force-pushed the rootmach branch from a4621eb to 65d825b Compare June 22, 2022 21:56

ashley-cui force-pushed the rootmach branch from 65d825b to 0438a21 Compare June 24, 2022 14:24

Luap99 reviewed Jun 24, 2022

View reviewed changes

ashley-cui force-pushed the rootmach branch from 0438a21 to e1841e6 Compare June 24, 2022 14:43

Luap99 reviewed Jun 24, 2022

View reviewed changes

ashley-cui force-pushed the rootmach branch from e1841e6 to abc0a07 Compare June 24, 2022 19:44

ashley-cui force-pushed the rootmach branch from abc0a07 to c749daa Compare June 28, 2022 18:40

cdoern mentioned this pull request Jun 28, 2022

Podman machine info #14762

Merged

cdoern reviewed Jun 28, 2022

View reviewed changes

ashley-cui force-pushed the rootmach branch from c749daa to 29c2cb1 Compare June 28, 2022 20:57

ashley-cui force-pushed the rootmach branch from 29c2cb1 to d9ff0fd Compare June 29, 2022 13:19

openshift-ci bot assigned rhatdan Jun 29, 2022

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

openshift-ci bot merged commit d6cdb99 into containers:main Jun 29, 2022

n1hility mentioned this pull request Jun 30, 2022

Fix podman machine on Windows #14794

Merged

ashley-cui deleted the rootmach branch July 15, 2022 19:27

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only allow Rootless runs of Podman Machine #14706

Only allow Rootless runs of Podman Machine #14706

ashley-cui commented Jun 22, 2022

openshift-ci bot commented Jun 22, 2022

ashley-cui commented Jun 22, 2022

Luap99 commented Jun 23, 2022

giuseppe commented Jun 23, 2022

ashley-cui commented Jun 24, 2022 •

edited

Loading

ashley-cui commented Jun 24, 2022

Luap99 Jun 24, 2022

Luap99 left a comment

giuseppe commented Jun 24, 2022

mheon commented Jun 24, 2022

ashley-cui commented Jun 24, 2022

Luap99 commented Jun 28, 2022

ashley-cui commented Jun 28, 2022

cdoern left a comment

ashley-cui commented Jun 28, 2022

rhatdan commented Jun 29, 2022

gbraad commented Jul 1, 2022 •

edited

Loading

Only allow Rootless runs of Podman Machine #14706

Only allow Rootless runs of Podman Machine #14706

Conversation

ashley-cui commented Jun 22, 2022

Does this PR introduce a user-facing change?

openshift-ci bot commented Jun 22, 2022

ashley-cui commented Jun 22, 2022

Luap99 commented Jun 23, 2022

giuseppe commented Jun 23, 2022

ashley-cui commented Jun 24, 2022 • edited Loading

ashley-cui commented Jun 24, 2022

Luap99 Jun 24, 2022

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

giuseppe commented Jun 24, 2022

mheon commented Jun 24, 2022

ashley-cui commented Jun 24, 2022

Luap99 commented Jun 28, 2022

ashley-cui commented Jun 28, 2022

cdoern left a comment

Choose a reason for hiding this comment

ashley-cui commented Jun 28, 2022

rhatdan commented Jun 29, 2022

gbraad commented Jul 1, 2022 • edited Loading

ashley-cui commented Jun 24, 2022 •

edited

Loading

gbraad commented Jul 1, 2022 •

edited

Loading