Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Use libsecret instead of gnome-keyring. #53

Closed
wants to merge 8 commits into from
Closed

Conversation

odensc
Copy link
Contributor

@odensc odensc commented Nov 19, 2016

This PR switches keytar to use libsecret.

This allows compatibility with any wallet/keyring implementing the Secret Service API (including KWallet and GNOME Keyring)

The difference between this PR and #47 is that (in limited field testing) this one keeps compatibility with previously stored GNOME Keyring passwords.

This would supersede #46, #47 and close #17, #7.

@odensc
Copy link
Contributor Author

odensc commented Nov 19, 2016

Looks like 0.10 build is broken, but doesn't seem to be related to this PR.

imlucas added a commit to mongodb-js/node-keytar that referenced this pull request Dec 28, 2016
imlucas added a commit to mongodb-js/node-keytar that referenced this pull request Dec 28, 2016
* Plucked from upstream atom#53

* Debugging travis
@mofux
Copy link

mofux commented Jan 23, 2017

Looks good. Is there any chance this will get merged anytime soon?

@cruzerld
Copy link

Is there any progress on this? This PR keeps keytar from working on RHEL and centOS. Seems like the only thing failing is the node 0.10 tests due to a bad param passed in.

@anaisbetts
Copy link
Contributor

This is almost certainly a breaking change, as anyone upgrading from a previous version of keytar has all their user's keys disappear. What's the migration path for user data here?

@odensc
Copy link
Contributor Author

odensc commented Feb 16, 2017

@paulcbetts, I'm glad you asked -

Currently with gnome-keyring, keytar uses the GNOME_KEYRING_ITEM_GENERIC_SECRET schema identifier.

As seen here, that schema uses org.freedesktop.Secret.Generic on the backend.

By setting our libsecret schema identifier to org.freedesktop.Secret.Generic, we can keep compatibility with previous keys stored by gnome-keyring. I have tested this on an Ubuntu 16.04 VM and it is compatible with previous keys.

If you'd like - I can create a simple test script that uses both versions of keytar and checks if the keys are the same.

@cruzerld
Copy link

If it helps the discussion any, I just pulled in the mongodb keytar fork and rebuilt my .deb and it was able to pick up the pw just fine from gnome-keyring.

@odensc
Copy link
Contributor Author

odensc commented Feb 16, 2017

Thanks for the confirmation, @cruzerld. What distro was that on? I've only tested on Ubuntu so far.

@cruzerld
Copy link

cruzerld commented Feb 16, 2017

Also Ubuntu. 16.04 specifically. I did confirm that I needed your change before even running on centOS / RHEL. So I guess we won't have to worry about upgrading there.

I haven't tried using this with KWallet, but if it is using the underlying dbus implementation then I'm not as worried about that.

@anaisbetts
Copy link
Contributor

@TheSBros This is an excellent answer. If the tests pass, I'm 👍 on merging this, /cc @kevinsawicki

@odensc
Copy link
Contributor Author

odensc commented Feb 16, 2017

@paulcbetts Great! The build passes, except for 0.10 - which seems to be related to the C++ build environment that Travis uses.

@thomasjo
Copy link

I was actually just about to begin working on a PR for this, so a huge 👍 from me.


Additionally, why are we still building against Node v0.10? That should be dropped immediately IMHO.

@odensc
Copy link
Contributor Author

odensc commented Feb 17, 2017

I can go ahead and drop 0.10 if everyone agrees. (LTS was dropped for it on Oct 31st)

@cruzerld
Copy link

Any movement on this?

script/cibuild Outdated
@@ -4,5 +4,4 @@ sudo service dbus start
eval $(gnome-keyring-daemon)
export GNOME_KEYRING_CONTROL GNOME_KEYRING_PID GPG_AGENT_INFO SSH_AUTH_SOCK

# FIXME Find out why we cound not connect to gnome-keyring-daemon.
# grunt test
grunt test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is still a no-go since all the tests timed out.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Judging from the output on Travis, we're not actually running any tests... the script is actually failing, but old versions of xvfb-run seem to be returning hiding that fact, and always eturning with a success exit code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BinaryMuse BinaryMuse mentioned this pull request Apr 5, 2017
@odensc
Copy link
Contributor Author

odensc commented Apr 7, 2017

I've disabled cibuild again. It now passes Travis, albeit w/o tests - which isn't a regression from before. (tests pass on my machines: Ubuntu 16.04, Arch Linux)

Alternatively #63 seems like a good PR; but contains a lot of other changes which may require more thought.

Is this good enough to merge, or could I get some guidance on what we can do to get this moving? A lot of electron applications use this module, and right now gnome-keyring needs to be included as a dependency in Linux distro packages - which will cause the user to have diverging wallet systems when they're not using a GNOME-compatible DE. I'm sure lots of people will be happy once that dependency is removed.

cc/ @kevinsawicki @paulcbetts

@BinaryMuse
Copy link
Contributor

Thanks so much for your work on this, @TheSBros! It's incorporated into #63 which is available as keytar@4.0.2. 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect and use KDE instead of GNOME
7 participants