[refactor] integrate new management protocol

8 files changed, 230 insertions, 311 deletions

diff --git a/src/leap/bitmask/vpn/_control.py b/src/leap/bitmask/vpn/_control.py
index 45d2f2f4..ff39db8d 100644
--- a/src/leap/bitmask/vpn/_control.py
+++ b/src/leap/bitmask/vpn/_control.py
@@ -1,16 +1,25 @@
 import os
 
-from twisted.internet.task import LoopingCall
 from twisted.internet import reactor, defer
 from twisted.logger import Logger
 
 from .process import VPNProcess
 from .constants import IS_LINUX
 
-POLL_TIME = 1
+
+# TODO
+# TODO merge these classes with service.
+# [ ] register change state listener
+# emit_async(catalog.VPN_STATUS_CHANGED)
+# [ ] catch ping-restart
+# 'NETWORK_UNREACHABLE': (
+#    'Network is unreachable (code=101)',),
+# 'PROCESS_RESTART_TLS': (
+#    "SIGTERM[soft,tls-error]",),
 
 
 class VPNControl(object):
+
     """
     This is the high-level object that the service knows about.
     It exposes the start and terminate methods.
@@ -19,24 +28,17 @@ class VPNControl(object):
     suited for the running platform and connect to the management interface
     opened by the openvpn process, executing commands over that interface on
     demand.
-
-    This class also has knowledge of the reactor, since it controlls the
-    pollers that write and read to the management interface.
     """
+
     TERMINATE_MAXTRIES = 10
     TERMINATE_WAIT = 1  # secs
     RESTART_WAIT = 2  # secs
 
-    OPENVPN_VERB = "openvpn_verb"
-
     log = Logger()
 
     def __init__(self, remotes, vpnconfig,
                  providerconfig, socket_host, socket_port):
         self._vpnproc = None
-        self._pollers = []
-
-        self._openvpn_verb = None
         self._user_stopped = False
 
         self._remotes = remotes
@@ -49,7 +51,6 @@ class VPNControl(object):
         self.log.debug('VPN: start')
 
         self._user_stopped = False
-        self._stop_pollers()
 
         args = [self._vpnconfig, self._providerconfig, self._host,
                 self._port]
@@ -57,22 +58,24 @@ class VPNControl(object):
                   'restartfun': self.restart}
 
         vpnproc = VPNProcess(*args, **kwargs)
-        if vpnproc.get_openvpn_process():
-            self.log.info(
-                'Another vpn process is running. Will try to stop it.')
-            vpnproc.stop_if_already_running()
+
+        # TODO -- restore
+        # if get_openvpn_process():
+        #    self.log.info(
+        #        'Another vpn process is running. Will try to stop it.')
+        #    vpnproc.stop_if_already_running()
 
         try:
             vpnproc.preUp()
         except Exception as e:
             self.log.error('Error on vpn pre-up {0!r}'.format(e))
-            return False
+            raise
         try:
             cmd = vpnproc.getCommand()
         except Exception as e:
             self.log.error(
                 'Error while getting vpn command... {0!r}'.format(e))
-            return False
+            raise
 
         env = os.environ
 
@@ -80,22 +83,13 @@ class VPNControl(object):
             runningproc = reactor.spawnProcess(vpnproc, cmd[0], cmd, env)
         except Exception as e:
             self.log.error(
-                'Error while spwanning vpn process... {0!r}'.format(e))
+                'Error while spawning vpn process... {0!r}'.format(e))
             return False
 
+        # TODO get pid from management instead
         vpnproc.pid = runningproc.pid
         self._vpnproc = vpnproc
 
-        # add pollers for status and state
-        # this could be extended to a collection of
-        # generic watchers
-
-        poll_list = [
-            LoopingCall(vpnproc.pollStatus),
-            LoopingCall(vpnproc.pollState),
-            LoopingCall(vpnproc.pollLog)]
-        self._pollers.extend(poll_list)
-        self._start_pollers()
         return True
 
     @defer.inlineCallbacks
