Kunzisoft / KeePassDX

Lightweight vault and password manager for Android, KeePassDX allows editing encrypted data in a single file in KeePass format and fill in the forms in a secure way.
https://www.keepassdx.com/
GNU General Public License v3.0
4.58k stars 270 forks source link

Support yubikey challenge response #8

Closed piratenpanda closed 1 year ago

piratenpanda commented 6 years ago

Would be nice if the app would support challenge response via the yubichallenge app

J-Jamet commented 6 years ago

This is not a priority because I have to solve bugs and put more basic features but why not in the future.

rugk commented 6 years ago

For reference KeePassXC implements this on the desktop and if you want code for Android OpenKeyChain supports YubiKey (in a different use case for PGP keys, but well… it's something.

piratenpanda commented 6 years ago

Keepass2android has a working implementation with calling the yubichallenge app

rugk commented 6 years ago

Hmm… don't know whether depending on another app is so good. At least it would be nice to not only support this one vendor, but well… future plans anyway. But thanks FYI.

piratenpanda commented 6 years ago

If there is an implementation like in openkeychain even better, sure

ovz93br43v7 commented 5 years ago

This feature would be great and please compatible to the KeepassXC implementation.

piratenpanda commented 5 years ago

might as well use ykdroid instead of the yubichallenge app as keepass2android does for a while now. Just to update on my first post

LivInTheLookingGlass commented 5 years ago

Is this still a planned feature?

J-Jamet commented 4 years ago

Yes, I'm just going to release the final 2.5 version before starting big jobs like this issue. I must also look at the other existing physical keys (open source) and study the functioning of KeePassXC.

dimmuboy commented 3 years ago

Is there any forecast which version could be have finally yubikey support?

J-Jamet commented 3 years ago

I have not yet looked at the technical operation of the yubikey for lack of time because of new bugs and more important features to implement, but if a technician is motivated to make a pull request, it will be very nice.

J-Jamet commented 3 years ago

I bought a Yubikey4 and an Onlykey to be able to test, I still have to study how the challenge response works with these keys and think about the architecture.

hughobrien commented 3 years ago

Hi there! I'm buying KeePassDX Pro now to show my support for this issue! I was using passwdsafe which implements it via NFC.

Thanks for your work.

invalid-error commented 3 years ago

Hi there! I'm buying KeePassDX Pro now to show my support for this issue! I was using passwdsafe which implements it via NFC.

Thanks for your work.

Hi there! I'm buying KeePassDX Pro now to show my support for this issue! I was using passwdsafe which implements it via NFC.

Thanks for your work.

I would like do the same but I am using f-droid (no Pro available). Any update on Yubikey support (like KeePassXC Desktop). I would also like to do a donation for this new feature :1st_place_medal: .

invalid-error commented 3 years ago

All my BAT to you @J-Jamet ;) ... you should add this to your Cryptocurrency donation section.

schmitmd commented 3 years ago

Bought KeePassDX Pro and would love to see this feature.

mariodsantana commented 3 years ago

I use F-Droid so can't buy pro, but I just donated E10 because thank you and to support this feature! I have to use different software until this feature makes it into KeePassDX. :(

J-Jamet commented 3 years ago

Thanks for your support, the feature is planned ~for version 3.3.0~ (priority bug fixes). I am aware that many users want this feature, I just need to do things in order because otherwise there will be bugs in the app. I prefer to do things properly by studying the technical functioning correctly and don't add any security holes. https://github.com/Kunzisoft/KeePassDX/projects/43 So for the moment I'm concentrating on version 3.0.0 which restructures the application for a better code architecture (so also indirectly prepares this feature). I would also like to be able to use my keys with KeePassDX, it just takes time and working with specific hardware is different than working with only the software side.

mariodsantana commented 3 years ago

Understood. I'll be waiting patiently. :)

invalid-error commented 3 years ago

For me the same, I just don't use the app now since I need to sync. my database and I do not sync. it without a challenge response. I really looking forward to this implementation. I will also wait patiently.

wts42 commented 3 years ago

Got me the pro version now and in happy anticipation of the yubikey feature. (100 euros, 1 yubikey, 1 spare yubikey and KeepassDX Pro) Money good invested. :blush:

Edit: I hope I got that right and the NFC feature will work.

J-Jamet commented 3 years ago

As I said in my previous comment, things are done in order to have as few bugs as possible. Of course, the integration of physical keys is still planned. The project section allows you to see all the work to be done and planned for future versions. Thanks for your patience.

phnx47 commented 2 years ago

I bought Pro version. But now, I started using Yubikey and switched to Keepass2android, because I can't unlock my db. I see that is planned, but I can't just wait

uduerholz commented 2 years ago

I hacked together a working prototype. "Working" means: I can open a database with my Yubikey via NFC in combination with the ykdroid app (needs to be installed first). The database was created with KeePassXC on Ubuntu.

