diff options
Diffstat (limited to 'crypto')
5 files changed, 77 insertions, 7 deletions
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 f85fe92c..9acd0489 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,15 @@ 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. */ + // 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 + /** * 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/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt index 6f852af4..33d61343 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 @@ -21,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 @@ -50,25 +53,27 @@ 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. + 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) throw KeyAlreadyExistsException( tryGetId(key)?.toString() ?: "Failed to retrieve key ID" ) - if (!keyFile.delete()) throw KeyDeletionFailedException } - keyFile.writeBytes(key.contents) + writeAtomically(keyFile, key.contents) key } @@ -92,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()) } @@ -130,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() } @@ -139,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/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()))) + } } 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..587f7239 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 @@ -71,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 @@ -177,6 +183,16 @@ class PGPKeyManagerTest { } @Test + fun replaceSecretKeyWithPublicKeyIsRefusedEvenWithReplace() = + runTest(dispatcher) { + keyManager.addKey(secretKey).unwrap() + assertIs<KeyDowngradeException>(keyManager.addKey(publicKey, replace = true).unwrapError()) + assertIs<KeyDowngradeException>(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) { assertTrue(keyManager.addKey(publicKey).isOk) |