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

Add --askpin option #130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add --askpin option #130

wants to merge 1 commit into from

Conversation

miska
Copy link

@miska miska commented Sep 18, 2019

Adding --askpin option modeled after --askpass, letting people enter pin early
int he startup or more importantly allow them to keep a password in separate
file to simplify unattended setup.

Signed-off-by: Michal Hrusecky Michal@Hrusecky.net

Thank you for your contribution

You are welcome to open PR, but they are used for discussion only. All
patches must eventually go to the openvpn-devel mailing list for review:

Please send your patch using git-send-email. For example to send your latest commit to the list:

$ git send-email --to=openvpn-devel@lists.sourceforge.net HEAD~1

For details, see these Wiki articles:

Adding --askpin option modeled after --askpass, letting people enter pin early
int he startup or more importantly allow them to keep a password in separate
file to simplify unattended setup.

Signed-off-by: Michal Hrusecky <Michal@Hrusecky.net>
token_pass.nocache = true;

if (!strlen(token_pass.password))
{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pass

@ordex
Copy link
Member

ordex commented Sep 17, 2022

It wasn't clear that this was about PKCS#11. How about renaming the option to --pkcs11-askpin ? I think it'll be much more self explanatory. At the same time I'd extend the doc a little bit to mention what the pin is for. May be obvious for you or me looking at the code, but not for the casual reader.

@miska if still interested, how about addressing these changes and sending the patch to the mailing list, please?

@ordex
Copy link
Member

ordex commented Oct 4, 2022

@miska @finda841 any motivation in bringing this forward? Thanks!

@becm
Copy link
Contributor

becm commented Dec 12, 2022

Since it's a violation of security (to a varying degree, the file/config may be on an encrypted file system), it may be nice to go the full way of

pkcs11-pin mypin.txt

and

<pkcs11-pin>
123456
</pkcs11-pin>

A process who can read the PIN file must have access to the config (and vice versa) anyway.

And it's still (kind of) more secure than askpass. ☺

@R0Wi
Copy link

R0Wi commented Jul 31, 2023

Would really love to see this feature and I would volunteer to bring the development forward. @ordex @becm could you please summarize your preferred way of implementing this? For me a new option --pkcs11-askpin would totally make sense, since there is already a --askpass option which does more or less the same to handle certificate passwords (instead of PKCS11 pins). Looking forward to your feedback!

@ordex
Copy link
Member

ordex commented Jul 31, 2023

Maybe @dsommers could be more helpful here

@becm
Copy link
Contributor

becm commented Aug 6, 2023

For me, it just felt like this should behave more like a config option that supports inline PIN data.
The triplet (pkcs11-provider, pkcs11-id, pkcs11-pin) is then contained without external dependencies.

Required presence of the token should still make this more secure than traditional askpass use.

The behavioral difference would also enhance the use of a new option (pkcs11-pin) instead of just re-purposing the askpass content as PIN in PKCS11 mode.

I'm not in any position to advise on or green-light implementation though. 😉

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.

5 participants