Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

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

@msfjarvis
Copy link
Member

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

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.

fmeum and others added 3 commits August 31, 2020 14:08
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
* 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
8 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants