trezord: init at 1.2.0 #22054

canndrew · 2017-01-23T12:39:54Z

Motivation for this change

I want to use my trezor with NixOS.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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.

mention-bot · 2017-01-23T12:39:57Z

@canndrew, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @offlinehacker and @joachifm to be potential reviewers.

canndrew · 2017-01-23T12:42:51Z

I'm very much a NixOS noob and barely know what I'm doing. So be sure to review this carefully.

jb55 · 2017-01-23T17:58:16Z

pkgs/servers/trezord/default.nix

+stdenv.mkDerivation rec {
+  name = "trezord-${version}";
+
+  src = fetchgit {


use fetchFromGitHub here

Okay, I tried using fetchFromGitHub but for some reason it was grabbing a different version with a different hash which for some reason didn't build. So I've gone back to fetchgit.

fetchgit should only be necessary if you need submodules. Can you elaborate on the build error?

Ah yes, the repo does have submodules in it. That's what's causing it.

jb55 · 2017-01-23T17:59:00Z

pkgs/servers/trezord/default.nix

+    description = "TREZOR Bridge daemon for TREZOR bitcoin hardware wallet";
+    homepage = https://mytrezor.com;
+    license = licenses.gpl3;
+    maintainers = [];


if you don't want to maintain this, feel free to add jb55 since I use trezor on NixOS as well

I'll add both of us.

LnL7 · 2017-01-23T23:07:10Z

I noticed darwin related things in that patch, but it's marked as linux only.

joachifm · 2017-01-24T01:01:58Z

nixos/modules/misc/ids.nix

@@ -284,6 +284,7 @@
      glance = 266;
      couchpotato = 267;
      gogs = 268;
+      trezord = 269;


What sort of data does trezord own?

It controls access to the trezor.

But the trezor is a hardware device, correct? Static uids are for ensuring consistent ownership across NixOS deployments.

Or if you need to refer to a user before the name/uid mapping is available.

@joachifm How do I use a non-static uid then? Do I just not give a uid/gid to extraUsers/extraGroups?

Edit: That's what I've done and it seems to work.

joachifm · 2017-01-24T01:02:32Z

nixos/modules/services/hardware/trezord.nix

+        '';
+      };
+
+      user = mkOption {


What is the use case for running as a user other than the default?

None that I know of, but I thought I'd make it flexible.

To my mind, adding options "just because" is not real flexibility, but rather expands the configuration state space to no benefit. Absent a plausible use case for running a daemon under a different user, please re-consider whether it needs to be possible to do so. This holds doubly true for daemons that can work with dynamic uids, as down the line we can rely on systemd to do that for us, obviating the need to declare daemon users up-front for those services.

Alright, I've removed the ability to run the daemon as anything other than the trezord user.

canndrew · 2017-01-24T05:01:57Z

I noticed darwin related things in that patch, but it's marked as linux only.

The line is if (NOT ${CMAKE_SYSTEM_NAME} MATCHES "Darwin"), so it effects non-darwin systems.

joachifm · 2017-01-29T19:17:51Z

nixos/modules/services/hardware/trezord.nix

+      };
+    };
+
+    users.extraUsers = singleton {


Can also be written

users.users.trezord = { group = "trezord"; description = /* ... */; };

When name is null, it is taken from the attrset key.

joachifm · 2017-01-29T19:18:53Z

nixos/modules/services/hardware/trezord.nix

+      description = "Trezor bridge daemon user";
+    };
+
+    users.extraGroups = singleton {


Can also be written

let # for syntax hl users.groups.trezord = {};

joachifm · 2017-01-29T19:20:32Z

nixos/modules/services/hardware/trezord.nix

+      description = "TREZOR Bridge";
+      after = [ "systemd-udev-settle.service" "network.target" ];
+      wantedBy = [ "multi-user.target" ];
+      path = [ pkgs.trezord ];


Note that it is not necessary to add stuff to path, if it's just used in the ExecStart.

canndrew · 2017-02-03T07:33:28Z

Is this waiting for me to make changes or is okay to merge?

* Use fetchFromGitHub * Add maintainers

* Remove static uid/gid * Use fetchgit rather than fetchFromGitHub. It seems to be grapping a different version with fetchFromGGitHub for some reason. * Always create a user/group named trezord

canndrew · 2017-02-08T09:41:20Z

I've updated with @joachifm's suggestions. Hopefully this can be merged now.

jb55 suggested changes Jan 23, 2017

View reviewed changes

LnL7 added the 8.has: package (new) label Jan 23, 2017

joachifm reviewed Jan 24, 2017

View reviewed changes

jb55 approved these changes Jan 29, 2017

View reviewed changes

joachifm reviewed Jan 29, 2017

View reviewed changes

canndrew added 5 commits February 8, 2017 17:36

trezord: init at 1.2.0

b4afeb9

trezord: minor fixes

5e1b580

* Use fetchFromGitHub * Add maintainers

trezord: more fixes

d6abf9a

* Remove static uid/gid * Use fetchgit rather than fetchFromGitHub. It seems to be grapping a different version with fetchFromGGitHub for some reason. * Always create a user/group named trezord

trezord: Add canndrew to maintainers list

cdec386

trezord: update with joachifm's suggestions

4286bea

canndrew force-pushed the trezord branch from 8a73332 to 4286bea Compare February 8, 2017 09:40

joachifm merged commit 3082647 into NixOS:master Feb 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trezord: init at 1.2.0 #22054

trezord: init at 1.2.0 #22054

canndrew commented Jan 23, 2017 •

edited

mention-bot commented Jan 23, 2017

canndrew commented Jan 23, 2017

jb55 Jan 23, 2017

canndrew Jan 24, 2017

canndrew Jan 29, 2017

joachifm Jan 29, 2017

canndrew Jan 29, 2017

jb55 Jan 23, 2017

canndrew Jan 24, 2017

LnL7 commented Jan 23, 2017

joachifm Jan 24, 2017

canndrew Jan 24, 2017

joachifm Jan 24, 2017

joachifm Jan 24, 2017

canndrew Jan 29, 2017 •

edited

joachifm Jan 24, 2017

canndrew Jan 24, 2017

joachifm Jan 24, 2017 •

edited

canndrew Jan 29, 2017 •

edited

canndrew commented Jan 24, 2017

joachifm Jan 29, 2017

joachifm Jan 29, 2017

joachifm Jan 29, 2017

canndrew commented Feb 3, 2017

canndrew commented Feb 8, 2017

trezord: init at 1.2.0 #22054

trezord: init at 1.2.0 #22054

Conversation

canndrew commented Jan 23, 2017 • edited

Motivation for this change

Things done

mention-bot commented Jan 23, 2017

canndrew commented Jan 23, 2017

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

LnL7 commented Jan 23, 2017

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

canndrew Jan 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joachifm Jan 24, 2017 • edited

Choose a reason for hiding this comment

canndrew Jan 29, 2017 • edited

Choose a reason for hiding this comment

canndrew commented Jan 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

canndrew commented Feb 3, 2017

canndrew commented Feb 8, 2017

canndrew commented Jan 23, 2017 •

edited

canndrew Jan 29, 2017 •

edited

joachifm Jan 24, 2017 •

edited

canndrew Jan 29, 2017 •

edited