Security and reliability fixes for jetstream - #5836
Conversation
RefreshOidcToken logged the token metadata JSON, the parsed metadata struct, and the OAuth client secret at Info level on every OIDC token refresh; getSSHCode dumped the full authorize response, whose Location header carries the one-time SSH auth code. Keep the non-sensitive fields (client ID, response status), state what is withheld, and leave the secret-bearing lines commented out.
getEncryptionKey used the configured key verbatim with no check, so an unmodified deploy of the packaged config.properties encrypts the token store with the well-known key from config.example. Detect that value at startup and log a warning, mirroring the existing default-session-secret check. Deploys are unaffected (warn and continue).
API key secrets were stored verbatim in api_keys.secret and matched by plaintext SQL equality, so a database dump yielded usable long-lived credentials for every user. Store and look up a SHA-256 hash instead (the 384-bit random secret needs no salt/bcrypt, and the 64-char hash fits the existing VARCHAR(64) column). The plaintext is still returned once at creation. A one-time migration hashes existing rows in place, so keys already issued keep working.
DoLoginToCNSIwithConsoleUAAtoken called log.Fatal (os.Exit(1)) when the CF info fetch failed during SSO auto-connect, so a transient CF outage at the moment one user logged in exited jetstream for every user. Log the error and return it, as every other branch in the function already does.
ProxyRequest and DoProxyRequest launched one doRequest goroutine per endpoint sending on an unbuffered channel. On the timeout paths (and on an early build-error return) the function returns without draining, so each goroutine blocked forever on the send, holding its full response body. Long-running ops past 30s leaked deterministically. Buffer the channel to the goroutine count so every send completes and the goroutine exits.
The session cookie set Secure and HttpOnly but never SameSite, so it was emitted with the browser default and cross-site protection relied on each browser's behaviour. Set SameSite=Lax on all three session stores (postgres/mysql/sqlite) as defence-in-depth alongside the XSRF token; Lax keeps the cookie on top-level navigations such as the SSO login redirect.
The WebSocket upgrader accepted any Origin (CheckOrigin always returned true), leaving app SSH and log-stream endpoints open to Cross-Site WebSocket Hijacking. Enforce an allow-list: same-origin requests and origins listed in ALLOWED_ORIGINS are accepted (empty Origin, i.e. non-browser clients, is allowed); everything else is rejected. The allow-list is populated at startup from the same config CORS uses.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Switch API key secret hashing from SHA-256 to HMAC-SHA256 keyed with the server encryption key. A database dump alone can no longer verify guessed secrets, and it clears the CodeQL weak-hashing alert that flags a bare fast hash on secret-like data (an API key is a 384-bit random token, so the fast hash was already appropriate; the pepper makes that explicit and adds defence in depth). The hash stays deterministic, preserving the indexed exact-match lookup. Note: the hash is now tied to the encryption key, so changing that key invalidates stored API keys.
|
Addressed the CodeQL The alert fired because a fast hash (SHA-256) was applied to secret-like data — the rule assumes a low-entropy password, where a slow hash (bcrypt/argon2) is needed. An API key here is a 384-bit random token, so a fast hash was already appropriate and a slow one would only cost the O(1) indexed Rather than dismiss it, I switched to HMAC-SHA256 keyed with the server encryption key (a pepper). This clears the alert and is a genuine improvement: a database dump alone can no longer verify guessed secrets. It stays deterministic, so the indexed lookup is preserved, and the one-time migration hashes existing rows in place so already-issued keys keep working. One operator-facing consequence, noted in a Verified locally with the same default query suite CI uses: the alert is gone from |
3 participants
Several security and reliability issues in the jetstream backend, found while reviewing the current develop. Each is a separate commit.
Security
RefreshOidcTokenlogged the token metadata (includingclientSecret) andgetSSHCodedumped the full authorize response — whoseLocationheader carries the one-time SSH code — at Info level. Keeps the non-sensitive fields; the secret-bearing lines are left commented out.ENCRYPTION_KEYis the well-known default fromconfig.example.getEncryptionKeyused the configured key verbatim, so an unmodified packaged deploy encrypts the token store with a publicly known key. Detected at startup and warned (deploys are unaffected); the warning also notes that changing the key later makes existing encrypted data unreadable.VARCHAR(64)column; a 384-bit random secret needs no salt). A one-time migration hashes existing rows so already-issued keys keep working.SameSite=Laxon the session cookie (previously unset, relying on the browser default).InsecureSkipVerify), leaving app SSH and log-stream endpoints open to cross-site WebSocket hijacking. Now enforces same-origin plus the hosts configured inALLOWED_ORIGINS, via the library'sOriginPatterns.Reliability
log.Fatalon an SSO-login info-fetch failure. A transient CF outage during one user's login exited the whole process for every user; now logs and returns.Backend test suite passes; fixes with logic carry a test (API-key hash lookup, WebSocket origin allow-list).