Let SshKeyGenFragment use the Android Keystore

[fmeum]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Note: This is ready for review, but requires a patched SSHJ dependency since version 0.30.0 hasn't been released yet. Once it is and the review is done, I will remove the .jar and depend on the Maven repository.

Implement an SSH auth method backed by an Android Keystore key and make SshKeyGenFragment generate such a key.

In detail, this means the following:

• SSH keys generated within the app are no longer kept in a simple file optionally encrypted with a passphrase, but rather in the Android Keystore optionally protected with the user's screen lock credential. On modern devices with a TEE this means that the private key will not be leaked even if the device is rooted or compromised remotely.
• SSH key files with passphrases remain supported for users who prefer them, they just can't be generated from within the app anymore.
• The offered key types are changed from RSA-2048 and RSA-4096 (default) to RSA-2048, RSA-3072 and P-384 (default) for the following reasons:
  • RSA-2048 is by far the most widely supported key type and reasonably fast. Although it is no longer recommended to be used for security needs that extend into the 2030s, this does not matter too much in the context of SSH authentication. It's a keep, but should not be the default.
  • RSA-3072 will remain secure for at least a decade or two and can't possibly be backdoored by the NSA, which likely appeals to a good part of our userbase.
  • RSA-4096, the previous default, is unreasonably slow. I am open to be convinced of its usefulness, but so far fail to see why this algorithm would ever be a good fit for authentication (I could see this being useful for long-term encryption though).
  • P-384 is both very fast and recommended for long-term security, which makes it a great default option. Of course not everybody will be happy with this (or any other NIST-supplied) curve, but for SSH authentication against a pass git repository, I don't see any actual issues.
• Small changes were made to BiometricAuthenticator to fix one of two memory leaks.
• I needed to duplicate some of SSHJ's code, but luckily its Apache 2.0 license is compatible with the GPL 3.

Note: SSHJ so far doesn't support SHA-256/512 signatures with ssh-rsa key types. This will probably be fixed upstream at some point, which is why we allow these signature algorithms to be used with our Keystore keys. Again, in the context of SSH authentication, relying on SHA1 is not a major issue.

💡 Motivation and Context

We were not making use of the extensive Android capabilities around cryptography and had to rely on Jsch to generate SSH keys for us.

💚 How did you test it?

I generated and used both RSA and EC keys, please do the same.

📝 Checklist

• I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
• I reviewed submitted code

🔮 Next steps

📸 Screenshots / GIFs

Screenshots

[fmeum]

I should probably that the reason ssh-ed25519 is not offered is purely because it is not yet supported by the Android Keystore.

[msfjarvis]

LGTM. Needs a release notes entry.

[msfjarvis]

Release builds crash for me with java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable;, adding a keep rule to proguard-rules.pro didn't have any effect.

diff --git app/proguard-rules.pro app/proguard-rules.pro
index 474b45d0065d..b6958c965e0f 100644
--- app/proguard-rules.pro
+++ app/proguard-rules.pro
@@ -27,3 +27,4 @@
 -keep class org.bouncycastle.jcajce.provider.** { *; }
 -keep class org.bouncycastle.jce.provider.** { *; }
 -keep class !org.bouncycastle.jce.provider.X509LDAPCertStoreSpi { *; }
+-keep class net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable { *; }

[fmeum]

Release builds crash for me with java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable;, adding a keep rule to proguard-rules.pro didn't have any effect.

diff --git app/proguard-rules.pro app/proguard-rules.pro
index 474b45d0065d..b6958c965e0f 100644
--- app/proguard-rules.pro
+++ app/proguard-rules.pro
@@ -27,3 +27,4 @@
 -keep class org.bouncycastle.jcajce.provider.** { *; }
 -keep class org.bouncycastle.jce.provider.** { *; }
 -keep class !org.bouncycastle.jce.provider.X509LDAPCertStoreSpi { *; }
+-keep class net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable { *; }

That seems to be an issue with the filesDir style of dependency declaration. I can only fix that by manually adding the dependencies of SSHJ to our build.gradle. But of course this will not be necessary once 0.30.0 is released.

[msfjarvis]

Release builds crash for me with java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable;, adding a keep rule to proguard-rules.pro didn't have any effect.

