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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support OpenPGP hardware keys #2170

Draft
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

tadfisher
Copy link

@tadfisher tadfisher commented Oct 9, 2022

Integrate hwsecurity to delegate decryption operations to OpenPGP security keys. This has been tested with NFC and USB hardware.

The integration point is a new callback to CryptoHandler.decrypt that provides the encrypted session key and expects the decrypted PGPSessionKey in return. The implementation then delegates as much as possible to hwsecurity's OpenPGPSecurityKeyDialogFragment.

Importing is handled by creating GnuPG-compatible stub keys, so no change to key storage is required, and it should be possible to import a secret keychain with stub keys that was exported from GnuPG. The import activity also prompts to "pair" a hardware key when importing a public key, so one could download their key from a keyserver and have hardware support without too much work.

Future improvements:

  • Support import from keyservers, so the only manual step in the best case would be to insert a hardware key.
  • The hwsecurity dialog is kind of ugly and doesn't fit well with Material 3.
  • Most of hwsecurity is unneeded for this implementation, so a new library could be much slimmer and more modernized.
  • It would be nice to view and manage hardware devices in the key management UI.

TODOs:

Comment on lines +43 to +47
val incomingKeyRing = tryParseKeyring(key)

if (incomingKeyRing is PGPPublicKeyRing) {
throw NoSecretKeyException(tryGetId(key)?.toString() ?: "Failed to retrieve key ID")
}
Copy link
Member

Choose a reason for hiding this comment

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

This method intentionally did not forbid public keys because there are use cases where folks encrypt their store to an additional public key to grant read-only access to the store for the owner of that key without having to disclose their own.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, this should be a TODO then.

msfjarvis added a commit that referenced this pull request Oct 10, 2022
This is used in #2170 so let's avoid the unnecessary conflict.

This reverts commit f5bf450.
@msfjarvis
Copy link
Member

Thanks for pushing this up! Realistically I will only be able to review it in a few days, but I've left some notes about your suggestions for future improvements.

  • Support import from keyservers, so the only manual step in the best case would be to insert a hardware key.

I don't think I'd want that, keyservers are a fairly cursed domain that I don't wish to include in the app.

  • The hwsecurity dialog is kind of ugly and doesn't fit well with Material 3.

I'll see if I can do something about that

  • Most of hwsecurity is unneeded for this implementation, so a new library could be much slimmer and more modernized.

Let's avoid additional maintenance burden if it's not strictly necessary.

@RyanSquared
Copy link

This is a bigger issue now for some Android users since OpenKeychain has stopped working on some versions of Android 13. pgpainless/pgpainless#323 has been merged, does that hopefully unblock some things relevant to this PR?

@msfjarvis
Copy link
Member

This is a bigger issue now for some Android users since OpenKeychain has stopped working on some versions of Android 13. pgpainless/pgpainless#323 has been merged, does that hopefully unblock some things relevant to this PR?

It was never really blocking the PR since we've been using a source dependency for the relevant PGPainless branch, the larger body of work here is to refine the user experience and ensure all edge cases are properly handled. This branch is also fairly out of date so I'm going to spend some time today rebasing it, but there are much more important things for me to work on so this PR is still on the back burner.

@msfjarvis msfjarvis added E-hard Effort: This will require a lot of work P-low Priority: low S-waiting-on-author Status: This PR is incomplete or needs to address review comments C-feature Category: This is a feature request A-PGPainless Area: PGPainless-backed PGP labels Dec 19, 2022
@msfjarvis
Copy link
Member

I've rebased the branch and preserved the previous state here in case there are any newly introduced errors in the rebase.

@vanitasvitae
Copy link

Great to see all the progress :)

@marrobHD
Copy link

marrobHD commented Mar 9, 2023

Is that stale?

@hashworks
Copy link

Are there any plans to continue this draft or to replace it with a new MR? Currently the new v2 has a regression, since the old OpenKeychain integration supported hardware keys.

@msfjarvis
Copy link
Member

Are there any plans to continue this draft or to replace it with a new MR? Currently the new v2 has a regression, since the old OpenKeychain integration supported hardware keys.

As of right now, no. There is an upper limit on how much I can do alone and this does not currently fit into my availability. I do intend for this to be merged at some point but cannot promise any timeline for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-PGPainless Area: PGPainless-backed PGP C-feature Category: This is a feature request E-hard Effort: This will require a lot of work P-low Priority: low S-waiting-on-author Status: This PR is incomplete or needs to address review comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants