Hi Arne,
That code/commit message is explicitly talking about renegotiation. So
if that is also broken, there seems to be something else wrong.
You are quite correct, I've muddled this up with a different issue I've
been working through with Deferred Auth, management should not be a part
of this discussion.
- Do we send the AUTH_FAILED over the new or the old tls session?
The AUTH_FAILED is sent in the old (current) tls session, that is of
course assuming that it is still active at the time of reneg. Normally
if reneg is done with only keys or an auth-token, there isn't the risk
of authentication being too slow. If a user takes too long to enter a
password, or deferred auth is in use, the server can sigterm the user on
it's end before authentication is complete and we have the same result.
With this said though, the configuration can be adjusted to give the
user a grace period on the server, so that specific scenario isn't
something that needs to be handled in my opinion as it can be tuned in
the configuration.
- Do we establish the data channel (key_method_2_write) before writing
the key?
This is the question I specifically ignored as I'm not sure why it's
relevant to plugin/script authentication. key_method_2_write, unless I'm
mistaken (and I'm happy to be corrected here), is client side. The
client knows it's time to renegotiate as part of it's regular wake up
routines or server trigger, of which key_method_2_write is called and
queries for the user/pass (either by management, file, terminal, etc),
and then pushes it all to the server. Unlike the initial connection
process however, a push_request does not follow this information push,
so if authentication fails at the server end, there is no indication of
such and we end up with desync.
understand the implications and whether the patch is really
fixing the root cause or just band aiding the symptoms.
This I can't answer. I don't think it's a band aid, however there are
still scenarios this will not cover as mentioned above. Someone with
more in depth knowledge will need to help review this patch to determine
if this is a fix, or indeed a band aid to a deeper problem that needs to
be reviewed.
With this in mind, I'm afraid I have spent enough time on this. We will
continue to maintain this patch in our fork. In the mean time, we've
tried to do the right thing by submitting a patch for an issue we've
discovered and brought to the mailing lists attention a long time ago
now, and then do the right thing again updating it when requested. I
will leave the conversation here and leave it to the repo maintainers
from here whether they wish to use this patch as is, modify it to their
needs or NAK it for a better future solution. I will keep an eye out for
a new patch and assist in it's testing if someone wishes to improve upon
this.
Cheers,
Eric
---
Eric Thorpe
SparkLabs Developer
https://www.sparklabs.com
https://twitter.com/sparklabs
supp...@sparklabs.com
On 26/08/2020 6:45 pm, Arne Schwabe wrote:
Am 26.08.20 um 03:12 schrieb Eric Thorpe:
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
That code/commit message is explicitly talking about renegotiation. So
if that is also broken, there seems to be something else wrong.
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?
Yeah but I as a said, I currently don't understand it well enough to
understand if your patch is correct or not and if the code path for
management sending AUTH_FAILED on renegotiation is also not working,
there is probably something else broken.
As I said:
- 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?
are questions that I don't have an answer to currently and you also
ignored those questions, so I assume you are not sure either. And I
don't want to merge a patch where neither I nor the author of the patch
really understand the implications and whether the patch is really
fixing the root cause or just band aiding the symptoms.
Arne
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
