Add tests for caching

- Change socketFactoryCache to use LinkedHashMap instead of ConcurrentHashMap
- Update cache key handling to use String? instead of Optional<String>

app/src/main/kotlin/at/bitfire/davdroid/di/CustomCertManagerModule.kt

@@ -0,0 +1,65 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.di
+
+import android.content.Context
+import at.bitfire.cert4android.CustomCertManager
+import at.bitfire.cert4android.CustomCertStore
+import at.bitfire.cert4android.SettingsProvider
+import at.bitfire.davdroid.BuildConfig
+import at.bitfire.davdroid.settings.Settings
+import at.bitfire.davdroid.settings.SettingsManager
+import at.bitfire.davdroid.ui.ForegroundTracker
+import dagger.Module
+import dagger.Provides
+import dagger.hilt.InstallIn
+import dagger.hilt.android.qualifiers.ApplicationContext
+import dagger.hilt.components.SingletonComponent
+import okhttp3.internal.tls.OkHostnameVerifier
+import java.util.Optional
+import javax.inject.Singleton
+
+@Module
+@InstallIn(SingletonComponent::class)
+/**
+ * cert4android integration module
+ */
+class CustomCertManagerModule {
+
+    @Provides
+    @Singleton
+    fun customCertManager(
+        @ApplicationContext context: Context,
+        settings: SettingsManager
+    ): Optional<CustomCertManager> =
+        if (BuildConfig.allowCustomCerts)
+            Optional.of(
+                CustomCertManager(
+                certStore = CustomCertStore.getInstance(context),
+                settings = object : SettingsProvider {
+
+                    override val appInForeground: Boolean
+                        get() = ForegroundTracker.inForeground.value
+
+                    override val trustSystemCerts: Boolean
+                        get() = !settings.getBoolean(Settings.DISTRUST_SYSTEM_CERTIFICATES)
+
+                }
+            ))
+        else
+            Optional.empty()
+
+    @Provides
+    @Singleton
+    fun customHostnameVerifier(
+        customCertManager: Optional<CustomCertManager>
+    ): Optional<CustomCertManager.HostnameVerifier> =
+        if (BuildConfig.allowCustomCerts && customCertManager.isPresent) {
+            val hostnameVerifier = customCertManager.get().HostnameVerifier(OkHostnameVerifier)
+            Optional.of(hostnameVerifier)
+        } else
+            Optional.empty()
+
+}

app/src/main/kotlin/at/bitfire/davdroid/network/ConnectionSecurityContext.kt

@@ -0,0 +1,24 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.network
+
+import javax.net.ssl.HostnameVerifier
+import javax.net.ssl.SSLSocketFactory
+import javax.net.ssl.X509TrustManager
+
+/**
+ * Holds information that shall be used to create TLS connections.
+ *
+ * @param sslSocketFactory  the socket factory that shall be used
+ * @param trustManager      the trust manager that shall be used
+ * @param hostnameVerifier  the hostname verifier that shall be used
+ * @param disableHttp2      whether HTTP/2 shall be disabled
+ */
+class ConnectionSecurityContext(
+    val sslSocketFactory: SSLSocketFactory?,
+    val trustManager: X509TrustManager?,
+    val hostnameVerifier: HostnameVerifier?,
+    val disableHttp2: Boolean
+)

app/src/main/kotlin/at/bitfire/davdroid/network/ConnectionSecurityManager.kt

