Single instance by antongulenko · Pull Request #510 · keepassxreboot/keepassxc

antongulenko · 2017-04-18T13:37:43Z

Description

This PR merges an older KeepassX PR that implements a single-instance mode, meaning that an existing keepassxc instance is raised instead of starting a second one. This has been proposed here, but I did not find a corresponding pull request.
For flexibility, a command line switch should probably be added to disable the single-instance mode if desired.

Motivation and context

Single-instance mode allows raising keepassxc from the tray by running keepassxc from a script or a global keyboard shortcut.
Resolves #14.

How has this been tested?

Compiled with the following cmake flags:
-DWITH_TESTS=OFF -DWITH_XC_AUTOTYPE=ON -DWITH_XC_HTTP=ON -DWITH_ASAN=ON
Everything seems to work fine. When running keepassxc repeatedly, it prints a message, notifies the running instance, and exits.

Types of changes

✅ New feature (non-breaking change which adds functionality)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

TheZ3ro · 2017-04-18T14:10:00Z

src/gui/Application.cpp

+    if (userName.isEmpty()) {
+        userName = qgetenv("USERNAME");
+    }
+    QString identifier = "keepassx2";


This should be changed to keepassxc

Commit pushed.

droidmonkey · 2017-04-18T21:13:16Z

I do not agree with this implementation as it adds the networking dependency to all builds. We explicitly moved networking out of the core build so that a totally unconnected app can be built.

phoerious · 2017-04-19T08:00:12Z

Can't we do that via DBus instead?

vsvyatski · 2017-04-20T14:59:10Z

Just an idea: it can be theoretically done via a mutex. Some of you may not like Microsoft, but nevertheless I've found that idea on their site (it's MFC most likely, but the idea can be borrowed) https://support.microsoft.com/en-us/help/243953/how-to-limit-32-bit-applications-to-one-instance-in-visual-c
Qt has the QMutex class.

antongulenko · 2017-04-21T11:00:23Z

Thanks for the feedback!
Unfortunately I lack the know-how to quickly re-implement this with another mechanism.
I hope that one of the devs will take this on, I think it would really increase the usability of keepassxc.

TheZ3ro · 2017-04-24T23:45:19Z

Better doing this with DBus or Mutex? @phoerious @droidmonkey

droidmonkey · 2017-04-25T01:22:01Z

Yes i think so. I do not want to reintroduce a network dependency for this feature. There are better ways to handle this.

nicwortel · 2017-04-28T09:34:33Z

I just wanted to point out that I stumbled upon this PR because I was looking for a solution to limit KeePassXC to a single instance. I often accidentally open a second instance instead of opening the existing one.

