[PM-7486] Detect Libsecret Service #8776

MGibson1 · 2024-04-16T20:45:31Z

Type of change

- [ ] Bug fix
- [ ] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We currently rely on a libsecret provider to store user-account secrets on linux. Auth tokens were moved to Secure storage if available with #7975, but this expands our dependencies to include something that provides libsecret without explicitly listing it.

This PR gracefully degrade request token storage to disk when OS-sponsored storage is unavailable.

Draft reason

Still vetting dbus and likely need build environment updates to ensure dbus headers are available to them.

Code changes

desktop_native: add a osSupportsSecuredStorage that syncronously indicates whether or not the host operating system supports encrypted disk storage.
- windows/mac: this is built into the OS and so is always true
- linux: Attempts to get an unlabeled password. Empirically, missing libsecret provider results in an Err response and indicates that the OS is not set up for user account encrypted storage.
electron platform utils service / main: use the new native interface to deterine whether or not the OS supports secure storage. This interface was already in place on TokenService
desktop-credential-listener / preload: We specifically want sync IPC in this case because the result is important for service initialization.

Before you submit

Please add unit tests where it makes sense to do so (encouraged but not required)
If this change requires a documentation update - notify the documentation team
If this change has particular deployment requirements - notify the DevOps team
Ensure that all UI additions follow WCAG AA requirements

codecov · 2024-04-16T20:51:28Z

Codecov Report

Attention: Patch coverage is 0% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 27.65%. Comparing base (7f207d2) to head (a0bffc2).

Files	Patch %	Lines
apps/desktop/src/app/services/services.module.ts	0.00%	3 Missing ⚠️
...atform/main/desktop-credential-storage-listener.ts	0.00%	2 Missing ⚠️
apps/desktop/src/main.ts	0.00%	1 Missing ⚠️
apps/desktop/src/platform/preload.ts	0.00%	1 Missing ⚠️
...atform/services/electron-platform-utils.service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8776      +/-   ##
==========================================
- Coverage   27.65%   27.65%   -0.01%     
==========================================
  Files        2376     2376              
  Lines       69193    69196       +3     
  Branches    12950    12950              
==========================================
  Hits        19133    19133              
- Misses      48624    48627       +3     
  Partials     1436     1436

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-04-16T21:21:06Z

Checkmarx One – Scan Summary & Details – 7bcf52ca-829f-4da3-baf1-9d2d3b570566

No New Or Fixed Issues Found

MGibson1 · 2024-04-17T13:25:41Z

Looking in to linker issue with desktop native module build

bwdil · 2024-04-17T14:12:24Z

@MGibson1 the commits in this PR are not signed.

MGibson1 · 2024-04-17T14:16:40Z

@bwdil thanks for the heads up! That's a catch-22 of sorts. It's because to replicate the issue I was forced to work on a system that didn't have my typical secret service set up.

MGibson1 · 2024-04-17T14:29:26Z

signed commits

bwdil · 2024-04-18T18:46:32Z

apps/desktop/desktop_native/src/password/unix.rs

+    fn list_names(&self) -> Result<Vec<String>>;
+}
+
+// Query dbus names looking for the org.freedesktop.secrets service, which indicates an active libsecret provider


I do not see any risk by simply querying dbus names and searching for org.freedesktop.secrets which is similar to performing the following:

dbus-send --session --dest=org.freedesktop.DBus --type=method_call --print-reply /org/freedesktop/DBus org.freedesktop.DBus.ListNames | grep 'org.freedesktop.secrets'

Result
string "org.freedesktop.secrets"

