Extract VNC changes from Tor Browser functional test branch #4288

rmol · 2019-03-21T23:09:37Z

Status

Ready for review

Description of Changes

These changes were cherry-picked from the tbb-0.12.0 branch. The VNC server used in the functional test container is switched to tightvncserver, and scripts are changed to ensure that it can be reached on both Mac and Linux developer machines.

Fixes #4287.

Testing

Run the functional tests with securedrop/bin/dev-shell bin/run-test -v tests/functional. In another local shell, run make -C securedrop func-vnc to verify that you can connect to the test container with VNC and watch the functional tests run.

Checklist

If you made non-trivial code changes:

I have written a test plan and validated it for this PR

codecov-io · 2019-03-22T02:07:41Z

Codecov Report

Merging #4288 into develop will decrease coverage by 0.07%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop    #4288      +/-   ##
===========================================
- Coverage     84.7%   84.63%   -0.08%     
===========================================
  Files           43       43              
  Lines         2785     2785              
  Branches       304      304              
===========================================
- Hits          2359     2357       -2     
- Misses         358      359       +1     
- Partials        68       69       +1

Impacted Files	Coverage Δ
securedrop/securedrop/crypto_util.py	`94.73% <0%> (-1.76%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6aacf9...7960dbc. Read the comment docs.

heartsucker · 2019-03-25T18:55:42Z

Works on Debian 9. Letting others confirm on other OSes.

kushaldas

I would wait before merging this PR. Nothing looks wrong, but, while testing, I got the following buffer-overflow error.

*** buffer overflow detected ***: vncpasswd terminated
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x777e5)[0x7f82480fa7e5]
/lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x5c)[0x7f824819c15c]
/lib/x86_64-linux-gnu/libc.so.6(+0x117160)[0x7f824819a160]
vncpasswd[0x40112d]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf0)[0x7f82480a3830]
vncpasswd[0x401379]
======= Memory map: ========
00400000-00404000 r-xp 00000000 fd:01 28520571                           /usr/bin/tightvncpasswd
00603000-00604000 r--p 00003000 fd:01 28520571                           /usr/bin/tightvncpasswd
00604000-00605000 rw-p 00004000 fd:01 28520571                           /usr/bin/tightvncpasswd
00baa000-00bcc000 rw-p 00000000 00:00 0                                  [heap]
7f8247e6d000-7f8247e83000 r-xp 00000000 fd:01 28341163                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f8247e83000-7f8248082000 ---p 00016000 fd:01 28341163                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f8248082000-7f8248083000 rw-p 00015000 fd:01 28341163                   /lib/x86_64-linux-gnu/libgcc_s.so.1
7f8248083000-7f8248243000 r-xp 00000000 fd:01 28767136                   /lib/x86_64-linux-gnu/libc-2.23.so
7f8248243000-7f8248443000 ---p 001c0000 fd:01 28767136                   /lib/x86_64-linux-gnu/libc-2.23.so
7f8248443000-7f8248447000 r--p 001c0000 fd:01 28767136                   /lib/x86_64-linux-gnu/libc-2.23.so
7f8248447000-7f8248449000 rw-p 001c4000 fd:01 28767136                   /lib/x86_64-linux-gnu/libc-2.23.so
7f8248449000-7f824844d000 rw-p 00000000 00:00 0 
7f824844d000-7f8248473000 r-xp 00000000 fd:01 28767125                   /lib/x86_64-linux-gnu/ld-2.23.so
7f8248665000-7f8248668000 rw-p 00000000 00:00 0 
7f8248671000-7f8248672000 rw-p 00000000 00:00 0 
7f8248672000-7f8248673000 r--p 00025000 fd:01 28767125                   /lib/x86_64-linux-gnu/ld-2.23.so
7f8248673000-7f8248674000 rw-p 00026000 fd:01 28767125                   /lib/x86_64-linux-gnu/ld-2.23.so
7f8248674000-7f8248675000 rw-p 00000000 00:00 0 
7fffa8f69000-7fffa8f8a000 rw-p 00000000 00:00 0                          [stack]
7fffa8fa6000-7fffa8fa9000 r--p 00000000 00:00 0                          [vvar]
7fffa8fa9000-7fffa8fab000 r-xp 00000000 00:00 0                          [vdso]
./bin/dev-deps: line 13:     7 Done                    echo "freedom"
         8 Aborted                 (core dumped) | vncpasswd -f > /tmp/vncpasswd
Retrying write of VNC credentials
      write static/css/journalist.css
      write static/css/journalist.css.map
      write static/css/source.css
      write static/css/source.css.map

rmol · 2019-03-26T13:41:44Z

@kushaldas Yes, vncpasswd can seg fault. I added a retry loop around it in securedrop/bin/dev-deps, but maybe we can take another look at the need for the passwd -- @zenmonkeykstop, what prompted that? Or, maybe there's another VNC server option -- again, I don't know the history for the switch from x11vnc.

kushaldas · 2019-03-27T13:49:56Z

Only segfault is different, and a buffer overflow is scary :)

rmol · 2019-03-27T15:44:43Z

In this case, I don't think it's that scary, as vncpasswd is running as an unprivileged user, invoked only by the run-test script. But if we want to eliminate the problem, we could:

Revert to x11vnc (Maybe @msheiny could weigh in on the motivation for the switch?).
Omit the authentication (though apparently it was necessary for Mac users, @zenmonkeykstop?).
Create the password file another way. There's at least one Python implementation of the obfuscation used, which we could incorporate.

Open to suggestions. I'd probably pick the first option.

rmol · 2019-03-31T17:25:02Z

I've reverted to x11vnc from tightvncserver. The reasons for the switch have been lost to time, the tests work and are visible with x11vnc, and it does support the passwords required on Macs, so I'd rather switch back than have to maintain workarounds for tightvncserver bugs.

Works for me, @kushaldas and @heartsucker, please check again when you have time, and I'll find a Mac user to test there as well.

Basically installed this because it can be used with pyvirtualdisplay as a backend AND because it brings in the Xvnc tooling which will start an X11 server as well as a VNC server. (cherry picked from commit af16906)

Had to remove x11 display logic inside test scaffolding (initially tried to integrate it there but it kept building and destroying the VNC server per test). Made a VNC helper command with support for GNOME desktop and macOS (havent tested it on mac yet). Updated the docs (cherry picked from commit 0a9567b)

(cherry picked from commit b53b657)

(cherry picked from commit f7aad63)

In dev-deps, work around random vncpasswd seg faults by retrying. In vnc-docker-connect.sh, suggest the installation of virt-viewer if it's missing. Add the continuation backslash I missed in dev-shell.

The vncpasswd utility of tightvncserver contains buffer overflows, and the reasons for switching to it have been lost, so rather than add workarounds for it, I'm just switching back to x11vnc. While updating the Dockerfiles, update the base Ubuntu images.

@rmol

@rmol pushed updated changes as requested

zenmonkeykstop · 2019-04-03T14:31:36Z

Failing for me on Qubes in a debian-9-based standaloneVM - docker output includes the following:

(securedrop) user@sd-dev:~/securedrop/securedrop$ ./bin/dev-shell  bin/run-test -v tests/functional
Run with DOCKER_BUILD_VERBOSE=true for more information
Docker image build in progress  done !
stored passwd in file: /tmp/vncpasswd
03/04/2019 14:27:00 passing arg to libvncserver: -rfbauth
03/04/2019 14:27:00 passing arg to libvncserver: /tmp/vncpasswd
03/04/2019 14:27:00 passing arg to libvncserver: -rfbport
03/04/2019 14:27:00 passing arg to libvncserver: 5901
03/04/2019 14:27:00 x11vnc version: 0.9.13 lastmod: 2011-08-10  pid: 14
03/04/2019 14:27:00 XOpenDisplay(":1") failed.
03/04/2019 14:27:00 Trying again with XAUTHLOCALHOSTNAME=localhost ...

03/04/2019 14:27:00 ***************************************
03/04/2019 14:27:00 *** XOpenDisplay failed (:1)

*** x11vnc was unable to open the X DISPLAY: ":1", it cannot continue.
*** There may be "Xlib:" error messages above with details about the failure.

Some tips and guidelines:

** An X server (the one you wish to view) must be running before x11vnc is
   started: x11vnc does not start the X server.  (however, see the -create
   option if that is what you really want).

** You must use -display <disp>, -OR- set and export your $DISPLAY
   environment variable to refer to the display of the desired X server.
 - Usually the display is simply ":0" (in fact x11vnc uses this if you forget
   to specify it), but in some multi-user situations it could be ":1", ":2",
   or even ":137".  Ask your administrator or a guru if you are having
   difficulty determining what your X DISPLAY is.

** Next, you need to have sufficient permissions (Xauthority) 
   to connect to the X DISPLAY.   Here are some Tips:

 - Often, you just need to run x11vnc as the user logged into the X session.
   So make sure to be that user when you type x11vnc.
 - Being root is usually not enough because the incorrect MIT-MAGIC-COOKIE
   file may be accessed.  The cookie file contains the secret key that
   allows x11vnc to connect to the desired X DISPLAY.
 - You can explicitly indicate which MIT-MAGIC-COOKIE file should be used
   by the -auth option, e.g.:
       x11vnc -auth /home/someuser/.Xauthority -display :0
       x11vnc -auth /tmp/.gdmzndVlR -display :0
   you must have read permission for the auth file.
   See also '-auth guess' and '-findauth' discussed below.

** If NO ONE is logged into an X session yet, but there is a greeter login
   program like "gdm", "kdm", "xdm", or "dtlogin" running, you will need
   to find and use the raw display manager MIT-MAGIC-COOKIE file.
   Some examples for various display managers:

     gdm:     -auth /var/gdm/:0.Xauth
              -auth /var/lib/gdm/:0.Xauth
     kdm:     -auth /var/lib/kdm/A:0-crWk72
              -auth /var/run/xauth/A:0-crWk72
     xdm:     -auth /var/lib/xdm/authdir/authfiles/A:0-XQvaJk
     dtlogin: -auth /var/dt/A:0-UgaaXa

   Sometimes the command "ps wwwwaux | grep auth" can reveal the file location.

   Starting with x11vnc 0.9.9 you can have it try to guess by using:

              -auth guess

   (see also the x11vnc -findauth option.)

   Only root will have read permission for the file, and so x11vnc must be run
   as root (or copy it).  The random characters in the filenames will of course
   change and the directory the cookie file resides in is system dependent.

See also: http://www.karlrunge.com/x11vnc/faq.html
...

rmol · 2019-04-03T14:37:04Z

@zenmonkeykstop Yeah, I started running into this in the big all-tbb merge too. I think the problem is that Xvfb sometimes hasn't fully started by the time we start x11vnc. Reordering things in run-test seems to have fixed it, but I'm planning on doing something more reliable.

zenmonkeykstop · 2019-04-03T14:53:22Z

@rmol fair enough, like maybe a test for xvfb's availability with a couple of sleep/retries?

FWIW, same issue under OS X

zenmonkeykstop · 2019-04-03T15:01:25Z

Added a sleep 30 between the xvfb and x11vnc startups, which cleared the above error. So the x11vnc bit itself is fine, it just needs the aforementioned reliability fix.

rmol · 2019-04-03T15:33:48Z

I've added a wait for Xvfb to be fully available.

zenmonkeykstop · 2019-04-03T16:44:11Z

Confirmed that VNC working with no issues on both Qubes(debian-9) and OS X with this fix. LGTM!

eloquence · 2019-04-03T20:10:55Z

Real lint failure for a change:

In ./securedrop/bin/dev-deps line 9:
    for i in {1..10}
    ^-- SC2034: i appears unused. Verify it or export it.

zenmonkeykstop · 2019-04-04T18:58:55Z

VNC still working fine on Qubes(debian-9) and OS X with the shellcheck fix above. 👍 from me.

emkll

Thanks @rmol this looks great. Went through the test plan and all 21 tests have passed under Debian 9. Some tests fail under a grsec kernel, due to missing PaX flags. I suspect those were there before, and I don't think we should block merge on that.

emkll · 2019-04-04T21:00:35Z

devops/scripts/vnc-docker-connect.sh

+
+if [ ! "$(which remote-viewer)" ]
+then
+    printf "\nError: We use the remote-viewer utility to reach Docker via VNC,\n"


this was a very helpful message :)

emkll · 2019-04-04T21:00:43Z

securedrop/bin/dev-shell

@@ -40,12 +40,14 @@ function docker_run() {

    find . \( -name '*.pyc' -o -name __pycache__ \) -delete
    docker run \
-	   --rm \
+       -p 127.0.0.1:5901:5901 \


5901 is only exposed only to localhost 👍

emkll · 2019-04-04T21:00:54Z

securedrop/dockerfiles/trusty/Dockerfile

-# ubuntu:14.04 as of 2019-01-22
-FROM ubuntu@sha256:cac55e5d97fad634d954d00a5c2a56d80576a08dcc01036011f26b88263f1578
+# ubuntu 14.04 image from 2019-03-12
+FROM ubuntu@sha256:6612de24437f6f01d6a2988ed9a36b3603df06e8d2c0493678f3ee696bc4bb2d


emkll · 2019-04-04T21:01:04Z

securedrop/dockerfiles/xenial/Dockerfile

-# ubuntu 16.04 image - 2019-01-22
-FROM ubuntu@sha256:e4a134999bea4abb4a27bc437e6118fdddfb172e1b9d683129b74d254af51675
+# ubuntu 16.04 image from 2019-03-12
+FROM ubuntu@sha256:58d0da8bc2f434983c6ca4713b08be00ff5586eb5cdff47bcde4b2e88fd40f88


emkll · 2019-04-04T21:08:02Z

devops/scripts/vnc-docker-connect.sh

+fi
+
+rv_config="${TMPDIR:-/tmp}/func-vnc.ini"
+echo -e "[virt-viewer]\ntype=vnc\nhost=${SD_DOCKER_IP}\nport=5901\npassword=freedom" > "${rv_config}"


Given VNC is exposed only to localhost and it's a dev container/test data, I think this is fine. We should follow up with a better way (maybe automatically generate a password?) in the future.

rmol requested review from conorsch, emkll, heartsucker, kushaldas, msheiny and redshiftzero as code owners March 21, 2019 23:09

rmol force-pushed the ft-vnc branch from a9beb9c to b5e844f Compare March 21, 2019 23:35

eloquence mentioned this pull request Mar 22, 2019

[wip] Add Tor Browser-based functional testing #4029

Closed

3 tasks

heartsucker previously approved these changes Mar 25, 2019

View reviewed changes

kushaldas previously requested changes Mar 26, 2019

View reviewed changes

rmol dismissed heartsucker’s stale review via c8890cd March 31, 2019 17:15

msheiny and others added 7 commits April 1, 2019 09:32

Swap x11vnc with tightvncserver

31616b0

Basically installed this because it can be used with pyvirtualdisplay as a backend AND because it brings in the Xvnc tooling which will start an X11 server as well as a VNC server. (cherry picked from commit af16906)

Added auth to VNC in test container, for OS X compatibility.

03a9a78

(cherry picked from commit b53b657)

added ini file to get around remote-viewer password prompt

4df4813

(cherry picked from commit f7aad63)

VNC script tweaks

3b8ba52

In dev-deps, work around random vncpasswd seg faults by retrying. In vnc-docker-connect.sh, suggest the installation of virt-viewer if it's missing. Add the continuation backslash I missed in dev-shell.

Fix shellcheck failures

4cdb2b9

rmol force-pushed the ft-vnc branch from c8890cd to 7960dbc Compare April 1, 2019 13:33

Wait for Xvfb to start in run-test

fb014bd

Appease shellcheck

e97f1f1

emkll approved these changes Apr 4, 2019

View reviewed changes

emkll merged commit 14ddf41 into freedomofpress:develop Apr 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract VNC changes from Tor Browser functional test branch #4288

Extract VNC changes from Tor Browser functional test branch #4288

rmol commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 22, 2019 •

edited

Loading

heartsucker commented Mar 25, 2019

kushaldas left a comment

rmol commented Mar 26, 2019

kushaldas commented Mar 27, 2019

rmol commented Mar 27, 2019

rmol commented Mar 31, 2019

zenmonkeykstop commented Apr 3, 2019

rmol commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

rmol commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

eloquence commented Apr 3, 2019 •

edited

Loading

zenmonkeykstop commented Apr 4, 2019

emkll left a comment

emkll Apr 4, 2019

emkll Apr 4, 2019

emkll Apr 4, 2019

emkll Apr 4, 2019

emkll Apr 4, 2019

Extract VNC changes from Tor Browser functional test branch #4288

Extract VNC changes from Tor Browser functional test branch #4288

Conversation

rmol commented Mar 21, 2019 • edited Loading

Status

Description of Changes

Testing

Checklist

If you made non-trivial code changes:

codecov-io commented Mar 22, 2019 • edited Loading

Codecov Report

heartsucker commented Mar 25, 2019

kushaldas left a comment

Choose a reason for hiding this comment

rmol commented Mar 26, 2019

kushaldas commented Mar 27, 2019

rmol commented Mar 27, 2019

rmol commented Mar 31, 2019

zenmonkeykstop commented Apr 3, 2019

rmol commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

rmol commented Apr 3, 2019

zenmonkeykstop commented Apr 3, 2019

eloquence commented Apr 3, 2019 • edited Loading

zenmonkeykstop commented Apr 4, 2019

emkll left a comment

Choose a reason for hiding this comment

emkll Apr 4, 2019

Choose a reason for hiding this comment

emkll Apr 4, 2019

Choose a reason for hiding this comment

emkll Apr 4, 2019

Choose a reason for hiding this comment

emkll Apr 4, 2019

Choose a reason for hiding this comment

emkll Apr 4, 2019

Choose a reason for hiding this comment

rmol commented Mar 21, 2019 •

edited

Loading

codecov-io commented Mar 22, 2019 •

edited

Loading

eloquence commented Apr 3, 2019 •

edited

Loading