Add optional web interface for QR codes and config downloads (Ingress-only) - #398
Add optional web interface for QR codes and config downloads (Ingress-only)#398Arbuzov wants to merge 8 commits into
Conversation
…rden access Responsible-review follow-up hardening the optional web interface: - Serve the UI behind Home Assistant Ingress (ingress: true, ingress_port 8099) instead of raw port 80, so the private-key-bearing QR codes and client configs are gated by HA authentication + TLS. - Add busybox-extras: stock Alpine busybox ships no httpd applet, so the original `busybox httpd` never actually ran; call httpd directly. - Run the UI as its own supervised s6 service (webui); leave the JSON status API on port 80 byte-for-byte unchanged so existing REST sensors keep working. - Restrict the Ingress port to the Supervisor proxy (172.30.32.2) via an httpd.conf allow/deny, as defence in depth. - Delete the duplicated hand-rolled CGI status script. - Bump musl to 1.2.5-r12 so the image builds against the current Alpine 3.22 repository (base image still pins r10). - Add a CI smoke test asserting httpd availability and ACL enforcement. - Document web_interface, its Ingress access model, unchanged status API, and the disabled-state behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e page Addresses Sourcery review feedback: cp/base64 on a peer's client.conf or qrcode.png would emit a broken card if the file were missing. Skip such peers with a bashio::log.warning instead. Both files are normally generated earlier in the same script, so this is defensive hardening. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses Codex review (P2): a peer named "httpd" produced configs/httpd.conf, which BusyBox httpd reserves as its per-directory config file and refuses to serve (403), leaving that peer's download link dead. Serve each peer's config as configs/<name>/client.conf so the served basename is always client.conf. Verified against ghcr.io/hassio-addons/base:19.0.0 (peer "httpd" now 200). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
WalkthroughAdds an optional Home Assistant Ingress web interface for WireGuard peer QR codes and client downloads, restricts access to the Ingress proxy, adds ACL smoke tests and CI wiring, and pins musl during image builds. ChangesIngress web interface
Alpine build compatibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yaml:
- Around line 22-23: Update the actions/checkout@v4 step in the CI workflow to
set persist-credentials to false, ensuring checkout credentials are not retained
for subsequent repository-controlled shell steps.
In `@wireguard/DOCS.md`:
- Around line 346-351: Update the documentation paragraph near the ingress
access description to retain the Home Assistant authentication requirement, but
qualify the encryption statement as depending on how the Home Assistant entry
point is exposed. Remove the assertion that the add-on page’s connection itself
is encrypted, while preserving the guidance to open it through the “Open Web UI”
button and not a raw unauthenticated port.
In `@wireguard/rootfs/etc/cont-init.d/config.sh`:
- Around line 347-360: Update the peer artifact generation in the config flow to
build each web card in a temporary per-peer directory, capturing failures from
both client.conf copying and QR/base64 processing with stderr redirected. Only
after every command succeeds should the existing peer directory be removed and
the temporary directory moved into place, ensuring partial artifacts cannot
update the served index.html.
In `@wireguard/tests/webui-acl-smoke.sh`:
- Around line 25-30: Remove the apk add --no-cache busybox-extras installation
from the docker run smoke-test command. Execute the httpd availability check
directly against the built runtime image referenced by base, preserving the
command -v httpd assertion so the test fails when the image itself lacks the
dependency.
- Around line 41-48: Update the allowed-client test in the smoke script to use
the production Web UI configuration from rootfs/etc/httpd.conf rather than
creating /tmp/allow.conf with 127.0.0.1. Assert that the production
configuration contains the exact A:172.30.32.2 and D:* ACL entries, then
exercise the server using the allowed 172.30.32.2 source while preserving the
expected 200 response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 721279ca-cf89-437e-973c-fb3daeecdda2
📒 Files selected for processing (8)
.github/workflows/ci.yamlwireguard/DOCS.mdwireguard/Dockerfilewireguard/config.yamlwireguard/rootfs/etc/cont-init.d/config.shwireguard/rootfs/etc/httpd.confwireguard/rootfs/etc/services.d/webui/runwireguard/tests/webui-acl-smoke.sh
| if ! bashio::fs.file_exists "${config_dir}/client.conf" \ | ||
| || ! bashio::fs.file_exists "${config_dir}/qrcode.png"; then | ||
| bashio::log.warning \ | ||
| "Skipping web card for ${name}: client.conf or qrcode.png missing" | ||
| continue | ||
| fi | ||
|
|
||
| # Make the client config available for download, from a per-peer | ||
| # subdirectory so the served basename is always "client.conf". A flat | ||
| # "configs/${name}.conf" would become "httpd.conf" for a peer named | ||
| # "httpd" (a schema-valid name), which BusyBox httpd reserves as its | ||
| # per-directory config file and refuses to serve (403). | ||
| mkdir -p "${www_dir}/configs/${name}" | ||
| cp "${config_dir}/client.conf" "${www_dir}/configs/${name}/client.conf" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)config\.sh$|wireguard/.*/config\.sh' || true
echo "== file excerpt around web card =="
if [ -f wireguard/rootfs/etc/cont-init.d/config.sh ]; then
nl -ba wireguard/rootfs/etc/cont-init.d/config.sh | sed -n '300,390p'
fi
echo "== references to qrcode/png and web card vars =="
rg -n "qrcode|qr|qrcode\.png|web card|www_dir|configs|client\.conf|card" wireguard/rootfs/etc/cont-init.d/config.sh || true
echo "== git diff stat/name for context =="
git diff --stat || true
git diff --name-only || true
echo "== changed context if any =="
if [ -f wireguard/rootfs/etc/cont-init.d/config.sh ]; then
git diff --unified=20 -- wireguard/rootfs/etc/cont-init.d/config.sh | sed -n '1,240p' || true
fiRepository: hassio-addons/addon-wireguard
Length of output: 303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt around web card =="
awk 'NR>=260 && NR<=430 { printf "%6d\t%s\n", NR, $0 }' wireguard/rootfs/etc/cont-init.d/config.sh
echo "== references to qrcode/png and web card vars =="
rg -n "qrcode|qr|qrcode\.png|web card|www_dir|configs|client\.conf|card" wireguard/rootfs/etc/cont-init.d/config.sh || true
echo "== git diff stat/name for context =="
git diff --stat || true
git diff --name-only || true
echo "== changed context if any =="
git diff --unified=20 -- wireguard/rootfs/etc/cont-init.d/config.sh | sed -n '1,260p' || trueRepository: hassio-addons/addon-wireguard
Length of output: 7466
Publish peer artifacts atomically.
The web card is written by mixing a client.conf copy with QR data read with base64; either command can fail or produce malformed HTML. Use a temporary per-peer directory, redirect stderr for command failures, and only rm -rf then mv it into place when all commands succeed, so a partial card never updates /var/lib/wireguard/www/index.html.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wireguard/rootfs/etc/cont-init.d/config.sh` around lines 347 - 360, Update
the peer artifact generation in the config flow to build each web card in a
temporary per-peer directory, capturing failures from both client.conf copying
and QR/base64 processing with stderr redirected. Only after every command
succeeds should the existing peer directory be removed and the temporary
directory moved into place, ensuring partial artifacts cannot update the served
index.html.
|
Hi maintainers 👋 This PR is ready for review. The failing Verify check is just the required-labels gate ( |
- tests: build and assert against the real add-on image instead of apk-installing busybox-extras inside the test, so a dropped dependency fails here rather than shipping. - tests: exercise the production rootfs/etc/httpd.conf ACL from a real 172.30.32.2 source address, and assert the exact allow-then-deny rule order, instead of standing in a synthetic A:127.0.0.1 config. - config.sh: read the QR code and publish client.conf behind explicit failure guards, so a failed base64/cp skips the peer instead of appending a broken card to index.html. - ci: do not persist checkout credentials in the smoke-test job. - DOCS.md: tie transport encryption to how Home Assistant itself is exposed rather than claiming it for the Ingress connection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rfile Two follow-ups from a second review pass: - config.sh: `base64 ... | tr -d '\n'` reported tr's exit status, so a base64 that died partway through still looked like success and could land a truncated data URI in index.html. Drop the pipe and strip the newlines in the shell, keeping base64's own status. - tests: building the whole add-on image to get an httpd made an httpd/ACL test depend on the Go toolchain and git.zx2c4.com, which fails on a flaky network. Read the busybox-extras spec out of the Dockerfile instead and install exactly that, so removing the dependency still fails the test, without the external build. The real image can still be tested via WEBUI_SMOKE_IMAGE. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Combines the PR hassio-addons#398 review fixes with the fork-only hardening already on main. Conflict resolutions: - config.sh: keep main's 0700/0600 modes (the tree holds peer private keys) but take the branch's base64 handling — main still piped into `tr -d '\n'`, which reports tr's exit status, so a base64 that died partway through could leave a truncated data URI in index.html. Failure now warns and skips the peer instead of aborting init: the VPN must not stop serving because one QR card could not be built. - webui-acl-smoke.sh: take the branch version. It already polls instead of sleeping (main's improvement) and additionally reads the httpd package out of the Dockerfile and exercises the shipped ACL from a real 172.30.32.2 source, rather than a stand-in A:127.0.0.1 config. - DOCS.md: keep main's wording, which covers the same transport-security qualification and also warns that every user sees every peer's keys. Verified: smoke test passes, and fails as expected when busybox-extras is dropped, when the ACL is loosened or misordered, and on both the 200 and 403 arms. install(1) confirmed to apply 0700/0600 in the base image. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…notice The served tree holds every peer's private key, so create it with explicit modes instead of the umask default: 0700 directories, 0600 files, including index.html (which embeds the same key material in its QR codes). The tree is also wiped unconditionally on start, so turning web_interface off drops what was generated while it was on. `ingress: true` is static, so Home Assistant shows the "Open Web UI" button whether or not the feature is enabled. Serve a short "web interface disabled" notice in that case rather than parking the service: an empty port behind that button reads as a broken add-on. The notice goes through the same Ingress-only ACL and contains no key material. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wireguard/rootfs/etc/services.d/webui/run`:
- Around line 16-18: Update the disabled-branch setup around notice_dir so both
mkdir and the index.html heredoc write are explicitly checked for failure. When
either operation fails, stop the service or return a nonzero status before
continuing, ensuring the disabled branch fails closed without a writable notice
file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fea2af2-e661-45d4-81ff-c8c1364b5836
📒 Files selected for processing (3)
wireguard/DOCS.mdwireguard/rootfs/etc/cont-init.d/config.shwireguard/rootfs/etc/services.d/webui/run
🚧 Files skipped from review as they are similar to previous changes (2)
- wireguard/DOCS.md
- wireguard/rootfs/etc/cont-init.d/config.sh
| notice_dir="/var/lib/wireguard/www-disabled" | ||
| mkdir -p "${notice_dir}" | ||
| cat > "${notice_dir}/index.html" << 'HTMLEOF' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the exact service run file and any wrapper/header that may set errexit.
printf 'Candidate files:\n'
fd -a '^(run|\.run|services.conf|webui|.*webui.*)$' wireguard/rootfs/etc/services.d 2>/dev/null || true
printf '\nTarget run file excerpt:\n'
if [ -f wireguard/rootfs/etc/services.d/webui/run ]; then
cat -n wireguard/rootfs/etc/services.d/webui/run | sed -n '1,120p'
fi
printf '\nHeader/includes and shell/error control references:\n'
fd -a '^(run|\.run)$' wireguard/rootfs/etc/services.d | while read -r f; do
printf '\n---- %s ----\n' "$f"
sed -n '1,80p' "$f"
done
printf '\nSearch errexit/bashio setup:\n'
rg -n 'set -e|set -o errexit|errexit|bashio::exit|bash_options|set.*e' wireguard/rootfs/etc/services.d wireguard | sed -n '1,200p'Repository: hassio-addons/addon-wireguard
Length of output: 10454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how s6 services behave when a run script makes an unguarded filesystem write and httpd fails.
# This model uses the same service entry points as the repository service without invoking repository code.
tmp="$(mktemp -d)"
trap 'rm -rf "$tmp"' EXIT
runfile="${tmp}/webui-run"
mkdir -p "$tmp/log $tmp/htdocs/missing" "$tmp/htdocs/works"
# Disabled path has unchecked mkdir and heredoc, then execs httpd against a document root whose files may not exist.
cat > "$runfile" << 'BASH'
#!/bin/bash
notice_dir="$1"
mkdir -p "${notice_dir}"
cat > "${notice_dir}/index.html" << 'HTMLEOF'
<title>Disabled</title>
HTMLEOF
exec httpd -f -p 0 -h "${notice_dir}"
BASH
chmod +x "$runfile"
printf 'Read-only filesystem:\n'
readonly_dir="$tmp/readonly"
mkdir -p "$readonly_dir"
if [ -x /usr/sbin/newgrp ]; then
useradd -m -d "$readonly_dir" testuser >/dev/null 2>&1 && \
newgrp - <<'HEREDOC'
set -eu
set +o pipefail
if output="$(env HOME="$readonly_dir" "$runfile" "$readonly_dir/htdocs" 2>&1)"; then
echo "started"
echo "index present? $(ls "$readonly_dir/htdocs/index.html" >/dev/null 2>&1 && echo yes || echo no)"
else
echo "script failed as expected: $(echo "$output" | head -n 3)"
fi
HEREDOC
else
echo "newgrp unavailable"
fi
printf '\nFull/missing index without read-only check:\n'
rm -rf "$tmp/htdocs/missing"
if bash "$runfile" "$tmp/htdocs/missing" 2>&1 >/tmp/webui-full-try.log; then
echo "started via unset error status"
else
echo "script exited: $(cat /tmp/webui-full-try.log | head -n 3)"
fi
printf '\nSuccessful write/path:\n'
rm -rf "$tmp/htdocs/works"
bash "$runfile" "$tmp/htdocs/works" 2>/tmp/webui-works-err.log || true
echo "index exists: $(ls "$tmp/htdocs/works/index.html" >/dev/null 2>&1 && echo yes || echo no)"Repository: hassio-addons/addon-wireguard
Length of output: 373
🌐 Web query:
bashio bashio::exit.nok Bash addon Home Assistant documentation
💡 Result:
The bashio::exit.nok function is a helper function in the Bashio library, a Bash utility collection used for developing Home Assistant apps (formerly known as add-ons) [1][2][3]. Purpose and Behavior: The primary purpose of bashio::exit.nok is to terminate a script with a non-zero exit status, indicating failure [4][5]. When called, it performs the following actions: 1. Logs a message: If an optional message is provided as an argument, it logs that message using the bashio::log.red function (which typically outputs to the logs in a red, fatal format) [4][5]. 2. Exits the script: It executes the command exit 1, which signals to the Home Assistant Supervisor or the calling process that the script has failed [4][5]. Code Implementation: The function is defined in the library's exit module as follows [4][5]: bashio::exit.nok { local m="${1:-}"; [ -n "$m" ] && bashio::log.red "$m"; exit 1; } Usage Context: It is commonly used throughout the Bashio library and in custom Home Assistant app scripts to handle fatal errors—such as missing required configuration options—where the script cannot safely continue [6]. For example, when a mandatory configuration option is missing, the script will log an error message and call bashio::exit.nok to stop execution [6]. It serves as the failure counterpart to bashio::exit.ok, which exits with a status code of 0 (success) [4][5].
Citations:
- 1: https://github.com/hassio-addons/bashio
- 2: https://github.com/hassio-addons/bashio/blob/main/README.md
- 3: https://developers.home-assistant.io/docs/apps/configuration
- 4: Add test coverage for lib/exit.sh bashio#232
- 5: https://raw.githubusercontent.com/alexbelgium/hassio-addons/master/.templates/bashio-standalone.sh
- 6: https://github.com/hassio-addons/bashio/blob/main/lib/config.sh
Fail closed if the disabled notice cannot be written.
This service runs without errexit, so checked mkdir/(cat <<...) if failures are needed for the disabled branch; otherwise the script can continue or restart without a writable notice file.
Proposed fix
- mkdir -p "${notice_dir}"
+ if ! mkdir -p "${notice_dir}"; then
+ bashio::exit.nok "Could not create disabled web interface folder"
+ fi
- cat > "${notice_dir}/index.html" << 'HTMLEOF'
+ if ! cat > "${notice_dir}/index.html" << 'HTMLEOF'
...
HTMLEOF
+ then
+ bashio::exit.nok "Could not write disabled web interface notice"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| notice_dir="/var/lib/wireguard/www-disabled" | |
| mkdir -p "${notice_dir}" | |
| cat > "${notice_dir}/index.html" << 'HTMLEOF' | |
| notice_dir="/var/lib/wireguard/www-disabled" | |
| if ! mkdir -p "${notice_dir}"; then | |
| bashio::exit.nok "Could not create disabled web interface folder" | |
| fi | |
| if ! cat > "${notice_dir}/index.html" << 'HTMLEOF' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wireguard/rootfs/etc/services.d/webui/run` around lines 16 - 18, Update the
disabled-branch setup around notice_dir so both mkdir and the index.html heredoc
write are explicitly checked for failure. When either operation fails, stop the
service or return a nonzero status before continuing, ensuring the disabled
branch fails closed without a writable notice file.
Proposed Changes
Adds an opt-in web UI that shows each WireGuard peer's QR code and offers one-click client-config downloads, so onboarding a device no longer requires digging through
/ssl/wireguard/on the host.Access model (security)
The QR codes and
client.conffiles contain each peer's private key, so the UI is served only through Home Assistant Ingress — authenticated, and never on a raw open port:ingress: true/ingress_port: 8099; opened via the add-on's "Open Web UI" button.webuiservice runsbusybox httpdon8099, andhttpd.confrestricts the port to the Supervisor Ingress proxy (172.30.32.2) as defence in depth.0700/0600— includingindex.html, which embeds the same key material in its QR codes — and is wiped on every start, so turning the option back off drops what was generated while it was on.Transport encryption is whatever the Home Assistant entry point itself uses: Ingress proxies this over the internal Supervisor network, so it is end-to-end encrypted only where Home Assistant is reached over HTTPS. The docs say so rather than claiming TLS on the add-on's behalf.
Status API — unchanged
services.d/api/runis byte-identical tomain: the JSON status API keeps serving on port 80 exactly as before, so existing REST sensors are unaffected. (No/cgi-bin/status; enabling the UI does not touch the status API.)What changed
config.yaml—web_interface: bool?(off by default) +ingress/ingress_port.cont-init.d/config.sh— when enabled, generates/var/lib/wireguard/www/: a self-containedindex.html(inline base64 QR codes + relative download links) and per-peerconfigs/<name>/client.confstatic files, all with explicit0700/0600modes. A peer whose files are missing, or whose QR read or config copy fails, is skipped with a warning rather than half-written into the page — a failed card must not take down the VPN itself.services.d/webui/run— new supervised service. When the option is off it serves a short "web interface disabled" notice through the same Ingress-only ACL, becauseingress: trueis static: the "Open Web UI" button exists either way, and an empty port behind it reads as a broken add-on.Dockerfile— addsbusybox-extras(stock Alpine busybox has nohttpdapplet, so the UI cannot run without it).tests/webui-acl-smoke.sh+ CI job — asserthttpdavailability and the Ingress-only ACL. The test installs thebusybox-extrasspec it reads out of theDockerfilerather than naming the package itself, so dropping that dependency fails the test instead of passing; and it exercises the shippedhttpd.conffrom a real172.30.32.2source address, rather than a stand-inA:127.0.0.1config.DOCS.md— documents the option, its Ingress access model, and the unchanged status API.Note for maintainers
The
Dockerfilealso pinsmusl=1.2.5-r12.base:19.0.0pinsmusl=r10in itsworld, but the current Alpine 3.22 repo'sbuild-base→musl-devrequiresr12, so the image no longer builds against live repos without it (this affectsmaintoo, independent of this PR). It is commented as temporary — happy to drop it in favour of a base-image bump if you'd prefer to handle it that way.Everything was verified against
ghcr.io/hassio-addons/base:19.0.0with Docker:httpdpresent viabusybox-extras, the Ingress-only ACL enforced (403 for a non-proxy client, 200 for the proxy), the missing-file guard, the reserved-httpd.confbasename fix (a peer namedhttpdis now served correctly), the0700/0600modes actually applied, the disabled-notice path serving only to Ingress and carrying no key material, and the fullapkinstall resolving.The smoke test was also checked for the failure it is supposed to catch — it fails, as intended, when
busybox-extrasis dropped from theDockerfile, when the ACL is loosened or its rules are reordered, and on each of its 200 and 403 assertions independently. (Worth knowing if that ACL is ever edited:A:*is not a valid BusyBox allow-wildcard and denies everything.)Review feedback has been applied, including the
checkoutcredential persistence, the encryption wording above, atomicity of the generated peer artifacts, and both smoke-test findings.Related Issues
Summary by CodeRabbit
web_interfacesetting and its security/access behavior, including that the existing status JSON API remains unchanged.