@J-Jamet: If you like, I can clean the code, do more tests and create a pull request, but I would need to know, which direction to take: Is it okay to depend on the ykdroid app? Or maybe you want to simply integrate the code (it has the same license, so shouldn't be a problem in that respect).

J-Jamet commented 2 years ago

@uduerholz Awesome! No problem for the ykdroid dependency, that looks very good. It's also an open source app so it will be easier for maintenance.

uduerholz commented 2 years ago

I just stumbled upon a small usability problem. The current implementation creates a new random transform seed (part of the database header) each time the database is saved. When the master key is derived, the algorithm sends this transform seed as challenge to the Yubikey and uses the response. In effect this means that each time the database is saved, the app needs access to the Yubikey. Via USB this is not a problem, but via NFC it could be annoying.

I see the following solutions:

  1. When the database is protected with a Yubikey, never modify the transform seed (makes it a bit less secure).
  2. Require of the user to present the Yubikey each time the database is saved.
  3. When the Yubikey is used the first time, create some random seeds in advance and send them to the Yubikey. Store the responses and use them later when the database is saved. (Does not work with the current ykdroid implementation though.)
J-Jamet commented 2 years ago

I imagine that you want to implement the Challenge Response of KeePassXC (And not the KeeChallenge plugin of KeePass2 which is very good because it requires another XML file to load and is a rather old technique.).

Solution 2 is the most relevant. KeePassXC does require the key to be present for each save in the database, so users will not be lost. I don't have the workflow in mind for NFC, does it require a lot of steps?

It would be nice to be able to easily select the credentials to unlock the database. I will think about a correct UI, because I would like to implement other algorithms in the future (hmac-sha1 kpxc, hmac-sha1 kp2, oath hotp, hmac-secret fido2).

uduerholz commented 2 years ago

Yes, one main requirement is to be compatible with KeePassXC. When you have a suggestion for the UI, please post it here.

J-Jamet commented 2 years ago

For the UI, I was thinking of simply adding a field with a switch button below the key file field. If compatible drivers or applications are detected (so with an intent test), (The field becomes visible, otherwise is hidden. Maybe all the time displayed at first, this is to not lose new users as it is an advanced feature but maybe the best is to hide the key-file and challenge-response field in a section to be extended.)

It would be composed of a dropdown list to select the type of challenge protocol used. (At first, can just be a label if there is only the hmac-sha1 kpxc).

For the recovery mode, a small icon to the right of the field. When clicked, would display an edit text field below the challenge-response field.

If the recovey mode text field is [hidden] or [displayed and its content is empty], we go through ykdroid otherwise we test the number of characters requested (we display an error if necessary), and we enter directly the recovery key if the size is good.

What do you think about it?

uduerholz commented 2 years ago

Sounds good. What do you mean by "recovery mode"?

J-Jamet commented 2 years ago

By recovery mode, I mean a way to open the database without a physical key. When configuring a yubikey in challenge-response, a key is generated which is normally saved on paper in case of loss. It can be used in the recovery mode to open the file without a physical key.

piratenpanda commented 2 years ago

Manually entering the challenge response most likely. At least that's how it's handled elsewhere

uduerholz commented 2 years ago

Okay, I see. I thought you talk about something that already exists. ;-)

J-Jamet commented 2 years ago

The recovery mode does not need to be implemented at the beginning. The idea is that you can recreate a physical key with the same behavior from the backup, so it's just a help to not have to buy new hardware in case of loss. (Link to https://github.com/keepassxreboot/keepassxc/issues/1734)

uduerholz commented 2 years ago

I am about to implement solution 2) which is compatible to KeePassXC. Each time the database is (automatically) saved, the activity from ykdroid is started. For NFC this means the user is asked to tap the yubikey again. The yubikey responds to the challenge and ykdroid passes the response back to the app. At that point a new master key has to be derived, which means the password has to be stored in memory. I think that's okay, because the current master key is also stored in memory, so that doesn't make a real difference, or?

uduerholz commented 2 years ago

To be clear: solution 1) is also compatible with KeePassXC

J-Jamet commented 2 years ago

You do what you think is best. Indeed, the master key is accessible as long as the database is open, it allows you to reload the database so you can use it again. I haven't studied the security of this feature yet, I've indicated solution 2 because it seemed the most relevant at the time without thinking too much about the architecture.

uduerholz commented 2 years ago

Coming back to the UI: How should it look like, when creating a new database?

In my opinion it would make sense to treat it similar to the "advanced" (biometric) creation or unlocking of a database: First the database is created "normally" with password and/or key file. Later you can add protection with the yubikey. It would not be possible to combine biometric and yubikey protection, the user would have to choose. (For a database protected by the yubikey, a simple unlocking with the touch of a finger would not be possible anyway. As the transform seed in the header could have changed outside the app, unlocking always requires interaction with the yubikey.)

J-Jamet commented 2 years ago

Currently, the fingerprint is linked to the keystore, so basically and currently it means that if the fingerprint is used for a database, it means that the password of a database is hashed in the keystore. The purpose of the Yubikey (and other keys) is to add a new way to unlock a database that can be added to or substituted for others part of master key (which KeePassXC does and allows you to use password and/or key file and/or yubikey)