Since the linked issue (#99) is more about adding a key combination than about (the ability of) limiting to a single instance, I figured I'd drop this comment here.

As a final note, thanks everyone for your work! 👍

TheZ3ro · 2017-04-28T15:53:02Z

Currently DBus is supported only on Linux

Seems that the only cross-platform option are:

setting a QLockFile in the user tmp folder
using a named QSharedMemory

http://blog.aeguana.com/2015/10/15/how-to-run-a-single-app-instance-in-qt/

The implementation in this PR is already using a QLockFile but add a QLocalServer

sledgehammer999 · 2017-05-01T02:15:46Z

My 2 cents: There is also qtsingleapplication. We use it in qBittorrent and seems to work across all platforms.

TheZ3ro · 2017-05-01T11:05:48Z

@sledgehammer999 thanks for the suggestion.
I've seen that but unfortunately QtSimpleApplication is using QLocalServer and QLocalSocket.

From QLocalSocket documentation:

On Windows this is a named pipe and on Unix this is a local domain socket.

But still you will need to compile with Qt5::Network and like @droidmonkey said:

I do not agree with this implementation as it adds the networking dependency to all builds. We explicitly moved networking out of the core build so that a totally unconnected app can be built.

Keisial · 2017-05-02T00:44:25Z

What do you define as as «totally unconnected app»? And what is the goal of getting that?

You could call directly the low-level functions, thus avoiding the usage of Qt5::Network, but it would be doing the same in disguise. It may be worth if you just want to avoid linking with the library, though. It's not as if the lack of linking with Qt5::Network meant that the binary would be unable to perform network calls.

droidmonkey · 2017-05-02T03:26:21Z

Its more of a symbolic gesture to those individuals that want some greater assurance that their passwords are not potentially exposed to a network interface of any kind.

It seems to me that a temporary lock file could easily do the trick. Just have to be clever about app crashes (store pid in the file and check for running?)

phoerious · 2017-05-02T07:16:57Z

QLockFile does that already.

TheZ3ro · 2017-05-02T21:42:16Z

I was trying to develop a solution without using any Qt5::Network API.

The main thing is:
In the PR implementation the first started instance place a QLockFile. The newer instances will try to lock that file but seeing that is already locked will fail and connect to the QLocalServer socket. The first instance then will catch the connection, close it and raise the window.

I've tried replacing the QLocalServer with unix signals and Win32 messages but that will make things more complicate.

Sincerely, I think we should include QLocalServer and use this PR.
Otherwise, by using only QLockFile we cannot raise the first instance and only detect if KeePassXC is already running.

I agree that a password manager shouldn't use the network stack at all. But like specified in the Qt docs:

QLocalSocket: On Windows this is a named pipe and on Unix this is a local domain socket.

This is not "making network connection", it will works even without a networkadapter

Keisial · 2017-05-02T23:33:43Z

Not linking the Qt5::Network API is indeed symbolic. I would simply accept that library and replace it with an optional module that -on modern Linux- sets up a seccomp sandbox forbidding any socket(2) call where domain is not AF_LOCAL.
Done early enough, that guarantees the keepassxc program won't open a network connection (there are other means of exfiltration, of course). It should be admitted that KeePassHTTP would fail when such extension is enabled.

The main issue I see is that the seccomp filter is inherited through fork+execve. So, if the user opened an url, and that launched a new browser. That browser descendant of keepassxc would be unable to connect to the network (and probably fail bizarrely).

droidmonkey · 2017-05-07T02:45:41Z

I think this PR needs to be properly brought up to speed with the KeePassXC code base. We already integrate the Qt5::Network plugin with HTTP so it shouldn't be added to CMake.

Also, this does not resolve #99 since that is asking for a global shortcut that raises and lowers the window similar to autotype shortcut. In fairness that issue was modified several times.

TheZ3ro · 2017-05-07T11:27:45Z

@droidmonkey This is not meant to resolve #99 but rather #14

droidmonkey · 2017-05-07T12:32:17Z

Yah I know, i edited the description to say that 😉

TheZ3ro · 2017-05-07T14:54:23Z

Missed the edit in the first post 😄

TheZ3ro · 2017-05-08T14:20:57Z

What type of works this PR needs? Can we use QLocalServer for detecting running instances?

droidmonkey · 2017-05-08T18:57:35Z

Let's do it, makes the most sense

TheZ3ro · 2017-05-08T20:04:22Z

This PR already use it. It needs only a rebase and should be fine/ready to merge

Closes keepassxreboot#193

antongulenko · 2017-05-09T15:37:42Z

I rebased the PR, please check if it matches your expectations.

phoerious · 2017-05-14T13:43:05Z

So, this still requires QNetworking?

TheZ3ro · 2017-05-14T14:51:55Z

@phoerious Yes, for QLocalSocket

phoerious · 2017-05-21T19:06:37Z

src/main.cpp

+                        mainWindow.raise();
+                        mainWindow.activateWindow();
+                    });
    QObject::connect(&app, SIGNAL(openFile(QString)), &mainWindow, SLOT(openDatabase(QString)));


Shouldn't all the stuff after this be skipped when another instance is already running?

Actually there is a return 0 at line 61 (if another instance is already running).

(Maybe should that be return 1 instead?)

droidmonkey

Need to fix windows compile errors separately in ScreenLockListenerWin.cpp

