On 19/10/16 22:12, Steffan Karger wrote: > Hi, > > This feature looks very useful, and the code seems to work well. Aside > from some nitpicking, I do have some questions on the bigger picture:
Thanks a lot!
> On 13-10-16 21:59, David Sommerseth wrote:
>> On a server with --auth-gen-token enabled, the server will have created
>> a random token and pushed it to the client. When the client needs to
>> renegotiate the connection or otherwise reconnect, it will at this point
>> use the auth-token as password.
>
> How does/should auth-token behave across the various restarts? Should
> auth-token for example be cleared on a sigusr1?
This isn't really well defined, from the initial design. This patch is
all based upon the --auth-token feature already present in OpenVPN 2.3,
and what is possible to do via the plug-in/script mechanisms available
today - which works even on v2.3 servers today.
So this feature I introduce here will currently only provide a similar
behaviour as if this the external authentication module had implemented
--auth-token support. Plug-ins and --auth-user-pass-verify can only
return "success" and "fail", which is what this patch in essence does.
With that said, I am not too happy about this ambiguity. But fixing
that should be done separately. And we need to ensure we don't break
existing v2.3 clients, and I believe we need to update the client code a
bit too to achieve these goals.
>> Here we check if we have a token generated and that it has been pushed
>> to the client, if so, then we check if the token matches the locally
>> stored token. If everything matches, we're done and the connection
>> is still authenticated.
>>
>> If the auth-token authentication fails, we delete our local copy of
>> the token and changes the connection to not being authenticated. From
>> this moment of, the client needs to do a full reconnect providing
>> the users password again.
>
> How does the client know this? This patch does not send an AUTH_FAILED
> (or something similar) to the client. Testing seems to indicate that
> the server keeps refusing the token, and the client keeps resending it.
Sending AUTH_FAILED explicit would definitely be a good idea. But it is
needed to verify that the OpenVPN is in a state where it can push and
that the client can accept the push.
But regardless of this, it this will not change the behaviour much at
all. AFAICT, the client code today cannot separate authentication
failures based on passwords or tokens or get a reason why the
authentication failed. (Unless there are something hidden inside IV_*
stuff or Peer-ID).
So what happens? The server goes back to normal username/password
authentication, which fails as the token isn't valid as a password to
the authentication module. The authentication module rejects and the
server sends an AUTH_FAILED, and the client disconnects. At least
that's what happens in my testing environment.
If it is possible to send an AUTH_FAILED directly without going via the
authentication module, we can do that. But going further than that will
most likely require some updates to the client code as well - which I
try to avoid in this round.
>> This token authentication also considers the token lifetime, if that
>> have been set via --auth-gen-token. If the token have expired, the
>> client is rejected and needs to do a full reconnect with a new
>> authentication using the users password.
>
> Similar to the above, I don't see a way for the client to understand
> that the token has expired. That results in the client sending its
> token indefinitely, like above.
Which would be optimal but equally hard as above, as we're currently
lacking a possibility to separate authentication failures.
> Some more nagging below ;)
>
>> Signed-off-by: David Sommerseth <dav...@openvpn.net>
>> ---
>> src/openvpn/ssl_verify.c | 50
>> ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 50 insertions(+)
>>
>> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
>> index 24ec56e..aa982e4 100644
>> --- a/src/openvpn/ssl_verify.c
>> +++ b/src/openvpn/ssl_verify.c
>> @@ -1139,6 +1139,55 @@ verify_user_pass(struct user_pass *up, struct
>> tls_multi *multi,
>> string_mod_remap_name (up->username, COMMON_NAME_CHAR_CLASS);
>> string_mod (up->password, CC_PRINT, CC_CRLF, '_');
>>
>> + /* If server is configured with --auth-gen-token and we have an
>> + * authentication token for this client, base this authentication round
>> + * based on this token instead.
>> + */
>
> base ... based, typo?
Yeah ... that needs to be improved. I'll fix that comment.
>> + if (session->opt->auth_generate_token && multi->auth_token_sent && NULL
>> != multi->auth_token)
>> + {
>> + /* Ensure that the username have not changed */
>
> has not changed
Eeek!
>> + if (!tls_lock_username(multi, up->username))
>> + {
>> + ks->authenticated = false;
>> + goto done;
>> + }
>> +
>> + /* If auth-token lifetime have been enabled, ensure the token is
>> still valid */
>
> has been enabled
*grmbl*
>> + if (session->opt->auth_token_lifetime > 0
>> + && (multi->auth_token_tstamp + session->opt->auth_token_lifetime)
>> < now )
>> + {
>> + msg (D_HANDSHAKE, "Auth-token for client expired\n");
>> + ks->authenticated = false;
>> + goto done;
>> + }
>
> As explained above, I think the client should somehow be notified of
> this expiration, such that it can take action.
Covered in the discussion above.
>> + if (memcmp_constant_time(multi->auth_token, up->password,
>> + strlen(multi->auth_token)) != 0)
>> + {
>> + memset (multi->auth_token, 0, AUTH_TOKEN_SIZE);
>> + free (multi->auth_token);
>> + multi->auth_token = NULL;
>> + multi->auth_token_sent = false;
>> + ks->authenticated = false;
>> + tls_deauthenticate (multi);
>> +
>> + msg (D_TLS_ERRORS, "TLS Auth Error: Auth token verification
>> failed for username '%s' %s",
>> + up->username,
>> + (session->opt->ssl_flags & SSLF_USERNAME_AS_COMMON_NAME) ?
>> "[CN SET]" : "");
>> + }
>
> As above. What should the client do?
Also covered in discussion above.
--
kind regards,
David Sommerseth
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
