[Openvpn-devel,1/2] Send auth-fail messages to clients on renegotiation failures via auth-token or user-pass expiry

Message ID 20200813072923.24385-1-eric@sparklabs.com
State New
Headers show
Series
  • [Openvpn-devel,1/2] Send auth-fail messages to clients on renegotiation failures via auth-token or user-pass expiry
Related show

Commit Message

Eric Thorpe Aug. 13, 2020, 7:29 a.m.
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(+)

Comments

Arne Schwabe Aug. 13, 2020, 8:30 a.m. | #1
>  /*
>   * 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
Eric Thorpe Aug. 13, 2020, 8:36 a.m. | #2
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
>
Selva Nair Aug. 23, 2020, 2:13 a.m. | #3
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 &lt;<a href="mailto:eric@sparklabs.com">eric@sparklabs.com</a>&gt; 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&#39;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-&gt;opt-&gt;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>
Eric Thorpe Aug. 24, 2020, 7:49 a.m. | #4
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-&gt;opt-&gt;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 &lt;<a href="mailto:eric@sparklabs.com"
              moz-do-not-send="true">eric@sparklabs.com</a>&gt; 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-&gt;opt-&gt;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>
Selva Nair Aug. 24, 2020, 2:25 p.m. | #5
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 &lt;<a href="mailto:eric@sparklabs.com">eric@sparklabs.com</a>&gt; 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-&gt;opt-&gt;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&#39;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>                                          &amp;mi-&gt;context.c2.mda_context, mi-&gt;<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-&gt;flags and that&#39;s what&#39;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 &lt;<a href="mailto:eric@sparklabs.com" target="_blank">eric@sparklabs.com</a>&gt; 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&#39;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-&gt;opt-&gt;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>
Eric Thorpe Aug. 24, 2020, 11:51 p.m. | #6
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>
                                                 
        &amp;mi-&gt;context.c2.mda_context, mi-&gt;<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-&gt;options.management_addr) &lt;----<br>
              {<br>
               ....<br>
              {<br>
              else<br>
              {<br>
                  close_management(); &lt;----<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 &amp; 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 &lt;<a href="mailto:eric@sparklabs.com"
              moz-do-not-send="true">eric@sparklabs.com</a>&gt; 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-&gt;opt-&gt;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>
                                                     
            &amp;mi-&gt;context.c2.mda_context, mi-&gt;<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-&gt;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 &lt;<a
                        href="mailto:eric@sparklabs.com" target="_blank"
                        moz-do-not-send="true">eric@sparklabs.com</a>&gt;
                      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-&gt;opt-&gt;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>
Arne Schwabe Aug. 25, 2020, 6:24 a.m. | #7
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
Eric Thorpe Aug. 26, 2020, 1:12 a.m. | #8
> 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
>
Arne Schwabe Aug. 26, 2020, 8:45 a.m. | #9
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
Eric Thorpe Aug. 26, 2020, 11:57 p.m. | #10
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
>
Arne Schwabe Aug. 27, 2020, 8:46 a.m. | #11
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

Patch

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");
     }