Skip to content
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

Unsafe 1password integration #60

Closed
SixFive7 opened this issue Feb 1, 2023 · 15 comments
Closed

Unsafe 1password integration #60

SixFive7 opened this issue Feb 1, 2023 · 15 comments

Comments

@SixFive7
Copy link

SixFive7 commented Feb 1, 2023

The current version of the 1password dynamic folder script requires the 1password windows hello and biometric unlock functions to be disabled. This is considered unsafe.
Even more important, the script requires one to save the secret key and master password in the Royal TS script configuration fields. This is considered very insecure and (correctly so) actively advised against by 1Password itself.

With the new version of the 1Password CLI tool these credentials are also not required. If the 1Password desktop app and CLI are installed one can just run op item list --vault Private and let 1Password handle all the (secure desktop) prompts for logging in and authorizing the request. Hence the need to store master password credentials inside Royal TS is no longer there.

We should not ask for or store credentials and use the CLI securely and as intended.

@SixFive7
Copy link
Author

SixFive7 commented Feb 1, 2023

Related to #46?

@StefanKoell
Copy link
Member

I agree, if these options are now available (and reliably working) we should definitely utilize the newest version of the 1Password CLI (also assuming that there are no other downsides/showstoppers)

@SixFive7 would you be able to create a PR and/or test for that?

@joachimtingvold
Copy link
Contributor

joachimtingvold commented Feb 19, 2023

I modified the existing code, removing historic stuff (v1 vs v2 CLI, login/logout, etc). Code reduced to about 25% of the original. Seems to work for me on my Mac. Have not done extensive testing yet, and untested on Windows. Biometric 1Password popup works as expected.

The only limitation (for now) is that it doesn't support multiple 1Password-accounts in the same dynamic folder. Also, the Account custom property is a required field even if you only have 1 account, and was done to make sure you are fetching credentials from the account you want (in case you have multiple, like I have).

I have not created a PR yet, as I'm not sure if it should be added as a separate script, or modify the existing one (as this script will only work on 1Password v8 or above).

  1. This assumes 1Password op CLI is setup and working. The script does no check if this is OK or not (could probably improve that part, but.. )
  2. Import the existing 1Password dynamic folder script.
  3. Replace the script code with the following; https://gist.github.com/joachimtingvold/fef17f2772ebd623b1b1ea4135e1e7f5
  4. Remove all of the Login Custom Properties (strictly not needed, but they are not used anymore)
  5. Add the Account custom property. Use the value from the USER ID column from op account list.
  6. Hit "Apply & Close", right click the dynamic folder, and choose "Reload".

The script should probably also be updated to be able to filter on tag(s).

@jvwam
Copy link

jvwam commented Apr 24, 2023

@joachimtingvold I've just tested this on Windows and it's working
The vault custom property is case-sensitive, not an issue if it's copied directly from op vault list

@jvwam
Copy link

jvwam commented Jul 26, 2023

@joachimtingvold I've updated your scripts to allow for multiple vaults
In the Custom Properties vault field, add a comma separated list of vaults, otherwise it works the same
I haven't tested this extensively yet but I haven't encountered any issues
https://gist.github.com/jvwam/bd7ebeec596507cb1149a3561c8d43ff

@sez76
Copy link

sez76 commented Aug 5, 2023

Hello, in my case it doesn't work

Details:
System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'TaskPath')
at ikdmg.pkfek(RoyalDynamicFolder ncrlj) in <9pbU/0LjpCexFwUaxM2copsqI6CCCN0x+6Hd3IhFJU1JsDLkQnLDomz/TIZwbEZbNCp1h9g5Fw5mi8/fIsEDaCN6gAqy3SaQ3IcDNn16PElix61Nvkgk/wNIikbYwvSXv3yauZsgmMrqg3nwCLjmyg==>:line 196

we still need to disable biometric usage for this to work ?

I'm in Windows 11

@unreliability
Copy link

3. https://gist.github.com/joachimtingvold/fef17f2772ebd623b1b1ea4135e1e7f5

Absolute lifesaver, can confirm this works PERFECTLY after months of trying just about anything to get multi-vault working with 1password 8 + Touch ID, cheers man, just made my evening! Should definitely be PR'd into the main branch!

@joachimtingvold
Copy link
Contributor

@joachimtingvold I've updated your scripts to allow for multiple vaults In the Custom Properties vault field, add a comma separated list of vaults, otherwise it works the same I haven't tested this extensively yet but I haven't encountered any issues https://gist.github.com/jvwam/bd7ebeec596507cb1149a3561c8d43ff

Excellent. I updated my gist with your changes, so people get the same result regardless of which gist they find (=

@elbcloudhh
Copy link

Hi guys, this is amazing! Thank you. Would it be possible for the script to get all vaults automatically in a way that we don't have to maintain the vault list?

@joachimtingvold
Copy link
Contributor

joachimtingvold commented Aug 7, 2023

@elbcloudhh, the version in PR #71 solves this (blank/undefined Vaults variable == use all vaults).

@lazynooblet
Copy link

@joachimtingvold thanks for your work. Using your PR I have RoyalTS working with 1Password using a dynamic folder and its really really neat.

I also use 1Password SSH agent, so that linux connections get the ssh key automatically. From a user perspective the SSH agent will approve the app (requiring re-auth) for the request and then subsequent requests for the same key are allowed automatically. So I only need to auth once.

With the dynamic folder, I have to re-auth 1Password every time I connect to a server, is there any way for it to cache the credentials for a period or an option in 1Password to allow a similar experience to the SSH agent?

@joachimtingvold
Copy link
Contributor

I think the problem is that Royal spawns a new iTerm2-process for each window/tab, which from 1Passwords perspective is not the same instance already authenticated. I notice that the 1Password authorization popup explicitly mentions Royal as the app (and not iTerm), so I guess there could be room for improvements in Royal.

@joachimtingvold
Copy link
Contributor

Actually, come to think of it, I do not need to re-auth to 1Password for each connection. I have to do it at launch, and when opening new connections after a while (i.e. it's been idle, or screensaver has been triggered, etc). Other than that I don't need to authorize in 1Password much.

@lazynooblet
Copy link

I'm not sure why I get a different experience. I get the Windows Hello request on every connection.

2023-09-12 09_55_57-app nooblet org (root) - Royal TS

  • I double click the connection
  • Authenticate with Windows Hello
  • Right click, duplicate, or double click a different connection
  • Authenticate with Windows Hello again

joachimtingvold added a commit to joachimtingvold/royalapplications-toolbox that referenced this issue Nov 1, 2023
@lemonmojo
Copy link
Member

Closing this now that the new implementation has been merged. If the issue is still present in the new version, please create a new GH issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants