From e0b80fe1bbc1572ecded7424a9f77f858040878d Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Wed, 2 Sep 2026 15:31:39 -0400 Subject: [PATCH 1/6] feat: dial an app service with the scheme it published The resolve endpoint kept only host:port and the reverse proxy hardcoded `proxy_pass http://`, so a service that terminates TLS itself was dialed as cleartext. That is not a quiet loss of encryption: a plaintext request at a TLS listener is refused, so those apps break outright the moment `apps.service_proxy_domain` is set. Two shipped apps front themselves with Caddy and already publish an https URL, so this is a prerequisite for enabling the proxy at all rather than a hardening step. The scheme travels as a second header, `X-Fg-Upstream-Scheme`, rather than as a prefix on the existing one. A reverse proxy configured before this existed then ignores a header it does not know and keeps dialing http, exactly as it does today; folding the scheme in would instead have it build `proxy_pass http://https://host:port`. `upstream_from_service_url` is left alone. It is the header-injection and proxy_pass-interpolation gate, and its shape is worth not disturbing, so the scheme comes from a sibling function that the endpoint calls only after the authority has been accepted. Anything that is not exactly https yields http, which is the safe direction: guessing https at a plaintext listener would turn a working service into a failed handshake, and `read_service_url_file` already refuses a URL that is not http:// or https:// before it can reach the database. The scheme is cached with the upstream instead of being recomputed per hit. Hits outnumber misses by design, so deriving it on the miss path alone would leave an HTTPS service dialed as plaintext for the rest of the TTL, flapping between the two as entries expire. A `plaintext` label is counted beside the hit or miss, not instead of it, so the once-a-minute totals read as a fraction: how much of the proxied traffic is still unencrypted on the hop to the app host. No settings are added. Requiring TLS of every app would refuse TensorBoard, which has no TLS option at all. Co-Authored-By: Claude Opus 5 (1M context) --- fileglancer/apps/__init__.py | 1 + fileglancer/apps/serviceproxy.py | 46 +++++++++++++++++++++++++++----- fileglancer/server.py | 42 ++++++++++++++++++++++------- tests/test_service_proxy.py | 46 ++++++++++++++++++++++++++++++++ tests/test_service_proxy_urls.py | 31 +++++++++++++++++++++ 5 files changed, 151 insertions(+), 15 deletions(-) diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index 228b20267..15aea3075 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -54,4 +54,5 @@ reset_resolve_metrics, resolve_counts, upstream_from_service_url, + upstream_scheme_from_service_url, ) diff --git a/fileglancer/apps/serviceproxy.py b/fileglancer/apps/serviceproxy.py index c8e8338fe..50b9187ba 100644 --- a/fileglancer/apps/serviceproxy.py +++ b/fileglancer/apps/serviceproxy.py @@ -214,6 +214,29 @@ def upstream_from_service_url(service_url: Optional[str], return netloc +def upstream_scheme_from_service_url(service_url: Optional[str]) -> str: + """Return the scheme the reverse proxy should dial an upstream with. + + Only meaningful for a URL ``upstream_from_service_url`` has already + accepted; on its own this says nothing about whether the authority is safe + to dial, so the resolve endpoint calls the two in that order. + + Anything that is not exactly ``https`` yields ``http``, including a URL that + fails to parse. That is the safe direction: plaintext to a plaintext + listener is what the proxy has always done, whereas guessing ``https`` at + one turns a working service into a failed handshake. Nothing needs refusing + here either — ``read_service_url_file`` rejects a URL that is not + ``http://`` or ``https://`` before it can reach the database. + """ + if not service_url: + return 'http' + try: + scheme = urlsplit(service_url).scheme + except ValueError: + return 'http' + return 'https' if scheme.lower() == 'https' else 'http' + + # --- Resolution cache and counters --- # # The reverse proxy calls the resolve endpoint once per proxied HTTP request, so @@ -225,6 +248,9 @@ def upstream_from_service_url(service_url: Optional[str], # published its URL yet must stay a miss, or clicking "Open Service" the moment a # service comes up would fail for the whole TTL. # +# An entry holds both the upstream and the scheme to dial it with, since the +# reverse proxy needs both and only one of them is in the hostname. +# # The TTL is deliberately short. It is the window during which a job that has # stopped can still be proxied, and the RUNNING check it bypasses exists because # compute-node ports get recycled. A page load's burst is sub-second, so a few @@ -241,14 +267,20 @@ def upstream_from_service_url(service_url: Optional[str], _resolve_last_logged = 0.0 -def cached_upstream(job_id: int) -> Optional[str]: - """Return a recently resolved upstream for a job, or None to consult the DB.""" +def cached_upstream(job_id: int) -> Optional[tuple]: + """Return a recently resolved ``(upstream, scheme)``, or None to consult the DB.""" return _resolve_cache.get(job_id) -def cache_upstream(job_id: int, upstream: str) -> None: - """Remember a successful resolution for the cache's short TTL.""" - _resolve_cache[job_id] = upstream +def cache_upstream(job_id: int, upstream: str, scheme: str) -> None: + """Remember a successful resolution for the cache's short TTL. + + The scheme is stored with the upstream rather than recomputed per hit. + Hits outnumber misses by design, so a scheme derived on the miss path alone + would leave an HTTPS service being dialed as plaintext for the rest of the + TTL — and flapping between the two as entries expire. + """ + _resolve_cache[job_id] = (upstream, scheme) def record_resolve(outcome: str) -> None: @@ -258,7 +290,9 @@ def record_resolve(outcome: str) -> None: every minute says the same thing as hundreds of individual lines, and says it in a form an operator can actually read. Outcomes are coarse on purpose — 'hit', 'miss', and a refusal reason — so the line stays useful without - naming any specific job. + naming any specific job. 'plaintext' is counted in addition to a hit or a + miss, not instead of one, so it reads as a fraction of the total: how much + of the proxied traffic is still unencrypted on the hop to the app host. """ global _resolve_last_logged _resolve_counts[outcome] += 1 diff --git a/fileglancer/server.py b/fileglancer/server.py index 2a5aacb1f..e7ca434c4 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -2719,6 +2719,25 @@ async def get_job(job_id: int, return _convert_job(db_job, service_url=proxied or service_url, files=files, phase=phase) + def _resolve_ok(upstream: str, scheme: str) -> Response: + """Build the 204 the reverse proxy reads, and count a plaintext hop. + + The scheme is its own header rather than a prefix on X-Fg-Upstream so a + reverse proxy configured before this existed keeps working: it ignores + the header it does not know and dials http, exactly as it did before. + Folding the scheme into the existing header would instead have it build + `proxy_pass http://https://host:port`. + + Counting happens here so the cached and uncached paths cannot disagree + about it. + """ + if scheme != 'https': + apps_module.record_resolve("plaintext") + return Response(status_code=204, headers={ + "X-Fg-Upstream": upstream, + "X-Fg-Upstream-Scheme": scheme, + }) + @app.get("/api/apps/resolve", include_in_schema=False) async def resolve_service_upstream(request: Request): """Map a service proxy hostname to its upstream, for the reverse proxy. @@ -2730,8 +2749,10 @@ async def resolve_service_upstream(request: Request): that the job's detail page already shows, and the reverse proxy marks its location `internal` so it is not reachable from outside. - Returns 204 with X-Fg-Upstream on success and 403 for everything else, so - auth_request denies the request. + Returns 204 with X-Fg-Upstream and X-Fg-Upstream-Scheme on success, and + 403 for everything else, so auth_request denies the request. The scheme + is whatever the service published: an app that terminates TLS itself is + dialed over HTTPS instead of being downgraded to cleartext. Successful resolutions are cached for a few seconds, which is also the window in which a job that has just stopped can still be proxied. See @@ -2749,11 +2770,10 @@ async def resolve_service_upstream(request: Request): # cache and keep the database out of the hot path. Only hits are cached; # a service that has not published its URL yet must be able to start # resolving the moment it does. - upstream = apps_module.cached_upstream(job_id) - if upstream is not None: + cached = apps_module.cached_upstream(job_id) + if cached is not None: apps_module.record_resolve("hit") - return Response(status_code=204, - headers={"X-Fg-Upstream": upstream}) + return _resolve_ok(*cached) with db.get_db_session(settings.db_url) as session: db_job = db.get_job_by_id(session, job_id) @@ -2762,8 +2782,9 @@ async def resolve_service_upstream(request: Request): or db_job.status != 'RUNNING'): apps_module.record_resolve("refused_not_running") raise HTTPException(status_code=403, detail="No running service for this host") + service_url = db_job.service_url upstream = apps_module.upstream_from_service_url( - db_job.service_url, + service_url, allowed_zone=settings.apps.service_proxy_upstream_zone, allowed_networks=tuple(settings.apps.service_proxy_upstream_networks)) @@ -2771,9 +2792,12 @@ async def resolve_service_upstream(request: Request): apps_module.record_resolve("refused_no_upstream") raise HTTPException(status_code=403, detail="No usable upstream for this host") - apps_module.cache_upstream(job_id, upstream) + # Read only after the authority has been accepted: the scheme says + # nothing about whether the host is safe to dial. + scheme = apps_module.upstream_scheme_from_service_url(service_url) + apps_module.cache_upstream(job_id, upstream, scheme) apps_module.record_resolve("miss") - return Response(status_code=204, headers={"X-Fg-Upstream": upstream}) + return _resolve_ok(upstream, scheme) @app.patch("/api/jobs/{job_id}", response_model=Job, description="Rename a job") diff --git a/tests/test_service_proxy.py b/tests/test_service_proxy.py index 3cdec9436..4f0a7d93a 100644 --- a/tests/test_service_proxy.py +++ b/tests/test_service_proxy.py @@ -201,6 +201,25 @@ def test_resolve_returns_upstream(app_factory): assert resp.headers["x-fg-upstream"] == "node01:41235" +@pytest.mark.parametrize("url,scheme", [ + ("http://node01:41235/lab?token=abc", "http"), + ("https://node01:41235/lab?token=abc", "https"), +]) +def test_resolve_reports_the_published_scheme(app_factory, url, scheme): + """An app that terminates TLS itself must not be dialed as cleartext. + + Two shipped apps front themselves with Caddy and publish an https URL; a + plaintext request at their TLS listener is answered with a 400, so dropping + the scheme here breaks them outright rather than merely leaving the hop + unencrypted.""" + app, db_url = app_factory(PROXY_DOMAIN) + job_id = _seed_running_service_with_url(db_url, url=url) + resp = _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + assert resp.status_code == 204 + assert resp.headers["x-fg-upstream"] == "node01:41235" + assert resp.headers["x-fg-upstream-scheme"] == scheme + + def test_resolve_rejects_finished_job(app_factory): """Compute-node ports get recycled. A stale subdomain must not be proxied to whatever service now holds that port on that node.""" @@ -368,9 +387,36 @@ def test_resolve_serves_repeats_from_the_cache(app_factory): second = _resolve(app, host) assert second.status_code == 204 assert second.headers["x-fg-upstream"] == "node01:41235" + assert apps.resolve_counts() == {"miss": 1, "hit": 1, "plaintext": 2} + + +def test_resolve_cache_does_not_downgrade_an_https_upstream(app_factory): + """The scheme has to be cached with the upstream, not derived per miss. + + Hits outnumber misses by design, so a scheme resolved on the miss path + alone would leave an HTTPS service being dialed as cleartext for the rest + of the TTL, and flapping between the two as entries expire.""" + app, db_url = app_factory(PROXY_DOMAIN) + job_id = _seed_running_service_with_url( + db_url, url="https://node01:41235/lab?token=abc") + host = f"job-{job_id}.{PROXY_DOMAIN}" + + assert _resolve(app, host).headers["x-fg-upstream-scheme"] == "https" + second = _resolve(app, host) + assert second.headers["x-fg-upstream-scheme"] == "https" assert apps.resolve_counts() == {"miss": 1, "hit": 1} +def test_resolve_counts_a_plaintext_hop_beside_the_hit(app_factory): + """'plaintext' is counted in addition to the hit or miss, not instead of it, + so the aggregate line reads as a fraction of the total rather than + redefining labels an operator already knows.""" + app, db_url = app_factory(PROXY_DOMAIN) + job_id = _seed_running_service_with_url(db_url) + _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + assert apps.resolve_counts() == {"miss": 1, "plaintext": 1} + + def test_resolve_does_not_cache_refusals(app_factory): """A service that has not published its URL yet must start resolving the moment it does, so a miss cannot be remembered.""" diff --git a/tests/test_service_proxy_urls.py b/tests/test_service_proxy_urls.py index 5e796b72c..95d5d6066 100644 --- a/tests/test_service_proxy_urls.py +++ b/tests/test_service_proxy_urls.py @@ -6,6 +6,7 @@ build_proxied_service_url, job_id_from_host, upstream_from_service_url, + upstream_scheme_from_service_url, ) DOMAIN = "services.example.org" @@ -253,3 +254,33 @@ def test_networks_still_refuse_loopback_inside_an_allowed_range(): must not make the app server's own loopback dialable.""" assert upstream_from_service_url( "http://127.0.0.1:8989/", allowed_networks=("127.0.0.0/8",)) is None + + +# --- upstream_scheme_from_service_url --- + +@pytest.mark.parametrize("url,expected", [ + ("https://node01:41235/lab", "https"), + ("HTTPS://node01:41235/lab", "https"), # urlsplit lowercases the scheme + ("http://node01:41235/lab", "http"), +]) +def test_scheme_follows_the_published_url(url, expected): + """A service that fronts itself with TLS is dialed over TLS. Two shipped + apps do exactly this, publishing an https URL from a Caddy sidecar.""" + assert upstream_scheme_from_service_url(url) == expected + + +@pytest.mark.parametrize("url", [ + "", + None, + "not a url", + "ftp://node01:41235/", + "//node01:41235/", +]) +def test_scheme_falls_back_to_plaintext(url): + """Anything unrecognized yields http, which is the safe direction: plaintext + to a plaintext listener is what the proxy has always done, whereas guessing + https at one turns a working service into a failed handshake. None of these + can reach this function in practice — read_service_url_file refuses a URL + that does not start with http:// or https:// — so this pins the fallback + rather than describing a live case.""" + assert upstream_scheme_from_service_url(url) == "http" From a8324ae28bc939e14bf3e8441422217849fef698 Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Wed, 2 Sep 2026 15:32:02 -0400 Subject: [PATCH 2/6] docs: document TLS to the app host, and park the certificate options MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `docs/ServiceProxy.md` described a path that is HTTPS end to end, which it was not. It now says which hop is encrypted, states plainly that upstream TLS is opportunistic and authenticates nothing, and explains why: the certificates are generated on the compute node at launch and signed by nobody, so there is no trust anchor to check and nothing stable to pin. It also warns against overriding `proxy_ssl_name`, whose default is the one name those certificates actually carry. The residual-risks note about a service that manages its own URL treated it purely as a staleness risk. The scheme in that file is now load-bearing too, so publishing the wrong one produces a hop that fails rather than one that merely works unencrypted. The parked spec records the options for actually authenticating the upstream — self-signed with pinning, an internal CA, a uniform TLS sidecar — and why each is more than a mostly trusted intranet warrants today. Written down so the reasoning is not re-derived if the deployment ever reaches a less trusted network. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ServiceProxy.md | 26 ++- ...-09-02-app-service-upstream-tls-options.md | 182 ++++++++++++++++++ 2 files changed, 204 insertions(+), 4 deletions(-) create mode 100644 docs/superpowers/specs/2026-09-02-app-service-upstream-tls-options.md diff --git a/docs/ServiceProxy.md b/docs/ServiceProxy.md index 23f3aedcd..6eed563fc 100644 --- a/docs/ServiceProxy.md +++ b/docs/ServiceProxy.md @@ -29,7 +29,7 @@ Leave it empty to disable; the direct `http://:` URL is then publish ## Reverse proxy configuration -Fileglancer does not proxy the traffic itself. It exposes `GET /api/apps/resolve`, which reads the `Host` header and answers `204` with `X-Fg-Upstream: :`, or `403`. The reverse proxy resolves each request through it and connects to the upstream directly, so no proxied bytes pass through the application server. +Fileglancer does not proxy the traffic itself. It exposes `GET /api/apps/resolve`, which reads the `Host` header and answers `204` with `X-Fg-Upstream: :` and `X-Fg-Upstream-Scheme: http|https`, or `403`. The reverse proxy resolves each request through it and connects to the upstream directly, so no proxied bytes pass through the application server. Add a server block for the wildcard zone. This assumes a `map $http_upgrade $connection_upgrade` block already exists at the http level: @@ -52,11 +52,18 @@ server { location / { auth_request /_fg_resolve; auth_request_set $upstream $upstream_http_x_fg_upstream; + auth_request_set $upscheme $upstream_http_x_fg_upstream_scheme; # Required because proxy_pass targets a variable. Use whatever resolver the # host actually runs; 127.0.0.53 is systemd-resolved's stub. resolver 127.0.0.53 valid=30s; - proxy_pass http://$upstream; + proxy_pass $upscheme://$upstream; + + # An app that terminates TLS does so with a certificate it generated on the + # compute node, so there is no trust anchor to verify against. See "TLS to + # the app host" below. Leave proxy_ssl_name at its default. + proxy_ssl_verify off; + proxy_ssl_server_name on; proxy_set_header Host $host; proxy_set_header X-Real-IP $remote_addr; @@ -79,15 +86,26 @@ Also add this to the **main** server block, so the resolve endpoint is not reach location = /api/apps/resolve { return 404; } ``` -Four details are load-bearing: +Five details are load-bearing: - **`internal;`** on the `/_fg_resolve` location makes it reachable only from nginx's own `auth_request` subrequest, never from a client. Together with the `return 404` in the main server block, it is what keeps the unauthenticated resolve endpoint off the network. Do not remove either. - **`proxy_set_header Host $host`** passes the app subdomain through unchanged, so the app sees `Host` and `Origin` as the same value. This is what makes JupyterLab's WebSocket origin check pass without per-app configuration. - **`resolver`** is mandatory. Without it nginx refuses to start when `proxy_pass` targets a variable. - **`proxy_buffering off`** and the long `proxy_read_timeout` suit long-lived WebSocket and streaming sessions, such as the remote desktop app. +- **`proxy_pass $upscheme://$upstream`**, rather than a hardcoded `http://`, is what lets an app that serves HTTPS be reached at all. Sending a plaintext request to a TLS listener does not silently lose encryption — the app rejects it — so hardcoding the scheme breaks those apps outright. The existing HTTP-to-HTTPS redirect block is typically `default_server` with `server_name _`, in which case it already covers the new subdomains. +## TLS to the app host + +The hop from the browser to the reverse proxy is always HTTPS. The hop from the proxy to the compute node is whatever the service itself published: Fileglancer reports the scheme from the service's own URL file and the proxy dials it with that. An app that fronts itself with a TLS terminator — Caddy, stunnel, its own `--certfile` — is reached over HTTPS; an app that serves plain HTTP is reached over HTTP, and nothing is required of it. + +**This is opportunistic encryption, and it authenticates nothing.** `proxy_ssl_verify` is `off` because the certificates in question are generated on the compute node at launch and signed by nobody: there is no trust anchor to check them against, and the node's name and port change every launch, so there is nothing stable to pin either. What it buys is that the service's token stops crossing the node network in cleartext. What it does not buy is any assurance that the thing answering on that host and port is the service — the upstream is read from a file the user's own job wrote, so the proxy could not make that claim regardless of certificates. + +Leave `proxy_ssl_name` at its default, which is the host from `proxy_pass`. Apps that generate their own certificate name it after the compute node, so overriding this to `$host` (the `job-` subdomain) would send an SNI value no app's certificate carries. + +To see how much traffic is still cleartext, read the `plaintext` figure in the aggregate log line described below. It counts alongside `hit` and `miss` rather than instead of them, so it reads as a fraction of the total. + ## Verification Once DNS and the certificate are in place, launch each service app and confirm it loads and stays connected. WebSocket behavior is the thing to watch: @@ -106,4 +124,4 @@ If an app rejects the proxied origin, fix it in that app's manifest (most server - Per-job subdomains make a running service much cheaper to *find* — `https://job-./` instead of port-scanning compute nodes — and the proxy vhost is reachable from wherever the Fileglancer HTTPS host is. The service's own token is still the only credential, but weigh this before exposing the vhost on a wide network. - The resolve endpoint is called once per proxied HTTP request, so a single page load of an app like JupyterLab generates dozens. Successful resolutions are cached in-process for 10 seconds, which collapses that burst to roughly one database read per service per 10 seconds per worker. Refusals are deliberately not cached, so a service starts resolving the moment it publishes its URL. The endpoint is excluded from the per-request access log for the same reason and reports running totals once a minute instead — grep for `service proxy resolve totals` to see hits, misses and refusals by reason. - That 10-second cache is also the window in which a job that has just stopped can still be proxied. Compute-node ports get recycled, so the window is kept short deliberately; if a port is reused within it, a client can briefly reach the new occupant, which will reject it for lack of that service's own token. -- A service that manages its own URL (`auto_url` unset) should write its URL file exactly once. The cached upstream is refreshed only while someone has the job's detail page open, so a URL that changes mid-run can go stale. +- A service that manages its own URL (`auto_url` unset) should write its URL file exactly once, and should write the scheme it actually serves. Both the upstream and its scheme are taken from that file: publishing `http://` for a listener that speaks TLS, or the reverse, produces a hop that fails rather than one that merely works unencrypted. The cached upstream is refreshed only while someone has the job's detail page open, so a URL that changes mid-run can go stale. diff --git a/docs/superpowers/specs/2026-09-02-app-service-upstream-tls-options.md b/docs/superpowers/specs/2026-09-02-app-service-upstream-tls-options.md new file mode 100644 index 000000000..e01e7973c --- /dev/null +++ b/docs/superpowers/specs/2026-09-02-app-service-upstream-tls-options.md @@ -0,0 +1,182 @@ +# Authenticating the app-service proxy's upstream hop — options, parked + +Deferred design notes, not a plan. Split out of #440 and +JaneliaSciComp/fileglancer-hub#15, which give the browser HTTPS to nginx and +leave the nginx→app-host hop plain HTTP. + +**Status: parked as too heavy for a mostly trusted intranet.** Recorded so the +reasoning is not re-derived if the deployment ever reaches a less trusted +network — a wider vhost exposure, a cloud burst, or a multi-tenant cluster. +The low-effort step actually taken is opportunistic encryption only (option A +below): the proxy dials whatever scheme the service published, and does not +verify the certificate. + +## Two apps already serve TLS with self-signed certificates + +Found after the options below were first written, and it changes their weighting +rather than any of their mechanics. + +- **`JaneliaSciComp/marimo_ai_sandbox`** — `container/caddy-lib.sh`'s + `caddy_generate_cert` makes an `openssl` self-signed EC P-256 leaf, 10-year + validity, persisted per host under `CERT_DIR` and regenerated only when the + hostname changes. SANs are the node FQDN, the short hostname, `localhost` and + the host's IP addresses. Caddy serves it with `tls $CERT_FILE $KEY_FILE` (not + Caddy's internal-CA issuer, which shells out to `sudo` and hangs on a compute + node). `container/https-wrap.sh:262` publishes + `https://:/?access_token=…`. It bypasses `auto_url` specifically + so Fileglancer does not publish the backend's plain-HTTP port instead of + Caddy's TLS one. +- **`JaneliaSciComp/mhs-spec-review#1`** — Caddy with `tls internal` + (Caddy's own internal CA, `skip_install_trust`) on a fixed port 8443, and + writes `https://$(hostname -f):8443/`. + +Neither certificate is trusted by any system store, so both need option A. +Neither app can be verified under option C without changing how it generates +certificates. + +## The constraint that shapes all of it + +The upstream is a new node and port on every launch, sometimes published as a +bare IP address, and it arrives in a file the user's own job wrote. Nothing +about it is stable enough to name in static configuration. + +## A. Self-signed, unverified — `proxy_ssl_verify off` + +Confidentiality against passive capture on the node network. No authentication +of the upstream. Not a regression, since nothing authenticates the upstream +today either, but it has to be documented as opportunistic encryption rather +than as verified TLS. **This is the option being pursued.** + +It is also the only option the two existing TLS apps work under as written. +Leave `proxy_ssl_name` at its default (the `proxy_pass` host) rather than +setting it to `$host`: both apps' certificates are SAN'd to the node's own +name, which is what the default sends. + +## B. Self-signed with fingerprint pinning + +What would make self-signed meaningful, and nginx cannot do it: +`proxy_ssl_trusted_certificate` is a static path with no variable support, and +there is no upstream-fingerprint directive. + +Workarounds are worse than they sound. Appending each job's certificate to a +trust bundle and reloading nginx per service launch is racy, and the bundle +grows without bound. Moving the hop onto something that can pin per connection +(ghostunnel, Envoy with ext_authz) introduces a new component that carries the +noVNC video stream — the thing #440 went out of its way to avoid. + +Considered and rejected on its own merits, independent of the intranet +judgement. + +## C. Internal CA + +Issue a short-lived leaf per service job with +`SAN = job-.services.int.janelia.org`, which is exactly the value nginx +already has in `$host`: + +```nginx +proxy_ssl_trusted_certificate /etc/nginx/certs/fg-app-ca.pem; +proxy_ssl_verify on; +proxy_ssl_verify_depth 2; +proxy_ssl_server_name on; +proxy_ssl_name $host; +``` + +The verification config is fully static, and the certificate binds to the *job* +rather than to the node. That is what makes it tractable at all when the node +name changes every launch and is sometimes an IP address that name-based +verification could never cover. + +Issuance would fit where the plumbing already is: write `service_tls.{crt,key}` +(0600, user-owned) into the work dir at submit time and export +`FG_SERVICE_TLS_CERT` / `FG_SERVICE_TLS_KEY` from the preamble at +`fileglancer/apps/jobs.py`, next to the `SERVICE_URL_PATH` export. Lifetime = max +walltime plus slack. The CA key stays root-only on the Fileglancer host and off +the shared filesystem, or comes from step-ca / Vault if one is already run. + +Strictly this is "we are our own CA," not per-app self-signed. + +Note the interaction with the two existing TLS apps: `proxy_ssl_name $host` +sends the job subdomain as the SNI and requires it in the leaf's SAN, which +neither app's self-generated certificate has. Adopting C therefore means +converting both apps to consume an issued certificate, not just adding nginx +directives — a migration cost the sketch above hides. + +Parked because it means running a CA — key custody, rotation, an issuance path +in the submit hot path — to defend against an active on-path attacker inside +the cluster network. That is not the threat model here. + +## Non-option, recorded so it is not re-proposed + +Compute nodes are `.int.janelia.org`, so the existing org wildcard +certificate would cover them with no new PKI at all. Rejected: its private key +would have to be readable by every user's job. + +## The app side: getting services to serve TLS + +Support is uneven across the shipped apps. Jupyter has `--certfile` / +`--keyfile`; openvscode-server and websockify vary; TensorBoard has none. +Per-manifest TLS flags therefore mean an open-ended per-app tail — the same trap +the subdomain-over-path-prefix decision in #440 was made to avoid. + +The uniform alternative, if this is ever revisited: keep the app on +`127.0.0.1:$PORT` and put a TLS terminator in the job wrapper, binding the +public port with the issued certificate. One change in the `jobs.py` preamble +instead of N manifest changes, and `auto_url` apps get it for free. It also +closes a hole that exists independent of all this — the raw app port is +reachable by any cluster user today. With ghostunnel, nginx could additionally +present a `proxy_ssl_certificate` that the sidecar requires, making the app +reachable *only* through the proxy. + +This is not hypothetical: both apps above already do exactly this with Caddy, +arrived at independently, and `marimo_ai_sandbox` has already factored the +machinery into a reusable `caddy-lib.sh` shared by its HTTPS and web-terminal +wrappers. If a uniform terminator is ever wanted, that file is the closest +thing to a prototype, and Caddy is the more likely choice than stunnel purely +because two apps already depend on it. + +Parked with C only insofar as *issued* certificates go: the sidecar needs +something to bind with, and without an issuance story it can only bind a +self-signed certificate — which is what these two apps already do for +themselves, so a Fileglancer-provided sidecar would add nothing at option A. + +## What is not parked + +The scheme plumbing: return the upstream scheme instead of discarding it, carry +it in its own `X-Fg-Upstream-Scheme` header (including through the TTL cache, or +cached hits silently downgrade), and let nginx `proxy_pass +$upscheme://$upstream`. An earlier draft gated this on an +`apps.service_proxy_upstream_tls: allow | require` setting; that was withdrawn, +because `require` would refuse every plaintext app and TensorBoard has no TLS +option at all, which left the setting with one legal value. A `plaintext` +counter in the existing once-a-minute resolve totals gives an operator the same +visibility without refusing anyone's launch. + +Because those two apps exist, this is not an enhancement — it is a +**prerequisite for setting `apps.service_proxy_domain` at all.** Discarding the +scheme means nginx does `proxy_pass http://:8443` at a TLS listener, and +Caddy answers a plaintext request to an HTTPS port with a 400. Both apps break +the moment the proxy is enabled. + +Two further per-app breakages, independent of the scheme and belonging to those +repos rather than to Fileglancer: + +- `mhs-spec-review`'s Caddyfile site address is Host-matched + (`{$CADDY_HOSTNAME}:8443, localhost:8443, 127.0.0.1:8443`). nginx passes + `Host $host` through unchanged — load-bearing for Jupyter's WebSocket origin + check — so Caddy sees `job-.services.int.janelia.org`, matches no site + and 404s even once the scheme is right. It needs a catch-all `:8443` site + address. `marimo_ai_sandbox` uses a port-only address and is unaffected. +- That app's GitHub OAuth callback is pinned to + `https://int.janelia.org:8443/callback` with wildcard matching, and its + `url_for(_external=True)` + `ProxyFix` would produce + `https://job-.services.int.janelia.org/callback` behind the proxy — + different port, two extra labels. Whether GitHub's wildcard matching accepts + that needs checking; if not, that app wants a way to opt out of being + republished rather than a fix. + +The second one suggests a gap worth considering separately: there is currently +no way for a manifest to say "do not proxy me." + +Also: hub#15's per-app verification checklist lists only the five shipped +service apps. Both Caddy apps should be on it, since they are the only two that +exercise the TLS path at all. From 88427fadad446f590d02a143825c20a268e5e32d Mon Sep 17 00:00:00 2001 From: Mark Kittisopikul Date: Wed, 2 Sep 2026 15:39:19 -0400 Subject: [PATCH 3/6] fix: run the Prettier pre-push hook through bash `pixi run ./prettier-check-and-fix.sh` hands the script to the OS to execute, which on Windows fails with `os error 193` -- not a valid Win32 application. The hook could therefore never run on a Windows checkout, and since lefthook fails the push when a job fails, no push from one could succeed either. Naming bash explicitly runs the same script the same way everywhere. Unrelated to the rest of this branch; it is here because it is what allowed the branch to be pushed at all. Co-Authored-By: Claude Opus 5 (1M context) --- lefthook.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lefthook.yml b/lefthook.yml index 012c34a02..8d6792661 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -6,7 +6,10 @@ pre-push: jobs: - name: Run Prettier check; fix if check fails root: frontend/ - run: pixi run ./prettier-check-and-fix.sh + # Invoked through bash explicitly: on Windows, pixi hands the script to + # the OS to execute and gets `os error 193` (not a valid Win32 + # application), so the hook could never run there. + run: pixi run bash ./prettier-check-and-fix.sh - name: Run ESLint check root: frontend/ From 06516787f2e2dcd01c11991f3c709fd74fe6cc71 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 3 Sep 2026 10:26:34 -0400 Subject: [PATCH 4/6] feat: sign service proxy hostnames so they can't be enumerated Job ids are a small global sequence, so a bare job- label let anyone who could reach the proxy sweep job-1..job-500 and find every running service on the instance -- and reach any of them that does not enforce its own token, which is opt-in per manifest and absent entirely from apps like TensorBoard. The label now carries a MAC over the id, keyed by session_secret_key: job-12-k7m2q9xr.services.example.org. Signing rather than storing a random key keeps the id in the hostname, so resolution stays one indexed read with no new column, and the MAC is verified before the database is touched at all. Eight base32 characters is 40 bits; guessing is online-only against a proxy that answers 403, so a sweep takes decades at 10k requests/second. session_secret_key becomes required when service_proxy_domain is set. An unset key is generated at random per process, so under `uvicorn --workers N` each worker would sign with a different key and most proxied requests would be refused -- better to fail at startup where the cause is obvious. Co-Authored-By: Claude Opus 5 (1M context) --- docs/ServiceProxy.md | 17 ++++-- docs/config.yaml.template | 9 +-- fileglancer/apps/__init__.py | 1 + fileglancer/apps/serviceproxy.py | 68 ++++++++++++++++++----- fileglancer/server.py | 6 +- fileglancer/settings.py | 16 ++++++ tests/test_service_proxy.py | 35 +++++++----- tests/test_service_proxy_urls.py | 94 +++++++++++++++++++++++++------- 8 files changed, 189 insertions(+), 57 deletions(-) diff --git a/docs/ServiceProxy.md b/docs/ServiceProxy.md index 23f3aedcd..2c0ce6e7f 100644 --- a/docs/ServiceProxy.md +++ b/docs/ServiceProxy.md @@ -1,6 +1,12 @@ # HTTPS Proxy for App Services -Running app services (`type: service`) bind a port on a compute node and publish `http://:/...`. Set `apps.service_proxy_domain` to republish them over HTTPS at a per-job subdomain instead. +Running app services (`type: service`) bind a port on a compute node and publish `http://:/...`. Set `apps.service_proxy_domain` to republish them over HTTPS at a per-job subdomain instead: + +``` +https://job-12-k7m2q9xr.services.example.org/lab?token=8f2c... +``` + +The `job--` label is signed with `session_secret_key`. Job ids are a small global sequence, so an unsigned `job-12` label would let anyone who can reach the proxy sweep `job-1`..`job-500` and find — and, for any service that does not enforce its own token, reach — every running service on the instance. The MAC is 8 base32 characters (40 bits); guessing it is online-only against a reverse proxy that answers 403, so a sweep of that space takes decades at 10k requests/second. Design rationale: `docs/superpowers/specs/2026-08-28-app-service-https-proxy-design.md`. @@ -21,11 +27,14 @@ Design rationale: `docs/superpowers/specs/2026-08-28-app-service-https-proxy-des ## Fileglancer configuration ```yaml +session_secret_key: "" apps: service_proxy_domain: "services.example.org" ``` -Leave it empty to disable; the direct `http://:` URL is then published unchanged. +Leave `service_proxy_domain` empty to disable; the direct `http://:` URL is then published unchanged. + +`session_secret_key` is required when the proxy domain is set, and the server refuses to start without it. An unset key is generated at random per process, so under `uvicorn --workers N` each worker would sign hostnames with a different key and most proxied requests would be refused. Rotating it invalidates live service URLs, on top of the session revocation rotation already causes. ## Reverse proxy configuration @@ -36,7 +45,7 @@ Add a server block for the wildcard zone. This assumes a `map $http_upgrade $con ```nginx server { listen 443 ssl http2; - server_name ~^job-\d+\.services\.example\.org$; + server_name ~^job-\d+-[a-z2-7]+\.services\.example\.org$; ssl_certificate /etc/nginx/certs/services-wildcard.crt; ssl_certificate_key /etc/nginx/certs/services-wildcard.key; @@ -103,7 +112,7 @@ If an app rejects the proxied origin, fix it in that app's manifest (most server ## Residual risks - Set `apps.service_proxy_upstream_zone` to the DNS zone your compute nodes live in, e.g. `nodes.example.org`. Without it the proxy will dial any host the Fileglancer host can reach, because the upstream comes from a file the user's job wrote. Loopback, the unspecified address, link-local (including cloud instance metadata), multicast and reserved addresses are always refused, since those are what reach the Fileglancer host itself. Private and public node addresses are allowed — an address on a routable interface is already reachable directly by any cluster user. Matching is on whole DNS labels, so a leading dot is optional and a sibling zone like `evil-nodes.example.org` does not qualify. The zone applies to hostnames only: a service that publishes the node's IP instead of its name is still accepted, so setting a zone will not break one. To confine those, set `apps.service_proxy_upstream_networks` to the CIDR networks your nodes occupy, e.g. `10.20.0.0/16`. The two settings divide the space between them — the zone governs upstreams published as names, the networks govern upstreams published as addresses — and each is empty by default, meaning no restriction on that form. Bad CIDR entries are refused at startup rather than at request time, since a typo there would otherwise reject every address upstream while looking configured. -- Per-job subdomains make a running service much cheaper to *find* — `https://job-./` instead of port-scanning compute nodes — and the proxy vhost is reachable from wherever the Fileglancer HTTPS host is. The service's own token is still the only credential, but weigh this before exposing the vhost on a wide network. +- The signed hostname is not a substitute for a service enforcing its own token. It is unguessable, but a hostname leaks where a query string does not: plaintext SNI on the wire, DNS resolvers, and the proxy's own access log. Treat it as what makes enumeration infeasible, and `${FG_SERVICE_TOKEN}` as the credential. An app with no authentication of its own (TensorBoard, for one) is protected only by the label. - The resolve endpoint is called once per proxied HTTP request, so a single page load of an app like JupyterLab generates dozens. Successful resolutions are cached in-process for 10 seconds, which collapses that burst to roughly one database read per service per 10 seconds per worker. Refusals are deliberately not cached, so a service starts resolving the moment it publishes its URL. The endpoint is excluded from the per-request access log for the same reason and reports running totals once a minute instead — grep for `service proxy resolve totals` to see hits, misses and refusals by reason. - That 10-second cache is also the window in which a job that has just stopped can still be proxied. Compute-node ports get recycled, so the window is kept short deliberately; if a port is reused within it, a client can briefly reach the new occupant, which will reject it for lack of that service's own token. - A service that manages its own URL (`auto_url` unset) should write its URL file exactly once. The cached upstream is refreshed only while someone has the job's detail page open, so a URL that changes mid-run can go stale. diff --git a/docs/config.yaml.template b/docs/config.yaml.template index c06ed1dee..70c0bb759 100644 --- a/docs/config.yaml.template +++ b/docs/config.yaml.template @@ -197,10 +197,11 @@ session_cookie_secure: true # # Wildcard DNS zone serving per-job HTTPS # # subdomains for running services. When set, a # # service is published at -# # https://job-./ instead of its own -# # http://:/ URL. Needs a wildcard -# # certificate for the zone and a reverse proxy -# # resolving upstreams via /api/apps/resolve; +# # https://job--./ instead of its +# # own http://:/ URL. Needs a wildcard +# # certificate for the zone, a reverse proxy +# # resolving upstreams via /api/apps/resolve, and +# # session_secret_key (which signs the hostname); # # see docs/ServiceProxy.md. Empty = direct URL. # service_proxy_upstream_zone: nodes.example.org # # DNS zone the proxy is allowed to dial, i.e. diff --git a/fileglancer/apps/__init__.py b/fileglancer/apps/__init__.py index 228b20267..581f608e1 100644 --- a/fileglancer/apps/__init__.py +++ b/fileglancer/apps/__init__.py @@ -53,5 +53,6 @@ record_resolve, reset_resolve_metrics, resolve_counts, + service_host_label, upstream_from_service_url, ) diff --git a/fileglancer/apps/serviceproxy.py b/fileglancer/apps/serviceproxy.py index c8e8338fe..afdd7c4ad 100644 --- a/fileglancer/apps/serviceproxy.py +++ b/fileglancer/apps/serviceproxy.py @@ -2,11 +2,14 @@ A service job publishes ``http://:`` to its work directory. When a proxy domain is configured, Fileglancer republishes that as -``https://job-.`` and tells the reverse proxy which -upstream the hostname maps to. These functions are the whole translation layer -between the two forms, kept pure so they can be tested without a database. +``https://job--.`` and tells the reverse proxy +which upstream the hostname maps to. These functions are the whole translation +layer between the two forms, kept pure so they can be tested without a database. """ +import base64 +import hashlib +import hmac import ipaddress import re import time @@ -28,45 +31,82 @@ # aim the proxy at the web host itself rather than at a compute node. _LOCAL_NAMES = frozenset({'localhost'}) +# Job ids are a small global sequence, so a bare job- label would let anyone +# who can reach the proxy sweep job-1..job-500 and find every running service on +# the instance — and reach any of them that does not enforce its own token. The +# label therefore carries a MAC over the id, keyed by the server's session +# secret. Signing rather than storing a random key keeps the id in the hostname, +# so resolution stays one indexed read with no extra column, and the MAC is +# checked before the database is touched at all. +# +# Eight base32 characters is 40 bits. Guessing is online-only — a wrong label is +# a 403 from the reverse proxy, and there is nothing to attack offline — so at +# 10k requests/second a sweep of the space takes decades. Rotating +# session_secret_key invalidates live service URLs, which is the same blast +# radius as the session revocation that rotation already causes. +_MAC_CHARS = 8 + + +def _service_mac(job_id: int, secret: str) -> str: + """The MAC for a job's proxy hostname label.""" + digest = hmac.new(secret.encode('utf-8'), + f'job:{job_id}'.encode('utf-8'), + hashlib.sha256).digest() + # base32 rather than hex: same DNS-safe alphabet cost, 25% fewer characters + # per bit. Lowercased because hostnames are compared case-insensitively. + return base64.b32encode(digest).decode('ascii').lower()[:_MAC_CHARS] + + +def service_host_label(job_id: int, secret: str) -> str: + """The leftmost hostname label for a job's proxy URL, e.g. ``job-12-k7m2q9xr``.""" + return f'job-{job_id}-{_service_mac(job_id, secret)}' + def build_proxied_service_url(service_url: Optional[str], job_id: int, - proxy_domain: str) -> Optional[str]: + proxy_domain: str, secret: str) -> Optional[str]: """Rewrite a published service URL to its HTTPS proxy form. Path, query and fragment are carried over verbatim: the query string holds the service's own access token, which remains the only credential. - Returns None when there is nothing to rewrite or no proxy domain is - configured, in which case the caller should publish the URL unchanged. + Returns None when there is nothing to rewrite, no proxy domain is + configured, or no secret is available to sign the hostname with, in which + case the caller should publish the URL unchanged. """ - if not service_url or not proxy_domain: + if not service_url or not proxy_domain or not secret: return None parts = urlsplit(service_url) return urlunsplit(( 'https', - f'job-{job_id}.{proxy_domain}', + f'{service_host_label(job_id, secret)}.{proxy_domain}', parts.path, parts.query, parts.fragment, )) -def job_id_from_host(host: Optional[str], proxy_domain: str) -> Optional[int]: +def job_id_from_host(host: Optional[str], proxy_domain: str, + secret: str) -> Optional[int]: """Extract the job id from a proxy hostname, or None if it isn't one. Matches the whole hostname, so neither a longer suffix - (``job-1.services.example.org.evil``) nor an extra label - (``x.job-1.services.example.org``) is accepted. + (``job-1-k7m2q9xr.services.example.org.evil``) nor an extra label + (``x.job-1-k7m2q9xr.services.example.org``) is accepted, and the label's MAC + must verify against the id it carries. """ - if not host or not proxy_domain: + if not host or not proxy_domain or not secret: return None # $host in nginx normally omits the port, but a client can send one. hostname = host.split(':', 1)[0].strip().lower() match = re.fullmatch( - r'job-(\d{1,9})\.' + re.escape(proxy_domain.lower()), hostname) + r'job-(\d{1,9})-([a-z2-7]{%d})\.' % _MAC_CHARS + + re.escape(proxy_domain.lower()), hostname) if match is None: return None - return int(match.group(1)) + job_id = int(match.group(1)) + if not hmac.compare_digest(match.group(2), _service_mac(job_id, secret)): + return None + return job_id def _is_ip_literal(host: str) -> bool: diff --git a/fileglancer/server.py b/fileglancer/server.py index 2a5aacb1f..410444cfd 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -2715,7 +2715,8 @@ async def get_job(job_id: int, # The cached value stays raw — it is the proxy's upstream. Only what # goes back to the browser is rewritten. proxied = apps_module.build_proxied_service_url( - service_url, job_id, settings.apps.service_proxy_domain) + service_url, job_id, settings.apps.service_proxy_domain, + settings.session_secret_key) return _convert_job(db_job, service_url=proxied or service_url, files=files, phase=phase) @@ -2740,7 +2741,8 @@ async def resolve_service_upstream(request: Request): """ proxy_domain = settings.apps.service_proxy_domain job_id = apps_module.job_id_from_host( - request.headers.get('host'), proxy_domain) + request.headers.get('host'), proxy_domain, + settings.session_secret_key) if job_id is None: apps_module.record_resolve("refused_bad_host") raise HTTPException(status_code=403, detail="Not a service proxy host") diff --git a/fileglancer/settings.py b/fileglancer/settings.py index 8a98cd05a..bff67106d 100644 --- a/fileglancer/settings.py +++ b/fileglancer/settings.py @@ -285,6 +285,22 @@ def settings_customise_sources( # noqa: PLR0913 file_secret_settings, ) + @model_validator(mode='after') + def validate_service_proxy_needs_a_session_secret(self): + """The service proxy signs its hostnames with session_secret_key. + + An unset key is generated at random per process, so under + `uvicorn --workers N` each worker would sign with a different key and + most proxied requests would be refused. Fail at startup instead, where + the cause is obvious. + """ + if self.apps.service_proxy_domain and not self.session_secret_key: + raise ValueError( + "apps.service_proxy_domain requires session_secret_key to be " + "set: service proxy hostnames are signed with it, and a " + "per-process random key breaks resolution across workers") + return self + @model_validator(mode='after') def set_jira_browse_url(self): if self.jira_browse_url is None: diff --git a/tests/test_service_proxy.py b/tests/test_service_proxy.py index 3cdec9436..bfe19dcc7 100644 --- a/tests/test_service_proxy.py +++ b/tests/test_service_proxy.py @@ -23,6 +23,12 @@ OWNER = "alice" PROXY_DOMAIN = "services.example.org" +SECRET = "test-session-secret" + + +def _host(job_id): + """The signed proxy hostname for a job, as the API would publish it.""" + return f"{apps.service_host_label(job_id, SECRET)}.{PROXY_DOMAIN}" @pytest.fixture @@ -54,6 +60,7 @@ def _build(proxy_domain="", upstream_zone=""): db_url=db_url, file_share_mounts=[], cli_mode=True, + session_secret_key=SECRET, apps=AppsSettings(service_proxy_domain=proxy_domain, service_proxy_upstream_zone=upstream_zone), ), db_url @@ -196,7 +203,7 @@ def _seed_running_service_with_url(db_url, url="http://node01:41235/lab?token=ab def test_resolve_returns_upstream(app_factory): app, db_url = app_factory(PROXY_DOMAIN) job_id = _seed_running_service_with_url(db_url) - resp = _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + resp = _resolve(app, _host(job_id)) assert resp.status_code == 204 assert resp.headers["x-fg-upstream"] == "node01:41235" @@ -212,7 +219,7 @@ def test_resolve_rejects_finished_job(app_factory): session.commit() finally: session.close() - resp = _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + resp = _resolve(app, _host(job_id)) assert resp.status_code == 403 assert "x-fg-upstream" not in resp.headers @@ -225,24 +232,26 @@ def test_resolve_rejects_non_service_job(app_factory): set_job_service_url(session, job_id, "http://node01:41235/") finally: session.close() - assert _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}").status_code == 403 + assert _resolve(app, _host(job_id)).status_code == 403 def test_resolve_rejects_job_without_cached_url(app_factory): app, db_url = app_factory(PROXY_DOMAIN) job_id = _seed_service_job(db_url) - assert _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}").status_code == 403 + assert _resolve(app, _host(job_id)).status_code == 403 def test_resolve_rejects_unknown_job(app_factory): app, db_url = app_factory(PROXY_DOMAIN) - assert _resolve(app, f"job-999999.{PROXY_DOMAIN}").status_code == 403 + assert _resolve(app, _host(999999)).status_code == 403 @pytest.mark.parametrize("host", [ "fileglancer.example.org", "job-1.evil.example.org", "job-abc.services.example.org", + "job-1.services.example.org", # unsigned: the old, guessable form + "job-1-aaaaaaaa.services.example.org", # wrong MAC ]) def test_resolve_rejects_bad_hosts(app_factory, host): app, db_url = app_factory(PROXY_DOMAIN) @@ -254,7 +263,7 @@ def test_resolve_disabled_when_no_proxy_domain(app_factory): """With the feature off, the endpoint refuses everything.""" app, db_url = app_factory("") job_id = _seed_running_service_with_url(db_url) - assert _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}").status_code == 403 + assert _resolve(app, _host(job_id)).status_code == 403 # --- proxied URL publication --- @@ -283,7 +292,7 @@ def test_job_detail_publishes_proxied_url(app_factory, monkeypatch): app, job_id, "http://node01:41235/lab?token=abc", monkeypatch) assert resp.status_code == 200 assert resp.json()["service_url"] == \ - f"https://job-{job_id}.{PROXY_DOMAIN}/lab?token=abc" + f"https://{_host(job_id)}/lab?token=abc" def test_job_detail_publishes_raw_url_when_proxy_disabled(app_factory, monkeypatch): @@ -315,7 +324,7 @@ def test_resolve_rejects_malformed_cached_url(app_factory): app, db_url = app_factory(PROXY_DOMAIN) job_id = _seed_running_service_with_url( db_url, url="http://evil.example.org:80\r\nX-Injected: 1/") - resp = _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + resp = _resolve(app, _host(job_id)) assert resp.status_code == 403 assert "x-fg-upstream" not in resp.headers @@ -325,7 +334,7 @@ def test_resolve_rejects_upstream_outside_suffix(app_factory): the helper.""" app, db_url = app_factory(PROXY_DOMAIN, upstream_zone=".nodes.example.org") job_id = _seed_running_service_with_url(db_url, url="http://127.0.0.1:8989/") - resp = _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + resp = _resolve(app, _host(job_id)) assert resp.status_code == 403 assert "x-fg-upstream" not in resp.headers @@ -338,7 +347,7 @@ def test_publish_then_resolve_roundtrip(app_factory, monkeypatch): job_id = _seed_service_job(db_url) published = _get_job_with_worker_url( app, job_id, "http://node01:41235/lab?token=abc", monkeypatch).json()["service_url"] - assert published == f"https://job-{job_id}.{PROXY_DOMAIN}/lab?token=abc" + assert published == f"https://{_host(job_id)}/lab?token=abc" resp = _resolve(app, urlsplit(published).netloc) assert resp.status_code == 204 assert resp.headers["x-fg-upstream"] == "node01:41235" @@ -355,7 +364,7 @@ def test_resolve_serves_repeats_from_the_cache(app_factory): in which a stopped job can still be proxied, which is why it is short.""" app, db_url = app_factory(PROXY_DOMAIN) job_id = _seed_running_service_with_url(db_url) - host = f"job-{job_id}.{PROXY_DOMAIN}" + host = _host(job_id) assert _resolve(app, host).status_code == 204 session = get_db_session(db_url) @@ -376,7 +385,7 @@ def test_resolve_does_not_cache_refusals(app_factory): moment it does, so a miss cannot be remembered.""" app, db_url = app_factory(PROXY_DOMAIN) job_id = _seed_service_job(db_url) - host = f"job-{job_id}.{PROXY_DOMAIN}" + host = _host(job_id) assert _resolve(app, host).status_code == 403 session = get_db_session(db_url) @@ -395,7 +404,7 @@ def test_resolve_counts_refusals_by_reason(app_factory): job_id = _seed_service_job(db_url) _resolve(app, "not-a-proxy-host.example.org") - _resolve(app, f"job-{job_id}.{PROXY_DOMAIN}") + _resolve(app, _host(job_id)) assert apps.resolve_counts() == { "refused_bad_host": 1, "refused_no_upstream": 1} diff --git a/tests/test_service_proxy_urls.py b/tests/test_service_proxy_urls.py index 5e796b72c..0902b6a26 100644 --- a/tests/test_service_proxy_urls.py +++ b/tests/test_service_proxy_urls.py @@ -1,14 +1,23 @@ """Tests for the pure URL helpers behind the app service HTTPS proxy.""" +import re + import pytest from fileglancer.apps.serviceproxy import ( + _MAC_CHARS, build_proxied_service_url, job_id_from_host, + service_host_label, upstream_from_service_url, ) DOMAIN = "services.example.org" +SECRET = "test-session-secret" + + +def _host(job_id, domain=DOMAIN): + return f"{service_host_label(job_id, SECRET)}.{domain}" # --- build_proxied_service_url --- @@ -16,64 +25,109 @@ def test_build_swaps_scheme_and_host_keeping_path_and_query(): """The query string carries $FG_SERVICE_TOKEN, so it must survive intact.""" assert build_proxied_service_url( - "http://node01:41235/lab?token=abc", 123, DOMAIN - ) == "https://job-123.services.example.org/lab?token=abc" + "http://node01:41235/lab?token=abc", 123, DOMAIN, SECRET + ) == f"https://{_host(123)}/lab?token=abc" def test_build_preserves_fragment(): assert build_proxied_service_url( - "http://node01:41235/vnc.html?autoconnect=true#top", 7, DOMAIN - ) == "https://job-7.services.example.org/vnc.html?autoconnect=true#top" + "http://node01:41235/vnc.html?autoconnect=true#top", 7, DOMAIN, SECRET + ) == f"https://{_host(7)}/vnc.html?autoconnect=true#top" def test_build_preserves_bare_root_url(): assert build_proxied_service_url( - "http://node01:41235", 7, DOMAIN - ) == "https://job-7.services.example.org" + "http://node01:41235", 7, DOMAIN, SECRET + ) == f"https://{_host(7)}" def test_build_returns_none_without_a_proxy_domain(): """Empty domain is the off switch: callers fall back to the raw URL.""" - assert build_proxied_service_url("http://node01:41235/", 1, "") is None + assert build_proxied_service_url("http://node01:41235/", 1, "", SECRET) is None + + +def test_build_returns_none_without_a_secret(): + """Nothing to sign the hostname with, so publish the raw URL rather than an + unsigned, guessable one.""" + assert build_proxied_service_url("http://node01:41235/", 1, DOMAIN, "") is None def test_build_returns_none_for_empty_service_url(): - assert build_proxied_service_url("", 1, DOMAIN) is None - assert build_proxied_service_url(None, 1, DOMAIN) is None + assert build_proxied_service_url("", 1, DOMAIN, SECRET) is None + assert build_proxied_service_url(None, 1, DOMAIN, SECRET) is None + + +# --- service_host_label --- + +def test_host_label_is_short_and_dns_safe(): + """The label goes in a hostname and in front of users, so keep it small: + job- + id + - + 8 base32 chars.""" + label = service_host_label(12, SECRET) + assert re.fullmatch(r"job-12-[a-z2-7]{8}", label), label + assert len(label) == 15 + + +def test_host_label_differs_per_job_and_per_secret(): + assert service_host_label(1, SECRET) != service_host_label(2, SECRET) + assert service_host_label(1, SECRET) != service_host_label(1, "other-secret") # --- job_id_from_host --- def test_job_id_from_host_extracts_id(): - assert job_id_from_host("job-123.services.example.org", DOMAIN) == 123 + assert job_id_from_host(_host(123), DOMAIN, SECRET) == 123 def test_job_id_from_host_strips_port_and_case(): - assert job_id_from_host("JOB-123.Services.Example.Org:443", DOMAIN) == 123 + assert job_id_from_host(f"{_host(123).upper()}:443", DOMAIN, SECRET) == 123 + + +def test_job_id_from_host_rejects_unsigned_and_mistyped_labels(): + """The point of the MAC: job ids are a small global sequence, so without it + anyone who can reach the proxy could sweep job-1..job-500 for live + services.""" + assert job_id_from_host(f"job-123.{DOMAIN}", DOMAIN, SECRET) is None + assert job_id_from_host(f"job-123-{'a' * _MAC_CHARS}.{DOMAIN}", + DOMAIN, SECRET) is None + # A valid label for another job does not authorize this one. + label = service_host_label(124, SECRET).split("-", 2)[2] + assert job_id_from_host(f"job-123-{label}.{DOMAIN}", DOMAIN, SECRET) is None + + +def test_job_id_from_host_rejects_a_label_signed_with_another_secret(): + assert job_id_from_host( + f"{service_host_label(123, 'other-secret')}.{DOMAIN}", + DOMAIN, SECRET) is None @pytest.mark.parametrize("host", [ - "services.example.org", # no job label - "job-.services.example.org", # no digits - "job-abc.services.example.org", # not numeric - "job-123.evil.example.org", # wrong zone - "job-123.services.example.org.evil", # suffix attack - "x.job-123.services.example.org", # extra label + "services.example.org", # no job label + "job-.services.example.org", # no digits + "job-abc.services.example.org", # not numeric + "job-123.evil.example.org", # wrong zone + "job-123.services.example.org.evil", # suffix attack + "x.job-123.services.example.org", # extra label "", + None, ]) def test_job_id_from_host_rejects_bad_hosts(host): - assert job_id_from_host(host, DOMAIN) is None + assert job_id_from_host(host, DOMAIN, SECRET) is None def test_job_id_from_host_rejects_everything_without_a_domain(): """Guards against an empty domain turning the pattern into a wildcard.""" - assert job_id_from_host("job-123.services.example.org", "") is None + assert job_id_from_host(_host(123), "", SECRET) is None + + +def test_job_id_from_host_rejects_everything_without_a_secret(): + assert job_id_from_host(_host(123), DOMAIN, "") is None def test_job_id_from_host_rejects_absurd_job_id(): """An unbounded digit run used to reach int()'s 4300-digit limit and raise, turning a bogus hostname into a 500 with a traceback instead of a refusal.""" - assert job_id_from_host(f"job-{'9' * 5000}.{DOMAIN}", DOMAIN) is None + assert job_id_from_host( + f"job-{'9' * 5000}-{'a' * _MAC_CHARS}.{DOMAIN}", DOMAIN, SECRET) is None # --- upstream_from_service_url --- From 59072a9d7b6b5ea7eba38a67211b5709f6e99972 Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 3 Sep 2026 14:21:27 -0400 Subject: [PATCH 5/6] style: separate the closing hint on the service-unavailable page The last line points the user somewhere else rather than continuing the list above it, so it reads better with space before it than tight against the bullets. Co-Authored-By: Claude Opus 5 --- fileglancer/server.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fileglancer/server.py b/fileglancer/server.py index 52defe43a..1e6404177 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -390,7 +390,7 @@ def _validate_short_name(short_name: str) -> None: h1 { font-size: 1.5rem; margin: 0 0 1rem; } p { margin: 0 0 1rem; } ul { margin: 0; padding-left: 1.25rem; } - .muted { opacity: 0.7; font-size: 0.9rem; } + .muted { margin-top: 1.5rem; opacity: 0.7; font-size: 0.9rem; } From 551091df2ff72c213f8a34b38cad48a081ca4b2d Mon Sep 17 00:00:00 2001 From: Konrad Rokicki Date: Thu, 3 Sep 2026 14:23:39 -0400 Subject: [PATCH 6/6] style: title the service-unavailable page with its status code "503 Service Unavailable" over "This service isn't running": the status line is what a user expects at the top of an error page, and the paragraph below it already says which service and why. The test and the docs quoted the old heading, so both move with it. Co-Authored-By: Claude Opus 5 --- docs/ServiceProxy.md | 2 +- fileglancer/server.py | 2 +- tests/test_service_proxy.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/ServiceProxy.md b/docs/ServiceProxy.md index 586efa45b..e1da27221 100644 --- a/docs/ServiceProxy.md +++ b/docs/ServiceProxy.md @@ -113,7 +113,7 @@ Seven details are load-bearing: - **`resolver`** is mandatory. Without it nginx refuses to start when `proxy_pass` targets a variable. - **`proxy_buffering off`** and the long `proxy_read_timeout` suit long-lived WebSocket and streaming sessions, such as the remote desktop app. - **`proxy_pass $upscheme://$upstream`**, rather than a hardcoded `http://`, is what lets an app that serves HTTPS be reached at all. Sending a plaintext request to a TLS listener does not silently lose encryption — the app rejects it — so hardcoding the scheme breaks those apps outright. -- **`proxy_intercept_errors` must stay off** (its default) for the `error_page 403` above to mean what it says. The 403 it catches is the one nginx generates when `auth_request` is denied; turning interception on would also catch a 403 from the app itself — a JupyterLab token rejection, say — and replace it with "this service isn't running". +- **`proxy_intercept_errors` must stay off** (its default) for the `error_page 403` above to mean what it says. The 403 it catches is the one nginx generates when `auth_request` is denied; turning interception on would also catch a 403 from the app itself — a JupyterLab token rejection, say — and replace it with the "503 Service Unavailable" page. - **`/_fg_unavailable` is a prefix location, not a named one**, because nginx refuses a `proxy_pass` with a URI part inside a named location (`proxy_pass cannot have URI part in location given by regular expression, or inside named location`). `internal;` is what keeps it out of the URL space the app sees, so a request for that path gets a 404 rather than the error page. The existing HTTP-to-HTTPS redirect block is typically `default_server` with `server_name _`, in which case it already covers the new subdomains. diff --git a/fileglancer/server.py b/fileglancer/server.py index 1e6404177..5644f3f18 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -395,7 +395,7 @@ def _validate_short_name(short_name: str) -> None:
-

This service isn't running

+

503 Service Unavailable

The app that was served at this address is no longer available. Services are reachable only while their job is running.

    diff --git a/tests/test_service_proxy.py b/tests/test_service_proxy.py index 43f78bdf1..93a6e4dbe 100644 --- a/tests/test_service_proxy.py +++ b/tests/test_service_proxy.py @@ -292,7 +292,7 @@ def test_service_unavailable_page_is_servable_without_auth(app_factory): resp = TestClient(app).get("/api/apps/service-unavailable") assert resp.status_code == 503 assert resp.headers["content-type"].startswith("text/html") - assert "isn't running" in resp.text + assert "503 Service Unavailable" in resp.text # Self-contained: the proxy would refuse a subresource fetched from here. assert "src=" not in resp.text and "