Update dashlane-vault extension by neo773 · Pull Request #11326 · raycast/extensions

neo773 · 2024-03-16T18:06:58Z

Description

This PR adds support for authenticating the vault via Touch ID and not leaving it exposed.
Do not merge this before this PR

Screencast

My.Movie.1.mp4

Checklist

I read the extension guidelines
I read the documentation about publishing
I ran npm run build and tested this distribution build in Raycast
I checked that files in the assets folder are used by the extension itself
I checked that assets used by the README are placed outside of the metadata folder

- feat: touchid authentication - Initial commit

raycastbot · 2024-03-16T18:07:23Z

Thank you for your contribution! 🎉

🔔 @tmwrnr you might want to have a look.

Mikescops · 2024-03-18T08:50:28Z

(Dashlane developer here): I'm not a huge fan of Raycast managing secrets like the master password, I would rather implement this in the Dashlane CLI.

neo773 · 2024-03-18T08:56:59Z

@Mikescops
Hi, the binary’s from sources/auth/main.swift

can be recompiled by running npm run build-swift

Mikescops · 2024-03-18T08:58:13Z

@Mikescops Hi, the binary’s from sources/auth/main.swift

can be recompiled by running nom run build-swift

Thanks I figured out after a second read. It's an interesting approach, I'm wondering if I can build this in the CLI directly.

Mikescops · 2024-03-19T09:38:56Z

extensions/dashlane-vault/Sources/auth/main.swift

+import Foundation
+import LocalAuthentication
+
+let policy = LAPolicy.deviceOwnerAuthenticationWithBiometrics


This policy here is insufficient, it can be bypassed, you should rather use kSecAttrAccessControl.

I recommend you to have a read to this post: https://www.andyibanez.com/posts/ios-keychain-touch-id-face-id/

I looked into this but couldn't get it working as kSecAttrAccessControl requires "Keychain Access Groups" entitlement which is not supported by CLI Apps and also requires code signing. Workaround seems to be using a separate daemon for this.

Given the scope it would make more for Dashlane to integrate this natively in a secure manner, I expect this to take quite some time assuming your team approves this feature.

However in the meantime we could still merge this since something is better than simply leaving the vault open at all times which is a huge security hazard.

cc
@tmwrnr

tmwrnr

Hi @neo773,

thanks for opening this pull request and contribute to this extension. I like the idea that you don't need to save the master password by the Dashlane CLI! When I created the extension I was unsure how to achieve this.

I don't know enough about Swift to really review this part of the code. Maybe someone from Raycast can do this?

I could not really test this new feature because it did not work on my machine as mentioned in one of my comments. Regardless of the implementation, I do have a general question about your approach. As of my understanding Raycast is not allowing extensions accessing the keychain as mentioned here. Maybe someone from Raycast can confirm this?

Furthermore I, as an user, don't like the idea to insert my password to a Raycast extension even if I know, that the extension itself handles the keychain access. It just does not feel right for me. If a user has concerns with using the Dashlane CLI with the master password saved, maybe the CLI can implement the biometrics itself? I assume Dashlane is also just saving the password within keychain @Mikescops?

Maybe we could prompt the password every time the user want to access the vault?

Again, thank you very much for your time implementing this and I'm curious to see where this will lead to.

extensions/dashlane-vault/package.json

extensions/dashlane-vault/src/lib/dcli.ts

tmwrnr · 2024-03-23T06:06:34Z

extensions/dashlane-vault/src/lib/dcli.ts

+  const masterPassword = preferences.useTouchID ? await getMasterPassword() : null;

