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.

Sign up for GitHub

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

Jump to bottom

Add Keystore backend for SSH public key authentication #1070

Merged
merged 5 commits into from Sep 1, 2020
Merged

Add Keystore backend for SSH public key authentication #1070

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 fmeum mentioned this pull request Aug 31, 2020
Closed
6 tasks
@msfjarvis msfjarvis added this to the 1.12.0 milestone Aug 31, 2020
@msfjarvis
Copy link
Member

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

msfjarvis
msfjarvis reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@msfjarvis msfjarvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

app/src/main/java/com/zeapo/pwdstore/git/sshj/SshKey.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/SshKey.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/SshKey.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/SshKey.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/git/sshj/SshKey.kt Outdated Show resolved Hide resolved
app/src/main/java/com/zeapo/pwdstore/sshkeygen/ShowSshKeyFragment.kt Show resolved Hide resolved
fmeum and others added 3 commits August 31, 2020 14:08
msfjarvis
msfjarvis approved these changes Sep 1, 2020
View reviewed changes
Copy link
Member

@msfjarvis msfjarvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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
@msfjarvis msfjarvis deleted the feature/sshj_keystore branch September 1, 2020 08:12
msfjarvis added a commit that referenced this pull request Sep 1, 2020
@msfjarvis
Merge branch 'develop' into refactor/git_operation
869b39d 
* develop:
  Add Keystore backend for SSH public key authentication (#1070)

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

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 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
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfjarvis msfjarvis msfjarvis approved these changes

@Skrilltrax Skrilltrax Awaiting requested review from Skrilltrax

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@fmeum @msfjarvis