FdoSecrets: Major Refactor and Code Consolidation

[Aetf]

This PR is another attempt to implement the optional confirmation for accessing passwords. Different from #4733, this time QDBusVirtualObject is used as the bridge between our business logic and the dbus interface, allowing greater access to caller information, so that the per-item locked state is correctly reported depending on the asking client.

This replaces #4733 and fixes #3837. The following changes are included:

• Refactor: use QDBusVirtualObject to interact with dbus instead of QDBusAbstractAdaptors
• Refactor: remove complex DBusReturnrelated types and replace with simpler DBusResult
• Refactor: greatly reduce the complexity of DBusObject
• Refactor: move dbus related files to separate folder
• New feature: implement per-client state managed by the new DBusMgr, moving session cipher negotiation state to per-client state
• New feature: implement optional confirmation dialog for accessing passwords

Moving to QDBusVirtualObject

The limitation of #4733 is that when using QDBusAbstractAdaptor to expose objects on DBus, Qt provides no way to get caller info when accessing object properties, which means we can't properly report locked state.

While I'm working on reviving #4733, I found that despite what I said, it's not hard to switch to QDBusVirtualObject and it actually helps reduce code complexity and creates cleaner separation between business logic and dbus handling boilerplates.

The new DBusMgr handles dbus service/path registration, signal relay, and method delivery. It also provides a centralized place to manage caller information. The comment on the class gives more implementation details about method delivery and Qt metatype registration.

New auth dialog

The new authentication dialog is similar to the one used in the browser plugin and allows the user to trust the client for all requests or only selected ones. The wording still needs improvements to maybe highlight the remembering only lasts for the duration of a single connection. Suggestions are welcome.

Currently, there is no way to only allow access once. But I think the current functionality is enough for an MVP.

The blocking prompt method

In some cases, the UnlockPrompt::prompt is blocking and has the potential to timeout the client. But this problem is not unique to this PR. Other prompts all have the same problem. The proper implementation should probably use a QTimer to post the actual dialog showing in the event loop and return immediately. This can be left to another PR.

TODO

There is something I wanted to finish before I consider this out of WIP, but I want to share it early to get feedback while I'm working on it. All done.

• Do exe link double-checking as outlined in [RFC] fdosecrets: add optional confirmation to secret access #4733 (comment)
• Update test code to work with changed interface
• Update text in the settings page to use consistent wording when referring to passwords

Screenshots

Testing strategy

Updated existing tests to match the new internal API. Also added new test cases.

Type of change

• ✅ New feature (change that adds functionality)
• ✅ Refactor (significant modification to existing code)

