nixos/programs.mosh: refactor #41375

phryneas · 2018-06-01T22:43:22Z

Adds programs.mosh.withUtempter (default: true).

Motivation for this change

The option enables --with-utempter for mosh, allowing it to write to
/var/run/utmp and thus making connected sessions appear in the output
of who -a.

Things done

For that, a guid-wrapper is required. Also, the path to the utempter was
hardcoded in the resulting binary until now (so it could never been found),
thus, libutempter was patched accordingly to point to
/run/wrappers/bin/utempter which at least works when the wrapper is
configured.

I wrote it this way so that installing the package mosh by itself still is compiled without utempter as usually a user just installing the mosh package most likely just wants to use the client and will not have the wrapper installed.

In any case: compiling against utempter without having the wrapper in place has no negative consequences. /var/run/utmp will not be written, but mosh continues working.

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/)
Fits CONTRIBUTING.md.

phryneas · 2018-06-02T07:30:54Z

One thing to consider: I'm not 100% sure if this would trigger a hydra build with pkgs.mosh.override { inherit = true; } - if someone could confirm that or tell me on how to add it, I'd be gratefui.

xeji · 2018-06-02T18:26:55Z

nixos/modules/programs/mosh.nix

+        This is required so that mosh can write to /var/run/utmp (which can be queried with `who` to display currently connected user sessions).
+        Note, this will add a suid wrapper!
+      '';
+      default = true;


I'd prefer the default to be false since this option adds a setuid wrapper.

Actually, it's just a setguid-wrapper, also one for a group that doesn't really have many implications. I'll definitely have to change that comment from suid to guid.
With that knowledge, would you still prefer the false default?

The functionality of mosh logging to /var/run/utmp is something that I believe should be activated by default, as it is pretty much expected behaviour that you can see pending logged in users. (ssh doesn't have that problem since it's running a root daemon)

Fine, let's go with true as default.

xeji · 2018-06-02T18:34:55Z

pkgs/development/libraries/libutempter/default.nix

