[feat] fix OpenVPN start/stop in OSX using a process canary

- correctly start the openvpn process canary
- use helper to fix tearing down of the vpn

7 files changed, 69 insertions, 51 deletions

diff --git a/src/leap/bitmask/vpn/_control.py b/src/leap/bitmask/vpn/_control.py
index 53d4d315..f20184f7 100644
--- a/src/leap/bitmask/vpn/_control.py
+++ b/src/leap/bitmask/vpn/_control.py
@@ -10,10 +10,7 @@ from .constants import IS_MAC
 
 log = Logger()
 
-# NOTE: We need to set a bigger poll time in OSX because it seems
-# openvpn malfunctions when you ask it a lot of things in a short
-# amount of time. Check this!
-POLL_TIME = 2.5 if IS_MAC else 1.0
+POLL_TIME = 1
 
 
 class VPNControl(object):
@@ -25,6 +22,9 @@ 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
@@ -84,7 +84,8 @@ class VPNControl(object):
         # generic watchers
 
         poll_list = [LoopingCall(vpnproc.pollStatus),
-                     LoopingCall(vpnproc.pollState)]
+                     LoopingCall(vpnproc.pollState),
+                     LoopingCall(vpnproc.pollLog)]
         self._pollers.extend(poll_list)
         self._start_pollers()
         return True
@@ -107,26 +108,29 @@ class VPNControl(object):
         :type restart: bool
         """
         self._stop_pollers()
+        try:
+            self._vpnproc.preDown()
+        except Exception as e:
+            log.error('Error on vpn pre-down {0!r}'.format(e))
+            raise
 
-        # First we try to be polite and send a SIGTERM...
-        if self._vpnproc is not None:
-            # We assume that the only valid stops are initiated
-            # by an user action, not hard restarts
-            self._user_stopped = not restart
-            self._vpnproc.restarting = restart
-
-            self._sentterm = True
-            self._vpnproc.terminate_openvpn(shutdown=shutdown)
-
-            # ...but we also trigger a countdown to be unpolite
-            # if strictly needed.
-            reactor.callLater(
-                self.TERMINATE_WAIT, self._kill_if_left_alive)
-        else:
-            log.debug('VPN is not running.')
-
-        self._vpnproc.traffic_status = (0, 0)
-
+        if IS_LINUX:
+            # TODO factor this out to a linux-only launcher mechanism.
+            # First we try to be polite and send a SIGTERM...
+            if self._vpnproc is not None:
+                # We assume that the only valid stops are initiated
+                # by an user action, not hard restarts
+                self._user_stopped = not restart
+                self._vpnproc.restarting = restart
+
+                self._sentterm = True
+                self._vpnproc.terminate(shutdown=shutdown)
+
+                # ...but we also trigger a countdown to be unpolite
+                # if strictly needed.
+                reactor.callLater(
+                    self.TERMINATE_WAIT, self._kill_if_left_alive)
+                self._vpnproc.traffic_status = (0, 0)
         return True
 
     @property
diff --git a/src/leap/bitmask/vpn/_management.py b/src/leap/bitmask/vpn/_management.py
index 26050be6..80a82c93 100644
--- a/src/leap/bitmask/vpn/_management.py
+++ b/src/leap/bitmask/vpn/_management.py
@@ -79,8 +79,6 @@ class VPNManagement(object):
         if not self._host or not self._port:
             raise ImproperlyConfigured('Connection is not configured')
 
-        if self.is_connected():
-            self._close_management_socket()
         try:
             self._tn = UDSTelnet(self._host, self._port)
             self._tn.read_eager()
@@ -117,7 +115,7 @@ class VPNManagement(object):
                 self.connect_retry, retry + 1)
 
     def process_log(self):
-        if not self._watcher:
+        if not self._watcher or not self._tn:
             return
 
         lines = self._send_command('log 20')
@@ -203,6 +201,7 @@ class VPNManagement(object):
         :type output: list
         """
         for line in output:
