Skip to content

Commit

Permalink
Git via SSH: Various fixes related to Ed25519 keys
Browse files Browse the repository at this point in the history 
Access to the master key that is used to wrap Ed25519 keys relies on
Android Keystore, Google Tink and androidx.security.crypto. We are using
alpha versions of the latter library, and it is notoriously flaky (see
e.g. tink-crypto/tink#535).

The main issue that caused me to go over this class was that on newly
created API 30 devices, a Ed25519 key with device lock protection could
never be used, even if they had just been created (i.e. the newly
created master key was probably not corrupted).

I realized that *something* had changed so that we now need to
authenticate in order to just create the private key object. This has
not been the case in my previous tests; authentication was not required
until we tried to _use_ they key to do something (in our case, sign an
arbitrary string).

Since we need to authenticate in more situations than before, I added a
function which can be called whenever we catch a
UserNotAuthenticatedException.

Concrete changes:

- Upgrade androidx.security.crypto from 1.1.0-alpha06 (which relies on a
  newer version of Google Tink, which is hopefully more stable). Note
  that this requires compiling against SDK >= 33, but I believe that
  should be fine.
- When preparing to use a Ed25519 key for signing, use a signing
  algorithm which actually works with those keys. This has been causing
  an InvalidKeyException during each use of a Ed25519 for a long time.
- Catch all UserNotAuthenticatedExceptions and try biometric auth when
  they occur.
- Remove the mustAuthenticate attribute, as it becomes useless now that
  we rely on catching UserNotAuthenticatedExceptions.
- Make privateKeyLoadAttempts a class attribute which can be updated
  from many places.
- Don't swallow and silently log all other exception types when failing
  to use a key for signing. We want to know when (yes, when, not if...)
  this code breaks again.
- Clean up duplicated RequiresApi annotations.
  • Loading branch information
@amberin
amberin committed Feb 13, 2024
1 parent 1897263 commit 038851d
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 91 deletions.
2 changes: 1 addition & 1 deletion app/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ apply plugin: 'kotlin-kapt'
android {
namespace 'com.orgzly'

compileSdkVersion 32
compileSdkVersion 33

defaultConfig {
def application_id = "com.orgzlyrevived"
Expand Down
143 changes: 54 additions & 89 deletions app/src/main/java/com/orgzly/android/git/SshKey.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,8 @@ import android.content.Intent
import android.content.pm.PackageManager
import android.os.Build
import android.security.keystore.KeyGenParameterSpec
import android.security.keystore.KeyInfo
import android.security.keystore.KeyProperties
import android.security.keystore.UserNotAuthenticatedException
import android.util.Log
import androidx.annotation.RequiresApi
import androidx.core.content.edit
import androidx.security.crypto.EncryptedFile
Expand All @@ -22,6 +20,7 @@ import com.orgzly.android.util.BiometricAuthenticator
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import kotlinx.coroutines.withContext
import net.i2p.crypto.eddsa.EdDSAEngine
import net.i2p.crypto.eddsa.EdDSAPrivateKey
import net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable
import net.i2p.crypto.eddsa.spec.EdDSAPrivateKeySpec
Expand All @@ -31,8 +30,6 @@ import java.io.File
import java.io.IOException
import java.security.*
import java.security.interfaces.RSAKey
import javax.crypto.SecretKey
import javax.crypto.SecretKeyFactory

private const val PROVIDER_ANDROID_KEY_STORE = "AndroidKeyStore"
private const val KEYSTORE_ALIAS = "orgzly_sshkey"
Expand Down Expand Up @@ -60,63 +57,29 @@ fun toSshPublicKey(publicKey: PublicKey): String {
return PublicKeyEntry.toString(publicKey)
}

@RequiresApi(Build.VERSION_CODES.N)
object SshKey {
private val TAG = SshKey::class.java.name
val sshPublicKey
get() = if (publicKeyFile.exists()) publicKeyFile.readText() else null
val canShowSshPublicKey
get() = type in listOf(Type.KeystoreNative, Type.KeystoreWrappedEd25519)
val exists
get() = type != null
private val mustAuthenticate: Boolean
@RequiresApi(Build.VERSION_CODES.N)
get() {
return runCatching {
if (type !in listOf(Type.KeystoreNative, Type.KeystoreWrappedEd25519)) return false
when (val key = androidKeystore.getKey(KEYSTORE_ALIAS, null)) {
is PrivateKey -> {
val factory =
KeyFactory.getInstance(key.algorithm, PROVIDER_ANDROID_KEY_STORE)
return factory.getKeySpec(
key,
KeyInfo::class.java
).isUserAuthenticationRequired
}
is SecretKey -> {
val factory =
SecretKeyFactory.getInstance(key.algorithm, PROVIDER_ANDROID_KEY_STORE)
(factory.getKeySpec(
key,
KeyInfo::class.java
) as KeyInfo).isUserAuthenticationRequired
}
else -> throw IllegalStateException("SSH key does not exist in Keystore")
}
}
.getOrElse {
// It is fine to swallow the exception here since it will reappear when the key
// is used for SSH authentication and can then be shown in the UI.
false
}
}

private val context: Context
get() = App.getAppContext()

private val privateKeyFile
get() = File(context.filesDir, "ssh_key")
private val publicKeyFile
get() = File(context.filesDir, "ssh_key.pub")

private var type: Type?
get() = Type.fromValue(AppPreferences.gitSshKeyType(context))
set(value) = AppPreferences.gitSshKeyType(context, value?.value)

private val isStrongBoxSupported by unsafeLazy {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.P)
context.packageManager.hasSystemFeature(PackageManager.FEATURE_STRONGBOX_KEYSTORE)
else false
}
private var privateKeyLoadAttempts = 0

private enum class Type(val value: String) {
KeystoreNative("keystore_native"),
Expand All @@ -128,7 +91,6 @@ object SshKey {
}
}

@RequiresApi(Build.VERSION_CODES.N)
enum class Algorithm(
val algorithm: String,
val applyToSpec: KeyGenParameterSpec.Builder.() -> Unit
Expand Down Expand Up @@ -217,7 +179,6 @@ object SshKey {
type = Type.KeystoreWrappedEd25519
}

@RequiresApi(Build.VERSION_CODES.N)
fun generateKeystoreNativeKey(algorithm: Algorithm, requireAuthentication: Boolean) {
delete()

Expand Down Expand Up @@ -248,10 +209,9 @@ object SshKey {
type = Type.KeystoreNative
}

@RequiresApi(Build.VERSION_CODES.N)
fun getKeyPair(): KeyPair {
privateKeyLoadAttempts = 0
var privateKey: PrivateKey? = null
var privateKeyLoadAttempts = 0
val publicKey: PublicKey? = when (type) {
Type.KeystoreNative -> {
kotlin.runCatching { androidKeystore.sshPublicKey }
Expand Down Expand Up @@ -282,70 +242,75 @@ object SshKey {
}
}
Type.KeystoreWrappedEd25519 -> {
runCatching {
// The current MasterKey API does not allow getting a reference to an existing
// one without specifying the KeySpec for a new one. However, the value for
// passed here for `requireAuthentication` is not used as the key already exists
// at this point.
val encryptedPrivateKeyFile = runBlocking {
getOrCreateWrappedPrivateKeyFile(false)
}
val rawPrivateKey =
encryptedPrivateKeyFile.openFileInput().use { it.readBytes() }
EdDSAPrivateKey(
EdDSAPrivateKeySpec(
rawPrivateKey, EdDSANamedCurveTable.ED_25519_CURVE_SPEC
)
)
}.getOrElse { error ->
throw IOException(context.getString(R.string.ssh_key_failed_get_private), error)
try {
tryToReadEd25519PrivateKey()
} catch (e: UserNotAuthenticatedException) {
tryBiometricAuthOrFail(e)
tryToReadEd25519PrivateKey()
} catch (e: Exception) {
throw IOException(context.getString(R.string.ssh_key_failed_get_private), e)
}
}
else -> throw IllegalStateException("SSH key does not exist in Keystore")
}
try {
// Try to sign something to see if the key is unlocked
val algorithm: String = if (privateKey is RSAKey) {
"SHA256withRSA"
} else {
"SHA256withECDSA"
val signature = when (privateKey) {
is EdDSAPrivateKey -> EdDSAEngine(MessageDigest.getInstance(
EdDSANamedCurveTable.getByName(EdDSANamedCurveTable.ED_25519).hashAlgorithm))
is RSAKey -> Signature.getInstance("SHA256withRSA")
else -> Signature.getInstance("SHA256withECDSA")
}
Signature.getInstance(algorithm).apply {
signature.apply {
initSign(privateKey)
update("loremipsum".toByteArray())
}.sign()
// The key is unlocked; exit the loop.
break
} catch (e: UserNotAuthenticatedException) {
if (privateKeyLoadAttempts > 0) {
// We expect this exception before trying auth, but after that, this means
// we have failed to unlock the SSH key.
Log.e(TAG, context.getString(R.string.ssh_key_failed_unlock_private), e)
throw IOException(context.getString(R.string.ssh_key_failed_unlock_private))
}
tryBiometricAuthOrFail(e)
} catch (e: Exception) {
// Our attempt to use the key for signing may go wrong in many unforeseen ways.
// Such failures are unimportant.
e.printStackTrace()
throw e
}
if (mustAuthenticate && privateKeyLoadAttempts == 0) {
// Time to try biometric auth
val currentActivity = App.getCurrentActivity()
checkNotNull(currentActivity) {
throw IOException(context.getString(R.string.ssh_key_locked_and_no_activity))
}
val biometricAuthenticator = BiometricAuthenticator(currentActivity)
runBlocking(Dispatchers.Main) {
biometricAuthenticator.authenticate(
context.getString(
R.string.biometric_prompt_title_unlock_ssh_key
)
}
return KeyPair(publicKey, privateKey)
}

private fun tryToReadEd25519PrivateKey(): EdDSAPrivateKey {
// The current MasterKey API does not allow getting a reference to an existing
// one without specifying the KeySpec for a new one. However, the value for
// passed here for `requireAuthentication` is not used as the key already exists
// at this point.
val encryptedPrivateKeyFile = runBlocking {
getOrCreateWrappedPrivateKeyFile(false)
}
val rawPrivateKey =
encryptedPrivateKeyFile.openFileInput().use { it.readBytes() }
return EdDSAPrivateKey(
EdDSAPrivateKeySpec(
rawPrivateKey, EdDSANamedCurveTable.ED_25519_CURVE_SPEC
)
)
}

private fun tryBiometricAuthOrFail(e: UserNotAuthenticatedException) {
if (privateKeyLoadAttempts == 0) {
val currentActivity = App.getCurrentActivity()
checkNotNull(currentActivity) {
throw IOException(context.getString(R.string.ssh_key_locked_and_no_activity))
}
val biometricAuthenticator = BiometricAuthenticator(currentActivity)
runBlocking(Dispatchers.Main) {
biometricAuthenticator.authenticate(
context.getString(
R.string.biometric_prompt_title_unlock_ssh_key
)
}
)
}
privateKeyLoadAttempts++
} else {
throw e
}
return KeyPair(publicKey, privateKey)
}

@RequiresApi(Build.VERSION_CODES.N)
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ buildscript {

versions.jgit = '5.13.3.202401111512-r'

versions.security_crypto = '1.1.0-alpha03'
versions.security_crypto = '1.1.0-alpha06'

versions.biometric_ktx = '1.2.0-alpha04'

Expand Down

0 comments on commit 038851d

Please sign in to comment.