@@ -0,0 +1,103 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.network
+
+import androidx.annotation.VisibleForTesting
+import at.bitfire.cert4android.CustomCertManager
+import java.lang.ref.SoftReference
+import java.security.KeyStore
+import java.util.Optional
+import javax.inject.Inject
+import javax.inject.Singleton
+import javax.net.ssl.SSLContext
+import javax.net.ssl.SSLSocketFactory
+import javax.net.ssl.TrustManagerFactory
+import javax.net.ssl.X509TrustManager
+import kotlin.jvm.optionals.getOrNull
+
+/**
+ * Caching provider for [ConnectionSecurityContext].
+ */
+@Singleton
+class ConnectionSecurityManager @Inject constructor(
+    private val customHostnameVerifier: Optional<CustomCertManager.HostnameVerifier>,
+    private val customTrustManager: Optional<CustomCertManager>,
+    private val keyManagerFactory: ClientCertKeyManager.Factory
+) {
+
+    /**
+     * Maps client certificate aliases (or `null` if no client authentication is used) to their SSLSocketFactory.
+     * Uses soft references for the values so that they can be garbage-collected when not used anymore.
+     *
+     * Not thread-safe, access must be synchronized by caller.
+     */
+    private val socketFactoryCache: MutableMap<String?, SoftReference<SSLSocketFactory>> =
+        LinkedHashMap(2)    // usually not more than: one for no client certificates + one for a certain certificate alias
+
+    /**
+     * The default TrustManager to use for connections. If [customTrustManager] provides a value, that value is
+     * used. Otherwise, the platform's default trust manager is used.
+     */
+    private val trustManager by lazy { customTrustManager.getOrNull() ?: defaultTrustManager() }
+
+    /**
+     * Provides the [ConnectionSecurityContext] for a given [certificateAlias].
+     *
+     * Uses [socketFactoryCache] to cache the entries (per [certificateAlias]).
+     *
+     * @param certificateAlias  alias of the client certificate that shall be used for authentication (`null` for none)
+     * @return the connection security context
+     */
+    fun getContext(certificateAlias: String?): ConnectionSecurityContext {
+        /* We only need a custom socket factory for
+           - client certificates and/or
+           - when cert4android is active (= there's a custom trustManager). */
+        val socketFactory = if (certificateAlias != null || customTrustManager.isPresent)
+            getSocketFactory(certificateAlias)
+        else
+            null
+
+        return ConnectionSecurityContext(
+            sslSocketFactory = socketFactory,
+            trustManager = if (socketFactory != null) trustManager else null,   // when there's a customTrustManager, there's always a socketFactory, too
+            hostnameVerifier = customHostnameVerifier.getOrNull(),
+            disableHttp2 = certificateAlias != null
+        )
+    }
+
+    @VisibleForTesting
+    internal fun getSocketFactory(certificateAlias: String?): SSLSocketFactory = synchronized(socketFactoryCache) {
+        // look up cache first
+        val cachedFactory = socketFactoryCache[certificateAlias]?.get()
+        if (cachedFactory != null)
+            return cachedFactory
+        // no cached value, calculate and store into cache
+
+        // when a client certificate alias is given, create and use the respective ClientKeyManager
+        val clientKeyManager = certificateAlias?.let { keyManagerFactory.create(it) }
+
+        // create SSLContext that provides the SSLSocketFactory
+        val sslContext = SSLContext.getInstance("TLS").apply {
+            init(
+                /* km = */ if (clientKeyManager != null) arrayOf(clientKeyManager) else null,
+                /* tm = */ arrayOf(trustManager),
+                /* random = */ null /* default RNG */
+            )
+        }
+
+        // cache reference and return socket factory
+        return sslContext.socketFactory.also { socketFactory ->
+            socketFactoryCache[certificateAlias] = SoftReference(socketFactory)
+        }
+    }
+
+    @VisibleForTesting
+    internal fun defaultTrustManager(): X509TrustManager {
+        val factory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm())
+        factory.init(null as KeyStore?)
+        return factory.trustManagers.filterIsInstance<X509TrustManager>().first()
+    }
+
+}

app/src/main/kotlin/at/bitfire/davdroid/network/HttpClientBuilder.kt

@@ -5,22 +5,16 @@
 package at.bitfire.davdroid.network
 
 import android.accounts.Account
-import android.content.Context
 import androidx.annotation.WorkerThread
-import at.bitfire.cert4android.CustomCertManager
-import at.bitfire.cert4android.CustomCertStore
 import at.bitfire.dav4jvm.okhttp.BasicDigestAuthHandler
 import at.bitfire.dav4jvm.okhttp.UrlUtils
-import at.bitfire.davdroid.BuildConfig
 import at.bitfire.davdroid.di.IoDispatcher
 import at.bitfire.davdroid.settings.AccountSettings
 import at.bitfire.davdroid.settings.Credentials
 import at.bitfire.davdroid.settings.Settings
 import at.bitfire.davdroid.settings.SettingsManager
