Skip to content

Ensure TigerVNC isn't accessible via the network #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 11, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@ jobs:

- name: Test vncserver
run: |
container_id=$(docker run -d -it -p 5901:5901 test vncserver -xstartup /opt/install/jupyter_remote_desktop_proxy/share/xstartup -verbose -fg -geometry 1680x1050 -SecurityTypes None -rfbport 5901)
# TigerVNC needs to be configured with -rfbport -1 to not open a TCP
# port, while TurboVNC doesn't support being passed -1 and won't open
# a TCP port anyhow.
rfbport_arg="-rfbport -1"
if [ "${{ matrix.vncserver }}" == "turbovnc" ]; then rfbport_arg=""; fi

container_id=$(docker run -d -it test vncserver -xstartup /opt/install/jupyter_remote_desktop_proxy/share/xstartup -verbose -fg -geometry 1680x1050 -SecurityTypes None -rfbunixpath /tmp/vncserver.socket $rfbport_arg)
sleep 1

echo "::group::Install netcat, a test dependency"
Expand All @@ -70,9 +76,18 @@ jobs:
'
echo "::endgroup::"

docker exec -it $container_id timeout --preserve-status 1 nc -v localhost 5901 2>&1 | tee -a /dev/stderr | \
docker exec -it $container_id timeout --preserve-status 1 nc -vU /tmp/vncserver.socket 2>&1 | tee -a /dev/stderr | \
grep --quiet RFB && echo "Passed test" || { echo "Failed test" && TEST_OK=false; }

echo "::group::Security - Verify TCP ports wasn't opened"
ports=(5800 5801 5900 5901)
for port in "${ports[@]}"
do
docker exec -it $container_id timeout --preserve-status 1 nc -vz localhost $port | tee -a /dev/stderr | \
grep --quiet succeeded && { echo "Failed security check - port $port open" && SECURITY_OK=false; } || echo "Passed security check - port $port not opened"
done
echo "::endgroup::"
Comment on lines +82 to +89
Copy link
Member Author

@consideRatio consideRatio Apr 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't testing exactly what jupyter-remote-desktop-proxy starts, it instead tests how we start the vncserver here - ideally we would test it against what is dynamically started.

Currently our test suites relies on a docker container with a jupyter server being started.

An ideal test would ensure no localhost:port is listened to, and not just 0.0.0.0:port is listened to. We could perhaps do that from outside the container if we started it with --network host or execed into it before trying to open a connection.


echo "::group::vncserver logs"
docker exec $container_id bash -c 'cat ~/.vnc/*.log'
echo "::endgroup::"
Expand All @@ -82,6 +97,10 @@ jobs:
echo "Test failed!"
exit 1
fi
if [ "$SECURITY_OK" == "false" ]; then
echo "Security check failed!"
exit 1
fi

- name: Install playwright
run: |
Expand Down
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Extra ignore patterns specific to this project
# Installed JS libraries
node_modules/
package-lock.json
# Built JS files
jupyter_remote_desktop_proxy/static/dist

Expand Down
21 changes: 20 additions & 1 deletion jupyter_remote_desktop_proxy/setup_websockify.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,27 @@ def setup_websockify():
"vncserver executable not found, please install a VNC server"
)

# TurboVNC and TigerVNC share the same origin and both use a Perl script
# as the executable vncserver. We can determine if vncserver is TigerVNC
# by searching tigervnc string in the Perl script.
#
# The content of the vncserver executable can differ depending on how
# TigerVNC and TurboVNC has been distributed. Below are files known to be
# read in some situations:
#
# - https://github.com/TigerVNC/tigervnc/blob/v1.13.1/unix/vncserver/vncserver.in
# - https://github.com/TurboVNC/turbovnc/blob/3.1.1/unix/vncserver.in
#
with open(vncserver) as vncserver_file:
vncserver_file_text = vncserver_file.read().casefold()
is_turbovnc = "turbovnc" in vncserver_file_text
Comment on lines +15 to +28
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This detection logic is what we had before, so I re-introduced it now that we needed to distinguish the VNC servers again.


# {unix_socket} is expanded by jupyter-server-proxy
vnc_args = [vncserver, '-rfbunixpath', '{unix_socket}']
vnc_args = [vncserver, '-rfbunixpath', "{unix_socket}", "-rfbport", "-1"]
if is_turbovnc:
# turbovnc doesn't handle being passed -rfbport -1, but turbovnc also
# defaults to not opening a TCP port which is what we want to ensure
vnc_args = [vncserver, '-rfbunixpath', "{unix_socket}"]
Comment on lines +31 to +35
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unknown vnc servers will receive -rfbport -1 now, and could perhaps due to that fail to start like TurboVNC would, but I figure its better we are safe than sorry about this if we have some weak support for unknown VNC servers.


xstartup = os.getenv("JUPYTER_REMOTE_DESKTOP_PROXY_XSTARTUP")
if not xstartup and not os.path.exists(os.path.expanduser('~/.vnc/xstartup')):
Expand Down