Add support for burp client/server services #48762

unode · 2018-10-20T23:21:20Z

Motivation for this change

BURP is currently available in nixpkgs but provides no services

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.

This is a reiteration of #30553 but basing on unode/nixpkgs:burp instead of unode/nixpkgs:master to avoid accidental mixing of work. Unfortunately GitHub doesn't allow editing the head branch.

~~This setup is mostly functional but there is currently a problem with validity of the SSL certificates after pairing - see grke/burp#769.~~ If anyone wants to give this a try feedback is welcome.

unode · 2018-11-04T11:57:37Z

Rebased to fix conflicts with master.

Pinging @tokudan @fpletz @ryantm as potential reviewers based on maintenance of burp.

tokudan · 2018-11-04T18:42:30Z

nixos/modules/services/backup/burp.nix

+        type = types.str;
+        default = "burp";
+        description = ''
+          Run server as user


Is this the client or the server config?

The server but I'm not sure I understand what the question is getting at.

nixos/modules/services/backup/burp.nix

tokudan · 2018-11-04T19:16:07Z

nixos/modules/services/backup/burp.nix

+
+      preStart = ''
+        if [ ! -f "${cfg.server.ssl_cert}" ]; then
+          mkdir -m 0750 -p ${cfg.server.libDir}


Why allow group read access to that directory?

tokudan · 2018-11-04T19:23:46Z

nixos/modules/services/backup/burp.nix

+          # See also https://github.com/grke/burp/issues/620
+          cp -f ${burp_conf} ${cfg.client.libDir}/burp.conf
+
+          # This may fail so keep trying until we get ${cfg.client.ssl_cert} from the server


As this may fail there should be some kind of throttle on it. Having this fail more than once a minute probably doesn't make any sense.

Any suggestion on how to achieve that? I've seen this pattern used in other services but didn't notice a throttling mechanism.

unode · 2018-11-13T23:08:05Z

All points but the throttling have been addressed. I'm unsure how to proceed on that front.
By default systemd already gives up after repeated unsuccessful starts.

tokudan · 2018-11-13T23:32:32Z

If systemd gives up or throttles on its own after a few attempts, that's probably good enough.
I wasn't aware that systemd does that, if the service is set to restart automatically.

Looks good to me. I think it's ready for merge.

infinisil · 2019-01-28T21:12:10Z

Not sure what I think of adding another 50+ options. Every options currently deincreases evaluation time for all NixOS users (whether they use the options or not), lots of options aren't very maintainable (upstream defaults and options change).

As an alternative, check out what I did in #45470, and try to apply the same strategy to this PR. This can reduce the option count to a minimum while still being able to set all current and future config values conveniently, but giving up having them all documented and type checked by the module system.

unode · 2019-01-28T21:25:42Z

Every options currently decreases evaluation time for all NixOS users

I'm guessing you mean "increases" there 😀 otherwise this would actually be a good thing.

I was not aware of this being an issue. Most of the options aren't actually required to start the service.
I could very easily move everything non-essential to extraConfig.

As for the solution you linked, was that a one-off or is this a pattern that is growing in popularity?

infinisil · 2019-01-28T22:00:53Z

I'm guessing you mean "increases" there grinning otherwise this would actually be a good thing.

Yes thanks, fixed now :)

I could very easily move everything non-essential to extraConfig.

That would be fine too by me

As for the solution you linked, was that a one-off or is this a pattern that is growing in popularity?

Well, I'm very much a favor of this approach, but I've only seen it used a couple other times, it's not very popular. I'm encouraging people to use it though. I'm thinking about writing some nixpkgs tooling to make writing such config options easier, by providing standard types and converters for common config file formats like JSON/YAML/INI. I've also started writing some general guidelines for writing NixOS services which should limit the amount of options one needs to use and increase code reuse.

pstn · 2019-07-18T10:28:28Z

@infinisil What pattern are you talking off exactly? Is it documented? I'm willing to put some time in to get this merged.

tokudan · 2019-07-18T20:22:27Z

I believe one of the issues is that burp's config looks like an INI file, but isn't. It must support multiple lines with the same key for some options like "keep". So this would need a special converter.
So the question is actually: Do we want a module that's basically ready to merge or do we reject it and ask for a rewrite of a special converter that's a lot more complicated than just having some additional options.
I think merging the options is better.

infinisil · 2019-07-21T22:08:20Z

I've since opened an RFC for this, see NixOS/rfcs#42. I'm currently in the process of rewriting it to address the discussions.

The second part of it is that it encourages people to keep option counts low. I think it's always a good idea to start with options that make sense for most people and tell users to use a structural config/settings option with a flexible type for when they need more (that's the first part of the RFC). More options can always be added later if they're really needed, in contrast options can't really be removed once added. See also here: NixOS/rfcs#42 (comment)

