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

Add yubikey param and DBus methods to unlock the database requiring yubikey #9251

Closed
wants to merge 3 commits into from

Conversation

piorkov
Copy link

@piorkov piorkov commented Mar 18, 2023

At the moment, we do not have the option to unlock the database in the GUI in case we use hmac-secret from YubiKey.
Let's add a yubikey parameter that will allow us to unlock the database using the Yubikey slot we specified, and add DBus methods that will allow us to do the same from other processes.

The code that I am adding maintains backward compatibility for all program parameters and DBus,and additional parameters in functions inside the code are optional, so I do not expect regression in existing use cases.

One of the main changes I had to add (because I didn't know how to solve it sensibly) is to disable the functionality of remembering the last key used to open a given database when using the --yubikey parameter. This is because in the case of a larger number of keys, asynchronous searches by Keepass for available keys created a conflict with my method of using yubikey from the --yubikey parameter. However, it does not affect the behavior of the program without this parameter.

I added two new DBus methods (openDatabaseYubiKey and listHardwareKeys) and extended the openDatabase method with an optional parameter to indicate the YubiKey slot

The listHardwareKeys parameter is used return a list of YubiKey slots available in the program.

I added the parameter openDatabaseYubiKey to maintain backward compatibility of the previous methods, and to give the possibility to unlock the database with just a password and yubikey (I don't like this way much, but I didn't have a better idea)

Screenshots

N/A

Testing strategy

Manual tests on existing databases:

  • testing combinations of new and old parameters:
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-onlypass.kdbx --pw-stdin
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-filekey.kdbx --pw-stdin --keyfile /home/piorkov/Passwords/test-key
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk-filekey.kdbx --pw-stdin --keyfile /home/piorkov/Passwords/test-key --yubikey 1
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk.kdbx --pw-stdin --yubikey 1
  • testing the new parameter itself (with two different YubiKey inserted to PC):
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk.kdbx --pw-stdin --yubikey "1:<first YubiKey serial>"
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk.kdbx --pw-stdin --yubikey "1:<second YubiKey serial>" #the same hmac-secret as in first YubiKey
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk2.kdbx --pw-stdin --yubikey "2:<first YubiKey serial>"
  • testing negative scenarios:
./keepassxc /home/piorkov/Passwords/Passwords-test-yk.kdbx --yubikey 1 #ignore yubikey param wihout password
echo "test" | ./keepassxc /home/piorkov/Passwords/Passwords-test-yk.kdbx --pw-stdin --yubikey 3 #wrong slot
  • DBus test:
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.listHardwareKeys #return Array of strings: ['2:<first YubiKey serial>', '1:<first YubiKey serial>', '1:<second YubiKey serial>']
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.openDatabase /home/piorkov/Passwords/Passwords-test-onlypass.kdbx test
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.openDatabase /home/piorkov/Passwords/Passwords-test-filekey.kdbx test /home/piorkov/Passwords/test-key
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.openDatabase /home/piorkov/Passwords/Passwords-test-yk-filekey.kdbx test /home/piorkov/Passwords/test-key 1
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.openDatabase /home/piorkov/Passwords/Passwords-test-yk-filekey.kdbx test /home/piorkov/Passwords/test-key "1:<first YubiKey serial>"
qdbus org.keepassxc.KeePassXC.MainWindow /keepassxc org.keepassxc.KeePassXC.MainWindow.openDatabaseYubiKey /home/piorkov/Passwords/Passwords-test-yk.kdbx test "1:<first YubiKey serial>"

Type of change

  • ✅ New feature (change that adds functionality)

Currently, we can specify as program parameters a password (as stdin)
and a key file to unlock the database. The problem is when the database
requires YubiKey to unlock.
Lets add a parameter that allows you to point to a specific slot and
YubiKey serial to unlock the database.
@piorkov piorkov changed the title Feature/yubikey param Add yubikey param and DBus methods to unlock the database requiring yubikey Mar 18, 2023
@droidmonkey
Copy link
Member

droidmonkey commented Mar 18, 2023

This is very complex code for an add on. I guess that speaks to how over coded the databaseopenwidget already is. Very unlikely to merge in this state.

It is becoming pretty clear that we need to abstract the actual opening of a database and key material handling away from the open widget itself. This would include the dance with yubikey code since it is async. We should be able to just bypass the entire widget operation if given key material, basically render it disabled while processing. Then if unlock fails just reset the widget.

@piorkov
Copy link
Author

piorkov commented Mar 20, 2023

Thanks for the answer. I agree extracting the logic opening the database would be the best option. This is my first experience with this code, so I was trying to add functionality that would be useful to me, at the lowest possible cost.

If I find some free time, I will try to prepare a more elegant code, with separate logic for opening the database.

What doesn't convince me is the API for DBus.
I would be very interested to hear other people's opinions on how such a new API that also uses a hardware key to open the database could look like

@droidmonkey
Copy link
Member

Unfortunately I am going to close this since it is incompatible with the changes made to how we handle Yubikeys.

@droidmonkey droidmonkey closed this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants