From b3a36b9f5a0e8782c39b6749d86a6527cbfaef7a Mon Sep 17 00:00:00 2001 From: Gaurav Ujjwal Date: Tue, 30 Jan 2024 23:59:42 +0530 Subject: [PATCH] Remove ability to save SSH private key passwords This brings AVNC's behavior inline with other SSH clients, e.g. OpenSSH. --- .../gaurav/avnc/ui/home/ProfileEditorTest.kt | 2 -- .../gaurav/avnc/ui/vnc/LoginFragmentTest.kt | 24 +++++++++++++------ .../com/gaurav/avnc/ui/home/ProfileEditor.kt | 6 ++--- .../com/gaurav/avnc/ui/vnc/LoginFragment.kt | 13 ++++++---- .../gaurav/avnc/viewmodel/EditorViewModel.kt | 1 - .../com/gaurav/avnc/viewmodel/VncViewModel.kt | 4 ---- .../main/res/layout/fragment_credential.xml | 15 +++++++++++- .../fragment_profile_editor_advanced.xml | 13 ---------- app/src/main/res/values/strings.xml | 1 + 9 files changed, 42 insertions(+), 37 deletions(-) diff --git a/app/src/androidTest/java/com/gaurav/avnc/ui/home/ProfileEditorTest.kt b/app/src/androidTest/java/com/gaurav/avnc/ui/home/ProfileEditorTest.kt index 84a9a3d..4f880a7 100644 --- a/app/src/androidTest/java/com/gaurav/avnc/ui/home/ProfileEditorTest.kt +++ b/app/src/androidTest/java/com/gaurav/avnc/ui/home/ProfileEditorTest.kt @@ -154,7 +154,6 @@ class PrivateKeyTest { setupFileOpenIntent(SAMPLE_KEY) onView(withId(R.id.key_import_btn)).doClick() onView(withText(R.string.msg_imported)).checkWillBeDisplayed() - onView(withId(R.id.ssh_key_password)).checkIsNotDisplayed() } @Test @@ -162,7 +161,6 @@ class PrivateKeyTest { setupFileOpenIntent(SAMPLE_ENCRYPTED_KEY) onView(withId(R.id.key_import_btn)).doClick() onView(withText(R.string.msg_imported)).checkWillBeDisplayed() - onView(withId(R.id.ssh_key_password)).checkWillBeDisplayed() } @Test diff --git a/app/src/androidTest/java/com/gaurav/avnc/ui/vnc/LoginFragmentTest.kt b/app/src/androidTest/java/com/gaurav/avnc/ui/vnc/LoginFragmentTest.kt index 27e59c4..431cc94 100644 --- a/app/src/androidTest/java/com/gaurav/avnc/ui/vnc/LoginFragmentTest.kt +++ b/app/src/androidTest/java/com/gaurav/avnc/ui/vnc/LoginFragmentTest.kt @@ -22,11 +22,20 @@ import androidx.test.espresso.Espresso.onIdle import androidx.test.espresso.Espresso.onView import androidx.test.espresso.matcher.ViewMatchers.withId import androidx.test.espresso.matcher.ViewMatchers.withText -import com.gaurav.avnc.* +import com.gaurav.avnc.R +import com.gaurav.avnc.checkIsDisplayed +import com.gaurav.avnc.checkIsNotDisplayed +import com.gaurav.avnc.checkWillBeDisplayed +import com.gaurav.avnc.doClick +import com.gaurav.avnc.doTypeText +import com.gaurav.avnc.inDialog import com.gaurav.avnc.model.LoginInfo import com.gaurav.avnc.model.ServerProfile import com.gaurav.avnc.model.db.MainDb +import com.gaurav.avnc.runOnMainSync +import com.gaurav.avnc.targetContext import com.gaurav.avnc.viewmodel.VncViewModel +import com.gaurav.avnc.withActivity import kotlinx.coroutines.runBlocking import org.junit.Assert.assertEquals import org.junit.Test @@ -129,17 +138,20 @@ class LoginFragmentTest { assertEquals(SAMPLE_PASSWORD, p.sshPassword) } + // We no longer save Private Key password. It is always asked from user and a message is shown to users + // who have previously saved key password. @Test - fun sshKeyPasswordLoginWithRememberChecked() = Scenario().use { scenario -> + fun sshKeyPasswordMigrationMessage() = Scenario(ServerProfile(sshPrivateKeyPassword = "foo")).use { scenario -> scenario.triggerLoginInfoRequest(LoginInfo.Type.SSH_KEY_PASSWORD) onView(withId(R.id.password)).inDialog().checkWillBeDisplayed().doTypeText(SAMPLE_PASSWORD) - onView(withId(R.id.remember)).inDialog().checkIsDisplayed().doClick() + onView(withId(R.id.remember)).inDialog().checkIsNotDisplayed() + onView(withId(R.id.pk_password_msg)).inDialog().checkIsDisplayed() onView(withText(android.R.string.ok)).inDialog().checkIsDisplayed().doClick() val l = scenario.waitForLoginInfo() val p = scenario.triggerLoginSave() assertEquals(SAMPLE_PASSWORD, l.password) - assertEquals(SAMPLE_PASSWORD, p.sshPrivateKeyPassword) + assertEquals("", p.sshPrivateKeyPassword) // Saved password should have been cleared } /** @@ -148,14 +160,12 @@ class LoginFragmentTest { */ @Test(timeout = 5000) fun savedLoginTest() { - val profile = ServerProfile(username = "AB", password = "BC", sshPassword = "CD", sshPrivateKeyPassword = "DE") + val profile = ServerProfile(username = "AB", password = "BC", sshPassword = "CD") val viewModel = runOnMainSync { VncViewModel(profile, ApplicationProvider.getApplicationContext()) } assertEquals("AB", viewModel.getLoginInfo(LoginInfo.Type.VNC_CREDENTIAL).username) assertEquals("BC", viewModel.getLoginInfo(LoginInfo.Type.VNC_CREDENTIAL).password) assertEquals("BC", viewModel.getLoginInfo(LoginInfo.Type.VNC_PASSWORD).password) - assertEquals("CD", viewModel.getLoginInfo(LoginInfo.Type.SSH_PASSWORD).password) - assertEquals("DE", viewModel.getLoginInfo(LoginInfo.Type.SSH_KEY_PASSWORD).password) } } diff --git a/app/src/main/java/com/gaurav/avnc/ui/home/ProfileEditor.kt b/app/src/main/java/com/gaurav/avnc/ui/home/ProfileEditor.kt index c70e6fe..a49c3c6 100644 --- a/app/src/main/java/com/gaurav/avnc/ui/home/ProfileEditor.kt +++ b/app/src/main/java/com/gaurav/avnc/ui/home/ProfileEditor.kt @@ -36,9 +36,9 @@ import com.gaurav.avnc.util.MsgDialog import com.gaurav.avnc.util.OpenableDocument import com.gaurav.avnc.viewmodel.EditorViewModel import com.gaurav.avnc.viewmodel.HomeViewModel -import com.gaurav.avnc.viewmodel.isPrivateKeyEncrypted import com.google.android.material.dialog.MaterialAlertDialogBuilder import com.google.android.material.snackbar.Snackbar +import com.trilead.ssh2.crypto.PEMDecoder import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -272,7 +272,6 @@ class AdvancedProfileEditor : Fragment() { lifecycleScope.launch(Dispatchers.IO) { var key = "" - var encrypted = false val result = runCatching { requireContext().contentResolver.openAssetFileDescriptor(uri, "r")!!.use { // Valid key files are only few KBs. So if selected file is too big, @@ -280,13 +279,12 @@ class AdvancedProfileEditor : Fragment() { check(it.length < 2 * 1024 * 1024) { "File is too big [${it.length}]" } key = it.createInputStream().use { s -> s.reader().use { r -> r.readText() } } } - encrypted = isPrivateKeyEncrypted(key) + PEMDecoder.parsePEM(key.toCharArray()) //Try to parse key } withContext(Dispatchers.Main) { result.onSuccess { viewModel.profile.sshPrivateKey = key - viewModel.isPrivateKeyEncrypted.value = encrypted viewModel.hasSshPrivateKey.value = true binding.keyImportBtn.error = null Snackbar.make(binding.root, R.string.msg_imported, Snackbar.LENGTH_SHORT).show() diff --git a/app/src/main/java/com/gaurav/avnc/ui/vnc/LoginFragment.kt b/app/src/main/java/com/gaurav/avnc/ui/vnc/LoginFragment.kt index 1fe1866..5200899 100644 --- a/app/src/main/java/com/gaurav/avnc/ui/vnc/LoginFragment.kt +++ b/app/src/main/java/com/gaurav/avnc/ui/vnc/LoginFragment.kt @@ -46,9 +46,12 @@ class LoginFragment : DialogFragment() { binding.loginInfo = loginInfo binding.usernameLayout.isVisible = loginInfo.username.isBlank() && loginType == LoginInfo.Type.VNC_CREDENTIAL binding.passwordLayout.isVisible = loginInfo.password.isBlank() - binding.remember.isVisible = viewModel.profile.ID != 0L - if (loginType == LoginInfo.Type.SSH_KEY_PASSWORD) + binding.remember.isVisible = viewModel.profile.ID != 0L && loginType != LoginInfo.Type.SSH_KEY_PASSWORD + + if (loginType == LoginInfo.Type.SSH_KEY_PASSWORD) { binding.passwordLayout.setHint(R.string.hint_key_password) + binding.pkPasswordMsg.isVisible = viewModel.profile.sshPrivateKeyPassword.isNotBlank() + } setupAutoComplete() isCancelable = false @@ -73,7 +76,7 @@ class LoginFragment : DialogFragment() { LoginInfo.Type.VNC_PASSWORD -> LoginInfo(p.name, p.host, "", p.password) LoginInfo.Type.VNC_CREDENTIAL -> LoginInfo(p.name, p.host, p.username, p.password) LoginInfo.Type.SSH_PASSWORD -> LoginInfo(p.name, p.sshHost, "", p.sshPassword) - LoginInfo.Type.SSH_KEY_PASSWORD -> LoginInfo(p.name, p.sshHost, "", p.sshPrivateKeyPassword) + LoginInfo.Type.SSH_KEY_PASSWORD -> LoginInfo(p.name, p.sshHost, "", "" /*p.sshPrivateKeyPassword*/) } } @@ -85,14 +88,14 @@ class LoginFragment : DialogFragment() { p.password = l.password } LoginInfo.Type.SSH_PASSWORD -> p.sshPassword = l.password - LoginInfo.Type.SSH_KEY_PASSWORD -> p.sshPrivateKeyPassword = l.password + LoginInfo.Type.SSH_KEY_PASSWORD -> p.sshPrivateKeyPassword = "" /* key password is not saved anymore */ } } private fun onOk() { loginInfo.password = getRealPassword(loginInfo.password) viewModel.loginInfoRequest.offerResponse(loginInfo) - if (binding.remember.isChecked) + if (binding.remember.isChecked || binding.pkPasswordMsg.isVisible /* to forget saved password */) saveLoginInfo(loginInfo) } diff --git a/app/src/main/java/com/gaurav/avnc/viewmodel/EditorViewModel.kt b/app/src/main/java/com/gaurav/avnc/viewmodel/EditorViewModel.kt index 4220c88..7d057d7 100644 --- a/app/src/main/java/com/gaurav/avnc/viewmodel/EditorViewModel.kt +++ b/app/src/main/java/com/gaurav/avnc/viewmodel/EditorViewModel.kt @@ -37,7 +37,6 @@ class EditorViewModel(app: Application, state: SavedStateHandle, initialProfile: val sshUsePassword = state.getLiveData("sshUsePassword", profile.sshAuthType == ServerProfile.SSH_AUTH_PASSWORD) val sshUsePrivateKey = state.getLiveData("sshUsePrivateKey", profile.sshAuthType == ServerProfile.SSH_AUTH_KEY) val hasSshPrivateKey = state.getLiveData("hasSshPrivateKey", profile.sshPrivateKey.isNotBlank()) - val isPrivateKeyEncrypted = state.getLiveData("isPrivateKeyEncrypted", profile.sshPrivateKey.isNotBlank() && isPrivateKeyEncrypted(profile.sshPrivateKey)) fun prepareProfileForSave(): ServerProfile { diff --git a/app/src/main/java/com/gaurav/avnc/viewmodel/VncViewModel.kt b/app/src/main/java/com/gaurav/avnc/viewmodel/VncViewModel.kt index 2aa082e..6067c02 100644 --- a/app/src/main/java/com/gaurav/avnc/viewmodel/VncViewModel.kt +++ b/app/src/main/java/com/gaurav/avnc/viewmodel/VncViewModel.kt @@ -316,7 +316,6 @@ class VncViewModel(val profile: ServerProfile, app: Application) : BaseViewModel val vu = profile.username val vp = profile.password val sp = profile.sshPassword - val kp = profile.sshPrivateKeyPassword if (type == LoginInfo.Type.VNC_PASSWORD && vp.isNotBlank()) return LoginInfo(password = vp) @@ -327,9 +326,6 @@ class VncViewModel(val profile: ServerProfile, app: Application) : BaseViewModel if (type == LoginInfo.Type.SSH_PASSWORD && sp.isNotBlank()) return LoginInfo(password = sp) - if (type == LoginInfo.Type.SSH_KEY_PASSWORD && kp.isNotBlank()) - return LoginInfo(password = kp) - // Something is missing, so we have to ask the user return loginInfoRequest.requestResponse(type) // Blocking call } diff --git a/app/src/main/res/layout/fragment_credential.xml b/app/src/main/res/layout/fragment_credential.xml index 65a16f0..92ece9d 100644 --- a/app/src/main/res/layout/fragment_credential.xml +++ b/app/src/main/res/layout/fragment_credential.xml @@ -7,7 +7,8 @@ --> + xmlns:app="http://schemas.android.com/apk/res-auto" + xmlns:tools="http://schemas.android.com/tools"> @@ -70,5 +71,17 @@ android:text="@string/title_remember" android:textAppearance="@style/TextAppearance.MaterialComponents.Body2" /> + + \ No newline at end of file diff --git a/app/src/main/res/layout/fragment_profile_editor_advanced.xml b/app/src/main/res/layout/fragment_profile_editor_advanced.xml index d3107d4..9d3baa5 100644 --- a/app/src/main/res/layout/fragment_profile_editor_advanced.xml +++ b/app/src/main/res/layout/fragment_profile_editor_advanced.xml @@ -442,19 +442,6 @@ app:layout_constraintStart_toStartOf="parent" app:layout_constraintTop_toBottomOf="@id/ssh_auth_type" /> - - diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index a8fd211..60d5567 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -81,6 +81,7 @@ Invalid private key file Invalid VNC URI Copied + AVNC no longer saves private key passwords to improve security. Some servers have limited Unicode support. Legacy key events can help input non-English text. Delaying click events can help in some rare cases if an app is not responding to clicks. Assigning an action to this gesture will change Long press detection:\n\nPress-hold-release → Long press\nPress-hold-swipe → Long press and swipe