From 4d7d1b97dcde8853ca9bfadd8a56279ccc93ab13 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Tue, 4 Aug 2026 15:26:30 -0300 Subject: [PATCH 1/2] fix(gateway): stop the healthcheck asking for a name nothing serves The container healthcheck probed http://gateway/healthcheck. That name is the container's own name on the Docker network, never one of the site addresses this proxy serves, so Caddy answered it the way it answers any name it does not recognise: with an empty 200. The probe called the gateway healthy whatever state the stack was in, and a made-up path got exactly the same answer as the real one. With SHELLHUB_AUTO_SSL on it stops being harmless. Automatic HTTPS redirects that name to itself over HTTPS, where no certificate can match it, so the handshake fails and the container never becomes healthy. Every deployment serving TLS has this today. Nothing acts on the status, so it surfaces only as an unhealthy label on a container that plainly works. It now answers on healthcheck.internal, a name of its own, written http:// so automatic HTTPS cannot redirect it. Compose resolves that name to 127.0.0.1 inside the container, so it shares port 80 with the main site without either matching the other's requests. Whether the API answers is left to the API's own healthcheck, which server and ui already have. --- docker-compose.yml | 4 +++- gateway/Caddyfile.tmpl | 12 ++++++++++++ gateway/caddy_test.go | 16 ++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/docker-compose.yml b/docker-compose.yml index 99fca07e628..6585c060532 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -94,8 +94,10 @@ services: - ${SHELLHUB_BIND_ADDRESS}:${SHELLHUB_HTTP_PORT}:80 networks: - shellhub + extra_hosts: + - "healthcheck.internal:127.0.0.1" healthcheck: - test: wget -q --spider -T 5 http://gateway/healthcheck || exit 1 + test: wget -q --spider -T 5 http://healthcheck.internal/healthz || exit 1 interval: 30s start_period: 10s redis: diff --git a/gateway/Caddyfile.tmpl b/gateway/Caddyfile.tmpl index 34c660639ae..da50d3b5093 100644 --- a/gateway/Caddyfile.tmpl +++ b/gateway/Caddyfile.tmpl @@ -185,6 +185,18 @@ } } } + +# The container healthcheck. Written http:// so automatic HTTPS cannot redirect +# it, and given a name of its own so it can share port 80 with the site above: +# the probe runs inside this container, where every other name available to it is +# one this proxy does not serve. docker-compose.yml repeats the name under +# extra_hosts, so the two have to move together. It reports that the proxy is up, +# and nothing else -- the API has its own. +http://healthcheck.internal { + handle /healthz { + respond "ok" 200 + } +} {{- if $cfg.WebEndpoints }} # Every device tunnel lives on its own subdomain of this domain. One block diff --git a/gateway/caddy_test.go b/gateway/caddy_test.go index e7870f2f7fe..ab40954ce22 100644 --- a/gateway/caddy_test.go +++ b/gateway/caddy_test.go @@ -156,6 +156,22 @@ func TestCaddyfileTLSFollowsTheFlagAlone(t *testing.T) { assert.NotContains(t, string(rendered), "auto_https off") } +// TestHealthcheckAnswersOnEveryConfiguration asserts every shape rather than one, +// because the bug it replaces was a healthcheck that passed under one setting and +// failed under another -- and passed by accident, on Caddy's reply to a name it +// did not serve, so nobody had reason to look at it. +func TestHealthcheckAnswersOnEveryConfiguration(t *testing.T) { + for description, cfg := range configurations() { + t.Run(description, func(t *testing.T) { + rendered, err := Caddyfile(cfg) + require.NoError(t, err) + + assert.Contains(t, string(rendered), "http://healthcheck.internal", + "the healthcheck site must not depend on how TLS is configured") + }) + } +} + // TestCaddyfileStoresCertificatesOnTheVolume guards a mistake that is invisible // until a container is recreated and the CA is asked for everything again: // Caddy's default storage is a directory under $HOME, which no volume covers. From d2fb4471c1305ca450d3b6a1845d867dcfe94179 Mon Sep 17 00:00:00 2001 From: Geovanne Washington Date: Tue, 4 Aug 2026 15:27:13 -0300 Subject: [PATCH 2/2] feat(gateway): serve a certificate supplied by the operator The gateway had one way to get a certificate: ask a public CA over ACME. That leaves no way to run HTTPS on a name no public authority will sign, which is every internal hostname and every domain a deployment does not own. SHELLHUB_AUTO_SSL could only mean "go and obtain one", and against such a name it fails and keeps retrying for as long as the process runs. SHELLHUB_TLS_CERT_FILE and SHELLHUB_TLS_KEY_FILE name a certificate and its key to serve as they are. Naming them also stops the ACME request, because a site with no tls directive gets Caddy's default, which is to go and ask for one. The paths are read inside the gateway container, so the certificate has to be mounted in. Both or neither, checked even with TLS off: a half-set pair is a typo in every case, and the alternative to refusing it at startup is quietly asking a public CA instead of serving what was supplied. --- .env | 13 +++++++++++++ docker-compose.yml | 2 ++ gateway/Caddyfile.tmpl | 5 +++++ gateway/README.md | 10 ++++++++++ gateway/caddy.go | 5 +++++ gateway/caddy_test.go | 24 ++++++++++++++++++++++++ gateway/config.go | 12 +++++++++++- 7 files changed, 70 insertions(+), 1 deletion(-) diff --git a/.env b/.env index 5e971a13baa..38e5d489897 100644 --- a/.env +++ b/.env @@ -34,6 +34,19 @@ SHELLHUB_PROXY=false # Enable automatic HTTPS with Let's Encrypt. SHELLHUB_AUTO_SSL=false +# Path to a TLS certificate for the gateway to serve as-is, instead of obtaining +# one from Let's Encrypt. The only way to run HTTPS on a name no public CA will +# sign: an internal hostname, or a domain this deployment does not own. +# NOTICE: Only used when automatic HTTPS is enabled. +# NOTICE: Set both this and SHELLHUB_TLS_KEY_FILE, or the gateway will not start. +# VALUES: An absolute path inside the gateway container +SHELLHUB_TLS_CERT_FILE= + +# Path to the private key for SHELLHUB_TLS_CERT_FILE. +# NOTICE: Set both this and SHELLHUB_TLS_CERT_FILE, or the gateway will not start. +# VALUES: An absolute path inside the gateway container +SHELLHUB_TLS_KEY_FILE= + SHELLHUB_DATABASE=postgres SHELLHUB_POSTGRES_HOST=postgres diff --git a/docker-compose.yml b/docker-compose.yml index 6585c060532..b6985b5103a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -85,6 +85,8 @@ services: - SHELLHUB_EDITION=${SHELLHUB_EDITION} - SHELLHUB_AUTO_SSL=${SHELLHUB_AUTO_SSL} - SHELLHUB_ACME_CA_SERVER=${SHELLHUB_ACME_CA_SERVER} + - SHELLHUB_TLS_CERT_FILE=${SHELLHUB_TLS_CERT_FILE} + - SHELLHUB_TLS_KEY_FILE=${SHELLHUB_TLS_KEY_FILE} - SHELLHUB_GATEWAY_ACCESS_LOGS=${SHELLHUB_GATEWAY_ACCESS_LOGS} - SHELLHUB_DATABASE=${SHELLHUB_DATABASE} depends_on: diff --git a/gateway/Caddyfile.tmpl b/gateway/Caddyfile.tmpl index da50d3b5093..12fa359773f 100644 --- a/gateway/Caddyfile.tmpl +++ b/gateway/Caddyfile.tmpl @@ -70,6 +70,11 @@ } {{ .SiteAddress }} { +{{- if and .TLS .TLSCertFile .TLSKeyFile }} + # Also what keeps Caddy off ACME: with no tls directive the default applies, + # which is to ask a public authority for this name. + tls {{ .TLSCertFile }} {{ .TLSKeyFile }} +{{- end }} encode gzip zstd # X-Forwarded-Port carries the port the client addressed, which is the one in diff --git a/gateway/README.md b/gateway/README.md index c0cec7df9ce..265dbbe803c 100644 --- a/gateway/README.md +++ b/gateway/README.md @@ -46,6 +46,16 @@ Losing it means asking the CA for everything again, and Let's Encrypt caps how o answer, so a deployment that cares should mount a named volume there rather than rely on the anonymous one Docker creates. +`SHELLHUB_TLS_CERT_FILE` and `SHELLHUB_TLS_KEY_FILE` serve a certificate the operator supplies +instead of obtaining one, which is the only way to run HTTPS on a name no public authority will +sign: an internal hostname, or a domain this deployment does not own. Both are paths inside the +gateway container, so whatever holds them has to be mounted there. + +They are read only when `SHELLHUB_AUTO_SSL` is enabled, and they are all or nothing: setting one +without the other stops the gateway from starting, checked even with automatic HTTPS off. A +half-set pair is a typo in every case, and the alternative to refusing it is quietly asking a +public CA for a name the operator meant to serve themselves. + ### Behind a load balancer `SHELLHUB_PROXY` enables the PROXY protocol, and `SHELLHUB_PROXY_TRUSTED_IPS` names the peers diff --git a/gateway/caddy.go b/gateway/caddy.go index 6a7a54ecefe..bbe92daa2c6 100644 --- a/gateway/caddy.go +++ b/gateway/caddy.go @@ -34,6 +34,8 @@ type caddyfileData struct { ACMECAServer string TrustedProxies string + TLSCertFile string + TLSKeyFile string // WebEndpointsInternalTLS asks for a locally-signed certificate instead of // a public one. Caddy falls back to its internal authority on its own for a @@ -90,6 +92,9 @@ func newCaddyfileData(cfg *GatewayConfig) *caddyfileData { ACMECAServer: cfg.ACMECAServer, TrustedProxies: cfg.ProxyTrustedIPs, + TLSCertFile: cfg.TLSCertFile, + TLSKeyFile: cfg.TLSKeyFile, + WebEndpointsInternalTLS: !certmagic.SubjectQualifiesForPublicCert("*." + domain), } } diff --git a/gateway/caddy_test.go b/gateway/caddy_test.go index ab40954ce22..1b68a51a5b0 100644 --- a/gateway/caddy_test.go +++ b/gateway/caddy_test.go @@ -47,6 +47,11 @@ func configurations() map[string]*GatewayConfig { "development": with(func(c *GatewayConfig) { c.Env = "development"; c.EnableEnterprise = true }), "dev community": with(func(c *GatewayConfig) { c.Env = "development" }), "no access logs": with(func(c *GatewayConfig) { c.EnableAccessLogs = false }), + "supplied certificate": with(func(c *GatewayConfig) { + c.EnableAutoSSL = true + c.TLSCertFile = "/etc/shellhub/certs/fullchain.pem" + c.TLSKeyFile = "/etc/shellhub/certs/privkey.pem" + }), } } @@ -156,6 +161,25 @@ func TestCaddyfileTLSFollowsTheFlagAlone(t *testing.T) { assert.NotContains(t, string(rendered), "auto_https off") } +// TestCaddyfileServesTheSuppliedCertificate pins both halves of the conditional, +// because a mistake in either is silent: the directive missing means an ACME +// request nobody asked for, and the directive appearing when no certificate was +// supplied means a site that cannot serve at all. +func TestCaddyfileServesTheSuppliedCertificate(t *testing.T) { + cfg := configurations()["supplied certificate"] + + rendered, err := Caddyfile(cfg) + require.NoError(t, err) + + assert.Contains(t, string(rendered), "tls "+cfg.TLSCertFile+" "+cfg.TLSKeyFile) + + without, err := Caddyfile(configurations()["auto ssl"]) + require.NoError(t, err) + + assert.NotContains(t, string(without), "tls ", + "automatic issuance is the default and must stay untouched when no certificate is supplied") +} + // TestHealthcheckAnswersOnEveryConfiguration asserts every shape rather than one, // because the bug it replaces was a healthcheck that passed under one setting and // failed under another -- and passed by accident, on Caddy's reply to a name it diff --git a/gateway/config.go b/gateway/config.go index 33d6dcee74a..b2c664c30db 100644 --- a/gateway/config.go +++ b/gateway/config.go @@ -54,7 +54,17 @@ type GatewayConfig struct { // ACMECAServer is the directory a certificate is asked for. Empty means the // default, which is Let's Encrypt's production endpoint; point it at their // staging endpoint to rehearse without spending the real rate limit. - ACMECAServer string `env:"SHELLHUB_ACME_CA_SERVER"` + ACMECAServer string `env:"SHELLHUB_ACME_CA_SERVER"` + + // TLSCertFile and TLSKeyFile name a certificate to serve instead of obtaining + // one, which is the only way to run TLS on a name no public authority will + // sign. + // + // Both or neither, checked even with TLS off: a half-set pair is a typo in + // every case, and the alternative to refusing it is quietly asking a public + // CA instead of serving what the operator supplied. + TLSCertFile string `env:"SHELLHUB_TLS_CERT_FILE" validate:"required_with=TLSKeyFile"` + TLSKeyFile string `env:"SHELLHUB_TLS_KEY_FILE" validate:"required_with=TLSCertFile"` Database string `env:"SHELLHUB_DATABASE,default=mongo"` EnableAccessLogs bool `env:"SHELLHUB_GATEWAY_ACCESS_LOGS,default=true"`