Add SSHJ backend for OpenKeychain authentication #995
Conversation
msfjarvis
left a comment
msfjarvis
left a comment
There was a problem hiding this comment.
LGTM, couple nits here and there.
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainKeyProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainKeyProvider.kt
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainWrappedKeyAlgorithmFactory.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainWrappedKeyAlgorithmFactory.kt
Outdated
Show resolved
Hide resolved
Updates sshj to 0.30.0, which brings support for rsa-sha2-* key types and bugfixes related to RSA certificates and Android Keystore backed keys. Along the way, this improves the algorithm preferences to be consistent with the Mozilla Intermediate SSH configuration (as far as possible, given that most certificate types and some encryption algorithms are not yet supported). We also add "ext-info-c" to the kex algorithm proposal to work around certain kinds of "user agent sniffing" that limits the support of rsa-sha2-* key types.
fmeum
force-pushed
the
feature/sshj_openkeychain
branch
from
9bb2726 to
703bc13
Compare
|
I just pushed an updated version. The switch away from the Git config string will be included in a different PR. |
* develop: Update sshj to 0.30.0 and improve algorithm order (#1026) Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
msfjarvis
left a comment
msfjarvis
left a comment
There was a problem hiding this comment.
LGTM, minor stylistic comments
app/src/main/java/com/zeapo/pwdstore/git/operation/GitOperation.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainKeyProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainKeyProvider.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/OpenKeychainKeyProvider.kt
Show resolved
Hide resolved
|
Thanks for the review, I hope that I addressed everything appropriately. |
msfjarvis
left a comment
msfjarvis
left a comment
There was a problem hiding this comment.
Everything looks solid, great job!
|
@FabianHenneke should we just release 1.11.0 early and get the ball rolling on 1.12.0? We don't have anything new happening that can be part of this month's release so might as well just get it over with. |
|
Sounds good to me! |
* develop: build: prepare next development version build: bump version Prepare release 1.11.0
|
Tested the PR with an RSA key on a Yubikey and am ed25519 key in software. |
|
I'll just leave this here as a note: Using ed25519 keys on security tokens for SSH authentication leads to exceptions being thrown within OpenKeychain. I don't think we are at fault here, but I will try to get this fixed upstream if I ever figure out why it doesn't work. |
OpenKeychain's maintainers have demonstrated radio silence for months now and seem to have moved over to working solely on Hagrid so I don't think we'll ever get anything fixed upstream. |
My strategy would be to fix it myself and then rely on personal connections to get this merged. Will see whether this pans out though. |
📢 Type of change
📜 Description
Introduces an SSHJ backend for OpenKeychain authentication based on coroutines. This allows us to finally get rid of the Jsch dependency and also brings support for modern public key types.
💡 Motivation and Context
💚 How did you test it?
I can connect via an OpenKeychain ed25519 key, but will test this more thoroughly once sshj 0.30.0 is released.
📝 Checklist
🔮 Next steps
Changelog entry and follow-up PR that removes Jsch.
📸 Screenshots / GIFs