Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

installer: Don't run graphical installer ISOs as root #42610

Closed
wants to merge 1 commit into from

Conversation

adisbladis
Copy link
Member

@adisbladis adisbladis commented Jun 26, 2018

Motivation for this change

This is currently causing issues with some tools like Dolphin (the KDE
file manager) that refuses to run as root.
It's also very rare for graphical install media to be running as root nowadays, all other distros I know of run their installation processes as unprivileged users.
We can instead allow passwordless sudo with similar effect.

On the TTY you are still root, it's just the graphical session user that has changed.

I also took the opportunity to deduplicate some configuration that was the same across KDE/GNOME live ISOs.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, did not know about GNOME installer, might be interesting to people at #41797

chmod a+rx /root/Desktop/gnome-terminal.desktop
cp ${pkgs.gparted}/share/applications/gparted.desktop /root/Desktop/gparted.desktop
chmod a+rx /root/Desktop/gparted.desktop
ln -sfT ${pkgs.gnome3.gnome-terminal}/share/applications/gnome-terminal.desktop /home/nix-live/Desktop/gnome-terminal.desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GNOME does not have desktop by default so I guess this is moot. You would need something like:

services.xserver.desktopManager.gnome3 = {
  extraGSettingsOverridePackages = with pkgs; [ gnome3.gnome_shell ];
  extraGSettingsOverrides = ''
    [org/gnome/shell]
    favorite-apps=['firefox.desktop', 'nixos-manual.desktop', 'gparted.desktop', 'org.gnome.Terminal.desktop']
  '';
};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. My solution for now is to remove gnome desktop entries and let someone more familiar with gnome take care of improving that installer.

@adisbladis adisbladis changed the title installer: Don't run graphical installer ISOs as root WIP: installer: Don't run graphical installer ISOs as root Jun 26, 2018
@worldofpeace
Copy link
Contributor

Thanks for going off of my idea. ❤️

@@ -27,47 +18,15 @@ with lib;
synaptics.enable = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be libinput? I don't think it's a default for plasma5.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. I didn't want to make any major functionality changes right now.
Running as non-root is major enough for a single PR and warrants consideration purely on it's own rights.

Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No resistence here.

I don't think a gnome wayland session would have ever worked with root login if there ever was interest to have GNOME the default DE for the graphical installation media.

Other things are:

  • perhaps a default password

  • please remove cd-dvd/installation-cd-graphical-kde-new-kernel.nix here.
    I don't think it's actually used for anything.

enable = true;

# Don't start the X server by default.
autorun = mkForce false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop this I think. This is a graphical media, I would expect it to just start.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this belongs in another PR.

I think we should add another boot entry so you can still choose not to start the graphical environment, at the same time we make the one that autostarts the graphical environment the default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add another boot entry so you can still choose not to start the graphical environment, at the same time we make the one that autostarts the graphical environment the default.

Why would that be needed? I don't see the graphical env ever being not functional, and you can always just start it and switch to a tty if needed. If someone wanted to not have a graphical env they would use the other iso...

Yeah a discussion for another PR. 👍

@davidak
Copy link
Member

davidak commented Jul 11, 2018

Great work!

I think the user should be less complex, like just 'live' without -.

@adisbladis adisbladis force-pushed the installer-noroot branch 2 times, most recently from 3a088a7 to ff7247c Compare July 13, 2018 07:08
@adisbladis adisbladis changed the title WIP: installer: Don't run graphical installer ISOs as root installer: Don't run graphical installer ISOs as root Jul 13, 2018
@adisbladis
Copy link
Member Author

@davidak Thanks for your feedback. I agree.

This is currently causing issues with some tools like Dolphin (the KDE
file manager) that refuses to run as root.

We can instead allow passwordless sudo with similar effect.
@adisbladis
Copy link
Member Author

This should be in a merge-ready state. I just rebased it on master.
Unless someone is opposed I'll merge in a few days.

CC @dezgeg @xeji @grahamc @edolstra

@@ -63,11 +34,23 @@ with lib;
Icon=text-html
'';

# Replace default gparted desktop file with one that does "sudo gparted"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this block be useful for both Gnome and KDE images?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gnome doesn't have files on the desktop anymore.

Copy link
Contributor

@etu etu Jul 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A desktop file is not a file on the desktop, a desktop file is still used to launch applications from launchers etc.

And from my point of view it would possibly be useful if a user could click "gparted" in the launcher and that it's executed with sudo. But I'm not sure if there's any magic involved.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But these entries would not be visible any more since gnome doesn't read ~/Desktop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You suggest this would be useful if it used a patched version of gparted that uses sudo?
Currently here it only copies this file to the desktop.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@worldofpeace Yeah, sure. That would be more useful. Especially on an installation media.

Not sure if that's in scope of this PR. But maybe it should be since the behaviour is changed from "I can click that button" to "Clicking that button tells me that it requires root to run".

@edolstra
Copy link
Member

Not in favor of this. It just seems to make the installation process needlessly complicated. I mean, installing an OS is an inherently privileged operation, so what security is gained by not running as root?

@adisbladis
Copy link
Member Author

adisbladis commented Jul 13, 2018

@edolstra Imo this is not at all about security.
It's about tools like dolphin refusing to run as root and catering to that case.

Currently our live media comes without a working file manager.

edit: Also, running a wayland session as root is not possible.

@worldofpeace
Copy link
Contributor

@edolstra Also it is substantiated that running graphical programs as root is insecure.
Here's as statement from a Gnome dev that supports this.

@edolstra
Copy link
Member

@worldofpeace That's cargo cult security. If a user account can sudo to root without a password, then that user account is pretty much equivalent to root in security terms.

<listitem>
<para>
The installer is no longer running the graphical session as root.
To gain root privileges in the graphical session use <literal>sudo</literal> without a password.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be mentioned in the installation section of the manual as well.

Also, it probably should be sudo -i.

@samueldr
Copy link
Member

samueldr commented Sep 9, 2018

Other than the file manager not being usable as root, which real other reasons are there for the installer environment to run in that double-faced manner? I will continue with arguments against, but I am open to arguments for.

1. Dolphin is fixed

image

This was taken from a freshly built graphical iso at the current tip of channels/nixos-18.09. We don't need this for 18.09.

2. Makes documentation harder to write

This is because anything related to manipulations using the installer image now has to account for the possibility of having the user not be root. The existing instructions and the existing user knowledge all assume this is always done under the root user account.

This will also cause issues when troubleshooting (discourse, IRC) where it will become plausible an issue a user has is caused by being in the graphical interface instead of being in a VT logged-in as root.

3. Wayland

I lied, this is not an argument against, but more of an open-ended question: when is it realistic to see Wayland on the installer image? I'm not up to speed with the state of Wayland, it may be more likely than I think!


I'm thinking this should be delayed, because we may need to figure out a better alternative than having the installer image sometimes be root, sometimes not be. It may be better (imho) to figure out a way so that all installer images behave the same way.

Since Wayland isn't (probably) for the next release, and definitely not this one, let's work on a proactive better integrated solution instead of a reactive (to dolphin) solution.

@adisbladis
Copy link
Member Author

@samueldr I completely share your concerns. I also noticed the change in behaviour of dolphing and was deliberately holding this off for after 18.09.

Now that my immediate reason for this change is no longer valid I think it's a good idea to close this and rethink how we can get a wayland session installer up and running for a future release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants