feat: vnc feature serving a contained GUI - #24
Conversation
Opt-in `vnc` mixin: a virtual X display (Xvnc) running a fullscreened Chromium, served over VNC (RFB). The raw RFB port is published on the host loopback so any VNC client can attach directly. Find the port with `enclave ps --json` and read the per-session password from /tmp/enclave-vnc/vnc-password. A supervisor keeps Xvnc, matchbox-window-manager, and a headful Chromium at $VNC_URL alive with restart loops. Xvnc listens on all container interfaces so the published port reaches it, and enforces a random password at the RFB layer (VncAuth). That password is what shapes the boundary: holding it is what grants control of the display, and because it is generated per session it reaches exactly one session's display, which is why the agent knowing it is harmless. Exporting DISPLAY and BROWSER=vnc-open routes "open in browser" flows onto the contained display, where all real browsing stays on the session's gateway-restricted network.
|
Love it! Didn't get to try it out yet. Just one thought about the naming... I tend to prefer something like "enclave-vnc" instead of GUI to avoid any collisions with the upcoming UI of enclave (homeshell). Edit: I'm mostly talking about #29 but already wanted to mention it here as this is going to be discussed first and may be impacted by any naming discussions. :-) |
|
I see, yes, "gui" could be misleading. What about "vnc-viewer"? |
|
Yes, vnc-viewer sounds great to me. Clear and descriptive and no collision with any other UI component. |
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. Noting why helps, since replies are read and taken into account in follow-up reviews.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Submitted via review-guard-mcp
Opt-in vnc mixin: Xvnc + matchbox + fullscreened Chromium kept alive by an in-container supervisor, RFB published on an auto-assigned host port and gated by a per-session VncAuth password, plus DISPLAY/BROWSER and an image-wide http(s) scheme handler so "open in browser" lands on the contained display.
No Go changes, and it is the first extension to use both feature-level commands.startup and feature-level ports, so it exercises existing machinery rather than adding any. Docs and the golden surface snapshot are updated in the right places; go test ./..., check-license-headers, validate-extensions and shellcheck are all clean, and I verified the hand-rolled VncAuth derivation is byte-identical to a reference vncpasswd implementation.
Worth a human look at three things: the Chromium restart loop in vnc-supervisor, which can spin every 2s (and flood the display with waiting-page windows) whenever another process owns the profile singleton; the chromium apt package against the documented ubuntu:24.04 base, where one unavailable package fails the shared feature apt step for every feature; and the reachability of the RFB port, which is broader than "host loopback" because Xvnc binds all interfaces in a netns that is shared with the gateway on a Docker bridge. The rest are smaller notes (unusable VNC_RFB_PORT knob, mimeapps.list clobber, shellcheck coverage gap, repeated rationale comments).
- share one Chromium invocation (new vnc-chromium wrapper) between the supervisor and vnc-open, and quiet Chromium's background networking - park the supervisor's browser restart loop while another live process holds the profile singleton instead of respawning every 2s - install chromium via install.sh instead of the shared aptPackages stage so an Ubuntu base fails only this feature, with a clear error - log vnc-open output to /tmp/enclave-vnc/log/ instead of /dev/null - merge into /etc/xdg/mimeapps.list instead of truncating it - drop the unusable VNC_RFB_PORT knob (spec.yaml pins the port) - cover the extension-less vnc scripts with shellcheck in make lint, lint-report, and lint-changed - document bridge-level RFB reachability under residual risks and keep the xdg-mime rationale in one place (vnc-open's header)
Namespace the feature's environment variables with ENCLAVE_, move the installed scripts under bin/ so the shellcheck globs generalize to any extension, bound the startup URL wait, and mirror supervisor diagnostics into a log file since the entrypoint discards stdout and stderr.
EclipseSourceAI
left a comment
There was a problem hiding this comment.
Note
Autonomous AI review.
This review was done by an AI agent and therefore may contain mistakes. Feel free to ignore any comment you disagree with. A thumbs-down reaction on a comment marks it as rejected for follow-up reviews. Noting why in a reply helps, since replies are read too.
Resolving all AI comments does not lead to an automatic approval. A maintainer still needs to review and sign off on the overall architecture and design.
To get an updated review after pushing changes, a maintainer may re-request a review from this account.
Running in Eclipse Enclave, submitted via review-guard-mcp
Follow-up review after the two "address review comments" commits (b93af3d, 57727b8).
All ten points from my earlier review are addressed:
- The Chromium restart loop now checks for a live singleton before respawning, so it no longer fights
vnc-open's fallback launch. vnc-opennow logs tovnc-open.loginstead of discarding output.- Chromium install moved out of the shared
aptPackagesstep intoinstall.shwith its own actionable error, so an Ubuntu base fails only this feature with a clear message instead of taking down the whole apt stage. - The RFB-reachability risk (Xvnc on all interfaces, shared gateway bridge) is now documented in the feature README's Residual risks and cross-linked from a new section in
docs/security/README.md. - The unusable
VNC_RFB_PORTknob is gone; the port is hardcoded with a comment explaining why. install.shnow merges intomimeapps.listinstead of truncating it (verified idempotent).vnc-supervisor/vnc-openmoved underbin/, and bothMakefileandlint-changed.shnow shellcheckextensions/*/bin/*.- The xdg-mime-before-
$BROWSERrationale is centralized invnc-open's header, with everything else pointing back at it. vnc-supervisorandvnc-opennow share one Chromium invocation via the newvnc-chromiumwrapper, removing the flag drift.vnc-chromiumadds--disable-background-networking --disable-component-update --disable-sync.
Also new: a vnc_feature_test.go covering the startup command shape and the install/spec argv, and a config golden-snapshot update for the apt package list change. I read through both commits in full, ran the new tests, go build, and shellcheck on the new bin/ scripts; all clean. I didn't find anything new worth flagging inline.
These previous comments can be resolved as they are now handled:
- Chromium restart loop could spin every 2s against vnc-open's fallback launch
- vnc-open discarded stdout/stderr, making a failed open undiagnosable
- chromium apt package fails the whole feature apt step on Ubuntu bases
- README didn't document the RFB port's reachability beyond loopback under network isolation
- VNC_RFB_PORT env knob was unusable since the published port is hardcoded
- install.sh truncated the shared mimeapps.list instead of merging
- vnc-supervisor/vnc-open lacked .sh extensions, skipping shellcheck
- xdg-mime-before-$BROWSER rationale was duplicated four times
- vnc-open started Chromium without the supervisor's flags, causing drift
- Chromium's background networking would fill the network log with gateway-denied requests
I can't resolve them myself as I would need write permission on this repository.
xai
left a comment
There was a problem hiding this comment.
I can see the AI review requested you to change the variables from VNC_... to ENCLAVE_VNC_... but I don't think this works.
When I use something like:
environment:
variables:
ENCLAVE_VNC_GEOMETRY: "800x600"
then there will be a not-too-obvious warning: "warn: Ignoring environment.variables key "ENCLAVE_VNC_GEOMETRY": reserved for enclave internals"
and then the variables are skipped in internal/runtime/runtime.go:956.
Maybe it would be best to rename them back? There shouldn't be a clash with anything else inside of the image hopefully.
What it does
Opt-in
vncmixin: a virtual X display (Xvnc) running a fullscreened Chromium, served over VNC (RFB). The raw RFB port is published on the host loopback so any VNC client can attach directly. Find the port withenclave ps --jsonand read the per-session password from /tmp/enclave-vnc/vnc-password.A supervisor keeps Xvnc, matchbox-window-manager, and a headful Chromium at $VNC_URL alive with restart loops. Xvnc listens on all container interfaces so the published port reaches it, and enforces a random password at the RFB layer (VncAuth). That password is what shapes the boundary: holding it is what grants control of the display, and because it is generated per session it reaches exactly one session's display, which is why the agent knowing it is harmless.
Exporting DISPLAY and BROWSER=vnc-open routes "open in browser" flows onto the contained display, where all real browsing stays on the session's gateway-restricted network.
How to test
./bin/enclave --features +vnc --rebuild./bin/enclave ps --jsondocker exec <container-id> cat /tmp/enclave-vnc/vnc-passwordxtigervncviewer 127.0.0.1:<port>You can ask the agent to open tabs or show other GUI tools in a way so that it's visible to you
If you want a more convenient way to test you can
I created the latter after the first one and it's conceptually the better approach for Enclave itself I think. The HomeShell could then later have a built-in feature similar to the first web-based approach.
Follow-ups
Breaking changes
Review checklist