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

allow options to be passed to the script #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Konfekt
Copy link

@Konfekt Konfekt commented Apr 7, 2020

This way, options can be set by, say NOTIFY=0 pinentry-wsl-ps1.sh; thus the script can be updated without losing once set options.

This way, options can be set by, say `NOTIFY=0 pinentry-wsl-ps1.sh`; thus the script can be updated without losing once set options.
@diablodale
Copy link
Owner

Nice idea. Thanks for the contribution.
For the implementation, perhaps using ${-} instead of ${:-}. The behavior changes when the incoming value is "" or null.

with ${-}

NOTIFY="" pinentry-wsl-ps1.sh
inside script NOTIFY=""

with ${:-}

NOTIFY="" pinentry-wsl-ps1.sh
inside script NOTIFY=1

And have you tested step 2 of setup using the VAR=VALUE pinentry-wsl-ps1.sh approach? A quick read of the gpg code makes me suspect this will fail. The pinentry-program expects a program path, not a script command. What did you see when testing?

An alternative might be to use the env var PINENTRY_USER_DATA
described https://gnupg.org/documentation/manuals/gnupg.pdf
and code https://github.com/gpg/gnupg/blob/fd79cadf7ba5ce45dfb5e266975f58bf5c7ce145/agent/call-pinentry.c#L425
The PINENTRY_USER_DATA isn't automatically parsed by gpg code, so pinentry-wsl-ps1.sh would need to manually parse it when it is received via the Assaun option OPTION pinentry-user-data for any settings contained within. For example https://kevinlocke.name/bits/2019/07/31/prefer-terminal-for-gpg-pinentry/

@Konfekt
Copy link
Author

Konfekt commented Apr 8, 2020

For the implementation, perhaps using ${-} instead of ${:-}

Well observed. The last commit adapts the code accordingly.

The pinentry-program expects a program path, not a script command. What did you see when testing?

I have no WSL system at hand, but this is rather a question of documenting the feature. I suppose then that env NOTIFY="" pinentry-wsl-ps1.sh instead of NOTIFIY="" pinentry-wsl-ps1.sh will go through fine?

@diablodale
Copy link
Owner

I do not think env will work either. Same problem as above, the gpg agent tries to run a program named env NOTIFY=""... with spaces in it which would naturally fail.

https://github.com/gpg/gnupg/blob/fd79cadf7ba5ce45dfb5e266975f58bf5c7ce145/agent/call-pinentry.c#L352

From what I can understand (unless you see otherwise in your testing), it is not possible to set environment variables on the "same line" as this pinentry program in the real-world. This is due to the gpg agent interface between that agent and a pinentry program. It might be possible to set NOTIFY in a global scope in which gpg-agent is launched and hope it is inherited by the pinentry program. Though a global env var named NOTIFY is somewhat concerning.

Testing is needed for your proposed code change. Trying your idea in a real-world WSL, Windows, GPG setup to see if it is successful.

I like your idea. But I predict problems that testing can surface or prove me wrong 😅

To move forward, you'll need to do the testing I've written about above to explore what works and not. Maybe its your envvar technique, maybe its PINENTRY_USER_DATA, or maybe something else.
When you find a method that does work, then I'm happy to consider it 🙂

@Konfekt
Copy link
Author

Konfekt commented Apr 8, 2020

Okay, testing is called for this setup. In the meanwhile, alternative setups, such as wrapping pinentry-wsl-ps1.sh into a script that sets these variables, or exporting them before calling it (and unsetting them afterwards?!) could be still of use and be kept in mind.

@Konfekt
Copy link
Author

Konfekt commented Sep 24, 2021

Okay, indeed either

PERSISTENCE=${PERSISTENCE-""}
NOTIFY=${NOTIFY-"1"}
DEBUGLOG=${DEBUGLOG-""}

be set globally, or better pinentry-wsl-ps1 parse $PINENTRY_USER_DATA.

Because the same config can be used on different machines, it could also fall back to other pinentry programs adding a case "wls".

@diablodale
Copy link
Owner

I don't see any gpg2 standard PINENTRY_USER_DATA or PINENTRY_BINARY in gpg2 documentation that is focused on this narrow feature area. Where are you seeing these features/standards?

I only found https://www.gnupg.org/documentation/manuals/gnupg/GPG-Configuration.html#index-PINENTRY_005fUSER_005fDATA which passes data from agent->pinentry, and the related Assuan option https://www.gnupg.org/documentation/manuals/gnupg/GPGSM-OPTION.html. Both of them do not seem (to me) to be designed for a user to set options. And not designed for concatenating/appending options together...instead options are overwritten and therefore a user's options are easily overwritten by the agent->pinentry step.

@Konfekt
Copy link
Author

Konfekt commented Sep 27, 2021

There is no PINENTRY_BINARY in gpg2 documentation. From what I gathered, it is implemented in Opensuse.

PINENTRY_USER_DATA is documented on page 89 of the GPG documentation, Version 2.2.29 from June 2021. Not sure if this makes it a gpg2 standard. Citing

PINENTRY_USER_DATA ... is useful to convey extra information to a custom pinentry.

One use case could be a custom pinentry that opts for pinentry-wsl-ps1 on a WSL system.

@diablodale
Copy link
Owner

Thanks. Here are my thoughts....

  1. Now I understand that PINENTRY_USER_DATA is an env ​variable that is read by gpg-agent. Then gpg-agent sends the value to the pinentry program via something.
  2. It is unclear to me the via something. How it is sent? Env var? Assuan? Pinentry command? This is probably easy to determine by some trial/error.
  3. It is important that the value of PINENTRY_USER_DATA not be a source of script injection problems. It should have no expansion, no replacement, no interpretation, etc.

I don't have the bandwidth to code this. Do you want to submit a PR? I will gladly review a PR and do some testing on it.

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

Successfully merging this pull request may close these issues.

2 participants