[Nix-dev] Re: Patches for a NixOS Tor service

[Ludovic Courtès]

Hi,

roconnor at theorem.ca writes:

> Here are the patches needed to create a tor service for NixOS.  It's a
> bit preliminary at the moment, but the main funcitonality is there.

This looks great!

Some comments:

+    services.tor = {
+
+      enable = mkOption {
+        default = false;
+        description = ''
+          Whether to enable the tor socks proxy.
+        '';

How about something more ‘descriptive’, such as “Whether to enable the
Tor anonymous routing daemon”?

(‘Tor’ should be capitalized.)

+      config = mkOption {
+        default = "";
+        description = ''
+          Extra configuration. Contents will be added verbatim to the configuration file.
+        '';

This kind of option should really be a last resort IMO.  Are the most
common options covered by the rest of this file?

+      enablePrivoxy = mkOption {
+        default = true;
+        description = ''
+          Whether to enable the tor instance of privoxy.

Does it mean “an instance of Privoxy running on top of Tor”?

+          For anonymity protocols need to be scrubbed of identifying information.

This sentence lacks a subject.  :-)

+          HTTP is the most common protocol that is used so by default we enable an
+          instance of privoxy specifically for tor so that it won't conflict with the
+          standard NixOS privoxy instance.

I don’t understand the connection between “HTTP is the most common
protocol” and “we enable an instance of”.  Can you clarify?

+          For example, If you are only going to use tor as a relay, then you can disable this option

This sentence lacks a period, and the meaning is also unclear to me.

Generally it would nice if lines were wrapped to 80 characters IMO.

Besides, it would be ideal if you could write a test case for this
module.

--- pkgs/tools/security/tor/default.nix (revision 0)
+++ pkgs/tools/security/tor/default.nix (revision 0)
@@ -0,0 +1,12 @@
+{stdenv, fetchurl, libevent, openssl, zlib}:
+
+stdenv.mkDerivation {
+  name = "tor-0.2.1.25";
+
+  src = fetchurl {
+    url = "http://www.torproject.org/dist/tor-0.2.1.25.tar.gz";
+    sha256 = "17hpnvlqimblgprx6qwv8akqy5ric08m0265rl4zm1jnxa4v8n47";
+  };
+
+  buildInputs = [libevent openssl zlib];
+}

Can you add a ‘meta’ field?  ‘doCheck = true’ would be a nice bonus.

Thanks,
Ludo’.