If for you, the yubikey uses the same workflow as the fingerprint (which is only an advanced unlock, a kind of overlay that does not replace the usual unlock methods to stay compatible with KeePassXC), it means that the Yubikey will replace the fingerprint as advanced unlock to hash the password in the Keystore?

But in this case, a database created with KeePassXC with the Yubikey as part of the master key, cannot be opened with KeePassDX. I think the Yubikey/Onlykey should be part of the password change/creation component first to be compatible with KeePassXC databases.

In the database opening workflow, if this is a problem you can disable advanced unlock (fingerprint) if the yubikey switch view is checked. (I thought that the request of the yubikey could be done when the associated switch view is checked so it wouldn't be a problem if the user has configured his fingerprint, which he will use after authenticating his physical key. It can be tedious but if the user does not want to type his big password using his fingerprint and also use his physical key, it is possible in this case)

Then, in a second time, we can think about associating the fingerprint to any part of the master key (password and/or key file and or physical key). #427

uduerholz commented 2 years ago

Sorry, I didn't clearly say what I had in mind. My suggestion was not to use the Keystore in combination with the yubikey. The implementation should of course be compatible with KeePassXC. I wanted to suggest that the user can either use the fingerprint xor the yubikey. Using the yubikey the password would need to be entered manually. If that is not what you want, I would need to check how to combine fingerprint and yubikey (without the need to enter the password). But it should be feasible to use both.

agates commented 2 years ago

Sorry, I didn't clearly say what I had in mind. My suggestion was not to use the Keystore in combination with the yubikey. The implementation should of course be compatible with KeePassXC. I wanted to suggest that the user can either use the fingerprint xor the yubikey. Using the yubikey the password would need to be entered manually. If that is not what you want, I would need to check how to combine fingerprint and yubikey (without the need to enter the password). But it should be feasible to use both.

Check out KeePassXC's implementation of TouchID for macOS which does something similar on the desktop. Might give a good frame of reference.

J-Jamet commented 2 years ago

I just refactored the credential view code to make it easier to integrate advanced unlock with the yubikey

uduerholz commented 2 years ago

Great, thank you. I was busy recently but hope to finish the pull request soon.

J-Jamet commented 2 years ago

Don't rush, I'll refactor as I go along, so it will be easier and more intuitive to integrate the functionality. Also it's fine if you work on an old version of the application, I can do the merge by hand afterwards.

Dedicated branch : https://github.com/Kunzisoft/KeePassDX/tree/feature/Hardware_Key

uduerholz commented 2 years ago

@J-Jamet: I will probably not have time to finish this any time soon, sorry. If you want, you can have a look at my develop branch: https://github.com/uduerholz/KeePassDX/tree/develop It can be used to open yubikey-protected databases in read-only mode.

J-Jamet commented 2 years ago

No problem, I understand it's not an easy feature and it requires a lot of design time. In any case, I'll be inspired by your work, it will greatly help me, so thank you ◕‿◕.

J-Jamet commented 2 years ago

I did the first integration of this feature inspired by your code (@uduerholz) and integrated it into a more modular architecture. I haven't tested all the cases yet, but I can connect to the YKdroid driver with the intent. I've been having trouble with OTG (linked to #137) on my Android device, so I'll have to look at what's causing this. I'm focusing for the moment on the Yubikey with SHA1 Challenge-response of KeePassXC, the KeePass plugin method requires a separate XML file, which is very restrictive to manage.

The idea is also to create a driver in the Pro application that will allow to manage the driver settings according to the key and the device. Here are the repo that can be useful for that : Ykdroid : https://github.com/pp3345/ykDroid Yubikey4java : https://github.com/Toporin/yubikey4java Yubico-C : https://github.com/Yubico/yubico-c | https://developers.yubico.com/yubico-c/ | https://developers.yubico.com/yubikey-personalization/

J-Jamet commented 2 years ago

Well, it's OK in OTG but I think the driver sometimes goes into an unrecognized state, maybe because of a thread that doesn't respect the life cycle of an activity. I'll probably redo a cleaner code of the driver in kotlin but it works! There is still a lot of work to do for a good implementation. I will take the opportunity to put the recovery mode when I do the unit tests but it is very promising.

J-Jamet commented 2 years ago

The implementation of this new factor involves several elements:

I am currently working on integrating a HardwareKey when creating a master key. Once this feature is finished, I will study the hmac-secret operation of FIDO2 in more detail.

J-Jamet commented 2 years ago

It will take longer than I thought. I understand better your previous comment @uduerholz https://github.com/Kunzisoft/KeePassDX/issues/8#issuecomment-1013912643

Currently, to test, we are using Ykdroid to retrieve a response after a challenge. It works because we bypass the workflow a bit by reading the file header to get the seed and get the response before doing the actions in the service. But this is not possible for the save because the seed is randomly generated in the KDF by a service action. So I have to find a way to do the challenge-response in the middle of a service action method. I also need to create another application to replace Ykdroid that will provide the response by Service (with binding) and not by Activity because it is not possible to get an Activity response from a Service to allow saving a new key. So a big architecture work.