interfaces: add password-manager-service implicit classic interface (LP: #1653769)

[jdstrand]

This interface builds on the work from @mvo5 in #3435 to address LP: #1653769.

After review of the secret-service and kwallet DBus client APIs I found that with typical usage they rely heavily on DBus member data as opposed to application-specific object paths. As a result, AppArmor cannot be used to mediate snap-specific keyrings and passwords and the accesses allowed in this PR give full access to these client APIs. While it is possible with the secret-service to force snap-specific object paths, upstream secret-service advises against this, so in practice client software does not do this (not to mention, (ab)using the secret-service API would result in information leaks to other snaps for non-secrets (passwords) because it wasn't really designed to be used this way).

Because of the allowed DBus access, this interface grants access to open collections/wallets (aka keyrings) to any application and therefore sensitive user data. In practice this results in all saved passwords because the default keyring is unlocked upon login on a typical desktop system. This interface is therefore set to be manually connected only. This interface should be considered transitional for now. For the future, improvements could be implemented in numerous ways to make secret-service and kwallet securely enforce application isolation, but probably the easiest would be to employ a DBus proxy service (ideally it would consult LSMs like AppArmor, but it wouldn't strictly have to). To employ a proxy, snapd would disallow direct communications with the real session bus (eg, via security policy where the bus is an abstract socket or via security policy/bind mount where it is a named socket), set DBUS_SESSION_BUS_ADDRESS to point the proxy, then the proxy would mediate access from the app to various services (again, this could tie into LSMs). This is similar to how portals works today. Also note, that upstream portals has no intention of improving secret-service or adding a portal for it.

It should be noted that secret-service typically encrypts secret data to applications as it travels over DBus which prevents leaks via dbus or X sniffing. Eg, a malicious application Mallory (snap or otherwise) cannot dbus-sniff or X11-sniff application Alice to obtain the secrets as they are transferred from secret-service to Alice (assuming Mallory can't MITM). I mention this because while I consider this a transitional interface that will be plugged alongside the x11 interface, the fact that x11 is in use doesn't negate the benefits of secret-service. More to the point, IMO just because x11 is autoconnected doesn't mean that this interface should also be.

I tested this with a variety of tools (secret-tool, qdbus, seahorse and chromium-browser (with --password-store=basic|gnome|kwallet).

Important note regarding electron applications: this interface was requested specifically to enable electron applications to use the password manager (because I wasn't given an electron snap that saved passwords, I had to use chromium-browser) but I strongly advise that electron applications use --password-store=basic in electron-builder/etc and not automatically include password-manager-service in their plugs since then all electron developers will then start to request an auto-connection of this interface, which would be inappropriate for most applications. Another option would be to have electron applications use --password-store=default which will make chromium try secret-service, then kwallet and fallback to basic and therefore password management would work. This has the added benefit that if the developer wants to plugs password-manager-service, the interface does not have to be connected to have a working password store and the user can opt into it. The downside is this will undoubtedly cause confusion since the probing chromium does will trigger apparmor denials on every launch.

[codecov-io]

Codecov Report

Merging #3525 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3525      +/-   ##
==========================================
+ Coverage   76.82%   76.83%   +<.01%     
==========================================
  Files         379      380       +1     
  Lines       26314    26324      +10     
==========================================
+ Hits        20216    20225       +9     
- Misses       4304     4305       +1     
  Partials     1794     1794

Impacted Files	Coverage Δ
interfaces/builtin/password_manager_service.go	100% <100%> (ø)
interfaces/sorting.go	90% <0%> (-6.67%)	⬇️
cmd/snap/cmd_aliases.go	96% <0%> (+2%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75e3302...1851200. Read the comment docs.

[jdstrand]

Accidentally closed. Reopening.

[chipaca]

Looks good. Needs deconflicting.

Also, some nits :-)

[chipaca]

a nit: if this fails, you'd get a much nicer error message if you were using MATCH instead of grep -Pzq.
MATCH is implemented via grep -E, so you'd have to drop the initial (?s).*?\n from DISCONNECTED_PATTERN, but I'm not sure that's buying you anything.

The same applies to the following greps also, of course.

[jdstrand]

Done

[zyga]

Small code layout tweaks. Feel free to ping me to edit if you are busy.

[zyga]

This now belongs in interfaces/builtin/password_manager_service.go

[jdstrand]

Yep, done

[zyga]

This is now spelled "partial"

[jdstrand]

@zyga - I think I did the code layout changes, but I don't know what you are referring to wrt "partial".

[mvo5]

What @zyga means is that now snap debug confinement will output either "strict" or "partial"

[jdstrand]

Done

[jdstrand]

@zyga, I think I addressed everything, can you take another look?

[zyga]

Looks good, thank you!

[zyga]

CC @fgimenez can we get this snap built and uploaded into the store?