While this touches a good number of files, many of them contain only trivial changes (i.e. changes to function parameters and such). Most of the work is in dbus/*.

[Aetf]

Updated the description.

Loop in people active in the original PR: @jkloetzke, @Gigadoc2, @einsweniger, @intika, @fushinari

@droidmonkey I know this PR touches many files. I tried my best to concentrate one commit to one thing, but the result is still quite messy. Please let me know is there anything I can do to make the review easier.

[einsweniger]

I really like the change to the single point of responsibility (DBusMgr), and the way connection/sessions are now handled.
335857e is a bit noisy since you mixed formatting and code changes, otherwise I'd say it's very pretty and nicely done until now. Thank you for picking this up!

[Aetf]

All existing tests should be green 🎉. Working on tests for the new confirmation dialog

335857e is a bit noisy since you mixed formatting and code changes

I'll see later if I can somehow improve this.

[Aetf]

Ready for review. The alarm in CI CodeFactor is a false positive: it's a completely different access method and has nothing to do with file access.

[droidmonkey]

Excellent! Will review this on Wednesday

[intika]

Great :) 👍

1. For the dialog you should make the requesting application more visible/readable... may be by changing to bold the path text and making it on a single line ex:

   /usr/bin/secret-tool (PID: 416908)
   is requesting access to the following entries:

2. "Allow all future access" should be renamed to "Allow All" if the requester is ran again (have a different PID), it won't be granted access if we had chosen allow all future access

3. Dialog buttons (just a suggestion) should be displayed like:
   [Deny All] ................[Allow All] [Allow Selected]

4. I don't know if it's related to this feature but while testing the branch Aetf:feature/fdo-auth when running the app and after opening the secret service database, exiting the application without a kill command is impossible, the app just hang and is no more responsive (after right click, quit on the tray icon whether the db is locked or not... but this does not happen if we did not unlock the db in the first place) this require more testing... eventually using mainstream dev branch... (i had no output error on the terminal while testing...) note that i don't have this issue with v2.6.2
   Update: tested with the main develop branch (v2.7.0) without this PR and i still have this issue... so it's not related to this PR

[droidmonkey]

Why not re-use the browser access request dialog? Can rename it to a general "EntryAccessRequest".

[Aetf]

Why not re-use the browser access request dialog? Can rename it to a general "EntryAccessRequest".

I wanted to. The code was mostly copied from the browser access request dialog. But there are some subtle differences in the auth model between the browser and fdosecrets, e.g. the remember decision in browser is permanent, browser can also remember deny decisions, while in fdosecrets we have at most per client per connection memory.

And I think we haven't settle down on what should be in the fdosecrets access dialog, e.g. whether we show warnings or tooltips about the client identity isn't that reliable, and the decision is only remembered for the duration of a connection.

So IMHO first iterate the dialog in fdosecrets and then when we know for sure what functionality we need for the auth dialog, we can make the browser auth dialog a generic one.

[droidmonkey]

sounds good, I just see a whole lot of similarities and would personally have chosen customization flags to the access control dialog over re-implementation. I can see other future uses of this dialog (for example CLI access to a GUI opened database).

[droidmonkey]

I couldn't get the "Allow all future accesses" to work when using secret-tool to test.
I cleaned up the commits.

[Aetf]

@droidmonkey After a second thought, the DBusTransaction doesn't make accessing client info explicit without a significant amount of rewrite:

• as long as the logic is split into functions, those functions need a way to access client info anyway.
• in addition to dbus calls resulting in a prompt, there are also many simple dbus calls don't require a prompt/transaction. Forcing them into one means lots of code changes without obvious benefits

Stepping back a bit, our goal is to make the client info access explicit, so code in the future doesn't accidentally read client info when there's none and also always read the correct one in case of concurrent calls. To achieve this explicitly, it's unavoidable to pass a client parameter around.

It is however possible to make this parameter optional, and only use it when the function really needs it.

So here's my latest implementation:

• each dbus exposed method can request an optional const DBusClientPtr& as the its parameter. This is the only source where objects can get a client pointer
• DBusMgr knows the method signature, and can provide the optional parameter when it delivers the dbus call
• if the function want to access the client info later, i.e. in a signal handler, it now has to explicitly save the info as a weak pointer, and check for validity in the handler

Overall, this approach doesn't require much change to the codebase, but removes the need to have a static global context.

[Aetf]

By the way, does the "Remember" feature rely solely on PIDs? If so, it might be vulnerable to PID reuse attacks.

@yan12125 The "remember" feature uses DBus address, which is guaranteed by the DBus spec to be unique. There are some good discussions about PID in #4733, but those are about showing the PID and program name to the user.

Reguarding to your issue about the auth dialog shows up twice, could you try to run the unittest and see if there's any failures? There's test making sure the remember feature is working correctly. If the test passes but the issue is still there, then we can investigate in details.

[droidmonkey]

@Aetf excellent work and that definitely addresses my concern and the assert issues

[yan12125]

The "remember" feature uses DBus address, which is guaranteed by the DBus spec to be unique. There are some good discussions about PID in #4733, but those are about showing the PID and program name to the user.

Great! Thanks for the information.

Reguarding to your issue about the auth dialog shows up twice, could you try to run the unittest and see if there's any failures? There's test making sure the remember feature is working correctly. If the test passes but the issue is still there, then we can investigate in details.

With the latest patchset, all tests pass on Arch Linux, but I still got two prompt dialogs for nextcloud-client.

Looking into my logs at #5747 (comment), both requests are from the same sender :1.164. It is still the case when I tested with the latest patchset. I think that is the D-Bus address?

[yan12125]

Ah, is the "Remember" feature done per entry? Then two prompt dialogs for nextcloud-client are expected as those are for different entries... username:https://nextcloud.domain/:0 and username_app-password:https://nextcloud.domain/:0. If so,

Your decision will be remembered for the duration the requesting client is running.

Might be better as

Your decision for this entry will be remembered for the duration the requesting client is running.

[Aetf]

@yan12125 Yeah it's per entry. I updated the text to be more clear.

[Aetf]

@droidmonkey Ready for another round of review :)

[droidmonkey]

Made final conversion of DBusMgr references to QSharedPointer's. This is ready for squash and merge. Due to the back and forth between all the commits here I am going to squash into one commit with a long message.

[Aetf]

After converting dbusobject to use qsharedpointer, their original parent should save a copy of the pointer. Right now Service only saves non owning raw pointer to collections, so does collection/item and prompt. So new objects are dropped immediately.

Sorry, never mind the above. I was on my phone and missed something and thought you converted every dbusobject to use Qsharedpointer.

[droidmonkey]

DONE! You earned a cookie for this one 🍪

[Aetf]

Big thanks to everyone involved!

[yan12125]

Congratulations!

[michaelk83]

The spec is completely unclear about what is expected from the prompt complete signal, though. But I think I can match the return value to what is expected by libsecret.

@Aetf You should post this and other spec issues you've encountered, such as lack of InteractiveAuthorizationRequired support, to https://gitlab.freedesktop.org/xdg/xdg-specs/-/issues . You're probably the most qualified person here to do that.

PS: Thanks for all the great work! Also to the maintainers etc!