From 6a614fe7dc1cbbec5783828d58b246a362edea07 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 3 Nov 2026 13:44:42 +0100 Subject: feat(app): add the setup model and stricter key checks URL checks and fixes, authentication modes per URL, the clone target, the store's recipients, a size- and symlink-checked .gpg-id read and the .gpg-id text for a new store (uppercase long key IDs), as pure functions for the Compose setup screens. The SSH key import now also requires a format sshj recognizes (an OpenPGP private key block matched the old marker check and was imported as the SSH key) and no longer crashes on an empty size cursor. KeyUtils tells secret keys apart and no longer throws on a key without user ID. Co-Authored-By: Claude Opus 5.5 --- .../main/kotlin/app/passwordstore/crypto/KeyUtils.kt | 7 ++++++- .../kotlin/app/passwordstore/crypto/KeyUtilsTest.kt | 19 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) (limited to 'crypto') diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt index f85fe92c..311e9049 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt @@ -10,6 +10,7 @@ import app.passwordstore.crypto.PGPIdentifier.UserId import com.github.michaelbull.result.get import com.github.michaelbull.result.runCatching import org.bouncycastle.openpgp.PGPKeyRing +import org.bouncycastle.openpgp.PGPSecretKeyRing import org.pgpainless.PGPainless import org.pgpainless.key.parsing.KeyRingReader @@ -35,9 +36,13 @@ public object KeyUtils { */ public fun tryGetEmail(key: PGPKey): UserId? { val keyRing = tryParseKeyring(key) ?: return null - return UserId(keyRing.publicKey.userIDs.next()) + // A key may carry no user ID at all: next() threw NoSuchElementException there. + return keyRing.publicKey.userIDs.asSequence().firstOrNull()?.let(::UserId) } + /** True when [key] parses as a secret key ring, the only kind that can decrypt. */ + public fun isSecretKey(key: PGPKey): Boolean = tryParseKeyring(key) is PGPSecretKeyRing + /** * Tests if the given [key] can be used for encryption, which is a bare minimum necessity for the * app. diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/KeyUtilsTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/KeyUtilsTest.kt index e6e3d7d1..3281c334 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/KeyUtilsTest.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/KeyUtilsTest.kt @@ -6,14 +6,18 @@ package app.passwordstore.crypto import app.passwordstore.crypto.KeyUtils.isKeyUsable +import app.passwordstore.crypto.KeyUtils.isSecretKey +import app.passwordstore.crypto.KeyUtils.tryGetEmail import app.passwordstore.crypto.KeyUtils.tryGetId import app.passwordstore.crypto.KeyUtils.tryParseKeyring import app.passwordstore.crypto.TestUtils.AllKeys import app.passwordstore.crypto.TestUtils.getArmoredSecretKeyWithMultipleIdentities import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertIs import kotlin.test.assertNotNull +import kotlin.test.assertTrue import org.bouncycastle.openpgp.PGPSecretKeyRing class KeyUtilsTest { @@ -37,4 +41,19 @@ class KeyUtilsTest { assertEquals(isUsable, isKeyUsable(key), "${allKeys.name} failed expectation:") } } + + @Test + fun secretKeysAreTold() { + assertTrue(isSecretKey(PGPKey(AllKeys.ARMORED_SEC.keyMaterial))) + assertTrue(isSecretKey(PGPKey(AllKeys.GNUPG_OCB_SEC.keyMaterial))) + assertFalse(isSecretKey(PGPKey(AllKeys.ARMORED_PUB.keyMaterial))) + assertFalse(isSecretKey(PGPKey("not a key".encodeToByteArray()))) + } + + @Test + fun firstUserIdIsReturned() { + val key = PGPKey(getArmoredSecretKeyWithMultipleIdentities()) + assertNotNull(tryGetEmail(key)) + assertEquals(null, tryGetEmail(PGPKey("not a key".encodeToByteArray()))) + } } -- cgit v1.2.3 From fcbd5999f28f287507844b55f2bee82e714620c8 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 3 Nov 2026 13:57:05 +0100 Subject: fix(app): harden the setup key imports - PGP key import reads at most 1 MiB (bounded read, buffer zeroed) and refuses larger files with a new message. - SshKeyViewModel catches any Exception from import and generation (CancellationException rethrown) instead of a short list, so an unexpected provider or sshj error no longer crashes the app. - Pending PGP key bytes are zeroed in a finally block, also on cancellation during addKey after a replace, and an earlier pending pick is zeroed before a new one takes its place. - PGPKeyManager never replaces a stored secret key with a public-only key of the same ID, with or without replace (KeyDowngradeException); the setup ViewModel shows a localized message for it. Unit test added. - storeRecipients strips '#' comments and a trailing '!' as pass reads .gpg-id; tests added. - GitServerViewModel.clearStore returns the failure when the store folder cannot be emptied, so the clone does not start; the plan's Task 3 call site reports it. - readGpgId opens the file with NOFOLLOW_LINKS and reads at most 64 KiB + 1 bytes, refusing more. - SshKey.import writes the new private key to a temp file and moves it over atomically before dropping the old key's Keystore entry, public key and passphrase, so an I/O error keeps the old key. - KeyUtils.isSecretKey notes (ponytail) that GnuPG stub and smartcard keyrings count as secret. Co-Authored-By: Claude Opus 5.5 --- .../passwordstore/ui/setup/GitServerViewModel.kt | 11 ++-- .../app/passwordstore/ui/setup/PgpKeysViewModel.kt | 58 +++++++++++++++------- .../java/app/passwordstore/ui/setup/SetupModel.kt | 45 ++++++++++++++--- .../app/passwordstore/ui/setup/SshKeyViewModel.kt | 22 ++++---- .../java/app/passwordstore/util/git/sshj/SshKey.kt | 32 +++++++++--- app/src/main/res/values-it/strings.xml | 3 ++ app/src/main/res/values/strings.xml | 3 ++ .../app/passwordstore/ui/setup/SetupModelTest.kt | 19 +++++++ .../passwordstore/crypto/errors/CryptoException.kt | 4 ++ .../kotlin/app/passwordstore/crypto/KeyUtils.kt | 2 + .../app/passwordstore/crypto/PGPKeyManager.kt | 4 ++ .../app/passwordstore/crypto/PGPKeyManagerTest.kt | 11 ++++ docs/superpowers/plans/2026-11-03-compose-setup.md | 8 ++- 13 files changed, 172 insertions(+), 50 deletions(-) (limited to 'crypto') diff --git a/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt index f2b8ca30..d549624e 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt @@ -83,13 +83,16 @@ constructor( /** * Empties the store folder at [root] for a clone, unless it is empty already. Never follows a * symlink: `deleteRecursively()` would, and a `store/link -> ../keys` link would take the PGP - * keys with it. + * keys with it. Returns null, or why it failed as a string resource: then the clone must not + * start. */ - suspend fun clearStore(root: File) { - if (withContext(dispatcherProvider.io()) { cloneTarget(root) } == CloneTarget.Empty) return + suspend fun clearStore(root: File): Int? { + if (withContext(dispatcherProvider.io()) { cloneTarget(root) } == CloneTarget.Empty) return null progress = R.string.delete_directory_progress_text - withContext(dispatcherProvider.io()) { root.deleteWithoutFollowingLinks() } + if (!withContext(dispatcherProvider.io()) { root.deleteWithoutFollowingLinks() }) + return R.string.git_clone_delete_failed progress = R.string.git_clone_progress + return null } /** Set while [finishClone]'s key check runs: the clone's [progress] stays up until it is done. */ diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt index e6000206..ed7fcf37 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt @@ -13,11 +13,13 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.setValue import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope +import app.passwordstore.R import app.passwordstore.crypto.KeyUtils import app.passwordstore.crypto.PGPIdentifier import app.passwordstore.crypto.PGPKey import app.passwordstore.crypto.PGPKeyManager import app.passwordstore.crypto.errors.KeyAlreadyExistsException +import app.passwordstore.crypto.errors.KeyDowngradeException import app.passwordstore.data.repo.PasswordRepository import app.passwordstore.util.coroutines.DispatcherProvider import com.github.michaelbull.result.onSuccess @@ -126,18 +128,23 @@ constructor( } } - /** Reads the key file at [uri] and imports it. */ + /** Reads the key file at [uri] (at most [MAX_KEY_BYTES]) and imports it. */ fun import(uri: Uri, context: Context) { if (busy) return busy = true val resolver = context.applicationContext.contentResolver + val tooLarge = context.getString(R.string.pgp_key_import_too_large) + val downgrade = context.getString(R.string.pgp_key_import_downgrade) viewModelScope.launch { try { val bytes = withContext(dispatcherProvider.io()) { - resolver.openInputStream(uri)?.use { it.readBytes() } + resolver.openInputStream(uri)?.use { + readAtMost(it, MAX_KEY_BYTES) ?: throw IOException(tooLarge) + } } - if (bytes == null) importResult = KeyImport.Failed(null) else add(bytes, replace = false) + if (bytes == null) importResult = KeyImport.Failed(null) + else add(bytes, replace = false, downgrade) } catch (e: IOException) { importResult = KeyImport.Failed(e.message) } catch (e: SecurityException) { @@ -168,22 +175,34 @@ constructor( importResult = null } - /** Imports [bytes]; they are zeroed afterwards unless the replace question keeps them. */ - private suspend fun add(bytes: ByteArray, replace: Boolean) { - val result = keyManager.addKey(PGPKey(bytes), replace) - if (!replace && result.isErr && result.error is KeyAlreadyExistsException) { - pending = bytes - importResult = KeyImport.Exists - return + /** + * Imports [bytes]; they are zeroed afterwards, also on failure or cancellation, unless the + * replace question keeps them. [downgrade] is the message for a public key over a secret one. + */ + private suspend fun add(bytes: ByteArray, replace: Boolean, downgrade: String? = null) { + var keep = false + try { + val result = keyManager.addKey(PGPKey(bytes), replace) + if (!replace && result.isErr && result.error is KeyAlreadyExistsException) { + // A question still open from an earlier pick is superseded: zero its bytes first. + dropPending() + pending = bytes + keep = true + importResult = KeyImport.Exists + return + } + // The ID is read before the bytes are zeroed: the returned key shares them. + importResult = + if (result.isOk) { + val id = withContext(dispatcherProvider.default()) { KeyUtils.tryGetId(result.value) } + KeyImport.Imported(id?.toString().orEmpty()) + } else if (result.error is KeyDowngradeException) { + KeyImport.Failed(downgrade ?: result.error.message) + } else KeyImport.Failed(result.error.message) + if (result.isOk) updateKeySet() + } finally { + if (!keep) bytes.fill(0) } - // The ID is read before the bytes are zeroed: the returned key shares them. - importResult = - if (result.isOk) { - val id = withContext(dispatcherProvider.default()) { KeyUtils.tryGetId(result.value) } - KeyImport.Imported(id?.toString().orEmpty()) - } else KeyImport.Failed(result.error.message) - bytes.fill(0) - if (result.isOk) updateKeySet() } /** @@ -223,3 +242,6 @@ constructor( dropPending() } } + +/** The largest key file imported: far above any real key, small enough to hold in memory. */ +private const val MAX_KEY_BYTES = 1024 * 1024 diff --git a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt index 8c7b20a1..ade23aa2 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt @@ -9,8 +9,11 @@ import app.passwordstore.crypto.PGPIdentifier import app.passwordstore.util.settings.AuthMode import java.io.File import java.io.IOException +import java.io.InputStream +import java.nio.channels.Channels import java.nio.file.Files import java.nio.file.LinkOption +import java.nio.file.StandardOpenOption import java.util.Locale /** Why a repository URL is questioned before it is saved, as the old server screen checked it. */ @@ -73,9 +76,17 @@ fun cloneTarget(dir: File): CloneTarget { } } -/** The recipients named by a `.gpg-id` file's [text]; blank and unparsable lines are skipped. */ +/** + * The recipients named by a `.gpg-id` file's [text], read as pass reads it: a `#` starts a comment, + * and a trailing `!` (GnuPG's "this exact subkey") is dropped. Blank and unparsable lines are + * skipped. + */ fun storeRecipients(text: String): List = - text.lines().map(String::trim).filter(String::isNotEmpty).mapNotNull(PGPIdentifier::fromString) + text + .lines() + .map { it.substringBefore('#').trim().removeSuffix("!") } + .filter(String::isNotEmpty) + .mapNotNull(PGPIdentifier::fromString) /** * A `.gpg-id` naming [keys]: each 16-hex long key ID in uppercase on a line of its own, the format @@ -91,12 +102,34 @@ private const val MAX_GPG_ID_BYTES = 64 * 1024 * than 64 KiB or unreadable. Blocking: call it off the main thread. */ fun readGpgId(dir: File): String? { - val file = File(dir, ".gpg-id") - val path = file.toPath() + val path = File(dir, ".gpg-id").toPath() + if (!Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) return null + // Opened without following a link too: one swapped in after the check is refused. + val options = setOf(StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS) return try { - if (!Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) null - else if (Files.size(path) > MAX_GPG_ID_BYTES) null else file.readText() + Channels.newInputStream(Files.newByteChannel(path, options)) + .use { readAtMost(it, MAX_GPG_ID_BYTES) } + ?.decodeToString() } catch (_: IOException) { null } } + +/** + * At most [limit] bytes of [input], or null when it holds more. Reads at most [limit] + 1 bytes, + * whatever the stream's size, and zeroes its buffer: the bytes may be a private key. + */ +fun readAtMost(input: InputStream, limit: Int): ByteArray? { + val buffer = ByteArray(limit + 1) + try { + var size = 0 + while (size < buffer.size) { + val read = input.read(buffer, size, buffer.size - size) + if (read < 0) break + size += read + } + return if (size > limit) null else buffer.copyOf(size) + } finally { + buffer.fill(0) + } +} diff --git a/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt index d70b06af..bc3e9474 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt @@ -17,10 +17,8 @@ import app.passwordstore.R import app.passwordstore.util.coroutines.DispatcherProvider import app.passwordstore.util.git.sshj.SshKey import dagger.hilt.android.lifecycle.HiltViewModel -import java.io.IOException -import java.security.GeneralSecurityException -import java.security.ProviderException import javax.inject.Inject +import kotlin.coroutines.cancellation.CancellationException import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -83,11 +81,10 @@ class SshKeyViewModel @Inject constructor(private val dispatcherProvider: Dispat withContext(dispatcherProvider.io()) { SshKey.import(uri) } imported = true version++ - } catch (e: IOException) { - failure = e.message.orEmpty() - } catch (e: IllegalArgumentException) { - failure = e.message.orEmpty() - } catch (e: SecurityException) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // Any failure of a provider or of sshj, not only I/O: an uncaught one would crash the app. failure = e.message.orEmpty() } finally { busy = false @@ -114,11 +111,10 @@ class SshKeyViewModel @Inject constructor(private val dispatcherProvider: Dispat SshKey.sshPublicKey.orEmpty() } version++ - } catch (e: GeneralSecurityException) { - failure = e.message.orEmpty() - } catch (e: ProviderException) { - failure = e.message.orEmpty() - } catch (e: IOException) { + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // Keystore and provider failures come as many exception types; none may crash the app. failure = e.message.orEmpty() } finally { busy = false diff --git a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt index 8d5519a8..06cf5bc8 100644 --- a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt +++ b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt @@ -29,6 +29,8 @@ import com.github.michaelbull.result.getOrElse import com.github.michaelbull.result.runCatching import java.io.File import java.io.IOException +import java.nio.file.Files +import java.nio.file.StandardCopyOption import java.security.KeyFactory import java.security.KeyPair import java.security.KeyPairGenerator @@ -203,15 +205,20 @@ object SshKey { } private fun delete() { - androidKeystore.deleteEntry(KEYSTORE_ALIAS) if (privateKeyFile.isFile) { privateKeyFile.delete() } + deleteKeyExtras() + type = null + } + + /** Everything of the current key but its private key file. */ + private fun deleteKeyExtras() { + androidKeystore.deleteEntry(KEYSTORE_ALIAS) if (publicKeyFile.isFile) { publicKeyFile.delete() } context.getEncryptedGitPrefs().edit { remove(PreferenceKeys.SSH_KEY_LOCAL_PASSPHRASE) } - type = null } fun import(uri: Uri) { @@ -241,12 +248,23 @@ object SshKey { ) // At this point, we are reasonably confident that we have actually been provided a private - // key and delete the old key. - delete() - // Canonicalize line endings to '\n'. - privateKeyFile.writeText(lines.joinToString("\n")) - + // key. It replaces the old one atomically (an I/O error keeps the old key), then the old key's + // Keystore entry, public key and passphrase go. + val temp = File(privateKeyFile.parentFile, ".${privateKeyFile.name}.tmp") + try { + // Canonicalize line endings to '\n'. + temp.writeText(lines.joinToString("\n")) + Files.move( + temp.toPath(), + privateKeyFile.toPath(), + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE, + ) + } finally { + temp.delete() + } type = Type.Imported + deleteKeyExtras() } /** Deletes an Ed25519 key wrapped by androidx security-crypto, which can no longer be read. */ diff --git a/app/src/main/res/values-it/strings.xml b/app/src/main/res/values-it/strings.xml index c2edc768..192345cb 100644 --- a/app/src/main/res/values-it/strings.xml +++ b/app/src/main/res/values-it/strings.xml @@ -372,4 +372,7 @@ Chiave privata importata Generata su questo dispositivo Clonazione… + Impossibile svuotare la cartella dell\'archivio, la clonazione non è partita. + Il file supera 1 MiB: non è una chiave PGP. + Una chiave privata con questo ID è già importata. La sola chiave pubblica non può sostituirla: elimina prima la chiave privata, se vuoi davvero farlo. diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index d53700a8..8c046ca5 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -407,4 +407,7 @@ Imported private key Generated on this device Cloning… + Could not empty the store folder, so the clone did not start. + The file is larger than 1 MiB: not a PGP key. + A private key with this ID is already imported. Its public key alone cannot replace it: delete the private key first if you really want to. diff --git a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt index 8d8243eb..53ea80cf 100644 --- a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt +++ b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt @@ -88,6 +88,25 @@ class SetupModelTest { assertEquals(emptyList(), storeRecipients("")) } + @Test + fun storeRecipientsDropCommentsAndExactSubkeyMarks() { + val ids = + storeRecipients( + "# store key\nF8993A789CDFB014! # laptop\n79E8208280490C77!\nalice@example.org#work\n # \n" + ) + assertEquals( + listOf("f8993a789cdfb014", "79e8208280490c77", "alice@example.org"), + ids.map(PGPIdentifier::toString), + ) + } + + @Test + fun readAtMostRefusesMoreThanTheLimit() { + assertEquals("abc", readAtMost("abc".byteInputStream(), 3)?.decodeToString()) + assertEquals("", readAtMost("".byteInputStream(), 3)?.decodeToString()) + assertNull(readAtMost("abcd".byteInputStream(), 3)) + } + @Test fun gpgIdTextIsUppercaseKeyIdsOnePerLine() { val ids = diff --git a/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt b/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt index af6145d0..9589ceec 100644 --- a/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt +++ b/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt @@ -39,6 +39,10 @@ public class KeyNotFoundException(keyId: String) : public class KeyAlreadyExistsException(keyId: String) : KeyManagerException("Pre-existing key was found for $keyId") +/** Attempting to replace a stored secret key with a public-only key of the same `keyId`. */ +public class KeyDowngradeException(keyId: String) : + KeyManagerException("A secret key is stored for $keyId; a public key cannot replace it") + /** Sealed exception types for [app.passwordstore.crypto.CryptoHandler]. */ public sealed class CryptoHandlerException(message: String? = null, cause: Throwable? = null) : CryptoException(message, cause) diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt index 311e9049..9acd0489 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt @@ -41,6 +41,8 @@ public object KeyUtils { } /** True when [key] parses as a secret key ring, the only kind that can decrypt. */ + // ponytail: GnuPG stub and smartcard keyrings (no usable secret material) count as secret too. + // Upgrade path: check for a secret subkey usable for decryption. public fun isSecretKey(key: PGPKey): Boolean = tryParseKeyring(key) is PGPSecretKeyRing /** diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt index 6f852af4..ef58ad19 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt @@ -14,6 +14,7 @@ import app.passwordstore.crypto.errors.InvalidKeyException import app.passwordstore.crypto.errors.KeyAlreadyExistsException import app.passwordstore.crypto.errors.KeyDeletionFailedException import app.passwordstore.crypto.errors.KeyDirectoryUnavailableException +import app.passwordstore.crypto.errors.KeyDowngradeException import app.passwordstore.crypto.errors.KeyNotFoundException import app.passwordstore.crypto.errors.NoKeysAvailableException import app.passwordstore.crypto.errors.UnusableKeyException @@ -59,6 +60,9 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : keyFile.writeBytes(keyBytes) return@runSuspendCatching key } + // Never with replace either: it would delete the only copy of the private key. + existingKeyRing is PGPSecretKeyRing && incomingKeyRing is PGPPublicKeyRing -> + throw KeyDowngradeException(tryGetId(key)?.toString() ?: "Failed to retrieve key ID") } // Check for replace flag first and if it is false, throw an error if (!replace) diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt index 0525c28a..81a666f4 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt @@ -9,6 +9,7 @@ import app.passwordstore.crypto.KeyUtils.tryGetId import app.passwordstore.crypto.PGPIdentifier.KeyId import app.passwordstore.crypto.PGPIdentifier.UserId import app.passwordstore.crypto.errors.KeyAlreadyExistsException +import app.passwordstore.crypto.errors.KeyDowngradeException import app.passwordstore.crypto.errors.KeyNotFoundException import app.passwordstore.crypto.errors.NoKeysAvailableException import app.passwordstore.crypto.errors.UnusableKeyException @@ -176,6 +177,16 @@ class PGPKeyManagerTest { assertTrue(keyManager.addKey(publicKey).isErr) } + @Test + fun replaceSecretKeyWithPublicKeyIsRefusedEvenWithReplace() = + runTest(dispatcher) { + keyManager.addKey(secretKey).unwrap() + assertIs(keyManager.addKey(publicKey, replace = true).unwrapError()) + assertIs(keyManager.addKey(publicKey, replace = false).unwrapError()) + // The private key is still the stored one. + assertContentEquals(secretKey.contents, keyManager.getAllKeys().unwrap().single().contents) + } + @Test fun replacePublicKeyWithSecretKey() = runTest(dispatcher) { diff --git a/docs/superpowers/plans/2026-11-03-compose-setup.md b/docs/superpowers/plans/2026-11-03-compose-setup.md index e4bcd5da..d62aeff6 100644 --- a/docs/superpowers/plans/2026-11-03-compose-setup.md +++ b/docs/superpowers/plans/2026-11-03-compose-setup.md @@ -1555,7 +1555,11 @@ fun GitServerDestination( viewModel.progress = R.string.git_clone_progress activity.lifecycleScope.launch { try { - viewModel.clearStore(root) + // A folder that could not be emptied is reported as such, not as a vague clone error. + viewModel.clearStore(root)?.let { failure -> + say(activity.getString(failure)) + return@launch + } activity .launchGitOperation(GitOp.CLONE) .fold( @@ -3757,7 +3761,7 @@ deleting `$OUT` if a visual check is wanted (copy them elsewhere first). `sshKeySummary`, `SshKeyType`, `SshKeyViewModel.import/generate/fail/dismissFailure/announced`, `type`, `protect`, `busy`, `version`, `imported`, `generated`, `failure` (Task 2) match Task 4; `GitServerViewModel.url/changeUrl/authMode/progress/cloned/hasHostKey/sshKey/refresh/ - clearHostKey/save/clearStore` (suspend) `/finishClone` (not suspend, launches in + clearHostKey/save/clearStore` (suspend, returns null or a failure string resource) `/finishClone` (not suspend, launches in `viewModelScope`) `/cloneEnded` (Task 2) match Task 3; `KeyRow(userId, keyId)` (no `gpgId` member any more), `KeyImport`, `PgpKeysViewModel.keys/recipients/loadRecipients/busy/ importResult/done/import/replace/dismissImport/deleteKey/createStore/markDone` (Task 2) match -- cgit v1.2.3 From 2b73e524d9624897f675ccc14f614fc450a7565f Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 3 Nov 2026 14:44:58 +0100 Subject: fix(app): address the step 5 review and emulator findings - A store a cancelled attempt already made for the chosen key (its .gpg-id names exactly that key) no longer counts as occupied: choosing the key again commits and moves on instead of refusing. - SSH key import reads at most 100 kB whatever the provider reports, like the PGP import, instead of reading the stream whole. - PGPKeyManager replaces a key through a temporary file moved over it atomically, so a failed write keeps the old key; listings only read .key files, so a stray temporary file is never taken for a key. - "View generated public SSH key" goes through the settings result launcher, so the preference visibility refreshes afterwards. - A plain start from LaunchActivity replaces the task: an open MainActivity (e.g. one whose store vanished) is no longer left below the new one, and one back leaves the app. - The PGP keys screen's import FAB carries its content description itself; M3 cleared the icon's. Co-Authored-By: Claude Opus 5.5 --- .../app/passwordstore/ui/main/LaunchActivity.kt | 3 ++ .../ui/settings/RepositorySettings.kt | 2 +- .../app/passwordstore/ui/setup/PgpKeysScreen.kt | 14 ++++++---- .../app/passwordstore/ui/setup/PgpKeysViewModel.kt | 10 +++++-- .../java/app/passwordstore/ui/setup/SetupModel.kt | 7 +++++ .../java/app/passwordstore/util/git/sshj/SshKey.kt | 14 +++++++++- .../app/passwordstore/ui/setup/SetupModelTest.kt | 10 +++++++ .../app/passwordstore/crypto/PGPKeyManager.kt | 32 ++++++++++++++++++---- .../app/passwordstore/crypto/PGPKeyManagerTest.kt | 5 ++++ 9 files changed, 81 insertions(+), 16 deletions(-) (limited to 'crypto') diff --git a/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt b/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt index befd37f9..66b5cf1f 100644 --- a/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt +++ b/app/src/main/java/app/passwordstore/ui/main/LaunchActivity.kt @@ -56,6 +56,9 @@ class LaunchActivity : AppCompatActivity() { val intentToStart = Intent(this, MainActivity::class.java) if (intent.action == ACTION_DECRYPT_PASS && isFileInRepo(filePath, repoDir)) intentToStart.putExtra(BasePGPActivity.EXTRA_FILE_PATH, filePath) + // A plain start replaces the task, so an instance already open (one whose store vanished, say) + // is not left below the new one. An entry opens on top, back returns to where the user was. + else intentToStart.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK) startActivity(intentToStart) Handler(Looper.getMainLooper()).postDelayed({ finish() }, if (noAuth) 0L else 500L) 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 af50b3ea..23dd2829 100644 --- a/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt +++ b/app/src/main/java/app/passwordstore/ui/settings/RepositorySettings.kt @@ -110,7 +110,7 @@ class RepositorySettings(private val activity: FragmentActivity) : SettingsProvi titleRes = R.string.pref_ssh_see_key_title visible = PasswordRepository.isGitRepo() && SshKey.canShowSshPublicKey onClick { - activity.startActivity(MainActivity.setupIntent(activity, SetupStart.Ssh)) + generateSshKey.launch(MainActivity.setupIntent(activity, SetupStart.Ssh)) true } } diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt index 34e5bed6..11fd979c 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysScreen.kt @@ -43,6 +43,8 @@ import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource +import androidx.compose.ui.semantics.contentDescription +import androidx.compose.ui.semantics.semantics import androidx.compose.ui.text.font.FontFamily import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp @@ -121,11 +123,13 @@ fun PgpKeysScreen( ) }, floatingActionButton = { - // M3 clears the label's semantics; the FAB takes its description from the icon. + // M3 clears the semantics of the FAB's content, label and icon alike: describe the FAB. + val label = stringResource(R.string.pref_import_pgp_key_title) ExtendedFloatingActionButton( - text = { Text(stringResource(R.string.pref_import_pgp_key_title)) }, - icon = { Icon(Icons.Filled.Add, stringResource(R.string.pref_import_pgp_key_title)) }, + text = { Text(label) }, + icon = { Icon(Icons.Filled.Add, null) }, onClick = { if (!busy) onImport() }, + modifier = Modifier.semantics { contentDescription = label }, ) }, ) { padding -> @@ -254,8 +258,8 @@ fun PgpKeysDestination( if (uri != null) viewModel.import(uri, activity) } - // ponytail: a theme or locale change in the middle cancels the commit; the next commit picks up - // the new `.gpg-id`, and choosing the key again re-creates nothing that would conflict. + // ponytail: a theme or locale change in the middle cancels the commit. Choosing the same key + // again finds the store already made for it (createdFor), commits the `.gpg-id` and moves on. fun choose(row: KeyRow) { if (viewModel.busy) return viewModel.busy = true diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt index d270275f..8f1b52ec 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt @@ -209,12 +209,16 @@ constructor( /** * Creates the local store at [root] for [row]: a git repository and a `.gpg-id` naming the key by * its uppercase long key ID ([gpgIdText]). Returns null, or why it failed; [occupied] is the - * reason given when [root] already holds a store. + * reason given when [root] already holds a store, unless that store was made for [row] already. */ suspend fun createStore(root: File, row: KeyRow, occupied: String): String? = withContext(dispatcherProvider.io()) { - // A restored, already-cloned store must keep its .gpg-id. - if (root.isDirectory && cloneTarget(root) == CloneTarget.Occupied) return@withContext occupied + if (root.isDirectory && cloneTarget(root) == CloneTarget.Occupied) { + // Made by an earlier attempt for this key whose commit a recreation cancelled: done. + if (createdFor(root, row.keyId)) return@withContext null + // A restored, already-cloned store must keep its .gpg-id. + return@withContext occupied + } try { check(root.isDirectory || root.mkdirs()) { "Could not create $root" } PasswordRepository.createRepository(root) diff --git a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt index ade23aa2..4e32505f 100644 --- a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt +++ b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt @@ -95,6 +95,13 @@ fun storeRecipients(text: String): List = fun gpgIdText(keys: List): String = keys.joinToString("") { "${it.toString().uppercase(Locale.ROOT)}\n" } +/** + * Whether [dir] holds a store already made for [key]: its `.gpg-id` is exactly what creating one + * for [key] writes. Blocking: call it off the main thread. + */ +fun createdFor(dir: File, key: PGPIdentifier.KeyId): Boolean = + readGpgId(dir) == gpgIdText(listOf(key)) + private const val MAX_GPG_ID_BYTES = 64 * 1024 /** diff --git a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt index 06cf5bc8..7c8f5ba5 100644 --- a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt +++ b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt @@ -16,6 +16,7 @@ import android.util.Base64 import androidx.core.content.edit import app.passwordstore.Application import app.passwordstore.R +import app.passwordstore.ui.setup.readAtMost import app.passwordstore.util.crypto.keystoreAesKey import app.passwordstore.util.crypto.requireUserAuthentication import app.passwordstore.util.crypto.seal @@ -239,7 +240,18 @@ object SshKey { val sshKeyInputStream = context.contentResolver.openInputStream(uri) ?: throw IOException(context.getString(R.string.ssh_key_does_not_exist)) - val lines = sshKeyInputStream.use { `is` -> `is`.bufferedReader().readLines() } + // Bounded whatever the provider claimed above: a larger stream is refused, not read whole. + val bytes = + sshKeyInputStream.use { readAtMost(it, 100_000) } + ?: throw IllegalArgumentException( + context.getString(R.string.ssh_key_import_error_not_an_ssh_key_message) + ) + val lines = + try { + bytes.inputStream().bufferedReader().readLines() + } finally { + bytes.fill(0) + } // Private key markers and a format sshj can read; the current key stays when this fails. if (!isSshPrivateKey(lines)) diff --git a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt index 53ea80cf..9ccddbd3 100644 --- a/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt +++ b/app/src/test/java/app/passwordstore/ui/setup/SetupModelTest.kt @@ -137,4 +137,14 @@ class SetupModelTest { val folder = File(root, "folder").apply { File(this, ".gpg-id").mkdirs() } assertNull(readGpgId(folder)) } + + @Test + fun createdForMatchesOnlyTheChosenKeysGpgId() { + val key = PGPIdentifier.fromString("0xf8993a789cdfb014") as PGPIdentifier.KeyId + assertEquals(false, createdFor(root, key)) + File(root, ".gpg-id").writeText(gpgIdText(listOf(key))) + assertEquals(true, createdFor(root, key)) + File(root, ".gpg-id").writeText("F8993A789CDFB014\nB950AE2813841585\n") + assertEquals(false, createdFor(root, key)) + } } diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt index ef58ad19..33d61343 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt @@ -22,6 +22,8 @@ import com.github.michaelbull.result.Result import com.github.michaelbull.result.coroutines.runSuspendCatching import com.github.michaelbull.result.unwrap import java.io.File +import java.nio.file.Files +import java.nio.file.StandardCopyOption import javax.inject.Inject import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.withContext @@ -51,13 +53,13 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : tryParseKeyring(PGPKey(existingKeyBytes)) ?: throw InvalidKeyException when { existingKeyRing is PGPPublicKeyRing && incomingKeyRing is PGPSecretKeyRing -> { - keyFile.writeBytes(key.contents) + writeAtomically(keyFile, key.contents) return@runSuspendCatching key } existingKeyRing is PGPPublicKeyRing && incomingKeyRing is PGPPublicKeyRing -> { val updatedPublicKey = PGPainless.mergeCertificate(existingKeyRing, incomingKeyRing) val keyBytes = PGPainless.asciiArmor(updatedPublicKey).encodeToByteArray() - keyFile.writeBytes(keyBytes) + writeAtomically(keyFile, keyBytes) return@runSuspendCatching key } // Never with replace either: it would delete the only copy of the private key. @@ -69,10 +71,9 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : throw KeyAlreadyExistsException( tryGetId(key)?.toString() ?: "Failed to retrieve key ID" ) - if (!keyFile.delete()) throw KeyDeletionFailedException } - keyFile.writeBytes(key.contents) + writeAtomically(keyFile, key.contents) key } @@ -96,7 +97,7 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : withContext(dispatcher) { runSuspendCatching { if (!keyDirExists()) throw KeyDirectoryUnavailableException - val keyFiles = keyDir.listFiles() + val keyFiles = keyDir.listFiles { file -> file.extension == KEY_EXTENSION } if (keyFiles.isNullOrEmpty()) throw NoKeysAvailableException val keys = keyFiles.map { file -> PGPKey(file.readBytes()) } @@ -134,7 +135,7 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : withContext(dispatcher) { runSuspendCatching { if (!keyDirExists()) throw KeyDirectoryUnavailableException - val keyFiles = keyDir.listFiles() + val keyFiles = keyDir.listFiles { file -> file.extension == KEY_EXTENSION } if (keyFiles.isNullOrEmpty()) return@runSuspendCatching emptyList() keyFiles.map { keyFile -> PGPKey(keyFile.readBytes()) }.toList() } @@ -143,6 +144,25 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : /** @see KeyManager.getKeyById */ override suspend fun getKeyId(key: PGPKey): PGPIdentifier? = tryGetId(key) + /** + * Writes [bytes] to a temporary file next to [file] and moves it over [file] in one step, so a + * failed write keeps the key that was there. + */ + private fun writeAtomically(file: File, bytes: ByteArray) { + val temp = File(file.parentFile, ".${file.name}.tmp") + try { + temp.writeBytes(bytes) + Files.move( + temp.toPath(), + file.toPath(), + StandardCopyOption.REPLACE_EXISTING, + StandardCopyOption.ATOMIC_MOVE, + ) + } finally { + temp.delete() + } + } + /** Checks if [keyDir] exists and attempts to create it if not. */ private fun keyDirExists(): Boolean { return keyDir.exists() || keyDir.mkdirs() diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt index 81a666f4..587f7239 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt @@ -72,6 +72,11 @@ class PGPKeyManagerTest { val keyId = keyManager.getKeyId(keyManager.addKey(secretKey, true).unwrap()) assertEquals(KeyId(CryptoConstants.KEY_ID), keyId) + // Replaced through a temporary file that does not stay behind. + assertEquals(listOf("$keyId.${PGPKeyManager.KEY_EXTENSION}"), keysDir.list()?.toList()) + // A stray temporary file left by a crash is not listed as a key. + File(keysDir, ".$keyId.${PGPKeyManager.KEY_EXTENSION}.tmp").writeText("partial") + assertEquals(1, keyManager.getAllKeys().unwrap().size) } @Test -- cgit v1.2.3