Propagate HTTP Basic Auth userinfo through the service proxy

docs/ServiceProxy.md

@@ -126,6 +126,7 @@ If an app rejects the proxied origin, fix it in that app's manifest (most server
 
 - 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.
 - 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.
+- A published `service_url` may instead carry standard HTTP Basic Auth userinfo (`http://user:pass@node:port/...`) for a service that enforces that rather than a query-string token — useful for services a CLI tool like `curl` also needs to authenticate against, not just a browser. It is forwarded to the proxied URL exactly like the query string is, and is never seen by nginx (only the bare `host:port` is used as the `proxy_pass` target). It is strictly weaker than a query-string token for anything embedded (JupyterLab, noVNC): browsers only honor `user:pass@host` on direct navigation to the link, not reliably inside an `iframe` or across a WebSocket upgrade, and some browsers show an interstitial warning or drop it across a redirect. It remains visible in the browser's address bar and history, the same class of exposure the query-string token already has.
 - 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.

fileglancer/apps/serviceproxy.py

@@ -5,6 +5,12 @@
 ``https://job-<id>-<mac>.<proxy_domain><suffix>`` 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.
+
+A published ``service_url`` may also carry HTTP Basic Auth userinfo (e.g.
+``http://user:pass@node:port/``), for a service that enforces its own such
+credential rather than a query-string token. ``build_proxied_service_url``
+forwards it to the browser-facing URL; ``upstream_from_service_url`` discards
+it, since nginx's ``proxy_pass`` target is only ever a bare ``host:port``.
 """
 
 import base64
@@ -15,16 +21,19 @@
 import time
 from collections import Counter
 from functools import lru_cache
-from typing import Iterable, Optional
+from typing import Iterable, Optional, Tuple
 from urllib.parse import urlsplit, urlunsplit
 
 from cachetools import TTLCache
 from loguru import logger
 
 # An upstream nginx can hand to proxy_pass: hostname and explicit port, nothing
 # else. Deliberately strict — the value is interpolated into a proxy_pass
-# directive, so userinfo, whitespace, CR/LF and bracketed IPv6 literals are all
-# rejected rather than escaped.
+# directive, so whitespace, CR/LF and bracketed IPv6 literals are all rejected
+# rather than escaped. Operates on the host:port remainder only -- userinfo,
+# if present, is split off by the caller (_split_userinfo) before this pattern
+# runs and is never validated here, since it never reaches this string's
+# consumer (nginx's proxy_pass).
 _UPSTREAM_RE = re.compile(r'^([A-Za-z0-9.-]+):(\d{1,5})$')
 
 # Hostnames that always name the local machine, so an upstream naming one would
@@ -62,12 +71,43 @@ def service_host_label(job_id: int, secret: str) -> str:
     return f'job-{job_id}-{_service_mac(job_id, secret)}'
 
 
+def _split_userinfo(netloc: str) -> Tuple[str, str]:
+    """Split a netloc into (userinfo, hostport); userinfo is '' when absent.
+
+    A literal '@' inside userinfo must be percent-encoded per RFC 3986, so the
+    LAST unencoded '@' is always the correct separator -- the same rule
+    urlsplit's own .username/.password/.hostname properties rely on
+    internally.
+    """
+    userinfo, _sep, hostport = netloc.rpartition('@')
+    return userinfo, hostport
+
+
+# Userinfo is opaque and forwarded verbatim to the browser-facing URL (see
+# build_proxied_service_url), so unlike the host:port remainder it gets one
+# narrow check of its own: no control characters, since this is the one place
+# the value could end up rendered in an HTML href or similar browser context.
+# In practice tab/CR/LF never reach this check at all -- urlsplit itself
+# strips those from the whole URL before parsing -- so this mainly guards
+# against other C0 controls and DEL.
+_USERINFO_CONTROL_CHARS_RE = re.compile(r'[\x00-\x1f\x7f]')
+
+
+def _is_safe_userinfo(userinfo: str) -> bool:
+    """Whether userinfo is free of control characters."""
+    return _USERINFO_CONTROL_CHARS_RE.search(userinfo) is None
+
+
 def build_proxied_service_url(service_url: Optional[str], job_id: int,
                               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.
+    Path, query and fragment are carried over verbatim: the query string may
+    hold the service's own access token. Userinfo (HTTP Basic Auth
+    credentials), if present, is also carried over verbatim -- unless it
+    contains control characters, in which case it is dropped and the rest of
+    the URL is still returned, since losing auto-auth is a much smaller
+    problem than refusing to publish the job's link at all.
 
     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
@@ -76,9 +116,12 @@ def build_proxied_service_url(service_url: Optional[str], job_id: int,
     if not service_url or not proxy_domain or not secret:
         return None
     parts = urlsplit(service_url)
+    userinfo, _hostport = _split_userinfo(parts.netloc)
+    host_label = f'{service_host_label(job_id, secret)}.{proxy_domain}'
+    netloc = f'{userinfo}@{host_label}' if userinfo and _is_safe_userinfo(userinfo) else host_label
     return urlunsplit((
         'https',
-        f'{service_host_label(job_id, secret)}.{proxy_domain}',
+        netloc,
         parts.path,
         parts.query,
         parts.fragment,
@@ -220,25 +263,30 @@ def upstream_from_service_url(service_url: Optional[str],
                               allowed_networks: Iterable[str] = ()) -> Optional[str]:
     """Extract a ``host:port`` upstream from a published service URL.
 
-    Returns None unless the authority is exactly a hostname and an in-range
-    port. The netloc regex is a header-injection gate — it constrains the
-    authority's shape only, since the result is interpolated into the reverse
-    proxy's ``proxy_pass`` target. The destination itself (where the proxy
-    actually dials) is bounded separately: hosts that reach the Fileglancer host
-    itself are rejected; ``allowed_zone``, when set, confines upstreams
-    published as hostnames to one DNS zone; and ``allowed_networks``, when
-    set, confines upstreams published as addresses to those CIDRs. The two
-    allowlists divide the space rather than overlapping, since a bare address
-    has no zone and a name has no address without a DNS lookup. All of this
-    matters because the source string is a file written by the user's own job.
+    Returns None unless the authority, once any userinfo is discarded, is
+    exactly a hostname and an in-range port. Userinfo (HTTP Basic Auth
+    credentials) is never validated here -- it's split off and discarded
+    before the shape check runs, since it never becomes part of nginx's
+    ``proxy_pass`` target either way. The netloc regex is a header-injection
+    gate — it constrains the authority's shape only, since the result is
+    interpolated into the reverse proxy's ``proxy_pass`` target. The
+    destination itself (where the proxy actually dials) is bounded
+    separately: hosts that reach the Fileglancer host itself are rejected;
+    ``allowed_zone``, when set, confines upstreams published as hostnames to
+    one DNS zone; and ``allowed_networks``, when set, confines upstreams
+    published as addresses to those CIDRs. The two allowlists divide the
+    space rather than overlapping, since a bare address has no zone and a
+    name has no address without a DNS lookup. All of this matters because the
+    source string is a file written by the user's own job.
     """
     if not service_url:
         return None
     try:
         netloc = urlsplit(service_url).netloc
     except ValueError:
         return None
-    match = _UPSTREAM_RE.fullmatch(netloc)
+    _userinfo, hostport = _split_userinfo(netloc)
+    match = _UPSTREAM_RE.fullmatch(hostport)
     if match is None:
         return None
     port = int(match.group(2))
@@ -251,7 +299,7 @@ def upstream_from_service_url(service_url: Optional[str],
         return None
     if not _host_in_networks(host, allowed_networks):
         return None
-    return netloc
+    return hostport
 
 
 # --- Resolution cache and counters ---

tests/test_service_proxy.py

@@ -365,6 +365,24 @@ def test_publish_then_resolve_roundtrip(app_factory, monkeypatch):
     assert resp.headers["x-fg-upstream"] == "node01:41235"
 
 
+def test_publish_then_resolve_roundtrip_with_userinfo(app_factory, monkeypatch):
+    """HTTP Basic Auth credentials embedded in the published service_url must
+    reach the browser-facing URL but never reach nginx's proxy_pass target:
+    the two consumers split the same raw service_url independently."""
+    app, db_url = app_factory(PROXY_DOMAIN)
+    job_id = _seed_service_job(db_url)
+    published = _get_job_with_worker_url(
+        app, job_id, "http://classroom:sometoken@node01:41235/", monkeypatch
+    ).json()["service_url"]
+    assert published == f"https://classroom:sometoken@{_host(job_id)}/"
+    # A real client never sends userinfo in the Host header (it's stripped
+    # before the request is made) -- .hostname strips both userinfo and port,
+    # matching the actual value nginx would forward.
+    resp = _resolve(app, urlsplit(published).hostname)
+    assert resp.status_code == 204
+    assert resp.headers["x-fg-upstream"] == "node01:41235"
+
+
 # --- resolution cache and counters ---
 
 def test_resolve_serves_repeats_from_the_cache(app_factory):

tests/test_service_proxy_urls.py

@@ -57,6 +57,34 @@ def test_build_returns_none_for_empty_service_url():
     assert build_proxied_service_url(None, 1, DOMAIN, SECRET) is None
 
 
+@pytest.mark.parametrize("userinfo", [
+    "user:pass",           # ordinary username:password
+    "user",                # no colon -- username only
+    ":pass",               # no username
+    "user:pa:ss",          # password containing a literal colon
+    "user%40x:p%25w",      # percent-encoded characters, preserved verbatim
+])
+def test_build_preserves_userinfo(userinfo):
+    """HTTP Basic Auth credentials embedded in the published service_url are
+    forwarded verbatim to the browser-facing URL -- carried through exactly
+    like path/query/fragment, not decoded/re-encoded."""
+    assert build_proxied_service_url(
+        f"http://{userinfo}@node01:41235/lab", 9, DOMAIN, SECRET
+    ) == f"https://{userinfo}@{_host(9)}/lab"
+
+
+def test_build_drops_userinfo_containing_control_characters():
+    """Unlike upstream_from_service_url (which discards userinfo unconditionally
+    since nginx never sees it), this function's output can end up rendered
+    for a browser, so control characters are refused here -- but only the
+    credential is dropped, not the whole URL. Uses a NUL byte rather than
+    CR/LF: urlsplit itself already strips \\t\\r\\n before this ever runs, so
+    only other C0/DEL control characters would actually reach this check."""
+    assert build_proxied_service_url(
+        "http://user:pw\x00evil@node01:41235/lab", 9, DOMAIN, SECRET
+    ) == f"https://{_host(9)}/lab"
+
+
 # --- service_host_label ---
 
 def test_host_label_is_short_and_dns_safe():
@@ -141,9 +169,22 @@ def test_upstream_accepts_fqdn():
         "node01.cluster.example.org:8080"
 
 
+def test_upstream_strips_userinfo():
+    """Userinfo (HTTP Basic Auth credentials) never becomes part of nginx's
+    proxy_pass target -- it's discarded, not merely tolerated."""
+    assert upstream_from_service_url("http://user:pw@node01:41235/") == "node01:41235"
+
+
+def test_upstream_strips_userinfo_containing_crlf():
+    """Proves userinfo is discarded wholesale rather than shape-checked: even
+    content that would be a header-injection attempt in the host:port part
+    is harmless here, since it's never referenced again once split off."""
+    assert upstream_from_service_url(
+        "http://user:pw\r\nX-Evil: 1@node01:41235/") == "node01:41235"
+
+
 @pytest.mark.parametrize("url", [
     "http://node01/lab",                     # no port: nothing to proxy to
-    "http://user:pw@node01:41235/",          # userinfo
     "http://node01:41235x/",                 # non-numeric port
     "http://node01:99999/",                  # port out of range
     "http://node01:0/",                      # port 0