[Openvpn-devel,v2] Rework OpenVPN auth-token support

Message ID 1520265024-2129-1-git-send-email-arne@rfc2549.org
State New
Headers show
Series
  • [Openvpn-devel,v2] Rework OpenVPN auth-token support
Related show

Commit Message

Arne Schwabe March 5, 2018, 3:50 p.m.
Auth-token is documented as a token that the client will use instead
of a auth-token. When using an external auth-token script and OTP
authentication, it is useful to have this token survive on reconnect
(e.g. mobile device roaming).  On the other hand if the token does
expire, the client should fall back to the previous authentication
methd (e.g. user/pass) or ask for a OTP password again.

Behaviour of OpenVPN client (prior to this patch) is to never fallback
to the previous authentication method. Depending on auth-retry it
either quit or tried endlessly with an expired token. Since
auth-gen-token tokens expire on reconnect, a client will not survive a
reconnect.

This patch changes the client behaviour:

- Treat a failed auth when using an auth-token as a soft error (USR1)
  and clean the auth-token falling back to the original auth method

- Implement a new pushable option forget-token-reconnect that forces
  to forget an auth-token when reconnecting

- Sending IV_PROTO=3 to signal that it is safe to send this client an
  expiring auth-token

The behaviour of the server option auth-gen-token:

- Automatically push forget-token-reconnect to avoid a failed
  authentication after reconnect

- By default only send auth-token to clients that will gracefully
  handle auth-token to avoid having clients not able to reconnect

- Add a force option to auth-gen-token that allow to ignore if the
  client can handle auth-tokens

Patch V2: properly formatted commit message, fix openvpn3 detection
---
 doc/openvpn.8            | 39 ++++++++++++++++++--
 src/openvpn/init.c       |  4 ++
 src/openvpn/misc.c       | 16 +++++---
 src/openvpn/misc.h       |  3 +-
 src/openvpn/options.c    | 11 +++++-
 src/openvpn/options.h    |  2 +
 src/openvpn/push.c       | 95 ++++++++++++++++++++++++++++++++----------------
 src/openvpn/ssl.c        | 41 +++++++++++++--------
 src/openvpn/ssl.h        |  2 +
 src/openvpn/ssl_common.h |  2 +
 10 files changed, 156 insertions(+), 59 deletions(-)

Comments

Selva Nair March 6, 2018, 9:04 p.m. | #1
Hi,

Based on the commit message this appears to cover all that is wrong
with current auth-token implementation. I haven't carefully reviewed the
code or tested it, but some initial remarks that looks relevant.

On Mon, Mar 5, 2018 at 10:50 AM, Arne Schwabe <arne@rfc2549.org> wrote:
> Auth-token is documented as a token that the client will use instead
> of a auth-token. When using an external auth-token script and OTP
> authentication, it is useful to have this token survive on reconnect
> (e.g. mobile device roaming).  On the other hand if the token does
> expire, the client should fall back to the previous authentication
> methd (e.g. user/pass) or ask for a OTP password again.
>
> Behaviour of OpenVPN client (prior to this patch) is to never fallback
> to the previous authentication method. Depending on auth-retry it
> either quit or tried endlessly with an expired token. Since
> auth-gen-token tokens expire on reconnect, a client will not survive a
> reconnect.

While this is mostly true, there is one point missing here. That being
the main reason for this email, here goes:

While its true that, on SIGUSR1, the client does retry with auth-token
it's only a minor nuisance:  it leads to an AUTH_FAILED and
then the client does go back to prompting for username/password
(assuming auth-retry is enabled). The real sticky problem is when
authentication fails during a TLS reneg due to an expired auth-token.
The client won't realize why the reneg did not complete and will
repeatedly try to renegotiate using the expired (or invalid) token. The
reason for this is that the server does not send back an AUTH_FAILED
message in such cases.

I want to stress this point: when the server sends back AUTH_FAILED,
the client does behave somewhat sanely, but not otherwise. And on that
count this patch appears to be lacking. It teaches the client to forget the
token during SIGUSR1 restarts which is good, but does not teach the server
to send back AUTH_FAILED in all instances of auth failures.

>
> This patch changes the client behaviour:
>
> - Treat a failed auth when using an auth-token as a soft error (USR1)
>   and clean the auth-token falling back to the original auth method

We could be more graceful here by not triggering USR1 -- clearing the
token is enough as that will automatically cause the client to fall
back to auth-user-pass prompting. I say graceful because some failures
do not need a restart, only a re-attempt to authenticate with
a valid username/password.

>
> - Implement a new pushable option forget-token-reconnect that forces
>   to forget an auth-token when reconnecting

I would have liked to see this as the default behaviour: IMO the "right"
way to use auth-token is to expire it on reconnect but if some
existing external scripts want it otherwise, fine...

>
> - Sending IV_PROTO=3 to signal that it is safe to send this client an
>   expiring auth-token

Once the server side auth-failure handling is fixed this will be less
of a concern. But a new IV_PROTO value cant hurt.

>
> The behaviour of the server option auth-gen-token:
>
> - Automatically push forget-token-reconnect to avoid a failed
>   authentication after reconnect

Yes. Unless "forget on reconnect" becomes the only possible client
behaviour :)

>
> - By default only send auth-token to clients that will gracefully
>   handle auth-token to avoid having clients not able to reconnect

I suppose this is regarding expiring tokens, not permanent tokens.

