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 as root #66338

Merged
merged 3 commits into from Aug 14, 2019
Merged

Conversation

worldofpeace
Copy link
Contributor

Motivation for this change

There's many reason why it is and is going to
continue to be difficult to do this:

  1. All display-managers (excluding slim) default PAM rules
    disallow root auto login.

  2. We can't use wayland

  3. We have to use system-wide pulseaudio

  4. It could break applications in the session.
    This happened to dolphin in plasma5
    in the past.

This is a growing technical debt, let's just use
passwordless sudo.


This is a per-requisite to having a GNOME3 iso.
Broken up from #66313

I've supplied all the requested changes to what was raised on this commit.
In particular #66313 (review).

Things done

I've built iso_minimal and it auto logs in as live when tested
in qemu.

  • 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 nix-review --run "nix-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@@ -17,7 +27,7 @@ with lib;
# Automatically login as root.
displayManager.slim = {
Copy link
Member

Choose a reason for hiding this comment

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

I won't ask it of this PR, but slim is long abandoned. We should get off of it sometime :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I raised this in #66313 (comment) at the very end. Some people had strong opinions though in the past.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

Shall we go for slightly more branding and call the "live" user "nixos"?

@worldofpeace
Copy link
Contributor Author

Shall we go for slightly more branding and call the "live" user "nixos"?

When adisbladis and I discussed this and he authored the original change
I think @davidak suggested to use live #42610 (comment).

@grahamc
Copy link
Member

grahamc commented Aug 8, 2019

It sounds like the name then was something with a hyphen, but I can't see what it was. I still like nixos as the name, but 🤷‍♂️ 😉 .

One thing I spotted in that linked PR is the suggestion of sudo -i, which we probably should document.

@worldofpeace
Copy link
Contributor Author

It sounds like the name then was something with a hyphen, but I can't see what it was. I still like nixos as the name, but man_shrugging wink .

One thing I spotted in that linked PR is the suggestion of sudo -i, which we probably should document.

I think I'll do this 👍

@@ -30,15 +30,27 @@ with lib;
Version=1.0
Type=Application
Name=NixOS Manual
Exec=firefox ${config.system.build.manual.manualHTMLIndex}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just realized that nixos-manual.desktop generation isn't needed anymore,
as nixos-manual has a desktop item that will intelligently launch the default browser.

Copy link
Member

Choose a reason for hiding this comment

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

So this can be removed?

@samueldr
Copy link
Member

samueldr commented Aug 8, 2019

(Noting for myself, mainly) this will affect sd_image, meaning both that it needs to be tested, and (external) documentation amended as needed.

@worldofpeace
Copy link
Contributor Author

bd59a8b was needed so users can use gparted and it will call into pkexec so we don't have to use a custom sudo desktop item.

They'll need no kind of authentication because of the polkit rule we added.

@bachp
Copy link
Member

bachp commented Aug 11, 2019

I remember that I once added the possibility do do root login into the install media via SSH. If the default user is no longer root than this is probably no longer needed.

The SSH install workflow would then just be as follows:

  1. Set a password (or public key) for user nixos
  2. Enable SSH daemon
  3. Login via ssh nixos@<target>
  4. Run sudo -i

So only password login for normal users is required.

@worldofpeace
Copy link
Contributor Author

worldofpeace commented Aug 11, 2019

I remember that I once added the possibility do do root login into the install media via SSH. If the default user is no longer root than this is probably no longer needed.

The SSH install workflow would then just be as follows:

1. Set a password (or public key) for user `nixos`

2. Enable SSH daemon

3. Login via `ssh nixos@<target>`

4. Run `sudo -i`

So only password login for normal users is required.

Guess that means you authored

 <para>
    If you would like to continue the installation from a different machine you
    need to activate the SSH daemon via <literal>systemctl start
    sshd</literal>. In order to be able to login you also need to set a
    password for <literal>root</literal> using <literal>passwd</literal>.
   </para>

I guess we should document it this way, but I believe we should still permit root login with ssh.

@worldofpeace
Copy link
Contributor Author

I've now tested the graphical iso.

Things done

  1. can sudo -i on tty
  2. can run sudo systemctl start display-manager on tty
  3. graphical session starts
  4. all desktop items launched properly (gparted without password prompt from polkit)
  5. can use sudo without password in konsole

Did the procedure to login via ssh for root and nixos.

@@ -29,13 +29,14 @@
</para>

<para>
You are logged-in automatically as <literal>root</literal>. (The
<literal>root</literal> user account has an empty password.)
You are logged-in automatically as <literal>nixos</literal>.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
You are logged-in automatically as <literal>nixos</literal>.
You are logged-in automatically as the <literal>nixos</literal> user.

@@ -33,6 +33,12 @@
PHP 7.1 is no longer supported due to upstream not supporting this version for the entire lifecycle of the 19.09 release.
</para>
</listitem>
<listitem>
<para>
The installer now uses a less privileged <literal>nixos</literal> user whereas before we logged in as root.
Copy link
Member

@grahamc grahamc Aug 12, 2019

Choose a reason for hiding this comment

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

Suggested change
The installer now uses a less privileged <literal>nixos</literal> user whereas before we logged in as root.
The installer now uses the <literal>nixos</literal> user instead of <literal>root</literal>.

Copy link
Member

@grahamc grahamc Aug 12, 2019

Choose a reason for hiding this comment

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

I drop "less privileged" because they're not actually less privileged., and saying it is might lead to questions about well how do I do the thing then.

Copy link
Member

@grahamc grahamc left a comment

Choose a reason for hiding this comment

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

I haven't tested it, but I am quite sure it has been tested :) Looks great.

There's many reason why it is and is going to
continue to be difficult to do this:

1. All display-managers (excluding slim) default PAM rules
   disallow root auto login.

2. We can't use wayland

3. We have to use system-wide pulseaudio

4. It could break applications in the session.
   This happened to dolphin in plasma5
   in the past.

This is a growing technical debt, let's just use
passwordless sudo.
Use wrapGAppsHook as well
This adds the icon theme to XDG_DATA_DIRS.
It doesn't appear Plasma5 is properly configured for gtk
apps so this works around there being no icon theme installed
for it.
@samueldr
Copy link
Member

samueldr commented Aug 14, 2019

Just validated that sd_image works as expected.

@worldofpeace worldofpeace merged commit dd49cf7 into NixOS:master Aug 14, 2019
@worldofpeace worldofpeace deleted the installer/no-root branch August 14, 2019 15:21
@worldofpeace
Copy link
Contributor Author

Thanks everyone.

@flokli
Copy link
Contributor

flokli commented Aug 17, 2019

There was some documentation fixes missing in nixos/doc/manual/configuration/profiles/installation-device.xml - I included them in #63773 now.

etu added a commit to etu/nixpkgs that referenced this pull request Nov 11, 2019
It's not needed since NixOS#66338 and should have been done earlier.

This is based on a follow-up on NixOS#56167.
etu added a commit to etu/nixpkgs that referenced this pull request Nov 13, 2019
It's not needed since NixOS#66338 and should have been done earlier.

This is based on a follow-up on NixOS#56167.

(cherry picked from commit 4403cd1)
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Feb 19, 2020
It's not needed since NixOS#66338 and should have been done earlier.

This is based on a follow-up on NixOS#56167.

(cherry picked from commit 4403cd1)
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

6 participants