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

Port to libsecret #439

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Port to libsecret #439

wants to merge 1 commit into from

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Jul 16, 2020

libgnome-keyring library has been deprecated for a long time now, it has been replaced by libsecret library. libsecret uses Freedesktop.org’s Secret Service D-Bus API, which is supported by GNOME Keyring, so the old keyring (called collection in Secret Service parlance) should be still usable.

Major differences are:

  • objects are now D-Bus objects and their properties can be watched for changes, most of them are also loaded automatically
  • items are no longer identified by an unsigned integer, applications should retrieve items based on their attributes. It is also possible to identify an item by its DBus object path
  • password prompt is now handled by the secret manager (e.g. GNOME Keyring)

Relevant documentation:

It was not clear from the documentation whether the keyring name set by libgnome-keyring is the same as alias in libsecret, I will need to find out how to actually use this code so that I could check that.
Some items are also not loaded by default; we will need to double check that there are no code paths forgetting to load the necessary data. Lastly, we will need to decide what to do on failures and check that all re-raised exceptions are caught. Maybe we should also use the object paths instead of iterating on labels

libgnome-keyring library has been [deprecated] for a long time now, it has been replaced by libsecret library. libsecret uses Freedesktop.org’s Secret Service D-Bus API, which is supported by GNOME Keyring, so the old keyring (called collection in Secret Service parlance) should be still usable.

Major differences are:
* objects are now D-Bus objects and their properties can be watched for changes, most of them are also loaded automatically
* items are no longer identified by an unsigned integer, applications should retrieve items based on their attributes. It is also possible to identify an item by its DBus object path
* password prompt is now handled by the secret manager (e.g. GNOME Keyring)

Relevant documentation:

* [PyGobject docs]
* [Secret Service spec]
* [libgnome-keyring docs]
* [libsecret docs]
* [migration guide]

[PyGobject docs]: https://lazka.github.io/pgi-docs/index.html#Secret-1/classes/Service.html#Secret.Service
[Secret Service spec]: https://specifications.freedesktop.org/secret-service/latest/re01.html
[libgnome-keyring docs]: https://developer.gnome.org/gnome-keyring/stable/gnome-keyring-Simple-Password-Storage.html
[libsecret docs]: https://developer.gnome.org/libsecret/unstable/SecretCollection.html#secret-collection-for-alias
[deprecated]: https://wiki.gnome.org/Initiatives/GnomeGoals/LibsecretMigration
[migration guide]: https://developer.gnome.org/libsecret/unstable/migrating-api.html
@@ -272,10 +266,16 @@ def get_password(self, entry):
return result

# get password
item_info = self.__get_entry_info(entry)
item = self.__get_entry(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should also use the object paths instead of iterating on labels.

@@ -234,11 +226,14 @@ def remove_entry(self, entry):
return result

# get entry id
entry_id = self.__get_entry_id(entry)
item = self.__get_entry(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should also use the object paths instead of iterating on labels.

@@ -294,56 +294,46 @@ def get_attributes(self, entry):
return result

# get password
item_info = self.__get_entry_info(entry)
item = self.__get_entry(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should also use the object paths instead of iterating on labels.

if info is not None:
info.set_display_name(new_name)
keyring.item_set_info_sync(self.KEYRING_NAME, entry_id, info)
item = self.__get_entry(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should also use the object paths instead of iterating on labels.

if info is not None:
info.set_secret(secret)
keyring.item_set_info_sync(self.KEYRING_NAME, entry_id, info)
item = self.__get_entry(entry)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should also use the object paths instead of iterating on labels.

return True
except GLib.Error as error:
print(error)
return False
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maube we should raise PasswordStoreError but it is not expected anywhere.

@MeanEYE
Copy link
Owner

MeanEYE commented Jul 17, 2020

This is the matter we discussed on IRC few nights ago. It seems libsecret is the way to go and I especially like the fact popup dialog is handled by the library/keyring manager itself rather than entrust that to Sunflower. However for this to be further developed and merged it requires mount manage to start working (among other things).

So I assume this is on hold until we get that part of software working?

@jtojnar
Copy link
Contributor Author

jtojnar commented Jul 17, 2020

I guess we could test it by creating keys in sunflower 0.3 but it can wait.

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