Make SABnzbd's bind address survive a restart, and assert the control that matters

docker-compose-nzb.yml

@@ -71,6 +71,14 @@ services:
       - ${CERTIFICATES_FOLDER}:/certs:ro,z
       - ${CONFIG_FOLDER}/sabnzbd/config:/config:z
       - ${USENET_FOLDER}:/data/usenet${DATA_VOLUME_FLAGS}
+      # The image's own service script passes `--server ::` on every start,
+      # which overrides `host` in sabnzbd.ini and is persisted back into it, so
+      # `host = 0.0.0.0` cannot survive a restart. Reported upstream twice,
+      # linuxserver/docker-sabnzbd#116 and #240, and closed as intended to stop
+      # anyone binding 127.0.0.1 and locking themselves out of the web UI. This
+      # replacement keeps that protection for a missing or loopback value and
+      # respects a reachable one. See patches/README.md and docs/TODO.md.
+      - ./patches/sabnzbd/svc-sabnzbd/run:/etc/s6-overlay/s6-rc.d/svc-sabnzbd/run:ro,z
     healthcheck:
       <<: *healthcheck-defaults
       test: >

docs/HARDENING.md

@@ -76,8 +76,28 @@ namespace, so their container-level IPv6 settings are controlled by gluetun.
 Gluetun intentionally keeps IPv6 sysctls enabled because its DNS listener binds
 to `[::]:53`; leak protection comes from not assigning IPv6 addresses and from
 gluetun's firewall rules. SABnzbd is additionally configured with