And while yes, the RFC isn't accepted yet (although it most likely will be eventually), and the general convenience functions aren't yet written, I'm still encouraging that approach, because it just makes so much sense. Every module I merge that doesn't adhere to that approach is a module that can't really be changed to that approach ever. So I'm not really feeling comfortable merging ones like this (which use extraConfig and have loads of options).

unode · 2019-10-07T11:21:09Z

I've removed some non-essential options. Didn't yet manage to implement @infinisil 's suggestion above.

2 checks are now failing but this seems to be unrelated, at least the previous commit fails locally as well.

tokudan · 2020-03-08T17:21:38Z

It's been some time, but with 20.03 split off, maybe this could be merged if it still works?

unode · 2020-03-09T20:13:18Z

I've been using it on my laptop without issues ever since I opened the PR.

stale · 2021-06-09T00:40:29Z

I marked this as stale due to inactivity. → More info

unode · 2021-06-13T19:36:02Z

I'm still using the original recipe and would like to get it merged at some point.
Will revisit once 21.05 lands on my system.

SuperSandro2000 · 2021-06-14T12:32:03Z

nixos/modules/misc/ids.nix

@@ -346,6 +346,7 @@ in
      zoneminder = 314;
      paperless = 315;
      #mailman = 316;  # removed 2019-08-30
+      burp = 317;


Can we use dynamic users?

I'm not familiar with this feature. Do you have a link I could read up on? Thanks!

IIRC dynamic users potentially requires a chown run on start of the service, which is not practical on a big data store with potentially millions of files, so I don't think using dynamic users feasable for burp.
http://0pointer.net/blog/dynamic-users-with-systemd.html

nixos/modules/services/backup/burp.nix

SuperSandro2000 · 2021-06-14T12:32:33Z

nixos/modules/services/backup/burp.nix

+      enable = mkEnableOption "BURP client";
+
+      frequency = mkOption {
+        type = types.str;


Should this be an enum?

This translates to systemd OnCalendar time specifications. A enum may restrict more than desired.

nixos/modules/services/backup/burp.nix

stale · 2022-01-09T00:06:55Z

I marked this as stale due to inactivity. → More info

unode · 2022-01-09T14:39:40Z

Addressed the conflict, which is likely to reappear if new services are added.

Working fine on 21.05.

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

infinisil · 2022-03-01T19:56:34Z

nixos/modules/misc/ids.nix

@@ -353,6 +353,7 @@ in
      distcc = 321;
      webdav = 322;
      pipewire = 323;
+      burp = 324;


Static ids shouldn't be used anymore, see NixOS/rfcs#52. This was also previously discussed in #48762 (comment), but I don't think that's a problem, because no chown needs to be run in normal circumstances, since systemd reuses the same dynamic uids for users if possible.

infinisil · 2022-03-01T19:57:00Z

nixos/modules/module-list.nix

@@ -281,6 +281,8 @@
  ./services/backup/btrbk.nix
  ./services/backup/duplicati.nix
  ./services/backup/duplicity.nix
+  ./services/backup/burp.nix
+  ./services/backup/crashplan.nix


That looks wrong

infinisil · 2022-03-01T20:02:09Z

nixos/modules/services/backup/burp.nix

+  burp_conf = pkgs.writeText "burp.conf" ''
+    mode = client
+    pidfile = /run/burp/burp.pid
+
+    port = ${toString cfg.client.port}


Already mentioned this in #48762 (comment), but by now we should really be using the approach from NixOS/rfcs#42 for this, see also #144575. By now there are a lot of NixOS modules which make use of that approach which you can use for reference. Grep for freeformType in nixpkgs to see them. You can also check out the documentation.

infinisil · 2022-03-01T20:04:07Z

nixos/modules/services/backup/burp.nix

+    };
+  };
+
+  config = mkIf cfg.client.enable or cfg.server.enable {


or isn't a boolean operator in Nix, check out the list of operators. You meant to use || here instead. Also needs parenthesis.

arjan-s · 2022-12-26T20:35:17Z

Are you still planning to work on this. @unode ? I could really use it. :)

unode · 2022-12-29T12:33:57Z

I'm still using the original recipe and didn't get to implement the suggested approach.
Feel free to push or suggest changes. I don't think I'll be able to get to it anytime soon.

This was referenced Oct 20, 2018

WIP: Add support for burp client/server services #30553

Closed

Certificate validation problems after pairing grke/burp#769

Closed

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Oct 20, 2018

unode force-pushed the burp branch 2 times, most recently from 7b1a9c5 to 0f519b4 Compare October 21, 2018 00:14

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Oct 21, 2018

