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

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

Merged
merged 3 commits into from Mar 17, 2020

Conversation

@mmilata
Copy link
Member

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

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

This comment has been minimized.

Copy link
@aanderse

aanderse Mar 8, 2020

Contributor

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

This comment has been minimized.

Copy link
@mmilata

mmilata Mar 8, 2020

Author Member

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 added 3 commits Feb 11, 2020
Moving the stateDir is needed in order to use ProtectSystem=strict
systemd option.
Python2 seems to be no longer supported by limnoria upstream.
@mmilata mmilata force-pushed the mmilata:supybot-enhancements branch from a2e7c90 to b7c3075 Mar 9, 2020
@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
Contributor

aanderse commented Mar 14, 2020

@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 mmilata force-pushed the mmilata:supybot-enhancements branch from b7c3075 to 1affd47 Mar 16, 2020
@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
Contributor

aanderse commented Mar 17, 2020

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
13 checks passed
13 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@mmilata mmilata deleted the mmilata:supybot-enhancements branch Mar 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.