- Added YubiKey 2FA integration for unlocking databases [#127] - Added TOTP support [#519] - Added CSV import tool [#146, #490] - Added KeePassXC CLI tool [#254] - Added diceware password generator [#373] - Added support for entry references [#370, #378] - Added support for Twofish encryption [#167] - Enabled DEP and ASLR for in-memory protection [#371] - Enabled single instance mode [#510] - Enabled portable mode [#645] - Enabled database lock on screensaver and session lock [#545] - Redesigned welcome screen with common features and recent databases [#292] - Multiple updates to search behavior [#168, #213, #374, #471, #603, #654] - Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480] - Fixed auto-type errors on Linux [#550] - Prompt user prior to executing a cmd:// URL [#235] - Entry attributes can be protected (hidden) [#220] - Added extended ascii to password generator [#538] - Added new database icon to toolbar [#289] - Added context menu entry to empty recycle bin in databases [#520] - Added "apply" button to entry and group edit windows [#624] - Added macOS tray icon and enabled minimize on close [#583] - Fixed issues with unclean shutdowns [#170, #580] - Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515] - Compare window title to entry URLs [#556] - Implemented inline error messages [#162] - Ignore group expansion and other minor changes when making database "dirty" [#464] - Updated license and copyright information on souce files [#632] - Added contributors list to about dialog [#629]

TheZ3ro reviewed Apr 18, 2017

View reviewed changes

droidmonkey added high priority 🚨 needs work labels May 7, 2017

droidmonkey mentioned this pull request May 7, 2017

Add DBus support #283

Merged

Raise existing instance

d1310b3

Closes keepassxreboot#193

antongulenko force-pushed the single-instance branch from 1f8f50f to 65edccb Compare May 9, 2017 12:07

Fixed string literals

58463bc

antongulenko force-pushed the single-instance branch from 65edccb to 58463bc Compare May 9, 2017 12:13

TheZ3ro approved these changes May 11, 2017

View reviewed changes

phoerious added new feature and removed high priority 🚨 needs work labels May 14, 2017

TheZ3ro requested review from droidmonkey and phoerious May 21, 2017 17:47

phoerious reviewed May 21, 2017

View reviewed changes

droidmonkey added this to the v2.2.0 milestone May 26, 2017

TheZ3ro requested review from louib and weslly May 26, 2017 08:35

phoerious approved these changes May 26, 2017

View reviewed changes

weslly approved these changes May 26, 2017

View reviewed changes

droidmonkey approved these changes May 27, 2017

View reviewed changes

Merge branch 'develop' into single-instance

adf61d9

TheZ3ro merged commit 22d533c into keepassxreboot:develop May 27, 2017

droidmonkey mentioned this pull request May 30, 2017

Add ability to toggle window visibility (to/from Tray) [$30] #99

Open

droidmonkey mentioned this pull request Jun 7, 2017

[feature] Allow only 1 instance of the program option #627

Closed

maevic mentioned this pull request Aug 1, 2017

Allow multiple instances #839

Closed

mhham mentioned this pull request Nov 29, 2017

Add a shortcut to minimize to tray #1241

Closed

phoerious added pr: new feature Pull request adds a new feature and removed new feature labels Nov 22, 2024

Uh oh!

Conversation

antongulenko commented Apr 18, 2017 • edited by droidmonkey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Uh oh!

TheZ3ro Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

antongulenko Apr 18, 2017

Choose a reason for hiding this comment

Uh oh!

droidmonkey commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoerious commented Apr 19, 2017

Uh oh!

vsvyatski commented Apr 20, 2017

Uh oh!

antongulenko commented Apr 21, 2017

Uh oh!

TheZ3ro commented Apr 24, 2017

Uh oh!

droidmonkey commented Apr 25, 2017

Uh oh!

nicwortel commented Apr 28, 2017

Uh oh!

TheZ3ro commented Apr 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sledgehammer999 commented May 1, 2017

Uh oh!

TheZ3ro commented May 1, 2017

Uh oh!

Keisial commented May 2, 2017

Uh oh!

droidmonkey commented May 2, 2017

Uh oh!

phoerious commented May 2, 2017

Uh oh!

TheZ3ro commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keisial commented May 2, 2017

Uh oh!

droidmonkey commented May 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheZ3ro commented May 7, 2017

Uh oh!

droidmonkey commented May 7, 2017

Uh oh!

TheZ3ro commented May 7, 2017

Uh oh!

TheZ3ro commented May 8, 2017

Uh oh!

droidmonkey commented May 8, 2017

Uh oh!

TheZ3ro commented May 8, 2017

Uh oh!

antongulenko commented May 9, 2017

Uh oh!

phoerious commented May 14, 2017

Uh oh!

TheZ3ro commented May 14, 2017

Uh oh!

phoerious May 21, 2017

Choose a reason for hiding this comment

Uh oh!

TheZ3ro May 25, 2017

Choose a reason for hiding this comment

Uh oh!

droidmonkey left a comment

Choose a reason for hiding this comment

Uh oh!

antongulenko commented Apr 18, 2017 •

edited by droidmonkey

Loading

TheZ3ro Apr 18, 2017 •

edited

Loading

droidmonkey commented Apr 18, 2017 •

edited

Loading

TheZ3ro commented Apr 28, 2017 •

edited

Loading

TheZ3ro commented May 2, 2017 •

edited

Loading

droidmonkey commented May 7, 2017 •

edited

Loading