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

services/spacecookie: init #75103

Merged
merged 1 commit into from Dec 23, 2019
Merged

services/spacecookie: init #75103

merged 1 commit into from Dec 23, 2019

Conversation

@sternenseemann
Copy link
Member

@sternenseemann sternenseemann commented Dec 6, 2019

Motivation for this change

Add a NixOS service using the systemd socket activation for spacecookie, a gopher server already packaged by NixOS via haskellPackages.

Feedback on the service implementation is appreciated, this is my first time adding a NixOS service.

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 nix-review --run "nix-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.
@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 6, 2019

Just noticed, that spacecookie is marked broken currently because of hackage2nix choosing the wrong dependency versions. That probably should be resolved first, see #75119.

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from 306ea51 to accc214 Dec 6, 2019
Copy link
Contributor

@aanderse aanderse left a comment

Thanks for taking the time to contribute a module! Here is what I came up with on my first pass. Let me know if you need any clarification, etc...

nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from accc214 to ab17b3f Dec 6, 2019
@spacekookie
Copy link
Member

@spacekookie spacekookie commented Dec 7, 2019

Unrelated but I feel honoured to (almost) get my own mudule in NixOS 😅

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch 2 times, most recently from fee7760 to a632101 Dec 10, 2019
@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 10, 2019

I updated the pull-request to an improved version that would work with the current version of spacecookie.

I noticed a bit of an annoyance though: Due to a bug in spacecookie, it would always call setuid and setgid even if it doesn't run as root or already uses the right user/group. I already prepared a fix, also making the user config parameter optional, which I could release in the next few days I have released as 0.2.1.1. That update will hopefully be part of haskellPackages within the next few weeks.

Also I'd love to solve the DynamicUser-Problem, but I'm not sure, it's possible to setup a systemd socket that can be used by a DynamicUser.

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from a632101 to 26017d8 Dec 10, 2019
@dasJ
dasJ approved these changes Dec 10, 2019
@flokli
Copy link
Contributor

@flokli flokli commented Dec 10, 2019

@sternenseemann how does the service in the currently released version behave if started with this module?
Could you add a nixos test? This would allow to verify it keeps working, too.

You can definitely set User= and Group= with DynamicUser=yes. I don't really get what you mean by whether it's possible to use DynamicUser in combination with sockets - we're talking about a bind() here, not a file socket - so the user "owning the socket" shouldn't matter, if it has the capabilities to do the bind.

@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 10, 2019

@flokli With the current released version, it would work fine. The only annoyance is that after starting, it would call setUserID to switch to the uid of cfg.user which it already has been started as. This just does nothing, as spacecookie is already running as cfg.user.

A NixOS test is a great idea though, I'll look into it.

The problem is not setting up the socket in the services, but letting systemd set it up. Spacecookie launch can together with systemd be set up like this:

  1. spacecookie.socket is started. Systemd sets up an AF_INET6 socket and listens on a port.
  2. spacecookie.service is started and requests the socket from systemd.

With DynamicUser=true this leads to the problem, that the dynamic user hasn't yet been created when spacecookie.socket is set up, since the user is bound to spacecookie.service which is started after that.

From checking Lennart Poettering's blog post on the topic and checking the systemd.socket man page, it seems it only works with default SocketMode of 0666, so the dynamic user can access the socket later, but I don't think that's a good idea.

We could also ditch the spacecookie.socket unit, but then we'd have to start spacecookie as root, so it can bind on port 70, and rely on it calling setuid.

@flokli
Copy link
Contributor

@flokli flokli commented Dec 11, 2019

SocketMode only applies to file-based sockets, see systemd.socket:

  SocketMode=
      If listening on a file system socket or FIFO, this option specifies the file system access mode used when creating the file node. Takes an access mode in octal
      notation. Defaults to 0666.

I assume you want to bind to a port, so this shouldn't matter. You might need CAP_NET_BIND_SERVICE to bind on a lower port, though.

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from 26017d8 to eb68b41 Dec 12, 2019
@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 12, 2019

@flokli Oh yeah, completely missed that. I guess systemd then ensures, the socket can only be accessed by the corresponding service?

Binding is no problem though, since this is done by systemd, not spacecookie.

I have updated the service to work with the new spacecookie release and DynamicUser.

Still looking into a test, but we'll have to wait for the hackage-package-update anyways.

@dasJ can you have another look at the changed service/socket config?

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from eb68b41 to 31ece41 Dec 12, 2019
@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 12, 2019

Added the test as well. Until haskellPackages is updated test and service are broken. To test it locally you can use this patch.

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from 31ece41 to 1cc2277 Dec 12, 2019
Copy link
Member

@dasJ dasJ left a comment

Minor stuff, but the rest looks okay :shipit:

nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/spacecookie.nix Outdated Show resolved Hide resolved
@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from 1cc2277 to 855258c Dec 12, 2019
@aanderse
Copy link
Contributor

@aanderse aanderse commented Dec 12, 2019

So should we add serving the NixOS homepage over gopher to the 20.03 milestones list? 😆

@sternenseemann sternenseemann force-pushed the sternenseemann:spacecookie branch from 855258c to 25503db Dec 17, 2019
@sternenseemann
Copy link
Member Author

@sternenseemann sternenseemann commented Dec 17, 2019

With 6242cf6, spacecookie is no longer broken and has been updated to 0.2.1.1.

The module builds fine and the test succeeds, so if no other changes are requested, this could be merged.

@flokli
Copy link
Contributor

@flokli flokli commented Dec 17, 2019

I agree. Nice!

@GrahamcOfBorg test spacecookie

@flokli flokli merged commit eeaf1f7 into NixOS:master Dec 23, 2019
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
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
tests.spacecookie on aarch64-linux Success
Details
tests.spacecookie on x86_64-linux Success
Details
@sternenseemann sternenseemann deleted the sternenseemann:spacecookie branch Apr 14, 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

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