From fb8d74fc1f01c73a4afc3003978ac4ad86e7b890 Mon Sep 17 00:00:00 2001 From: Harsh Shandilya Date: Mon, 10 Apr 2023 16:54:13 +0530 Subject: [PATCH] feat: put new SSH layer behind a feature flag There hasn't been nearly enough regression testing on this and it is causing breakage, let's move it out of the default path until we've ironed out the kinks. --- .../ui/settings/RepositorySettings.kt | 8 +- .../ui/settings/SettingsActivity.kt | 6 +- .../ui/sshkeygen/ShowSshKeyFragment.kt | 6 +- .../ui/sshkeygen/SshKeyGenActivity.kt | 8 +- .../ui/sshkeygen/SshKeyImportActivity.kt | 8 +- .../passwordstore/util/features/Feature.kt | 3 + .../util/git/operation/GitOperation.kt | 12 +-- .../util/git/sshj/SshjSessionFactory.kt | 10 +-- .../app/passwordstore/util/ssh/SSHFacade.kt | 86 +++++++++++++++++++ 9 files changed, 118 insertions(+), 29 deletions(-) create mode 100644 app/src/main/java/app/passwordstore/util/ssh/SSHFacade.kt diff --git a/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt b/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt index c1ec8cbb..8c20becc 100644 --- a/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt +++ b/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt @@ -16,7 +16,6 @@ import androidx.fragment.app.FragmentActivity import app.passwordstore.R import app.passwordstore.data.repo.PasswordRepository import app.passwordstore.injection.prefs.GitPreferences -import app.passwordstore.ssh.SSHKeyManager import app.passwordstore.ui.git.config.GitConfigActivity import app.passwordstore.ui.git.config.GitServerConfigActivity import app.passwordstore.ui.proxy.ProxySelectorActivity @@ -30,6 +29,7 @@ import app.passwordstore.util.extensions.snackbar import app.passwordstore.util.extensions.unsafeLazy import app.passwordstore.util.settings.GitSettings import app.passwordstore.util.settings.PreferenceKeys +import app.passwordstore.util.ssh.SSHFacade import com.github.michaelbull.result.onFailure import com.github.michaelbull.result.runCatching import com.google.android.material.dialog.MaterialAlertDialogBuilder @@ -45,11 +45,11 @@ import de.Maxr1998.modernpreferences.helpers.switch class RepositorySettings( private val activity: FragmentActivity, - private val sshKeyManager: SSHKeyManager, + private val sshFacade: SSHFacade, ) : SettingsProvider { private val generateSshKey = activity.registerForActivityResult(StartActivityForResult()) { - showSshKeyPref?.visible = sshKeyManager.canShowPublicKey() + showSshKeyPref?.visible = sshFacade.canShowPublicKey() } private val hiltEntryPoint by unsafeLazy { @@ -114,7 +114,7 @@ class RepositorySettings( showSshKeyPref = pref(PreferenceKeys.SSH_SEE_KEY) { titleRes = R.string.pref_ssh_see_key_title - visible = PasswordRepository.isGitRepo() && sshKeyManager.canShowPublicKey() + visible = PasswordRepository.isGitRepo() && sshFacade.canShowPublicKey() onClick { ShowSshKeyFragment().show(activity.supportFragmentManager, "public_key") true diff --git a/app/src/main/java/app/passwordstore/ui/settings/SettingsActivity.kt b/app/src/main/java/app/passwordstore/ui/settings/SettingsActivity.kt index 697d0156..a78e6b4b 100644 --- a/app/src/main/java/app/passwordstore/ui/settings/SettingsActivity.kt +++ b/app/src/main/java/app/passwordstore/ui/settings/SettingsActivity.kt @@ -11,8 +11,8 @@ import androidx.appcompat.app.AppCompatActivity import androidx.core.os.BundleCompat import app.passwordstore.R import app.passwordstore.databinding.ActivityPreferenceRecyclerviewBinding -import app.passwordstore.ssh.SSHKeyManager import app.passwordstore.util.extensions.viewBinding +import app.passwordstore.util.ssh.SSHFacade import com.google.android.material.dialog.MaterialAlertDialogBuilder import dagger.hilt.android.AndroidEntryPoint import de.Maxr1998.modernpreferences.Preference @@ -24,7 +24,7 @@ import javax.inject.Inject @AndroidEntryPoint class SettingsActivity : AppCompatActivity() { - @Inject lateinit var sshKeyManager: SSHKeyManager + @Inject lateinit var sshFacade: SSHFacade private lateinit var repositorySettings: RepositorySettings private val miscSettings = MiscSettings(this) private val autofillSettings = AutofillSettings(this) @@ -40,7 +40,7 @@ class SettingsActivity : AppCompatActivity() { super.onCreate(savedInstanceState) setContentView(binding.root) Preference.Config.dialogBuilderFactory = { context -> MaterialAlertDialogBuilder(context) } - repositorySettings = RepositorySettings(this, sshKeyManager) + repositorySettings = RepositorySettings(this, sshFacade) val screen = screen(this) { subScreen { diff --git a/app/src/main/java/app/passwordstore/ui/sshkeygen/ShowSshKeyFragment.kt b/app/src/main/java/app/passwordstore/ui/sshkeygen/ShowSshKeyFragment.kt index 4f52b540..b96342e5 100644 --- a/app/src/main/java/app/passwordstore/ui/sshkeygen/ShowSshKeyFragment.kt +++ b/app/src/main/java/app/passwordstore/ui/sshkeygen/ShowSshKeyFragment.kt @@ -9,7 +9,7 @@ import android.content.Intent import android.os.Bundle import androidx.fragment.app.DialogFragment import app.passwordstore.R -import app.passwordstore.ssh.SSHKeyManager +import app.passwordstore.util.ssh.SSHFacade import com.google.android.material.dialog.MaterialAlertDialogBuilder import dagger.hilt.android.AndroidEntryPoint import javax.inject.Inject @@ -17,11 +17,11 @@ import javax.inject.Inject @AndroidEntryPoint class ShowSshKeyFragment : DialogFragment() { - @Inject lateinit var sshKeyManager: SSHKeyManager + @Inject lateinit var sshFacade: SSHFacade override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { val activity = requireActivity() - val publicKey = sshKeyManager.publicKey() + val publicKey = sshFacade.publicKey() return MaterialAlertDialogBuilder(requireActivity()).run { setMessage(getString(R.string.ssh_keygen_message, publicKey)) setTitle(R.string.your_public_key) diff --git a/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyGenActivity.kt b/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyGenActivity.kt index eea2d659..68267fff 100644 --- a/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyGenActivity.kt +++ b/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyGenActivity.kt @@ -18,11 +18,11 @@ import app.passwordstore.R import app.passwordstore.databinding.ActivitySshKeygenBinding import app.passwordstore.injection.prefs.GitPreferences import app.passwordstore.ssh.SSHKeyAlgorithm -import app.passwordstore.ssh.SSHKeyManager import app.passwordstore.util.auth.BiometricAuthenticator import app.passwordstore.util.auth.BiometricAuthenticator.Result import app.passwordstore.util.extensions.keyguardManager import app.passwordstore.util.extensions.viewBinding +import app.passwordstore.util.ssh.SSHFacade import com.github.michaelbull.result.fold import com.github.michaelbull.result.runCatching import com.google.android.material.dialog.MaterialAlertDialogBuilder @@ -40,7 +40,7 @@ class SshKeyGenActivity : AppCompatActivity() { private var sshKeyAlgorithm = SSHKeyAlgorithm.ECDSA private val binding by viewBinding(ActivitySshKeygenBinding::inflate) @GitPreferences @Inject lateinit var gitPrefs: SharedPreferences - @Inject lateinit var sshKeyManager: SSHKeyManager + @Inject lateinit var sshFacade: SSHFacade override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) @@ -48,7 +48,7 @@ class SshKeyGenActivity : AppCompatActivity() { supportActionBar?.setDisplayHomeAsUpEnabled(true) with(binding) { generate.setOnClickListener { - if (sshKeyManager.keyExists()) { + if (sshFacade.keyExists()) { MaterialAlertDialogBuilder(this@SshKeyGenActivity).run { setTitle(R.string.ssh_keygen_existing_title) setMessage(R.string.ssh_keygen_existing_message) @@ -126,7 +126,7 @@ class SshKeyGenActivity : AppCompatActivity() { if (result !is Result.Success) throw UserNotAuthenticatedException(getString(R.string.biometric_auth_generic_failure)) } - sshKeyManager.generateKey(sshKeyAlgorithm, requireAuthentication) + sshFacade.generateKey(sshKeyAlgorithm, requireAuthentication) } } // Check if we still need this diff --git a/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyImportActivity.kt b/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyImportActivity.kt index a5d276ae..0d8ec7bf 100644 --- a/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyImportActivity.kt +++ b/app/src/main/java/app/passwordstore/ui/sshkeygen/SshKeyImportActivity.kt @@ -12,7 +12,7 @@ import androidx.activity.result.contract.ActivityResultContracts import androidx.appcompat.app.AppCompatActivity import androidx.lifecycle.lifecycleScope import app.passwordstore.R -import app.passwordstore.ssh.SSHKeyManager +import app.passwordstore.util.ssh.SSHFacade import com.github.michaelbull.result.onFailure import com.github.michaelbull.result.runCatching import com.google.android.material.dialog.MaterialAlertDialogBuilder @@ -23,7 +23,7 @@ import kotlinx.coroutines.launch @AndroidEntryPoint class SshKeyImportActivity : AppCompatActivity() { - @Inject lateinit var sshKeyManager: SSHKeyManager + @Inject lateinit var sshFacade: SSHFacade private val sshKeyImportAction = registerForActivityResult(ActivityResultContracts.OpenDocument()) { uri: Uri? -> @@ -33,7 +33,7 @@ class SshKeyImportActivity : AppCompatActivity() { } runCatching { lifecycleScope.launch { - sshKeyManager.importKey(uri) + sshFacade.importKey(uri) Toast.makeText( this@SshKeyImportActivity, resources.getString(R.string.ssh_key_success_dialog_title), @@ -55,7 +55,7 @@ class SshKeyImportActivity : AppCompatActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) - if (sshKeyManager.keyExists()) { + if (sshFacade.keyExists()) { MaterialAlertDialogBuilder(this@SshKeyImportActivity).run { setTitle(R.string.ssh_keygen_existing_title) setMessage(R.string.ssh_keygen_existing_message) diff --git a/app/src/main/java/app/passwordstore/util/features/Feature.kt b/app/src/main/java/app/passwordstore/util/features/Feature.kt index f2427c0f..a714587a 100644 --- a/app/src/main/java/app/passwordstore/util/features/Feature.kt +++ b/app/src/main/java/app/passwordstore/util/features/Feature.kt @@ -19,6 +19,9 @@ enum class Feature( level = DeprecationLevel.ERROR ) EnablePGPainlessBackend(true, "enable_pgp_v2_backend"), + + /** Opt into the new SSH layer implemented as a freestanding module. */ + EnableNewSSHLayer(false, "enable_new_ssh"), ; companion object { diff --git a/app/src/main/java/app/passwordstore/util/git/operation/GitOperation.kt b/app/src/main/java/app/passwordstore/util/git/operation/GitOperation.kt index 07ce7245..396e3c7d 100644 --- a/app/src/main/java/app/passwordstore/util/git/operation/GitOperation.kt +++ b/app/src/main/java/app/passwordstore/util/git/operation/GitOperation.kt @@ -10,7 +10,6 @@ import androidx.appcompat.app.AppCompatActivity import androidx.fragment.app.FragmentActivity import app.passwordstore.R import app.passwordstore.data.repo.PasswordRepository -import app.passwordstore.ssh.SSHKeyManager import app.passwordstore.ui.sshkeygen.SshKeyGenActivity import app.passwordstore.ui.sshkeygen.SshKeyImportActivity import app.passwordstore.util.auth.BiometricAuthenticator @@ -22,6 +21,7 @@ import app.passwordstore.util.git.GitCommandExecutor import app.passwordstore.util.git.sshj.SshAuthMethod import app.passwordstore.util.git.sshj.SshjSessionFactory import app.passwordstore.util.settings.AuthMode +import app.passwordstore.util.ssh.SSHFacade import com.github.michaelbull.result.Err import com.github.michaelbull.result.Ok import com.github.michaelbull.result.Result @@ -71,7 +71,7 @@ abstract class GitOperation(protected val callingActivity: FragmentActivity) { callingActivity.applicationContext, GitOperationEntryPoint::class.java ) - private val sshKeyManager = hiltEntryPoint.sshKeyManager() + private val sshFacade = hiltEntryPoint.sshFacade() protected val repository = PasswordRepository.repository!! protected val git = Git(repository) private val authActivity @@ -125,7 +125,7 @@ abstract class GitOperation(protected val callingActivity: FragmentActivity) { authMethod: SshAuthMethod, credentialsProvider: CredentialsProvider? = null ) { - sshSessionFactory = SshjSessionFactory(authMethod, hostKeyFile, sshKeyManager) + sshSessionFactory = SshjSessionFactory(authMethod, hostKeyFile, sshFacade) commands.filterIsInstance>().forEach { command -> command.setTransportConfigCallback { transport: Transport -> (transport as? SshTransport)?.sshSessionFactory = sshSessionFactory @@ -173,8 +173,8 @@ abstract class GitOperation(protected val callingActivity: FragmentActivity) { suspend fun executeAfterAuthentication(authMode: AuthMode): Result { when (authMode) { AuthMode.SshKey -> - if (sshKeyManager.keyExists()) { - if (sshKeyManager.needsAuthentication()) { + if (sshFacade.keyExists()) { + if (sshFacade.needsAuthentication()) { val result = withContext(Dispatchers.Main) { suspendCoroutine { cont -> @@ -245,6 +245,6 @@ abstract class GitOperation(protected val callingActivity: FragmentActivity) { @EntryPoint @InstallIn(SingletonComponent::class) interface GitOperationEntryPoint { - fun sshKeyManager(): SSHKeyManager + fun sshFacade(): SSHFacade } } diff --git a/app/src/main/java/app/passwordstore/util/git/sshj/SshjSessionFactory.kt b/app/src/main/java/app/passwordstore/util/git/sshj/SshjSessionFactory.kt index c6e648c5..0875913f 100644 --- a/app/src/main/java/app/passwordstore/util/git/sshj/SshjSessionFactory.kt +++ b/app/src/main/java/app/passwordstore/util/git/sshj/SshjSessionFactory.kt @@ -6,9 +6,9 @@ package app.passwordstore.util.git.sshj import android.util.Base64 import androidx.appcompat.app.AppCompatActivity -import app.passwordstore.ssh.SSHKeyManager import app.passwordstore.util.git.operation.CredentialFinder import app.passwordstore.util.settings.AuthMode +import app.passwordstore.util.ssh.SSHFacade import com.github.michaelbull.result.getOrElse import com.github.michaelbull.result.runCatching import java.io.File @@ -69,7 +69,7 @@ abstract class InteractivePasswordFinder : PasswordFinder { class SshjSessionFactory( private val authMethod: SshAuthMethod, private val hostKeyFile: File, - private val sshKeyManager: SSHKeyManager, + private val sshFacade: SSHFacade, ) : SshSessionFactory() { private var currentSession: SshjSession? = null @@ -81,7 +81,7 @@ class SshjSessionFactory( tms: Int ): RemoteSession { return currentSession - ?: SshjSession(uri, uri.user, authMethod, hostKeyFile, sshKeyManager).connect().also { + ?: SshjSession(uri, uri.user, authMethod, hostKeyFile, sshFacade).connect().also { logcat { "New SSH connection created" } currentSession = it } @@ -125,7 +125,7 @@ private class SshjSession( private val username: String, private val authMethod: SshAuthMethod, private val hostKeyFile: File, - private val sshKeyManager: SSHKeyManager, + private val sshFacade: SSHFacade, ) : RemoteSession { private lateinit var ssh: SSHClient @@ -160,7 +160,7 @@ private class SshjSession( is SshAuthMethod.SshKey -> { val pubkeyAuth = AuthPublickey( - sshKeyManager.keyProvider(ssh, CredentialFinder(authMethod.activity, AuthMode.SshKey)) + sshFacade.keyProvider(ssh, CredentialFinder(authMethod.activity, AuthMode.SshKey)) ) ssh.auth(username, pubkeyAuth, passwordAuth) } diff --git a/app/src/main/java/app/passwordstore/util/ssh/SSHFacade.kt b/app/src/main/java/app/passwordstore/util/ssh/SSHFacade.kt new file mode 100644 index 00000000..a28f18a5 --- /dev/null +++ b/app/src/main/java/app/passwordstore/util/ssh/SSHFacade.kt @@ -0,0 +1,86 @@ +package app.passwordstore.util.ssh + +import android.net.Uri +import app.passwordstore.ssh.SSHKeyAlgorithm +import app.passwordstore.ssh.SSHKeyManager +import app.passwordstore.util.features.Feature +import app.passwordstore.util.features.Features +import app.passwordstore.util.git.operation.CredentialFinder +import app.passwordstore.util.git.sshj.SshKey +import javax.inject.Inject +import javax.inject.Provider +import net.schmizz.sshj.SSHClient +import net.schmizz.sshj.userauth.keyprovider.KeyProvider + +/** A wrapper around [SshKey] and [SSHKeyManager] to allow switching between them at runtime. */ +class SSHFacade +@Inject +constructor( + private val features: Provider, + private val sshKeyManager: SSHKeyManager, +) { + + private val useNewSSH + get() = features.get().isEnabled(Feature.EnableNewSSHLayer) + + fun canShowPublicKey(): Boolean { + return if (useNewSSH) { + sshKeyManager.canShowPublicKey() + } else { + SshKey.canShowSshPublicKey + } + } + + fun publicKey(): String? { + return if (useNewSSH) { + sshKeyManager.publicKey() + } else { + SshKey.sshPublicKey + } + } + + fun keyExists(): Boolean { + return if (useNewSSH) { + sshKeyManager.keyExists() + } else { + SshKey.exists + } + } + + suspend fun generateKey(keyAlgorithm: SSHKeyAlgorithm, requireAuthentication: Boolean) { + if (useNewSSH) { + sshKeyManager.generateKey(keyAlgorithm, requireAuthentication) + } else { + when (keyAlgorithm) { + SSHKeyAlgorithm.RSA -> + SshKey.generateKeystoreNativeKey(SshKey.Algorithm.Rsa, requireAuthentication) + SSHKeyAlgorithm.ECDSA -> + SshKey.generateKeystoreNativeKey(SshKey.Algorithm.Ecdsa, requireAuthentication) + SSHKeyAlgorithm.ED25519 -> SshKey.generateKeystoreWrappedEd25519Key(requireAuthentication) + } + } + } + + suspend fun importKey(uri: Uri) { + if (useNewSSH) { + sshKeyManager.importKey(uri) + } else { + SshKey.import(uri) + } + } + + fun needsAuthentication(): Boolean { + return if (useNewSSH) { + sshKeyManager.needsAuthentication() + } else { + SshKey.mustAuthenticate + } + } + fun keyProvider(client: SSHClient, credentialFinder: CredentialFinder): KeyProvider? { + return if (useNewSSH) { + sshKeyManager.keyProvider(client, credentialFinder) + } else { + SshKey.provide(client, credentialFinder) + } + } +}