diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt index 9ab6c01..64eae5a 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/AccountRepository.kt @@ -17,6 +17,7 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import java.io.IOException import javax.inject.Inject import javax.inject.Singleton @@ -193,7 +194,15 @@ class AccountRepository @Inject constructor( inserted += attach(id, selected) id } - serverTrust.put(accountId, trust) + // Inside the rollback's reach: a DataStore failure here would otherwise + // leave the row with no credential, the hazard the comment above names. + if (!saveTrust(accountId, trust)) { + rollback(accountId, inserted) + return@withContext Outcome.CredentialFailed( + Outcome.Cause.NOT_SAVED, + "the server trust could not be stored", + ) + } if (!credentials.put(accountId, appPassword)) { // Never leave a half-made account behind: without a credential it @@ -323,7 +332,8 @@ class AccountRepository @Inject constructor( -> Collections.NeedsSignIn is CalDavDiscovery.Outcome.NotCalDav -> Collections.Failed(outcome.cause) is CalDavDiscovery.Outcome.Failed -> Collections.Failed(outcome.cause) - // A certificate that changed since it was trusted; signing in again re-asks. + // A certificate that changed since it was trusted. The account offers + // "Sign in again" for a certificate failure, and that flow re-asks. is CalDavDiscovery.Outcome.UntrustedCertificate -> Collections.Failed(CalDavDiscovery.Outcome.Cause.TLS) } @@ -415,6 +425,11 @@ class AccountRepository @Inject constructor( "the device keystore would not store the password", ) } + // Before anything else changes, so a failure leaves the account stopped + // and the sign-in retryable rather than signed in with the old trust. + if (!withContext(NonCancellable) { saveTrust(accountId, trust) }) { + return Outcome.CredentialFailed(Outcome.Cause.NOT_SAVED, "the server trust could not be stored") + } withContext(NonCancellable) { database.runInTransaction { database.accounts().update( @@ -429,7 +444,6 @@ class AccountRepository @Inject constructor( ) attach(accountId, selected) } - serverTrust.put(accountId, trust) // ⚠️ Ticked lists are attached; unticked ones are left alone. The // picker pre-ticks everything *writable*, not everything already // attached, so detaching what is unticked would silently stop @@ -464,9 +478,18 @@ class AccountRepository @Inject constructor( inserted.forEach { database.taskLists().delete(it) } database.accounts().delete(accountId) } - serverTrust.forget(accountId) + // Ids are never reused, and a later create overwrites the keys anyway. + runCatching { serverTrust.forget(accountId) } } + private suspend fun saveTrust(accountId: Long, trust: ServerTrust): Boolean = + try { + serverTrust.put(accountId, trust) + true + } catch (_: IOException) { + false + } + /** * Puts every existing account back on the periodic schedule. * diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt index efcf17c..8befbb8 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/PendingLoginFlowStore.kt @@ -2,11 +2,9 @@ package de.jeanlucmakiola.agendula.data.sync import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences -import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey -import androidx.datastore.preferences.core.stringSetPreferencesKey import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore import de.jeanlucmakiola.caldav.NextcloudLoginFlow import de.jeanlucmakiola.caldav.ServerTrust @@ -67,8 +65,7 @@ class PendingLoginFlowStore @Inject constructor( prefs[POLL_ENDPOINT] = flow.pollEndpoint.toString() prefs[POLL_TOKEN] = flow.pollToken prefs[DEADLINE] = flow.deadlineEpochSeconds - prefs[TRUSTED_CERTIFICATES] = trust.pinnedCertificates - prefs[CLEARTEXT] = trust.allowCleartext + TRUST.write(prefs, trust) } } @@ -79,8 +76,7 @@ class PendingLoginFlowStore @Inject constructor( prefs.remove(POLL_ENDPOINT) prefs.remove(POLL_TOKEN) prefs.remove(DEADLINE) - prefs.remove(TRUSTED_CERTIFICATES) - prefs.remove(CLEARTEXT) + TRUST.remove(prefs) } } @@ -145,20 +141,13 @@ class PendingLoginFlowStore @Inject constructor( ) } - private suspend fun trust(): ServerTrust { - val prefs = dataStore.data.first() - return ServerTrust( - pinnedCertificates = prefs[TRUSTED_CERTIFICATES].orEmpty(), - allowCleartext = prefs[CLEARTEXT] ?: false, - ) - } + private suspend fun trust(): ServerTrust = TRUST.read(dataStore.data.first()) private companion object { val LOGIN_URL = stringPreferencesKey("login_flow_url") val POLL_ENDPOINT = stringPreferencesKey("login_flow_poll_endpoint") val POLL_TOKEN = stringPreferencesKey("login_flow_poll_token") val DEADLINE = longPreferencesKey("login_flow_deadline") - val TRUSTED_CERTIFICATES = stringSetPreferencesKey("login_flow_trusted_certificates") - val CLEARTEXT = booleanPreferencesKey("login_flow_cleartext") + val TRUST = ServerTrustKeys("login_flow_trusted_certificates", "login_flow_cleartext") } } diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt new file mode 100644 index 0000000..1a574a7 --- /dev/null +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustKeys.kt @@ -0,0 +1,44 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.preferences.core.MutablePreferences +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey +import androidx.datastore.preferences.core.stringSetPreferencesKey +import androidx.lifecycle.SavedStateHandle +import de.jeanlucmakiola.caldav.ServerTrust + +/** + * The one mapping between a [ServerTrust] and the key-value stores it is kept + * in, so a field added to it cannot be dropped by one of them. + */ +class ServerTrustKeys(private val certificatesName: String, private val cleartextName: String) { + + private val certificates = stringSetPreferencesKey(certificatesName) + private val cleartext = booleanPreferencesKey(cleartextName) + + fun read(prefs: Preferences) = ServerTrust( + pinnedCertificates = prefs[certificates].orEmpty(), + allowCleartext = prefs[cleartext] ?: false, + ) + + fun write(prefs: MutablePreferences, trust: ServerTrust) { + if (trust.pinnedCertificates.isEmpty()) { + prefs.remove(certificates) + } else { + prefs[certificates] = trust.pinnedCertificates + } + if (trust.allowCleartext) prefs[cleartext] = true else prefs.remove(cleartext) + } + + fun remove(prefs: MutablePreferences) = write(prefs, ServerTrust.DEFAULT) + + fun read(handle: SavedStateHandle) = ServerTrust( + pinnedCertificates = handle.get>(certificatesName).orEmpty().toSet(), + allowCleartext = handle[cleartextName] ?: false, + ) + + fun write(handle: SavedStateHandle, trust: ServerTrust) { + handle[certificatesName] = ArrayList(trust.pinnedCertificates) + handle[cleartextName] = trust.allowCleartext + } +} diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt index 95efa49..364d759 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/data/sync/ServerTrustStore.kt @@ -2,9 +2,7 @@ package de.jeanlucmakiola.agendula.data.sync import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences -import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit -import androidx.datastore.preferences.core.stringSetPreferencesKey import de.jeanlucmakiola.agendula.data.di.SyncStateDataStore import de.jeanlucmakiola.caldav.ServerTrust import kotlinx.coroutines.flow.Flow @@ -26,29 +24,15 @@ class ServerTrustStore @Inject constructor( dataStore.data.map { read(it, accountId) }.distinctUntilChanged() suspend fun put(accountId: Long, trust: ServerTrust) { - dataStore.edit { prefs -> - if (trust.pinnedCertificates.isEmpty()) { - prefs.remove(certificates(accountId)) - } else { - prefs[certificates(accountId)] = trust.pinnedCertificates - } - if (trust.allowCleartext) prefs[cleartext(accountId)] = true else prefs.remove(cleartext(accountId)) - } + dataStore.edit { keys(accountId).write(it, trust) } } suspend fun forget(accountId: Long) { - dataStore.edit { prefs -> - prefs.remove(certificates(accountId)) - prefs.remove(cleartext(accountId)) - } + dataStore.edit { keys(accountId).remove(it) } } - private fun read(prefs: Preferences, accountId: Long) = ServerTrust( - pinnedCertificates = prefs[certificates(accountId)].orEmpty(), - allowCleartext = prefs[cleartext(accountId)] ?: false, - ) + private fun read(prefs: Preferences, accountId: Long) = keys(accountId).read(prefs) - private fun certificates(accountId: Long) = stringSetPreferencesKey("server_trust_certs_$accountId") - - private fun cleartext(accountId: Long) = booleanPreferencesKey("server_trust_cleartext_$accountId") + private fun keys(accountId: Long) = + ServerTrustKeys("server_trust_certs_$accountId", "server_trust_cleartext_$accountId") } diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt index 1d04595..36e815a 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountDetailScreen.kt @@ -169,7 +169,7 @@ internal fun AccountDetailScreen( ) } } - if (shown.needsSignIn) { + if (shown.offersSignIn) { add { position -> GroupedRow( title = stringResource(R.string.accounts_sign_in_again), @@ -178,7 +178,8 @@ internal fun AccountDetailScreen( onClick = { onSignInAgain(account) }, ) } - } else { + } + if (!shown.needsSignIn) { add { position -> GroupedRow( title = stringResource(R.string.accounts_sync_now), diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt index a4467d5..f6567a5 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt @@ -119,7 +119,7 @@ internal fun AccountsScreen( when { row.removing -> CircularProgressIndicator(Modifier.size(20.dp)) !usable -> Unit - row.needsSignIn -> IconButton(onClick = { onSignInAgain(account) }) { + row.offersSignIn -> IconButton(onClick = { onSignInAgain(account) }) { Icon( Icons.AutoMirrored.Rounded.Login, contentDescription = stringResource(R.string.accounts_sign_in_again), diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt index ff6c475..6455e85 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt @@ -10,6 +10,7 @@ import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.AccountStateStore import de.jeanlucmakiola.agendula.data.sync.ServerTrustStore import de.jeanlucmakiola.agendula.data.sync.SyncAvailability +import de.jeanlucmakiola.agendula.data.sync.SyncFailure import de.jeanlucmakiola.agendula.data.sync.SyncNotice import de.jeanlucmakiola.agendula.data.sync.SyncNoticeStore import de.jeanlucmakiola.agendula.data.sync.SyncTrigger @@ -191,7 +192,18 @@ class AccountsViewModel @Inject constructor( */ val notices: List = emptyList(), val lists: ListPush = ListPush.NONE, - ) + ) { + /** + * The last sync was refused over the server's certificate — typically a + * pinned one that was renewed. Nothing sets [needsSignIn] for that, yet + * signing in again is the only route that can trust the new one. + */ + val certificateRefused: Boolean + get() = account.lastSyncError?.let { SyncFailure.of(it).kind == SyncFailure.Kind.CERTIFICATE } == true + + /** Whether "Sign in again" is the way forward for this account. */ + val offersSignIn: Boolean get() = needsSignIn || certificateRefused + } /** * Accounts whose removal has started and not finished. diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt index 4b96efb..5dfc4ae 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountScreen.kt @@ -440,11 +440,19 @@ private fun ColumnScope.StepActions(state: AddAccountUiState, viewModel: AddAcco ) { Text(stringResource(R.string.add_account_change_address)) } } - is AddAccountStep.TrustCertificate -> PrimaryAction( - label = stringResource(R.string.add_account_certificate_trust), - enabled = true, - onClick = viewModel::onCertificateTrusted, - ) + // The safe answer stays visible, as it does for plain http: back is + // not something every user knows to look for on a security prompt. + is AddAccountStep.TrustCertificate -> { + PrimaryAction( + label = stringResource(R.string.add_account_certificate_trust), + enabled = true, + onClick = viewModel::onCertificateTrusted, + ) + TextButton( + onClick = { viewModel.onBackWithin() }, + modifier = Modifier.fillMaxWidth(), + ) { Text(stringResource(R.string.add_account_certificate_decline)) } + } is AddAccountStep.WaitingForBrowser -> { // A failed flow is retried by default. With no browser a retry diff --git a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt index 300de82..95bff6d 100644 --- a/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt +++ b/app/src/full/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModel.kt @@ -8,6 +8,7 @@ import de.jeanlucmakiola.agendula.data.sync.AccountCreator import de.jeanlucmakiola.agendula.data.sync.AccountRepository import de.jeanlucmakiola.agendula.data.sync.CalDavGateway import de.jeanlucmakiola.agendula.data.sync.LoginFlowRecord +import de.jeanlucmakiola.agendula.data.sync.ServerTrustKeys import de.jeanlucmakiola.caldav.CalDavDiscovery import de.jeanlucmakiola.caldav.CalDavProvider import de.jeanlucmakiola.caldav.NextcloudLoginFlow @@ -17,6 +18,8 @@ import de.jeanlucmakiola.caldav.ServerTrust import de.jeanlucmakiola.caldav.ServiceDiscovery import de.jeanlucmakiola.caldav.TaskCollection import de.jeanlucmakiola.agendula.data.di.ApplicationScope +import kotlinx.coroutines.CancellationException +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.NonCancellable import kotlinx.coroutines.withContext @@ -337,26 +340,33 @@ class AddAccountViewModel @Inject constructor( /** What the user agreed to trust in this flow, handed to every gateway call. */ private var trust: ServerTrust - get() = ServerTrust( - pinnedCertificates = handle.get>(KEY_TRUSTED_CERTIFICATES).orEmpty().toSet(), - allowCleartext = handle[KEY_CLEARTEXT] ?: false, - ) - set(value) { - handle[KEY_TRUSTED_CERTIFICATES] = ArrayList(value.pinnedCertificates) - handle[KEY_CLEARTEXT] = value.allowCleartext - } + get() = TRUST_KEYS.read(handle) + set(value) = TRUST_KEYS.write(handle, value) - /** The address plain http was accepted for; another address asks again. */ - private var cleartextAddress: String? - get() = handle[KEY_CLEARTEXT_ADDRESS] - set(value) { handle[KEY_CLEARTEXT_ADDRESS] = value } + /** + * The address [trust] was given for. Pins and plain http both belong to it, + * so submitting any other address starts from nothing. + */ + private var trustAddress: String? + get() = handle[KEY_TRUST_ADDRESS] + set(value) { handle[KEY_TRUST_ADDRESS] = value } + + /** + * The pins in [trust] came from the account being signed in to, not from + * this flow. The first certificate trusted now replaces them, so a renewed + * certificate retires the one it renews instead of joining it. + */ + private var inheritedPins: Boolean + get() = handle[KEY_INHERITED_PINS] ?: false + set(value) { handle[KEY_INHERITED_PINS] = value } /** Loading a re-authenticated account's stored trust, which discovery waits for. */ private var trustLoad: Job? = null - /** What [AddAccountStep.TrustCertificate] re-runs once trusted, and where Back goes. */ - private var trustRetry: (() -> Unit)? = null - private var trustDeclined: AddAccountStep? = null + /** The open [AddAccountStep.TrustCertificate] question, and where declining it goes. */ + private class TrustPrompt(val answer: CompletableDeferred, val back: AddAccountStep) + + private var trustPrompt: TrustPrompt? = null /** Which hosts asked for credentials, for the cross-domain diagnostic. */ private var hostsNeedingAuth: List @@ -461,7 +471,8 @@ class AddAccountViewModel @Inject constructor( trustLoad = viewModelScope.launch { val stored = runCatching { repository.trustFor(accountId) }.getOrDefault(ServerTrust.DEFAULT) trust = stored - if (stored.allowCleartext) cleartextAddress = address + trustAddress = address + inheritedPins = stored.pinnedCertificates.isNotEmpty() } } @@ -521,9 +532,6 @@ class AddAccountViewModel @Inject constructor( updateState { it.copy(originMismatch = null) } typedInput = input addressInput = input - trustRetry = null - trustDeclined = null - if (input != cleartextAddress) trust = trust.copy(allowCleartext = false) val quirk = ServerQuirk.forInput(input) if (quirk?.isFatal == true) { @@ -547,8 +555,18 @@ class AddAccountViewModel @Inject constructor( private fun discoverAddress() { working(AddAccountMessage.Progress.Discovering) viewModelScope.launch { + // ⚠️ After the stored trust has landed, not before: reset first and + // the load would put the old address's trust back on top. trustLoad?.join() - when (val outcome = gateway.discover(typedInput, trust = trust)) { + if (typedInput != trustAddress) { + trust = ServerTrust.DEFAULT + inheritedPins = false + trustAddress = typedInput + } + val outcome = discoverTrusting(addressStep(typedInput), AddAccountMessage.Progress.Discovering) { + gateway.discover(typedInput, trust = trust) + } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) is CalDavDiscovery.Outcome.NeedsAuthentication -> { @@ -570,8 +588,8 @@ class AddAccountViewModel @Inject constructor( backToAddressOrServer(outcome.cause) } - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(typedInput)) { discoverAddress() } + // Asked about inside discoverTrusting, which only returns it declined. + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -584,29 +602,49 @@ class AddAccountViewModel @Inject constructor( /** The warning is read and plain http is wanted anyway. */ fun onCleartextConfirmed() { if (current.step !is AddAccountStep.ConfirmCleartext) return - cleartextAddress = typedInput trust = trust.copy(allowCleartext = true) + trustAddress = typedInput discoverAddress() } /** The fingerprint is checked; pin it and re-run what it stopped. */ fun onCertificateTrusted() { val step = current.step as? AddAccountStep.TrustCertificate ?: return - val retry = trustRetry ?: return - trust = trust.withCertificate(step.certificate.fingerprint) - trustRetry = null - trustDeclined = null - retry() + val prompt = trustPrompt ?: return + trustPrompt = null + val fingerprint = step.certificate.fingerprint + trust = if (inheritedPins) { + trust.copy(pinnedCertificates = setOf(fingerprint.lowercase())) + } else { + trust.withCertificate(fingerprint) + } + inheritedPins = false + prompt.answer.complete(Unit) } - private fun askToTrust(certificate: ServerCertificate, back: AddAccountStep, retry: () -> Unit) { - // Already pinned and still refused: asking again could only loop. - if (certificate.fingerprint in trust.pinnedCertificates) { - return backToAddress(CalDavDiscovery.Outcome.Cause.TLS) + /** + * Runs [discover], and on a certificate the user could trust asks about it + * and runs it again once they have. Null when they declined; the step is + * then already [back]. + */ + private suspend fun discoverTrusting( + back: AddAccountStep, + progress: AddAccountMessage.Progress, + discover: suspend () -> CalDavDiscovery.Outcome, + ): CalDavDiscovery.Outcome? { + while (true) { + val outcome = discover() + if (outcome !is CalDavDiscovery.Outcome.UntrustedCertificate) return outcome + val prompt = TrustPrompt(CompletableDeferred(), back) + trustPrompt = prompt + updateState { it.copy(step = AddAccountStep.TrustCertificate(outcome.certificate), openInBrowser = null) } + try { + prompt.answer.await() + } catch (_: CancellationException) { + return null + } + working(progress) } - trustRetry = retry - trustDeclined = back - updateState { it.copy(step = AddAccountStep.TrustCertificate(certificate), openInBrowser = null) } } /** @@ -663,7 +701,11 @@ class AddAccountViewModel @Inject constructor( val root = serverRoot ?: return@launch backToAddress(CalDavDiscovery.Outcome.Cause.NOT_AN_ADDRESS) val credentials = CalDavGateway.Credentials(username, appPassword, root) - when (val outcome = gateway.discover(typedInput, credentials, trust)) { + val outcome = discoverTrusting( + AddAccountStep.EnterCredentials(username = username), + AddAccountMessage.Progress.SigningIn, + ) { gateway.discover(typedInput, credentials, trust) } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) // A second rejection is the point at which naming the provider's @@ -685,10 +727,7 @@ class AddAccountViewModel @Inject constructor( is CalDavDiscovery.Outcome.NotCalDav -> backToAddressOrServer(outcome.cause) is CalDavDiscovery.Outcome.Failed -> backToAddressOrServer(outcome.cause) - is CalDavDiscovery.Outcome.UntrustedCertificate -> askToTrust( - outcome.certificate, - back = AddAccountStep.EnterCredentials(username = username), - ) { discoverWithCredentials() } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -872,15 +911,17 @@ class AddAccountViewModel @Inject constructor( /** Discovery with a password the login flow just minted, at the server that issued it. */ private suspend fun discoverApproved(server: HttpUrl) { working(AddAccountMessage.Progress.ReadingLists) - val outcome = gateway.discover( - // The server the credentials were *issued by*, not the - // host the user typed — the two differ in exactly the - // host-mismatch case this flow already models, and - // getting it wrong burns the one-shot app password. - target = server.toString(), - credentials = CalDavGateway.Credentials(username, appPassword, server), - trust = trust, - ) + val outcome = discoverTrusting(addressStep(typedInput), AddAccountMessage.Progress.ReadingLists) { + gateway.discover( + // The server the credentials were *issued by*, not the + // host the user typed — the two differ in exactly the + // host-mismatch case this flow already models, and + // getting it wrong burns the one-shot app password. + target = server.toString(), + credentials = CalDavGateway.Credentials(username, appPassword, server), + trust = trust, + ) + } ?: return when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome) // ⚠️ Forward what actually happened. Reporting every @@ -909,10 +950,7 @@ class AddAccountViewModel @Inject constructor( CalDavDiscovery.Outcome.Unauthenticated -> backToAddress(CalDavDiscovery.Outcome.Cause.SERVER_ERROR) - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(typedInput)) { - viewModelScope.launch { discoverApproved(server) } - } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } @@ -1112,10 +1150,10 @@ class AddAccountViewModel @Inject constructor( } is AddAccountStep.TrustCertificate -> { - val back = trustDeclined ?: addressStep(typedInput) - trustRetry = null - trustDeclined = null - updateState { it.copy(step = back, fatal = null) } + val prompt = trustPrompt + trustPrompt = null + prompt?.answer?.cancel() + updateState { it.copy(step = prompt?.back ?: addressStep(typedInput), fatal = null) } true } @@ -1163,9 +1201,10 @@ class AddAccountViewModel @Inject constructor( trustLoad?.cancel() trustLoad = null trust = ServerTrust.DEFAULT - cleartextAddress = null - trustRetry = null - trustDeclined = null + trustAddress = null + inheritedPins = false + trustPrompt?.answer?.cancel() + trustPrompt = null current = AddAccountUiState() } @@ -1324,7 +1363,10 @@ class AddAccountViewModel @Inject constructor( ) viewModelScope.launch { val credentials = CalDavGateway.Credentials(username, appPassword, root) - when (val outcome = gateway.discover(target, credentials, trust)) { + val outcome = discoverTrusting(addressStep(addressInput), AddAccountMessage.Progress.ReadingLists) { + gateway.discover(target, credentials, trust) + } ?: return@launch + when (outcome) { is CalDavDiscovery.Outcome.Found -> onDiscovered(outcome, selection) is CalDavDiscovery.Outcome.NotCalDav -> backToAddressOrServer(outcome.cause) is CalDavDiscovery.Outcome.Failed -> backToAddressOrServer(outcome.cause) @@ -1335,8 +1377,7 @@ class AddAccountViewModel @Inject constructor( credentialsRejected() } CalDavDiscovery.Outcome.Unauthenticated -> credentialsRejected() - is CalDavDiscovery.Outcome.UntrustedCertificate -> - askToTrust(outcome.certificate, back = addressStep(addressInput)) { resumeDiscovery(hasSetup) } + is CalDavDiscovery.Outcome.UntrustedCertificate -> Unit } } } @@ -1566,13 +1607,19 @@ class AddAccountViewModel @Inject constructor( const val KEY_FALLBACK_EMAIL = "add_account.fallback_email" const val KEY_TRUSTED_CERTIFICATES = "add_account.trusted_certificates" const val KEY_CLEARTEXT = "add_account.cleartext" - const val KEY_CLEARTEXT_ADDRESS = "add_account.cleartext_address" + const val KEY_TRUST_ADDRESS = "add_account.trust_address" + const val KEY_INHERITED_PINS = "add_account.inherited_pins" + + val TRUST_KEYS = ServerTrustKeys(KEY_TRUSTED_CERTIFICATES, KEY_CLEARTEXT) /** What sends an email to the server field: no CalDAV on its domain, or nothing answering for it. */ val NO_SERVER_FOUND = setOf( CalDavDiscovery.Outcome.Cause.NOT_A_DAV_SERVER, CalDavDiscovery.Outcome.Cause.NO_CALENDAR_SUPPORT, CalDavDiscovery.Outcome.Cause.UNREACHABLE, + // The domain's own host would not set up TLS: usually its website, + // not a CalDAV server, so the server field is the way on. + CalDavDiscovery.Outcome.Cause.TLS, ) /** diff --git a/app/src/full/res/xml/network_security_config.xml b/app/src/full/res/xml/network_security_config.xml index 15e9a90..4e0774f 100644 --- a/app/src/full/res/xml/network_security_config.xml +++ b/app/src/full/res/xml/network_security_config.xml @@ -11,6 +11,9 @@ itself refuses an http:// URL for everyone else. This file cannot express a per-account choice, only a per-host one, and the hosts are the user's. + ⚠️ That makes CalDavHttp the only gate. A client built any other way would + speak http silently, so HttpClientConstructionTest fails the build on one. + ⚠️ KNOWN TRADE-OFF, and it is the widest form of this. base-config applies to *all* traffic, so any CA in the user store — a corporate MDM profile, a "free VPN" app's certificate, one installed during some earlier debugging — can diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 96cac40..eb37419 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -832,6 +832,7 @@ Issued by Valid until Trust this certificate + Don\u2019t trust it Which lists should sync? Read-only diff --git a/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt b/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt new file mode 100644 index 0000000..79c1ecf --- /dev/null +++ b/app/src/testFull/java/de/jeanlucmakiola/agendula/data/sync/HttpClientConstructionTest.kt @@ -0,0 +1,41 @@ +package de.jeanlucmakiola.agendula.data.sync + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test +import java.io.File + +/** + * `network_security_config.xml` permits cleartext app-wide, and the per-account + * opt-in is enforced only by the clients `CalDavHttp` builds. Any HTTP client + * built elsewhere would speak plain http without asking. + */ +class HttpClientConstructionTest { + + private val forbidden = listOf( + "OkHttpClient()", + "OkHttpClient.Builder()", + "HttpURLConnection", + "HttpsURLConnection", + ".openConnection(", + ".openStream(", + ) + + @Test + fun `every HTTP client is built through CalDavHttp`() { + val roots = listOf(File("src"), File("../caldav/src/main")) + assertThat(roots.filter { it.isDirectory }).isNotEmpty() + + val offenders = roots + .filter { it.isDirectory } + .flatMap { root -> root.walk().filter { it.isFile && it.extension in setOf("kt", "java") }.toList() } + .filterNot { "/test" in it.invariantSeparatorsPath || "/androidTest" in it.invariantSeparatorsPath } + .filterNot { it.name == "CalDavHttp.kt" } + .flatMap { file -> + file.readLines().mapIndexedNotNull { index, line -> + forbidden.firstOrNull { it in line }?.let { "${file.path}:${index + 1}: $it" } + } + } + + assertThat(offenders).isEmpty() + } +} diff --git a/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt b/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt index 17803ea..622f82d 100644 --- a/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt +++ b/app/src/testFull/java/de/jeanlucmakiola/agendula/ui/accounts/add/AddAccountViewModelTest.kt @@ -1525,18 +1525,52 @@ class AddAccountViewModelTest { } @Test - fun `a certificate already pinned is not asked about again`() = runTest(dispatcher) { + fun `a certificate trusted for one address is not carried to another`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + val vm = viewModel() + vm.enterAddress("https://a.lan") + vm.onAddressSubmitted() + advanceUntilIdle() + vm.onCertificateTrusted() + advanceUntilIdle() + assertThat(vm.onBackWithin()).isTrue() + + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + vm.enterAddress("https://b.example.com") + vm.onAddressSubmitted() + advanceUntilIdle() + + assertThat(gateway.discoveries.last().trust).isEqualTo(ServerTrust.DEFAULT) + } + + @Test + fun `declining a certificate is offered without the back gesture`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) val vm = viewModel() vm.enterAddress("https://nas.home.arpa") vm.onAddressSubmitted() advanceUntilIdle() - vm.onCertificateTrusted() + + assertThat(vm.onBackWithin()).isTrue() + advanceUntilIdle() + assertThat((vm.state.value.step as AddAccountStep.EnterAddress).input).isEqualTo("https://nas.home.arpa") + assertThat(gateway.discoveries).hasSize(1) + } + + @Test + fun `an email whose domain will not set up TLS asks for the server`() = runTest(dispatcher) { + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed(CalDavDiscovery.Outcome.Cause.TLS, "tls") + val vm = viewModel() + vm.onProviderChosen(ProviderChoice.Service(CalDavProvider.MAILBOX_ORG)) + vm.onAddressChanged("me@mailbox.org") + vm.onAddressSubmitted() advanceUntilIdle() - assertThat((vm.state.value.step as AddAccountStep.EnterAddress).error) - .isEqualTo(CalDavDiscovery.Outcome.Cause.TLS) + val step = vm.state.value.step as AddAccountStep.EnterAddress + assertThat(step.manualServer).isTrue() + assertThat(step.error).isNull() } @Test @@ -1553,6 +1587,42 @@ class AddAccountViewModelTest { assertThat(gateway.discoveries.single().trust).isEqualTo(stored) } + @Test + fun `a stored trust that loads late is not applied to an edited address`() = runTest(dispatcher) { + creator.storedTrust[9] = ServerTrust(allowCleartext = true) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.Failed( + CalDavDiscovery.Outcome.Cause.INSECURE, + "http", + ) + val vm = viewModel() + vm.startReauthentication(9, "http://nas.local/dav/principals/me/", "me") + // Submitted before the stored trust has been read. + vm.onAddressChanged("http://other.local") + vm.onAddressSubmitted() + advanceUntilIdle() + + assertThat(gateway.discoveries.single().trust).isEqualTo(ServerTrust.DEFAULT) + assertThat(vm.state.value.step).isEqualTo(AddAccountStep.ConfirmCleartext("http://other.local")) + } + + @Test + fun `a renewed certificate replaces the one the account trusted`() = runTest(dispatcher) { + creator.storedTrust[9] = ServerTrust(pinnedCertificates = setOf("cd".repeat(32))) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) + gateway.discoveryOutcomes += CalDavDiscovery.Outcome.NeedsAuthentication(emptyList()) + gateway.loginFlow = null + val vm = viewModel() + vm.startReauthentication(9, "https://nas.home.arpa/dav/principals/me/", "me") + advanceUntilIdle() + vm.onAddressSubmitted() + advanceUntilIdle() + vm.onCertificateTrusted() + advanceUntilIdle() + + assertThat(gateway.discoveries.last().trust.pinnedCertificates) + .containsExactly(certificate.fingerprint) + } + @Test fun `starting over forgets what was trusted`() = runTest(dispatcher) { gateway.discoveryOutcomes += CalDavDiscovery.Outcome.UntrustedCertificate(certificate) diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 4eb5869..ec5e0e0 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -157,6 +157,11 @@ class CalDavDiscovery( * A 401 stops the walk immediately: it means we found a DAV server and simply * have no credentials for it, and continuing down the ladder would replace a * precise "sign in" with a vague "nothing found". + * + * An untrusted certificate does not: a lower-priority SRV target may answer + * with one the phone trusts, so it is only offered once the ladder is spent. + * On a guessed host — the email's own domain, with no SRV record — it is not + * offered at all, since that host is most likely the domain's website. */ fun discover(input: String): Outcome { ServiceDiscovery.asBaseUrl(input)?.let { typed -> @@ -174,17 +179,31 @@ class CalDavDiscovery( } var lastFailure: Outcome = Outcome.Failed(Outcome.Cause.UNREACHABLE, "no candidate answered") + var untrusted: Outcome.UntrustedCertificate? = null + // Same host, same certificate: the remaining rungs on it would only fail again. + val refusedOrigins = mutableSetOf() for (candidate in candidates) { + if (candidate.url.originKey() in refusedOrigins) continue when (val outcome = probe(candidate.url)) { is Outcome.Found, is Outcome.NeedsAuthentication, Outcome.Unauthenticated, - is Outcome.UntrustedCertificate, -> return outcome + is Outcome.UntrustedCertificate -> { + refusedOrigins += candidate.url.originKey() + if (candidate.guessed) { + lastFailure = Outcome.Failed( + Outcome.Cause.TLS, + "untrusted certificate on guessed host ${candidate.url.host}", + ) + } else if (untrusted == null) { + untrusted = outcome + } + } else -> lastFailure = outcome } } - return lastFailure + return untrusted ?: lastFailure } internal fun probe(base: HttpUrl): Outcome { @@ -354,6 +373,8 @@ class CalDavDiscovery( untrustedCertificateIn(error)?.let { Outcome.UntrustedCertificate(ServerCertificate.of(it)) } ?: Outcome.Failed(causeOf(error), error.message ?: error.toString()) + private fun HttpUrl.originKey() = "$scheme://$host:$port" + private fun isUnauthorized(error: Throwable): Boolean = error is at.bitfire.dav4jvm.exception.UnauthorizedException diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index c8ad5a6..9c0bdbd 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -9,7 +9,6 @@ import okhttp3.Response import okhttp3.ResponseBody.Companion.asResponseBody import okio.GzipSource import okio.buffer -import java.util.concurrent.ConcurrentHashMap import java.util.concurrent.TimeUnit import javax.net.ssl.SSLContext import javax.net.ssl.SSLSocketFactory @@ -138,7 +137,8 @@ object CalDavHttp { ) } - private class Tls(trust: ServerTrust) { + private class Tls(pins: Set) { + private val trust = ServerTrust(pinnedCertificates = pins) val trustManager = PinningTrustManager(trust) val socketFactory: SSLSocketFactory = SSLContext.getInstance("TLS") .apply { init(null, arrayOf(trustManager), null) } @@ -147,13 +147,21 @@ object CalDavHttp { } /** - * One socket factory per distinct trust, because OkHttp only pools a + * One socket factory per distinct pin set, because OkHttp only pools a * connection between clients whose factory and verifier are the same * instances — a fresh one per call would reopen TLS for every client. + * Keyed on the pins alone (cleartext has no bearing on TLS), and bounded, + * since every abandoned setup attempt leaves a pin set behind. */ - private val tlsCache = ConcurrentHashMap() + private val tlsCache = object : LinkedHashMap, Tls>(TLS_CACHE_SIZE, 0.75f, true) { + override fun removeEldestEntry(eldest: MutableMap.MutableEntry, Tls>) = size > TLS_CACHE_SIZE + } - private fun tlsFor(trust: ServerTrust): Tls = tlsCache.getOrPut(trust) { Tls(trust) } + private fun tlsFor(trust: ServerTrust): Tls = synchronized(tlsCache) { + tlsCache.getOrPut(trust.pinnedCertificates) { Tls(trust.pinnedCertificates) } + } + + private const val TLS_CACHE_SIZE = 16 /** * The two headers that decide whether ETags are usable and whether our bytes diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt index 4ed5a22..b6e1fc3 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ServiceDiscovery.kt @@ -51,8 +51,14 @@ object ServiceDiscovery { private const val SRV_SERVICE = "_caldavs._tcp" private const val WELL_KNOWN = "/.well-known/caldav" - /** A URL to probe, and where it came from — the "why" a failure report needs. */ - data class Candidate(val url: HttpUrl, val origin: String) + /** + * A URL to probe, and where it came from — the "why" a failure report needs. + * + * @property guessed the host is the email's domain taken on spec, with no + * SRV record naming it. Usually that is the domain's website, so its + * certificate is not one worth asking the user to trust. + */ + data class Candidate(val url: HttpUrl, val origin: String, val guessed: Boolean = false) /** * Hosts whose SRV record leads somewhere that is not a DAV server. @@ -149,16 +155,15 @@ object ServiceDiscovery { // explicit, which is the whole point of Posteo's 8443. val port = if (record.port == 443) "" else ":${record.port}" "https://$host$port" to "SRV $host$port" - }.ifEmpty { - listOf("https://$domain" to "domain as typed") } + val guessed = origins.isEmpty() - for ((origin, label) in origins) { + for ((origin, label) in origins.ifEmpty { listOf("https://$domain" to "domain as typed") }) { if (txtPath != null) { - add(candidates, origin, txtPath, "$label + TXT path=$txtPath") + add(candidates, origin, txtPath, "$label + TXT path=$txtPath", guessed) } - add(candidates, origin, WELL_KNOWN, "$label + .well-known") - add(candidates, origin, "/", "$label + root") + add(candidates, origin, WELL_KNOWN, "$label + .well-known", guessed) + add(candidates, origin, "/", "$label + root", guessed) } return candidates } @@ -181,9 +186,15 @@ object ServiceDiscovery { return url.topPrivateDomain() ?: url.host } - private fun add(into: MutableList, origin: String, path: String, label: String) { + private fun add( + into: MutableList, + origin: String, + path: String, + label: String, + guessed: Boolean = false, + ) { val url = (origin.trimEnd('/') + "/" + path.trimStart('/')).toHttpUrlOrNull() ?: return - if (into.none { it.url == url }) into += Candidate(url, label) + if (into.none { it.url == url }) into += Candidate(url, label, guessed) } /** diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt index 44b1202..9e2762f 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ServerTrustTest.kt @@ -12,6 +12,7 @@ import org.junit.Before import org.junit.Test import java.io.IOException import java.net.ConnectException +import java.net.InetAddress import java.net.UnknownServiceException import java.security.cert.CertificateException import javax.net.ssl.SSLContext @@ -160,4 +161,51 @@ class ServerTrustTest { assertThat(discovery.probe(server.url("/dav/"))) .isInstanceOf(CalDavDiscovery.Outcome.NeedsAuthentication::class.java) } + + @Test + fun `an untrusted SRV target does not stop the ladder before one that answers`() { + val loopback = InetAddress.getByName("127.0.0.1") + fun tlsServer(certificate: HeldCertificate) = MockWebServer().apply { + useHttps(HandshakeCertificates.Builder().heldCertificate(certificate).build().sslSocketFactory(), false) + start(loopback, 0) + } + val untrusted = tlsServer(selfSigned("127.0.0.1")) + val pinned = selfSigned("127.0.0.1") + val answering = tlsServer(pinned).apply { + enqueue(options()) + enqueue(MockResponse().setResponseCode(401)) + } + val dns = object : DnsResolver { + override fun lookup(name: String) = DnsRecords( + srv = listOf( + SrvRecord(priority = 0, weight = 0, port = untrusted.port, target = "127.0.0.1."), + SrvRecord(priority = 10, weight = 0, port = answering.port, target = "127.0.0.1."), + ), + txt = emptyList(), + ) + } + val trust = ServerTrust.DEFAULT.withCertificate(fingerprintOf(pinned.certificate)) + + try { + val outcome = CalDavDiscovery(CalDavHttp.anonymous("test", trust), dns).discover("me@127.0.0.1") + assertThat(outcome).isInstanceOf(CalDavDiscovery.Outcome.NeedsAuthentication::class.java) + } finally { + untrusted.shutdown() + answering.shutdown() + } + } + + @Test + fun `the email's own domain is a guess, and an SRV target is not`() { + assertThat(ServiceDiscovery.candidatesFor("me@example.com").all { it.guessed }).isTrue() + + val dns = object : DnsResolver { + override fun lookup(name: String) = DnsRecords( + srv = listOf(SrvRecord(priority = 0, weight = 0, port = 443, target = "dav.example.com.")), + txt = emptyList(), + ) + } + assertThat(ServiceDiscovery.candidatesFor("me@example.com", dns).none { it.guessed }).isTrue() + assertThat(ServiceDiscovery.candidatesFor("https://example.com").none { it.guessed }).isTrue() + } }