Add optional built-in HTTPS/TLS - #144
Conversation
README/SECURITY.md previously assumed a reverse proxy terminates TLS. For a simple single-Pi setup, the server can now terminate TLS itself: setting both tls_cert and tls_key makes it listen with (*http.Server).ListenAndServeTLS instead of plain ListenAndServe. Plain HTTP remains the default when both are left empty; setting only one is rejected by config validation rather than silently falling back. Closes #41
SonarCloud's quality gate flagged coverage on new code below the 80% threshold, and CI's golangci-lint failed on unchecked error returns in the new TLS server test. - Factor the httpapi.Config construction in run() into a serverConfig helper (mirroring the existing clientConfig), so the TLSCertFile/ TLSKeyFile wiring is unit-testable instead of only reachable through the untested run() function. - Silence errcheck on intentionally-ignored Close()/Shutdown() calls in internal/httpapi/server_test.go, matching the _ = ... convention already used elsewhere in the codebase.
There was a problem hiding this comment.
Reviewed against the project checklist (error handling, resource cleanup, command-execution safety, privilege separation, /api/v1 stability, test coverage, dependencies). Verified locally on 6a5a86b: go build ./..., go vet ./... clean; go test ./... -race -cover passes (internal/httpapi 98.6%, internal/config 96.0%).
No blocking issues. /api/v1/... response shapes are untouched, no new dependency, no new exec.Command or root-only file dependency, and the both-or-neither Validate() rule is covered by table-driven tests plus end-to-end Load cases.
Two non-blocking findings inline: a diagnosability/flakiness problem in the new server_test.go, and two operational gaps the example-config comment should cover (privileged-port binding under the hardened unit, and certificate renewal requiring a restart).
…LS ops gotchas - server_test.go: thread the ListenAndServe error channel through waitUntilUp so a startup failure (bad cert pair, lost port race) fails the test immediately with the real cause instead of a generic "did not come up in time" timeout after 2s of polling. Add a package comment explaining why this file binds a real socket instead of following the httptest convention docs/TESTS.md describes for internal/httpapi. - pimonitor.example.yaml: note that tls_cert/tls_key requires a restart to pick up a renewed certificate, and that a privileged listen_addr (e.g. ":443") needs AmbientCapabilities=CAP_NET_BIND_SERVICE since the service otherwise has no capability to bind it.
|
2 participants
📖 Description
README/SECURITY.mdpreviously assumed a reverse proxy (nginx, Caddy, ...) terminates TLS. For a simple single-Pi setup, the server can now terminate TLS itself: setting bothtls_certandtls_keystarts it with(*http.Server).ListenAndServeTLSinstead of plainListenAndServe. Plain HTTP remains the default when both are left empty.This is an additive feature, not a breaking change.
🎫 Issues
Closes #41
👩💻 Reviewer Notes
internal/config/config.go: newTLSCertFile/TLSKeyFilefields (tls_cert/tls_keyYAML keys) and aValidate()check rejecting the case where only one is set.internal/httpapi/server.go:Configgained matchingTLSCertFile/TLSKeyFilefields;ListenAndServepicksListenAndServeTLSwhen both are set, plainListenAndServeotherwise.cmd/pimonitor/main.go: wires the resolved config values intohttpapi.Config.packaging/pimonitor.example.yamlandSECURITY.mddocument the new option.Smoke test: run with
tls_cert/tls_keypointing at a self-signed cert (e.g. viaopenssl req -x509 -newkey rsa:2048 -nodes -keyout key.pem -out cert.pem -days 1 -subj /CN=localhost) and confirmcurl -k https://localhost:8080/healthzworks, while leaving both unset still serves plain HTTP as before.📑 Test Plan
internal/config/config_test.go:TestValidate_TLScovers the three acceptance-criteria branches (both set → valid, neither set → valid, only one set → error), plus two new rows inTestValidate_RejectsBadValues.TestLoad_TLSFromYAML/TestLoad_TLSOnlyCertIsRejectedcover YAML wiring end-to-end throughLoad.internal/httpapi/server_test.go(new):TestListenAndServe_TLSConfiguredgenerates a throwaway self-signed cert, starts the server withListenAndServe, and confirms atls.Dialhandshake succeeds while a plain HTTP request to the same address is rejected.TestListenAndServe_PlainHTTPByDefaultconfirms the unset case still serves plain HTTP.go build ./...,go vet ./...,go test ./... -race -cover, andgofmt -l .all pass locally.golangci-lint runcould not be run in this environment: the installedgolangci-lintbinary (built with go1.25) refuses to load the project'sgo 1.26.7go.mod(pre-existing environment/toolchain mismatch, unrelated to this change) — please run it in CI or a matching local toolchain.✅ Checklist
General
go test ./... -race -coverpasses locally).go vet ./...andgolangci-lint runare clean. (go vetclean;golangci-lintcould not be run locally, see Test Plan above)ARCHITECTURE.mdif this changes a documented design decision. (config/HTTP-layer addition only, no architectural change)REST API / configuration / packaging
docs/API.mdto reflect a REST API change. (no API shape change, per issue scope)/api/v1/...response shapes, or a new API version (/api/v2/...) was introduced instead.README.md/packaging/pimonitor.example.yamlto reflect a new or changed configuration option.packaging/install.shor the systemd units if this changes installation/packaging, and kept the unprivileged/privileged service split intact (seeSECURITY.md). (no packaging changes needed; cert/key file permissions are documented in the example config)⏭ Next Steps
None.