@@ -122,7 +116,6 @@ class VPNControl(object):
         if self._vpnproc is not None:
             self._vpnproc.restarting = restart
 
-        self._stop_pollers()
         try:
             if self._vpnproc is not None:
                 self._vpnproc.preDown()
@@ -136,16 +129,16 @@ class VPNControl(object):
             # First we try to be polite and send a SIGTERM...
             if self._vpnproc is not None:
                 self._sentterm = True
-                self._vpnproc.terminate(shutdown=shutdown)
+                self._vpnproc.terminate()
 
                 # we trigger a countdown to be unpolite
                 # if strictly needed.
                 d = defer.Deferred()
                 reactor.callLater(
                     self.TERMINATE_WAIT, self._kill_if_left_alive, d)
-                self._vpnproc.traffic_status = (0, 0)
         return d
 
+    # TODO -- remove indirection
     @property
     def status(self):
         if not self._vpnproc:
@@ -157,15 +150,11 @@ class VPNControl(object):
         return self._vpnproc.traffic_status
 
     def _killit(self):
-        """
-        Sends a kill signal to the process.
-        """
-        self._stop_pollers()
         if self._vpnproc is None:
             self.log.debug("There's no vpn process running to kill.")
         else:
             self._vpnproc.aborted = True
-            self._vpnproc.killProcess()
+            self._vpnproc.kill()
 
     def _kill_if_left_alive(self, deferred, tries=0):
         """
@@ -194,21 +183,3 @@ class VPNControl(object):
         except OSError:
             self.log.error('Could not kill process!')
         deferred.callback(True)
-
-    def _start_pollers(self):
-        """
-        Iterate through the registered observers
-        and start the looping call for them.
-        """
-        for poller in self._pollers:
-            poller.start(POLL_TIME)
-
-    def _stop_pollers(self):
-        """
-        Iterate through the registered observers
-        and stop the looping calls if they are running.
-        """
-        for poller in self._pollers:
-            if poller.running:
-                poller.stop()
-        self._pollers = []
diff --git a/src/leap/bitmask/vpn/_management.py b/src/leap/bitmask/vpn/_management.py
index d05790c4..4cc582f8 100644
--- a/src/leap/bitmask/vpn/_management.py
+++ b/src/leap/bitmask/vpn/_management.py
@@ -7,6 +7,7 @@ from twisted.logger import Logger
 
 import psutil
 try:
+    # TODO - we can deprecate this error
     # psutil < 2.0.0
     from psutil.error import AccessDenied as psutil_AccessDenied
     PSUTIL_2 = False
@@ -17,144 +18,9 @@ except ImportError:
 
 
 
-class OpenVPNAlreadyRunning(Exception):
-    message = ("Another openvpn instance is already running, and could "
-               "not be stopped.")
 
 
-class AlienOpenVPNAlreadyRunning(Exception):
-    message = ("Another openvpn instance is already running, and could "
-               "not be stopped because it was not launched by LEAP.")
-
-
-class ImproperlyConfigured(Exception):
-    pass
-
-
-class Management(object):
-
-
-    def terminate(self, shutdown=False):
-        """
-        Attempts to terminate openvpn by sending a SIGTERM.
-        """
-        if self.is_connected():
-            self._send_command("signal SIGTERM")
-        if shutdown:
-            _cleanup_tempfiles()
-
 
 # TODO -- finish porting ----------------------------------------------------
 
-def _cleanup_tempfiles(self):
-    """
-    Remove all temporal files we might have left behind.
-
-    Iif self.port is 'unix', we have created a temporal socket path that,
-    under normal circumstances, we should be able to delete.
-    """
-    if self._socket_port == "unix":
-        tempfolder = _first(os.path.split(self._host))
-        if tempfolder and os.path.isdir(tempfolder):
-            try:
-                shutil.rmtree(tempfolder)
-            except OSError:
-                self.log.error(
-                    'Could not delete tmpfolder %s' % tempfolder)
-
-def _get_openvpn_process():
-    """
-    Looks for openvpn instances running.
-
-    :rtype: process
-    """
-    openvpn_process = None
-    for p in psutil.process_iter():
-        try:
-            # XXX Not exact!
-            # Will give false positives.
-            # we should check that cmdline BEGINS
-            # with openvpn or with our wrapper
-            # (pkexec / osascript / whatever)
-
-            # This needs more work, see #3268, but for the moment
-            # we need to be able to filter out arguments in the form
-            # --openvpn-foo, since otherwise we are shooting ourselves
-            # in the feet.
-
-            if PSUTIL_2:
-                cmdline = p.cmdline()
-            else:
-                cmdline = p.cmdline
-            if any(map(lambda s: s.find(
-                    "LEAPOPENVPN") != -1, cmdline)):
-                openvpn_process = p
-                break
-        except psutil_AccessDenied:
-            pass
-    return openvpn_process
-
-def _stop_if_already_running():
-    """
-    Checks if VPN is already running and tries to stop it.
-
-    Might raise OpenVPNAlreadyRunning.
-
-    :return: True if stopped, False otherwise
-
-    """
-    process = _get_openvpn_process()
-    if not process:
-        self.log.debug('Could not find openvpn process while '
-                       'trying to stop it.')
-        return
-
-    log.debug('OpenVPN is already running, trying to stop it...')
-    cmdline = process.cmdline
-
-    manag_flag = "--management"
-
-    if isinstance(cmdline, list) and manag_flag in cmdline:
-
-        # we know that our invocation has this distinctive fragment, so
-        # we use this fingerprint to tell other invocations apart.
-        # this might break if we change the configuration path in the
-        # launchers
-
-        def smellslikeleap(s):
-            return "leap" in s and "providers" in s
-
-        if not any(map(smellslikeleap, cmdline)):
-            self.log.debug("We cannot stop this instance since we do not "
-                           "recognise it as a leap invocation.")
-            raise AlienOpenVPNAlreadyRunning
-
-        try:
-            index = cmdline.index(manag_flag)
-            host = cmdline[index + 1]
-            port = cmdline[index + 2]
-            self.log.debug("Trying to connect to %s:%s"
-                           % (host, port))
-            _connect()
-
-            # XXX this has a problem with connections to different
-            # remotes. So the reconnection will only work when we are
-            # terminating instances left running for the same provider.
-            # If we are killing an openvpn instance configured for another
-            # provider, we will get:
-            # TLS Error: local/remote TLS keys are out of sync
-            # However, that should be a rare case right now.
-            self._send_command("signal SIGTERM")
-        except (Exception, AssertionError):
-            log.failure('Problem trying to terminate OpenVPN')
-    else:
-        log.debug('Could not find the expected openvpn command line.')
 
-    process = _get_openvpn_process()
-    if process is None:
-        self.log.debug('Successfully finished already running '
-                       'openvpn process.')
-        return True
-    else:
-        self.log.warn('Unable to terminate OpenVPN')
-        raise OpenVPNAlreadyRunning
diff --git a/src/leap/bitmask/vpn/launchers/darwin.py b/src/leap/bitmask/vpn/launchers/darwin.py
index 6fbc0bec..ed1c034c 100644
--- a/src/leap/bitmask/vpn/launchers/darwin.py
+++ b/src/leap/bitmask/vpn/launchers/darwin.py
@@ -88,3 +88,6 @@ class DarwinVPNLauncher(VPNLauncher):
     else:
         # let's try with the homebrew path
         OPENVPN_BIN_PATH = '/usr/local/sbin/openvpn'
+
+    def kill_previous_openvpn():
+        pass
diff --git a/src/leap/bitmask/vpn/launchers/linux.py b/src/leap/bitmask/vpn/launchers/linux.py
index 2edfb5ea..38b9e417 100644
--- a/src/leap/bitmask/vpn/launchers/linux.py
+++ b/src/leap/bitmask/vpn/launchers/linux.py
@@ -19,18 +19,55 @@
 Linux VPN launcher implementation.
 """
 