unode force-pushed the burp branch from 0f519b4 to d70bd0f Compare October 21, 2018 00:26

unode changed the title ~~WIP: Add support for burp client/server services~~ Add support for burp client/server services Oct 21, 2018

unode force-pushed the burp branch from d70bd0f to a1f89fa Compare October 21, 2018 23:28

unode force-pushed the burp branch from a1f89fa to bd37a53 Compare November 4, 2018 11:13

tokudan reviewed Nov 4, 2018

View reviewed changes

nixos/modules/services/backup/burp.nix Outdated Show resolved Hide resolved

tokudan reviewed Nov 4, 2018

View reviewed changes

unode force-pushed the burp branch from bd37a53 to fca4b5c Compare November 13, 2018 23:06

unode requested a review from infinisil as a code owner November 13, 2018 23:06

unode force-pushed the burp branch from 51739a1 to 5378322 Compare October 6, 2019 19:14

unode force-pushed the burp branch from 5378322 to d3faad9 Compare April 26, 2020 18:55

ryantm added the 2.status: merge conflict label Oct 3, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 9, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 13, 2021

SuperSandro2000 requested changes Jun 14, 2021

View reviewed changes

stale bot added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md and removed 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md labels Jan 9, 2022

ofborg bot removed the 2.status: merge conflict label Jan 9, 2022

unode and others added 6 commits January 9, 2022 16:16

Add support for burp services

0a86bcc

Update nixos/modules/services/backup/burp.nix

f1095f8

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

Update nixos/modules/services/backup/burp.nix

8b9fed8

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

Update nixos/modules/services/backup/burp.nix

2ffd57e

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

Update nixos/modules/services/backup/burp.nix

5e451d0

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>

Update nixos/modules/services/backup/burp.nix

8fde142

unode force-pushed the burp branch from d4f8d5e to 8fde142 Compare January 9, 2022 15:17

infinisil requested changes Mar 1, 2022

View reviewed changes

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 21, 2022

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 26, 2022

Artturin marked this pull request as draft February 2, 2023 18:52

wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for burp client/server services #48762

Add support for burp client/server services #48762

unode commented Oct 20, 2018 •

edited

unode commented Nov 4, 2018

tokudan Nov 4, 2018

unode Jun 14, 2021

tokudan Nov 4, 2018

tokudan Nov 4, 2018

unode Nov 5, 2018

unode commented Nov 13, 2018

tokudan commented Nov 13, 2018

infinisil commented Jan 28, 2019 •

edited

unode commented Jan 28, 2019

infinisil commented Jan 28, 2019

pstn commented Jul 18, 2019

tokudan commented Jul 18, 2019 •

edited

infinisil commented Jul 21, 2019

unode commented Oct 7, 2019

tokudan commented Mar 8, 2020

unode commented Mar 9, 2020

stale bot commented Jun 9, 2021

unode commented Jun 13, 2021

SuperSandro2000 Jun 14, 2021

unode Jun 14, 2021

tokudan Jun 14, 2021 •

edited

SuperSandro2000 Jun 14, 2021

unode Jun 14, 2021 •

edited

stale bot commented Jan 9, 2022

unode commented Jan 9, 2022

infinisil Mar 1, 2022

infinisil Mar 1, 2022

infinisil Mar 1, 2022

infinisil Mar 1, 2022

arjan-s commented Dec 26, 2022

unode commented Dec 29, 2022

Add support for burp client/server services #48762

Are you sure you want to change the base?

Add support for burp client/server services #48762

Conversation

unode commented Oct 20, 2018 • edited

Motivation for this change

Things done

unode commented Nov 4, 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

unode commented Nov 13, 2018

tokudan commented Nov 13, 2018

infinisil commented Jan 28, 2019 • edited

unode commented Jan 28, 2019

infinisil commented Jan 28, 2019

pstn commented Jul 18, 2019

tokudan commented Jul 18, 2019 • edited

infinisil commented Jul 21, 2019

unode commented Oct 7, 2019

tokudan commented Mar 8, 2020

unode commented Mar 9, 2020

stale bot commented Jun 9, 2021

unode commented Jun 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokudan Jun 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unode Jun 14, 2021 • edited

Choose a reason for hiding this comment

stale bot commented Jan 9, 2022

unode commented Jan 9, 2022

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

arjan-s commented Dec 26, 2022

unode commented Dec 29, 2022

unode commented Oct 20, 2018 •

edited

infinisil commented Jan 28, 2019 •

edited

tokudan commented Jul 18, 2019 •

edited

tokudan Jun 14, 2021 •

edited

unode Jun 14, 2021 •

edited