@@ -26,6 +27,17 @@ stdenv.mkDerivation rec {

  meta = {
    description = "Interface for terminal emulators such as screen and xterm to record user sessions to utmp and wtmp files";
+    longDescription = ''
+    Requires a wrapper to work.


This explanation only makes sense for NixOS systems. Please try to document the more general requirement (also for non-NixOS systems): utempter must be run as a user belonging to group utmp. On NixOS systems, this can be achieved by creating a setuid wrapper...

Ah, didn't think about non-NixOs users. You're right, thanks!

I have thought about this and I think it has more implications than just documentation.

You see, I'm patching the path of utempter from /var/lib/utempter/utempter to /run/wrappers/bin/utempter. So, as it currently stands, there would be a file with that path required, no matter if on NixOS or any other operating system.

So this might break other systems that had /var/lib/utempter/utempter present, maybe as part of the original operating system.
So I guess, I should also make another modification that this path is only patched for NixOs targets. Would this be the preferred way to go?

You see, I'm patching the path of utempter from /var/lib/utempter/utempter to /run/wrappers/bin/utempter.

Correct, I missed that. libutempter should not be patched at all. nixpkgs packages must be self-contained and work on all supported systems (NixOs or other) without any implicit assumptions like the existence of such wrappers.

So I guess, I should also make another modification that this path is only patched for NixOs targets. Would this be the preferred way to go?

No, as explained above. But maybe you can patch mosh to search for utempter in PATH. That would work on both NixOS and non-nixOS systems: on NixOS systems, /run/wrappers/bin is in PATH, and on non-NixOS systems the user can add a setgit'd copy of utempter to their PATH. But you need to be able to install and use the mosh package with or without the corresponding module.

The path to the utempter binary is hard-coded (in libutempter itself, not in mosh) to a path that is usually not part of $PATH on other systems. So, I could patch libutempter to look in $PATH, and that would work on NixOS, but that again would fail on other systems, because on them the binary would be found on a OS-specific path like /var/lib/utempter/utempter, which is usuallynot found on PATH :/

I fail to see a way that this will work in NixOS and other OSes. The way it currently is, it will work most likely in other OSes, but only if they didn't change the path to the utempter binary on their distribution, and libutempter is installed.

The path to the utempter binary is hard-coded (in libutempter itself, not in mosh) to a path that is usually not part of $PATH on other systems.

Looking at the libutempter source code and derivation, only a default path is hardcoded to ${out}/lib/utempter/utempter which is in the nix store and will not work on any system.

But libutempter provides an API function utempter_set_helper() to set a path to the helper binary. The application must call it to set a path to a setgid version of that helper (or setgid wrapper) because the default path is useless. So you have two options:

patch mosh to call utempter_set_helper() in a way that works on both NixOS and other systems.

patch libutempter to look for the helper binary in PATH (or at a sensible location outside the nix store) by default.

The only other nixpkgs package currently using libutempter is guake. It doesn't do any related patching, so (without having tested it) I assume utempter doesn't work with it anyway. But if you patch libutempter please verify that you don't break guake.

Anyway, it's up to you to decide if the utmp functionality is worth the effort.

I guess I'll undo the patch to libutempter, modify the code of mosh to call utempter_set_helperto use utempter from the PATH, and wrap the generated mosh binary, adding all three /run/wrappers/bin/utempter, /var/lib/utempter/utempterand ${out}/lib/utempter/utempter (in that priority) to PATH.
I guess that should cover as many bases as possible.

But I'll be gone for the week, so I'll have to continue this the week after. So expect this to take a while.

xeji · 2018-06-02T18:42:01Z

nixos/modules/programs/mosh.nix

  };

  config = mkIf cfg.enable {
-    environment.systemPackages = with pkgs; [ mosh ];
+    environment.systemPackages = [ ( pkgs.mosh.override { inherit (cfg) withUtempter; } ) ];


This will trigger a local rebuild of mosh if withUtempter=true; because Hydra only builds the default version.

You could add mosh-with-utempter = mosh.override { withUtempter = true; } to pkgs/top-level/all-packages.nix to have Hydra build that version too.

Or you can build mosh with utempter support by default, provided it doesn't break when the setuid wrapper is missing.

Yup, I feared as much. Will change that, thanks :)

In any case: compiling against utempter without having the wrapper in place has no negative consequences. /var/run/utmp will not be written, but mosh continues working.

Then I believe it is better to build mosh with utempter support by default, so we don't have to maintain two variants. It shouldn't add much to closure size, and simplifies the module.

fine, just ping me when it's ready.

phryneas · 2018-06-06T19:15:00Z

This should cover everything discussed above. I'll check quake later today or tomorrow. Also I'll get a mac user I know to try this to make sure it doesn't break anything over there.

If that went well and everything looks okay to you @xeji I'll do a final rebase. (I just did a commit so you could better review what I changed)

phryneas · 2018-06-06T20:41:36Z

feedback 1: guake compiles & runs fine.

xeji · 2018-06-07T12:15:22Z

pkgs/tools/networking/mosh/default.nix

+    wrapProgram $out/bin/mosh-server \
+      --prefix PATH : "/run/wrappers/bin" \
+      --suffix PATH : "${libutempter}/lib/utempter"
+  '';


This wrapProgram doesn't make much sense to me, I think it can be deleted:
On NixOS systems, /run/wrappers/bin is already in PATH.
On non-NixOS systems, putting ${libutempter}/bin/utempter in PATH is useless because it's in the nix store and lacks the privileges (setgid). In this case the user must manually copy the helper program to a location in PATH but outside the nix store, and set ownership and permissions accordingly. We cannot automate this.

You're absolutely right, I was thinking way too complicated there. Removed it.

xeji · 2018-06-08T17:00:31Z

@GrahamcOfBorg build mosh libutempter

GrahamcOfBorg · 2018-06-08T17:01:09Z

No attempt on x86_64-darwin (full log)

The following builds were skipped because they don't evaluate on x86_64-darwin: mosh, libutempter

Partial log (click to expand)


a) For `nixos-rebuild` you can set
  { nixpkgs.config.allowUnsupportedSystem = true; }