-import commands
 import os
 
+import psutil
+
+from twisted.internet import defer, reactor
+from twisted.internet.endpoints import clientFromString, connectProtocol
 from twisted.logger import Logger
 
 from leap.bitmask.util import STANDALONE
 from leap.bitmask.vpn.utils import first, force_eval
 from leap.bitmask.vpn.privilege import LinuxPolicyChecker
+from leap.bitmask.vpn.management import ManagementProtocol
 from leap.bitmask.vpn.launcher import VPNLauncher
 
-logger = Logger()
-COM = commands
+log = Logger()
+
+
+class OpenVPNAlreadyRunning(Exception):
+    message = ("Another openvpn instance is already running, and could "
+               "not be stopped.")
+
+
+class AlienOpenVPNAlreadyRunning(Exception):
+    message = ("Another openvpn instance is already running, and could "
+               "not be stopped because it was not launched by LEAP.")
+
+
+def _maybe_get_running_openvpn():
+    """
+    Looks for previously running openvpn instances.
+
+    :rtype: psutil Process
+    """
+    openvpn = None
+    for p in psutil.process_iter():
+        try:
+            # This needs more work, see #3268, but for the moment
+            # we need to be able to filter out arguments in the form
+            # --openvpn-foo, since otherwise we are shooting ourselves
+            # in the feet.
+
+            cmdline = p.cmdline()
+            if any(map(lambda s: s.find(
+                    "LEAPOPENVPN") != -1, cmdline)):
+                openvpn = p
+                break
+        except psutil.AccessDenied:
+            pass
+    return openvpn
 
 
 class LinuxVPNLauncher(VPNLauncher):
