nixos/forgejo: Small cleanups and service capability changes

[pyrox0]

Also resolves #306205 and adds myself as a maintainer to the module.

Note: This won't eval cleanly until #306349, which fixes my maintainer attribute name, is merged. However, I've checked and it does work properly with that merged into the tree.

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[emilylange]

Regarding #306457 (comment):

See result of nix-build -A nixosTests.forgejo.sqlite3 (or CI)
@ofborg build nixosTests.forgejo.sqlite3

[pyrox0]

Had to rebase, CI should be green now.

[bendlas]

Has the capabilities change been mixed in accidentally?

Feels like this should be a separate PR, because it's security-relevant and hasn't been mentioned in this PR before.

[bendlas]

Retracting my change request, since I'm realizing that this is mainly about using the built-in ssh server. That's a use case that I simply don't have, but happen to know that @emilylange does. So I'll trust her expertise on this.

I'd still like to leave one self-quote from the resolved thread for posterity, because I feel like it meaningfully adds to the conversation.

Even when running a server without a reverse proxy, I prefer binding to high ports and then just redirecting the appropriate low ports via firewall.

See comment

[mweinelt]

I'm slightly worried about unconditionally handing out capabilities, that are not strictly required. My deployment currently only binds to a UNIX socket, and I honestly don't want it to be able bind to privileged ports by default.

# ss -lpn | grep forgejo
u_str LISTEN 0      128                     /run/forgejo/forgejo.sock 22892                  * 0    users:((".gitea-wrapped",pid=1603,fd=11))    

[emilylange]

It's a bit unfortunate the CAP discussion/suggestion has been marked as resolved in the meantime.

From what I can tell the majority of Forgejo maintainers leaned towards something like allowBindPrivilegedPort (defaulting to false) in the end.

I get that it was my suggestion to make it unconditional in the first place.
But I changed my mind in #306457 (comment), voicing my support for such option (suggesting the inferior name allowPortBindBelow1024).

We could even backport that variant to 24.05 whenever this PR is ready and merged, long after the branch-off tomorrow, since it wouldn't be a breaking change.

Or, slightly more controversial, we could continue to not provide an option for this at all.

While we do not have concrete guidelines for this in nixpkgs, most folks I speak with expect users of unusual, edge-case-y and advanced configurations to be comfortable doing such override in question themselves via systemd.services.<name>.

{ lib, ... }: {
  systemd.services.forgejo.serviceConfig = {
    AmbientCapabilities = lib.mkForce [ "CAP_NET_BIND_SERVICE" ];
    CapabilityBoundingSet = lib.mkForce [ "CAP_NET_BIND_SERVICE" ];
    PrivateUsers = lib.mkForce false;
  };
}

---

What if we, as suggested somewhere in one of the many collapsed comments, split the CAP thingy from this PR as it currently stands.

That way, this whole CAP discussion does not hinder the uncontroversial changes from getting merged.

Specifically, the services.openssh conditional bug fix (#306205):

-    services.openssh.settings.AcceptEnv = mkIf (!cfg.settings.START_SSH_SERVER or false) "GIT_PROTOCOL";
+    services.openssh.settings.AcceptEnv = mkIf (!cfg.settings.server.START_SSH_SERVER or false) "GIT_PROTOCOL";

@pyrox0 getting added as maintainer and the GITEA_ -> FORGEJO_ thingy (noop).

PS: I genuinely haven't thought about port forwarding on the machine itself (what @bendlas suggested). I genuinely like that approach. Might copy that for my stuff.

[emilylange]

Are you still interested in working on this, @pyrox0? :)

[pyrox0]

I am still interested in working on this. I'll rebase and remove the CAP override so that the rest of this can get merged.

[pyrox0]

This should be ready to merge.

[emilylange]

@ofborg test forgejo