in configuration.nix to override this.

b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
  { allowUnsupportedSystem = true; }
to ~/.config/nixpkgs/config.nix.

GrahamcOfBorg · 2018-06-08T17:02:03Z

Success on x86_64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2
shrinking /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/mosh-client
shrinking /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/mosh-server
gzipping man pages under /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/share/man/
strip is /nix/store/21ymadblbmsbb2bk4q7gl4kjasp8zmgd-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin
patching script interpreter paths in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2
/nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/.mosh-wrapped: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/qrsbpchqw6xkfjk74cvg7p3h0pz3agzy-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2...

GrahamcOfBorg · 2018-06-08T17:03:55Z

Success on aarch64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

shrinking /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2/bin/mosh-server
shrinking /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2/bin/mosh-client
gzipping man pages under /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2/share/man/
strip is /nix/store/qg2agrqkf240s656d207zqhipl0bc2id-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2/bin
patching script interpreter paths in /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2
/nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2/bin/.mosh-wrapped: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/ich2f3n52l6jhdsfhas93zqpkyss9cca-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2...
/nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2
/nix/store/gb6lmny3a4qbr5nwxah1wjjn7ij6z5qq-libutempter-1.1.6

xeji · 2018-06-08T17:16:28Z

Thank you, LGTM.

phryneas · 2018-06-08T18:06:42Z

@xeji: wait a second, we just noticed that this breaks builds for darwin (libutempter is linux-only)

xeji · 2018-06-08T18:29:23Z

pkgs/tools/networking/mosh/default.nix

-, makeWrapper, perl, openssl, autoreconfHook, openssh, bash-completion }:
+{ lib, stdenv, fetchurl, zlib, protobuf, ncurses, pkgconfig, IOTty
+, makeWrapper, perl, openssl, autoreconfHook, openssh, bash-completion
+, libutempter, withUtempter ? true }:


libutempter ? null, withUtempter ? stdenv.isLinux should fix mosh on darwin

thanks :) let's see if the borg builds correctly and then I'll squash this for merging :)

you don't need to squash, I can do that when merging

xeji · 2018-06-08T18:43:17Z

@GrahamcOfBorg build mosh libutempter

GrahamcOfBorg · 2018-06-08T18:43:39Z

Success on aarch64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

/nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2
/nix/store/gb6lmny3a4qbr5nwxah1wjjn7ij6z5qq-libutempter-1.1.6

GrahamcOfBorg · 2018-06-08T18:44:31Z

Success on x86_64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

shrinking /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/mosh-client
shrinking /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/mosh-server
gzipping man pages under /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/share/man/
strip is /nix/store/21ymadblbmsbb2bk4q7gl4kjasp8zmgd-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin
patching script interpreter paths in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2
/nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2
/nix/store/4y0xkhxqp2g8x390d4xjdqrd75p9xa4k-libutempter-1.1.6
/nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2/bin/.mosh-wrapped: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/qrsbpchqw6xkfjk74cvg7p3h0pz3agzy-perl-5.24.3/bin/perl"
checking for references to /build in /nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2...

GrahamcOfBorg · 2018-06-08T18:44:43Z

Success on x86_64-darwin (full log)

Attempted: mosh

The following builds were skipped because they don't evaluate on x86_64-darwin: libutempter

Partial log (click to expand)

make[3]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
make[2]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
make[1]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
post-installation fixup
gzipping man pages under /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/share/man/
strip is /nix/store/yyak5sjv68n9vdgnkd2cb5djk1l9sqj5-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/bin
patching script interpreter paths in /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2
/nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/bin/.mosh-wrapped: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/zhg6b797gc7iqaimph1lvw971npmcx8m-perl-5.24.3/bin/perl"
/nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2

xeji · 2018-06-08T18:55:23Z

looks like your rebase went wrong, these other commits don't belong here

