Enhance SSL handling and testing

opencloudApp/src/main/java/eu/opencloud/android/ui/dialog/SslUntrustedCertDialog.java

@@ -82,7 +82,9 @@ public static SslUntrustedCertDialog newInstanceForFullSslError(CertificateCombi
             throw new IllegalArgumentException("Trying to create instance with parameter sslException == null");
         }
         SslUntrustedCertDialog dialog = new SslUntrustedCertDialog();
-        dialog.m509Certificate = sslException.getServerCertificate();
+        dialog.m509Certificate = sslException.getSslPeerUnverifiedException() == null
+                ? sslException.getServerCertificate()
+                : null;
         dialog.mErrorViewAdapter = new CertificateCombinedExceptionViewAdapter(sslException);
         dialog.mCertificateViewAdapter = new X509CertificateViewAdapter(sslException.getServerCertificate());
         return dialog;
@@ -144,6 +146,11 @@ public View onCreateView(LayoutInflater inflater, ViewGroup container, Bundle sa
         Button cancel = mView.findViewById(R.id.btnCancel);
         cancel.setOnClickListener(new OnCertificateNotTrusted());
 
+        if (m509Certificate == null) {
+            ok.setText(android.R.string.ok);
+            mView.findViewById(R.id.question).setVisibility(View.GONE);
+            cancel.setVisibility(View.GONE);
+        }
         Button details = mView.findViewById(R.id.details_btn);
         details.setOnClickListener(new OnClickListener() {
 
@@ -219,6 +226,8 @@ public void onClick(View v) {
                     ((OnSslUntrustedCertListener) activity).onFailedSavingCertificate();
                     Timber.e(e, "Server certificate could not be saved in the known-servers trust store ");
                 }
+            } else if (mHandler == null) {
+                ((OnSslUntrustedCertListener) getActivity()).onCancelCertificate();
             }
         }
 

opencloudComLibrary/build.gradle

@@ -27,6 +27,7 @@ dependencies {
     testImplementation 'org.robolectric:robolectric:4.15.1'
     // MockWebServer for HTTP integration tests
     testImplementation 'com.squareup.okhttp3:mockwebserver:4.9.2'
+    testImplementation 'com.squareup.okhttp3:okhttp-tls:4.9.2'
     // AndroidX test core to obtain application context in unit tests
     testImplementation 'androidx.test:core:1.5.0'
     debugImplementation 'com.facebook.stetho:stetho-okhttp3:1.6.0'

opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/http/HttpClient.java

@@ -125,7 +125,6 @@ private OkHttpClient buildNewOkHttpClient(SSLSocketFactory sslSocketFactory, X50
                 .connectTimeout(HttpConstants.DEFAULT_CONNECTION_TIMEOUT, TimeUnit.MILLISECONDS)
                 .followRedirects(false)
                 .sslSocketFactory(sslSocketFactory, trustManager)
-                .hostnameVerifier((asdf, usdf) -> true)
                 .cookieJar(cookieJar)
                 .build();
     }

opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/network/AdvancedX509TrustManager.java

@@ -84,11 +84,16 @@ public void checkClientTrusted(X509Certificate[] certificates, String authType)
         mStandardTrustManager.checkClientTrusted(certificates, authType);
     }
 
+    public static final ThreadLocal<X509Certificate> sLastCert = new ThreadLocal<>();
+
     /**
      * @see javax.net.ssl.X509TrustManager#checkServerTrusted(X509Certificate[],
      * String authType)
      */
     public void checkServerTrusted(X509Certificate[] certificates, String authType) {
+        if (certificates != null && certificates.length > 0) {
+            sLastCert.set(certificates[0]);
+        }
         if (!isKnownServer(certificates[0])) {
             CertificateCombinedException result = new CertificateCombinedException(certificates[0]);
             try {

opencloudComLibrary/src/main/java/eu/opencloud/android/lib/common/operations/RemoteOperationResult.java

@@ -32,6 +32,7 @@
 import eu.opencloud.android.lib.common.accounts.AccountUtils;
 import eu.opencloud.android.lib.common.http.HttpConstants;
 import eu.opencloud.android.lib.common.http.methods.HttpBaseMethod;
+import eu.opencloud.android.lib.common.network.AdvancedX509TrustManager;
 import eu.opencloud.android.lib.common.network.CertificateCombinedException;
 import okhttp3.Headers;
 import org.apache.commons.lang3.exception.ExceptionUtils;
@@ -148,6 +149,13 @@ public RemoteOperationResult(Exception e) {
 
         } else if (e instanceof SSLException || e instanceof RuntimeException) {
             if (e instanceof SSLPeerUnverifiedException) {
+                java.security.cert.X509Certificate lastCert = AdvancedX509TrustManager.sLastCert.get();
+                AdvancedX509TrustManager.sLastCert.remove();
+                CertificateCombinedException sslPeerUnverifiedException =
+                        new CertificateCombinedException(lastCert);
+                sslPeerUnverifiedException.setSslPeerUnverifiedException((SSLPeerUnverifiedException) e);
+                sslPeerUnverifiedException.initCause(e);
+                mException = sslPeerUnverifiedException;
                 mCode = ResultCode.SSL_RECOVERABLE_PEER_UNVERIFIED;
             } else {
                 CertificateCombinedException se = getCertificateCombinedException(e);

opencloudComLibrary/src/test/java/eu/opencloud/android/lib/common/http/HttpClientTlsTest.kt

@@ -0,0 +1,115 @@
+package eu.opencloud.android.lib.common.http
+
+import android.content.Context
+import android.os.Build
+import androidx.test.core.app.ApplicationProvider
+import eu.opencloud.android.lib.common.network.CertificateCombinedException
+import eu.opencloud.android.lib.common.network.NetworkUtils
+import eu.opencloud.android.lib.common.operations.RemoteOperationResult
+import okhttp3.Request
+import okhttp3.mockwebserver.MockResponse
+import okhttp3.mockwebserver.MockWebServer
+import okhttp3.tls.HandshakeCertificates
+import okhttp3.tls.HeldCertificate
+import org.junit.After
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertSame
+import org.junit.Assert.assertThrows
+import org.junit.Assert.assertTrue
+import org.junit.Before
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.annotation.Config
+import java.lang.reflect.Field
+import javax.net.ssl.SSLPeerUnverifiedException
+
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [Build.VERSION_CODES.O], manifest = Config.NONE)
+class HttpClientTlsTest {
+
+    private val context by lazy { ApplicationProvider.getApplicationContext<Context>() }
+    private lateinit var server: MockWebServer
+
+    @Before
+    fun setUp() {
+        resetKnownServersStore()
+        server = MockWebServer()
+    }
+
+    @After
+    fun tearDown() {
+        runCatching { server.shutdown() }
+        resetKnownServersStore()
+    }
+
+    @Test
+    fun `rejects trusted certificate for the wrong hostname`() {
+        val wrongHostnameCertificate = HeldCertificate.Builder()
+            .commonName(WRONG_HOSTNAME)
+            .addSubjectAlternativeName(WRONG_HOSTNAME)
+            .build()
+        val serverCertificates = HandshakeCertificates.Builder()
+            .heldCertificate(wrongHostnameCertificate)
+            .build()
+
+        server.useHttps(serverCertificates.sslSocketFactory(), false)
+        server.enqueue(MockResponse().setResponseCode(200).setBody("ok"))
+        server.start()
+
+        NetworkUtils.addCertToKnownServersStore(wrongHostnameCertificate.certificate, context)
+
+        val request = Request.Builder()
+            .url(server.url("/"))
+            .build()
+
+        val thrown = assertThrows(Exception::class.java) {
+            TestHttpClient(context).okHttpClient.newCall(request).execute().use { }
+        }
+
+        assertNotNull(findCause<SSLPeerUnverifiedException>(thrown))
+    }
+
+    @Test
+    fun `wraps hostname mismatch as certificate combined exception`() {
+        val peerUnverifiedException = SSLPeerUnverifiedException("Hostname localhost not verified")
+
+        val result = RemoteOperationResult<Unit>(peerUnverifiedException)
+
+        assertEquals(
+            RemoteOperationResult.ResultCode.SSL_RECOVERABLE_PEER_UNVERIFIED,
+            result.code
+        )
+        assertTrue(result.exception is CertificateCombinedException)
+
+        val combinedException = result.exception as CertificateCombinedException
+        assertSame(peerUnverifiedException, combinedException.sslPeerUnverifiedException)
+    }
+
+    private inline fun <reified T : Throwable> findCause(throwable: Throwable): T? {
+        var current: Throwable? = throwable
+        while (current != null) {
+            if (current is T) {
+                return current
+            }
+            current = current.cause
+        }
+        return null
+    }
+
+    private fun resetKnownServersStore() {
+        context.deleteFile(KNOWN_SERVERS_STORE_FILE)
+
+        val field: Field = NetworkUtils::class.java.getDeclaredField("mKnownServersStore")
+        field.isAccessible = true
+        field.set(null, null)
+    }
+
+    private class TestHttpClient(context: Context) : HttpClient(context)
+
+    companion object {
+        private const val WRONG_HOSTNAME = "wrong.example.test"
+        private const val KNOWN_SERVERS_STORE_FILE = "knownServers.bks"
+    }
+}