>
> - Add a force option to auth-gen-token that allow to ignore if the
>   client can handle auth-tokens
>
> Patch V2: properly formatted commit message, fix openvpn3 detection

... snipped...

> diff --git a/src/openvpn/push.c b/src/openvpn/push.c
> index 6a30e479..efe4e5a5 100644
> --- a/src/openvpn/push.c
> +++ b/src/openvpn/push.c
> @@ -53,25 +53,31 @@ receive_auth_failed(struct context *c, const struct buffer *buffer)
>      msg(M_VERB0, "AUTH: Received control message: %s", BSTR(buffer));
>      c->options.no_advance = true;
>
> -    if (c->options.pull)
> -    {
> -        switch (auth_retry_get())
> +    if (c->options.pull) {
> +        /* Before checking how to react on AUTH_FAILED, first check if the failed authed might be
> +         * the result of an expired auth-token */
> +        if (ssl_clean_auth_token())
>          {

This is not going to work. The only time the server sends back
AUTH_FAILED is during the initial connection. See that
send_auth_failed() is called only during PUSH request processing[*].
So, failure due to token expiry that normally happens during a reneg[*]
will not trigger AUTH_FAILED and the client will continue trying reneg
until the previous TLS session expires (1 hour?). This is a
basic limitation of the present implementation and I do not see it
being addressed by the patch.

Other than this, the patch is looking good.

Selva
[*] Unless management-client-auth is in use.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Arne Schwabe March 7, 2018, 11:52 a.m. | #2
Am 06.03.18 um 22:04 schrieb Selva Nair:
> Hi,
> 
> Based on the commit message this appears to cover all that is wrong
> with current auth-token implementation. I haven't carefully reviewed the
> code or tested it, but some initial remarks that looks relevant.
> 
> On Mon, Mar 5, 2018 at 10:50 AM, Arne Schwabe <arne@rfc2549.org> wrote:
>> Auth-token is documented as a token that the client will use instead
>> of a auth-token. When using an external auth-token script and OTP
>> authentication, it is useful to have this token survive on reconnect
>> (e.g. mobile device roaming).  On the other hand if the token does
>> expire, the client should fall back to the previous authentication
>> methd (e.g. user/pass) or ask for a OTP password again.
>>
>> Behaviour of OpenVPN client (prior to this patch) is to never fallback
>> to the previous authentication method. Depending on auth-retry it
>> either quit or tried endlessly with an expired token. Since
>> auth-gen-token tokens expire on reconnect, a client will not survive a
>> reconnect.
> 
> While this is mostly true, there is one point missing here. That being
> the main reason for this email, here goes:
> 
> While its true that, on SIGUSR1, the client does retry with auth-token
> it's only a minor nuisance:  it leads to an AUTH_FAILED and
> then the client does go back to prompting for username/password
> (assuming auth-retry is enabled). The real sticky problem is when
> authentication fails during a TLS reneg due to an expired auth-token.
> The client won't realize why the reneg did not complete and will
> repeatedly try to renegotiate using the expired (or invalid) token. The
> reason for this is that the server does not send back an AUTH_FAILED
> message in such cases.
> 
> I want to stress this point: when the server sends back AUTH_FAILED,
> the client does behave somewhat sanely, but not otherwise. And on that
> count this patch appears to be lacking. It teaches the client to forget the
> token during SIGUSR1 restarts which is good, but does not teach the server
> to send back AUTH_FAILED in all instances of auth failures.


I think failed authentication during a renogiation is also a valid issue
that this patch does not address and the behaviour is somewhat broken,
also for non auth-token usage. I would prefer to fix that thing in a
sperate patch. This patch mainly focuses on fixing auth-gen-token and
client interaction as much as possible.


>>
>> This patch changes the client behaviour:
>>
>> - Treat a failed auth when using an auth-token as a soft error (USR1)
>>   and clean the auth-token falling back to the original auth method
> 
> We could be more graceful here by not triggering USR1 -- clearing the
> token is enough as that will automatically cause the client to fall
> back to auth-user-pass prompting. I say graceful because some failures
> do not need a restart, only a re-attempt to authenticate with
> a valid username/password.
> 

Currently this code is only triggered on inital connnection (as the
server fails to send AUTH_FAILED as you noted before). I have not
checked how easy it is for the client to send a second key_exchange message.


>>
>> - Implement a new pushable option forget-token-reconnect that forces
>>   to forget an auth-token when reconnecting
> 
> I would have liked to see this as the default behaviour: IMO the "right"
> way to use auth-token is to expire it on reconnect but if some
> existing external scripts want it otherwise, fine...

That is something we should discuss. I did here err on the side of
caution and not break existing setups since currently the only working
that that does not break on reconnect is one where auth-tokens are valid
after reconnect and changing this default would break those scnearios
and in the past we have most times erred on the side of caution and
compatibility.

>>
>> - Sending IV_PROTO=3 to signal that it is safe to send this client an
>>   expiring auth-token
> 
> Once the server side auth-failure handling is fixed this will be less
> of a concern. But a new IV_PROTO value cant hurt.


You still need the ability in the client to actually be able to forget
and the server needs to know if the client supports forgetting and it is
safe to send the auth-token.

> 
>>
>> The behaviour of the server option auth-gen-token:
>>
>> - Automatically push forget-token-reconnect to avoid a failed
>>   authentication after reconnect
> 
> Yes. Unless "forget on reconnect" becomes the only possible client
> behaviour :)

Sure but then we should add a keep-token-reconnect to support "OTP with
often roaming clients" scenario.

> 
>>
>> - By default only send auth-token to clients that will gracefully
>>   handle auth-token to avoid having clients not able to reconnect
> 
> I suppose this is regarding expiring tokens, not permanent tokens.


Yes, since auth-gen-token has no permanent tokens (they only valid for
the session and after reconnect are invalid), they expire instantly when
a client reconnects.



>> -        switch (auth_retry_get())
>> +    if (c->options.pull) {
>> +        /* Before checking how to react on AUTH_FAILED, first check if the failed authed might be
>> +         * the result of an expired auth-token */
>> +        if (ssl_clean_auth_token())
>>          {
> 
> This is not going to work. The only time the server sends back
> AUTH_FAILED is during the initial connection. See that
> send_auth_failed() is called only during PUSH request processing[*].

At least it fixes a client's behaviour with this patch with server that
has the default auth-gen-token without expiry set. :)

> So, failure due to token expiry that normally happens during a reneg[*]
> will not trigger AUTH_FAILED and the client will continue trying reneg
> until the previous TLS session expires (1 hour?). This is a
> basic limitation of the present implementation and I do not see it
> being addressed by the patch.

I will look into sending AUTH_FAILED also without management-client-auth.

Arne

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
David Sommerseth March 7, 2018, 12:13 p.m. | #3
On 07/03/18 12:52, Arne Schwabe wrote:
>> So, failure due to token expiry that normally happens during a reneg[*]
>> will not trigger AUTH_FAILED and the client will continue trying reneg
>> until the previous TLS session expires (1 hour?). This is a
>> basic limitation of the present implementation and I do not see it
>> being addressed by the patch.
>
> I will look into sending AUTH_FAILED also without management-client-auth.

Just a quick response.  AUTH_FAILED can provide an explanation which the
client can pick up and act upon.  I did that in an earlier attempt, but it
required lots of refactoring as the send_control() function depends on a few
structs not being available where the token failures can be tackled.

See this mail thread for details:
   Message-Id: 1477918318-18561-1-git-send-email-davids@openvpn.net
   Mail archive:
<https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg12848.html>

This approach is what James initially recommended me to do, and this approach
of providing a failure message back to the client should already be supported
in OpenVPN 3 and the OpenVPN Connect clients.  This messaging I believe is
already used by the Access Server and Private Tunnel.  OpenVPN 2 in client
mode, could in these cases catch this "message" and ask for user credentials
again.

In regards to having the tokens survive reconnect, I agree.  But lets try to
split this challenge into a few more minor patches.

The challenge I see with starting with a new IV_PROTO=3 is that this also
requires support in the OpenVPN 3 library as well.  I believe the "expiring
auth-token" feature would be better handled with proper AUTH_FAILED signalling.

I'll have a closer look (hopefully) later today, just wanted to give this
quick feedback before its too late.
Selva Nair March 7, 2018, 3:47 p.m. | #4
Hi,


On Wed, Mar 7, 2018 at 6:52 AM, Arne Schwabe <arne@rfc2549.org> wrote:
> Am 06.03.18 um 22:04 schrieb Selva Nair:
>
..

>> I want to stress this point: when the server sends back AUTH_FAILED,
>> the client does behave somewhat sanely, but not otherwise. And on that
>> count this patch appears to be lacking. It teaches the client to forget the
>> token during SIGUSR1 restarts which is good, but does not teach the server
>> to send back AUTH_FAILED in all instances of auth failures.
>
>
> I think failed authentication during a renogiation is also a valid issue
> that this patch does not address and the behaviour is somewhat broken,
> also for non auth-token usage. I would prefer to fix that thing in a
> sperate patch. This patch mainly focuses on fixing auth-gen-token and
> client interaction as much as possible.

That is fine. In that case let's not pretend that an expiring
auth-token using auth-gen-token is a supported feature. Disable it in
the server until we have the ability to send AUTH_FAILED in such
cases.

>
>
>>>
>>> This patch changes the client behaviour:
>>>
>>> - Treat a failed auth when using an auth-token as a soft error (USR1)
>>>   and clean the auth-token falling back to the original auth method
>>
>> We could be more graceful here by not triggering USR1 -- clearing the
>> token is enough as that will automatically cause the client to fall
>> back to auth-user-pass prompting. I say graceful because some failures
>> do not need a restart, only a re-attempt to authenticate with
>> a valid username/password.
>>
>
> Currently this code is only triggered on inital connnection (as the
> server fails to send AUTH_FAILED as you noted before). I have not
> checked how easy it is for the client to send a second key_exchange message.

Agreed SIGUSR1 is appropriate for initial failure, but initial failure
happens with auth-token only because the client does not forget the
token during SIGUSR1/SIGHUP reconnects. Once it learns to do that (as
this patch does), there are no "initial failures" with auth-token
(more on that below).

>
>
>>>
>>> - Implement a new pushable option forget-token-reconnect that forces
>>>   to forget an auth-token when reconnecting
>>
>> I would have liked to see this as the default behaviour: IMO the "right"
>> way to use auth-token is to expire it on reconnect but if some
>> existing external scripts want it otherwise, fine...
>
> That is something we should discuss. I did here err on the side of
> caution and not break existing setups since currently the only working
> that that does not break on reconnect is one where auth-tokens are valid
> after reconnect and changing this default would break those scnearios
> and in the past we have most times erred on the side of caution and
> compatibility.

Agreed, if external scripts that implement auth-token without using
auth-gen-token wants to reuse the token, fine..

>
>
>
>>> -        switch (auth_retry_get())
>>> +    if (c->options.pull) {
>>> +        /* Before checking how to react on AUTH_FAILED, first check if the failed authed might be
>>> +         * the result of an expired auth-token */
>>> +        if (ssl_clean_auth_token())
>>>          {
>>
>> This is not going to work. The only time the server sends back
>> AUTH_FAILED is during the initial connection. See that
>> send_auth_failed() is called only during PUSH request processing[*].
>
> At least it fixes a client's behaviour with this patch with server that
> has the default auth-gen-token without expiry set. :)

Not really -- that gets fixed by forget-token-reconnect which is
pushed when auth-gen-token is set. So I think this code path will
never get exercised. As this patch will push forget-token-reconnect,
every restart will call ssl_clean_auth_token() from init.c so the
client will use the username/password, not any previously set token.
As AUTH_FAILED is received only during such first attempts, there is
no token to clean at that point and thus this will be skipped. It will
"mysteriously" start to get exercised if/when we later fix
AUTH_FAILED. So better leave it out of this patch. Or am I missing
something?

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
David Sommerseth March 7, 2018, 10:38 p.m. | #5
A bit more thorough review this time.

On 05/03/18 16:50, Arne Schwabe wrote:
[...snip...]
> 
> This patch changes the client behaviour:
> 
> - Treat a failed auth when using an auth-token as a soft error (USR1)
>   and clean the auth-token falling back to the original auth method

Conceptually, this makes sense.

> - Implement a new pushable option forget-token-reconnect that forces
>   to forget an auth-token when reconnecting

This I am not happy about.  We should use an AUTH_FAILED,SESSION:$message
approach instead.  This is what OpenVPN 3 based clients expects, and it should
actually do (read: behave) as the previous point.  OpenVPN 2 does not do that,
today, so it would need to be taught that instead.

> - Sending IV_PROTO=3 to signal that it is safe to send this client an
>   expiring auth-token

And as we shouldn't need forget-token-reconnect, this would also not be needed.

> The behaviour of the server option auth-gen-token:
> 
> - Automatically push forget-token-reconnect to avoid a failed
>   authentication after reconnect
This should not be needed with the AUTH_FAILED,SESSION:$message approach.

> - By default only send auth-token to clients that will gracefully
>   handle auth-token to avoid having clients not able to reconnect

What do you mean with "gracefully handle auth-token"?  By fixing OpenVPN 2's
behaviour (as suggested above), it should behave gracefully by default.

And I'd be willing to /consider/ this fix even to OpenVPN 2.3 code base, for
those who can't upgrade.  But it will be a hard sell, though; OpenVPN 2.4 is
over a year old already.

> - Add a force option to auth-gen-token that allow to ignore if the
>   client can handle auth-tokens

And this should also not be needed.


I'll admit I might see this with a bit too narrow perspective.  But how I have
understood this issue is that OpenVPN 2.x does not behave correctly as it
doesn't understand *why* the authentication failed.  If the client side would
understand why auth failed, then it can query the user for credentials again -
which I believe should resolve the current issues ... Or have I missed something?
Selva Nair March 7, 2018, 11:22 p.m. | #6
Hi,

...some good stuff snipped...

>
> I'll admit I might see this with a bit too narrow perspective.  But how I have
> understood this issue is that OpenVPN 2.x does not behave correctly as it
> doesn't understand *why* the authentication failed.  If the client side would
> understand why auth failed, then it can query the user for credentials again -
> which I believe should resolve the current issues ... Or have I missed something?

I hope we are slowly spiralling towards a solution; not going around
in circles...

Anyway, to reiterate: we currently have two issues. (i) client is not
told when authentication fails during reneg
and (ii) client doesn't know that auth-gen-token's token is not
reusable across reconnects
(SIGHUP and SIGUSR1).

Fixing (i) does not fix (ii). But (ii) easier to fix although we could
keep arguing whether a forget-token-reconnect should be used or not
etc..
(i) is the trickier, though I'm not convinced it requires so much refactoring.

Anyway, if there is an immediate solution to (i) it may be better to
fix (ii) along with it. Else just fixing (ii) along the lines Arne has
proposed looks like the way to go.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
David Sommerseth March 7, 2018, 11:52 p.m. | #7
On 08/03/18 00:22, Selva Nair wrote:
> Hi,
> 
> ...some good stuff snipped...
> 
>>
>> I'll admit I might see this with a bit too narrow perspective.  But how I have
>> understood this issue is that OpenVPN 2.x does not behave correctly as it
>> doesn't understand *why* the authentication failed.  If the client side would
>> understand why auth failed, then it can query the user for credentials again -
>> which I believe should resolve the current issues ... Or have I missed something?
> 
> I hope we are slowly spiralling towards a solution; not going around
> in circles...
> 
> Anyway, to reiterate: we currently have two issues. (i) client is not
> told when authentication fails during reneg
> and (ii) client doesn't know that auth-gen-token's token is not
> reusable across reconnects
> (SIGHUP and SIGUSR1).

Okay, (i) is handled with AUTH_FAILED,SESSION messages today in OpenVPN 3.
OpenVPN 2 should be taught to do the same - both server and client side.

In regards to (ii), that is an implementation detail which I would say is
incorrect with --auth-gen-token.  As I am the one who implemented that
feature, I would rather have this fixed so auth-gen-token works across restarts.

Another new feature I can accept, is to enable rotating the auth-tokens.  This
is currently not possible.  Not a need-to-have feature now, but more a
nice-to-have.

> Fixing (i) does not fix (ii). But (ii) easier to fix although we could
> keep arguing whether a forget-token-reconnect should be used or not
> etc..
> (i) is the trickier, though I'm not convinced it requires so much refactoring.
> 
> Anyway, if there is an immediate solution to (i) it may be better to
> fix (ii) along with it. Else just fixing (ii) along the lines Arne has
> proposed looks like the way to go.

Arne's approach with forget-token and IV_PROTO=3 diverges how OpenVPN 2 and
OpenVPN 3 behaves.  This is *not* an option for this issue.

The reason is that OpenVPN Access Server 2.5 is now shipping with an
unmodified (to my knowledge, at least) upstream OpenVPN 2.4.  This server
integrates well with the OpenVPN Connect clients, which we have for Android,
iOS, macOS and Windows (All OpenVPN 3 based).  It would be sad if the
community side fixes something which isn't really an issue (as part of the
solution is already there); we're just doing it wrong currently.  The current
approach by Arne would also require support in OpenVPN 3 too; and I doubt this
approach will get easily accepted there.

Also beware that more and more users will start deploying OpenVPN 3 based
clients too; it is already widespread on iOS.  And there are efforts on
getting OpenVPN Connect into even more appstores for different platforms.  On
top of that, in addition to the new open source openvpn3-linux client, there
will also be put some efforts to at least provide more mature reference
implementation clients for Windows and macOS too.  Plus Arne is also poking at
using OpenVPN 3 in his Android client.  Many of them will definitely connect
against OpenVPN 2.x based servers; so OpenVPN 2 and OpenVPN 3 need to be well
aligned.

(With that said, I'm not saying OpenVPN 3 supports all OpenVPN 2 features 100%
today, but the most important features are there - like authentication).
Selva Nair March 8, 2018, 12:25 a.m. | #8
Hi,

On Wed, Mar 7, 2018 at 6:52 PM, David Sommerseth
<openvpn@sf.lists.topphemmelig.net> wrote:
> On 08/03/18 00:22, Selva Nair wrote:
>> Hi,
>>
>> ...some good stuff snipped...
>>
>>>
>>> I'll admit I might see this with a bit too narrow perspective.  But how I have
>>> understood this issue is that OpenVPN 2.x does not behave correctly as it
>>> doesn't understand *why* the authentication failed.  If the client side would
>>> understand why auth failed, then it can query the user for credentials again -
>>> which I believe should resolve the current issues ... Or have I missed something?
>>
>> I hope we are slowly spiralling towards a solution; not going around
>> in circles...
>>
>> Anyway, to reiterate: we currently have two issues. (i) client is not
>> told when authentication fails during reneg
>> and (ii) client doesn't know that auth-gen-token's token is not
>> reusable across reconnects
>> (SIGHUP and SIGUSR1).
>
> Okay, (i) is handled with AUTH_FAILED,SESSION messages today in OpenVPN 3.
> OpenVPN 2 should be taught to do the same - both server and client side.
>
> In regards to (ii), that is an implementation detail which I would say is
> incorrect with --auth-gen-token.  As I am the one who implemented that
> feature, I would rather have this fixed so auth-gen-token works across restarts.
>

Well, IMO, fixing (i) is more important than (ii). It would also make it
easy to add support for dynamic auth through scripts and plugin,
for example. Currently not possible because a specially crafted
"reason" for AUTH_FAILED cannot be sent back unless
management-client-auth is used. Unrelated to auth-token but a sorely
missing feature. That said, personally I prefer management-client-auth
to using scripts and the former works fine as is.

Regarding (ii), reusing auth-token across SIGHUP and SIGUSR1
reconnects could have security implications. At the minimum we should
treat auth-token as a high value asset on the client side and not send
it to management (at least on the Windows GUI it currently shows up in
plain text in the status log). In the current state of things, the
client-side management has no use for the actual value of the token as
it cant override the client daemon's handling of it.

Selva

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot

Patch

diff --git a/doc/openvpn.8 b/doc/openvpn.8
index d1fb4ec6..02c51a10 100644
--- a/doc/openvpn.8
+++ b/doc/openvpn.8
@@ -3058,7 +3058,8 @@  IV_LZO_STUB=1 \-\- if client was built with LZO stub capability
 
 IV_LZ4=1 \-\- if the client supports LZ4 compressions.
 
-IV_PROTO=2 \-\- if the client supports peer\-id floating mechansim
+IV_PROTO=3 \-\- if the client supports peer\-id floating mechansim (2 or greater) and
+fallback to normal authentication after a failed auth-token authentication (3 or greater)
 
 IV_NCP=2 \-\- negotiable ciphers, client supports
 .B \-\-cipher
@@ -3664,7 +3665,7 @@  For a sample script that performs PAM authentication, see
 in the OpenVPN source distribution.
 .\"*********************************************************
 .TP
-.B \-\-auth\-gen\-token [lifetime]
+.B \-\-auth\-gen\-token [lifetime] [force]
 After successful user/password authentication, the OpenVPN
 server will with this option generate a temporary
 authentication token and push that to client.  On the following
@@ -3684,6 +3685,12 @@  This feature is useful for environments which is configured
 to use One Time Passwords (OTP) as part of the user/password
 authentications and that authentication mechanism does not
 implement any auth\-token support.
+
+The
+.B force
+option will send auth-token even if the client does not signal
+proper auth-token support on reconnect and will fail on reconnect with
+failed authentication. This is the behaviour of versions prior to 2.4.6.
 .\"*********************************************************
 .TP
 .B \-\-opt\-verify
@@ -5337,7 +5344,33 @@  push "auth\-token UNIQUE_TOKEN_VALUE"
 into the file/buffer for dynamic configuration data.  This
 will then make the OpenVPN server to push this value to the
 client, which replaces the local password with the
-UNIQUE_TOKEN_VALUE.
+UNIQUE_TOKEN_VALUE. If you want the client (2.4.6+) to forget the token when
+reconnecting also use:
+
+.nf
+.ft 3
+.in +4
+push "forget\-token\-reconnect"
+.in -4
+.ft
+.fi
+
+Newer clients (2.4.6+) will fall back to the original password method
+after a failed auth-token attempt. Older clients will keep using the token
+value and react acording to
+.B \-\-auth-retry
+.
+.\**********************************************************
+.TP
+.B forget\-token\-reconnect
+
+Normally a client will retry an auth-token after a reconnect (e.g. USR1 triggered).
+If the token fails, the client will fall back to the original method and make an additional
+connection attempt. If the token is only valid for a single connection this option can be used
+to skip the first attempt on reconnect. The
+.B
+auth\-gen\-token
+option automatically pushes this option.
 .\"*********************************************************
 .TP
 .B \-\-tls\-verify cmd
diff --git a/src/openvpn/init.c b/src/openvpn/init.c
index 36c1a4c4..59be55e8 100644
--- a/src/openvpn/init.c
+++ b/src/openvpn/init.c
@@ -2750,6 +2750,7 @@  do_init_crypto_tls(struct context *c, const unsigned int flags)
     to.auth_user_pass_file = options->auth_user_pass_file;
     to.auth_token_generate = options->auth_token_generate;
     to.auth_token_lifetime = options->auth_token_lifetime;
+    to.auth_token_generate_force = options->auth_token_generate_force;
 #endif
 
     to.x509_track = options->x509_track;
@@ -3390,6 +3391,9 @@  do_close_tls(struct context *c)
         md_ctx_cleanup(c->c2.pulled_options_state);
         md_ctx_free(c->c2.pulled_options_state);
     }
+
+    if (c->options.forget_token_on_reconnect)
+        ssl_clean_auth_token();
 }
 
 /*
diff --git a/src/openvpn/misc.c b/src/openvpn/misc.c
index 72ffc406..e41198ee 100644
--- a/src/openvpn/misc.c
+++ b/src/openvpn/misc.c
@@ -1258,7 +1258,7 @@  purge_user_pass(struct user_pass *up, const bool force)
      * don't show warning if the pass has been replaced by a token: this is an
      * artificial "auth-nocache"
      */
-    else if (!warn_shown && (!up->tokenized))
+    else if (!warn_shown)
     {
         msg(M_WARN, "WARNING: this configuration may cache passwords in memory -- use the auth-nocache option to prevent this");
         warn_shown = true;
@@ -1266,14 +1266,18 @@  purge_user_pass(struct user_pass *up, const bool force)
 }
 
 void
-set_auth_token(struct user_pass *up, const char *token)
+set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token)
 {
-    if (token && strlen(token) && up && up->defined && !up->nocache)
+
+    if (token && strlen(token) && up && up->defined)
     {
-        CLEAR(up->password);
-        strncpynt(up->password, token, USER_PASS_LEN);
-        up->tokenized = true;
+        strncpynt(tk->password, token, USER_PASS_LEN);
+        strncpynt(tk->username, up->username, USER_PASS_LEN);
+        tk->defined = true;
     }
+
+    /* Cleans user/pass for nocache */
+    purge_user_pass(up, false);
 }
 
 /*
diff --git a/src/openvpn/misc.h b/src/openvpn/misc.h
index dad6a5de..68ec6a94 100644
--- a/src/openvpn/misc.h
+++ b/src/openvpn/misc.h
@@ -168,7 +168,6 @@  struct user_pass
 {
     bool defined;
     bool nocache;
-    bool tokenized; /* true if password has been substituted by a token */
     bool wait_for_push; /* true if this object is waiting for a push-reply */
 
 /* max length of username/password */
