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

Tracking issue for RFC 1195: Migrate away from OpenKeychain #1523

Open
10 of 12 tasks
msfjarvis opened this issue Oct 23, 2021 · 45 comments
Open
10 of 12 tasks

Tracking issue for RFC 1195: Migrate away from OpenKeychain #1523

msfjarvis opened this issue Oct 23, 2021 · 45 comments
Assignees
Labels
A-meta Area: meta A-PGP Area: OpenKeychain-backed PGP 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-high Priority: high, must be resolved before next major release S-in-progress Status: Implementation is underway
Milestone

Comments

@msfjarvis
Copy link
Member

msfjarvis commented Oct 23, 2021

This is the tracking issue for the implementation of RFC #1195

Steps

  • Introduce alternate PGP backend based on Gopenpgp (Add initial implementation of Gopenpgp-backed PGP #1441)
  • Replace Gopenpgp with PGPainless (Switch new PGP backend to use PGPainless #1522)
  • Implement a key management interface
    • Allow listing keys by type (public/private)
    • Allow deleting keys
  • Implement key import step in onboarding to allow users to pick a key to initialize repositories with
  • Implement passphrase caching (ref OpenKeychain)
  • Offer a migration path to OpenKeychain users to import keys exported from OpenKeychain into APS
  • Remove OpenKeychain support completely

Unresolved questions

  • How will we handle migration? Do we attempt to automate this in any fashion or simply write documentation for users to follow? We will not
  • What constitutes feature parity?
  • Do we release another major version when we drop OpenKeychain? We're dropping it straight away
@msfjarvis msfjarvis added P-high Priority: high, must be resolved before next major release A-PGP Area: OpenKeychain-backed PGP A-meta Area: meta labels Oct 23, 2021
@daraul
Copy link

daraul commented Oct 23, 2021

Do we release another major version when we drop OpenKeychain?

I would say yes. This is a fairly major change.

How will we handle migration? Do we attempt to automate this in any fashion or simply write documentation for users to follow?

I would prefer the migration be automatic, but concise documentation on the process would be welcome either way.

@msfjarvis
Copy link
Member Author

Do we release another major version when we drop OpenKeychain?

I would say yes. This is a fairly major change.

Agreed.

How will we handle migration? Do we attempt to automate this in any fashion or simply write documentation for users to follow?

I would prefer the migration be automatic, but concise documentation on the process would be welcome either way.

I haven't thought a lot about how this migration would happen, considering the fact that OpenKeychain encrypts the exported keys but I do think it'd be nice to be able to handle it on-device. Will have to investigate this down the line.

@msfjarvis msfjarvis pinned this issue Nov 22, 2021
@msfjarvis msfjarvis added the S-in-progress Status: Implementation is underway label Dec 27, 2021
@msfjarvis msfjarvis added this to the v2.0.0 milestone Dec 27, 2021
@msfjarvis
Copy link
Member Author

Passing note, we definitely wanna do passphrase caching. Doing it securely might be a challenge but dear god users will chew me out if I make them enter their passphrase every single time.

@malte-v
Copy link

malte-v commented Jan 5, 2022

I assume support for hardware OpenPGP keys needs to be implemented by us, then? It looks like PGPainless doesn't support this kind of functionality.

@msfjarvis
Copy link
Member Author

I assume support for hardware OpenPGP keys needs to be implemented by us, then? It looks like PGPainless doesn't support this kind of functionality.

That is correct.

@Myridium
Copy link

Myridium commented Jan 8, 2022

Thanks all for your work. I rely on this application.

@msfjarvis
Copy link
Member Author

Status update

  • PGPainless backend now supports importing keys
  • Decryption now takes the password from the user rather than hard-code it into the app
  • You can opt into the PGPainless backend through a settings option

Next steps

  • Make encrypt/decrypt use the key from .gpg-id instead of attempting all keys
  • Reimplement error handling to surface them to the UI rather than crash the app

@malte-v
Copy link

malte-v commented Jan 10, 2022

For hardware security key support, I think this might be of use: https://github.com/cotechde/hwsecurity
It's GPL3 licensed, pure Java and thoroughly documented at https://hwsecurity.dev/.

@msfjarvis
Copy link
Member Author

For hardware security key support, I think this might be of use: cotechde/hwsecurity It's GPL3 licensed, pure Java and thoroughly documented at hwsecurity.dev.

We're aware of the Cotech SDK, but we don't have plans to use their OpenPGP library as of now. So far, I can't even get their official sample app to generate an OpenPGP key onto my test Yubikey so it's fairly certain that a lot of pain is headed my way when it eventually comes time to support security keys with the PGPainless backend.

@msfjarvis
Copy link
Member Author

Error handling rework has landed in #1672, incorrect passwords will now pop up the password dialog once more with indication that the previously entered password was incorrect. Work on respecting the values from .gpg-id has also begun, with the refactor in #1669 paving the way for more accurately mimicking the OpenKeychain behavior w.r.t. key selection.

@daiaji
Copy link

daiaji commented Feb 2, 2022

Sometimes I have to wait a while to view my saved passwords and they don't show up right away, is this because OpenKeychain performs badly?

@msfjarvis
Copy link
Member Author

Sometimes I have to wait a while to view my saved passwords and they don't show up right away, is this because OpenKeychain performs badly?

Not strictly OpenKeychain's performance, but the transport mechanism it uses to talk to apps. Android's Binder IPC is typically rather constrained and can often be very slow.

@daiaji

This comment was marked as off-topic.

@moppman
Copy link
Contributor

moppman commented Feb 17, 2022

Status update

  • PGPainless backend now supports importing keys
  • ...

I just installed the most recent snapshot version and tried to import a pgp secret key.
I'm getting a (generic?) error message saying "error on pgp key import for input string "<my_key_id>"".

@msfjarvis
Copy link
Member Author

Status update

  • PGPainless backend now supports importing keys
  • ...

I just installed the most recent snapshot version and tried to import a pgp secret key. I'm getting a (generic?) error message saying "error on pgp key import for input string "<my_key_id>"".

That's expected, the diagnostics are not in great shape at the moment. If you can provide clear reproduction steps from creating a new key on your PC to the import failing in APS I can try to replicate and debug it.

@moppman
Copy link
Contributor

moppman commented Feb 17, 2022

Status update

  • PGPainless backend now supports importing keys
  • ...

I just installed the most recent snapshot version and tried to import a pgp secret key. I'm getting a (generic?) error message saying "error on pgp key import for input string "<my_key_id>"".

That's expected, the diagnostics are not in great shape at the moment. If you can provide clear reproduction steps from creating a new key on your PC to the import failing in APS I can try to replicate and debug it.

Sure. Note that I tried to import my "regular" key, i.e. I didn't create a new one.

  1. gpg --export-secret-keys --output sec.pgp --armor my@email.com
  2. Transfer sec.pgp to phone
  3. Import sec.pgp via pgp settings in APS

The key is a password-protected RSA2048 key with two user IDs. The import error is the same for armored and non-armored keys.

@msfjarvis
Copy link
Member Author

Reproduced the issue.

@msfjarvis
Copy link
Member Author

Import failure is fixed in snapshot builds with #1741

@msfjarvis
Copy link
Member Author

pgpainless/pgpainless#261 should help with improving how errors are surfaced to users

@msfjarvis msfjarvis added E-hard Effort: This will require a lot of work C-feature Category: This is a feature request A-PGPainless Area: PGPainless-backed PGP labels May 1, 2022
@msfjarvis
Copy link
Member Author

Thinking back and forth about this issue and #1862 is making me realize that there is little to no chance of me ever shipping these changes if I have to retain backwards compatibility. Having to maintain essentially twice the code for both accessing files and cryptography really hobbles my ability to push the project forward, as is evident in the commit activity on the project since May, where the only "useful" changes I've made are adding one small feature and merging a refactor I had finished all the way back in April.

I'm leaning towards not publishing any updates for the 1.13.x release train and instead letting v2 to be a proper break the world update, listed separately under a new package name, which adopters would migrate to themselves rather than Play Store or F-Droid automatically updating the app.

@msfjarvis
Copy link
Member Author

#1880 has now been resolved which paves the way forward for a PGPainless-only future. By next week I aim to land the package name change as well as the removal of OpenKeychain, and then we can move on with a singular focus on improving the user story around the PGPainless backend. It's been a while since I've been this pumped about working on APS :)

@msfjarvis
Copy link
Member Author

#2003 and #2004 complete the package rename and OpenKeychain removal steps respectively, my plan now is to implement the key management interface and plug it into key selection to restore parity.

@raj-magesh
Copy link

Looking forward to the updates! I noticed you'd mentioned potentially using age as a backend in this comment: #1195 (comment).

Is this still planned/in the works?

@msfjarvis
Copy link
Member Author

Looking forward to the updates! I noticed you'd mentioned potentially using age as a backend in this comment: #1195 (comment).

Is this still planned/in the works?

It is! @simao has been doing great work over on the kage library to implement the age specification in Kotlin, you can track the progress of it here. Once the MVP is ready we can start experimenting with it in Password Store.

@msfjarvis
Copy link
Member Author

I've started work on key management, beginning with #2014 to kick off building the underlying APIs before the UI can come in.

@skewballfox
Copy link

I ran into 2 issues which are probably relevant to a lot of yubikey owners as my setup was created from drduh's Yubikey Guide almost ver batim, but I wanted to confirm this isn't user error first.

here's some problems I noticed while running a free debug build:

  1. ssh doesn't seem to recognize how to handle a gpg based ssh key
  2. After importing my public key(found here), when attempting to access the contents of a password, it asks for the password, despite fact that the encryption subkey doesn't have a password (though the non-encryption masterkey does).

do you think these warrant creating new issues, or am I messing up initial configuration somehow?

@msfjarvis
Copy link
Member Author

I ran into 2 issues which are probably relevant to a lot of yubikey owners as my setup was created from drduh's Yubikey Guide almost ver batim, but I wanted to confirm this isn't user error first.

here's some problems I noticed while running a free debug build:

  1. ssh doesn't seem to recognize how to handle a gpg based ssh key
  2. After importing my public key(found here), when attempting to access the contents of a password, it asks for the password, despite fact that the encryption subkey doesn't have a password (though the non-encryption masterkey does).

do you think these warrant creating new issues, or am I messing up initial configuration somehow?

  1. is a known issue but you can file separate issues for both to ensure they get triaged and resolved before release.

@msfjarvis
Copy link
Member Author

Key management UI has landed, and I've started working on restoring support for .gpg-id. I expect it to be done sometime next week after which I'll start exploring passphrase caching.

I'm still on the fence about supporting SSH via PGP keys since firstly I have no idea how to build it, and secondly it doesn't seem any safer than the SSH key generation capabilities we already offer. If anyone has any compelling use cases for the feature feel free to mention them in this thread, otherwise there's a strong chance I simply do not build the feature out.

@skewballfox
Copy link

skewballfox commented Jul 24, 2022

it doesn't seem any safer than the SSH key generation capabilities we already offer

It can be. Part of yubikey's design is the keys stored in them never leave the hardware device: encrypted content is sent to the key, decrypted content is returned. So it's safer in the sense that the keys wouldn't be stored in memory at any point.

that being said this is something which I believe will only be desired if a user is using a yubikey (or similar hardware key with OpenPGP support), but this will be likely be the same set of users who want/need pkcs11 support. i.e. if the user is using a yubikey (or similar hardware key) with gpg to decrypt passwords, then they probably also have it set up for ssh.

I have no idea how to build it

I feel like gpg for ssh, is going to be a pebble on top of a mountain: support for (gpg tokens stored on) smart cards for encryption. That will likely be make or break case for a set of users, and will also be the most difficult part. If the code is implemented to use smart cards/hardware keys over usb, or NFC for encryption/decryption, then much of the functionality necessary to implement ssh authentication will already be there.

@msfjarvis
Copy link
Member Author

it doesn't seem any safer than the SSH key generation capabilities we already offer

It can be. Part of yubikey's design is the keys stored in them never leave the hardware device: encrypted content is sent to the key, decrypted content is returned. So it's safer in the sense that the keys wouldn't be stored in memory at any point.

that being said this is something which I believe will only be desired if a user is using a yubikey (or similar hardware key with OpenPGP support), but this will be likely be the same set of users who want/need pkcs11 support. i.e. if the user is using a yubikey (or similar hardware key) with gpg to decrypt passwords, then they probably also have it set up for ssh.

I have no idea how to build it

I feel like gpg for ssh, is going to be a pebble on top of a mountain: support for (gpg tokens stored on) smart cards for encryption. That will likely be make or break case for a set of users, and will also be the most difficult part. If the code is implemented to use smart cards/hardware keys over usb, or NFC for encryption/decryption, then much of the functionality necessary to implement ssh authentication will already be there.

Support for Yubikey and friends with PGPainless is currently not implemented and will be its own separate initiative. That said, it's a compelling enough argument for SSH support, so I'll add it to the checklist when we get to it.

@msfjarvis
Copy link
Member Author

Work on .gpg-id support is stalled at the moment, I ran into a couple issues that I didn't have enough time and motivation to fix. Will try and pick it back up again.

In the mean time I ended up doing a modernization pass on the cotech hwsecurity SDK since the GPL version seems all but abandoned. The changes are up in our fork. I'm not going to do anything else with it anytime soon, but if someone wants to work on supporting hardware security keys in APS then they should use that fork.

@msfjarvis
Copy link
Member Author

.gpg-id support has been implemented in #2080

@blabno
Copy link

blabno commented Jan 8, 2023

What's the status? Which branch is this features being worked on?

@msfjarvis
Copy link
Member Author

What's the status? Which branch is this features being worked on?

The status is in the issue body. The work is being merged as it's being completed so the branch to track is develop, the default branch of this repo.

@msfjarvis
Copy link
Member Author

8af09d5 guards crypto operations with a check that ensures there is at least one PGP key imported or offers up the key import flow. This fixes the dubious UX of asking for a password during decryption before throwing an unhelpful NoKeysProvided error.

@apprehensions
Copy link

apprehensions commented Feb 26, 2024

Implement passphrase caching

I have imported my GPG key from OpenKeychain manually, and Password store keeps asking for my passphrase, in which it does not care about the passphrase and accepts anything anyway..

Edit: it seems that Password store requires a passphrase for the PGP key, even if it is empty. I have learned that having it be empty seems like a bad security practice, so i added one and my problem was fixed (alongside enabling passphrase caching.)

@msfjarvis
Copy link
Member Author

Implement passphrase caching

I have imported my GPG key from OpenKeychain manually, and Password store keeps asking for my passphrase, in which it does not care about the passphrase and accepts anything anyway..

The prompt being shown is a known bug (#2836), I tried fixing it but it didn't quite work and I haven't had any time to dedicate to Password Store since.

@l3u
Copy link

l3u commented Mar 9, 2024

Considering the newly emerged problems with GnuPG 2.4 and OpenKeychain (cf. open-keychain/open-keychain#2900 etc.): The porting away from OpenKeychain seems to be quite finished. Are there plans to release this soonish?

@msfjarvis
Copy link
Member Author

msfjarvis commented Mar 9, 2024

Considering the newly emerged problems with GnuPG 2.4 and OpenKeychain (cf. open-keychain/open-keychain#2900 etc.): The porting away from OpenKeychain seems to be quite finished. Are there plans to release this soonish?

There are still plenty of open bugs and general UX issues with the app that I would like to address before a release.

Edit: FWIW, migrating away from OpenKeychain does not resolve the issues with AEAD which remains unsupported by PGPainless as well.

@mannp
Copy link

mannp commented Mar 9, 2024

For an attempting new user like myself, some ux issues make the app annoying to use and I am still not onboard.

A shame, as there isn't really an alternative.

@l3u
Copy link

l3u commented Mar 9, 2024

Edit: FWIW, migrating away from OpenKeychain does not resolve the issues with AEAD which remains unsupported by PGPainless as well.

Yeah, I know meanwhile ;-) But it can be shipped around by simply disabling AEAD (OCB) in the key itself, cf. https://wiki.archlinux.org/title/GnuPG#Disable_unsupported_AEAD_mechanism

I'm just using your outstanding pass Android frontend on a quasi-daily basis (also for OTP auth) and I wondered what's the plan for the new major release. However, thanks a lot for working on this!

@nicolidin
Copy link

Hi, is this version handles smart card (yubikey) for gpg ? if, no will it handles it ?

@msfjarvis
Copy link
Member Author

Hi, is this version handles smart card (yubikey) for gpg ? if, no will it handles it ?

Support for hardware keys is a work in progress: #2170

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: meta A-PGP Area: OpenKeychain-backed PGP 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-high Priority: high, must be resolved before next major release S-in-progress Status: Implementation is underway
Projects
Status: 🏗 In progress
Development

No branches or pull requests