I'm glad you managed to rebase it, but I'm not a fan of how all the commits in keepassx/keepassx#52 are squished into one. There was information about the design and implementation of the feature in those messages. Thoughts?

What was your reasoning for rebasing the branch instead of merging the current develop branch into it?

@Thynix this is a merge from #119 where commits were squished unfortunately

@Thynix good call. Would be nice to preserve the commit history. There isn't much of a reason not to other then rushing things, and rushing things with a password manager seems reckless.

@kylemanna @Thynix the thing is that previous commit were done on a different repo, and now that we are forked the codebase is different and the old commit can have some conflict.
I will try to re-do the merge

Edit: I've just tried:

Auto-merging tests/CMakeLists.txt
Auto-merging src/keys/CompositeKey.cpp
CONFLICT (content): Merge conflict in src/keys/CompositeKey.cpp
Auto-merging src/gui/IconModels.h
CONFLICT (content): Merge conflict in src/gui/IconModels.h
Auto-merging src/gui/DatabaseOpenWidget.ui
CONFLICT (content): Merge conflict in src/gui/DatabaseOpenWidget.ui
Auto-merging src/gui/DatabaseOpenWidget.cpp
CONFLICT (content): Merge conflict in src/gui/DatabaseOpenWidget.cpp
Auto-merging src/gui/ChangeMasterKeyWidget.cpp
CONFLICT (content): Merge conflict in src/gui/ChangeMasterKeyWidget.cpp
Auto-merging src/format/KeePass2Reader.cpp
CONFLICT (content): Merge conflict in src/format/KeePass2Reader.cpp
Auto-merging src/core/Database.h
CONFLICT (content): Merge conflict in src/core/Database.h
Auto-merging src/CMakeLists.txt
CONFLICT (content): Merge conflict in src/CMakeLists.txt
Auto-merging CMakeLists.txt
CONFLICT (content): Merge conflict in CMakeLists.txt

Edit2: I'm solving the conflict, can you reply to this? @kylemanna https://github.com/keepassx/keepassx/pull/52/files#r94312692

@kylemanna @Thynix the thing is that previous commit were done on a different repo, and now that we are forked the codebase is different and the old commit can have some conflict.
I will try to re-do the merge

The previous commits were done against the upstream keepassx repository, which should have the same ancestors as all forks. So, that's a relatively moot point.

The real problem is that the patch set has sat on the sidelines and never got merged. As a result, after a few years I stopped rebasing it myself since it was apparent it would never get merged and I didn't want to half ass rebase it and risk corrupting password files for the users that stumbled upon and use my fork. So, sadly it has sat frozen in time.

But, with the new found attention for the this pull request, let me see what I can do in the next few days to bring this back up to speed. :)

@kylemanna Now KeePassXC repo is detached from KeePassX one, we have added other features and fixed things.

This PR is a rebase with fixed conflict on our develop branch. Yes, the original PR's creator (#119) squished all the commit and I agree is bad.

I've checked manually all the code from this PR and from yours and (a part from minor changes) it's the same code.

I think the commit history is a small problem and that we can get over it, focusing instead on the Things to do in the main comment.

