| Message ID | 20200813072923.24385-1-eric@sparklabs.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [Openvpn-devel,1/2] Send auth-fail messages to clients on renegotiation failures via auth-token or user-pass expiry | expand |
> /* > * Send restart message from server to client. > */ > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index 96897e48..b5cc9dc9 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -576,6 +576,7 @@ struct tls_multi > > char *remote_ciphername; /**< cipher specified in peer's config file */ > > + bool connection_established; /** Notifies future auth calls this is a reneg */ > /* > * Our session objects. > */ > NAK from my side. Adding another variable to state machine for just this feature that duplicates already existing states is something I really want to avoid. This might look and work fine for this patch but at the end you end up with a plenthora of mini states and unclear interdependency. I worked hard in the connect-client patches to remove duplication of these states and are not eager to reduce them, especially not "connection_established" which duplicates the name of link_socket->connection_established that has a completely different meaning. Arne
Hi Arne, The issue is your state is not accessible from where that boolean needs to be used unless I am missing something? Please advise if I'm mistaken or of another route. Eric --- Eric Thorpe SparkLabs Developer https://www.sparklabs.com https://twitter.com/sparklabs support@sparklabs.com On 13/08/2020 6:30 pm, Arne Schwabe wrote: >> /* >> * Send restart message from server to client. >> */ >> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h >> index 96897e48..b5cc9dc9 100644 >> --- a/src/openvpn/ssl_common.h >> +++ b/src/openvpn/ssl_common.h >> @@ -576,6 +576,7 @@ struct tls_multi >> >> char *remote_ciphername; /**< cipher specified in peer's config file */ >> >> + bool connection_established; /** Notifies future auth calls this is a reneg */ >> /* >> * Our session objects. >> */ >> > NAK from my side. Adding another variable to state machine for just this > feature that duplicates already existing states is something I really > want to avoid. This might look and work fine for this patch but at the > end you end up with a plenthora of mini states and unclear > interdependency. I worked hard in the connect-client patches to remove > duplication of these states and are not eager to reduce them, especially > not "connection_established" which duplicates the name of > link_socket->connection_established that has a completely different meaning. > > Arne >
Hi, On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <eric@sparklabs.com> wrote: > Hi Arne, > > The issue is your state is not accessible from where that boolean needs > to be used unless I am missing something? Please advise if I'm mistaken > or of another route. > I agree with Arne that duplicating a state machine variable is not a good approach. But we have to somehow get the REAUTH (reneg) info in here. This has stalled for too long, so my suggestion would be to make this conditional on MANAGEMNET_DEF_AUTH so that we can then get it from session->opt->mda_context just as we do it when auth is done via the management. In practice, that would cover most builds where this is really useful. In fact, I think we should always enable MANAGEMENT_DEF_AUTH when management is enabled. That also gets rid of a lot of IFDEFs and allow the use of useful bits like CID more widely in the code. I see no compelling reason for such fine-grained build options. A marginal increase in code size is of little consequence all but embedded devices which can continue to cope without this as they do now. Selva <div dir="ltr"><div dir="ltr">Hi,<div><br></div></div><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com">eric@sparklabs.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Arne,<br> <br> The issue is your state is not accessible from where that boolean needs <br> to be used unless I am missing something? Please advise if I'm mistaken <br> or of another route.<br></blockquote><div><br></div><div>I agree with Arne that duplicating a state machine variable is </div><div>not a good approach. But we have to somehow get the REAUTH (reneg) </div><div>info in here.</div><div><br></div><div>This has stalled for too long, so my suggestion would be to make</div><div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div><div>then get it from session->opt->mda_context just as we do it when </div><div>auth is done via the management. In practice, that would cover</div><div>most builds where this is really useful.</div><div><br></div><div>In fact, I think we should always enable MANAGEMENT_DEF_AUTH</div><div>when management is enabled. That also gets rid of a lot of IFDEFs and</div><div>allow the use of useful bits like CID more widely in the code. I see</div><div>no compelling reason for such fine-grained build options.</div><div><br></div><div>A marginal increase in code size is of little consequence all but</div><div>embedded devices which can continue to cope without this</div><div>as they do now.</div><div><br></div><div>Selva</div></div></div>
Hi Selva, > my suggestion would be to make > this conditional on MANAGEMNET_DEF_AUTH so that we can > then get it from session->opt->mda_context just as we do it when > auth is done via the management. In practice, that would cover > most builds where this is really useful. Unfortunately this doesn't help. The mda_context flags are only set when --management-client-auth is in use, meaning this patch would not cover plugin or script authentication, which are the more commonly used, and this patch set specifically addresses plugin authentication. Regards, Eric --- Eric Thorpe SparkLabs Developer https://www.sparklabs.com https://twitter.com/sparklabs support@sparklabs.com On 23/08/2020 12:13 pm, Selva Nair wrote: > Hi, > > On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <eric@sparklabs.com > <mailto:eric@sparklabs.com>> wrote: > > Hi Arne, > > The issue is your state is not accessible from where that boolean > needs > to be used unless I am missing something? Please advise if I'm > mistaken > or of another route. > > > I agree with Arne that duplicating a state machine variable is > not a good approach. But we have to somehow get the REAUTH (reneg) > info in here. > > This has stalled for too long, so my suggestion would be to make > this conditional on MANAGEMNET_DEF_AUTH so that we can > then get it from session->opt->mda_context just as we do it when > auth is done via the management. In practice, that would cover > most builds where this is really useful. > > In fact, I think we should always enable MANAGEMENT_DEF_AUTH > when management is enabled. That also gets rid of a lot of IFDEFs and > allow the use of useful bits like CID more widely in the code. I see > no compelling reason for such fine-grained build options. > > A marginal increase in code size is of little consequence all but > embedded devices which can continue to cope without this > as they do now. > > Selva <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <p>Hi Selva,</p> <p> <blockquote type="cite">my suggestion would be to make <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> </blockquote> Unfortunately this doesn't help. The mda_context flags are only set when --management-client-auth is in use, meaning this patch would not cover plugin or script authentication, which are the more commonly used, and this patch set specifically addresses plugin authentication.<br> </p> <p>Regards,<br> Eric<br> </p> <pre class="moz-signature" cols="72">--- Eric Thorpe SparkLabs Developer <a class="moz-txt-link-freetext" href="https://www.sparklabs.com">https://www.sparklabs.com</a> <a class="moz-txt-link-freetext" href="https://twitter.com/sparklabs">https://twitter.com/sparklabs</a> <a class="moz-txt-link-abbreviated" href="mailto:support@sparklabs.com">support@sparklabs.com</a></pre> <div class="moz-cite-prefix">On 23/08/2020 12:13 pm, Selva Nair wrote:<br> </div> <blockquote type="cite" cite="mid:CAKuzo_h_xWf-Jbkow=H3sL_CQw5CC23pw3DxH0Nsydj2f57usg@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div dir="ltr"> <div dir="ltr">Hi, <div><br> </div> </div> <div class="gmail_quote"> <div dir="ltr" class="gmail_attr">On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com" moz-do-not-send="true">eric@sparklabs.com</a>> wrote:<br> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Arne,<br> <br> The issue is your state is not accessible from where that boolean needs <br> to be used unless I am missing something? Please advise if I'm mistaken <br> or of another route.<br> </blockquote> <div><br> </div> <div>I agree with Arne that duplicating a state machine variable is </div> <div>not a good approach. But we have to somehow get the REAUTH (reneg) </div> <div>info in here.</div> <div><br> </div> <div>This has stalled for too long, so my suggestion would be to make</div> <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> <div><br> </div> <div>In fact, I think we should always enable MANAGEMENT_DEF_AUTH</div> <div>when management is enabled. That also gets rid of a lot of IFDEFs and</div> <div>allow the use of useful bits like CID more widely in the code. I see</div> <div>no compelling reason for such fine-grained build options.</div> <div><br> </div> <div>A marginal increase in code size is of little consequence all but</div> <div>embedded devices which can continue to cope without this</div> <div>as they do now.</div> <div><br> </div> <div>Selva</div> </div> </div> </blockquote> </body> </html>
On Mon, Aug 24, 2020 at 3:49 AM Eric Thorpe <eric@sparklabs.com> wrote: > Hi Selva, > > my suggestion would be to make > this conditional on MANAGEMNET_DEF_AUTH so that we can > then get it from session->opt->mda_context just as we do it when > auth is done via the management. In practice, that would cover > most builds where this is really useful. > > Unfortunately this doesn't help. The mda_context flags are only set when > --management-client-auth is in use, meaning this patch would not cover > plugin or script authentication, which are the more commonly used, and this > patch set specifically addresses plugin authentication. > Are you sure of that? 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); } #endif management_connection_established will set DAF_CONENCTION_ESATBLISHED in mdac->flags and that's what's used to determine REAUTH. I do not see why this requires --management-client-auth or any management related options to be in use. Sure, management support and deferred auth support have to be enabled but restricting the usefulness of your patch to those cases is not really a limitation. What am I missing? Selva --- > Eric Thorpe > SparkLabs Developerhttps://www.sparklabs.comhttps://twitter.com/sparklabssupport@sparklabs.com > > On 23/08/2020 12:13 pm, Selva Nair wrote: > > Hi, > > On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <eric@sparklabs.com> wrote: > >> Hi Arne, >> >> The issue is your state is not accessible from where that boolean needs >> to be used unless I am missing something? Please advise if I'm mistaken >> or of another route. >> > > I agree with Arne that duplicating a state machine variable is > not a good approach. But we have to somehow get the REAUTH (reneg) > info in here. > > This has stalled for too long, so my suggestion would be to make > this conditional on MANAGEMNET_DEF_AUTH so that we can > then get it from session->opt->mda_context just as we do it when > auth is done via the management. In practice, that would cover > most builds where this is really useful. > > In fact, I think we should always enable MANAGEMENT_DEF_AUTH > when management is enabled. That also gets rid of a lot of IFDEFs and > allow the use of useful bits like CID more widely in the code. I see > no compelling reason for such fine-grained build options. > > A marginal increase in code size is of little consequence all but > embedded devices which can continue to cope without this > as they do now. > > Selva > > <div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Aug 24, 2020 at 3:49 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com">eric@sparklabs.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div> <p>Hi Selva,</p> <p> </p><blockquote type="cite">my suggestion would be to make <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> </blockquote> Unfortunately this doesn't help. The mda_context flags are only set when --management-client-auth is in use, meaning this patch would not cover plugin or script authentication, which are the more commonly used, and this patch set specifically addresses plugin authentication.<br></div></blockquote><div><br></div><div>Are you sure of that? </div><div><br></div><div>In multi_connection_estableished, we have</div><div><br></div><div>#ifdef MANAGEMENT_DEF_AUTH<br> if (management)<br> {<br> management_connection_established(management,<br> &mi->context.c2.mda_context, mi-><a href="http://context.c2.es">context.c2.es</a>);<br> }<br>#endif<br></div><div><br></div><div>management_connection_established will set DAF_CONENCTION_ESATBLISHED in mdac->flags and that's what's used to determine REAUTH. </div><div><br></div><div>I do not see why this requires --management-client-auth or any management related options to be in use. Sure, management support and deferred auth support have to be enabled but restricting the usefulness of your patch to those cases is not really a limitation.</div><div><br></div><div>What am I missing?</div><div><br></div><div>Selva</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div> <pre cols="72">--- Eric Thorpe SparkLabs Developer <a href="https://www.sparklabs.com" target="_blank">https://www.sparklabs.com</a> <a href="https://twitter.com/sparklabs" target="_blank">https://twitter.com/sparklabs</a> <a href="mailto:support@sparklabs.com" target="_blank">support@sparklabs.com</a></pre> <div>On 23/08/2020 12:13 pm, Selva Nair wrote:<br> </div> <blockquote type="cite"> <div dir="ltr"> <div dir="ltr">Hi, <div><br> </div> </div> <div class="gmail_quote"> <div dir="ltr" class="gmail_attr">On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com" target="_blank">eric@sparklabs.com</a>> wrote:<br> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Arne,<br> <br> The issue is your state is not accessible from where that boolean needs <br> to be used unless I am missing something? Please advise if I'm mistaken <br> or of another route.<br> </blockquote> <div><br> </div> <div>I agree with Arne that duplicating a state machine variable is </div> <div>not a good approach. But we have to somehow get the REAUTH (reneg) </div> <div>info in here.</div> <div><br> </div> <div>This has stalled for too long, so my suggestion would be to make</div> <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> <div><br> </div> <div>In fact, I think we should always enable MANAGEMENT_DEF_AUTH</div> <div>when management is enabled. That also gets rid of a lot of IFDEFs and</div> <div>allow the use of useful bits like CID more widely in the code. I see</div> <div>no compelling reason for such fine-grained build options.</div> <div><br> </div> <div>A marginal increase in code size is of little consequence all but</div> <div>embedded devices which can continue to cope without this</div> <div>as they do now.</div> <div><br> </div> <div>Selva</div> </div> </div> </blockquote> </div> </blockquote></div></div>
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, 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. Cheers, Eric --- Eric Thorpe SparkLabs Developer https://www.sparklabs.com https://twitter.com/sparklabs support@sparklabs.com On 25/08/2020 12:25 am, Selva Nair wrote: > > > On Mon, Aug 24, 2020 at 3:49 AM Eric Thorpe <eric@sparklabs.com > <mailto:eric@sparklabs.com>> wrote: > > Hi Selva, > >> my suggestion would be to make >> this conditional on MANAGEMNET_DEF_AUTH so that we can >> then get it from session->opt->mda_context just as we do it when >> auth is done via the management. In practice, that would cover >> most builds where this is really useful. > Unfortunately this doesn't help. The mda_context flags are only > set when --management-client-auth is in use, meaning this patch > would not cover plugin or script authentication, which are the > more commonly used, and this patch set specifically addresses > plugin authentication. > > > Are you sure of that? > > 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 > > management_connection_established will set DAF_CONENCTION_ESATBLISHED > in mdac->flags and that's what's used to determine REAUTH. > > I do not see why this requires --management-client-auth or any > management related options to be in use. Sure, management support and > deferred auth support have to be enabled but restricting the > usefulness of your patch to those cases is not really a limitation. > > What am I missing? > > Selva > > --- > Eric Thorpe > SparkLabs Developer > https://www.sparklabs.com <https://www.sparklabs.com> > https://twitter.com/sparklabs <https://twitter.com/sparklabs> > support@sparklabs.com <mailto:support@sparklabs.com> > > On 23/08/2020 12:13 pm, Selva Nair wrote: >> Hi, >> >> On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <eric@sparklabs.com >> <mailto:eric@sparklabs.com>> wrote: >> >> Hi Arne, >> >> The issue is your state is not accessible from where that >> boolean needs >> to be used unless I am missing something? Please advise if >> I'm mistaken >> or of another route. >> >> >> I agree with Arne that duplicating a state machine variable is >> not a good approach. But we have to somehow get the REAUTH (reneg) >> info in here. >> >> This has stalled for too long, so my suggestion would be to make >> this conditional on MANAGEMNET_DEF_AUTH so that we can >> then get it from session->opt->mda_context just as we do it when >> auth is done via the management. In practice, that would cover >> most builds where this is really useful. >> >> In fact, I think we should always enable MANAGEMENT_DEF_AUTH >> when management is enabled. That also gets rid of a lot of IFDEFs and >> allow the use of useful bits like CID more widely in the code. I see >> no compelling reason for such fine-grained build options. >> >> A marginal increase in code size is of little consequence all but >> embedded devices which can continue to cope without this >> as they do now. >> >> Selva > <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <p>Hi Selva,</p> <p> <blockquote type="cite"> <div>In multi_connection_estableished, we have</div> <div><br> </div> #ifdef MANAGEMENT_DEF_AUTH<br> if (management)<br> {<br> management_connection_established(management,<br> &mi->context.c2.mda_context, mi-><a href="http://context.c2.es">context.c2.es</a>);<br> }<br> #endif</blockquote> <blockquote type="cite">I do not see why this requires --management-client-auth or any management related options to be in use. </blockquote> <br> management is set to NULL unless a management address is defined (--management), so the above is not called for a plugin or script<br> </p> <p>bool<br> open_management(struct context *c)<br> {<br> /* initialize management layer */<br> if (management)<br> {<br> if (c->options.management_addr) <----<br> {<br> ....<br> {<br> else<br> {<br> close_management(); <----<br> }</p> <p>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, script and plugin authentication, however I have had a good walk through the code and I can't see another way.</p> <p>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.</p> <p>Cheers,<br> Eric<br> </p> <pre class="moz-signature" cols="72">--- Eric Thorpe SparkLabs Developer <a class="moz-txt-link-freetext" href="https://www.sparklabs.com">https://www.sparklabs.com</a> <a class="moz-txt-link-freetext" href="https://twitter.com/sparklabs">https://twitter.com/sparklabs</a> <a class="moz-txt-link-abbreviated" href="mailto:support@sparklabs.com">support@sparklabs.com</a></pre> <div class="moz-cite-prefix">On 25/08/2020 12:25 am, Selva Nair wrote:<br> </div> <blockquote type="cite" cite="mid:CAKuzo_heHMcpRd31Lc=ZHDjPAb3GsQoBQB4iQvXCd7EPTnGEug@mail.gmail.com"> <meta http-equiv="content-type" content="text/html; charset=UTF-8"> <div dir="ltr"> <div dir="ltr"><br> </div> <br> <div class="gmail_quote"> <div dir="ltr" class="gmail_attr">On Mon, Aug 24, 2020 at 3:49 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com" moz-do-not-send="true">eric@sparklabs.com</a>> wrote:<br> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div> <p>Hi Selva,</p> <p> </p> <blockquote type="cite">my suggestion would be to make <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> </blockquote> Unfortunately this doesn't help. The mda_context flags are only set when --management-client-auth is in use, meaning this patch would not cover plugin or script authentication, which are the more commonly used, and this patch set specifically addresses plugin authentication.<br> </div> </blockquote> <div><br> </div> <div>Are you sure of that? </div> <div><br> </div> <div>In multi_connection_estableished, we have</div> <div><br> </div> <div>#ifdef MANAGEMENT_DEF_AUTH<br> if (management)<br> {<br> management_connection_established(management,<br> &mi->context.c2.mda_context, mi-><a href="http://context.c2.es" moz-do-not-send="true">context.c2.es</a>);<br> }<br> #endif<br> </div> <div><br> </div> <div>management_connection_established will set DAF_CONENCTION_ESATBLISHED in mdac->flags and that's what's used to determine REAUTH. </div> <div><br> </div> <div>I do not see why this requires --management-client-auth or any management related options to be in use. Sure, management support and deferred auth support have to be enabled but restricting the usefulness of your patch to those cases is not really a limitation.</div> <div><br> </div> <div>What am I missing?</div> <div><br> </div> <div>Selva</div> <div><br> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <div> <pre cols="72">--- Eric Thorpe SparkLabs Developer <a href="https://www.sparklabs.com" target="_blank" moz-do-not-send="true">https://www.sparklabs.com</a> <a href="https://twitter.com/sparklabs" target="_blank" moz-do-not-send="true">https://twitter.com/sparklabs</a> <a href="mailto:support@sparklabs.com" target="_blank" moz-do-not-send="true">support@sparklabs.com</a></pre> <div>On 23/08/2020 12:13 pm, Selva Nair wrote:<br> </div> <blockquote type="cite"> <div dir="ltr"> <div dir="ltr">Hi, <div><br> </div> </div> <div class="gmail_quote"> <div dir="ltr" class="gmail_attr">On Thu, Aug 13, 2020 at 4:37 AM Eric Thorpe <<a href="mailto:eric@sparklabs.com" target="_blank" moz-do-not-send="true">eric@sparklabs.com</a>> wrote:<br> </div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Arne,<br> <br> The issue is your state is not accessible from where that boolean needs <br> to be used unless I am missing something? Please advise if I'm mistaken <br> or of another route.<br> </blockquote> <div><br> </div> <div>I agree with Arne that duplicating a state machine variable is </div> <div>not a good approach. But we have to somehow get the REAUTH (reneg) </div> <div>info in here.</div> <div><br> </div> <div>This has stalled for too long, so my suggestion would be to make</div> <div>this conditional on MANAGEMNET_DEF_AUTH so that we can</div> <div>then get it from session->opt->mda_context just as we do it when </div> <div>auth is done via the management. In practice, that would cover</div> <div>most builds where this is really useful.</div> <div><br> </div> <div>In fact, I think we should always enable MANAGEMENT_DEF_AUTH</div> <div>when management is enabled. That also gets rid of a lot of IFDEFs and</div> <div>allow the use of useful bits like CID more widely in the code. I see</div> <div>no compelling reason for such fine-grained build options.</div> <div><br> </div> <div>A marginal increase in code size is of little consequence all but</div> <div>embedded devices which can continue to cope without this</div> <div>as they do now.</div> <div><br> </div> <div>Selva</div> </div> </div> </blockquote> </div> </blockquote> </div> </div> </blockquote> </body> </html>
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
> 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 support@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 >
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
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 support@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 >
Am 27.08.20 um 01:57 schrieb Eric Thorpe: > 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), client starts with key_method2_write, then server does _read and then _write again and after that client does _read. See also (https://github.com/OpenVPN/openvpn/blob/master/doc/doxygen/doc_key_generation.h) After the write2 from the server and read2 from client is done, the client can assume that the keys from that TLS session are valid. > 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. Yeah, that sounds about right. >> 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. I will put this on my TODO list. Maybe I can also cleanup the code to make this a litte bit less of a mess. Arne
> /* > * Send restart message from server to client. > */ > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index 96897e48..b5cc9dc9 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -576,6 +576,7 @@ struct tls_multi > > char *remote_ciphername; /**< cipher specified in peer's config file */ > > + bool connection_established; /** Notifies future auth calls this is a reneg */ > /* > * Our session objects. > */ > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index 97ccb93b..8d8531c7 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -1318,6 +1318,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > } > else > { > + send_push_reply_auth_failed(multi, "SESSION:Auth-token expired"); > wipe_auth_token(multi); > ks->authenticated = KS_AUTH_FALSE; > msg(M_WARN, "TLS: Username/auth-token authentication " > @@ -1432,6 +1433,12 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > } > else > { > + if (multi->connection_established) > + { > + /* Notify the client */ > + send_push_reply_auth_failed(multi, "SESSION:Auth failed"); > + > + } > ks->authenticated = KS_AUTH_FALSE; > msg(D_TLS_ERRORS, "TLS Auth Error: Auth Username/Password verification failed for peer"); > } > Here comes a late NACK to the patch. I am currently looking into this problem in detail and this patch fails for deferred authentication. The patch will only send the AUTH_FAILED message if the result of the user/password authentication is already known in verify_user_pass. If the it is deferred with this patch does not send the AUTH_FAILED. Arne
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 13738180..288680c9 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2689,6 +2689,8 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi) mi->context.c2.context_auth = CAS_FAILED; } + /* Set connection established for reneg handling */ + mi->context.c2.tls_multi->connection_established = true; /* increment number of current authenticated clients */ ++m->n_clients; diff --git a/src/openvpn/push.c b/src/openvpn/push.c index e0d2eeaf..3567b22d 100644 --- a/src/openvpn/push.c +++ b/src/openvpn/push.c @@ -311,6 +311,36 @@ send_auth_pending_messages(struct context *c, const char *extra) return true; } +/* +* Send auth failed message from server to client without scheduling. +* Main use for queuing a message during renegotiation +*/ +void +send_push_reply_auth_failed(struct tls_multi *multi, const char *client_reason) +{ + struct gc_arena gc = gc_new(); + static const char auth_failed[] = "AUTH_FAILED"; + size_t len; + + len = (client_reason ? strlen(client_reason)+1 : 0) + sizeof(auth_failed); + if (len > PUSH_BUNDLE_SIZE) + { + len = PUSH_BUNDLE_SIZE; + } + + { + struct buffer buf = alloc_buf_gc(len, &gc); + buf_printf(&buf, auth_failed); + if (client_reason) + { + buf_printf(&buf, ",%s", client_reason); + } + send_control_channel_string_dowork(multi, BSTR(&buf), D_PUSH); + } + + gc_free(&gc); +} + /* * Send restart message from server to client. */ diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index 96897e48..b5cc9dc9 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -576,6 +576,7 @@ struct tls_multi char *remote_ciphername; /**< cipher specified in peer's config file */ + bool connection_established; /** Notifies future auth calls this is a reneg */ /* * Our session objects. */ diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 97ccb93b..8d8531c7 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1318,6 +1318,7 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, } else { + send_push_reply_auth_failed(multi, "SESSION:Auth-token expired"); wipe_auth_token(multi); ks->authenticated = KS_AUTH_FALSE; msg(M_WARN, "TLS: Username/auth-token authentication " @@ -1432,6 +1433,12 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, } else { + if (multi->connection_established) + { + /* Notify the client */ + send_push_reply_auth_failed(multi, "SESSION:Auth failed"); + + } ks->authenticated = KS_AUTH_FALSE; msg(D_TLS_ERRORS, "TLS Auth Error: Auth Username/Password verification failed for peer"); }
Signed-off-by: Eric Thorpe <eric@sparklabs.com> --- src/openvpn/multi.c | 2 ++ src/openvpn/push.c | 30 ++++++++++++++++++++++++++++++ src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_verify.c | 7 +++++++ 4 files changed, 40 insertions(+)