@@ -250,7 +249,7 @@  void fail_user_pass(const char *prefix,
 
 void purge_user_pass(struct user_pass *up, const bool force);
 
-void set_auth_token(struct user_pass *up, const char *token);
+void set_auth_token(struct user_pass *up, struct user_pass *tk, const char *token);
 
 /*
  * Process string received by untrusted peer before
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index cea143b8..0dc6cc5c 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -883,6 +883,7 @@  init_options(struct options *o, const bool init_gc)
 /* P2MP server context features */
 #if P2MP_SERVER
     o->auth_token_generate = false;
+    o->auth_token_generate_force = false;
 
     /* Set default --tmp-dir */
 #ifdef _WIN32
@@ -1334,6 +1335,7 @@  show_p2mp_parms(const struct options *o)
     SHOW_STR(auth_user_pass_verify_script);
     SHOW_BOOL(auth_user_pass_verify_script_via_file);
     SHOW_BOOL(auth_token_generate);
+    SHOW_BOOL(auth_token_generate_force);
     SHOW_INT(auth_token_lifetime);
 #if PORT_SHARE
     SHOW_STR(port_share_host);
@@ -6718,11 +6720,13 @@  add_option(struct options *options,
                         &options->auth_user_pass_verify_script,
                         p[1], "auth-user-pass-verify", true);
     }
