Use SSHJ for SSH public key authentication

[fmeum]

📢 Type of change

• Bugfix
• New feature
• Enhancement
• Refactoring

📜 Description

Add an SSHJ backend for SSH public key and password authentication and enable it for public key authentication.

More specifically, this commit achieves the following:

• Adds a JGit SSH backend based on SSHJ, which is much more modern and well-designed than Jsch. As a result, we can now use, configure and add modern key types/ciphers/MACs.
• Enable the new backend for SSH sessions using public key authentication.
• Performs host key verification via a simple trust-on-first-use model: The host key encountered on the first connection is stored in .host_key and compared on all future connection attempts.
• GitAsyncTask is translated to Kotlin, refactored and cleaned up, which improves error messages and should simplify a future rewrite with coroutines.

There is currently a minor usability issue: If the user imports a private key in OpenSSH format (rather than the old-style PKCS8 format), the connection will fail if an incorrect passphrase is entered, there is no second passphrase prompt. This is fixed by hierynomus/sshj#587.

💡 Motivation and Context

Fixes #494 and fixes #448 for SSH public key authentication, which is by far the most common. I expect us to transition over to SSHJ for password authentication rather soon. OpenKeychain integration may take more time, so we could unlink the issues if you prefer it that way.

💚 How did you test it?

I verified that sync/pull/push work with both RSA keys generated in the app as well as imported ssh-ed25519 keys. I also verified that the host key verification is effective.

Please do test with all kinds of SSH keys and server configs, as a new backend could always lead to regressions.

📝 Checklist

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

🔮 Next steps

• Use the new backend also for SSH password authentication.
• When Make KeyType compatible with Android Keystore hierynomus/sshj#586 gets merged, explore the possibility of using the Android Keystore instead of simple files for the storage of SSH private keys.
• Replace the current SSH keygen flow relying on Jsch with a new flow based on an Android Keystore key.
• Switch OpenKeychain authentication to the new backend. This also gets easier with Make KeyType compatible with Android Keystore hierynomus/sshj#586, but could probably also be done without it.

📸 Screenshots / GIFs

Example of failed host key verification:

[fmeum]

The CI fails should now be fixed, there was a lint issue triggered by BC.

[msfjarvis]

Awesome stuff, great job on getting this done so quickly! I'll test and review this in a few hours.

[fmeum]

Please don't merge this after the review, it might be possible to improve the BouncyCastle integration and I am still looking into that.

[Skrilltrax]

@msfjarvis this good enough to start working on top of it or will there be more changes required to it first ?

[msfjarvis]

@msfjarvis this good enough to start working on top of it or will there be more changes required to it first ?

Fabian's still working on it and we need to battle test it in production before the Jsch backend can be phased out. It's a long process.

[fmeum]

Okay, this PR should be ready for review now. The BouncyCastle setup may not be ideal yet with respect to performance, but we can always revisit that later. It did perform pretty well in my tests.

@msfjarvis I rebased onto the current master, but when I restart Password Store, I just see a blank activity. Is it possible that something is amiss with the ViewBinding commits?

[msfjarvis]

Okay, this PR should be ready for review now. The BouncyCastle setup may not be ideal yet with respect to performance, but we can always revisit that later. It did perform pretty well in my tests.

👍

@msfjarvis I rebased onto the current master, but when I restart Password Store, I just see a blank activity. Is it possible that something is amiss with the ViewBinding commits?

Yeah I'm looking into it...

[msfjarvis]

Fixed that, PR coming up

[fmeum]

I ended up making one more change which moves the BC setup into Application and hopefully chooses the priorities of the security provider optimally. But who knows...

[msfjarvis]

I ended up making one more change which moves the BC setup into Application and hopefully chooses the priorities of the security provider optimally. But who knows...

The only crypto operations this app runs are these ones that we just introduced. Everything else is done by OpenKeychain who maintain their own bouncycastle fork which runs in their own app completely uncoupled from us. We should be fine.

And well, Tink has it's own thing for androidx.security which again isn't relying on Android.

[fmeum]

I ended up making one more change which moves the BC setup into Application and hopefully chooses the priorities of the security provider optimally. But who knows...

The only crypto operations this app runs are these ones that we just introduced. Everything else is done by OpenKeychain who maintain their own bouncycastle fork which runs in their own app completely uncoupled from us. We should be fine.

And well, Tink has it's own thing for androidx.security which again isn't relying on Android.

That is one aspect of it, but I also want to make sure that we use the native implementations of algorithms whenever possible since they are likely faster. The code in its current form should insert the Java BC provider at the same position the Android BC provider was at, which seems like the safest bet.

[msfjarvis]

Seems to work correctly when upgrading but I am having trouble getting a clone going on a fresh install (most probably unrelated to this) so I'll just review this based on the stuff I've tested so far and will look into the bug separately tomorrow.

[msfjarvis]

LGTM! Great job with this :)

[fmeum]

We can merge this now or wait until you had time to reproduce/resolve that bug. As you prefer.

I have received a review for one of the upstream PRs, so this might work as intended quite soon.

[msfjarvis]

We can merge this now or wait until you had time to reproduce/resolve that bug. As you prefer.

I want to confirm the PR isn't causing the regression (unlikely but just wanna be sure) so I'm gonna merge this tomorrow when I triage the bug.

I have received a review for one of the upstream PRs, so this might work as intended quite soon.

Awesome 👍

[msfjarvis]

Can't repro the bug now...