-  throw Error("Dashlane CLI is not found!");
+  const { stdout, stderr } = await execa(CLI_PATH, args, {


This is not working on my machine. The promise never resolves. I updated the Dashlane CLI and set my master password. The Search Passwords command just shows me the loading state.

Apparently my upstream commit for dashlane cli did not work, will look into this.

extensions/dashlane-vault/src/lib/authcli.ts

tmwrnr · 2024-03-23T06:11:49Z

extensions/dashlane-vault/src/lib/authcli.ts

+
+const executeCommand = async (args: string[]) => {
+  const command = join(environment.assetsPath, "auth");
+  await chmod(command, "755");


Do we need to change this before every run or could this also be done once?

tmwrnr · 2024-03-23T06:17:24Z

extensions/dashlane-vault/src/master-password.tsx

+  const [keyName, setKeyName] = useState("");
+  const [passwordExists, setPasswordExists] = useState(false);
+
+  const fetchKeyNameAndCheckPassword = async () => {


In this command the user wants to set his master password. Why do we need to fetch and insert the current password?

It's mainly for UI purposes.

tmwrnr · 2024-03-23T06:19:43Z

extensions/dashlane-vault/src/master-password.tsx

+      setPasswordExists(true);
+      setPasswordInput(existingPassword);
+    } else {
+      const keyName = `dashlane-vault-master-password-${Math.random().toString(36).substring(2, 15)}`;


Is there the need of the random key name? I thought that the keychain has a "space" for each application. If so we just could use a static keyName.

extensions/dashlane-vault/src/master-password.tsx

mil3na · 2024-03-27T12:41:13Z

Is this ready for a final review or does @tmwrnr and @Mikescops have any more comments?

tmwrnr · 2024-03-27T13:39:04Z

Hi @mil3na,

there are still general questions that are not answered, mentioned in my comment.

E.g.:

I don't know enough about Swift to really review this part of the code. Maybe someone from Raycast can do this?

Could you or someone from Raycast do that?

As of my understanding Raycast is not allowing extensions accessing the keychain as mentioned here. Maybe someone from Raycast can confirm this?

What is your opinion about that?

Furthermore I, as an user, don't like the idea to insert my password to a Raycast extension even if I know, that the extension itself handles the keychain access. It just does not feel right for me. If a user has concerns with using the Dashlane CLI with the master password saved, maybe the CLI can implement the biometrics itself? I assume Dashlane is also just saving the password within keychain @Mikescops?

What is the typical way of combining the ideas of an author and the contributors?

Furthermore the dashlane CLI is not ready as mentioned here. So in my opinion this pull request is on hold even if everything else would be ready.

neo773 · 2024-03-27T21:26:56Z

@tmwrnr

Furthermore the dashlane CLI is not ready as mentioned #11326 (comment). So in my opinion this pull request is on hold even if everything else would be ready.

This is fixed in the latest commit, I've tested it locally and it's working great Dashlane/dashlane-cli#229

Mikescops · 2024-03-27T22:15:48Z

Is this ready for a final review or does @tmwrnr and @Mikescops have any more comments?

For me this PR is a no go, the swift does not meet proper code quality standards and the security concerns raised in my first comments are not solved.

neo773 · 2024-03-27T22:45:57Z

Is this ready for a final review or does @tmwrnr and @Mikescops have any more comments?

For me this PR is a no go, the swift does not meet proper code quality standards and the security concerns raised in my first comments are not solved.

Partly agree with this statement, you're basing it off from the blog post you mentioned and specifically this part
"This is actually a very naïve approach because in theory, a jailbreak tweak could hook into your app"
macs don't have jailbreak and stuff like this is protected by System Integrity Protection.

Current implementation literally leaves the vault open, any program can call dcli password output --json and send the vault dump to their malicious server.

Mikescops · 2024-03-27T22:59:03Z

I understand the initial concern, and as I mentioned in my very first comment I'd rather have this implemented in the CLI itself rather than in a third party which won't solve what you previously mentioned about accessing the vault with a command line.
Also by making a wrapper on how we store the master password you're excluding users who have 2FA or SSO on their account which is handled by the CLI natively.
I'm going to raise this need internally and see what we can do in the short term.
I'm not a raycast owner so this is just my two cents, you folks are deciding on if this is good to be merged.

neo773 · 2024-03-27T23:41:46Z

Yes, I said the same thing before long term Dashlane should do this natively but short term this isn't so bad.

tmwrnr · 2024-03-28T07:00:13Z

I understand the initial concern, and as I mentioned in my very first comment I'd rather have this implemented in the CLI itself rather than in a third party which won't solve what you previously mentioned about accessing the vault with a command line.

@Mikescops It would be nice, if the CLI could handling this kind of stuff. In Raycast there are also extensions for Bitwarden and 1Password.

Bitwarden works with a client ID and a client secret to access the vault, without knowing exactly the master password.

1Password is also using I think a secret which is I think just token.

Yes, I said the same thing before long term Dashlane should do this natively but short term this isn't so bad.

@neo773 I don't want to include a refactoring and a new command for short term. Yes the current solution is not the best, but with the limitation of the CLI I think it's acceptable. If these constrains does not match your security standards, which is fine, than maybe wait to use the extension until the CLI is updated.

Until a newer version of the CLI is released I could think of a prompt for the password every time you want to access the vault. The master password would only be saved in memory during runtime of the specific command. Would that be compromise?

Mikescops · 2024-03-28T09:21:45Z

Bitwarden works with a client ID and a client secret to access the vault, without knowing exactly the master password.

I've checked their code, the cID/secret grants access to the CLI. The CLI still handles the encryption of the vault. The vault can be open, similar to what we do, but can also be locked and then the Master Password will be requested and they will prompt this in Raycast.

We have already started discussions internally on what we can do here, I'll keep you posted.

Mikescops · 2024-03-29T15:40:08Z

@neo773 I added a couple of settings, more to be added in the next weeks:

Mikescops · 2024-04-03T15:50:19Z

I was not convinced by the current nodejs implementations so I built my own native module for the macos keychain and biometrics.
It's still quite early stage for now so it's likely missing some features but I think it's good to be integrated in projects.
https://github.com/Mikescops/node-native-keychain

Mikescops · 2024-04-08T10:50:42Z

@neo773 You can start using https://github.com/Dashlane/dashlane-cli/releases/tag/v6.2415.0 and update this PR 🎉

tmwrnr · 2024-04-23T12:09:04Z

Hi @neo773,

the Dashlane CLI was updated again, because of an error where the complete output could not be read by nodejs.

I opened a new pull request #11950 where I added the master password as an extension preference. With this you can already use the following configuration. I also refactored some code and added improved error handling.

dcli c save-master-password false

The CLI now supports biometrics so we don't have to implement this in the extension. If you still want to use biometrics to unlock the vault and want to contribute to this extension, I would suggest you to create a new branch after my pull request is merged.

Thank you again for the pull requests, as this allowed the extension and CLI to be improved! I think we should close this pull request, what do you think?

neo773 · 2024-04-23T16:31:08Z

@tmwrnr
Agreed, closing this!

Update dashlane-vault extension

98586b7

- feat: touchid authentication - Initial commit

raycastbot added extension fix / improvement Label for PRs with extension's fix improvements extension: dashlane-vault Issues related to the dashlane-vault extension labels Mar 16, 2024

neo773 added 2 commits March 17, 2024 01:19

fix: lint

5c0c047

fix: add changelog

ef533f2

Mikescops reviewed Mar 19, 2024

View reviewed changes

pernielsentikaer assigned pernielsentikaer and mil3na Mar 19, 2024

tmwrnr suggested changes Mar 23, 2024

View reviewed changes

neo773 added 2 commits March 24, 2024 08:20

add changes

011ee31

works now

5c2007e

tmwrnr mentioned this pull request Apr 9, 2024

Password JSON output is cut off when using CLI with Nodejs Dashlane/dashlane-cli#235

Closed

pernielsentikaer unassigned mil3na Apr 12, 2024

tmwrnr mentioned this pull request Apr 23, 2024

[Dashlane Vault] Master password preferences and improved error handling #11950

Merged

5 tasks

neo773 closed this Apr 23, 2024

Conversation

neo773 commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screencast

Checklist

Uh oh!

raycastbot commented Mar 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mikescops commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

neo773 commented Mar 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mikescops commented Mar 18, 2024

Uh oh!

Mikescops Mar 19, 2024

Choose a reason for hiding this comment

Uh oh!

neo773 Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmwrnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tmwrnr Mar 23, 2024

Choose a reason for hiding this comment

Uh oh!

neo773 Mar 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmwrnr Mar 23, 2024

Choose a reason for hiding this comment

Uh oh!

tmwrnr Mar 23, 2024

Choose a reason for hiding this comment

Uh oh!

neo773 Mar 24, 2024

Choose a reason for hiding this comment

Uh oh!

tmwrnr Mar 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mil3na commented Mar 27, 2024

Uh oh!

tmwrnr commented Mar 27, 2024

Uh oh!

neo773 commented Mar 27, 2024

Uh oh!

Mikescops commented Mar 27, 2024

Uh oh!

neo773 commented Mar 27, 2024

Uh oh!

Mikescops commented Mar 27, 2024

Uh oh!

neo773 commented Mar 27, 2024

Uh oh!

tmwrnr commented Mar 28, 2024

Uh oh!

Mikescops commented Mar 28, 2024

Uh oh!

Mikescops commented Mar 29, 2024

Uh oh!

Mikescops commented Apr 3, 2024

Uh oh!

Mikescops commented Apr 8, 2024

Uh oh!

tmwrnr commented Apr 23, 2024

Uh oh!

neo773 commented Apr 23, 2024

Uh oh!

Reviewers

Assignees

neo773 commented Mar 16, 2024 •

edited

Loading

raycastbot commented Mar 16, 2024 •

edited

Loading

Mikescops commented Mar 18, 2024 •

edited

Loading

neo773 commented Mar 18, 2024 •

edited

Loading

neo773 Mar 24, 2024 •

edited

Loading