From 9c64f9c24c4b9679080d8dcba50ca1c4b5e80b42 Mon Sep 17 00:00:00 2001 From: Daniel Gultsch Date: Wed, 1 Mar 2023 22:02:04 +0100 Subject: [PATCH] add UI for certificate trust --- .../android/ui/NavControllers.java | 4 +- .../android/ui/activity/SetupActivity.java | 7 + .../setup/TrustCertificateFragment.java | 40 +++++ .../android/ui/model/SetupViewModel.java | 77 +++++++++- .../android/util/PendingItem.java | 9 +- .../android/xmpp/XmppConnection.java | 9 +- .../android/xmpp/manager/TrustManager.java | 14 +- .../res/layout/fragment_trust_certificate.xml | 142 ++++++++++++++++++ .../main/res/navigation/setup_navigation.xml | 13 ++ app/src/main/res/values/strings.xml | 4 + 10 files changed, 306 insertions(+), 13 deletions(-) create mode 100644 app/src/main/java/im/conversations/android/ui/fragment/setup/TrustCertificateFragment.java create mode 100644 app/src/main/res/layout/fragment_trust_certificate.xml diff --git a/app/src/main/java/im/conversations/android/ui/NavControllers.java b/app/src/main/java/im/conversations/android/ui/NavControllers.java index 4dc39e805..42778a0fc 100644 --- a/app/src/main/java/im/conversations/android/ui/NavControllers.java +++ b/app/src/main/java/im/conversations/android/ui/NavControllers.java @@ -1,8 +1,8 @@ package im.conversations.android.ui; import androidx.annotation.IdRes; -import androidx.appcompat.app.AppCompatActivity; import androidx.fragment.app.Fragment; +import androidx.fragment.app.FragmentActivity; import androidx.fragment.app.FragmentManager; import androidx.navigation.NavController; import androidx.navigation.fragment.NavHostFragment; @@ -12,7 +12,7 @@ public final class NavControllers { private NavControllers() {} public static NavController findNavController( - final AppCompatActivity activity, @IdRes int fragmentId) { + final FragmentActivity activity, @IdRes int fragmentId) { final FragmentManager fragmentManager = activity.getSupportFragmentManager(); final Fragment fragment = fragmentManager.findFragmentById(fragmentId); if (fragment instanceof NavHostFragment) { diff --git a/app/src/main/java/im/conversations/android/ui/activity/SetupActivity.java b/app/src/main/java/im/conversations/android/ui/activity/SetupActivity.java index 84b12bc33..dfa651abb 100644 --- a/app/src/main/java/im/conversations/android/ui/activity/SetupActivity.java +++ b/app/src/main/java/im/conversations/android/ui/activity/SetupActivity.java @@ -55,6 +55,13 @@ public class SetupActivity extends BaseActivity { case ENTER_HOSTNAME: navController.navigate(SetupNavigationDirections.enterHostname()); break; + case TRUST_CERTIFICATE: + final var currentDestination = navController.getCurrentDestination(); + if (currentDestination == null + || currentDestination.getId() != R.id.certificate) { + navController.navigate(SetupNavigationDirections.trustCertificate()); + } + break; case DONE: startActivity(new Intent(this, MainActivity.class)); finish(); diff --git a/app/src/main/java/im/conversations/android/ui/fragment/setup/TrustCertificateFragment.java b/app/src/main/java/im/conversations/android/ui/fragment/setup/TrustCertificateFragment.java new file mode 100644 index 000000000..8297f468f --- /dev/null +++ b/app/src/main/java/im/conversations/android/ui/fragment/setup/TrustCertificateFragment.java @@ -0,0 +1,40 @@ +package im.conversations.android.ui.fragment.setup; + +import android.os.Bundle; +import android.view.LayoutInflater; +import android.view.View; +import android.view.ViewGroup; +import androidx.activity.OnBackPressedCallback; +import androidx.annotation.NonNull; +import androidx.databinding.DataBindingUtil; +import im.conversations.android.R; +import im.conversations.android.databinding.FragmentTrustCertificateBinding; +import im.conversations.android.ui.NavControllers; + +public class TrustCertificateFragment extends AbstractSetupFragment { + + @Override + public View onCreateView( + @NonNull LayoutInflater inflater, ViewGroup container, Bundle savedInstanceState) { + super.onCreateView(inflater, container, savedInstanceState); + FragmentTrustCertificateBinding binding = + DataBindingUtil.inflate( + inflater, R.layout.fragment_trust_certificate, container, false); + binding.setSetupViewModel(setupViewModel); + binding.setLifecycleOwner(getViewLifecycleOwner()); + requireActivity() + .getOnBackPressedDispatcher() + .addCallback( + getViewLifecycleOwner(), + new OnBackPressedCallback(true) { + @Override + public void handleOnBackPressed() { + setupViewModel.rejectTrustDecision(); + NavControllers.findNavController( + requireActivity(), R.id.nav_host_fragment) + .navigateUp(); + } + }); + return binding.getRoot(); + } +} diff --git a/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java b/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java index 228fb7b8e..3f7a21c18 100644 --- a/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java +++ b/app/src/main/java/im/conversations/android/ui/model/SetupViewModel.java @@ -26,8 +26,11 @@ import im.conversations.android.xmpp.ConnectionPool; import im.conversations.android.xmpp.ConnectionState; import im.conversations.android.xmpp.XmppConnection; import im.conversations.android.xmpp.manager.TrustManager; +import java.nio.ByteBuffer; import java.util.Arrays; +import java.util.HashMap; import java.util.Locale; +import java.util.concurrent.CancellationException; import java.util.function.Function; import org.jetbrains.annotations.NotNull; import org.jxmpp.jid.BareJid; @@ -56,13 +59,24 @@ public class SetupViewModel extends AndroidViewModel { private final MutableLiveData> redirection = new MutableLiveData<>(); private final MutableLiveData trustDecision = new MutableLiveData<>(); + private final HashMap trustDecisions = new HashMap<>(); private final Function> trustDecisionCallback = fingerprint -> { + final var decision = this.trustDecisions.get(ByteBuffer.wrap(fingerprint)); + if (decision != null) { + LOGGER.info("Using previous trust decision ({})", decision); + return Futures.immediateFuture(decision); + } + LOGGER.info("Trust decision arrived in UI"); final SettableFuture settableFuture = SettableFuture.create(); final var trustDecision = new TrustDecision(fingerprint, settableFuture); - LOGGER.debug("posting trust decision"); + final var currentOperation = this.currentOperation; + if (currentOperation != null) { + currentOperation.cancel(false); + } this.trustDecision.postValue(trustDecision); + this.redirection.postValue(new Event<>(Target.TRUST_CERTIFICATE)); return settableFuture; }; @@ -143,7 +157,7 @@ public class SetupViewModel extends AndroidViewModel { this.xmppAddressError.postValue(getApplication().getString(R.string.invalid_jid)); return true; } - + this.trustDecisions.clear(); if (account != null) { if (account.address.equals(address)) { this.accountRepository.reconnect(account); @@ -167,6 +181,51 @@ public class SetupViewModel extends AndroidViewModel { return true; } + public boolean trustCertificate() { + final var trustDecision = this.trustDecision.getValue(); + final var account = this.account; + if (trustDecision == null || account == null) { + // TODO navigate back to sign in or show error? + return true; + } + LOGGER.info( + "trying to commit trust for fingerprint {}", + TrustManager.fingerprint(trustDecision.fingerprint)); + // in case the UI interface hook gets called again before this gets written to DB + this.trustDecisions.put(ByteBuffer.wrap(trustDecision.fingerprint), true); + if (trustDecision.decision.isDone()) { + ConnectionPool.getInstance(getApplication()).reconnect(account); + LOGGER.info("it was already done. we should reconnect"); + } + trustDecision.decision.set(true); + decideNextStep(Target.TRUST_CERTIFICATE, account); + return true; + } + + public void rejectTrustDecision() { + final var trustDecision = this.trustDecision.getValue(); + if (trustDecision == null) { + return; + } + LOGGER.info( + "Rejecting trust decision for {}", + TrustManager.fingerprint(trustDecision.fingerprint)); + trustDecision.decision.set(false); + this.trustDecisions.put(ByteBuffer.wrap(trustDecision.fingerprint), false); + } + + public LiveData getFingerprint() { + return Transformations.map( + this.trustDecision, + td -> { + if (td == null) { + return null; + } else { + return TrustManager.fingerprint(td.fingerprint, 8); + } + }); + } + private void createAccount(final BareJid address) { // if the user hasn't entered anything we want this to be null so we don't store credentials @@ -229,6 +288,8 @@ public class SetupViewModel extends AndroidViewModel { final var optionalTrustManager = getTrustManager(); if (optionalTrustManager.isPresent()) { optionalTrustManager.get().removeUserInterfaceCallback(this.trustDecisionCallback); + } else { + LOGGER.warn("No trust manager found"); } } @@ -243,6 +304,7 @@ public class SetupViewModel extends AndroidViewModel { public void onSuccess(final XmppConnection result) { // TODO only when configured for loginAndBind LOGGER.info("Account setup successful"); + unregisterTrustDecisionCallback(); SetupViewModel.this.account = null; redirect(Target.DONE); } @@ -250,6 +312,10 @@ public class SetupViewModel extends AndroidViewModel { @Override public void onFailure(@NonNull final Throwable throwable) { loading.postValue(false); + if (throwable instanceof CancellationException) { + LOGGER.info("connection future was cancelled"); + return; + } if (throwable instanceof ConnectionException) { decideNextStep(current, ((ConnectionException) throwable)); } else { @@ -317,6 +383,7 @@ public class SetupViewModel extends AndroidViewModel { this.portError.postValue(getApplication().getString(R.string.invalid)); return true; } + this.trustDecisions.clear(); final boolean directTls = Boolean.FALSE.equals(this.opportunisticTls.getValue()); final var connection = new Connection(hostname, port, directTls); final var setConnectionFuture = @@ -388,15 +455,19 @@ public class SetupViewModel extends AndroidViewModel { return this.redirection; } + @Override public void onCleared() { - super.onCleared(); + LOGGER.info("Clearing view model"); this.unregisterTrustDecisionCallback(); + super.onCleared(); } public enum Target { ENTER_ADDRESS, ENTER_PASSWORD, ENTER_HOSTNAME, + + TRUST_CERTIFICATE, DONE } diff --git a/app/src/main/java/im/conversations/android/util/PendingItem.java b/app/src/main/java/im/conversations/android/util/PendingItem.java index 0a5d94f13..4eba1b892 100644 --- a/app/src/main/java/im/conversations/android/util/PendingItem.java +++ b/app/src/main/java/im/conversations/android/util/PendingItem.java @@ -29,7 +29,7 @@ package im.conversations.android.util; -import java.util.function.Supplier; +import java.util.function.Function; public class PendingItem { @@ -49,10 +49,9 @@ public class PendingItem { return item; } - public synchronized T peekOrCreate(final Supplier supplier) { - if (this.item == null) { - this.item = supplier.get(); - } + public synchronized T peekOrSwap(final Function swap) { + final T item = this.item; + this.item = swap.apply(item); return this.item; } diff --git a/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java b/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java index 2ec6aa226..fe8eb8f2d 100644 --- a/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java +++ b/app/src/main/java/im/conversations/android/xmpp/XmppConnection.java @@ -1882,7 +1882,14 @@ public class XmppConnection implements Runnable { } else if (Arrays.asList(ConnectionState.OFFLINE, ConnectionState.CONNECTING) .contains(state) || waitOnError) { - return this.connectedFuture.peekOrCreate(SettableFuture::create); + return this.connectedFuture.peekOrSwap( + f -> { + if (f == null || f.isDone()) { + return SettableFuture.create(); + } else { + return f; + } + }); } else { return Futures.immediateFailedFuture(new ConnectionException(state)); } diff --git a/app/src/main/java/im/conversations/android/xmpp/manager/TrustManager.java b/app/src/main/java/im/conversations/android/xmpp/manager/TrustManager.java index 1b047dee2..192c2e1d6 100644 --- a/app/src/main/java/im/conversations/android/xmpp/manager/TrustManager.java +++ b/app/src/main/java/im/conversations/android/xmpp/manager/TrustManager.java @@ -68,6 +68,7 @@ public class TrustManager extends AbstractManager implements X509TrustManager { try { decision = Boolean.TRUE.equals(futureDecision.get(10, TimeUnit.SECONDS)); } catch (final ExecutionException | InterruptedException | TimeoutException e) { + futureDecision.cancel(true); throw new CertificateException( "Timeout waiting for user response", Throwables.getRootCause(e)); } @@ -97,8 +98,17 @@ public class TrustManager extends AbstractManager implements X509TrustManager { } public static String fingerprint(final byte[] bytes) { - return Joiner.on(':') - .join(Lists.transform(Bytes.asList(bytes), b -> String.format("%02X", b))); + return fingerprint(bytes, bytes.length); + } + + public static String fingerprint(final byte[] bytes, final int segments) { + return Joiner.on('\n') + .join( + Lists.transform( + Lists.transform( + Lists.partition(Bytes.asList(bytes), segments), + s -> Lists.transform(s, b -> String.format("%02X", b))), + hex -> Joiner.on(':').join(hex))); } public void setUserInterfaceCallback( diff --git a/app/src/main/res/layout/fragment_trust_certificate.xml b/app/src/main/res/layout/fragment_trust_certificate.xml new file mode 100644 index 000000000..d052e92dd --- /dev/null +++ b/app/src/main/res/layout/fragment_trust_certificate.xml @@ -0,0 +1,142 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/app/src/main/res/navigation/setup_navigation.xml b/app/src/main/res/navigation/setup_navigation.xml index f5edef831..1d84ab3fd 100644 --- a/app/src/main/res/navigation/setup_navigation.xml +++ b/app/src/main/res/navigation/setup_navigation.xml @@ -28,6 +28,15 @@ app:popExitAnim="@anim/slide_to_right" app:popUpTo="@+id/hostname" /> + + + + Info required Invalid! Certificate login + Potential security risk ahead + To continue compare this SHA-256 fingerprint with that of the server certificate + The server certificate is not trustworthy. If you don’t know what this means it’s best to go back! + Trust certificate