add ability to save pin for smartcards

[tantra35]

This patch adds ability to save smartcards(i.e. yubikey) pins

[selvanair]

Thanks for the PR.

I'm not sure whether saving a smartcard/token PIN is a good idea. Many tokens lock-up if a wrong PIN is entered multiple times. Its easy for this to happen if we use a saved PIN and somehow miss to promptly clear it on failure.

Apart from that, the patch has some issues: it handles token password similar to auth-password. Instead treat it like the private key password and use the flags related to it (such as FLAG_SAVE_KEY_PASS). Also, if saved, consider submitting with no delay as done for private key password.

failed_psw_attempts has to be incremented when token password fails. Unclear whether it will be caught by the cases currently handled in OnStateChange().

We can intercept PIN prompts only when a token is used through pkcs11-helper or pkcs11-provider. On windows, its much better to use the cryptoapicert option which can cover certificates/keys in tokens as well. In that case PIN prompt is triggered by Windows itself and not seen by OpenVPN-GUI.

[selvanair]

This flag determines whether user-auth password should be saved or not. Do not disable it here as this is not about user-auth, but token password. Instead use FLAG_KEY_SAVE_PASS. Also respect FLAG_DISABLE_SAVE_PASS which allows administrators to ban password saving.

[tantra35]

@thanks for your review, i fixed as i think all you remarks, also i want to coment something:

I'm not sure whether saving a smartcard/token PIN is a good idea. Many tokens lock-up if a wrong PIN >is entered multiple times. Its easy for this to happen if we use a saved PIN and somehow miss to >promptly clear it on failure.

Yeah you right but for example viscosity client have ability to save pin, so i think openvpn-gui also must have this...

We can intercept PIN prompts only when a token is used through pkcs11-helper or pkcs11-provider. On >windows, its much better to use the cryptoapicert option which can cover certificates/keys in tokens as >well. In that case PIN prompt is triggered by Windows itself and not seen by OpenVPN-GUI.

I don't think using windows cryptoapi is a good idea, since the dialog it shows is completely different from the openvpn gui, and may confuse users - in other words, they may not understand what it is all about.

[selvanair]

Isn't it better to make this hiding of the save password checkbox the default outside of all if-else clauses? That will simplify the logic and have less deeply nested code.
Like, the following line could become else if (!(param->c->flags & FLAG_DISABLE_SAVE_PASS) && RecallSmart...())

[selvanair]

Have you confirmed that c->failed_auth_attempts does get incremented when token password fails?
PS. Also, shouldn't it be c->failed_psw_attempts ?

[selvanair]

Have you confirmed that c->failed_auth_attempts does get incremented when token password fails?

In fact, a wrong PIN causes pkcs11-helper to re-prompt for it three times without a restart. When a SIGUSR1 restart happens finally, its triggered as a generic "tls-error" leading to a further round of wrong PIN submission and so on.

This is not acceptable. So, NAK from me.

[tantra35]

Have you confirmed that c->failed_auth_attempts does get incremented when token password fails?

In fact, a wrong PIN causes pkcs11-helper to re-prompt for it three times without a restart. When a SIGUSR1 restart happens finally, its triggered as a generic "tls-error" leading to a further round of wrong PIN submission and so on.

Thanks for you investigations, its very helpful for me(I not fully yet understand how full process works), and I have few questions:

1. if I understand all correctly openvpn it case of wrong ping just reprompt it throw management socket?
2. About restart I not fully understand, why you write about SIGUSR1? Its a *nix signal handler as I think...
3. If openvpn just reprompt pin within existent management connection, it's very easy enter additional counter for pin attempt in connection structure(https://github.com/OpenVPN/openvpn-gui/blob/master/options.h#L137), or just increment already existent counters failed_psw_attempts or failed_auth_attempts(not sure that this is acceptable), if so, it's looks like a solution

[selvanair]

1. if I understand all correctly openvpn it case of wrong ping just reprompt it throw management socket?

Yes. You can test this yourself using a config and deliberately enter a wrong PIN. Testing that the patch works when all goes well is not good enough --- wrong PIN and combinations like right PIN followed by some other failures (say, wrong auth-password or wrong certificate) need to work correctly too.

2. About restart I not fully understand, why you write about SIGUSR1? Its a *nix signal handler as I think...

"SIGUSR1 restart" is what OpenVPN does on non-fatal errors. It uses a pseudo-signal generated internally. For the purpose of this discussion, just think of it as an internal "soft restart" of the connection process. When that happens the management interface gets the "RECONNECTING" message with a short description of what went wrong. But token prompt comes from pkcs11-helper library and it retries 3 times before a restart can trigger.

3. If openvpn just reprompt pin within existent management connection, it's very easy enter additional counter for pin attempt in connection structure(https://github.com/OpenVPN/openvpn-gui/blob/master/options.h#L137), or just increment already existent counters failed_psw_attempts or failed_auth_attempts(not sure that this is acceptable), if so, it's looks like a solution

At what point to increment the counter? Correct location is where restart occurs and we get an appropriate "RECONNECTING" message as done for key password and auth password. But that's too late in this case as pkcs11-helper library does the multiple prompting (see above). Also, the RECONNECTING error message in case of PIN failure is too generic to be sure that it was PIN that failed.

You can try, but do test various scenarios to ensure the user experience is not adversely affected. A wrongly incremented counter will cause a confusing PIN error warning in the next round if a restart occurs due to some other failure.