However, what we do after the query matters, so I reviewed the zbus project and tested the logic in your code, which produces the expected results (you know what you're doing and the approach is good).

I would note that the zbus project doesn't have any information regarding the handling of security vulnerabilities in SECURITY.md at https://github.com/dbus2/zbus/security

And since we're using zbus API to essentially broker the validation of libsecret provider on a host where we need to securely store secrets, we want to make sure that the maintainer is security conscious.

I'd note too that zbus api does not provide any assurances on what comes back from the message bus, so I think we are just trusting that org.freedesktop.secrets belongs to org.freedesktop.DBus if this is the only gate before we hand deliver secrets, then I would be concerned that using a string value from the message bus could be spoofed.

I agree with what you're saying except for

trusting that org.freedesktop.secrets belongs to org.freedesktop.DBus

We actually have no expectation that the DBus service has anything to do with what is providing the secrets service. What we're trusting is that the machine we're running on does not have a bad actor running a service as a libsecret provider.

At that point, though, we're just taking issue with the way an OS's secure storage works -- which is fine and we've done before -- but the alternative in this case is to store the access tokens and refresh tokens in plaintext to a well-known location on disk. Without handling some form of PAM listening service ourselves I don't currently know a better way to do it. Even then, validating the receiving end of IPC is roughly impossible without a trusted intermediary hosting/validating both services.

This method allows us to not require an unlocked keyring to store data. However, the keyring will need to be unlocked prior to storage. This is typically done on login to the user's account.

Hinton

LGTM, not sure if we need to be so verbose about the naming, and the rust module doesn't currently convery a secure storage concept. Imho from the module scope perspective it would be better to convey that the passwords module is supported or not.

Hinton · 2024-04-22T15:26:45Z

apps/desktop/desktop_native/index.d.ts

@@ -12,6 +12,7 @@ export namespace passwords {
  export function setPassword(service: string, account: string, password: string): Promise<void>
  /** Delete the stored password from the keychain. */
  export function deletePassword(service: string, account: string): Promise<void>
+  export function osSupportsSecuredStorage(): boolean


We could probably shorten this and just call it osSupport or enabled.

went with osSupport. Everything is namespaced to passwords, so I quite liked that readability

github-actions bot added the needs-qa Marks a PR as requiring QA approval label Apr 16, 2024

MGibson1 force-pushed the ps/pm-7486/detect-libsecret-service branch from 138ae7f to d3345c6 Compare April 17, 2024 12:25

MGibson1 force-pushed the ps/pm-7486/detect-libsecret-service branch 2 times, most recently from ef0686d to 01a73bd Compare April 17, 2024 14:28

MGibson1 force-pushed the ps/pm-7486/detect-libsecret-service branch 3 times, most recently from ba68a4f to 0571887 Compare April 18, 2024 00:26

jlf0dev mentioned this pull request Apr 18, 2024

Error when syncing the vault on Linux desktop #8651

Open

1 task

bwdil reviewed Apr 18, 2024

View reviewed changes

MGibson1 force-pushed the ps/pm-7486/detect-libsecret-service branch 2 times, most recently from 407d298 to 987f8da Compare April 22, 2024 12:29

MGibson1 and others added 5 commits April 22, 2024 08:30

Add libsecret service detection to native module

e09c528

Make secure storage support sync

6e7fbc4

Store auth tokens based on accessibility of secure storage

d7c3016

Query dbus for secret service rather than using libsecret directly

c241d0f

This method allows us to not require an unlocked keyring to store data. However, the keyring will need to be unlocked prior to storage. This is typically done on login to the user's account.

Switch to zbus to avoid glibc

ff3e839

MGibson1 force-pushed the ps/pm-7486/detect-libsecret-service branch from 987f8da to ff3e839 Compare April 22, 2024 12:31

npm run prettier 🤖

026b211

MGibson1 marked this pull request as ready for review April 22, 2024 12:48

MGibson1 requested a review from a team as a code owner April 22, 2024 12:48

MGibson1 requested review from coroiu and Hinton April 22, 2024 12:48

MGibson1 changed the title ~~Ps/pm-7486/detect-libsecret-service~~ [PM-7486] Detect Libsecret Service Apr 22, 2024

Hinton reviewed Apr 22, 2024

View reviewed changes

ghisvail mentioned this pull request Apr 22, 2024

Errors when syncing, needs secret service access flathub/com.bitwarden.desktop#192

Open

Shorten name

fd3027c

MGibson1 requested a review from Hinton April 22, 2024 16:31

Hinton approved these changes Apr 22, 2024

View reviewed changes

Merge branch 'main' into ps/pm-7486/detect-libsecret-service

a0bffc2

djjuhasz mentioned this pull request Apr 25, 2024

Infinite loading after logging in on ubuntu desktop #8915

Closed

1 task

coroiu removed their request for review April 29, 2024 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-7486] Detect Libsecret Service #8776

[PM-7486] Detect Libsecret Service #8776

MGibson1 commented Apr 16, 2024 •

edited

codecov bot commented Apr 16, 2024 •

edited

github-actions bot commented Apr 16, 2024 •

edited

MGibson1 commented Apr 17, 2024

bwdil commented Apr 17, 2024

MGibson1 commented Apr 17, 2024

MGibson1 commented Apr 17, 2024

bwdil Apr 18, 2024

bwdil Apr 18, 2024 •

edited

MGibson1 Apr 18, 2024

Hinton left a comment

Hinton Apr 22, 2024

MGibson1 Apr 22, 2024 •

edited

[PM-7486] Detect Libsecret Service #8776

Are you sure you want to change the base?

[PM-7486] Detect Libsecret Service #8776

Conversation

MGibson1 commented Apr 16, 2024 • edited

Type of change

Objective

Draft reason

Code changes

Before you submit

codecov bot commented Apr 16, 2024 • edited

Codecov Report

github-actions bot commented Apr 16, 2024 • edited

No New Or Fixed Issues Found

MGibson1 commented Apr 17, 2024

bwdil commented Apr 17, 2024

MGibson1 commented Apr 17, 2024

MGibson1 commented Apr 17, 2024

bwdil Apr 18, 2024

Choose a reason for hiding this comment

bwdil Apr 18, 2024 • edited

Choose a reason for hiding this comment

MGibson1 Apr 18, 2024

Choose a reason for hiding this comment

Hinton left a comment

Choose a reason for hiding this comment

Hinton Apr 22, 2024

Choose a reason for hiding this comment

MGibson1 Apr 22, 2024 • edited

Choose a reason for hiding this comment

MGibson1 commented Apr 16, 2024 •

edited

codecov bot commented Apr 16, 2024 •

edited

github-actions bot commented Apr 16, 2024 •

edited

bwdil Apr 18, 2024 •

edited

MGibson1 Apr 22, 2024 •

edited