Use cert pinning only if available

7 files changed, 36 insertions, 41 deletions

diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/FacebookKeyserver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/FacebookKeyserver.java
index 1c592003c..6217d1a01 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/FacebookKeyserver.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/FacebookKeyserver.java
@@ -106,10 +106,10 @@ public class FacebookKeyserver extends Keyserver {
             String request = String.format(FB_KEY_URL_FORMAT, fbUsername);
             Log.d(Constants.TAG, "fetching from Facebook with: " + request + " proxy: " + mProxy);
 
-            OkHttpClient client = OkHttpClientFactory.getClient(mProxy);
-
             URL url = new URL(request);
 
+            OkHttpClient client = OkHttpClientFactory.getClientPinnedIfAvailable(url, mProxy);
+
             Response response = client.newCall(new Request.Builder().url(url).build()).execute();
 
             // contains body both in case of success or failure
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java
index bdf43b6da..5e3d2ebc6 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/keyimport/HkpKeyserver.java
@@ -205,7 +205,7 @@ public class HkpKeyserver extends Keyserver {
         try {
             URL url = new URL(getUrlPrefix() + mHost + ":" + mPort + request);
             Log.d(Constants.TAG, "hkp keyserver query: " + url + " Proxy: " + proxy);
-            OkHttpClient client = OkHttpClientFactory.getPinnedClient(url, proxy);
+            OkHttpClient client = OkHttpClientFactory.getClientPinnedIfAvailable(url, proxy);
             Response response = client.newCall(new Request.Builder().url(url).build()).execute();
 
             String responseBody = response.body().string(); // contains body both in case of success or failure
@@ -396,7 +396,7 @@ public class HkpKeyserver extends Keyserver {
                     .post(body)
                     .build();
 
-            Response response = OkHttpClientFactory.getPinnedClient(url, mProxy).newCall(request).execute();
+            Response response = OkHttpClientFactory.getClientPinnedIfAvailable(url, mProxy).newCall(request).execute();
 
             Log.d(Constants.TAG, "response code: " + response.code());
             Log.d(Constants.TAG, "answer: " + response.body().string());
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/linked/LinkedTokenResource.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/linked/LinkedTokenResource.java
index 4a8882d50..a5f882dd0 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/linked/LinkedTokenResource.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/linked/LinkedTokenResource.java
@@ -244,7 +244,7 @@ public abstract class LinkedTokenResource extends LinkedResource {
         Log.d("Connection to: " + request.url().url().getHost(), "");
         OkHttpClient client;
         if (pins != null) {
-            client = OkHttpClientFactory.getPinnedSimpleClient(getCertificatePinner(request.url().url().getHost(), pins));
+            client = OkHttpClientFactory.getSimpleClientPinned(getCertificatePinner(request.url().url().getHost(), pins));
         } else {
             client = OkHttpClientFactory.getSimpleClient();
         }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java
index a10b94b8e..3dcc2f58b 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/dialog/AddEditKeyserverDialogFragment.java
@@ -353,8 +353,6 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On
 
                     Log.d("Converted URL", newKeyserver.toString());
 
-                    OkHttpClient client = OkHttpClientFactory.getPinnedClient(newKeyserver.toURL(), proxy);
-
                     if (onlyTrustedKeyserver
                             && TlsHelper.getPinnedSslSocketFactory(newKeyserver.toURL()) == null) {
                         Log.w(Constants.TAG, "No pinned certificate for this host in OpenKeychain's assets.");
@@ -362,6 +360,8 @@ public class AddEditKeyserverDialogFragment extends DialogFragment implements On
                         return reason;
                     }
 
+                    OkHttpClient client = OkHttpClientFactory.getClientPinnedIfAvailable(newKeyserver.toURL(), proxy);
+
                     client.newCall(new Request.Builder().url(newKeyserver.toURL()).build()).execute();
                 } catch (TlsHelper.TlsHelperException e) {
                     reason = FailureReason.CONNECTION_FAILED;
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpClientFactory.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpClientFactory.java
index f3606aa2f..ea2ae8368 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpClientFactory.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpClientFactory.java
@@ -38,7 +38,7 @@ public class OkHttpClientFactory {
         return client;
     }
 
-    public static OkHttpClient getPinnedSimpleClient(CertificatePinner pinner) {
+    public static OkHttpClient getSimpleClientPinned(CertificatePinner pinner) {
         return new OkHttpClient.Builder()
                 .connectTimeout(5000, TimeUnit.MILLISECONDS)
                 .readTimeout(25000, TimeUnit.MILLISECONDS)
@@ -46,32 +46,31 @@ public class OkHttpClientFactory {
                 .build();
     }
 
-    public static OkHttpClient getPinnedClient(URL url, Proxy proxy) throws IOException, TlsHelper.TlsHelperException {
+    public static OkHttpClient getClientPinnedIfAvailable(URL url, Proxy proxy) throws IOException,
+            TlsHelper.TlsHelperException {
+        OkHttpClient.Builder builder = new OkHttpClient.Builder();
 
-        return new OkHttpClient.Builder()
-                // don't follow any redirects for keyservers, as discussed in the security audit
-                .followRedirects(false)
-                .followSslRedirects(false)
-                .proxy(proxy)
-                        // higher timeouts for Tor
-                .connectTimeout(30000, TimeUnit.MILLISECONDS)
-                .readTimeout(45000, TimeUnit.MILLISECONDS)
-                        // use pinned cert with SocketFactory
-                .sslSocketFactory(TlsHelper.getPinnedSslSocketFactory(url))
-                .build();
-    }
+        // don't follow any redirects for keyservers, as discussed in the security audit
+        builder.followRedirects(false)
+                .followSslRedirects(false);
+
+        if (proxy != null) {
+            // set proxy and higher timeouts for Tor
+            builder.proxy(proxy);
+            builder.connectTimeout(30000, TimeUnit.MILLISECONDS)
+                    .readTimeout(45000, TimeUnit.MILLISECONDS);
+        } else {
+            builder.connectTimeout(5000, TimeUnit.MILLISECONDS)
+                    .readTimeout(25000, TimeUnit.MILLISECONDS);
+        }
 
-    public static OkHttpClient getClient(Proxy proxy) throws IOException, TlsHelper.TlsHelperException {
+        // If a pinned cert is available, use it!
+        // NOTE: this fails gracefully back to "no pinning" if no cert is available.
+        if (url != null && TlsHelper.getPinnedSslSocketFactory(url) != null) {
+            builder.sslSocketFactory(TlsHelper.getPinnedSslSocketFactory(url));
+        }
 
-        return new OkHttpClient.Builder()
-                // don't follow any redirects for keyservers, as discussed in the security audit
-                .followRedirects(false)
-                .followSslRedirects(false)
-                .proxy(proxy)
-                        // higher timeouts for Tor
-                .connectTimeout(30000, TimeUnit.MILLISECONDS)
-                .readTimeout(45000, TimeUnit.MILLISECONDS)
-                .build();
+        return builder.build();
     }
 
 }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpKeybaseClient.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpKeybaseClient.java
index 9b7b31d68..8d3eb6963 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpKeybaseClient.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/OkHttpKeybaseClient.java
@@ -21,16 +21,13 @@ package org.sufficientlysecure.keychain.util;
 import com.textuality.keybase.lib.KeybaseUrlConnectionClient;
 
 import okhttp3.OkHttpClient;
-import okhttp3.OkUrlFactory;
 import okhttp3.Request;
-import okhttp3.Response;
 
 import org.sufficientlysecure.keychain.Constants;
 
 import java.io.IOException;
 import java.net.Proxy;
 import java.net.URL;
-import java.net.URLConnection;
 
 /**
  * Wrapper for Keybase Lib
@@ -42,10 +39,8 @@ public class OkHttpKeybaseClient implements KeybaseUrlConnectionClient {
         OkHttpClient client = null;
 
         try {
-            if (isKeybase && proxy != null) {
-                client = OkHttpClientFactory.getPinnedClient(url, proxy);
-            } else if (proxy != null) {
-                client = OkHttpClientFactory.getClient(proxy);
+            if (proxy != null) {
+                client = OkHttpClientFactory.getClientPinnedIfAvailable(url, proxy);
             } else {
                 client = OkHttpClientFactory.getSimpleClient();
             }
diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java
index 77ed6fe0b..fe62eff55 100644
--- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java
+++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/util/TlsHelper.java
@@ -86,9 +86,10 @@ public class TlsHelper {
     }
 
     /**
-     * Modifies the builder to accept only requests with a given certificate. Applies to all URLs requested by the
-     * builder.
-     * Therefore a builder that is pinned this way should be used to only make requests to URLs with passed certificate.
+     * Modifies the builder to accept only requests with a given certificate.
+     * Applies to all URLs requested by the builder.
+     * Therefore a builder that is pinned this way should be used to only make requests
+     * to URLs with passed certificate.
      *
      * @param certificate certificate to pin
      * @throws TlsHelperException