-    else if (streq(p[0], "auth-gen-token"))
+    else if (streq(p[0], "auth-gen-token") && !p[3])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
         options->auth_token_generate = true;
         options->auth_token_lifetime = p[1] ? positive_atoi(p[1]) : 0;
+        if (p[2] && streq(p[2], "force"))
+            options->auth_token_generate_force = true;
     }
     else if (streq(p[0], "client-connect") && p[1])
     {
@@ -7791,6 +7795,11 @@  add_option(struct options *options,
         }
 #endif
     }
+    else if (streq(p[0], "forget-token-reconnect") && !p[1])
+    {
+        VERIFY_PERMISSION(OPT_P_GENERAL|OPT_P_ECHO);
+        options->forget_token_on_reconnect = true;
+    }
     else if (streq(p[0], "single-session") && !p[1])
     {
         VERIFY_PERMISSION(OPT_P_GENERAL);
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index d9a5b372..5e3a999e 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -443,6 +443,7 @@  struct options
     const char *auth_user_pass_verify_script;
     bool auth_user_pass_verify_script_via_file;
     bool auth_token_generate;
+    bool auth_token_generate_force;
     unsigned int auth_token_lifetime;
 #if PORT_SHARE
     char *port_share_host;
@@ -457,6 +458,7 @@  struct options
     unsigned int push_option_types_found;
     const char *auth_user_pass_file;
     struct options_pre_pull *pre_pull;
+    bool forget_token_on_reconnect;
 
     int scheduled_exit_interval;
 
diff --git a/src/openvpn/push.c b/src/openvpn/push.c
index 6a30e479..efe4e5a5 100644
--- a/src/openvpn/push.c
+++ b/src/openvpn/push.c
@@ -53,25 +53,31 @@  receive_auth_failed(struct context *c, const struct buffer *buffer)
     msg(M_VERB0, "AUTH: Received control message: %s", BSTR(buffer));
     c->options.no_advance = true;
 
-    if (c->options.pull)
-    {
-        switch (auth_retry_get())
+    if (c->options.pull) {
+        /* Before checking how to react on AUTH_FAILED, first check if the failed authed might be
+         * the result of an expired auth-token */
+        if (ssl_clean_auth_token())
         {
-            case AR_NONE:
-                c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
-                break;
-
-            case AR_INTERACT:
-                ssl_purge_auth(false);
-
-            case AR_NOINTERACT:
-                c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
-                break;
-
-            default:
-                ASSERT(0);
+            c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
+            c->sig->signal_text = "auth-failure (auth-token)";
+        } else {
+            switch (auth_retry_get()) {
+                case AR_NONE:
+                    c->sig->signal_received = SIGTERM; /* SOFT-SIGTERM -- Auth failure error */
+                    break;
+
+                case AR_INTERACT:
+                    ssl_purge_auth(false);
+
+                case AR_NOINTERACT:
+                    c->sig->signal_received = SIGUSR1; /* SOFT-SIGUSR1 -- Auth failure error */
+                    break;
+
+                default:
+                    ASSERT(0);
+            }
+            c->sig->signal_text = "auth-failure";
         }
-        c->sig->signal_text = "auth-failure";
 #ifdef ENABLE_MANAGEMENT
         if (management)
         {
@@ -325,7 +331,6 @@  static bool
 prepare_push_reply(struct context *c, struct gc_arena *gc,
                    struct push_list *push_list)
 {
-    const char *optstr = NULL;
     struct tls_multi *tls_multi = c->c2.tls_multi;
     const char *const peer_info = tls_multi->peer_info;
     struct options *o = &c->options;
@@ -355,18 +360,33 @@  prepare_push_reply(struct context *c, struct gc_arena *gc,
                                         0, gc));
     }
 
-    /* Send peer-id if client supports it */
-    optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
+    /* Extract the version info from IV_PROTO */
+    const char* optstr = peer_info ? strstr(peer_info, "IV_PROTO=") : NULL;
+    int protover = 0;
     if (optstr)
     {
-        int proto = 0;
-        int r = sscanf(optstr, "IV_PROTO=%d", &proto);
-        if ((r == 1) && (proto >= 2))
-        {
-            push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
-                            tls_multi->peer_id);
-            tls_multi->use_peer_id = true;
-        }
+        int r = sscanf(optstr, "IV_PROTO=%d", &protover);
+        if (r != 1)
+            protover = 0;
+    }
+
+    /* OpenVPN 3 C++ core *always* forgets the auth-token at the end of a session, check if client
+     * is an OpenVPN 3 client */
+    int clientmajor=0;
+    optstr = peer_info ? strstr(peer_info, "IV_VER=") : NULL;
+    if (optstr)
+    {
+      int r = sscanf(optstr, "IV_VER=%d", &clientmajor);
+      if (r != 1)
+        clientmajor=0;
+    }
+
+    /* Send peer-id if client supports it */
+    if (protover >= 2)
+    {
+        push_option_fmt(gc, push_list, M_USAGE, "peer-id %d",
+                        tls_multi->peer_id);
+        tls_multi->use_peer_id = true;
     }
 
     /* Push cipher if client supports Negotiable Crypto Parameters */
@@ -400,12 +420,25 @@  prepare_push_reply(struct context *c, struct gc_arena *gc,
 
     /* If server uses --auth-gen-token and we have an auth token
      * to send to the client
+     * Also push forget-token-reconnect since our token is only valid for
+     * a sigle connection. Older clients will just ignore the option
      */
     if (false == tls_multi->auth_token_sent && NULL != tls_multi->auth_token)
     {
-        push_option_fmt(gc, push_list, M_USAGE,
-                        "auth-token %s", tls_multi->auth_token);
-        tls_multi->auth_token_sent = true;
+        if (protover >= 3 || clientmajor == 3 || o->auth_token_generate_force)
+        {
+            push_option_fmt(gc, push_list, M_USAGE,
+                            "auth-token %s", tls_multi->auth_token);
+            push_option_fmt(gc, push_list, M_USAGE,
+                            "forget-token-reconnect");
+            tls_multi->auth_token_sent = true;
+        }
+        else
+        {
+            msg(D_TLS_DEBUG_LOW, "Not sending auth-token to client since it will not "
+                "properly reconnect. (Use force option to auth-gen-token "
+                "to force sending)");
+        }
     }
     return true;
 }
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index d758c31a..d90b914d 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -398,6 +398,7 @@  pem_password_callback(char *buf, int size, int rwflag, void *u)
 
 static bool auth_user_pass_enabled;     /* GLOBAL */
 static struct user_pass auth_user_pass; /* GLOBAL */
+static struct user_pass auth_token;     /* GLOBAL */
 
 #ifdef ENABLE_CLIENT_CR
 static char *auth_challenge; /* GLOBAL */
@@ -407,7 +408,7 @@  void
 auth_user_pass_setup(const char *auth_file, const struct static_challenge_info *sci)
 {
     auth_user_pass_enabled = true;
-    if (!auth_user_pass.defined)
+    if (!auth_user_pass.defined && !auth_token.defined)
     {
 #if AUTO_USERID
         get_user_pass_auto_userid(&auth_user_pass, auth_file);
@@ -449,7 +450,7 @@  ssl_set_auth_nocache(void)
 {
     passbuf.nocache = true;
     auth_user_pass.nocache = true;
-    /* wait for push-reply, because auth-token may invert nocache */
+    /* wait for push-reply, because auth-token may still need the username */
     auth_user_pass.wait_for_push = true;
 }
 
@@ -459,15 +460,18 @@  ssl_set_auth_nocache(void)
 void
 ssl_set_auth_token(const char *token)
 {
-    if (auth_user_pass.nocache)
-    {
-        msg(M_INFO,
-            "auth-token received, disabling auth-nocache for the "
-            "authentication token");
-        auth_user_pass.nocache = false;
-    }
+    set_auth_token(&auth_user_pass, &auth_token, token);
+}
 
-    set_auth_token(&auth_user_pass, token);
+/*
+ * Cleans an auth token and checks if it was active
+ */
+bool
+ssl_clean_auth_token (void)
+{
+    bool wasdefined = auth_token.defined;
+    purge_user_pass(&auth_token, true);
+    return wasdefined;
 }
 
 /*
@@ -2270,8 +2274,9 @@  push_peer_info(struct buffer *buf, struct tls_session *session)
         buf_printf(&out, "IV_PLAT=win\n");
 #endif
 
-        /* support for P_DATA_V2 */
-        buf_printf(&out, "IV_PROTO=2\n");
+        /* support for P_DATA_V2 and
+         * fallback to normal authentication after a failed auth-token authentication  */
+        buf_printf(&out, "IV_PROTO=3\n");
 
         /* support for Negotiable Crypto Paramters */
         if (session->opt->ncp_enabled
@@ -2377,19 +2382,23 @@  key_method_2_write(struct buffer *buf, struct tls_session *session)
 #else
         auth_user_pass_setup(session->opt->auth_user_pass_file, NULL);
 #endif
-        if (!write_string(buf, auth_user_pass.username, -1))
+        struct user_pass* up = &auth_user_pass;
+
+        /* If we have a valid auth-token, send that instead of real username/password */
+        if (auth_token.defined)
+            up = &auth_token;
+
+        if (!write_string(buf, up->username, -1))
         {
             goto error;
         }
-        if (!write_string(buf, auth_user_pass.password, -1))
+        else if (!write_string(buf, up->password, -1))
         {
             goto error;
         }
         /* if auth-nocache was specified, the auth_user_pass object reaches
          * a "complete" state only after having received the push-reply
          * message.
-         * This is the case because auth-token statement in a push-reply would
-         * invert its nocache.
          *
          * For this reason, skip the purge operation here if no push-reply
          * message has been received yet.
diff --git a/src/openvpn/ssl.h b/src/openvpn/ssl.h
index a2501c9b..116e76a1 100644
--- a/src/openvpn/ssl.h
+++ b/src/openvpn/ssl.h
@@ -428,6 +428,8 @@  void ssl_purge_auth(const bool auth_user_pass_only);
 
 void ssl_set_auth_token(const char *token);
 
+bool ssl_clean_auth_token(void);
+
 #ifdef ENABLE_CLIENT_CR
 /*
  * ssl_get_auth_challenge will parse the server-pushed auth-failed
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 08ef6ffa..d39380d4 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -299,6 +299,8 @@  struct tls_options
     const char *auth_user_pass_file;
     bool auth_token_generate;   /**< Generate auth-tokens on successful user/pass auth,
                                  *   set via options->auth_token_generate. */
+    bool auth_token_generate_force;  /* Sent auth-tokens even if the client will not reconnect
+                                      * properly with them */
     unsigned int auth_token_lifetime;
 
     /* use the client-config-dir as a positive authenticator */