Added config file to allow backend selection (linux) #103

akallabeth · 2017-06-12T14:53:31Z

Checking for an existing configuration file /etc/xdg/qt5-keychain.conf or /etc/xdg/qt4-keychain.conf depending on Qt version compiled.
With backend=[libsecret|kwallet4|kwallet5|gnome-keyring] a specific backend can be forced and autodetection disabled.
With disable-backend=[libsecret|kwallet4|kwallet5|gnome-keyring],[[[libsecret|kwallet4|kwallet5|gnome-keyring]]? some / all backends can be skipped during auto detection

ckamm · 2018-06-14T08:23:10Z

It would be great to get this PR merged: the ownCloud client now ships qtkeychain with the libsecret backend enabled and there will likely be people who'd prefer to continue using the kwallet backend instead (see #99). With the config file setting there'd be an option for that.

Alternatively there could be an API for controling backend choice.

I'd be up for helping to get this to work.

guruz · 2018-06-14T08:27:06Z

@ckamm did you review+test this PR?
Maybe @frankosterfeld wants to merge it then..

@frankosterfeld BTW, i think we (ownCloud client guys @ogoffart @ckamm @guruz @dschmidt ) are interested in taking over more power+responsibility here if needed.

frankosterfeld · 2018-06-14T08:32:24Z

I can merge this, although having the user to do this choice seems like the very last resort to me.

I definitely could help, I have no time for this project right now, especially those issues where I would have to test on a handful of different test setups. Especially on Linux, I'm out of the loop regarding the status of the various desktops and distributions.

ckamm · 2018-06-14T08:36:09Z

@frankosterfeld Do you know if libsecret is going to superseede kwallet on KDE? If not, maybe the detection code should prefer kwallet backends for KDE sessions, instead of always prefering libsecret?

frankosterfeld · 2018-06-14T08:38:54Z

@ckamm yeah, i'm wondering if going back to kwallet by default would make sense, if it is running. But that's the part where I'm out of loop, I don't know what the status of kwallet vs gnome-keyring vs. libsecret is these days, both in deployed systems and in current development.

ckamm · 2018-06-14T08:41:08Z

@frankosterfeld At least for gnome sessions it looks like libsecret is preferred now - libgnome-keyring will actually be removed from some distros soon.

For kde I don't know either unfortunately. It looks like mid 2016 there was talk of going to ksecretservice or libsecret: https://mail.kde.org/pipermail/plasma-devel/2016-July/055668.html

guruz · 2018-06-14T08:43:07Z

@frankosterfeld I meant it the other way: We can help reviewing+merging stuff if you want.

frankosterfeld · 2018-06-14T08:46:33Z

@guruz that would probably best, moving this out of my personal space and give you guys the correct permissions.

ckamm · 2018-06-14T09:02:37Z

There was a similar discussion here: jaraco/keyring#273 that ended up with them prefering the kwallet backend over the secret service dbus api.

ckamm · 2018-06-14T09:09:30Z

It looks like the secret service dbus api on kubuntu 18.04 is provided by gnome-keyring-daemon. Switching to libsecret will thus talk to a different keyring backend than before.

I'll make a PR for prefering kwallet on kde sessions.

frankosterfeld · 2018-06-14T08:33:11Z

keychain_unix.cpp

    }

 }

-static KeyringBackend getKeyringBackend()
+static KeyringBackend getKeyringBackend(void)


Please remove this change

frankosterfeld · 2018-06-14T08:34:25Z

keychain_unix.cpp