```diff

diff --git app/proguard-rules.pro app/proguard-rules.pro
index 474b45d0065d..b6958c965e0f 100644
--- app/proguard-rules.pro
+++ app/proguard-rules.pro
@@ -27,3 +27,4 @@
-keep class org.bouncycastle.jcajce.provider.** { ; }
-keep class org.bouncycastle.jce.provider.* { *; }
-keep class !org.bouncycastle.jce.provider.X509LDAPCertStoreSpi { *; }
+-keep class net.i2p.crypto.eddsa.spec.EdDSANamedCurveTable { *; }

That seems to be an issue with the filesDir style of dependency declaration. I can only fix that by manually adding the dependencies of SSHJ to our build.gradle. But of course this will not be necessary once 0.30.0 is released.

Can probably just apply the shadow plugin and build a 'fat JAR' in the mean time.

[fmeum]

I just looked into how we do password authentication via SSH, but am confused: Is this function

Android-Password-Store/app/src/main/java/com/zeapo/pwdstore/git/GitOperation.kt

Line 55 in a8f6669

internal open fun setAuthentication(username: String, password: String): GitOperation {

dealing with password authentication both via SSH and via HTTPS? It relies on an SshSessionFactory, but the code that calls it supplies the https_password to it. I don't want to break HTTPS when I switch the backend for SSH password authentication over to SSHJ.

[msfjarvis]

I just looked into how we do password authentication via SSH, but am confused: Is this function

Android-Password-Store/app/src/main/java/com/zeapo/pwdstore/git/GitOperation.kt

Line 55 in a8f6669

internal open fun setAuthentication(username: String, password: String): GitOperation {

dealing with password authentication both via SSH and via HTTPS? It relies on an SshSessionFactory, but the code that calls it supplies the https_password to it. I don't want to break HTTPS when I switch the backend for SSH password authentication over to SSHJ.

It's using an SshSessionFactory because it looks like HTTPS was tacked on later and this was just never fixed. It should not need that factory to work.

[msfjarvis]

I can't test the ED25519 key I generated on my PC with builds off this branch, what am I missing? The crash is caused by java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable; and reproducible on both debug and release binaries.

[fmeum]

I can't test the ED25519 key I generated on my PC with builds off this branch, what am I missing? The crash is caused by java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable; and reproducible on both debug and release binaries.

I think that is because of the way I build the SSHJ jar. Could you try adding the dependency (https://github.com/hierynomus/sshj/blob/master/build.gradle#L52) manually?

[msfjarvis]

I can't test the ED25519 key I generated on my PC with builds off this branch, what am I missing? The crash is caused by java.lang.NoClassDefFoundError: Failed resolution of: Lnet/i2p/crypto/eddsa/spec/EdDSANamedCurveTable; and reproducible on both debug and release binaries.

I think that is because of the way I build the SSHJ jar. Could you try adding the dependency (hierynomus/sshj:build.gradle@master#L52) manually?

On it

[msfjarvis]

Works on release builds with the explicit dependency.

[msfjarvis]

JitPack doesn't seem to have Java 9 runners which SSHJ mandates now so we can't rely on them to be able to eliminate this JAR either. Gonna have to wait it out for an SSHJ release...

[fmeum]

I'm positive we'll get a new release soon, the current maintainer seems to be quite responsive.

[msfjarvis]

@FabianHenneke this is gonna need a rebase soon-ish, I wanted to test drive this a bit more to confirm that the opening the biometric dialog is not error-prone.

[fmeum]

I rebased and along the way removed some unnecessary getString calls.

[msfjarvis]

Not sure how well I did on the blind merge but this should be okay. I'm gonna email Jeroen and see if we can somehow expedite a 0.30.0 release.

[fmeum]

Not sure how well I did on the blind merge but this should be okay. I'm gonna email Jeroen and see if we can somehow expedite a 0.30.0 release.

Unfortunately, a release at this point wouldn't help too much: The issues hierynomus/sshj#600 and hierynomus/sshj#608 are still open, only the former has a PR to fix it. These would break the new SHA256 and SHA512 signature types and possibly the EC algorithms for us.

[fmeum]

No longer blocked on hierynomus/sshj#608 since we could work around this issue on our side.

[fmeum]

I changed the config to be compatible with the concept of key algorithms introduced in SSHJ 0.30.0.

[fmeum]

I will radically simplify the algorithm selection and remove Strongbox support since on second thought I fail to see how it has a strictly positive effect on any reasonable threat model. The new UI will briefly explain pros and cons of the only two choices (RSA 3072 and P-384).

[fmeum]

While working on v2 of this PR, I came up with an idea: We could offer ed25519 by wrapping the secret key with a Keystore-backed EncryptedFile, which is probably strictly better than having users generate the key on their computer and import it into APS. Should I add this?

[msfjarvis]

While working on v2 of this PR, I came up with an idea: We could offer ed25519 by wrapping the secret key with a Keystore-backed EncryptedFile, which is probably strictly better than having users generate the key on their computer and import it into APS. Should I add this?

Seems like a good option, certainly in favor of adding.

[fmeum]

Superseded by #1070.