-`host = 0.0.0.0`, `ipv6_hosting = 0`, and `ipv6_servers = 0` in
-`configs/sabnzbd/config/sabnzbd.ini`.
+`ipv6_hosting = 0` and `ipv6_servers = 0` in
+`configs/sabnzbd/config/sabnzbd.ini`, which is what stops it selecting Usenet
+servers over IPv6 or opening the `[::1]` listener.
+
+Its bind address is not ours to set, and expecting otherwise cost a red build on
+the 5.0.4 bump. linuxserver's start script passes `--server ::` whenever
+`/proc/net/if_inet6` exists, `--server` overrides the config file, and SABnzbd
+persists the override back into the ini, so `host = 0.0.0.0` cannot survive a
+start. The file exists here because gluetun keeps IPv6 sysctls enabled for its
+own DNS listener, per the paragraph above. Reported upstream twice,
+[#116](https://github.com/linuxserver/docker-sabnzbd/issues/116) and
+[#240](https://github.com/linuxserver/docker-sabnzbd/issues/240), and closed as
+intended behavior: forcing it is what stops a user binding `127.0.0.1` and
+locking themselves out of the web UI.
+
+Accepting `::` costs nothing here, which is why the test allows either value. The
+listener lives in gluetun's network namespace, which is assigned no IPv6 address
+and is firewalled, so binding `::` reaches nothing that `0.0.0.0` would not. The
+alternatives were both worse: disabling IPv6 in that namespace to make the
+container choose `0.0.0.0` would break gluetun's DNS listener, and overriding the
+start script would mean vendoring a permanent fork of upstream's launcher to
+control a value that protects nothing in this topology.
 
 ### VPN kill-switch
 

docs/TODO.md

@@ -26,6 +26,45 @@ for what was fixed and when.
   Check for a `baseimage-gui` bump to 4.13.0+ in `docker-jdownloader-2`'s
   Dockerfile before removing this patch
 
+## SABnzbd
+
+- [ ] Drop `patches/sabnzbd/svc-sabnzbd/run` and its bind mount in
+  `docker-compose-nzb.yml` once the image respects a configured host. The
+  shipped service script passes `--server ::` on every start, which overrides
+  `host` in `sabnzbd.ini` and is then persisted back into it, so a deliberate
+  `host = 0.0.0.0` cannot survive a container start. This surfaced on the 5.0.1
+  to 5.0.4 bump, where the suite went red on
+  `test_sabnzbd_config_disables_ipv6`. Reported upstream twice before this
+  repository hit it,
+  [linuxserver/docker-sabnzbd#116](https://github.com/linuxserver/docker-sabnzbd/issues/116)
+  (2021) and
+  [#240](https://github.com/linuxserver/docker-sabnzbd/issues/240) (2024-12-07,
+  closed `NOT_PLANNED` with "That's the intended behavior"), on the grounds that
+  forcing the value stops anyone binding `127.0.0.1` and locking themselves out
+  of the web UI. The patch keeps exactly that protection, imposing the family
+  address when the configured host is missing or loopback, and respects a
+  reachable one. Filed as
+  [linuxserver/docker-sabnzbd#274](https://github.com/linuxserver/docker-sabnzbd/issues/274),
+  framed as a feature request rather than a bug because the bug framing is what
+  #240 was closed under, with the fix submitted as
+  [#275](https://github.com/linuxserver/docker-sabnzbd/pull/275). The patch is
+  deliberately identical to that pull request, so it becomes a no-op the moment
+  the change ships and can simply be deleted. Temper expectations: the same
+  behavior was reported in #2 (2016), #35 (2017), #42 (2018), #116 (2021) and
+  #240 (2024), and every one was closed. Before deleting, check whether the
+  upstream `run` script has
+  gained any condition around `--server`, where `RAW` is
+  `https://raw.githubusercontent.com/linuxserver/docker-sabnzbd/master`:
+
+  ```shell
+  curl -s "$RAW/root/etc/s6-overlay/s6-rc.d/svc-sabnzbd/run" | grep -n server
+  ```
+
+  Note that the bind address is not the control that matters in this stack:
+  `test_vpn_namespace_has_no_global_ipv6_address` asserts the property
+  `docker-compose-vpn.yml` actually relies on, so this patch is defense in depth
+  and its removal is safe whenever upstream changes
+
 ## LazyLibrarian
 
 - [ ] Drop `patches/lazylibrarian/lazylibrarian/auth.py` once it is no longer

patches/README.md

@@ -15,6 +15,7 @@ under those terms. See [NOTICE.md](../NOTICE.md).
 | `mylar/` | [MylarComics/mylar3](https://github.com/mylarcomics/mylar3) | GPL-3.0-or-later | Five complete source files, modified to add qBittorrent HTTPS support (self-signed certificate handling in the torrent client and its config UI) |
 | `jdownloader2/` | [jlesage/docker-jdownloader-2](https://github.com/jlesage/docker-jdownloader-2) | MIT | `10-webauth.sh`, a modified copy of the image's own `cont-init.d` script, reading credentials from mounted compose secrets because the image's documented `CONT_ENV_*` Docker-secrets support does not fire |
 | `lazylibrarian/` | [LazyLibrarian](https://gitlab.com/LazyLibrarian/LazyLibrarian) | GPL-3.0 | `auth.py`, fixing `login()`/`logout()` unconditionally prepending `HTTP_ROOT` to `from_page`, which `check_auth()`'s own redirect already prefixed since nginx forwards this app's URIs unstripped; 404'd the redirect back after logging in from a protected page reached while logged out |
+| `sabnzbd/` | [linuxserver/docker-sabnzbd](https://github.com/linuxserver/docker-sabnzbd) | GPL-3.0 | `svc-sabnzbd/run`, the image's own s6 service script, changed to pass `--server` only when the configured host is missing or loopback. Upstream passes it unconditionally, and it overrides `host` in `sabnzbd.ini` and is persisted back into it, so `host = 0.0.0.0` cannot survive a start |
 
 `mylar/mylar/config.py` and `mylar/mylar/webserve.py` carry their original GPL
 headers. Do not strip them. Because these are complete GPL-3.0 files rather

patches/sabnzbd/svc-sabnzbd/run

@@ -0,0 +1,53 @@
+#!/usr/bin/with-contenv bash
+# shellcheck shell=bash
+
+FAMILY=::
+
+# Local patch, not upstream, and deliberately identical to the change proposed
+# to linuxserver/docker-sabnzbd so that this file becomes a no-op if they take
+# it. See patches/README.md and docs/TODO.md.
+#
+# The shipped script passes `--server "$FAMILY"` unconditionally. That overrides
+# `host` in sabnzbd.ini and is persisted back into it, so a deliberate
+# `host = 0.0.0.0` cannot survive a container start. Below, a configured host is
+# passed back through the same flag, which makes it a no-op, unless the value is
+# unset or loopback, in which case FAMILY is imposed exactly as upstream does.
+# That keeps the protection upstream cares about: nobody can bind 127.0.0.1 and
+# lock themselves out of the web UI.
+#
+# Section tracking rather than a plain grep for `^host`, because the NNTP server
+# entries under [servers] carry their own unindented `host = ...` line. Matching
+# the wrong one would hand a news server's hostname to --server.
+CONFIGURED_HOST=$(awk '/^\[/{s=$0} s=="[misc]" && /^host[[:space:]]*=/{sub(/^host[[:space:]]*=[[:space:]]*/,""); print; exit}' /config/sabnzbd.ini 2>/dev/null)
+
+if [[ -z ${LSIO_NON_ROOT_USER} ]]; then
+    if ! test -f /proc/net/if_inet6; then
+        FAMILY=0.0.0.0
+        sed '/ip6-/d' /etc/hosts > /etc/hosts.new
+        cat /etc/hosts.new > /etc/hosts
+        rm /etc/hosts.new
+    fi
+
+    # After the fallback above, not before it: a single check ahead of the `if`
+    # would let that fallback clobber a configured host on a machine without
+    # IPv6, which is the bug this exists to fix.
+    case ${CONFIGURED_HOST} in
+        "" | 127.0.0.1 | ::1 | localhost) ;;
+        *) FAMILY=${CONFIGURED_HOST} ;;
+    esac
+
+    exec \
+        s6-notifyoncheck -d -n 300 -w 1000 \
+            s6-setuidgid abc python3 /app/sabnzbd/SABnzbd.py \
+            --config-file /config --server "$FAMILY"
+else
+    case ${CONFIGURED_HOST} in
+        "" | 127.0.0.1 | ::1 | localhost) ;;
+        *) FAMILY=${CONFIGURED_HOST} ;;
+    esac
+
+    exec \
+        s6-notifyoncheck -d -n 300 -w 1000 \
+            python3 /app/sabnzbd/SABnzbd.py \
+            --config-file /config --server "$FAMILY"
+fi

tests/test_security.py

@@ -346,10 +346,64 @@ def test_ipv6_sysctls_disabled(service_name, running_containers, docker_client):
         assert output.decode(errors="replace").strip() == "1"
 
 
+def test_vpn_namespace_has_no_global_ipv6_address(running_containers, docker_client):
+    """Nothing in gluetun's namespace may hold a routable IPv6 address.
+
+    This is the control docker-compose-vpn.yml names: IPv6 is deliberately left
+    enabled there, because gluetun's DNS server binds `[::]:53`, and leaks are
+    prevented by no interface holding an IPv6 address plus gluetun's ip6tables
+    rules. Until now nothing asserted the first half of that.
+
+    It is the property the SABnzbd bind address was standing in for, and a
+    better one to test. The bind address belongs to linuxserver's start script
+    rather than to us, so asserting it proved only what upstream felt like doing
+    that week, and a listener on `::` reaches nothing while this holds. Should a
+    provider ever hand out an IPv6 address inside the tunnel, this fails and the
+    bind address becomes worth arguing about again.
+    """
+    service_name = env("VPN_PROVIDER", "gluetun")
+    skip_if_disabled(service_name)
+    skip_if_not_running(service_name, running_containers)
+    container = fresh_container(docker_client, service_name)
+
+    # /proc/net/if_inet6 rather than `ip -6 addr`, which the image may not carry.
+    # Columns are address, index, prefix length, scope, flags, device. Scope 10
+    # is link-local and scope 20 is host, neither of which is routable; scope 00
+    # is global, which is the one that must not appear.
+    exit_code, output = container.exec_run(["cat", "/proc/net/if_inet6"])
+    assert exit_code == 0, output.decode(errors="replace")
+
+    global_addresses = [
+        line
+        for line in output.decode(errors="replace").splitlines()
+        if len(line.split()) >= 4 and line.split()[3] == "00"
+    ]
+    assert not global_addresses, (
+        "gluetun's namespace holds a routable IPv6 address, so the download "
+        f"clients sharing it are reachable over IPv6: {global_addresses}"
+    )
+
+
 def test_sabnzbd_config_disables_ipv6():
-    """SABnzbd should not bind or select Usenet servers over IPv6."""
+    """SABnzbd should not select Usenet servers over IPv6, nor open [::1]."""
     misc = _sabnzbd_misc()
-    assert misc["host"] == "0.0.0.0"  # nosec B104 - asserting a config value, not binding a socket
+    # Either value is acceptable, because the bind address is the container's to
+    # choose and not ours. linuxserver's start script passes `--server ::`
+    # whenever /proc/net/if_inet6 exists, which it does here since gluetun keeps
+    # IPv6 sysctls enabled for its own DNS listener, and `--server` overrides the
+    # config file and is persisted back into it. So `host = 0.0.0.0` in the seeded
+    # ini cannot survive a start, which is what turned the 5.0.4 bump red.
+    #
+    # Reported upstream twice, linuxserver/docker-sabnzbd#116 and #240, and closed
+    # as intended behavior: forcing it stops a user binding 127.0.0.1 and locking
+    # themselves out of the web UI.
+    #
+    # Nothing is lost by accepting it. The listener sits in gluetun's network
+    # namespace, which has no IPv6 address and is firewalled, so `::` reaches
+    # nothing `0.0.0.0` would not. See docs/HARDENING.md.
+    assert misc["host"] in ("0.0.0.0", "::")  # nosec B104 - asserting a config value, not binding a socket
+    # These two are ours. `--server` does not touch them, and they are what
+    # actually governs IPv6 behavior rather than the bind address.
     assert misc["ipv6_hosting"] == "0"
     assert misc["ipv6_servers"] == "0"