Management goes another code path and management_client_auth directly
calls send_auth_failed.
I'm afraid in the case of renegotiation this is not relevant
But I
also haven't digged deep enough to actually understand if your is
actually fixing the problem correctly.
May I request that we resolve this first to ensure the content of the
patch is correct, and then we can move onto finding a way to avoid this
extra state?
You haven't described the bug/symptons you are seeing yet but I assume
you get desynced keys etc?
If I haven't been detailed enough, I apologise. Here is an in-depth
explanation.
In the event of a renegotiation where there is some form of user/pass in
place, and auth-tokens are not being used, if the verification of the
user/pass fails, for example, they enter an incorrect password and are
not using auth-nocache or are using some form of static-challenge
attached to the password which has expired, or the server has a hiccup
and rejects auth for some reason (the list goes on), the client will
receive no indication that authentication has failed as AUTH_FAILED is
only sent in response to a push_request. The client will appear
connected but traffic will cease until a ping-restart occurs due to a
key desync. This occurs regardless of auth method used, whether it be
plugin, script or management.
In the event an auth-token is used, if the auth-token fails, whether it
be because the auth-token is incorrect or expired, the user will also
receive no indication that it has failed, and again, the client will
appear as if it is in a connected state even though it is not until a
ping-restart occurs.
This is very easy to test, simply setup a basic server using the
included PAM plugin module, set reneg-sec to something short like 10
seconds, set auth-nocache on the client, connect, then on the reneg
enter an incorrect password.
Originally we treated various scenarios as a misconfiguration, however
there are server administrators out there that genuinely don't want to
use auth tokens, and others that genuinely want their users to reauth
manually each period via a renegotiation, so we feel this should be
fixed. We've had this patch available as-is now for over a year and have
had no reports of issues with it for those using it.
Regards,
Eric
---
Eric Thorpe
SparkLabs Developer
https://www.sparklabs.com
https://twitter.com/sparklabs
supp...@sparklabs.com
On 25/08/2020 4:24 pm, Arne Schwabe wrote:
Am 25.08.20 um 01:51 schrieb Eric Thorpe:
Hi Selva,
In multi_connection_estableished, we have
#ifdef MANAGEMENT_DEF_AUTH
if (management)
{
management_connection_established(management,
&mi->context.c2.mda_context,
mi->context.c2.es <http://context.c2.es>);
}
#endif
I do not see why this requires --management-client-auth or any
management related options to be in use.
management is set to NULL unless a management address is defined
(--management), so the above is not called for a plugin or script
bool
open_management(struct context *c)
{
/* initialize management layer */
if (management)
{
if (c->options.management_addr) <----
{
....
{
else
{
close_management(); <----
}
I'm not trying to be resistant in finding another way here to avoid this
extra state you guys don't want, this is a real bug in re-auth/reneg
that we have seen occur multiple times in various setups that we want to
see fixed, and it effects management,
Management goes another code path and management_client_auth directly
calls send_auth_failed.
script and plugin authentication,
however I have had a good walk through the code and I can't see another way.
The other alternative here is to refactor up the call chain so the
context_2 is passed to each of these functions instead of just multi &
session, however this is about a 500 line refactor that I really don't
want to do.
Well the tls session should basically only care about its own session
and the outer logic that ties the tls session together should handle
this. The current code is not as clean as it could be/should be. But I
also haven't digged deep enough to actually understand if your is
actually fixing the problem correctly. We have a working scenario (I
assume) with management and client-auth
- Do we send the AUTH_FAILED over the new or the old tls session?
- Do we establish the data channel (key_method_2_write) before writing
the key?
(and the same for the new approach)
You haven't described the bug/symptons you are seeing yet but I assume
you get desynced keys etc?
I understand that is frustrating from your end since you have a quite
simple fix and fixes your problem but I am here maintaining OpenVPN and
it took me 1-2 weeks to get rid of 2-3 similar variables to the one you
want to introduce.
Arne
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
