Add per-user custom env vars, injectable into docker create / IDE / SSH - #45
Draft
struanb wants to merge 16 commits into
Draft
Add per-user custom env vars, injectable into docker create / IDE / SSH#45struanb wants to merge 16 commits into
struanb wants to merge 16 commits into
Conversation
Lets a user define arbitrary KEY=VALUE env vars on their account (self- and admin-editable, mirroring the existing gh_token/ssh patterns), each taggable with any subset of three injection targets: - docker: baked into `docker create`, visible to the container's own process env (Reservation::Launch::cmdline_user_env) - ide: reaches the IDE server process and anything it forks (integrated terminals) via a new launch.sh apply_user_env wrapper that survives the env -i wipe in launch_theia/launch_openvscode, plus a belt-and-braces terminal.integrated.env.linux write in openvscode's launch-ide.sh - ssh: reaches SSH sessions via a marker-guarded .bashrc/.profile snippet, mirroring the existing install_launch_status_notice mechanism Secret-flagged values are masked in API/CLI output (first/last chars, same convention as gh_token) with restore-on-unchanged-POST. Server-side validation rejects reserved/malformed names and unsafe values. Includes a companion fix for a pre-existing log-redaction gap on the docker create path, a CLI display fix so admin `user get/create/edit` actually renders user fields in text mode, a new EnvVarsEditor.vue admin/account UI component, and an integration test module (14_user_env_vars.py) covering the storage/API round-trips and live injection/target-isolation across all three targets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
Two real bugs surfaced by actually launching a devtainer and inspecting the
IDE process env, an SSH session, and the account page after save (not just
static checks):
- apply_user_env() re-derived $HOME from $IDE_USER, but IDE_USER is not part
of either env -i call site's fixed allowlist in launch_theia/
launch_openvscode (only HOME itself is, already correctly pre-computed by
the outer script) — so IDE_USER was empty at that point, HOME resolved to
the wrong path, and the ide-target env file was silently never found or
read. Now uses the inherited $HOME directly.
- install_user_env_notice() only appended its snippet to .bashrc/.profile if
they already existed, silently doing nothing on images with neither (e.g.
plain Alpine, confirmed via a live devtainer with no /etc/skel) — unlike
the diagnostic-only install_launch_status_notice it was modeled on, this is
the *only* delivery path for the ssh target, so a silent no-op defeated the
feature outright rather than missing a nice-to-have. .profile (the one
POSIX login shells universally read) is now created if absent; .bashrc
stays append-only-if-exists.
- UserDetail.vue's save() relied on the currentUserRecord watcher to
re-populate the form after a save, but the underlying store commit (inside
the dispatch) can trigger that watcher before isEditMode flips back to
false, so its guard skips the repopulation and it never fires again —
leaving stale, unmasked just-typed values on screen for freshly-saved
secret env vars (confirmed via direct Vuex store inspection: the store had
the correctly-masked value while the DOM still showed plaintext). Both the
self-edit and admin-edit branches now call populateForm() explicitly after
save.
Also fixes the integration test module itself: `targets` is an object
({"docker":true}), not an array (["docker"]) — the schema and every other
part of the implementation (Perl, Vue) already agreed on this shape; only the
test's own --set invocations and CLI doc example had it wrong. Reworked the
two root-requiring injection checks (docker create env, IDE process env) to
not depend on /proc/<pid>/environ, which needs CAP_SYS_PTRACE unavailable in
some restricted Docker setups — the IDE check now reads launch-ide.sh's own
env dump instead, which is both more portable and IDE-variant-agnostic.
Verified via `t/integration/tests/14_user_env_vars.py` (14/14) plus
regression runs of modules 11 and 12, all against a live :feature deployment
with these files docker cp'd into the running container, and via a Playwright
pass through the actual account-page UI (add/save/reveal a secret env var).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
A user-defined custom var named literally IDE_USER wasn't blocked, unlike its close relatives IDE/IDE_PATH/IIDE_PATH which are — an inconsistency with the blocklist's own stated intent (prevent a user-defined var from shadowing internal plumbing names). Not a privilege-escalation path: nothing reads $IDE_USER downstream of the point where apply_user_env runs (it's already consumed for all its security-relevant decisions earlier in launch.sh, and Theia/openvscode's own launch-ide.sh scripts never reference it), so this closes a naming-hygiene gap rather than an actual leak. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
struanb
force-pushed
the
claude/user-env-vars-6cs63v
branch
from
July 24, 2026 22:41
5508117 to
829ee99
Compare
Contributor
Author
Code review findings (per-user env vars)Automated review of this branch's diff ( Each item below is intended to be actionable independently — file/line, the concrete failure
Additional test gaps (not tied to a single finding above)
Findings from a Codex review session against this branch's diff; posting here so a Claude Code |
- launch.sh: use printf instead of echo for DOCKSIDE_USER_ENV, since some
echo implementations interpret backslash escapes and could reintroduce a
newline that jq would split into extra env lines.
- Reserve DEBUG as an env var name: launch.sh dumps the full launch
environment (including DOCKSIDE_USER_ENV/SSH_AGENT_KEYS/GH_TOKEN) when
DEBUG is set, so a user-settable docker-target DEBUG=1 could force that
unsanitized dump.
- Fix _restore_redacted_env's masked-sentinel detection: it previously
treated any secret=1 value containing '*' as an unchanged mask, gated on
the NEW secret flag. This silently deleted new secrets whose real value
contains '*', reverted changed secret values back to the old value if the
new one contains '*', and persisted the mask itself as plaintext when
toggling secret->non-secret. Now restores only on an exact match against
_mask_secret(original_value), independent of the new secret flag.
- _validate_env_vars: reject non-scalar 'value' (would otherwise reach
cmdline_user_env/Reservation::exec as a literal env value); reject
non-boolean-like 'secret'/'targets.*' values (a JSON string "false" is
Perl-truthy, silently inverting caller intent); enforce an aggregate
encoded-size limit on the combined ide+ssh blob, since per-var limits
alone allow it to exceed a single docker exec argv element's practical
size.
- EnvVarsEditor.vue: normalize entry() to always return a full
{value,secret,targets} shape, fixing a crash on valid entries that omit
'targets'.
- Add integration test coverage for all of the above.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
apply_user_env exports ide-target user env vars (including any flagged secret) into the IDE server process's environment before launch. Both theia/latest and openvscode's launch-ide.sh then unconditionally dumped the full environment to their log file for diagnostics — previously harmless, since env -i's fixed allowlist meant nothing user-controlled reached that point, but now a real secret-leak path. Only these two IDE version dirs are patched (the ones actually reachable via mountIDE:false / current profiles); older pinned Theia versions keep their existing unconditional dump. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
…g perms The env dump (launch.sh's init(), and theia/openvscode launch-ide.sh) wrote the full process environment - including secret-flagged per-user env vars - to log files that were world-readable (theia.log/openvscode.log chmod 666), and DEBUG could be enabled via a profile's dockerArgs, not just per-user config. Removing the dumps closes the leak regardless of how DEBUG gets set. theia.log/openvscode.log are now chmod 600, matching the per-user env files they sit alongside; both are created by a process already running as IDE_USER, so this doesn't affect who needs to write them. Co-Authored-By: Claude <noreply@anthropic.com>
… gone test_02 used to grep launch-ide.sh's "- environment variables:" log dump to prove an 'ide'-target var reached the IDE process; that dump was removed as a secret-exposure risk (f6ae71a). Read user-env-ide.env instead - the same file apply_user_env exports from before exec'ing the IDE server - which proves populate_user_env wrote the right value, though (unlike the log dump) it no longer proves apply_user_env's export into the live process actually succeeded. Co-Authored-By: Claude <noreply@anthropic.com>
The UI server runs embedded in nginx (ngx_http_perl_module), whose $r->request_body silently returns empty for a POST body nginx buffers to a temp file rather than memory - empirically ~10240 bytes on this build, not configured anywhere. A limit anywhere near the intended 65536 can never actually be enforced: the request's env field is dropped before validation even runs. 8192 stays safely under that ceiling so the check is reachable again; raise back to 65536 once the app server moves to the standalone Mojolicious process (docs/plans/mojolicious-app-server-split-plan.md), whose request body reader has no such limitation. test_21_aggregate_ide_ssh_blob_size_rejected's payload shrinks to match (4 vars x 1500 bytes instead of 20 x 4000) so it clears 8192 while staying well under the ~10KB wire ceiling - the old payload silently lost its env field before reaching the check, passing for the wrong reason. Co-Authored-By: Claude <noreply@anthropic.com>
The env-vars feature (ba6a022..30ffa2a) works end-to-end and passed an automated PR review, but working code surfaced a harder question: whether Dockside should be auto-injecting secret values into a container's filesystem/process environment at all, and whether a user's vars should apply uniformly to every devtainer they can launch. These documents record that realignment without changing any code yet. - docs/plans/profile-user-env-vars.md: current branch state, the realignment, a staged roadmap, explicit in/out-of-scope boundaries, and the still-open SSH per-connection-identity question. - docs/adr/0005: secret-flagged vars become metadata-server-pull-only, never auto-injected into docker/ide/ssh, once encryption-at-rest (claude/secrets-encryption-users-json-zghgcl) and a metadata env-fetch endpoint exist. - docs/adr/0006: a profile must explicitly admit a launching user's env vars per target (default-deny), superseding the current user-target-flags-alone model. - docs/adr/0007: accept the shared-IDE-process limitation as the current security boundary (one process, one environment, shared by every collaborator with access) and require share-time UI/CLI disclosure of the effective var set instead of claiming technical isolation that doesn't exist. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
…rwarding Researched both SSH-identity options left open in the plan doc against the actual bundled software rather than general SSH knowledge: - Reverse-proxy -> wstunnel -> dropbear identity injection: the nginx-to- wstunnel leg is real (reuses the same perl_set/proxy_set_header pattern already used for Cookie forwarding), but the wstunnel-to-dropbear leg can't carry data into an already-encrypted SSH session without Dockside building a full SSH-terminating proxy - blocked by SSH's own transport security, not a missing feature. - Client-side SetEnv/SendEnv: checked dropbear's actual source (svr-chansession.c, fetched twice for corroboration) - the "env" channel request has never been implemented server-side (still an open, unmerged community PR upstream). Confirmed against the exact version this image bundles (dropbear 2025.88 via Alpine 3.22, unpatched). Not viable today, not a config gap. Both were dead ends, but the research surfaced a mechanism dropbear does support: authorized_keys' command= forced-command binding, using $SSH_ORIGINAL_COMMAND to still run the user's real request. Proposes restructuring AUTHORIZED_KEYS construction (Reservation::exec, update_ssh_authorized_keys) to stop flattening per-account key ownership, so each key can carry a forced-command wrapper stamping session identity before handoff - with a hard requirement that the value delivered must be an unforgeable, server-validated bearer token, not a plaintext identity claim, since a bare env var is trivially spoofable by anything else running in that session. Status: Proposed, pending confirmation - not yet implemented. Updated the plan doc's SSH-identity section to point here instead of restating the two now-superseded options. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
Two follow-up questions on the SSH-identity ADR: would OpenSSH avoid the limitation dropbear has, and is patching dropbear itself realistic (the premise that Dockside's Dockerfile already patches dropbear didn't hold up - it's a plain `apk add`, with only patchelf-based binary relocation applied afterwards for portability, not a behavioral patch). - OpenSSH's sshd genuinely supports AcceptEnv/SendEnv and authorized_keys environment=, which dropbear lacks - checked and confirmed. But sshd isn't currently bundled at all (only OpenSSH's client tools are), and dropbear's small footprint was very likely the reason it was chosen for a per-devtainer daemon in the first place. Rejected as disproportionate: a full SSH daemon swap to reach one authorized_keys parsing feature. - Patching dropbear directly is smaller than it sounds: checked svr-authpubkeyoptions.c directly, and a new single-purpose, server-authored option (not a general environment= reimplementation) follows the exact pattern command= already uses - copy a quoted value onto the per-key options struct, apply it when the session starts. This is materially smaller and safer than upstream's still-unmerged, maintainer- contested SendEnv/AcceptEnv PR (mkj/dropbear#205), since there's no client-supplied value to distrust at all. The build stage that installs dropbear already has make/gcc/g++ for other reasons, so building from source instead of apk-installing it is mechanically compatible with the existing Dockerfile. - Found and documented a real, checked-not-assumed asymmetry between the patch route and the no-patch command=-wrapper fallback from the previous revision: command='s apply-point never fires for a connection that opens only a port-forwarding channel (no shell/exec), since that logic lives entirely inside the session-channel dispatch. A patch applying the value at auth-success time doesn't share that gap. Revised the decision to keep both routes live (patch vs. no-patch fallback) rather than picking one, since the brief was "one winning option if the other is impossible, or two viable options even if one is costly" - both are viable, with a real, now-documented cost/capability trade-off between them. Updated the plan doc's SSH-identity section to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
…written Several turns of follow-up scrutiny had established things the documents never actually caught up to. This folds them in: - ADR-0008: drop the forced-command-wrapper fallback entirely. It was kept as a lower-cost alternative to patching dropbear, but examined against the actual threat model it's forgeable by any co-located session - the wrapper runs as the same shared account every session lands as and has no way to prove it was genuinely invoked by a real, distinct key auth event rather than manually re-run with a forged argument. Patching dropbear is now the only recommended mechanism, not one of two. - ADR-0008: require every session to be wrapped in an independent Landlock sandbox (landrun), fail-closed if unavailable, as a mandatory part of the mechanism. The minted identity token has nowhere to live but the session's own env var, and closing the /proc/<pid>/environ cross-session read - which a co-located session could otherwise use to read another's token - needs this regardless of the host's ptrace_scope setting, which Dockside doesn't control. - ADR-0008: the minted token now carries reservation_id alongside account. - ADR-0005: replace the per-reservation-volume Unix socket sketch (found impossible - Docker can't attach a new mount to an already-running container, and the Dockside server is one long-lived container serving devtainers created long after its own startup) with a single shared socket, identifying which reservation is asking via the same token ADR-0008 mints rather than a new mechanism (SO_PEERCRED was considered and rejected - it would need the Dockside server to share the host PID namespace, a real new privilege grant, confirmed docker-compose.yml doesn't currently set this). - ADR-0005: document why the socket beats HTTP(S) - CAP_NET_RAW is in Docker's default capability set (checked), so plain HTTP is realistically sniffable by a co-located attacker with no extra privilege, and HTTPS without hostname verification only stops passive capture, not an active redirect via /etc/hosts tampering. - Both ADRs: retire the "source-IP-matching" framing of App::Metadata.pm's existing FIXME - the new transport supersedes it rather than just making it more urgent. - Plan doc: updated the SSH-identity/metadata-transport summary to match, since it still described the now-superseded two-live-options framing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
Leaving "query the metadata server and export the results" entirely to each user's own script would likely reproduce the exact bug class this branch's PR review spent most of its effort closing - a naive eval-based fetch script is a shell-injection vector the moment a value contains a special character. Specifies a sourced shell function (not an eval-emitting binary) using the same safe while-read/export pattern apply_user_env already establishes, delivered via the existing marker-guarded rc-file mechanism, using curl+jq (already bundled). Deliberately not auto-invoked. Considered and rejected two ways to make it automatic: calling it from the session wrapper (defeats ADR-0005's deliberate-pull model - every login would auto-materialize the account's full var set, secret and non-secret both, with no user action, which is just the auto-push model relocated rather than removed) and having the patched dropbear fetch/export the vars itself (rejected on top for growing the attack surface of dropbear's own privileged, pre-fork code path with an embedded HTTP/JSON client, the same class of risk that was the reason to prefer a minimal custom patch over upstream's own fuller SendEnv/AcceptEnv PR in the first place). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
…el fix Adds a second, independent dropbear patch: on any connection to a per-session local socket (starting with the forwarded SSH agent socket), check SO_PEERCRED and walk the connecting process's ancestry via /proc/<pid>/stat, rejecting anything that isn't a descendant of the specific dropbear-forked process that owns that session. Unlike the metadata-socket SO_PEERCRED case in ADR-0005, there's no cross-namespace problem here - dropbear and every session share one PID namespace - so this needs no new privilege grant. Documents the PID-reuse race in the ancestry walk and its mitigation (starttime-pair comparison, or SO_PEERPIDFD if available), and requires fail-closed behaviour on any error. Generalizes beyond the agent socket to any per-session local socket dropbear manages. Keeps the two-stage Landlock ruleset sketched earlier as a documented fallback, not discarded, in case the dropbear patch proves harder to land. Records no-agent-forwarding as the zero-cost alternative, rejected as primary only because agent forwarding was judged too valuable to give up. Also fixes a real gap found while explaining the mechanism: as specified, dockside-identity='s label was writable by $IDE_USER, since update_ssh_authorized_keys chowns both the .ssh directory and authorized_keys to that account (checked against the actual launch.sh code) - meaning any connected session could relabel its own key to claim a different identity, or replace another line's key material outright, and have it persist until the next relaunch. Fixes it by chowning authorized_keys to root and setting the sticky bit on .ssh rather than locking the directory outright, since populate_known_hosts still needs $IDE_USER to be able to write known_hosts there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
The dockside-identity= authorized_keys label (however permissioned) is defeatable by a connecting user with root inside the container - Docker's default capabilities include CAP_CHOWN/CAP_DAC_OVERRIDE/CAP_FOWNER, which is all root needs to bypass any DAC-based file protection regardless of how it's configured, and sudo is standard on developer-focused devtainer images. No permission scheme inside one container can protect data from that same container's own root user - last turn's root-owned/sticky-bit fix only ever defended against a non-root same-UID attacker. Replaces it with a live, authenticated query: at auth-success, dropbear computes a fingerprint of the just-verified public key and asks the outer Dockside server (over the same shared socket ADR-0005 builds) which account owns it, authenticated with the same per-reservation HMAC key already used for token-minting so the query itself can't be used to probe other reservations' account mappings. Bounded, short timeout; fails closed on the identity token specifically, not on the SSH connection - a session just doesn't get metadata-server access if the query fails, login still succeeds via the unchanged local authorized_keys check. A pleasant side effect: since account resolution now happens entirely on the outer server's side, from its own existing per-user records, none of the previously-planned Reservation::exec/update_ssh_authorized_keys restructuring is needed - the existing flat, unlabeled AUTHORIZED_KEYS blob stays exactly as it is today. Records the file-label design as a superseded alternative rather than silently dropping it, since it was a real, considered design (smaller than the query-based one) before being found insecure against the actual threat model. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
Sequential ADR numbers are a real branch/merge hazard for a project with active parallel work: two branches adding a new ADR independently will both reach for the same "next" number, and renumbering after the fact breaks every cross-reference by number scattered across other documents. Descriptive slugs don't have this problem - two unrelated decisions naturally get two different names. Renamed all eight existing ADRs (docs/adr/NNNN-slug.md -> docs/adr/slug.md) via git mv to preserve history, dropped the "ADR-NNNN:" prefix from each title (now just "# ADR: <description>"), and replaced every "ADR-NNNN" cross-reference across the ADRs, the plan doc, and docs/developing/user-data-model.md with the backticked target filename, verified to all resolve. Also adds per-user-devtainer-accounts.md: a full write-up of separate per-collaborator Unix accounts (and optionally separate IDE processes) as a long-term alternative to the token/Landlock machinery in ssh-session-identity-for-metadata-server.md. Works through what it fixes structurally (SSH identity, /proc cross-session reads, agent-socket isolation, and - unlike the token/Landlock approach - the filesystem- persisted-secrets gap that approach can never close), what new engineering it needs (dynamic account provisioning, UID stability across relaunches, per-user IDE ports/proxying at the second adoption level, workspace relocation with setgid/umask handling, dropping role-based dynamic sharing), and a real UX trade-off (loses live shared-IDE pair programming if separate IDE processes are adopted). Recorded as Proposed - a considered long-term direction, not blocking or superseding the already-designed near-term plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258
struanb
marked this pull request as draft
August 16, 2026 21:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lets a user define arbitrary KEY=VALUE env vars on their account (self- and
admin-editable, mirroring the existing gh_token/ssh patterns), each taggable
with any subset of three injection targets:
docker create, visible to the container's own processenv (Reservation::Launch::cmdline_user_env)
terminals) via a new launch.sh apply_user_env wrapper that survives the
env -i wipe in launch_theia/launch_openvscode, plus a belt-and-braces
terminal.integrated.env.linux write in openvscode's launch-ide.sh
mirroring the existing install_launch_status_notice mechanism
Secret-flagged values are masked in API/CLI output (first/last chars, same
convention as gh_token) with restore-on-unchanged-POST. Server-side
validation rejects reserved/malformed names and unsafe values. Includes a
companion fix for a pre-existing log-redaction gap on the docker create path,
a CLI display fix so admin
user get/create/editactually renders userfields in text mode, a new EnvVarsEditor.vue admin/account UI component, and
an integration test module (14_user_env_vars.py) covering the storage/API
round-trips and live injection/target-isolation across all three targets.
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_0116MnjogszU8WjW7cgbM258