Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Keystore backend for SSH public key authentication #1070

Merged
merged 5 commits into from Sep 1, 2020

Conversation

@fmeum
Copy link
Member

@fmeum fmeum commented Aug 31, 2020

馃摙 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

馃摐 Description

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-3072, P-256 and Ed25519 for the following reasons:
    • RSA-3072 is supported by all servers and provides the target security level of 128 bits.
    • P-256 is supported by most servers (support added to OpenSSH in 2011, will be in all major LTS distros by September) and provides fast authentication. Should maybe be the default since its supported natively by the Android Keystore so that we would never have the key material in memory in the context of our app. On modern devices, this key type uses the Strongbox.
    • Ed25519 since its what everybody should use if they can (support introduced in 2014, but should also be in all major LTS distros by September). However, it's not supported natively by the Keystore and we need to load the private key material from an encrypted file during authentication. Is the default for now, but we can talk about this.

The new SshKey objects deals with everything SSH pubkey related, e.g. also with key import. I also modernized the SSH key gen and key view activities.

Also, Small changes were made to BiometricAuthenticator to fix one of two memory leaks.

馃挕 Motivation and Context

Supersedes #807.

馃挌 How did you test it?

Tried to authenticate with all key types. Please test this extensively.

馃摑 Checklist

  • I formatted the code with the IDE's reformat action (Ctrl + Shift + L/Cmd + Shift + L)
  • I reviewed submitted code
  • I added a CHANGELOG entry if applicable

馃敭 Next steps

CHANGELOG entry

馃摳 Screenshots / GIFs

fmeum added 2 commits Aug 18, 2020
@fmeum fmeum requested review from msfjarvis and Skrilltrax as code owners Aug 31, 2020
@fmeum fmeum mentioned this pull request Aug 31, 2020
4 of 6 tasks complete
@msfjarvis msfjarvis added this to the 1.12.0 milestone Aug 31, 2020
@msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Aug 31, 2020

We're definitely going to have to document this in the wiki when it ships. I'll review now.

Copy link
Member

@msfjarvis msfjarvis left a comment

Everything looks pretty solid, really great work here! I'll take it for a spin.

fmeum and others added 3 commits Aug 31, 2020
Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
Copy link
Member

@msfjarvis msfjarvis left a comment

Everything works in my testing, if the changelog entries look good then you can go ahead and merge it.

@fmeum
Copy link
Member Author

@fmeum fmeum commented Sep 1, 2020

Thanks, they do a pretty good job at communicating the changes.

@msfjarvis msfjarvis merged commit cbb9639 into develop Sep 1, 2020
5 checks passed
5 checks passed
test-pr (23, freeDebug)
Details
test-pr (23, nonFreeDebug)
Details
test-pr (29, freeDebug)
Details
test-pr (29, nonFreeDebug)
Details
WIP Ready for review
Details
@msfjarvis msfjarvis deleted the feature/sshj_keystore branch Sep 1, 2020
msfjarvis added a commit that referenced this pull request Sep 1, 2020
* develop:
  Add Keystore backend for SSH public key authentication (#1070)

Signed-off-by: Harsh Shandilya <me@msfjarvis.dev>
@msfjarvis
Copy link
Member

@msfjarvis msfjarvis commented Sep 4, 2020

Is this change compatible for users upgrading from v1.11.3 w/ SSH key authentication? I privately contacted the reporter for #1067 to see if #1066 solved his problem but after upgrading to the latest snapshot git operations prompted that their SSH key is missing. Generating a new key fixed the issue, but we'll either need to add a migration or notify users that they'll need new SSH keys and why.

Screenshot

image

@fmeum
Copy link
Member Author

@fmeum fmeum commented Sep 4, 2020

Is this change compatible for users upgrading from v1.11.3 w/ SSH key authentication? I privately contacted the reporter for #1067 to see if #1066 solved his problem but after upgrading to the latest snapshot git operations prompted that their SSH key is missing. Generating a new key fixed the issue, but we'll either need to add a migration or notify users that they'll need new SSH keys and why.

Screenshot

Sorry about that, I forgot to add the (trivial) migration. Will send a PR in a minute.

@fmeum fmeum mentioned this pull request Sep 4, 2020
3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can鈥檛 perform that action at this time.