-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Block and ask for unlock when applications query the secret service #4443
Comments
This is indeed one place where keepassxc is not fully adherent to the secret service spec. The spec requires the query operation to be functional even when the database is locked (i.e. the application can still search into various fields of entries without unlocking the database first). But it is technically impossible for keepassxc to do so. Because in keepassxc, once the database is locked, there is no database in memory at all. So keepassxc simply replies with empty results in this case. I guess that's why the behavior you are seeing:
As you can see, it is up to the application to request unlock the database, and there is no direct way for keepassxc to insert blocking operation (pop up a dialog) into non-blocking operations (query/saving). There are two ways to go around this:
|
is it no possible to not send a reply until the user has unlocked it? |
That's exactly what I want to avoid. DBUS reply times out in about 20s. The application will probably react much worse to that than not finding entry. While the timeout limit is configurable, it is not controlled by keepassxc. And I think limiting the user action with a timeout will do more harm than good. Unlocking the database is not always as easy as typing in the password, there may be hardware keys, password files, etc, not to mention people who just happen to type slowly. |
It's certainly a hackish solution, but it does solve a problem. another workaround I could think of is to have some way to get my startup script to only start those applications after I unlocked keepassxc, and make keepassxc keep the secrets in memory/cached when it's locked again. |
As a workaround, if your startup script is after login, and you use PAM for authentication, you can have a script to unlock keepassxc while login/unlock screen using pam_exec. It does require the login password is the same as the database, though. |
@Aetf as far as I understand, the only problem is that keepassxc does not store the "attributes" separately to the database, so when being queried for them, it cannot return them in the locked items array. As the spec states, attributes are not secret. So why not just keep a local, unencrypted copy of all attributes, that is update whenever the database gets unlocked or saved. What do you think? |
Attributes in the spec are not the same as attributes in keepassxc database. It is just an implementation detail that the keepassxc database entry custom attributes are used to implement "attributes" as stated in the spec. The custom attributes are part of the database. And I don't think it's a good idea to save them separately unencrypted, which could leak the presence of entries/groups in the database because there needs a way to associate attributes to entries. |
How about making it an optional thing? It could be a checkbox in the settings, with a warning, that enabling this would expose all non-hidden custom attribute fields of all entries in the selected group. This would prevent accidental leaks, but still improve usage for power users. I for example have a group used exclusively for secret service entries, so it would not be a problem for me. |
Is that not an option for you? |
I found something in the DBus protocol that may help the situation: The So in theory we could return this error for SearchItem requests on locked databases, and the client should retry the search with However in reality I highly doubt any client would respect the error and behave correctly, given the only working implementation of the API is gnome-keyring which can search a locked database... |
I have read the secret-service spec and i think it can work this way:
With an Prompt we don't have to worry about timeouts from DBus, since we answer all Requests in time. |
That's already what keepassxc does: returning prompt object for apps to react on if the collection is locked, just not per item locking state, which is #4733 (and now #5747) tries to do. Anyway, the problem is before the first step. The program usually starts with a search to get a path to the item it wants, and the spec expects us to be able to search even when the database is locked, and there is no way in the search API to return a similar prompt object. |
@Aetf I'm just starting out with D-Bus and Secret Service, and I wonder if you can help me understand the current situation and limitations with KeepassXC and Secret Service. Suppose I want to write a very simple client (in Python, probably) specifically to retrieve a single password from KeepassXC via Secret Service and pass it along to the process that needs it. Ideally, I'd like my client to run in the background with no UI, no terminal window, no user interaction. Given that I can accommodate whatever order KeepassXC wants things done, is it possible to open a session to KeepassXC, check whether the database is locked/ask KeepassXC to unlock the database, block while KeepassXC handles prompting the user and unlocking the database if it's not already unlocked, then retrieve the password (using the identifier attribute which is already known to the client script)? Am I right that all of the above will work as expected as long as I don't try to do a search without first ensuring the database is unlocked? I've got as far as setting up a group for the purpose and, provided the database is already unlocked, successfully retrieving a password from a test entry in the secret service group with |
@scruloose Happy to know that you are working on a new client. A standard will not be of much use if there's only one implementation for either the client or the server ;) What you describe is all possible. And you can open new issues to discuss if you find something should be possible in the spec but not implemented so in KeepassXC.
As long as your client share the same user dbus session with KeepassXC
Once you ensure the database itself is unlocked, the ordering doesn't matter. As for unlocking, it will soon become a little more complex than unlocking the database once #5747 merges. You need to be prepared to handle the per-item locking state. And don't assume the unlocked item will remain so, because the user may revoke the authorization. You should handle these anyway as that's what the spec implies. Except for what discussed in this issue, there's nothing deviated from the spec, so as long as you follow the spec, there shouldn't be many limitations.
If you want to directly construct the dbus object path from the identifier, please don't do that, as the path may change and the usage of entry UUID in the path is an implementation detail. It's fine if you get the object path by using the identifier attribute to perform a search first.
Correct.
Correct. |
So maybe I should have read the whole thread before posting this, since it's all kind of been mentioned before. I'll leave the rest up as a summary... Hi, so I have run into this issue as well and wanted to chime in.
|
Hey, so I have an idea for a workaround that I think hasn't been proposed and I think may break the deadlock facing the list of approaches suggested so far. I am ... not much of a coder, so please somebody do tell me if my idea is naive or just plain wrong, or whatever. Anyway, what I'm thinking is as a runtime thing, before re-locking an unlocked database, we stash all of its lookup attributes in memory. Some thoughts:
Does that make sense? |
Ultimately, blocking the search for unlocking is still needed, just as @scruloose said. Whether we cache the look up attributes in memory or on disk for later searches is a minor issue in comparison. Having thought about this again, I now feel that it may not be that bad to block, though. There are actually bugs (that I plan to fix but haven't got time to) in the prompt object handling code, which will block the call on prompt. And it seems no one has noticed or complained about that yet. Now I'm considering implementing this (try unlock before search). A timeout search is arguably better than a search succeeded but returning nothing. I'll give this a try and see if it works will in practice. |
Makes perfect sense to me, I like your synopsis |
BTW @Aetf while you are here. We should document in our user guide how to use secret service integration and how to disable the inbuilt gnome or whatever managers. If you go for this last major change it would be good to include the documentation and acreenshots as well. |
I'd like to add my vote in favor of the However, I agree that it doesn't solve the immediate problem for existing applications, so in the mean time we need some other solution such as proposed by @scruloose . But I do think this should be added to the Secret Service spec. The spec is still a "version 0.2 draft", so very much a work-in-progress (though apparently it hasn't been updated since 2011). As I noted elsewhere, it may be worth reaching out to Stef Walter ( One final thought: if KPXC enforces its own timeout for a blocking search attempt, and then falls back to an empty response, then return |
On further thought, most applications that talk to Secret Service should be doing that via The other possible target is QtKeyChain. GNOME/Gtk users are far more likely to use It may actually be ok to leave this broken for KDE apps that haven't migrated to QtKeyChain. That should encourage them further to migrate. So I submit an updated proposal:
This is a more holistic approach, taking into account the bigger picture outside KPXC. The advantage for KPXC is less new code and nearly zero security impact (no extra unencrypted DB schemas or attributes need to be kept anywhere). If we really want to, we can still add @scruloose 's solution with a checkbox "Workaround for ...", off by default. In that case, the difference is just the extra checkbox and the bits outside KPXC. But if @Aetf , @droidmonkey , thoughts? (sorry this is a little long) |
Thinking about the bigger picture is good. The Another benefit of fixing the spec is that the search API's asynchronousness can be well expressed within the For example, there could be a new And we can bring this up together with #5747 (comment) given the current spec is still a "draft". |
In addition to |
I think this is one place where the implementation(s) will have to lead the spec, rather than follow it. There's nothing like practical experience to iron things out. Fixing the spec is ideal, but it will take time, even after posting to https://gitlab.freedesktop.org/xdg/xdg-specs/-/issues . There needs to be some back-and-forth with Specifically re
QtKeychain uses |
At least some python clients may be using the |
Reading the spec some more. In chapter 3: Collection and Items, it says:
It's a little ambiguous, but "invalid access" can be interpreted to include lookup of a locked item (the previous sentence can be taken as an example, rather than a complete list of all invalid access types). So I think returning Furthermore, in chapter 5: Lookup Attributes, it says:
IOW, it expects the lookup attributes to be unencrypted, but does not guarantee or require it. So again, having them encrypted (and therefore returning |
Returning IsLocked on Collection.SearchItems is probably ok, because the collection itself can be locked. But Service.SearchItems explicitly returns both locked and unlocked items. So IMHO samentically it's not appropriate to return IsLocked if we stick to the current signature. And many clients don't expect to handle IsLocked error when calling SearchItems on Service. |
For
So this can be treated as 3 cases:
I agree this is a little wacky, but I think that's the best choice we have. And this does fall within "will probably [but not definitely] store attributes in an unencrypted manner" from chapter 5. If
As we discussed earlier, most of them will be using |
Fixes keepassxreboot#6942 and fixes keepassxreboot#4443 * Block and ask for unlock when applications query the secret service
Fixes keepassxreboot#6942 and fixes keepassxreboot#4443 - Return number of deleted entries - Fix minor memory leak - FdoSecrets: make all prompt truly async per spec and update tests * the waited signal may already be emitted before calling spy.wait(), causing the test to fail. This commit checks the count before waiting. * check unlock result after waiting for signal - FdoSecrets: implement unlockBeforeSearch option - FdoSecrets: make search always work regardless of entry group searching settings, fixes keepassxreboot#6942 - FdoSecrets: cleanup gracefully even if some test failed - FdoSecrets: make it safe to call prompts concurrently - FdoSecrets: make sure in unit test we click on the correct dialog Note on the unit tests: objects are not deleted (due to deleteLater event not handled). So there may be multiple AccessControlDialog. But only one of it is visible and is the correctly one to click on. Before this change, a random one may be clicked on, causing the completed signal never be sent.
Fixes keepassxreboot#6942 and fixes keepassxreboot#4443 - Return number of deleted entries - Fix minor memory leak - FdoSecrets: make all prompt truly async per spec and update tests * the waited signal may already be emitted before calling spy.wait(), causing the test to fail. This commit checks the count before waiting. * check unlock result after waiting for signal - FdoSecrets: implement unlockBeforeSearch option - FdoSecrets: make search always work regardless of entry group searching settings, fixes keepassxreboot#6942 - FdoSecrets: cleanup gracefully even if some test failed - FdoSecrets: make it safe to call prompts concurrently - FdoSecrets: make sure in unit test we click on the correct dialog Note on the unit tests: objects are not deleted (due to deleteLater event not handled). So there may be multiple AccessControlDialog. But only one of it is visible and is the correctly one to click on. Before this change, a random one may be clicked on, causing the completed signal never be sent.
Fixes keepassxreboot#6942 and fixes keepassxreboot#4443 - Return number of deleted entries - Fix minor memory leak - FdoSecrets: make all prompt truly async per spec and update tests * the waited signal may already be emitted before calling spy.wait(), causing the test to fail. This commit checks the count before waiting. * check unlock result after waiting for signal - FdoSecrets: implement unlockBeforeSearch option - FdoSecrets: make search always work regardless of entry group searching settings, fixes keepassxreboot#6942 - FdoSecrets: cleanup gracefully even if some test failed - FdoSecrets: make it safe to call prompts concurrently - FdoSecrets: make sure in unit test we click on the correct dialog Note on the unit tests: objects are not deleted (due to deleteLater event not handled). So there may be multiple AccessControlDialog. But only one of it is visible and is the correctly one to click on. Before this change, a random one may be clicked on, causing the completed signal never be sent.
Fixes #6942 and fixes #4443 - Return number of deleted entries - Fix minor memory leak - FdoSecrets: make all prompt truly async per spec and update tests * the waited signal may already be emitted before calling spy.wait(), causing the test to fail. This commit checks the count before waiting. * check unlock result after waiting for signal - FdoSecrets: implement unlockBeforeSearch option - FdoSecrets: make search always work regardless of entry group searching settings, fixes #6942 - FdoSecrets: cleanup gracefully even if some test failed - FdoSecrets: make it safe to call prompts concurrently - FdoSecrets: make sure in unit test we click on the correct dialog Note on the unit tests: objects are not deleted (due to deleteLater event not handled). So there may be multiple AccessControlDialog. But only one of it is visible and is the correctly one to click on. Before this change, a random one may be clicked on, causing the completed signal never be sent.
Expected Behavior
When app launch and the database is locked, keepassxc would popup and ask for unlock.
Current Behavior
When app launch and the database is locked, the app fails to get password, therefore asking user to enter it again (or ask to re-authenticate in case of nextcloud), keepassxc sit in silence.
Then, when the application trys to save it, keepassxc then ask user to unlock, and stores a duplicate entry.
Possible Solution
Make querying password acts the same as saving password in this case.
Steps to Reproduce
Context
It causes nextcloud to ask for auth everytime system boots up, and my mail client sometimes can't get password when keepassxc is locked.
Debug Info
KeePassXC - 2.5.3
Revision: f8c962b
Libraries:
Operating system: Arch Linux
CPU architecture: Arch Linux
Kernel: 5.5.9-arch1-2
Enabled extensions:
The text was updated successfully, but these errors were encountered: