From fd83560852fef0fc16305b383c08369a1c9f6cb2 Mon Sep 17 00:00:00 2001 From: nrobi144 Date: Tue, 6 Jan 2026 17:08:28 +0200 Subject: [PATCH] key storage implementation --- .../commons/account/AccountManager.kt | 15 +- .../vitorpamplona/amethyst/desktop/Main.kt | 7 +- .../amethyst/desktop/ui/LoginScreen.kt | 9 +- .../nip01Core/crypto/SecureKeyStorage.kt | 37 ++- .../nip01Core/crypto/SecureKeyStorage.kt | 40 +++- .../nip01Core/crypto/SecureKeyStorage.kt | 224 ++++++++++++++---- 6 files changed, 274 insertions(+), 58 deletions(-) diff --git a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/account/AccountManager.kt b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/account/AccountManager.kt index 1b9321077..3afa19761 100644 --- a/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/account/AccountManager.kt +++ b/commons/src/jvmAndroid/kotlin/com/vitorpamplona/amethyst/commons/account/AccountManager.kt @@ -47,9 +47,22 @@ sealed class AccountState { ) : AccountState() } -class AccountManager( +class AccountManager private constructor( private val secureStorage: SecureKeyStorage, ) { + companion object { + /** + * Creates an AccountManager instance. + * + * @param context Platform-specific context (required on Android, ignored on Desktop) + * @return AccountManager instance + */ + fun create(context: Any? = null): AccountManager { + val storage = SecureKeyStorage.create(context) + return AccountManager(storage) + } + } + private val _accountState = MutableStateFlow(AccountState.LoggedOut) val accountState: StateFlow = _accountState.asStateFlow() diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt index c442390d3..eaf54d7d5 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/Main.kt @@ -85,7 +85,6 @@ import com.vitorpamplona.amethyst.desktop.ui.FeedScreen import com.vitorpamplona.amethyst.desktop.ui.LoginScreen import com.vitorpamplona.amethyst.desktop.ui.NotificationsScreen import com.vitorpamplona.amethyst.desktop.ui.UserProfileScreen -import com.vitorpamplona.quartz.nip01Core.crypto.SecureKeyStorage import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.SupervisorJob @@ -213,14 +212,14 @@ fun App( ) { var currentScreen by remember { mutableStateOf(DesktopScreen.Feed) } val relayManager = remember { DesktopRelayConnectionManager() } - val secureStorage = remember { SecureKeyStorage() } - val accountManager = remember { AccountManager(secureStorage) } + val accountManager = remember { AccountManager.create() } val accountState by accountManager.accountState.collectAsState() val scope = remember { CoroutineScope(SupervisorJob() + Dispatchers.Main) } // Try to load saved account on startup DisposableEffect(Unit) { - scope.launch { + scope.launch(Dispatchers.IO) { + // Load account on IO dispatcher to avoid blocking UI with password prompt (readLine) accountManager.loadSavedAccount() } diff --git a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt index d32026cef..f194564ae 100644 --- a/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt +++ b/desktopApp/src/jvmMain/kotlin/com/vitorpamplona/amethyst/desktop/ui/LoginScreen.kt @@ -41,6 +41,7 @@ import com.vitorpamplona.amethyst.commons.account.AccountManager import com.vitorpamplona.amethyst.commons.account.AccountState import com.vitorpamplona.amethyst.commons.ui.auth.LoginCard import com.vitorpamplona.amethyst.commons.ui.auth.NewKeyWarningCard +import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch @Composable @@ -76,8 +77,8 @@ fun LoginScreen( LoginCard( onLogin = { keyInput -> accountManager.loginWithKey(keyInput).map { - // Save account to secure storage - scope.launch { + // Save account to secure storage (use IO dispatcher to avoid blocking UI) + scope.launch(Dispatchers.IO) { accountManager.saveCurrentAccount() onLoginSuccess() } @@ -96,8 +97,8 @@ fun LoginScreen( nsec = generatedAccount!!.nsec, onContinue = { showNewKeyDialog = false - // Save generated account - scope.launch { + // Save generated account (use IO dispatcher to avoid blocking UI) + scope.launch(Dispatchers.IO) { accountManager.saveCurrentAccount() onLoginSuccess() } diff --git a/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt b/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt index f6a8754aa..771c7e971 100644 --- a/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt +++ b/quartz/src/androidMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt @@ -29,25 +29,50 @@ import kotlinx.coroutines.withContext /** * Android implementation of SecureKeyStorage using EncryptedSharedPreferences * backed by Android Keystore (AES-256-GCM, hardware-backed when available). + * + * ## Security Features + * + * - **Hardware Security:** Uses Android Keystore (hardware-backed on supported devices with StrongBox) + * - **Encryption:** AES-256-GCM for both keys and values + * - **Key Derivation:** AES-256-SIV for preference keys, AES-256-GCM for values + * - **Application Context:** Uses applicationContext to prevent memory leaks + * - **Auto-backup Disabled:** EncryptedSharedPreferences automatically excluded from cloud backups + * + * **Note:** While the encryption keys are protected by hardware security modules (when available), + * the decrypted private keys returned by [getPrivateKey] are still subject to the String memory + * limitation described in [SecureKeyStorage]. */ -actual class SecureKeyStorage( - private val context: Context, -) { - companion object { +actual class SecureKeyStorage private actual constructor() { + actual companion object { private const val PREFS_NAME = "amethyst_secure_keys" private const val KEY_PREFIX = "privkey_" + + private lateinit var appContext: Context + + /** + * Creates a SecureKeyStorage instance for Android. + * + * @param context Android Context (will use applicationContext to avoid leaks) + * @return SecureKeyStorage instance + * @throws IllegalArgumentException if context is null or not a valid Context + */ + actual fun create(context: Any?): SecureKeyStorage { + require(context is Context) { "Android requires a valid Context" } + appContext = context.applicationContext + return SecureKeyStorage() + } } private val masterKey: MasterKey by lazy { MasterKey - .Builder(context, MasterKey.DEFAULT_MASTER_KEY_ALIAS) + .Builder(appContext, MasterKey.DEFAULT_MASTER_KEY_ALIAS) .setKeyScheme(MasterKey.KeyScheme.AES256_GCM) .build() } private val encryptedPrefs by lazy { EncryptedSharedPreferences.create( - context, + appContext, PREFS_NAME, masterKey, EncryptedSharedPreferences.PrefKeyEncryptionScheme.AES256_SIV, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt index b7cd8018b..d94171404 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt @@ -23,10 +23,41 @@ package com.vitorpamplona.quartz.nip01Core.crypto /** * Secure storage for private keys using platform-specific secure storage mechanisms. * - * Android: Android Keystore + EncryptedDataStore (AES-256-GCM, StrongBox when available) - * Desktop: OS native credential managers (macOS Keychain, Windows Credential Manager, Linux Secret Service) + * ## Platform Implementations + * + * **Android:** Android Keystore + EncryptedSharedPreferences (AES-256-GCM, StrongBox when available) + * + * **Desktop:** OS native credential managers (macOS Keychain, Windows Credential Manager, Linux Secret Service) + * with encrypted file fallback. + * + * ## Security Considerations + * + * **String Memory Limitation:** Private keys are returned as String (hexadecimal format). Unlike char arrays, + * Kotlin/JVM Strings are immutable and cannot be securely zeroed from memory after use. The private key + * will remain in memory until garbage collected, which may be delayed. This is an inherent limitation of + * the JVM platform. + * + * **Mitigation:** Private keys are only exposed through this API when needed for signing operations. + * They are encrypted at rest on all platforms. For maximum security, minimize the time private keys + * are held in memory and ensure they are dereferenced promptly after use. + * + * **Production Recommendation:** For high-security applications, consider using hardware security modules + * (HSM) or secure enclaves (Android StrongBox, iOS Secure Enclave) that never expose private keys to + * application memory. + * + * Use [SecureKeyStorage.create] factory method to create instances. */ -expect class SecureKeyStorage { +expect class SecureKeyStorage private constructor() { + companion object { + /** + * Creates a SecureKeyStorage instance. + * + * @param context Platform-specific context (required on Android, ignored on Desktop) + * @return SecureKeyStorage instance + */ + fun create(context: Any? = null): SecureKeyStorage + } + /** * Saves a private key securely for the given npub. * @@ -42,6 +73,9 @@ expect class SecureKeyStorage { /** * Retrieves a private key for the given npub. * + * **Security Warning:** The returned String cannot be securely zeroed from memory (JVM limitation). + * Dereference the returned value immediately after use to minimize exposure time. + * * @param npub The public key in npub (Bech32) format * @return The private key in hexadecimal format, or null if not found * @throws SecureStorageException if retrieval operation fails diff --git a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt index a8a7537f9..f64a8a755 100644 --- a/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt +++ b/quartz/src/jvmMain/kotlin/com/vitorpamplona/quartz/nip01Core/crypto/SecureKeyStorage.kt @@ -24,8 +24,14 @@ import com.github.javakeyring.BackendNotSupportedException import com.github.javakeyring.Keyring import com.github.javakeyring.PasswordAccessException import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import java.io.File +import java.io.RandomAccessFile +import java.nio.file.Files +import java.nio.file.StandardCopyOption +import java.nio.file.attribute.PosixFilePermission import java.security.SecureRandom import java.util.Base64 import javax.crypto.Cipher @@ -40,9 +46,30 @@ import javax.crypto.spec.SecretKeySpec * * Falls back to encrypted file storage with user-provided password if OS keyring * is unavailable. + * + * ## Fallback Storage Security + * + * When OS keyring is unavailable, the implementation uses: + * - **Encryption:** AES-256-GCM with PBKDF2 (100k iterations) + * - **File Permissions:** Owner-only read/write (600 for files, 700 for directories) on Unix systems + * - **Atomic Writes:** Temp file + atomic move to prevent corruption + * - **File Locking:** Prevents concurrent access race conditions + * + * **Password Memory Limitation:** The fallback password is stored as a String and cannot be + * securely zeroed from memory. It remains cached for the application lifetime to avoid repeated + * password prompts. This is acceptable for desktop applications where the user's session is + * already trusted, but may not be suitable for shared/multi-user systems. */ -actual class SecureKeyStorage { - companion object { +actual class SecureKeyStorage private actual constructor() { + actual companion object { + /** + * Creates a SecureKeyStorage instance for Desktop. + * + * @param context Ignored on Desktop (no context needed) + * @return SecureKeyStorage instance + */ + actual fun create(context: Any?): SecureKeyStorage = SecureKeyStorage() + private const val SERVICE_NAME = "amethyst-desktop" private const val FALLBACK_DIR = ".amethyst" private const val FALLBACK_FILE = "keys.enc" @@ -57,6 +84,7 @@ actual class SecureKeyStorage { private var keyringAvailable: Boolean = true private var fallbackPassword: String? = null + private val fallbackMutex = Mutex() // Protects concurrent access to fallback file actual suspend fun savePrivateKey( npub: String, @@ -143,52 +171,78 @@ actual class SecureKeyStorage { } // Fallback encrypted file storage - private fun saveToFallback( + private suspend fun saveToFallback( npub: String, privKeyHex: String, ) { - val password = getFallbackPassword() - val encrypted = encryptData(privKeyHex, password) + fallbackMutex.withLock { + val password = getFallbackPassword() + val encrypted = encryptData(privKeyHex, password) - val fallbackFile = getFallbackFile() - val data = loadFallbackData().toMutableMap() - data[npub] = encrypted + val fallbackFile = getFallbackFile() - fallbackFile.parentFile?.mkdirs() - fallbackFile.writeText(data.entries.joinToString("\n") { "${it.key}:${it.value}" }) - } + // Create directory with restrictive permissions + fallbackFile.parentFile?.let { dir -> + if (!dir.exists()) { + dir.mkdirs() + setRestrictivePermissions(dir) + } + } - private fun getFromFallback(npub: String): String? { - val password = fallbackPassword ?: return null // No password set yet - val data = loadFallbackData() - val encrypted = data[npub] ?: return null - - return try { - decryptData(encrypted, password) - } catch (e: Exception) { - null - } - } - - private fun deleteFromFallback(npub: String): Boolean { - val fallbackFile = getFallbackFile() - if (!fallbackFile.exists()) return false - - val data = loadFallbackData().toMutableMap() - val existed = data.remove(npub) != null - - if (existed) { - if (data.isEmpty()) { - fallbackFile.delete() - } else { - fallbackFile.writeText(data.entries.joinToString("\n") { "${it.key}:${it.value}" }) + withFileLock(fallbackFile) { + val data = loadFallbackDataUnsafe().toMutableMap() + data[npub] = encrypted + atomicWriteFallbackData(fallbackFile, data) } } - - return existed } - private fun loadFallbackData(): Map { + private suspend fun getFromFallback(npub: String): String? { + val password = fallbackPassword ?: return null // No password set yet + + return fallbackMutex.withLock { + val fallbackFile = getFallbackFile() + if (!fallbackFile.exists()) return@withLock null + + withFileLock(fallbackFile) { + val data = loadFallbackDataUnsafe() + val encrypted = data[npub] ?: return@withFileLock null + + try { + decryptData(encrypted, password) + } catch (e: Exception) { + null + } + } + } + } + + private suspend fun deleteFromFallback(npub: String): Boolean { + return fallbackMutex.withLock { + val fallbackFile = getFallbackFile() + if (!fallbackFile.exists()) return@withLock false + + withFileLock(fallbackFile) { + val data = loadFallbackDataUnsafe().toMutableMap() + val existed = data.remove(npub) != null + + if (existed) { + if (data.isEmpty()) { + fallbackFile.delete() + } else { + atomicWriteFallbackData(fallbackFile, data) + } + } + + existed + } + } + } + + /** + * Loads fallback data without locking. Caller must hold mutex and file lock. + */ + private fun loadFallbackDataUnsafe(): Map { val fallbackFile = getFallbackFile() if (!fallbackFile.exists()) return emptyMap() @@ -200,6 +254,56 @@ actual class SecureKeyStorage { }.toMap() } + /** + * Atomically writes fallback data using temp file + rename. + */ + private fun atomicWriteFallbackData( + fallbackFile: File, + data: Map, + ) { + val tempFile = File(fallbackFile.parentFile, "${fallbackFile.name}.tmp") + try { + // Write to temp file + tempFile.writeText(data.entries.joinToString("\n") { "${it.key}:${it.value}" }) + setRestrictivePermissions(tempFile) + + // Atomic rename + Files.move( + tempFile.toPath(), + fallbackFile.toPath(), + StandardCopyOption.ATOMIC_MOVE, + StandardCopyOption.REPLACE_EXISTING, + ) + } finally { + // Clean up temp file if it still exists + if (tempFile.exists()) { + tempFile.delete() + } + } + } + + /** + * Executes block with file lock held. + */ + private fun withFileLock( + file: File, + block: () -> T, + ): T { + // Ensure lock file exists + val lockFile = File(file.parentFile, "${file.name}.lock") + lockFile.parentFile?.mkdirs() + if (!lockFile.exists()) { + lockFile.createNewFile() + setRestrictivePermissions(lockFile) + } + + return RandomAccessFile(lockFile, "rw").use { raf -> + raf.channel.lock().use { lock -> + block() + } + } + } + private fun getFallbackFile(): File { val homeDir = System.getProperty("user.home") return File(homeDir, "$FALLBACK_DIR/$FALLBACK_FILE") @@ -208,12 +312,52 @@ actual class SecureKeyStorage { private fun getFallbackPassword(): String { if (fallbackPassword == null) { println("OS keyring not available. Fallback encrypted storage requires a password.") - print("Enter master password: ") - fallbackPassword = readLine() ?: throw SecureStorageException("Password required for fallback storage") + val console = System.console() + fallbackPassword = + if (console != null) { + // Use Console.readPassword() for masked input + val password = console.readPassword("Enter master password: ") + password?.let { + val str = String(it) + it.fill('\u0000') // Clear the char array from memory + str + } ?: throw SecureStorageException("Password required for fallback storage") + } else { + // Fallback for non-interactive environments (testing, etc.) + print("Enter master password: ") + readLine() ?: throw SecureStorageException("Password required for fallback storage") + } } return fallbackPassword!! } + private fun setRestrictivePermissions(file: File) { + try { + val path = file.toPath() + // Set owner-only read/write permissions (600 for files, 700 for directories) + val permissions = + if (file.isDirectory) { + setOf( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE, + ) + } else { + setOf( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE, + ) + } + Files.setPosixFilePermissions(path, permissions) + } catch (e: UnsupportedOperationException) { + // Windows doesn't support POSIX permissions - file system security handles this + // No action needed + } catch (e: Exception) { + // Log but don't fail - permissions are a security enhancement, not critical + System.err.println("Warning: Could not set restrictive file permissions: ${e.message}") + } + } + private fun encryptData( plaintext: String, password: String,