Use secret-service 1.5.0 #1
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- use static method `SimpleCollection.isAvailable()` - handle `AccessControlException`s
|
The actual changes are good (especially the static But a integration test revealed an issue. Using this lib with Cryptomator leads to a) The application blocks for two minutes (aka 120seconds) if the unlock dialogue of the secret service is canceld. This makes it nearly unusable. Later, a locked keyring is also not utterly well handled: |
infeo
requested changes
Dec 9, 2020
Do you remember if the prompt was visible in that scenario? On which system did you test?
Thanks for the feedback. There seems also to be the new https://github.com/hypfvieh/dbus-java |
The prompt was visible and one can interact with it. No matter what action I choose (Canceling or Unlocking keyring). i always have to wait 120seconds. I tested it on Ubuntu 20.04 with the latest updates (5.4.0-56 kernel, GNOME 3.36.3) inside a VM. |
|
We've got 3 problems here: First return handler.get(timeout.toMillis(), TimeUnit.MILLISECONDS);Second, the Note that I did not cancel a prompt at all. This brings us to the third problem: somehow the prompt handling seems to dismiss prompts before creating them. I suggest the following:
A branch with a changed timeout that allows to run Cryptomator with secret-service 1.2.3 and investigate things further can be found here. |
|
I released the secret-service
There are also some other changes https://github.com/swiesend/secret-service/blob/master/CHANGELOG.md @infeo and @purejava please let me know, if there are still issues! |
|
@swiesend Thanks for the update. I tested 1.3.0 with the current cryptomator snapshot on a recent ubuntu 20.10 and kubuntu 20.10. The results are mixed. On the ubuntu machine everything runs smoothly. Unlock, lock, save passwords, remove passwords work out of the box without any problem. Very nice (: Talking about the kubuntu system, things are different. First start of Cryptomator with the updated lib and a single vault I looked into the keyring selection dialogue. To my surprise the gnome keyring was selectable, hence i chose it and tried to unlock the vault. Not surprinsingly stacktraces were thrown. Was also able to check the save-password-box resulting in more stack traces of similar kind: The bottom line is, that the change in the Second, even if the Hence, maybe the |
|
I could identify two problems:
This needs more investigation. There is something weird going on. Maybe with
I order to get both versions "working" I first had to apply the changes from #4. Built and tested with Regarding to your second point @infeo from your last comment: I think this is a weird artifact, as by installing the |
swiesend
changed the title
|
@swiesend To easier testing it I added in eaa880b a simple unit test to check if the Additionally, since the github CI servers don't run a session dbus, it will also show if some errors will be thrown if it is not present. |
|
This all looks very interesting. 😃 ralph@ubuntu:~$ aptitude search libkf5wallet5 gnome-keyring
p gnome-keyring
v gnome-keyring:i386
p gnome-keyring-pkcs11
i A libkf5wallet5
p libpam-gnome-keyring
p pidgin-gnome-keyring
ralph@ubuntu:~$ ps aux | grep -i wallet
ralph 13056 0.0 0.0 8900 660 pts/2 S+ 19:25 0:00 grep --color=auto -i wallet
ralph@ubuntu:~$As you see, the After starting Cryptomator, I'll see if I can give a helping hand for the above at the weekend. |
I tested As a side note for anybody interested in testing, please note that KUbuntu 20.04 comes with The problems @infeo and I noticed with an The according code looks like this: public static boolean isAvailable() {
if (connection != null && connection.isConnected()) {
try {
DBus bus = connection.getRemoteObject("org.freedesktop.DBus",
"/org/freedesktop/DBus", DBus.class);
if (!Arrays.asList(bus.ListActivatableNames()).contains("org.freedesktop.secrets")) {
return false;
}
org.freedesktop.secret.interfaces.Service service = connection.getRemoteObject(
Static.Service.SECRETS,
Static.ObjectPaths.SECRETS,
org.freedesktop.secret.interfaces.Service.class);Another thing I noticed during testing is, that and a non working When the above mentioned pre-conditions are met, There seems to be some missing bolt in integrations-linux. Maybe the result of calling |
infeo
requested changes
Feb 12, 2021
There was a problem hiding this comment.
@purejava Thanks for also reviewing this PR and digging a little bit deeper to find the problems.
@swiesend You need to do two things in library, before the PR is merged. The broken isSupported() method must be fixed. And a graceful handling of the case, when there is no "login" keyring present. (including no warning spam in the log).
For the latter case, maybe this can be integrated into the isSupported call.
pom.xml
Outdated
| @@ -39,7 +39,7 @@ | |||
|
|
|||
| <!-- runtime dependencies --> | |||
| <api.version>0.1.3</api.version> | |||
| <secret-service.version>1.2.1</secret-service.version> | |||
| <secret-service.version>1.3.0</secret-service.version> | |||
There was a problem hiding this comment.
1.3.0 is still buggy (as is 1.4.0). This needs to be changed once a new release is out (1.4.x)
src/main/java/org/cryptomator/linux/keychain/SecretServiceKeychainAccess.java
Show resolved
Hide resolved
- fix `SimpleCollection.isAvailable()` - handle `org.freedesktop.DBus.Error.ServiceUnknown` errors - log `org.freedesktop.DBus.Error.*` errors only in debug mode
swiesend
changed the title
|
Hey folks! Sorry that I did not respond to this earlier! The issues of my I tested
@purejava Feel free to use the @infeo and @purejava can you please test this as well with the first 3 test cases. I find testing this manually in a virtual machine very tedious and error prone. I guess I had different behavior in the past as I accidentally started the launcher script as root and not as common user. |
infeo
approved these changes
Feb 19, 2021
There was a problem hiding this comment.
That is wonderful news :D
The isSupported() works now. (: If no login keyring is present, the log stays in non-debug mode clean and does not create any errors. Of course a user can still check to store the password, but it is not stored. While this is not perfect, for this PR the behaviour is sufficient.
Before merge, i'd like to wait for feedback fo @purejava to be absolutely sure to not to break anything.
|
This is great news! |
|
secret-service 1.5.0 is great! I tested it on Kubuntu 20.04.1 with and without either gnome-keyring and kwallet-daemon installed.
Did you test on Kubuntu 20.04.1 as well? In this case, this is due to the kwallet-daemon being available. The checkbox to remember a password should only get displayed in general, if a working backend is available and choosen in the prefs. On testing I noticed (and I think this is what you've experienced too) that, with gnome-keyring uninstalled and the according process killed, Gnome Keyring is available as a backend in the prefs. So this is a small bug that does not break anything but needs to be addressed in a new issue. |
You missunderstood me, this is unrelated to kdewallet. If the GNOME keyring is installed and no keyring called "login" present, you can still select to store the password. But it won't get saved and after unlocking and relocking the vault, you have to reenter the passwords. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SimpleCollection.isAvailable()AccessControlExceptions