@@ -106,3 +143,80 @@ class LinuxVPNLauncher(VPNLauncher):
                 command.insert(0, first(pkexec))
 
         return command
+
+    def kill_previous_openvpn(kls):
+        """
+        Checks if VPN is already running and tries to stop it.
+
+        Might raise OpenVPNAlreadyRunning.
+
+        :return: a deferred, that fires with True if stopped.
+        """
+        @defer.inlineCallbacks
+        def gotProtocol(proto):
+            return proto.signal('SIGTERM')
+
+        def connect_to_management(path):
+            # XXX this has a problem with connections to different
+            # remotes. So the reconnection will only work when we are
+            # terminating instances left running for the same provider.
+            # If we are killing an openvpn instance configured for another
+            # provider, we will get:
+            # TLS Error: local/remote TLS keys are out of sync
+            # However, that should be a rare case right now.
+            endpoint = clientFromString(reactor, path)
+            d = connectProtocol(endpoint, ManagementProtocol(verbose=False))
+            d.addCallback(gotProtocol)
+            return d
+
+        def verify_termination(ignored):
+            openvpn = _maybe_get_running_openvpn()
+            if openvpn is None:
+                log.debug('Successfully finished already running '
+                          'openvpn process.')
+                return True
+            else:
+                log.warn('Unable to terminate OpenVPN')
+                raise OpenVPNAlreadyRunning
+
+        openvpn = _maybe_get_running_openvpn()
+        if not openvpn:
+            log.debug('Could not find openvpn process while '
+                      'trying to stop it.')
+            return False
+
+        log.debug('OpenVPN is already running, trying to stop it...')
+        cmdline = openvpn.cmdline
+        management = "--management"
+
+        if isinstance(cmdline, list) and management in cmdline:
+            # we know that our invocation has this distinctive fragment, so
+            # we use this fingerprint to tell other invocations apart.
+            # this might break if we change the configuration path in the
+            # launchers
+
+            def smellslikeleap(s):
+                return "leap" in s and "providers" in s
+
+            if not any(map(smellslikeleap, cmdline)):
+                log.debug("We cannot stop this instance since we do not "
+                          "recognise it as a leap invocation.")
+                raise AlienOpenVPNAlreadyRunning
+
+            try:
+                index = cmdline.index(management)
+                host = cmdline[index + 1]
+                port = cmdline[index + 2]
+                log.debug("Trying to connect to %s:%s"
+                          % (host, port))
+
+                if port == 'unix':
+                    path = b"unix:path=%s" % host
+                d = connect_to_management(path)
+                d.addCallback(verify_termination)
+                return d
+
+            except (Exception, AssertionError):
+                log.failure('Problem trying to terminate OpenVPN')
+        else:
+            log.debug('Could not find the expected openvpn command line.')
diff --git a/src/leap/bitmask/vpn/management.py b/src/leap/bitmask/vpn/management.py
index b9bda6c9..1994e0bf 100644
--- a/src/leap/bitmask/vpn/management.py
+++ b/src/leap/bitmask/vpn/management.py
@@ -23,7 +23,6 @@ Handles an OpenVPN process through its Management Interface.
 import time
 from collections import OrderedDict
 