-            return Backend_Kwallet4;
-        }
+		if (!disabled.contains(backendGnomeKeyring, Qt::CaseInsensitive)) {
+			if ( GnomeKeyring::isAvailable() ) {


Code looks good, but please cleanup the indentation (4 spaces, no tabs)

(Had this review unsubmitted here..)

akallabeth · 2018-07-09T07:55:32Z

@frankosterfeld will have a look at it, need to rebase though.
Quick question: Would it be possible to introduce a Backend_None if all of the backends are disabled?

frankosterfeld · 2018-07-09T11:25:03Z

@akallabeth would make sense to me

guruz · 2018-07-09T11:57:18Z

@akallabeth Why do you want to use QSettings::SystemScope?
From the documentation:

If scope is QSettings::UserScope, the QSettings object searches user-specific settings first, before it searches system-wide settings as a fallback. If scope is QSettings::SystemScope, the QSettings object ignores user-specific settings and provides access to system-wide settings.

http://doc.qt.io/qt-5/qsettings.html#QSettings-1

akallabeth · 2018-07-09T12:00:34Z

@guruz thought it would make most sense to have such a setting per installation and not per user.
Additionally I didn't want to think about implications with user level settings (from a security perspective)

guruz · 2018-07-09T12:06:37Z

@akallabeth I'd think user is better (it also falls back to system).
Your KDE and Gnome are anyway running as user?

ogoffart · 2018-07-10T21:22:27Z

keychain_unix.cpp

+#if QT_VERSION >= 0x050000
+	const QString prefix = "qt5-";
+#else
+	const QString prefix = "qt4-";


I don't think we need this prefix.
The settings should apply to both version equally.

Also, there should not be so many apps using Qt4 at this point anywyay.

ogoffart · 2018-07-10T21:22:50Z

keychain_unix.cpp

+#else
+	const QString prefix = "qt4-";
+#endif
+	const QSettings settings(QSettings::SystemScope, prefix + "keychain");


Should not be system scope.

This is something the user would configure.

ogoffart · 2018-07-10T21:23:19Z

keychain_unix.cpp

@@ -76,37 +82,68 @@ static DesktopEnvironment detectDesktopEnvironment() {
    return DesktopEnv_Other;
 }

-static KeyringBackend detectKeyringBackend()
+static KeyringBackend detectKeyringBackend(void)


another useless "void"

ah, old C habits, always forget that the definition in C++ differs ^^

ogoffart · 2018-07-11T07:59:57Z

keychain_unix.cpp

+static const QString backendLibSecret = "libsecret";
+static const QString backendKwallet4 = "kwallet4";
+static const QString backendKwallet5 = "kwallet5";
+static const QString backendGnomeKeyring = "gnome-keyring";


Also, non-trivial static global object is to be avoided in libraries.

Edit: I'd use plain const char [], or not have them there at all and just use the plain text in the function.

guruz · 2018-07-13T07:44:45Z

@akallabeth Any news on when you can update this? :) Otherwise it can't go into 0.9 (which is fine)
(Ref: #111 (comment) )

Added config file to allow backend selection

a11152e

ckamm mentioned this pull request Jun 12, 2018

Reading from keychain failed with error: No keychain service available Ubuntu 18.04 owncloud/client#6455

Closed

ckamm added a commit to ckamm/qtkeychain that referenced this pull request Jun 14, 2018

Prefer kwallet over libsecret for KDE frankosterfeld#103

ed2ff1b

ckamm mentioned this pull request Jun 14, 2018

Prefer kwallet over libsecret for KDE #111

Merged

frankosterfeld reviewed Jul 6, 2018

View reviewed changes

ogoffart reviewed Jul 10, 2018

View reviewed changes

ogoffart reviewed Jul 11, 2018

View reviewed changes

michaelk83 mentioned this pull request Aug 29, 2022

Feature request: Add ability to choose the backend from an environment variable. #171

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added config file to allow backend selection (linux) #103

Added config file to allow backend selection (linux) #103

akallabeth commented Jun 12, 2017

ckamm commented Jun 14, 2018

guruz commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018 •

edited

Loading

ckamm commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018

ckamm commented Jun 14, 2018

guruz commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018

ckamm commented Jun 14, 2018

ckamm commented Jun 14, 2018 •

edited

Loading

frankosterfeld Jun 14, 2018

frankosterfeld Jun 14, 2018

frankosterfeld Jul 6, 2018

akallabeth commented Jul 9, 2018

frankosterfeld commented Jul 9, 2018

guruz commented Jul 9, 2018

akallabeth commented Jul 9, 2018

guruz commented Jul 9, 2018

ogoffart Jul 10, 2018

ogoffart Jul 10, 2018

ogoffart Jul 10, 2018

akallabeth Jul 11, 2018

ogoffart Jul 11, 2018 •

edited

Loading

guruz commented Jul 13, 2018

Added config file to allow backend selection (linux) #103

Are you sure you want to change the base?

Added config file to allow backend selection (linux) #103

Conversation

akallabeth commented Jun 12, 2017

ckamm commented Jun 14, 2018

guruz commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018 • edited Loading

ckamm commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018

ckamm commented Jun 14, 2018

guruz commented Jun 14, 2018

frankosterfeld commented Jun 14, 2018

ckamm commented Jun 14, 2018

ckamm commented Jun 14, 2018 • edited Loading

frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

frankosterfeld Jun 14, 2018

Choose a reason for hiding this comment

frankosterfeld Jul 6, 2018

Choose a reason for hiding this comment

akallabeth commented Jul 9, 2018

frankosterfeld commented Jul 9, 2018

guruz commented Jul 9, 2018

akallabeth commented Jul 9, 2018

guruz commented Jul 9, 2018

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

ogoffart Jul 10, 2018

Choose a reason for hiding this comment

akallabeth Jul 11, 2018

Choose a reason for hiding this comment

ogoffart Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

guruz commented Jul 13, 2018

frankosterfeld commented Jun 14, 2018 •

edited

Loading

ckamm commented Jun 14, 2018 •

edited

Loading

ogoffart Jul 11, 2018 •

edited

Loading