+            status_step = ''
             stripped = line.strip()
             if stripped == "END":
                 continue
@@ -212,7 +211,10 @@ class VPNManagement(object):
             try:
                 ts, status_step, ok, ip, remote, port, _, _, _ = parts
             except ValueError:
-                ts, status_step, ok, ip, remote, port, _, _ = parts
+                try:
+                    ts, status_step, ok, ip, remote, port, _, _ = parts
+                except ValueError:
+                    self.log.debug('Could not parse %s' % parts)
 
             state = status_step
             if state != self._last_state:
@@ -280,7 +282,7 @@ class VPNManagement(object):
         if self.is_connected():
             return self._parse_status_and_notify(self._send_command("status"))
 
-    def terminate_openvpn(self, shutdown=False):
+    def terminate(self, shutdown=False):
         """
         Attempts to terminate openvpn by sending a SIGTERM.
         """
diff --git a/src/leap/bitmask/vpn/fw/firewall.py b/src/leap/bitmask/vpn/fw/firewall.py
index dcd956d4..71479718 100644
--- a/src/leap/bitmask/vpn/fw/firewall.py
+++ b/src/leap/bitmask/vpn/fw/firewall.py
@@ -48,27 +48,31 @@ class _OSXFirewallManager(object):
     def __init__(self, remotes):
         self._remotes = list(remotes)
         self._helper = darwin.HelperCommand()
+        self._started = False
 
     def start(self, restart=False):
         gateways = [gateway for gateway, port in self._remotes]
         cmd = 'firewall_start %s' % (' '.join(gateways),)
         self._helper.send(cmd)
+        self._started = True
         # TODO parse OK from result
         return True
 
     def stop(self):
         cmd = 'firewall_stop'
         self._helper.send(cmd)
+        self._started = False
         return True
 
     def is_up(self):
         # TODO implement!!!
-        return True
+        return self._started
 
     @property
     def status(self):
         # TODO implement!!! -- factor out, too
-        return {'status': 'on', 'error': None}
+        status = 'on' if self._started else 'off'
+        return {'status': status, 'error': None}
 
 
 class _LinuxFirewallManager(object):
diff --git a/src/leap/bitmask/vpn/launchers/darwin.py b/src/leap/bitmask/vpn/launchers/darwin.py
index d83ede25..440ed59c 100644
--- a/src/leap/bitmask/vpn/launchers/darwin.py
+++ b/src/leap/bitmask/vpn/launchers/darwin.py
@@ -49,7 +49,6 @@ class HelperCommand(object):
             raise RuntimeError(msg)
 
     def send(self, cmd, args=''):
-        # TODO check cmd is in allowed list
         self._connect()
         sock = self._sock
         data = ""
diff --git a/src/leap/bitmask/vpn/process.py b/src/leap/bitmask/vpn/process.py
index 3de652ff..170df446 100644
--- a/src/leap/bitmask/vpn/process.py
+++ b/src/leap/bitmask/vpn/process.py
@@ -137,7 +137,6 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
             # TODO -- internalize this into _status!!! so that it can be shared
             if 'SIGTERM[soft,ping-restart]' in line:
                 self.restarting = True
-            self.log.info(line)
 
     def processExited(self, failure):
         """
@@ -151,10 +150,14 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
         elif err == internet_error.ProcessTerminated:
             status, errmsg = 'failure', failure.value.exitCode
             if errmsg:
-                self.log.debug('processExited, status %d' % (errmsg,))
+                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
 
@@ -195,6 +198,9 @@ class _VPNProcess(protocol.ProcessProtocol, _management.VPNManagement):
     def preUp(self):
         pass
 
+    def preDown(self):
+        pass
+
     def getCommand(self):
         """
         Gets the vpn command from the aproppriate launcher.