-from twisted.internet import defer
 from twisted.protocols.basic import LineReceiver
 from twisted.internet.defer import Deferred
 from twisted.python import log
@@ -68,6 +67,9 @@ class ManagementProtocol(LineReceiver):
 
     def lineReceived(self, line):
         if self.verbose:
+            # TODO get an integer parameter instead
+            # TODO if very verbose, print everything
+            # if less verbose, print (log) only the "DEBUG" lines.
             print line
 
         if line[0] == '>':
@@ -191,7 +193,7 @@ class ManagementProtocol(LineReceiver):
     def _parsePid(self, result):
         self.pid = int(result.split('=')[1])
 
-    def get_pid(self):
+    def getPid(self):
         d = self._pushdef()
         self.sendLine('pid')
         d.addCallback(self._parsePid)
diff --git a/src/leap/bitmask/vpn/process.py b/src/leap/bitmask/vpn/process.py
index 3a86160b..0ba4b85a 100644
--- a/src/leap/bitmask/vpn/process.py
+++ b/src/leap/bitmask/vpn/process.py
@@ -22,22 +22,24 @@ A custom processProtocol launches the VPNProcess and connects to its management
 interface.
 """
 
+import shutil
 import sys
 
-from twisted.internet import protocol, reactor
+import psutil
+
+from twisted.internet import protocol, reactor, defer
 from twisted.internet import error as internet_error
+from twisted.internet.endpoints import clientFromString, connectProtocol
 from twisted.logger import Logger
 
 from leap.bitmask.vpn.utils import get_vpn_launcher
-from leap.bitmask.vpn._status import VPNStatus
-from leap.bitmask.vpn._management import VPNManagement
+from leap.bitmask.vpn.management import ManagementProtocol
 
 from leap.bitmask.vpn.launchers import darwin
 from leap.bitmask.vpn.constants import IS_MAC, IS_LINUX
 
 
-# OpenVPN verbosity level - from flags.py
-OPENVPN_VERBOSITY = 1
+OPENVPN_VERBOSITY = 4
 
 
 class _VPNProcess(protocol.ProcessProtocol):
@@ -71,75 +73,53 @@ class _VPNProcess(protocol.ProcessProtocol):
         :param socket_port: either string "unix" if it's a unix
                             socket, or port otherwise
         :type socket_port: str
-
-        :param openvpn_verb: the desired level of verbosity in the
-                             openvpn invocation
-        :type openvpn_verb: int
         """
-        self._management = VPNManagement()
-        self._management.set_connection(socket_host, socket_port)
         self._host = socket_host
         self._port = socket_port
 
+        if socket_port == 'unix':
+            self._management_endpoint = clientFromString(
+                reactor, b"unix:path=%s" % socket_host)
+        else:
+            raise ValueError('tcp endpoint not configured')
         self._vpnconfig = vpnconfig
         self._providerconfig = providerconfig
         self._launcher = get_vpn_launcher()