• Replacing Q_FOREACH with C++11 foreach Rebased 2FA code by Kyle Manna #119 (review)
• Add GUI Signals for letting users know when enter the Yubikey Rebased 2FA code by Kyle Manna #119 (review)
• Fix libXtst Rebased 2FA code by Kyle Manna #119 (review)
• Fix Travis build with Yubico lib
• Hiding interface when plugin is disabled or not loaded (based on Option to select extensions compile-time. Fix #50 #123 #125)
• See this PR for more Rebased 2FA code by Kyle Manna #119

I think the commit history is a small problem and that we can get over it, focusing instead on the Things to do in the main comment.

I think we're on different pages and the direction of this project seems much different then one I'm interested in supporting.

I think we're on different pages and the direction of this project seems much different then one I'm interested in supporting.

I've just stated my personal opinion about it. I'm not even the one who squished the commit history so there is no need to be "rude" at me.
I, personally, don't have the time to re-do the work someone already done. (I agree, squishing commit history is bad)

I'm trying to understanding your rejection to us.
Is it a problem about code copyright and authoring? If yes there is no problem, before the 2.1.0 release we will insert every contributor in the COPYING file.
Is it a problem about being "transparent"? You can review the commit (7d95bed) or better you can make a new PR with the commit history from keepassx/keepassx#52 (with conflict fixed)

We are maintaining this tool for hobby, no one pays us, we dedicate most of our free time to this project for the community, we are very active and we listen and reply to every single issue.
Your comment is very sad and negative and I don't think it reflects the current state of the project.

@kylemanna, I want to make sure we are working closely with the individuals (such as yourself) who put significant effort into previous submissions to the original KeePassX project and were previously ignored.

It is our intention to preserve the integrity and trustworthiness of the code being submitted and merged into the baseline. To that end I would like to know what you do not agree with so that we can make corrections to our process where needed.

Your orignal work must be preserved for both credit and posterity. @TheZ3ro, the commits must be unsquished.

Fine by me.
I haven't done anything wrong, I haven't squished the commit, I don't have the time to re-do the merge and fixing all the conflicts :)

You guys can re-do the merge and submit a new PR. I'm closing this

Alright, ladies and gents, I rebased Kyle's branch with all commits intact. It was a bit messy because upstream changes were merged in rather than rebased upon, but I got it working. Please test.

I haven't yet changed any of the things I (or @TheZ3ro) annotated during code review. That still needs to be done. There are also two annoying bugs which should be fixed as well before this can be merged:

• YubiKey appears more than once in the list when unlocking the database and then locking it again (got a really long list after a couple of locks and unlocks). This issue already existed in the original branch by Kyle when I tried it a year or two ago.
• KeePassXC should remember the check state of the "Challenge Response" checkbox. Having to check it manually every time is super annoying.

Added the 2 new bug to the TODO list in the first comment
Added also the fix for override instead of Q_DECL_OVERRIDE

And I forgot, Nice work @phoerious

Is this working on MacOS (El Capitan) as well? No matter what i try, installing from dmg or building myself, i am not able to choose a Yubikey based challenge response. Even though Yubikey is listed as extension. Do you have any insights?

I suspect you have the 2.1.4 version. Yubikey support requires 2.2.0 which hasn't been released yet.

You are correct. Sorry for not mentioning the version number, it was quite late already. Is there an approximate timeframe, when 2.2.0 is going to be released? Or could I build it myself?

You can build the develop branch. We do not have a definite release date for 2.2.0 yet, but it won't be too long.

I'd like to correct a few misconceptions about U2F that have been uttered here. A good start might be to read https://www.yubico.com/about/background/fido/ for those interested. U2F 2.0 will be supported as part of Windows Hello, a sort of authentication replacement for smartcards.

Briefly, nothing about U2F requires online components, and that is why it can and is used for things like (offline) ssh private key authentication. It's basically an easy to use UX around 2 factor, where the master key never leaves the device or can even be read (without cracking the physical key open I suppose). Also, they're the cheapest 2 factor hardware around, which is another reason I hope it's inclusion into KeePaasXC might be reconsidered. In my view it's a perfect fit, and it solves 2 factor for those that are not familiar with TOTP or (not underservedly so imho) consider it not an ideal UX. It comes closest to that real world analog that is so readily understood: the hardware token is a key and inserting it unlocks your website/program/etc. I don't know if it is any more or less safe than TOTP, but I do know that U2F keys I have been able to 'sell' (cheap+ easy to use UX) where TOTP proved bothersome.

The fact that one U2F key can't be backup up is usually circumvented by adding multiple keys.

The problem is that U2F devices maintain a counter, so the same challenge doesn't yield the same response when we issue it twice, but we need a deterministic response in order to decrypt the database. The counter is there to prevent device cloning (see https://developers.yubico.com/U2F/Protocol_details/Overview.html#_4_device_cloning_detection), but makes the protocol useless for our purpose. The usual way to allow OTP-based authentication for offline crypto vaults is to pre-calculate a series of OTPs and use them to encrypt a secret which is then used to decrypt the actual crypto vault. But that has a few disadvantages:

• you need an extra sidecar file for storing those pre-calculated OTPs
• you have a fixed secret, which somehow defeats the purpose of 2FA
• if the sidecar file and your device get out of sync, you are screwed

This, however still assumes a deterministic response. The fact that U2F generates a signature from a dynamic component (i.e., the counter), complicates this a lot. We cannot pre-calculate the signature (since we don't know the device's secret) and we don't even know how the counter will increase. It could increase by one, but it could also increase by a 1000. The only requirement is that it is larger than the last time we saw it.

All that considered, I don't see a way to make U2F work for KeePassXC. SSH authentication and WinLogon are totally different scenarios.

Hmm, I was not aware of that counter, thanks for the pointers. How is that handled by current implementers of U2F, because the key can be used with multiple services simultaneously.

Personally, unlocking my keepass db is a very similar scenario to unlocking my computer/ssh-agent: I want access to something secured.

The device generates a signature using public key cryptography and returns both the counter and the signature. An online service (or your PC in case of SSH login/pam_u2f) can easily verify the signature that way, but we cannot use it as an auxiliary encryption key.

You mean it can easily verify that because it can easily store the previous counter number?

Online services don't share their counter, yet may share the public key. In that sense, online or offline does not seem to make the difference.

A service can store the previous counter (or just ignore it, doesn't matter here), but more importantly, it can verify the signature using the public key. But signature verification isn't the same thing as using the signature to strengthen your encryption key. Both A and B (with A != B) may be valid signatures for a message M (and a variable, but known counter c), but to strengthen our encryption key, we need need A and B to always be the same if M stays the same, because in the end we need a fixed byte sequence that goes into our encryption key. That's were online and offline differ.

I think it's dawning on me: in fact it is where authentication and encryption differs. Right? Keepass not only needs authentication, but also a key to encrypt, or some combo, and U2F does not (seem to) provide that, since it's different every time. Useful for authentication (which is why PAM and Hello can use it), encryption not so much.

Yes. The counter basically modifies the challenge (i.e., the message). You send a message M to the device and it generates a signature sig(M, c), where c is the counter, and then returns both the signature and the counter. So the service can now append M and c and check if the signature is valid for (M, c) and then let you pass or not (authentication). But a password vault does not have such an authentication scheme. All it has is a fixed encryption key, so we cannot respond to the counter. If we send a challenge M and get a signature sig(M, c), which will always be different as c is incremented, we cannot use it to decrypt the database.

Thanks for helping me understand! Keep up the good work!

I'm running MacOS X 10.11.6 and KeePassXC is unable to detect my YubiKey NEO.

$ ioreg -p IOUSB -l -w 0 -x | grep Yubikey -A10
    | +-o Yubikey NEO OTP+CCID@14120000  <class AppleUSBDevice, id 0x1000008a8, registered, matched, active, busy 0 (35 ms), retain 19>
    |     {
    |       "sessionID" = 0x1b6d076c52e
    |       "iManufacturer" = 0x1
    |       "bNumConfigurations" = 0x1
    |       "idProduct" = 0x111
    |       "bcdDevice" = 0x340
    |       "Bus Power Available" = 0x1f4
    |       "USB Address" = 0x9
    |       "bMaxPacketSize0" = 0x40
    |       "iProduct" = 0x2
--
    |       "USB Product Name" = "Yubikey NEO OTP+CCID"
    |       "PortNum" = 0x2
    |       "non-removable" = "no"
    |       "IOCFPlugInTypes" = {"9dc7b780-9ec0-11d4-a54f-000a27052861"="IOUSBFamily.kext/Contents/PlugIns/IOUSBLib.bundle"}
    |       "bDeviceProtocol" = 0x0
    |       "IOUserClientClass" = "IOUSBDeviceUserClientV2"
    |       "IOPowerManagement" = {"DevicePowerState"=0x0,"CurrentPowerState"=0x4,"CapabilityFlags"=0x8000,"MaxPowerState"=0x4,"DriverPowerState"=0x4}
    |       "Device Speed" = 0x1
    |       "USB Vendor Name" = "Yubico"
    |       "idVendor" = 0x1050
    |       "IOGeneralInterest" = "IOCommand is not serializable"

You need to configure at least one slot to provide HMAC-SHA1.

@tristan-k @phoerious this should be a FAQ somewhere, I had the same problem and eventually figured it out.

The solution is to use the official YubiKey Personalization Tool
https://itunes.apple.com/us/app/yubikey-personalization-tool/id638161122?mt=12 - go to Challenge-Response, select Configuration Slot 2, specify the HMAC-SHA1 mode, and Write Configuration.

Hi everyone,

If you need encryption and authentication for the database, why don't you support CCID as well?
For instance, some U2F keys are CCID ready. I think of some FTSafe keys that are cheap (less than 20$).
That would be interesting as well as suppose and it has the encryption and authentication according to Wikipedia (I am not that familiar with that technology).

I couldn't find a quick protocol overview for CCID, but it will either have the same problems as U2F (please read my other answers for that) or it will serve the same purpose as HMAC-SHA1 to add entropy to the master key (in which case it would be rather redundant).

The usual way to integrate smart cards, however, is to let them handle the actual encryption (not sure if that is what you meant). In that case, though, I would rather prefer using GnuPG's gpg-agent for that, which optionally allows you to use smart card devices for encryption. But that is an issue related to #440 and #278, not this one.

if I understand this feature correctly, when using a Yubikey, the database uses the master-password plus a random seed, then passed through the yubikey challenge-response, and is encrypted with the Yubikey response.
So if I have several KeePass files on the same machine, both with the same password and both with the YubiKey step, then even if an attacker compromises my machine while I'm repeatedly using one KeePass file, he won't be able to open the 2nd one, as it's encrypted with a YubiKey response that was created using a different seed. Is that correct?