Only use cert4android when needed (#1802)

* Refactor ClientCertKeyManager and HttpClientBuilder

- Add logging to `ClientCertKeyManager` for better error handling.
- Update `HttpClientBuilder` to conditionally use custom trust manager and hostname verifier based on `allowCustomCerts` flag.
- Rename `customCertsUI` to `allowCustomCerts` in build configuration.

* Update trust manager and hostname verifier selection logic

- Improve logging and error handling in `ClientCertKeyManager`

* App settings: hide certificate settings when custom certificates are not allowed

* Typo

app/build.gradle.kts

@@ -27,7 +27,8 @@ android {
         minSdk = 24        // Android 7.0
         targetSdk = 36     // Android 16
 
-        buildConfigField("boolean", "customCertsUI", "true")
+        // whether the build supports and allows to use custom certificates
+        buildConfigField("boolean", "allowCustomCerts", "true")
 
         testInstrumentationRunner = "at.bitfire.davdroid.HiltTestRunner"
     }

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

@@ -6,22 +6,31 @@ package at.bitfire.davdroid.network
 
 import android.content.Context
 import android.security.KeyChain
+import android.security.KeyChainException
 import dagger.assisted.Assisted
 import dagger.assisted.AssistedFactory
 import dagger.assisted.AssistedInject
 import dagger.hilt.android.qualifiers.ApplicationContext
 import java.net.Socket
 import java.security.Principal
+import java.security.PrivateKey
+import java.security.cert.X509Certificate
+import java.util.logging.Level
+import java.util.logging.Logger
 import javax.net.ssl.X509ExtendedKeyManager
 
 /**
- * KeyManager that provides a client certificate and private key from the Android KeyChain.
+ * KeyManager that provides a client certificate and private key from the Android [KeyChain].
  *
- * @throws IllegalArgumentException if the alias doesn't exist or is not accessible
+ * Requests for certificates / private keys for other aliases than the specified one
+ * will be ignored.
+ *
+ * @param alias     alias of the desired certificate / private key
  */
 class ClientCertKeyManager @AssistedInject constructor(
     @Assisted private val alias: String,
-    @ApplicationContext private val context: Context
+    @ApplicationContext private val context: Context,
+    private val logger: Logger
 ): X509ExtendedKeyManager() {
 
     @AssistedFactory
@@ -29,19 +38,42 @@ class ClientCertKeyManager @AssistedInject constructor(
         fun create(alias: String): ClientCertKeyManager
     }
 
-    val certs = KeyChain.getCertificateChain(context, alias) ?: throw IllegalArgumentException("Alias doesn't exist or not accessible: $alias")
-    val key = KeyChain.getPrivateKey(context, alias) ?: throw IllegalArgumentException("Alias doesn't exist or not accessible: $alias")
-
     override fun getServerAliases(p0: String?, p1: Array<out Principal>?): Array<String>? = null
     override fun chooseServerAlias(p0: String?, p1: Array<out Principal>?, p2: Socket?) = null
 
     override fun getClientAliases(p0: String?, p1: Array<out Principal>?) = arrayOf(alias)
     override fun chooseClientAlias(p0: Array<out String>?, p1: Array<out Principal>?, p2: Socket?) = alias
 
-    override fun getCertificateChain(forAlias: String?) =
-        certs.takeIf { forAlias == alias }
+    override fun getCertificateChain(forAlias: String): Array<X509Certificate>? {
+        if (forAlias != alias)
+            return null
 
-    override fun getPrivateKey(forAlias: String?) =
-        key.takeIf { forAlias == alias }
+        return try {
+            KeyChain.getCertificateChain(context, alias).also { result ->
+                if (result == null)
+                    logger.warning("Couldn't obtain certificate chain for alias $alias")
+            }
+        } catch (e: KeyChainException) {
+            // Android <Q throws an exception instead of returning null
+            logger.log(Level.WARNING, "Couldn't obtain certificate chain for alias $alias", e)
+            null
+        }
+    }
+
+    override fun getPrivateKey(forAlias: String): PrivateKey? {
+        if (forAlias != alias)
+            return null
+
+        return try {
+            KeyChain.getPrivateKey(context, alias).also { result ->
+                if (result == null)
+                    logger.log(Level.WARNING, "Couldn't obtain private key for alias $alias")
+            }
+        } catch (e: KeyChainException) {
+            // Android <Q throws an exception instead of returning null
+            logger.log(Level.WARNING, "Couldn't obtain private key for alias $alias", e)
+            null
+        }
+    }
 
 }

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

@@ -33,12 +33,16 @@ 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 [OkHttpClient].
@@ -79,6 +83,7 @@ class HttpClientBuilder @Inject constructor(
     }
 
     private var loggerInterceptorLevel: HttpLoggingInterceptor.Level = HttpLoggingInterceptor.Level.BODY
+
     fun loggerInterceptorLevel(level: HttpLoggingInterceptor.Level): HttpClientBuilder {
         loggerInterceptorLevel = level
         return this
@@ -86,6 +91,7 @@ class HttpClientBuilder @Inject constructor(
 
     // default cookie store for non-persistent cookies (some services like Horde use cookies for session tracking)
     private var cookieStore: CookieJar = MemoryCookieStore()
+
     fun setCookieStore(cookieStore: CookieJar): HttpClientBuilder {
         this.cookieStore = cookieStore
         return this
@@ -94,6 +100,7 @@ class HttpClientBuilder @Inject constructor(
     private var authenticationInterceptor: Interceptor? = null
     private var authenticator: Authenticator? = null
     private var certificateAlias: String? = null
+
     fun authenticate(host: String?, getCredentials: () -> Credentials, updateAuthState: ((AuthState) -> Unit)? = null): HttpClientBuilder {
         val credentials = getCredentials()
         if (credentials.authState != null) {
@@ -130,6 +137,7 @@ class HttpClientBuilder @Inject constructor(
     }
 
     private var followRedirects = false
+
     fun followRedirects(follow: Boolean): HttpClientBuilder {
         followRedirects = follow
         return this
@@ -224,7 +232,8 @@ class HttpClientBuilder @Inject constructor(
         // app-wide custom proxy support
         buildProxy(okBuilder)
 
-        // add authentication
+        // add connection security (including client certificates) and authentication
+        buildConnectionSecurity(okBuilder)
         buildAuthentication(okBuilder)
 
         // add network logging, if requested
@@ -246,15 +255,17 @@ class HttpClientBuilder @Inject constructor(
         // basic/digest auth and OAuth
         authenticationInterceptor?.let { okBuilder.addInterceptor(it) }
         authenticator?.let { okBuilder.authenticator(it) }
+    }
 
+    private fun buildConnectionSecurity(okBuilder: OkHttpClient.Builder) {
         // client certificate
-        val keyManager: KeyManager? = certificateAlias?.let { alias ->
+        val clientKeyManager: KeyManager? = certificateAlias?.let { alias ->
             try {
                 val manager = keyManagerFactory.create(alias)
                 logger.fine("Using certificate $alias for authentication")
 
                 // HTTP/2 doesn't support client certificates (yet)
-                // see https://tools.ietf.org/html/draft-ietf-httpbis-http2-secondary-certs-04
+                // see https://datatracker.ietf.org/doc/draft-ietf-httpbis-secondary-server-certs/
                 okBuilder.protocols(listOf(Protocol.HTTP_1_1))
 
                 manager
@@ -264,25 +275,49 @@ class HttpClientBuilder @Inject constructor(
             }
         }
 
-        // cert4android integration
-        val certManager = CustomCertManager(
-            context = context,
-            trustSystemCerts = !settingsManager.getBoolean(Settings.DISTRUST_SYSTEM_CERTIFICATES),
-            appInForeground = if (BuildConfig.customCertsUI)
-                ForegroundTracker.inForeground  // interactive mode
-            else
-                null                            // non-interactive mode
-        )
+        // select trust manager and hostname verifier depending on whether custom certificates are allowed
+        val customTrustManager: X509TrustManager?
+        val customHostnameVerifier: HostnameVerifier?
 
-        val sslContext = SSLContext.getInstance("TLS")
-        sslContext.init(
-            /* km = */ if (keyManager != null) arrayOf(keyManager) else null,
-            /* tm = */ arrayOf(certManager),
-            /* random = */ null
-        )
-        okBuilder
-            .sslSocketFactory(sslContext.socketFactory, certManager)
-            .hostnameVerifier(certManager.HostnameVerifier(OkHostnameVerifier))
+        if (BuildConfig.allowCustomCerts) {
+            // use cert4android for custom certificate handling
+            customTrustManager = CustomCertManager(
+                context = 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()
     }
 
     private fun buildProxy(okBuilder: OkHttpClient.Builder) {

app/src/main/kotlin/at/bitfire/davdroid/ui/AppSettingsScreen.kt

@@ -67,6 +67,7 @@ import androidx.compose.ui.unit.dp
 import androidx.core.graphics.drawable.toBitmap
 import androidx.lifecycle.compose.collectAsStateWithLifecycle
 import androidx.lifecycle.viewmodel.compose.viewModel
+import at.bitfire.davdroid.BuildConfig
 import at.bitfire.davdroid.R
 import at.bitfire.davdroid.settings.Settings
 import at.bitfire.davdroid.ui.AppSettingsModel.PushDistributorInfo
@@ -107,10 +108,11 @@ fun AppSettingsScreen(
             onProxyPortUpdated = model::updateProxyPort,
 
             // Security
+            onNavPermissionsScreen = onNavPermissionsScreen,
+            showCertSettings = BuildConfig.allowCustomCerts,
             distrustSystemCerts = model.distrustSystemCertificates().collectAsStateWithLifecycle(null).value ?: false,
             onDistrustSystemCertsUpdated = model::updateDistrustSystemCertificates,
             onResetCertificates = model::resetCertificates,
-            onNavPermissionsScreen = onNavPermissionsScreen,
 
             // User interface
             onShowNotificationSettings = onShowNotificationSettings,
@@ -149,10 +151,11 @@ fun AppSettingsScreen(
     onProxyPortUpdated: (Int) -> Unit,
 
     // AppSettings security
+    onNavPermissionsScreen: () -> Unit,
+    showCertSettings: Boolean,
     distrustSystemCerts: Boolean,
     onDistrustSystemCertsUpdated: (Boolean) -> Unit,
     onResetCertificates: () -> Unit,
-    onNavPermissionsScreen: () -> Unit,
 
     // AppSettings UserInterface
     theme: Int,
@@ -224,6 +227,8 @@ fun AppSettingsScreen(
 
                 val resetCertificatesSuccessMessage = stringResource(R.string.app_settings_reset_certificates_success)
                 AppSettings_Security(
+                    onNavPermissionsScreen = onNavPermissionsScreen,
+                    showCertSettings = showCertSettings,
                     distrustSystemCerts = distrustSystemCerts,
                     onDistrustSystemCertsUpdated = onDistrustSystemCertsUpdated,
                     onResetCertificates = {
@@ -231,8 +236,7 @@ fun AppSettingsScreen(
                         coroutineScope.launch {
                             snackbarHostState.showSnackbar(resetCertificatesSuccessMessage)
                         }
-                    },
-                    onNavPermissionsScreen = onNavPermissionsScreen
+                    }
                 )
 
                 val resetHintsSuccessMessage = stringResource(R.string.app_settings_reset_hints_success)
@@ -282,9 +286,10 @@ fun AppSettingsScreen_Preview() {
             onNavUp = {},
             onProxyTypeUpdated = {},
             onProxyPortUpdated = {},
+            onNavPermissionsScreen = {},
+            showCertSettings = true,
             onDistrustSystemCertsUpdated = {},
             onResetCertificates = {},
-            onNavPermissionsScreen = {},
             onThemeSelected = {},
             onResetHints = {},
             tasksAppName = "No tasks app",
@@ -420,48 +425,51 @@ fun AppSettings_Connection(
 
 @Composable
 fun AppSettings_Security(
+    onNavPermissionsScreen: () -> Unit,
+    showCertSettings: Boolean,
     distrustSystemCerts: Boolean,
     onDistrustSystemCertsUpdated: (Boolean) -> Unit,
-    onResetCertificates: () -> Unit,
-    onNavPermissionsScreen: () -> Unit
+    onResetCertificates: () -> Unit
 ) {
     SettingsHeader(divider = true) {
         Text(stringResource(R.string.app_settings_security))
     }
 
-    var showingDistrustWarning by remember { mutableStateOf(false) }
-    if (showingDistrustWarning) {
-        DistrustSystemCertificatesAlertDialog(
-            onDistrustSystemCertsRequested = { onDistrustSystemCertsUpdated(true) },
-            onDismissRequested = { showingDistrustWarning = false }
-        )
-    }
-
-    SwitchSetting(
-        checked = distrustSystemCerts,
-        name = stringResource(R.string.app_settings_distrust_system_certs),
-        summaryOn = stringResource(R.string.app_settings_distrust_system_certs_on),
-        summaryOff = stringResource(R.string.app_settings_distrust_system_certs_off)
-    ) { checked ->
-        if (checked) {
-            // Show warning before enabling.
-            showingDistrustWarning = true
-        } else {
-            onDistrustSystemCertsUpdated(false)
-        }
-    }
-
-    Setting(
-        name = stringResource(R.string.app_settings_reset_certificates),
-        summary = stringResource(R.string.app_settings_reset_certificates_summary),
-        onClick = onResetCertificates
-    )
-
     Setting(
         name = stringResource(R.string.app_settings_security_app_permissions),
         summary = stringResource(R.string.app_settings_security_app_permissions_summary),
         onClick = onNavPermissionsScreen
     )
+
+    if (showCertSettings) {
+        var showingDistrustWarning by remember { mutableStateOf(false) }
+        if (showingDistrustWarning) {
+            DistrustSystemCertificatesAlertDialog(
+                onDistrustSystemCertsRequested = { onDistrustSystemCertsUpdated(true) },
+                onDismissRequested = { showingDistrustWarning = false }
+            )
+        }
+
+        SwitchSetting(
+            checked = distrustSystemCerts,
+            name = stringResource(R.string.app_settings_distrust_system_certs),
+            summaryOn = stringResource(R.string.app_settings_distrust_system_certs_on),
+            summaryOff = stringResource(R.string.app_settings_distrust_system_certs_off)
+        ) { checked ->
+            if (checked) {
+                // Show warning before enabling.
+                showingDistrustWarning = true
+            } else {
+                onDistrustSystemCertsUpdated(false)
+            }
+        }
+
+        Setting(
+            name = stringResource(R.string.app_settings_reset_certificates),
+            summary = stringResource(R.string.app_settings_reset_certificates_summary),
+            onClick = onResetCertificates
+        )
+    }
 }
 
 @Composable