-
-        self._alive = False
-
-        # XXX use flags, maybe, instead of passing
-        # the parameter around.
-        self._openvpn_verb = openvpn_verb
         self._restartfun = restartfun
 
-        self._status = VPNStatus()
-        self._management.set_watcher(self._status)
-
         self.restarting = True
+        self.proto = None
         self._remotes = remotes
 
-    @property
-    def status(self):
-        status = self._status.status
-        if status['status'] == 'off' and self.restarting:
-            status['status'] = 'starting'
-        return status
-
-    @property
-    def traffic_status(self):
-        return self._status.get_traffic_status()
-
     # processProtocol methods
 
+    @defer.inlineCallbacks
+    def _got_management_protocol(self, proto):
+        self.proto = proto
+        try:
+            yield proto.logOn()
+            yield proto.getVersion()
+            yield proto.getPid()
+            yield proto.stateOn()
+            yield proto.byteCount(2)
+        except Exception as exc:
+            print('[!] Error: %s' % exc)
+
+    def _connect_to_management(self):
+        # TODO -- add retries, twisted style, to this.
+        # this sometimes raises 'file not found' error
+        self._d = connectProtocol(
+            self._management_endpoint,
+            ManagementProtocol(verbose=True))
+        self._d.addCallback(self._got_management_protocol)
+        self._d.addErrback(self.log.error)
+
     def connectionMade(self):
-        """
-        Called when the connection is made.
-        """
-        self._alive = True
         self.aborted = False
-        self._management.connect_retry(max_retries=10)
-
-    def outReceived(self, data):
-        """
-        Called when new data is available on stdout.
-        We only use this to drive the status state machine in linux, OSX uses
-        the management interface.
-
-        :param data: the data read on stdout
-        """
-        # TODO deprecate, use log through management  interface too.
-
-        if IS_LINUX:
-            # truncate the newline
-            line = data[:-1]
-            # TODO -- internalize this into _status!!! so that it can be shared
-            if 'SIGTERM[soft,ping-restart]' in line:
-                self.restarting = True
+        # TODO cut this wait time when retries are done
+        reactor.callLater(0.5, self._connect_to_management)
 
     def processExited(self, failure):
-        """
-        Called when the child process exits.
-        """
         err = failure.trap(
             internet_error.ProcessDone, internet_error.ProcessTerminated)
 
@@ -151,14 +131,10 @@ class _VPNProcess(protocol.ProcessProtocol):
                 self.log.debug('Process Exited, status %d' % (errmsg,))
             else:
                 self.log.warn('%r' % failure.value)
-
         if IS_MAC:
             # TODO: need to exit properly!
             status, errmsg = 'off', None
 
-        self._status.set_status(status, errmsg)
-        self._alive = False
-
     def processEnded(self, reason):
         """
         Called when the child process exits and all file descriptors associated
@@ -169,43 +145,46 @@ class _VPNProcess(protocol.ProcessProtocol):
             self.log.debug('processEnded, status %d' % (exit_code,))
             if self.restarting:
                 self.log.debug('Restarting VPN process')
+                self._cleanup()
                 self._restartfun()
 
-    # polling
-
-    def pollStatus(self):
+    def _cleanup(self):
         """
