Support remote database access using external tools

[t-h-e]

As discussed here: https://github.com/keepassxreboot/keepassxc/issues/1775#issuecomment-983056115

This change provides the ability to sync a database with a remote database with the help of scp. (Fixes #1775) The RemoteSettingsDialog works similar to DatabaseSettingsDialog and can be extended in the future to store previous settings and load them again. Although currently scp is supported, the implementation is extensible to include further programs in the future to download and upload .kdbx files to sync with them.

Feedback is very welcome as this would be my first contribution to KeePassXC

Testing strategy

Place a second database on a separate server that is accesible via ssh and have scp installed and in your PATH variable. The second database can include keys and/or groups that the current database does not. The new "Remote Sync ..." feature is available in the UI next to "Merge From Database...".

Regarding unit tests: Help is very welcome as I am uncertain how to mock the QProcess. I expect the rest to work similarly to TestGui::testMergeDatabase

Type of change

• ✅ New feature (change that adds functionality)

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 68.77828% with 69 lines in your changes missing coverage. Please review.

Project coverage is 64.47%. Comparing base (ad8a00d) to head (6b811dc).

:exclamation: Current head 6b811dc differs from pull request most recent head 01224de

Please upload reports for the commit 01224de to get more accurate results.

Files	Patch %	Lines
src/gui/DatabaseTabWidget.cpp	50.79%	31 Missing :warning:
src/gui/remote/RemoteProcess.cpp	8.33%	11 Missing :warning:
src/gui/remote/RemoteSettingsDialog.cpp	76.92%	9 Missing :warning:
src/gui/DatabaseWidget.cpp	87.76%	6 Missing :warning:
src/gui/remote/RemoteSettingsWidgetScp.cpp	76.47%	4 Missing :warning:
src/gui/remote/RemoteSettingsWidget.cpp	71.43%	2 Missing :warning:
src/gui/remote/RemoteSettingsWidgetScp.h	0.00%	2 Missing :warning:
src/gui/remote/ScpParams.cpp	90.91%	2 Missing :warning:
src/gui/remote/RemoteProcess.h	0.00%	1 Missing :warning:
src/gui/remote/RemoteProgramParams.h	0.00%	1 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #7222      +/-   ##
===========================================
+ Coverage    63.75%   64.47%   +0.72%     
===========================================
  Files          360      348      -12     
  Lines        44276    43980     -296     
===========================================
+ Hits         28226    28353     +127     
+ Misses       16050    15627     -423     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[t-h-e]

@droidmonkey I think this PR is ready for review. As mentioned in the discussion of the issue, I'm not a C++ developer, so feedback is very welcome 😃

[droidmonkey]

Excellent, will review when I get the chance

[hifi]

I'll add a note that scp is considered harmful and this should probably use sftp instead.

[t-h-e]

I'll add a note that scp is considered harmful and this should probably use sftp instead.

While OpenSSH recommends to use modern protocols like sftp or rsync, in the near future scp will be running with SFTP protocol. Since version 8.7, scp can already be used with SFTP protocol with the switch -s. The benefit of scp over sftp is that a single file upload can be done with a single command, which makes it easier to use.

I have changes for using rsync and curl nearly finished, but I would prefer to at least get some feedback first to know if this feature is heading in the right direction. More protocols can easily be added.

[hifi]

AFAIK you can use a single command with sftp if you batch it through stdin. That's without any temporary files.

I have high personal interest in this PR so I'll be testing this as well and providing feedback once I get around to.

[hifi]

I took a quick look, it's promising but maybe it's not being utilized to its fullest potential with hardcoded commands.

I'm thinking sync over SSH is an advanced user feature so what if this would be simplified to "Sync Command" and instead provide a generic mechanism to run a command that can be templated with the database path and possibly other options? We'd only check that the return code of the command is a success and if not possibly show the code and stdout+stderr for debugging.

We could still provide presets for scp, sftp etc. for convenience. This allows the user to write script that takes the arguments from KeePassXC and does whatever with them.

[KFDCompiled]

I've contributed to the #1775 bounty and am excited to test this PR!

[t-h-e]

@hifi using templating and running arbitrary commands is a great idea. It is way more flexible. Running the sync command after unlocking as shown in your comment would definitely be a convenient feature. 👍 You seem to have way more keepassxc experience. I am not sure if I should even proceed with my PR then 😆

[t-h-e]

@droidmonkey @phoerious Any chance to get a review for this PR? It is nearly 10 months old now 😞

[phoerious]

Yes, sorry for the delay. But first, could you please rebase it to the latest develop HEAD and fix the conflicts?

[t-h-e]

Done 😃 Edit: I will check next week, why the build fails

[t-h-e]

PR is rebased and conflicts are resolved. The build works mostly. The only issue seems to be that TestGui::testSaveBackup fails sometimes, but I cannot reproduce the behaviour locally. Do you have any idea why this test might fail? It does not seem to fail for all build configurations.

[droidmonkey]

I'll give this a review soon

[t-h-e]

Hi, I added now the possibility to run any commend to download or upload a keepass file for syncing. For some reason my test fails on the CI. I can execute the test on a windows and linux machine without issues though 🤷

[HexF]

I've tested this out a little and come across a few quirks:

1. When doing a remote sync, the UI entirely locks up whilst downloading and uploading the database, giving no indication to the user as to what is going on.
2. While using the anyCommand feature with rclone, I managed to copy my Keepass database into a directory named with {TEMP_DATABASE}, rather than copying the file which produced no error up until I fill all the credentials in and get a "cannot open file" error.
3. When synchronizing the exact same database I am required to select my key file and enter my password again, this is a little inconvenient. I would much prefer the derived key from my currently open database be tried first before asking for my password and key file again.
4. Whenever I open the menu and click "Remote Sync", I am always brought to the Config UI, having to click OK before a sync will take place. I would prefer this only occur once.
5. If I restart KeepassXC, my I inputted into the config UI are lost and have to be re-entered. This appears to be indicated here
6. The database name containing {} may cause some file name compatibility errors on some filesystems/operating systems.

I'll keep you updated as I use this feature more extensively, but for now its looking pretty good.

For those interested, here is the upload/download commands I'm using for syncing with Nextcloud:

# Download
rclone copyto nextcloud:Passwords.kdbx {TEMP_DATABASE}
# Upload
rclone copyto {TEMP_DATABASE} nextcloud:Passwords.kdbx

[t-h-e]

@HexF Thank you so much for testing and your feedback. 😊 Some comments from my side:

1. Yes, that should be handled better, but is further down the priority list. Although I think that can be fixed quickly
2. That is weird. Do you have more input on this error how to reproduce it?
3. This annoys me as well. The code is inspired by the merging feature, where this is also necessary. I have to check if there is a more convinient solution.
4. Hmm, that I understand, but what if you want to sync to a different location afterwards. Maybe the menu can have an option "sync with previous settings" or offer a drop down with the last options. This is probably connected to 5.
5. Not all functionality is implemented yet.
6. True, I might just use a dash.

On a side note: I had some time the last weeks and added an option to also be able to open a remote database. I will update the PR as soon as I have some tests and cleaned the code. Afterwards I will work on your suggestions. Thanks again.

[HexF]

That is weird. Do you have more input on this error how to reproduce it?

I imagine using a command such as mkdir {TEMP_DATABASE} should do it.

Hmm, that I understand, but what if you want to sync to a different location afterwards. Maybe the menu can have an option "sync with previous settings" or offer a drop down with the last options. This is probably connected to 5.

If multiple destinations are to be supported, I would prefer adding them in a preferences menu, then being able to select them in a flyout menu from the "Remote Sync" button. You could also put a quick-link to this preferences page in this flyout.

Something a little like this:

...
Remote Sync > | NextCloud
Import        | Google Drive
Export        | ---
Quit          | Edit remote sync destinations

[t-h-e]

I have added a "remote open dialog" to allow people to open remote databases with KeePassXC directly. This uses a lot of the functionality that I added previously. Next step is to address some of the issues mentioned by @HexF

[HexF]

Looking at the remote open, I don't see any hook for a save to upload/merge the database with the remote one. Could this be added?

[t-h-e]

Sure that is easy enough. I still need to figure out what's wrong with the tests on TeamCity. Locally (windows and linux), all tests run fine.

[mazunki]

I haven't tried out this feature yet, but I hope that when the feature is implemented, there would be a hook to automatically push passwords to ${DEFAULT_REMOTE} whenever an update is done, and some mechanism to automatically pull from the remote each ${RESYNC_INTERVAL} seconds.

This would make it seamless to the user, provided that ssh is set up already.

Is this feasible to implement?

[t-h-e]

Update:

• Remote sync does not require password input if the same key/password is used. If a different key/password is used, the user will be prompted to input as before
• Download/upload run concurrently and do not block GUI anymore
• Added more info in the status bar during remote sync
• Values (command, url, port etc.) used in the remote sync are stored in the config and are not lost anymore when restarting KeepassXC
• Braces in the filename have been removed

[t-h-e]

@droidmonkey Can you help me figure out what is wrong with my tests? Locally everything works fine. I ran the tests on Windows 10 and Manjaro Linux. Nevertheless the pipeline fails consistently. As I cannot reproduce the failing tests, it is difficult to fix them. Can you reproduce the failing tests or have a look what is going on?

Also, which version of QT is the pipeline using? I had to remove the usage of QUuid::WithoutBraces as the symbol was not recognized. See commit https://github.com/keepassxreboot/keepassxc/pull/7222/commits/51ddff19efdf1ce9c17badf8e5a384bca2369144. This symbol was already introduced in version 5.11

[droidmonkey]

You have a crash that is occurring on one of the download tests, specifically with the scp download

[t-h-e]

Update:

• No more scp specific widget. All commands have to be defined in the same way (Previously named anyCommand)
• Multiple remote settings can be defined with a name
• Upload without sync is supported
• Menu offers the option to directly sync with a previously defined remote setting. The saved setting needs the "Add to Menu checked"

Remote sync settings:

Remote sync menu

@HexF Can you give it another look? @mazunki For now I have decided against an automated sync mechanism. Mainly to get the current version out of the door. But yes, it can be done at some point.

@droidmonkey @phoerious Can one of you give the PR a review? I think the feature has matured since the first implementation 😃

[droidmonkey]

This looks great! Will give a thorough review when I can, currently absolutely swamped

[HexF]

I'll give this a test later tonight

[HexF]

I'm a big fan of the changes made, especially the ability to save my upload/download scripts - makes all the difference. Not having to enter my password & select my keyfile is also great.

Remote Opening appears to be a bit broken - it opens for me and pulls the latest database, but when I save & sync, the database doesn't update on the remote.

Remote opening also indicates the database path as /tmp/RemoteDatabase-..... in the open dialog, which isn't very user friendly:

My Recent Databases is also filled with these databases:

Ideally, these should say something along the lines of "NextCloud Remote Database" (replacing NextCloud with the name of the remote config)

When I remotely open I get a dialog like this after hitting OK with no feedback as to what is happening:

I'm not sure that syncing is working at all for me, my commands are

# Download
rclone --config /run/secrets/thobson_rclone_config copyto nextcloud:Passwords.kdbx {TEMP_DATABASE}
# Upload
rclone --config /run/secrets/thobson_rclone_config copyto {TEMP_DATABASE} nextcloud:Passwords.kdbx

These commands to upload a file to nextcloud.

If I create an entry "Test Entry":

then click Remote Sync -> NextCloud, I confirm on the bottom right action bar I see "Uploading"

I then download the file from NextCloud, opening it in Keepass. I note the entry does not exist.

I am on commit 74220fa622989550ba54115a297d9bf45b5d35a1

My suspicion is this is to do with the actual merging step.

[t-h-e]

@HexF Thanks for the detailed feedback.

• Bug: The uploaded file in case of both databases having the same key, has not been saved. Thanks for finding this one 😃
  • In case the keys are different, it works, but probably only due to the automatic save setting
• I will add a progress bar to the dialog so that the user can see that something is happening
• I forgot about the "Recent databases". Yes that can be improved. I will have a look, but don't see it as major blocker.