Adds programs.mosh.withUtempter (default: true). The option enables -with-utempter for mosh, allowing it to write to /var/run/utmp and thus making connected sessions appear in the output of `who -a`. For that, a guid-wrapper is required. Also, the path to the `utempter` was hardcoded in the resulting binary until now (so it could never been found), thus, libutempter was patched accordingly to point to /run/wrappers/bin/utempter which at least works when the wrapper is configured.

phryneas · 2018-06-08T18:58:00Z

yup, just noticed that myself. should be better now.

phryneas · 2018-06-08T19:23:41Z

@GrahamcOfBorg build mosh libutempter

phryneas · 2018-06-08T19:24:07Z

nah, doesn't listen to me xD

LnL7 · 2018-06-08T19:40:00Z

@GrahamcOfBorg build mosh libutempter

GrahamcOfBorg · 2018-06-08T19:40:22Z

Success on aarch64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

/nix/store/222b2kv4bxbf12w051xybnka1db7llyb-mosh-1.3.2
/nix/store/gb6lmny3a4qbr5nwxah1wjjn7ij6z5qq-libutempter-1.1.6

GrahamcOfBorg · 2018-06-08T19:41:05Z

Success on x86_64-linux (full log)

Attempted: mosh, libutempter

Partial log (click to expand)

/nix/store/bfn20yq7cqcy8cfl33rj2a1j6ri5myc9-mosh-1.3.2
/nix/store/4y0xkhxqp2g8x390d4xjdqrd75p9xa4k-libutempter-1.1.6

GrahamcOfBorg · 2018-06-08T19:46:46Z

Success on x86_64-darwin (full log)

Attempted: mosh

The following builds were skipped because they don't evaluate on x86_64-darwin: libutempter

Partial log (click to expand)

make[3]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
make[2]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
make[1]: Leaving directory '/private/tmp/nix-build-mosh-1.3.2.drv-0/mosh-1.3.2'
post-installation fixup
gzipping man pages under /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/share/man/
strip is /nix/store/yyak5sjv68n9vdgnkd2cb5djk1l9sqj5-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/bin
patching script interpreter paths in /nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2
/nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2/bin/.mosh-wrapped: interpreter directive changed from "/usr/bin/env perl" to "/nix/store/zhg6b797gc7iqaimph1lvw971npmcx8m-perl-5.24.3/bin/perl"
/nix/store/j0ax2l8v7dpkznakgwi7ml76md6xj6bz-mosh-1.3.2

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Jun 1, 2018

xeji reviewed Jun 2, 2018

View reviewed changes

xeji added the 2.status: work-in-progress label Jun 3, 2018

GrahamcOfBorg added 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 labels Jun 6, 2018

xeji reviewed Jun 7, 2018

View reviewed changes

xeji removed the 2.status: work-in-progress label Jun 8, 2018

xeji reviewed Jun 8, 2018

View reviewed changes

phryneas force-pushed the mosh-with-libutempter branch from 546a809 to 08f9410 Compare June 8, 2018 18:54

phryneas force-pushed the mosh-with-libutempter branch from 08f9410 to 7c34c28 Compare June 8, 2018 18:57

xeji merged commit 951d3cc into NixOS:master Jun 8, 2018

phryneas deleted the mosh-with-libutempter branch June 8, 2018 20:03

nixos/programs.mosh: refactor #41375

nixos/programs.mosh: refactor #41375

Conversation

phryneas commented Jun 1, 2018

Motivation for this change

Things done

phryneas commented Jun 2, 2018

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

xeji Jun 2, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Jun 6, 2018

phryneas commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeji commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

xeji commented Jun 8, 2018

phryneas commented Jun 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeji commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

xeji commented Jun 8, 2018

phryneas commented Jun 8, 2018

phryneas commented Jun 8, 2018

phryneas commented Jun 8, 2018

LnL7 commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

GrahamcOfBorg commented Jun 8, 2018

xeji Jun 2, 2018 •

edited

phryneas commented Jun 8, 2018 •

edited