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

Jump to bottom

nixos/supybot: switch to python3, enable systemd sandboxing, add option for installing plugins #79851

Merged
merged 3 commits into from Mar 17, 2020
Merged

nixos/supybot: switch to python3, enable systemd sandboxing, add option for installing plugins #79851

merged 3 commits into from Mar 17, 2020

Conversation

mmilata
Copy link
Member

@mmilata mmilata commented Feb 11, 2020

Adds several enhancements to the supybot module:

  • switch to python3
  • move default state directory under /var/lib where it belongs
  • enable systemd sandboxing options
  • add options for declaratively installing plugins and their python dependencies
  • use tmpfiles for stateDir management
Motivation for this change
  • upstream doesn't list python2.7 as supported
  • sandboxing is desirable for network service processing input from untrusted sources using various third-party plugins
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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.

cc @cillianderoiste

@mmilata mmilata force-pushed the supybot-enhancements branch 2 times, most recently from f2e917a to a2e7c90 Compare February 25, 2020 15:51
aanderse
aanderse reviewed Mar 8, 2020
View reviewed changes
nixos/modules/services/networking/supybot.nix
description = "The root directory, logs and plugins are stored here";
};

configFile = mkOption {
type = types.path;
description = ''
Path to a supybot config file. This can be generated by
Path to initial supybot config file. This can be generated by
Copy link
Member

Choose a reason for hiding this comment

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

Why is this module coded in a way that the configuration file is initial only? 😞

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question! I think that most supybot operators generate initial configuration with supybot-wizard and afterwards configure it by sending it commands over IRC. Even the user guide discourages editing the config file.

I was considering adding mutableConfig option (true by default to be backwards compatible) but figured not much people would use it. It's a simple change though, can add it if it makes sense.

@mmilata
nixos/supybot: stateDir in /var/lib, use tmpfiles
b150e08 
Moving the stateDir is needed in order to use ProtectSystem=strict
systemd option.
@mmilata
nixos/supybot: enable systemd sandboxing options
57f5fb6
@mmilata
nixos/supybot: python3 switch, add plugin options
1affd47 
Python2 seems to be no longer supported by limnoria upstream.
@mmilata
Copy link
Member Author

mmilata commented Mar 9, 2020

Fixed merge conflict & added mutableConfig. The user experience is not great when it's false though, sending configuration command to the bot appears as if the configuration was saved but it isn't. At least there's a stacktrace in the logs.

@aanderse
Copy link
Member

@mmilata I finally took the time to (briefly) read some supybot documentation as I had never heard of this program before. I think this program does not "flow" with the way NixOS works at all when it comes to configuration. It seems like our (NixOS) definition of "configuration" means "mutable state" to upstream - and that is OK. To me having an immutable config feels like trying to make supybot something it is not. The program just isn't designed that way.

If a NixOS module didn't currently exist and I wanted to use this program I would likely write it to use DynamicUser and consider all config/state a black box NixOS shouldn't touch at all, entirely managed by supybot itself. I would probably only have 2 options: enable and something like initialConfig (the "initial" part being emphasized), which documents the sysadmin should simply run supybot-wizard and reference the file.

Sorry for mutableConfig option suggestion, now that I understand the program better I think that option probably isn't so useful...

That being said... I have never ran supybot, nor ever plan to. I think this PR is likely fine, but it probably makes sense for someone with more supybot experience than myself (ie. more than zero) to review this quickly too.

@mmilata
Copy link
Member Author

mmilata commented Mar 16, 2020

@aanderse thanks for looking at this. Nowadays I probably wouldn't choose supybot either but don't want to spend the time rewriting plugins that I use with it and that work well.

Dropped the mutableConfig option. Adding DynamicUser might be a good idea, though there probably should be a way to turn it off in the case it breaks some plugin - there's large amount of them and most of them are going to work fine with it but probably not all. Or for the sake of compatibility with the previous version of the module. Would that warrant adding an option?

@aanderse
Copy link
Member

I keep seeing "I would love to use DynamicUser but can't because it might break some specific undetermined workflow" arguments come up... My guess is that we'll see some patterns emerge and hopefully a unified solution to that problem will be presented in nixpkgs. So for now maybe just without DynamicUser unless you're overly compelled to add it.

Would be nice if someone familiar with this module could review.

@cillianderoiste cillianderoiste merged commit 5241e5a into NixOS:master Mar 17, 2020
@mmilata mmilata deleted the supybot-enhancements branch March 17, 2020 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse left review comments

@cillianderoiste cillianderoiste cillianderoiste approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mmilata @aanderse @cillianderoiste