improve state handling of obfsvpn; try to restart obfsvpn in on different proxy port in case the default one is already boudn

4 files changed, 146 insertions, 57 deletions

diff --git a/app/src/main/java/de/blinkt/openvpn/core/OpenVPNService.java b/app/src/main/java/de/blinkt/openvpn/core/OpenVPNService.java
index c8ac965f..1ed7bc39 100644
--- a/app/src/main/java/de/blinkt/openvpn/core/OpenVPNService.java
+++ b/app/src/main/java/de/blinkt/openvpn/core/OpenVPNService.java
@@ -182,6 +182,7 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
         synchronized (mProcessLock) {
             mProcessThread = null;
         }
+        stopObfsvpn();
         VpnStatus.removeByteCountListener(this);
         unregisterDeviceStateReceiver(mDeviceStateReceiver);
         mDeviceStateReceiver = null;
@@ -230,15 +231,20 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
             mDeviceStateReceiver.userPause(shouldBePaused);
     }
 
+    private boolean stopObfsvpn() {
+        if (obfsVpnClient == null || !obfsVpnClient.isStarted()) {
+            return true;
+        }
+        boolean success = obfsVpnClient.stop();
+        obfsVpnClient = null;
+        return success;
+    }
     @Override
     public boolean stopVPN(boolean replaceConnection) {
+        stopObfsvpn();
         if(isVpnRunning()) {
             if (getManagement() != null && getManagement().stopVPN(replaceConnection)) {
                 if (!replaceConnection) {
-                    if (obfsVpnClient != null && obfsVpnClient.isStarted()) {
-                        obfsVpnClient.stop();
-                        obfsVpnClient = null;
-                    }
                     VpnStatus.updateStateString("NOPROCESS", "VPN STOPPED", R.string.state_noprocess, ConnectionStatus.LEVEL_NOTCONNECTED);
                 }
                 return true;
@@ -369,17 +375,42 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
     }
 
     private void startOpenVPN() {
-        //TODO: investigate how connections[n] with n>0 get called during vpn setup (on connection refused?)
-        // Do we need to check if there's any obfs4 connection in mProfile.mConnections and start
-        // the dispatcher here? Can we start the dispatcher at a later point of execution, e.g. when
-        // connections[n], n>0 gets choosen?
-
         Connection connection = mProfile.mConnections[0];
         VpnStatus.setCurrentlyConnectingProfile(mProfile);
 
+        // Set a flag that we are starting a new VPN
+        mStarting = true;
+        // Stop the previous session by interrupting the thread.
+        stopOldOpenVPNProcess();
+        // An old running VPN should now be exited
+        mStarting = false;
+
+        // stop old running obfsvpn client
+        if (!stopObfsvpn()) {
+            VpnStatus.logError("Failed to stop already running obfsvpn client");
+            endVpnService();
+            return;
+        }
+
+        // optionally start start obfsvpn and adapt openvpn config to the port obfsvpn is listening to
+        Connection.TransportType transportType = connection.getTransportType();
+        if (mProfile.usePluggableTransports() && transportType.isPluggableTransport()) {
+            try {
+                obfsVpnClient = new ObfsvpnClient(((Obfs4Connection) connection).getObfs4Options());
+                obfsVpnClient.start();
+                int port = obfsVpnClient.getPort();
+                connection.setServerPort(String.valueOf(port));
+            } catch (RuntimeException e) {
+                e.printStackTrace();
+                VpnStatus.logException(e);
+                endVpnService();
+                return;
+            }
+        }
+
+        // write openvpn config
         VpnStatus.logInfo(R.string.building_configration);
         VpnStatus.updateStateString("VPN_GENERATE_CONFIG", "", R.string.building_configration, ConnectionStatus.LEVEL_START);
-
         try {
             mProfile.writeConfigFile(this);
         } catch (IOException e) {
@@ -387,6 +418,7 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
             endVpnService();
             return;
         }
+
         String nativeLibraryDirectory = getApplicationInfo().nativeLibraryDir;
         String tmpDir;
         try {
@@ -399,25 +431,6 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
         // Write OpenVPN binary
         String[] argv = VPNLaunchHelper.buildOpenvpnArgv(this);
 
-
-        // Set a flag that we are starting a new VPN
-        mStarting = true;
-        // Stop the previous session by interrupting the thread.
-
-        stopOldOpenVPNProcess();
-        // An old running VPN should now be exited
-        mStarting = false;
-        Connection.TransportType transportType = connection.getTransportType();
-        if (mProfile.usePluggableTransports() && transportType.isPluggableTransport()) {
-            if (obfsVpnClient != null && obfsVpnClient.isStarted()) {
-                obfsVpnClient.stop();
-            }
-            obfsVpnClient = new ObfsvpnClient(((Obfs4Connection) connection).getObfs4Options());
-            obfsVpnClient.start();
-            Log.d(TAG, "obfsvpn client started");
-        }
-
-
         // Start a new session by creating a new thread.
         boolean useOpenVPN3 = VpnProfile.doUseOpenVPN3(this);
 
@@ -471,11 +484,6 @@ public class OpenVPNService extends VpnService implements StateListener, Callbac
             if (mOpenVPNThread != null)
                 ((OpenVPNThread) mOpenVPNThread).setReplaceConnection();
             if (mManagement.stopVPN(true)) {
-                if (obfsVpnClient != null && obfsVpnClient.isStarted()) {
-                    Log.d(TAG, "-> stop obfsvpnClient");
-                    obfsVpnClient.stop();
-                    obfsVpnClient = null;
-                }
                 try {
                     Thread.sleep(1000);
                 } catch (InterruptedException e) {
diff --git a/app/src/main/java/de/blinkt/openvpn/core/connection/Obfs4Connection.java b/app/src/main/java/de/blinkt/openvpn/core/connection/Obfs4Connection.java
index 0fe6bff2..e2c596ac 100644
--- a/app/src/main/java/de/blinkt/openvpn/core/connection/Obfs4Connection.java
+++ b/app/src/main/java/de/blinkt/openvpn/core/connection/Obfs4Connection.java
@@ -15,7 +15,7 @@ public class Obfs4Connection extends Connection {
 
     public Obfs4Connection(Obfs4Options options) {
         setServerName(ObfsvpnClient.IP);
-        setServerPort(String.valueOf(ObfsvpnClient.PORT));
+        setServerPort(String.valueOf(ObfsvpnClient.DEFAULT_PORT));
         setUseUdp(true);
         setProxyType(ProxyType.NONE);
         setProxyName("");
diff --git a/app/src/main/java/se/leap/bitmaskclient/eip/VpnConfigGenerator.java b/app/src/main/java/se/leap/bitmaskclient/eip/VpnConfigGenerator.java
index 0288ab25..76e32349 100644
--- a/app/src/main/java/se/leap/bitmaskclient/eip/VpnConfigGenerator.java
+++ b/app/src/main/java/se/leap/bitmaskclient/eip/VpnConfigGenerator.java
@@ -379,7 +379,7 @@ public class VpnConfigGenerator {
         }
 
         stringBuilder.append(getRouteString(ipAddress, transport));
-        String transparentProxyRemote = REMOTE + " " + ObfsvpnClient.IP + " " + ObfsvpnClient.PORT + " udp" + newLine;
+        String transparentProxyRemote = REMOTE + " " + ObfsvpnClient.IP + " " + ObfsvpnClient.DEFAULT_PORT + " udp" + newLine;
         stringBuilder.append(transparentProxyRemote);
     }
 
diff --git a/app/src/main/java/se/leap/bitmaskclient/pluggableTransports/ObfsvpnClient.java b/app/src/main/java/se/leap/bitmaskclient/pluggableTransports/ObfsvpnClient.java
index 625bbfd8..0691c826 100644
--- a/app/src/main/java/se/leap/bitmaskclient/pluggableTransports/ObfsvpnClient.java
+++ b/app/src/main/java/se/leap/bitmaskclient/pluggableTransports/ObfsvpnClient.java
@@ -5,12 +5,16 @@ import static se.leap.bitmaskclient.base.models.Constants.QUIC;
 
 import android.util.Log;
 
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+
 import client.Client;
 import client.Client_;
 import client.EventLogger;
 import de.blinkt.openvpn.core.VpnStatus;
 import de.blinkt.openvpn.core.connection.Connection;
-import se.leap.bitmaskclient.base.models.Constants;
 import se.leap.bitmaskclient.pluggableTransports.models.HoppingConfig;
 import se.leap.bitmaskclient.pluggableTransports.models.KcpConfig;
 import se.leap.bitmaskclient.pluggableTransports.models.Obfs4Options;
@@ -19,20 +23,19 @@ import se.leap.bitmaskclient.pluggableTransports.models.QuicConfig;
 
 public class ObfsvpnClient implements EventLogger {
 
-    public static final int PORT = 8080;
+    public static final int DEFAULT_PORT = 8080;
     public static final String IP = "127.0.0.1";
+    private static final String ERROR_BIND = "bind: address already in use";
     private final Object LOCK = new Object();
-
+    private final AtomicInteger currentPort = new AtomicInteger(DEFAULT_PORT);
+    private CountDownLatch startCallback = null;
 
     private static final String TAG = ObfsvpnClient.class.getSimpleName();
 
     public final Client_ client;
 
     public ObfsvpnClient(Obfs4Options options) throws IllegalStateException {
-
-        //FIXME: use a different strategy here
-        //Basically we would want to track if the more performant transport protocol (KCP?/TCP?) usage was successful
-        //if so, we stick to it, otherwise we flip the flag
+        // each obfuscation transport has only 1 protocol
         String protocol = options.transport.getProtocols()[0];
         boolean kcpEnabled = KCP.equals(protocol);
         boolean quicEnabled = QUIC.equals(protocol);
@@ -42,57 +45,135 @@ public class ObfsvpnClient implements EventLogger {
         }
         KcpConfig kcpConfig = new KcpConfig(kcpEnabled);
         QuicConfig quicConfig = new QuicConfig(quicEnabled);
-        HoppingConfig hoppingConfig = new HoppingConfig(hoppingEnabled,IP+":"+PORT, options);
-        ObfsvpnConfig obfsvpnConfig = new ObfsvpnConfig(IP+":"+PORT, hoppingConfig, kcpConfig, quicConfig, options.bridgeIP, options.transport.getPorts()[0], options.transport.getOptions().getCert() );
+        HoppingConfig hoppingConfig = new HoppingConfig(hoppingEnabled,IP+":"+ DEFAULT_PORT, options);
+        ObfsvpnConfig obfsvpnConfig = new ObfsvpnConfig(IP+":"+ DEFAULT_PORT, hoppingConfig, kcpConfig, quicConfig, options.bridgeIP, options.transport.getPorts()[0], options.transport.getOptions().getCert() );
         try {
-            Log.d(TAG, obfsvpnConfig.toString());
+            Log.d(TAG, "create new obfsvpn client: " + obfsvpnConfig);
             client = Client.newFFIClient(obfsvpnConfig.toString());
         } catch (Exception e) {
             throw new IllegalStateException(e);
         }
     }
 
-    public int start() {
+    public void start() throws RuntimeException {
         synchronized (LOCK) {
+            client.setEventLogger(this);
+
+            // this CountDownLatch stops blocking if:
+            // a) obfsvpn changed its state to RUNNING
+            // b) an unrecoverable error happened
+            final CountDownLatch callback = new CountDownLatch(1);
+            this.startCallback = callback;
+            AtomicReference<Exception> err = new AtomicReference<>();
             new Thread(() -> {
                 try {
-                    if (client.isStarted()) {
-                        return;
-                    }
-                    client.setEventLogger(this);
-                    client.start();
-                } catch (Exception e) {
+                    start(0);
+                } catch (RuntimeException e) {
+                    // save exception and stop blocking
                     e.printStackTrace();
+                    err.set(e);
+                    callback.countDown();
                 }
             }).start();
-            return PORT;
+
+            try {
+                boolean completedBeforeTimeout = callback.await(35, TimeUnit.SECONDS);
+                Exception startException =  err.get();
+                this.startCallback = null;
+                if (!completedBeforeTimeout) {
+                    client.setEventLogger(null);
+                    throw new RuntimeException("failed to start obfsvpn: timeout error");
+                } else if (startException != null) {
+                    client.setEventLogger(null);
+                    throw new RuntimeException("failed to start obfsvpn: ", startException);
+                }
+            } catch (InterruptedException e) {
+                this.startCallback = null;
+                client.setEventLogger(null);
+                throw new RuntimeException("failed to start obfsvpn: ", e);
+            }
+        }
+    }
+
+    private void start(int portOffset) throws RuntimeException {
+        currentPort.set(DEFAULT_PORT + portOffset);
+        Log.d(TAG, "listen to 127.0.0.1:"+ (currentPort.get()));
+        final CountDownLatch errOnStartCDL = new CountDownLatch(1);
+        AtomicReference<Exception> err = new AtomicReference<>();
+        new Thread(() -> {
+            try {
+                client.setProxyAddr(IP + ":" + (DEFAULT_PORT+portOffset));
+                client.start();
+            } catch (Exception e) {
+                err.set(e);
+                errOnStartCDL.countDown();
+            }
+        }).start();
+
+        try {
+            // wait for 250 ms, in case there is an immediate error due to misconfiguration
+            // or bound ports the CountDownLatch is set to 0 and thus the return value of await is true
+            boolean receivedErr = errOnStartCDL.await(250, TimeUnit.MILLISECONDS);
+            if (receivedErr) {
+                Exception e = err.get();
+                // attempt to restart the client with a different local proxy port in case
+                // there's a port binding error
+                if (e != null &&
+                        e.getMessage() != null &&
+                        e.getMessage().contains(ERROR_BIND) &&
+                        portOffset < 10) {
+                    start(portOffset + 1);
+                    return;
+                } else {
+                    resetAndThrow(new RuntimeException("Failed to start obfsvpn: " + e));
+                }
+            }
+        } catch (InterruptedException e) {
+            resetAndThrow(new RuntimeException(e));
         }
     }
 
-    public void stop() {
+    private void resetAndThrow(RuntimeException e) throws RuntimeException{
+        startCallback.countDown();
+        startCallback = null;
+        client.setEventLogger(null);
+        throw e;
+    }
+
+    public boolean stop() {
         synchronized (LOCK) {
             try {
                 client.stop();
             } catch (Exception e) {
                 e.printStackTrace();
+                return false;
             } finally {
                 client.setEventLogger(null);
             }
+            return true;
         }
     }
 
+    public int getPort() {
+        return currentPort.get();
+    }
+
     public boolean isStarted() {
         return client.isStarted();
     }
 
     @Override
     public void error(String s) {
-        VpnStatus.logError("[obfs4-client] " + s);
-
+        VpnStatus.logError("[obfs4-client] error: " + s);
     }
 
     @Override
     public void log(String state, String message) {
         VpnStatus.logDebug("[obfs4-client] " + state + ": " + message);
+        CountDownLatch startCallback = this.startCallback;
+        if (startCallback != null && "RUNNING".equals(state)) {
+            startCallback.countDown();
+            this.startCallback = null;
+        }
     }
 }