-import at.bitfire.davdroid.ui.ForegroundTracker
 import com.google.common.net.HttpHeaders
 import com.google.errorprone.annotations.MustBeClosed
-import dagger.hilt.android.qualifiers.ApplicationContext
 import io.ktor.client.HttpClient
 import io.ktor.client.engine.okhttp.OkHttp
 import io.ktor.client.plugins.contentnegotiation.ContentNegotiation
@@ -35,20 +29,13 @@ import okhttp3.Interceptor
 import okhttp3.OkHttpClient
 import okhttp3.Protocol
 import okhttp3.brotli.BrotliInterceptor
-import okhttp3.internal.tls.OkHostnameVerifier
 import okhttp3.logging.HttpLoggingInterceptor
 import java.net.InetSocketAddress
 import java.net.Proxy
-import java.security.KeyStore
 import java.util.concurrent.TimeUnit
 import java.util.logging.Level
 import java.util.logging.Logger
 import javax.inject.Inject
-import javax.net.ssl.HostnameVerifier
-import javax.net.ssl.KeyManager
-import javax.net.ssl.SSLContext
-import javax.net.ssl.TrustManagerFactory
-import javax.net.ssl.X509TrustManager
 
 /**
  * Builder for the HTTP client.
@@ -60,10 +47,9 @@ import javax.net.ssl.X509TrustManager
  */
 class HttpClientBuilder @Inject constructor(
     private val accountSettingsFactory: AccountSettings.Factory,
-    @ApplicationContext private val context: Context,
+    private val connectionSecurityManager: ConnectionSecurityManager,
     defaultLogger: Logger,
     @IoDispatcher private val ioDispatcher: CoroutineDispatcher,
-    private val keyManagerFactory: ClientCertKeyManager.Factory,
     private val oAuthInterceptorFactory: OAuthInterceptor.Factory,
     private val settingsManager: SettingsManager
 ) {
@@ -283,72 +269,29 @@ class HttpClientBuilder @Inject constructor(
     }
 
     private fun buildConnectionSecurity(okBuilder: OkHttpClient.Builder) {
-        // allow cleartext and TLS 1.2+
+        // Allow cleartext and TLS 1.2+
         okBuilder.connectionSpecs(listOf(
             ConnectionSpec.CLEARTEXT,
             ConnectionSpec.MODERN_TLS
         ))
 
-        // client certificate
-        val clientKeyManager: KeyManager? = certificateAlias?.let { alias ->
-            try {
-                val manager = keyManagerFactory.create(alias)
-                logger.fine("Using certificate $alias for authentication")
+        /* Set SSLSocketFactory, TrustManager and HostnameVerifier (if needed).
+         * We shouldn't create these things here, because
+         *
+         * a. it involves complex logic that should be the responsibility of a dedicated class, and
+         * b. we need to cache the instances because otherwise, HTTPS connection are not used
+         *    correctly. okhttp checks the SSLSocketFactory/TrustManager of a connection in the pool
+         *    and creates a new connection when they have changed. */
+        val securityContext = connectionSecurityManager.getContext(certificateAlias)
 
-                // HTTP/2 doesn't support client certificates (yet)
-                // see https://datatracker.ietf.org/doc/draft-ietf-httpbis-secondary-server-certs/
-                okBuilder.protocols(listOf(Protocol.HTTP_1_1))
+        if (securityContext.disableHttp2)
+            okBuilder.protocols(listOf(Protocol.HTTP_1_1))
 
-                manager
-            } catch (e: IllegalArgumentException) {
-                logger.log(Level.SEVERE, "Couldn't create KeyManager for certificate $alias", e)
-                null
-            }
-        }
+        if (securityContext.sslSocketFactory != null && securityContext.trustManager != null)
+            okBuilder.sslSocketFactory(securityContext.sslSocketFactory, securityContext.trustManager)
 
-        // select trust manager and hostname verifier depending on whether custom certificates are allowed
-        val customTrustManager: X509TrustManager?
-        val customHostnameVerifier: HostnameVerifier?
-
-        if (BuildConfig.allowCustomCerts) {
-            // use cert4android for custom certificate handling
-            customTrustManager = CustomCertManager(
-                certStore = CustomCertStore.getInstance(context),
-                trustSystemCerts = !settingsManager.getBoolean(Settings.DISTRUST_SYSTEM_CERTIFICATES),
-                appInForeground = ForegroundTracker.inForeground
-            )
-            // allow users to accept certificates with wrong host names
-            customHostnameVerifier = customTrustManager.HostnameVerifier(OkHostnameVerifier)
-
-        } else {
-            // no custom certificates, use default trust manager and hostname verifier
-            customTrustManager = null
-            customHostnameVerifier = null
-        }
-
-        // change settings only if we have at least only one custom component
-        if (clientKeyManager != null || customTrustManager != null) {
-            val trustManager = customTrustManager ?: defaultTrustManager()
-
-            // use trust manager and client key manager (if defined) for TLS connections
-            val sslContext = SSLContext.getInstance("TLS")
-            sslContext.init(
-                /* km = */ if (clientKeyManager != null) arrayOf(clientKeyManager) else null,
-                /* tm = */ arrayOf(trustManager),
-                /* random = */ null
-            )
-            okBuilder.sslSocketFactory(sslContext.socketFactory, trustManager)
-        }
-
-        // also add the custom hostname verifier (if defined)
-        if (customHostnameVerifier != null)
-            okBuilder.hostnameVerifier(customHostnameVerifier)
-    }
-
-    private fun defaultTrustManager(): X509TrustManager {
-        val factory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm())
-        factory.init(null as KeyStore?)
-        return factory.trustManagers.filterIsInstance<X509TrustManager>().first()
+        if (securityContext.hostnameVerifier != null)
+            okBuilder.hostnameVerifier(securityContext.hostnameVerifier)
     }
 
     private fun buildProxy(okBuilder: OkHttpClient.Builder) {

app/src/test/kotlin/at/bitfire/davdroid/network/ConnectionSecurityManagerTest.kt

@@ -0,0 +1,197 @@
+/*
+ * Copyright © All Contributors. See LICENSE and AUTHORS in the root directory for details.
+ */
+
+package at.bitfire.davdroid.network
+
+import at.bitfire.cert4android.CustomCertManager
+import io.mockk.mockk
+import io.mockk.verify
+import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
+import org.junit.Assert.assertNotNull
+import org.junit.Assert.assertNull
+import org.junit.Assert.assertTrue
+import org.junit.Test
+import java.util.Optional
+
+class ConnectionSecurityManagerTest {
+
+    @Test
+    fun `getContext(no customTrustManager, no client certificate)`() {
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = mockk()
+        )
+        val context = manager.getContext(null)
+        assertNull(context.sslSocketFactory)
+        assertNull(context.trustManager)
+        assertNull(context.hostnameVerifier)
+        assertFalse(context.disableHttp2)
+    }
+
+    @Test
+    fun `getContext(no customTrustManager, with client certificate)`() {
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = kmf
+        )
+        val context = manager.getContext("alias")
+        assertNotNull(context.sslSocketFactory)
+        assertEquals(manager.defaultTrustManager().javaClass, context.trustManager?.javaClass)
+        assertNull(context.hostnameVerifier)
+        assertTrue(context.disableHttp2)
+        verify(exactly = 1) {
+            kmf.create("alias")
+        }
+    }
+
+    @Test
+    fun `getContext(with customTrustManager, no client certificate)`() {
+        val customTrustManager: CustomCertManager = mockk()
+        val customHostnameVerifier: CustomCertManager.HostnameVerifier = mockk()
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.of(customTrustManager),
+            customHostnameVerifier = Optional.of(customHostnameVerifier),
+            keyManagerFactory = mockk()
+        )
+        val context = manager.getContext(null)
+        assertNotNull(context.sslSocketFactory)
+        assertEquals(customTrustManager, context.trustManager)
+        assertEquals(customHostnameVerifier, context.hostnameVerifier)
+        assertFalse(context.disableHttp2)
+    }
+
+    @Test
+    fun `getContext(with customTrustManager, with client certificate)`() {
+        val customTrustManager: CustomCertManager = mockk()
+        val customHostnameVerifier: CustomCertManager.HostnameVerifier = mockk()
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.of(customTrustManager),
+            customHostnameVerifier = Optional.of(customHostnameVerifier),
+            keyManagerFactory = kmf
+        )
+        val context = manager.getContext("alias")
+        assertNotNull(context.sslSocketFactory)
+        assertEquals(customTrustManager, context.trustManager)
+        assertEquals(customHostnameVerifier, context.hostnameVerifier)
+        assertTrue(context.disableHttp2)
+        verify(exactly = 1) {
+            kmf.create("alias")
+        }
+    }
+
+    @Test
+    fun `getSocketFactory(no customTrustManager, no client certificate)`() {
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = mockk()
+        )
+        val socketFactory = manager.getSocketFactory(null)
+        assertNotNull(socketFactory.javaClass)
+    }
+
+    @Test
+    fun `getSocketFactory(no customTrustManager, with client certificate)`() {
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = kmf
+        )
+        val socketFactory = manager.getSocketFactory("alias")
+        assertNotNull(socketFactory.javaClass)
+        verify(exactly = 1) {
+            kmf.create("alias")
+        }
+    }
+
+    @Test
+    fun `getSocketFactory(with customTrustManager, no client certificate)`() {
+        val customTrustManager: CustomCertManager = mockk()
+        val customHostnameVerifier: CustomCertManager.HostnameVerifier = mockk()
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.of(customTrustManager),
+            customHostnameVerifier = Optional.of(customHostnameVerifier),
+            keyManagerFactory = mockk()
+        )
+        val socketFactory = manager.getSocketFactory(null)
+        assertNotNull(socketFactory.javaClass)
+    }
+
+    @Test
+    fun `getSocketFactory(with customTrustManager, with client certificate)`() {
+        val customTrustManager: CustomCertManager = mockk()
+        val customHostnameVerifier: CustomCertManager.HostnameVerifier = mockk()
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.of(customTrustManager),
+            customHostnameVerifier = Optional.of(customHostnameVerifier),
+            keyManagerFactory = kmf
+        )
+        val socketFactory = manager.getSocketFactory("alias")
+        assertNotNull(socketFactory.javaClass)
+        verify(exactly = 1) {
+            kmf.create("alias")
+        }
+    }
+
+    @Test
+    fun `getContext caches socket factories for same certificateAlias`() {
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = kmf
+        )
+        
+        // First call - should create new socket factory
+        val context1 = manager.getContext("alias")
+        assertNotNull(context1.sslSocketFactory)
+        
+        // Second call with same alias - should return cached socket factory
+        val context2 = manager.getContext("alias")
+        assertNotNull(context2.sslSocketFactory)
+        
+        // Both should return the same socket factory instance
+        assert(context1.sslSocketFactory === context2.sslSocketFactory)
+        
+        // Should only create key manager once
+        verify(exactly = 1) {
+            kmf.create("alias")
+        }
+    }
+
+    @Test
+    fun `getContext does not cache socket factories for different certificateAlias`() {
+        val kmf: ClientCertKeyManager.Factory = mockk(relaxed = true)
+        val manager = ConnectionSecurityManager(
+            customTrustManager = Optional.empty(),
+            customHostnameVerifier = Optional.empty(),
+            keyManagerFactory = kmf
+        )
+        
+        // Get context for first alias
+        val context1 = manager.getContext("alias1")
+        assertNotNull(context1.sslSocketFactory)
+        
+        // Get context for different alias
+        val context2 = manager.getContext("alias2")
+        assertNotNull(context2.sslSocketFactory)
+        
+        // Should be different instances
+        assert(context1.sslSocketFactory !== context2.sslSocketFactory)
+        
+        // Should create key managers for both aliases
+        verify(exactly = 1) {
+            kmf.create("alias1")
+            kmf.create("alias2")
+        }
+    }
+
+}

gradle/libs.versions.toml

@@ -18,7 +18,7 @@ androidx-test-runner = "1.7.0"
 androidx-test-rules = "1.7.0"
 androidx-test-junit = "1.3.0"
 androidx-work = "2.11.0"
-bitfire-cert4android = "42d883e958"
+bitfire-cert4android = "75cc6913fd"
 bitfire-dav4jvm = "acf8e4ef9b"
 bitfire-synctools = "25b92ef99a"
 compose-accompanist = "0.37.3"