darrenjrobinson/1Pwd

Module can't use op from system path

Closed this issue · 8 comments

Even the instructions say Install it in your system path, it won't be able to locate and use it:

image

Am I missing something here?

P.S. Also expression .\\op.exe doesn't make sense - it assumes op is available in current directory, while it is not:

1Pwd/1Pwd.psm1

Line 212 in 5d82c68

$Global:sessionToken = echo $1PMasterPasswordDecrypted | .\\op.exe signin $SignInAddress $SignInAccount $1PSecretKeyDecrypted -r

1Pwd/1Pwd.psm1

Line 272 in 5d82c68

$Global:sessionToken = echo $1PMasterPasswordDecrypted | .\\op.exe signin $SignInAddress $SignInAccount $1PSecretKeyDecrypted -r

Actually, I found a much bigger issue with Invoke-1PasswordExpression - it doesn't work after Powershell restarted. Reason is simple: line

1Pwd/1Pwd.psm1

Line 272 in 5d82c68

$Global:sessionToken = echo $1PMasterPasswordDecrypted | .\\op.exe signin $SignInAddress $SignInAccount $1PSecretKeyDecrypted -r

expects $1PMasterPasswordDecrypted and $1PSecretKeyDecrypted to be set, but they only set once in Test-1PasswordCredentials. I am confused how it supposed to work?!

Try putting OP.exe in the same directory as your script or the directory in which your PS shell is in. It will then use it from the local directory.

For the config after setting it, you need to save it using Set-1PasswordConfiguration. The config is then read in each time the module loads.

Try putting OP.exe in the same directory as your script or the directory in which your PS shell is in. It will then use it from the local directory.

This doesn't make sense, honestly. I am not managing op myself - I got it installed using scoop, it is on the system path and this is sufficient to use it. I would completely drop anything op related in the scripts and just assume/require op to be in path.

For the config after setting it, you need to save it using Set-1PasswordConfiguration. The config is then read in each time the module loads.

Yes, I see it is done when module is loaded, however master password and secret key are loaded in encrypted format, but to retrieve sessionToken they need to be decrypted first and this never happens in the Invoke-1PasswordExpression function.
Looks like there is re-use of $Test-1PasswordCredentials method inside, sorry. However somehow code didn't work for me out of the box...

After spending more time with the code I definitely like the idea of this utility, but implementation doesn't looks logical to me. Here is what I think can be done better:

  • simplify setup: right now to set it up, I need to define 4 variables, run test command with them, assigning 5th and then run set command with all 5 variables. I would just do this in wizard style, so user just invoke some set method and get asked for required parameters, which are validated and saved. Simple. This can be enhanced to support multiple accounts.
  • 1password CLI already supports multiple accounts (you mistakenly refer them as vaults, vaults in 1Password is different thing) - they are setting $env:OP_SESSION_<account> env variables, so op will re-use issues session key automatically. I would just allow add another parameter for Invoke-1PasswordExpression function to specify desired account when running a command - will be much easier than explicitly switching them using separate command.

What do you think about these items? I have some time and desire re-work this project. I can fork it, however if you are fine with suggested changes, let me know and I can make a PR to this project instead.

One of my original goals was to ensure that the module worked in both Windows PowerShell and PowerShell.
It would be good to validate it on other platforms (linux, mac etc).
I have always used it with op.exe in the current path but it could be improved to get the path of op and call it will full path. And do that on other platforms too.
Yes the setup is a little involved, but it one off (per host).

I would happily accept enhancements and PRs.

I have always used it with op.exe in the current path but it could be improved to get the path of op and call it will full path. And do that on other platforms too.

There is no need to call it with full path if it is already in path (and otherwise you won't be able to locate it anyway). I would suggest to allow to supply the location of the op binary as a parameter and default it to op (without .exe), which will make it work cross-platform when op is in PATH.

Another question I have here: do we really need to store the secret key? It is needed only once during 1st time setup for the account. And while we can store it, it is not really needed for subsequent signin operations (master password is sufficient). If to re-work the Set-1PasswordConfiguration to be interactive, all we need to store is shorthand (account) and master password (and still query everything it does now, but perform initial login right during setup, so no address/email/secret key are needed after the setup). This will reduce number of items stored in the config a lot.

For 'op' location I think having the flexibility of it being in the path and the local (script) directory are valid, especially on systems where you can modify the path or have privileges to add binaries to an existing listed path.

Definitely like the idea of minimising what is stored in the configuration. Even though the config is protected to the profile of the user that created it, having less in it is highly preferred.

Ouch, there was 1Password CLI v2 release recently and it breaks everything... I am currently evaluating new version, but having biometric unlock feature solves lots of issues I wanted 1Pwd for... Playing with new version now.