@@ -255,8 +261,8 @@ elif IS_MAC:
     class _VPNCanary(_VPNProcess):
 
         """
-        Special form of _VPNProcess, for Darwin Launcher (windows might use
-        same strategy).
+        Special form of _VPNProcess, for Darwin Launcher (windows might end up
+        using the same strategy).
 
         This is a Canary Process that does not run openvpn itself, but it's
         notified by the privileged process when the process dies.
@@ -274,10 +280,13 @@ elif IS_MAC:
             cmd = self.getVPNCommand()
             self.helper.send('openvpn_start %s' % ' '.join(cmd))
 
+        def preDown(self):
+            self.helper.send('openvpn_stop')
+
         def connectionMade(self):
-            super(_VPNProcess, self).connectionMade()
             self.setupHelper()
             reactor.callLater(2, self.registerPID)
+            _VPNProcess.connectionMade(self)
 
         def registerPID(self):
             cmd = 'openvpn_set_watcher %s' % self.pid
@@ -293,11 +302,9 @@ elif IS_MAC:
 
         def getCommand(self):
             canary = '''import sys, signal, time
-    def receive_signal(signum, stack):
-        sys.exit()
-    signal.signal(signal.SIGTERM, receive_signal)
-    while True:
-        time.sleep(60)'''
+def receive_signal(signum, stack): sys.exit()
+signal.signal(signal.SIGTERM, receive_signal)
+while True: time.sleep(90)'''
             return ['python', '-c', '%s' % canary]
 
     VPNProcess = _VPNCanary
diff --git a/src/leap/bitmask/vpn/tunnel.py b/src/leap/bitmask/vpn/tunnel.py
index 4236edf5..70b4be0e 100644
--- a/src/leap/bitmask/vpn/tunnel.py
+++ b/src/leap/bitmask/vpn/tunnel.py
@@ -34,6 +34,8 @@ from .constants import IS_WIN
 # TODO DO NOT pass VPNConfig/ProviderConfig beyond this class.
 # TODO split sync/async vpn control mechanisms.
 
+# TODO ConfiguredVPNConnection ?
+
 
 class VPNTunnel(object):
 
@@ -63,16 +65,16 @@ class VPNTunnel(object):
 
         host, port = self._get_management_location()
 
-        self._vpn = VPNControl(remotes=remotes,
-                               vpnconfig=self._vpnconfig,
-                               providerconfig=self._providerconfig,
-                               socket_host=host, socket_port=port)
+        self._vpncontrol = VPNControl(
+            remotes=remotes, vpnconfig=self._vpnconfig,
+            providerconfig=self._providerconfig,
+            socket_host=host, socket_port=port)
 
     def start(self):
         """
         Start the VPN process.
         """
-        result = self._vpn.start()
+        result = self._vpncontrol.start()
         return result
 
     def stop(self):
@@ -83,16 +85,16 @@ class VPNTunnel(object):
         :rtype: bool
         """
         # TODO how to return False if this fails
-        result = self._vpn.stop(False, False)  # TODO review params
+        result = self._vpncontrol.stop(False, False)  # TODO review params
         return result
 
     @property
     def status(self):
-        return self._vpn.status
+        return self._vpncontrol.status
 
     @property
     def traffic_status(self):
-        return self._vpn.traffic_status
+        return self._vpncontrol.traffic_status
 
     def _get_management_location(self):
         """
diff --git a/src/leap/bitmask/vpn/tunnelmanager.py b/src/leap/bitmask/vpn/tunnelmanager.py
index 5faac662..4fb50047 100644
--- a/src/leap/bitmask/vpn/tunnelmanager.py
+++ b/src/leap/bitmask/vpn/tunnelmanager.py
@@ -30,7 +30,7 @@ from leap.bitmask.vpn.tunnel import VPNTunnel
 class TunnelManager(object):
 
     """
-    A TunnelManager controls VPN and Firewall
+    A TunnelManager controls a VPNTunnel and the Firewall
     """
 
     def __init__(self, provider, remotes, cert, key, ca, flags):