crueber/walhub

Watch

Fork 0

Remove server-side TLS; terminate at the reverse proxy #165

New issue

Closed

opened 2026-09-05 23:02:12 +00:00 by crueber · 3 comments

crueber commented

2026-09-05 23:02:12 +00:00

Owner

Remove server-side TLS: TLS termination belongs on the reverse proxy

Decision

The server should not itself be a TLS server and should not deal with SSL certs. TLS termination belongs on the reverse proxy in front of it.

Verification that no cert is needed (checked 2026-09-05)

Inbound (UI, JSON API, git smart HTTP, SSH transport) all work fine behind a TLS-terminating proxy; HTTP/2 without TLS is already covered by h2c (cmd/walhub/serve.go).
Outbound HTTPS (webhook delivery, OIDC, git clone https:// during import) validates against system root CAs — it never uses server.tls.cert/key.
Today the server supports server.tls.mode = off|files|self_signed (TLSStruct, tlsConfigFor, TLSServerConfig, checkTLS) — all of this becomes dead surface after removal.

Scope

Remove files/self_signed modes, cert/key loading, and TLS listener wrapping; server listens plain HTTP (h2c retained).
Remove/gate the corresponding Setup UI fields and schema entries; config validation rejects (or ignores with a loud warning?) any residual server.tls cert settings — decide in the change, fail closed.
Update docs (docs/go/06_server_http.md, 10-series/config docs, README/compose TLS references, DEVIATIONS.md if the removal deviates from the Rust spec) with a Decisions entry (law 12); deployment docs gain a reverse-proxy snippet/pointer (caddy/nginx minimal example).
Related follow-up (note, don't necessarily implement here): honor X-Forwarded-Proto when building absolute URLs (e.g. clone URLs) behind a proxy — file separately if out of scope.

Acceptance criteria

No TLS cert handling code or config surface remains (grep-clean for cert/key flags outside tests); go vet/coverage gates hold on touched packages.
Behind a TLS-terminating proxy the app works end to end (clone/push/UI/API); document the proxy setup.
make cover/make test green for touched packages; e2e over plain HTTP green.

# Remove server-side TLS: TLS termination belongs on the reverse proxy ## Decision The server should not itself be a TLS server and should not deal with SSL certs. TLS termination belongs on the reverse proxy in front of it. ## Verification that no cert is needed (checked 2026-09-05) - Inbound (UI, JSON API, git smart HTTP, SSH transport) all work fine behind a TLS-terminating proxy; HTTP/2 without TLS is already covered by h2c (`cmd/walhub/serve.go`). - Outbound HTTPS (webhook delivery, OIDC, `git clone https://` during import) validates against system root CAs — it never uses `server.tls.cert/key`. - Today the server supports `server.tls.mode = off|files|self_signed` (`TLSStruct`, `tlsConfigFor`, `TLSServerConfig`, `checkTLS`) — all of this becomes dead surface after removal. ## Scope - Remove `files`/`self_signed` modes, cert/key loading, and TLS listener wrapping; server listens plain HTTP (h2c retained). - Remove/gate the corresponding Setup UI fields and schema entries; config validation rejects (or ignores with a loud warning?) any residual `server.tls` cert settings — decide in the change, fail closed. - Update docs (`docs/go/06_server_http.md`, `10`-series/config docs, README/compose TLS references, `DEVIATIONS.md` if the removal deviates from the Rust spec) with a Decisions entry (law 12); deployment docs gain a reverse-proxy snippet/pointer (caddy/nginx minimal example). - Related follow-up (note, don't necessarily implement here): honor `X-Forwarded-Proto` when building absolute URLs (e.g. clone URLs) behind a proxy — file separately if out of scope. ## Acceptance criteria - [ ] No TLS cert handling code or config surface remains (grep-clean for cert/key flags outside tests); `go vet`/coverage gates hold on touched packages. - [ ] Behind a TLS-terminating proxy the app works end to end (clone/push/UI/API); document the proxy setup. - [ ] `make cover`/`make test` green for touched packages; e2e over plain HTTP green.

crueber referenced this issue from a commit

2026-09-05 23:26:00 +00:00

Fix #165: remove server-side TLS; terminate at the reverse proxy

crueber referenced this issue from a pull request that will close it,

2026-09-05 23:26:04 +00:00

Fix #165: remove server-side TLS #167

crueber commented

2026-09-05 23:26:09 +00:00

Author

Owner

Fixed by PR #167 (#167) — branch fix/issue-165, awaiting review (not merged).

What landed: server-side TLS removed end to end (modes, cert/key loading, self-signed generation, listener wrap, ca.pem route, setup.json ca_url/trust, install.sh CA step, TLS setup fields). Plain HTTP + h2c; x/crypto (SSH) and x/net (h2c) retained.

Two judgment calls worth flagging:

Residual server.tls.* fails closed with a reverse-proxy pointer in both channels (TOML load rejection + fatal env override — the env fatality deliberately overrides the soft unknown-key rule).
The X-Forwarded-Proto follow-up is implemented in this change (requestScheme, display-only), not filed separately — it closed the note in docs/go/12_web_ui.md.

Verification: vet clean; -race green (config/server/api/cmd); cover 95.5% on config/server/api; node --test 325/325; e2e green; live smoke confirmed plain-HTTP serve, https clone URLs under X-Forwarded-Proto, ca.pem 404, and zero server.tls keys in the setup schema.

Fixed by PR #167 (https://git.packden.us/crueber/walhub/pulls/167) — branch fix/issue-165, awaiting review (not merged). What landed: server-side TLS removed end to end (modes, cert/key loading, self-signed generation, listener wrap, ca.pem route, setup.json ca_url/trust, install.sh CA step, TLS setup fields). Plain HTTP + h2c; x/crypto (SSH) and x/net (h2c) retained. Two judgment calls worth flagging: 1. Residual server.tls.* fails closed with a reverse-proxy pointer in both channels (TOML load rejection + fatal env override — the env fatality deliberately overrides the soft unknown-key rule). 2. The X-Forwarded-Proto follow-up is implemented in this change (requestScheme, display-only), not filed separately — it closed the note in docs/go/12_web_ui.md. Verification: vet clean; -race green (config/server/api/cmd); cover 95.5% on config/server/api; node --test 325/325; e2e green; live smoke confirmed plain-HTTP serve, https clone URLs under X-Forwarded-Proto, ca.pem 404, and zero server.tls keys in the setup schema.

crueber commented

2026-09-05 23:29:53 +00:00

Author

Owner

Review of PR #167 (fix/issue-165, remove server-side TLS) — verified in scratch worktree /tmp/pr167 at 6b33a47 (c61c4e9 + 1 review fixup).

VERDICT: ready to merge (after the 1-line fixup already pushed).

DANGLING REFS (all clear, grepped myself):

No remaining tlsOn/TLSOn/TLSStruct/checkTLS/TLSServerConfig/EnsureSelfSigned/loadCACert/caPem/CASelfSigned/tlsConfigFor anywhere in .go. No sslCAInfo, no server.tls. keys outside the fail-closed paths + tests + docs. /services/public/ca.pem route gone from router.go (both full and setup-only mounts). install.sh.tmpl CA steps renumbered 2..6, no ca.pem. web FIELDS has zero server.tls.* entries; stale saved values surface as unknown-key (setup-form.test.js asserts this). MASTER_RUST_SPEC.md untouched — correct, it is the normative Rust reference; divergence recorded in DEVIATIONS.md D-HTTP-4.

TRUST BOUNDARY (X-Forwarded-Proto):

requestScheme (server/middleware.go:209) honors X-Forwarded-Proto first value, else r.TLS, else http. Used ONLY for display URL building: api baseURL (refs.go:33), server baseURL (lfs.go:172), setup.json base (display), canonicalBrowserHost 302 target, OIDC loopback redirect-URI construction. Never gates auth or access control — verified by grep of all call sites. Cookie Secure no longer consults scheme at all (now: len(CorsOrigins)>0, middleware.go:464 + auth_oidc.go:277) — note: behind a TLS proxy without CORS origins the session cookie is non-Secure; safe on the wire (browser->proxy is HTTPS) and keeps direct plain-HTTP LAN working. Acceptable trade-off, flagging for awareness.
api/refs.go duplicates the proto-parsing instead of sharing requestScheme — justified by layering (api must not import server); fine.

DEPENDENCY BUDGET: x/crypto still imported (sshd.go, command.go, sshkeys.go — SSH transport only); x/net still imported (listener.go h2c/h2c handler). No new modules. DEVIATIONS D-DEP-3 updated (I removed a duplicated Rationale: line left by the edit — pushed as 6b33a47).

COMPAT / FAIL-CLOSED:

Plain configs (no [server.tls]) load + validate identically — verified with a throwaway TestTmpPlainCompat165 (passed, then deleted).
Residual server.tls.* (even mode=off) fails closed in BOTH channels with the reverse-proxy pointer: TOML rejected at load (load.go:129, tested self_signed/files/off); WALHUB__/WALGIT__SERVER__TLS__* env fatal (env.go:133, tested both prefixes). The env fatal returns before assignPath and only matches server.tls.* prefix — unrelated env handling untouched (full config suite green).
server.public_url still takes precedence over the header for base URLs — canonical origin behind a proxy is pinned, header is fallback only.

COVERAGE / TESTS (all in scratch worktree):

go vet ./... clean (needed a copied web/dist for the embed; node_modules symlinked read-only from main worktree — scratch had neither).
internal/config -race PASS, 95.5% statements; internal/server -race PASS, 95.5%; internal/api -race PASS, 95.5%; cmd/walhub -race PASS. (internal/server/auth has no test files — pre-existing, untouched by this PR.)
node --test web/test/unit/*.test.js: 325 pass, 0 fail.
internal/e2e: PASS (45.9s, plain HTTP).
gofmt clean on all touched trees. No browser run (no browser-facing behavior change beyond URL scheme text already covered by tests; per instructions, tests + reasoning noted).

DOCS: 01_overview, 06_server_http (§2.2/§3/§9.1-9.3/§10.4/§11 + Decisions), 11_config_cli (keys table, example, validation renumber 8/9/10, env-exception note), 12_web_ui (#124 follow-up marked resolved), 16_packaging (standalone/S3 shapes, new §3.4 Caddy+nginx snippets with the load-bearing X-Forwarded-Proto line; §4.2 reworded). Proxy snippets accurate. Nested decision recorded in same change per law 12.

FIX APPLIED DIRECTLY: DEVIATIONS.md D-DEP-3 duplicated Rationale line removed (commit 6b33a47, pushed to origin/fix/issue-165).

RECOMMENDATION: ready to merge.

Review of PR #167 (fix/issue-165, remove server-side TLS) — verified in scratch worktree /tmp/pr167 at 6b33a47 (c61c4e9 + 1 review fixup). VERDICT: ready to merge (after the 1-line fixup already pushed). DANGLING REFS (all clear, grepped myself): - No remaining tlsOn/TLSOn/TLSStruct/checkTLS/TLSServerConfig/EnsureSelfSigned/loadCACert/caPem/CASelfSigned/tlsConfigFor anywhere in *.go. No sslCAInfo, no server.tls.* keys outside the fail-closed paths + tests + docs. /services/public/ca.pem route gone from router.go (both full and setup-only mounts). install.sh.tmpl CA steps renumbered 2..6, no ca.pem. web FIELDS has zero server.tls.* entries; stale saved values surface as unknown-key (setup-form.test.js asserts this). MASTER_RUST_SPEC.md untouched — correct, it is the normative Rust reference; divergence recorded in DEVIATIONS.md D-HTTP-4. TRUST BOUNDARY (X-Forwarded-Proto): - requestScheme (server/middleware.go:209) honors X-Forwarded-Proto first value, else r.TLS, else http. Used ONLY for display URL building: api baseURL (refs.go:33), server baseURL (lfs.go:172), setup.json base (display), canonicalBrowserHost 302 target, OIDC loopback redirect-URI construction. Never gates auth or access control — verified by grep of all call sites. Cookie Secure no longer consults scheme at all (now: len(CorsOrigins)>0, middleware.go:464 + auth_oidc.go:277) — note: behind a TLS proxy without CORS origins the session cookie is non-Secure; safe on the wire (browser->proxy is HTTPS) and keeps direct plain-HTTP LAN working. Acceptable trade-off, flagging for awareness. - api/refs.go duplicates the proto-parsing instead of sharing requestScheme — justified by layering (api must not import server); fine. DEPENDENCY BUDGET: x/crypto still imported (sshd.go, command.go, sshkeys.go — SSH transport only); x/net still imported (listener.go h2c/h2c handler). No new modules. DEVIATIONS D-DEP-3 updated (I removed a duplicated Rationale: line left by the edit — pushed as 6b33a47). COMPAT / FAIL-CLOSED: - Plain configs (no [server.tls]) load + validate identically — verified with a throwaway TestTmpPlainCompat165 (passed, then deleted). - Residual server.tls.* (even mode=off) fails closed in BOTH channels with the reverse-proxy pointer: TOML rejected at load (load.go:129, tested self_signed/files/off); WALHUB__/WALGIT__SERVER__TLS__* env fatal (env.go:133, tested both prefixes). The env fatal returns before assignPath and only matches server.tls.* prefix — unrelated env handling untouched (full config suite green). - server.public_url still takes precedence over the header for base URLs — canonical origin behind a proxy is pinned, header is fallback only. COVERAGE / TESTS (all in scratch worktree): - go vet ./... clean (needed a copied web/dist for the embed; node_modules symlinked read-only from main worktree — scratch had neither). - internal/config -race PASS, 95.5% statements; internal/server -race PASS, 95.5%; internal/api -race PASS, 95.5%; cmd/walhub -race PASS. (internal/server/auth has no test files — pre-existing, untouched by this PR.) - node --test web/test/unit/*.test.js: 325 pass, 0 fail. - internal/e2e: PASS (45.9s, plain HTTP). - gofmt clean on all touched trees. No browser run (no browser-facing behavior change beyond URL scheme text already covered by tests; per instructions, tests + reasoning noted). DOCS: 01_overview, 06_server_http (§2.2/§3/§9.1-9.3/§10.4/§11 + Decisions), 11_config_cli (keys table, example, validation renumber 8/9/10, env-exception note), 12_web_ui (#124 follow-up marked resolved), 16_packaging (standalone/S3 shapes, new §3.4 Caddy+nginx snippets with the load-bearing X-Forwarded-Proto line; §4.2 reworded). Proxy snippets accurate. Nested decision recorded in same change per law 12. FIX APPLIED DIRECTLY: DEVIATIONS.md D-DEP-3 duplicated Rationale line removed (commit 6b33a47, pushed to origin/fix/issue-165). RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-05 23:30:32 +00:00

crueber commented

2026-09-05 23:30:32 +00:00

Author

Owner

Fixed by PR #167 incl. review doc fixup (TLS modes/certs removed, proxy snippets, X-Forwarded-Proto honored display-only; all gates green), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 23:30:32 +00:00

Merge pull request 'Fix #165: remove server-side TLS' (#167) from fix/issue-165 into main

crueber added the

git storage

label

2026-09-10 22:26:41 +00:00

crueber added this to the v1 milestone

2026-09-10 22:27:16 +00:00