From c8e5395d4e3c3dcc349ebe6bb300016f44d430d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sat, 9 Apr 2016 18:34:00 +0200 Subject: Use cert pinning only if available --- .../keychain/keyimport/FacebookKeyserver.java | 4 +- .../keychain/keyimport/HkpKeyserver.java | 4 +- .../keychain/linked/LinkedTokenResource.java | 2 +- .../ui/dialog/AddEditKeyserverDialogFragment.java | 4 +- .../keychain/util/OkHttpClientFactory.java | 47 +++++++++++----------- .../keychain/util/OkHttpKeybaseClient.java | 9 +---- .../keychain/util/TlsHelper.java | 7 ++-- 7 files changed, 36 insertions(+), 41 deletions(-) (limited to 'OpenKeychain/src/main/java/org/sufficientlysecure') 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 -- cgit v1.2.3