-        Polls connection status.
-        """
-        if self._alive:
-            try:
-                up, down = self._management.get_traffic_status()
-                self._status.set_traffic_status(up, down)
-            except Exception:
-                self.log.debug('Could not parse traffic status')
-
-    def pollState(self):
-        """
-        Polls connection state.
+        Remove all temporal files we might have left behind.
+
+        Iif self.port is 'unix', we have created a temporal socket path that,
+        under normal circumstances, we should be able to delete.
         """
-        if self._alive:
-            try:
-                state = self._management.get_state()
-                self._status.set_status(state, None)
-            except Exception:
-                self.log.debug('Could not parse connection state')
-
-    def pollLog(self):
-        if self._alive:
-            try:
-                self._management.process_log()
-            except Exception:
-                self.log.debug('Could not parse log')
+        if self._port == "unix":
+            tempfolder = os.path.split(self._host)[0]
+            if tempfolder and os.path.isdir(tempfolder):
+                try:
+                    shutil.rmtree(tempfolder)
+                except OSError:
+                    self.log.error(
+                        'Could not delete VPN temp folder %s' % tempfolder)
+
+    # status handling
+
+    @property
+    def status(self):
+        if not self.proto:
+            status = {'status': 'off', 'error': None}
+            return status
+        status = {'status': self.proto.state.simple.lower(),
+                  'error': None}
+        if self.proto.traffic:
+            down, up = self.proto.traffic.get_rate()
+            status['up'] = up
+            status['down'] = down
+        if status['status'] == 'off' and self.restarting:
+            status['status'] = 'starting'
+        return status
 
     # launcher
 
     def preUp(self):
-        pass
+        self._launcher.kill_previous_openvpn()
 
     def preDown(self):
         pass
@@ -225,7 +204,6 @@ class _VPNProcess(protocol.ProcessProtocol):
             providerconfig=self._providerconfig,
             socket_host=self._host,
             socket_port=self._port,
-            openvpn_verb=self._openvpn_verb,
             remotes=self._remotes)
 
         encoding = sys.getfilesystemencoding()
@@ -237,21 +215,12 @@ class _VPNProcess(protocol.ProcessProtocol):
         self.log.debug("{0}".format(" ".join(command)))
         return command
 
-    def get_openvpn_process(self):
-        return self._management.get_openvpn_process()
-
     # shutdown
 
-    def stop_if_already_running(self):
-        return self._management.stop_if_already_running()
-
-    def terminate(self, shutdown=False):
-        self._management.terminate(shutdown)
+    def terminate(self):
+        self.proto.signal('SIGTERM')
 
-    def killProcess(self):
-        """
-        Sends the KILL signal to the running process.
-        """
+    def kill(self):
         try:
             self.transport.signalProcess('KILL')
         except internet_error.ProcessExitedAlready:
diff --git a/src/leap/bitmask/vpn/service.py b/src/leap/bitmask/vpn/service.py
index 704bbc71..2390391f 100644
--- a/src/leap/bitmask/vpn/service.py
+++ b/src/leap/bitmask/vpn/service.py
@@ -96,22 +96,16 @@ class VPNService(HookableService):
 
         yield self._setup(domain)
 
-        try:
-            fw_ok = self._firewall.start()
-            if not fw_ok:
-                raise Exception('Could not start firewall')
+        fw_ok = self._firewall.start()
+        if not fw_ok:
+            raise Exception('Could not start firewall')
 
+        try:
             vpn_ok = self._tunnel.start()
-            if not vpn_ok:
-                self._firewall.stop()
-                raise Exception('Could not start VPN')
-
-        # XXX capture it inside start method
-        # here I'd like to get (status, message)
-        except NoPolkitAuthAgentAvailable as e:
-            e.expected = True
-            raise e
-        # --------------------------------------
+        except Exception as exc:
+            self._firewall.stop()
+            # TODO get message from exception
+            raise Exception('Could not start VPN (reason: %r)' % exc)
 
         self._domain = domain
         self._write_last(domain)
diff --git a/src/leap/bitmask/vpn/tunnel.py b/src/leap/bitmask/vpn/tunnel.py
index c62d0670..c2b0f58b 100644
--- a/src/leap/bitmask/vpn/tunnel.py
+++ b/src/leap/bitmask/vpn/tunnel.py
@@ -79,7 +79,7 @@ class TunnelManager(object):
 
     def stop(self):
         """
-        Bring openvpn down using the privileged wrapper.
+        Bring openvpn down.
 
         :returns: True if succeeded, False otherwise.
         :rtype: bool