Updated 1Password Dynamic Folder with support for biometrics #71
Conversation
|
This has been tested & verified on macOS Ventura with Royal TSX 6.0.0.14 and 1Password for Mac 8.10.9 (81009046). It has not been tested on Windows, so if any Windows-users would test this, that would be excellent. |
|
@lemonmojo, you said the version in #69 did not work for you. Could you please try this version? (edit: also note the Windows All your other issues have been addressed, more or less. There is no check if any of the values are wrong (like the OS Path), but at least they have sane default values (OS Path for Windows is based on the specific example path used by 1Password documentation, and for macOS it's the default path when installing via |
|
I removed the old 1Password script as part of the PR, as it's so unsafe that it should not exist in it's current form IMHO. Also updated the notes to reflect that the Royal TS 7+ or Royal TSX 6+ betas are required (due to the new .rdfx format). |
|
@lemonmojo, also, the |
|
@lemonmojo By the excellent work of all parties involved this PR fixes issues #46, #60 and #92 and improves upon PR #69. Can this be merged? And if not, what is the blocker? I have some time to contribute if need be. |
|
@SixFive7 I can take another look at this over the next few days.
Thx, |
|
@SixFive7 @joachimtingvold Please also remove the updated readme file from the PR. I just updated it to reflect your changes but it really is off-topic for this PR. |
joachimtingvold
force-pushed
the
1pass-dynamic-folder-rdfx
branch
from
c3d5c41
to
71a3232
Compare
|
Changes to readme reverted & separate script folder removed. |
It supports multiple accounts accross multiple dynamic folders (it does not, however, support multiple accounts in the same dynamic folder). I use private and work account in separate dynamic folders without any issues. Reloading of credentials are separate for each dynamic folder. As long as you have set the appropriate account ID in each dynamic folder, that should work just fine.
I assume you mean "op" here (not "bw", which is BitWarden). As for the "op" CLI tool for 1Password, the only requirement for things to work is that the CLI-integration is enabled in 1Password (which is part of the "Getting started with 1Password CLI" documentation that is linked in the "Notes"-section). There should be no need to do any additional logins or other commands. The only "caveat" is that the last logged in account will be the one used unless an account ID is specified in the configuration (which is also mentioned in the "Notes"-section). The latter is only relevant if you are signed into multiple accounts.
Handled seamlessly and automatically by the 1Password app (as long as the CLI tool has been installed according to the instructions). |
|
@joachimtingvold Sounds promising! I wasn't aware that the 1Password app was required for this to work. It wasn't required in the previous version of the script but I guess it's ok to have that as a dependency. Maybe it makes sense to clarify in the notes that the 1Password 8 "app" is required for this to work. But the most important omission is that nowhere in the notes it's mentioned that "Integrate with 1Password CLI" must be enabled in the 1Password app's "Developer" settings. Without this enabled, a reload of the dynamic folder always yields One thing that should be improved is error handling when one is not signed in to any account in the 1Password app. Currently, when that is the case and I reload the dynamic folder I get the following error message which is so large that it doesn't even fit on my screen: The same thing happens when cancelling the "1Password Access Requested" prompt. I guess one option to handle this is to just filter out any lines that have Another way to reproduce the "too long error messages" problem is to specify an account or vault that doesn't exist. Another issue with this new approach is that when I quit the 1Password app, reloading or resolving dynamic credentials results in the following error: I guess we cannot do anything about this situation since the new script fully relies on the 1Password app being installed and running. But, in that case I suggest at least making sure that any error messages are readable by the user. I did my tests only on a Mac but all of this (mainly error handling) should be tested on Windows as well. I'll play with the new version of the script some more and let you know if I find anything else. |
|
Regarding the "MISSING POOLS" problem: You can work around this by setting the environment variable Here's an example on how to do this in the |
The notes already state that it "Requires 1Password version 8 or above.".
The notes mentions that "1Password CLI tool" is required, and links to this url, where the second step of the official CLI tool installation guide is to enable that setting.
While I agree that error handling could probably be improved, the main goal of the rewrite was to address the huge security issues of the old implementation (and secondly to add biometrics).
Yes, the nature of the MFA/Biometrics introduces that dependency. The security benefits far outweigh that constraint IMHO. |
Thanks. Added to PR. |
|
@joachimtingvold I'm aware that the notes already say 1Password version 8. I just suggest adding the word "app" so that it's more clear. The same applies to the setting. It's good that it's in 1Password's documentation but it doesn't hurt to also mention it in our documentation since it's so vital for the script to work. |
|
Should be pretty explicitly specified in the notes now (-: |
|
@st9rm1337 Please take a look at this on the Windows side of things. Thx! |
|
hi @joachimtingvold |
|
I'm traveling right now, so I don't have any Windows machine readily available at the moment. However, it seems like Windows Hello is required to enable the CLI integration (as per the requirements at the top of the CLI tool install guide);
It links to this page for instructions of how to enable that within 1Password. This is also further confirmed by this forum post. |
|
@joachimtingvold So does that mean that only computers compatible with Windows Hello would be able to use the new version of the 1Password Dynamic Folder? If that's the case I don't think we can get rid of the current version as it would probably prevent a lot of users from being able to integrate 1Password into Royal TS. @StefanKoell What's your take on this? |
|
@lemonmojo No it doesn't mean you need a full Windows Hello compatible computer. I understand the confusion given the wording of the ambiguous 1Password documentation. But what they mean to say is you need a non-ancient Windows version. See, they leverage the Windows Hello security subsystem to offload some security and authentication to Windows its secure desktop (secure popups an application cannot interact with). But there is no requirements for the additional login functionality provided by Windows Hello like signing in with biometrics or IR camera's. |
|
Interesting! Thx for the insights @SixFive7! Now we still have to figure out why the setting is greyed out for @st9rm1337. |
|
The greyed out option should be due to not enabling Windows Hello, as already explained above? Unless @st9rm1337 already tried that to no avail? |
|
@st9rm1337 Is on vacation this week. We'll continue looking into this when he returns. |
|
Just tried this version. Previous version (2.0.5) works with SSH keys. This version does not appear to work with SSH keys. Also, both versions still (new and old) still generate the following error when trying to load a "Connection with Options" SFTP:
|
|
I was able to modify this script to work with SSH keys. Not sure if you want me to make changes to this file and commit for review or pass it up, but it a simple add in on that. Still having the string cannot have zero length issue with Parameter name: userName. Not sure what's driving that error. |
|
Update, the "Parameter userName" issue appears to be a bug in Royal TSX. If I turn on "Prompt for Credentials" on the Connect Using menu, and select the credential, it works as normal. It only fails if that option isn't selected. So likely not a issue with the dynamic folders but how Royal TSX handles that option. |
|
@tezgno There's currently no specific handling for private key files in this version of the 1Password Script. The issue here is likely caused by the fact that your private key item in 1Password has either no username specified or that the username field is not named in a way that we expect. |
Hi, just tested the script whilst also activating Windows Hello within the Security settings of the 1Password application. After doing so, I was also able to activate the 1PW CLI Integration, which also let me reload the script. |
I fixed it myself by updating the script here. Unlike the existing 1password script, this version doesn’t pass the private key at all (those lines of code are completely missing). Once I added in to pass the private key, it worked just as the previous version of the script. I can share the copy I made. |
|
I see what you're talking about. Not sure why I initially removed those lines of code (it was done at the very beginning when I was slimming the old code, and I probably confused it with something else). I've re-added them now, should probably work again now? |
|
Yep. Working again. |
|
This has been merged now. Thx for everyone involved! |
Based on my original gist, improved by @jvwam by adding support for filtering multiple vaults.
Further refined by me;
This solves issue #46 and #60 and supersedes PR #69 (sorry for "hijacking" your PR, @jvwam (= ).