[Openvpn-devel] boolean short-circuit auth upon failure

Message ID CAAYFXLmcybkvkw0RJ5gPVhkxrYKRr4XkdPUJwB4Cm--jQjdXKQ@mail.gmail.com
State Superseded
Headers show
Series
  • [Openvpn-devel] boolean short-circuit auth upon failure
Related show

Commit Message

Pete Nelson Nov. 9, 2021, 7:47 p.m.
When evaluating authentication plugins, stop further evaluation
once the first failure is detected.



Implementation notes: Refactoring from a switch-case to an
if-else block allows the break statement to break out of the
outer for loop without additional control variables.  Also,
moving the pr->n setting to within the loop keeps the value
correct if one does break out early.

First email patch submission and first patch submitted to this
list; be gentle please... -- Pete
---
 src/openvpn/plugin.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

Comments

Frank Lichtenheld Nov. 10, 2021, 11:36 a.m. | #1
> Pete Nelson <petiepooo@gmail.com> hat am 09.11.2021 20:47 geschrieben:
> 
> 
> When evaluating authentication plugins, stop further evaluation
> once the first failure is detected.
> 
Since plugin_call is only a thin wrapper around plugin_call_ssl I think this would short-circuit ALL plugin calls. Doesn't sound like you intended that.

Regards,
  Frank

--
  Frank Lichtenheld
<!doctype html>
<html>
 <head> 
  <meta charset="UTF-8"> 
 </head>
 <body>
  <div class="default-style">
   <br>
  </div>
  <blockquote type="cite">
   <div>
    Pete Nelson &lt;petiepooo@gmail.com&gt; hat am 09.11.2021 20:47 geschrieben:
   </div>
   <div>
    <br>
   </div>
   <div>
    <br>
   </div>
   <div dir="ltr">
    When evaluating authentication plugins, stop further evaluation
    <br>once the first failure is detected.
    <br>
   </div>
  </blockquote>
  <div class="io-ox-signature">
   <div class="default-style">
    Since plugin_call is only a thin wrapper around plugin_call_ssl I think this would short-circuit ALL plugin calls. Doesn't sound like you intended that.
    <br>
   </div>
   <div class="default-style">
    <br>
   </div>
   <div class="default-style">
    Regards,
    <br>
   </div>
   <div class="default-style">
    &nbsp; Frank
    <br>
   </div>
   <div class="default-style">
    <br>
   </div>
   <div class="default-style">
    --
   </div>
   <div class="default-style">
    &nbsp; Frank Lichtenheld
   </div>
   <div class="default-style">
    <br>
   </div>
  </div>
 </body>
</html>
Pete Nelson Nov. 11, 2021, 1:23 p.m. | #2
Thank you, Frank.  Amended patch so it applies only to auth attempts.

Not sure how patchwork handles this.. my intent is this amended patch
overwrites the patch from the OP.
-- Pete

---
 src/openvpn/plugin.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index d5704e07..02b17378 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -818,26 +818,24 @@ plugin_call_ssl(const struct plugin_list *pl,
                                                 certdepth,
                                                 current_cert
                                                 );
-            switch (status)
+            if (pr)
             {
-                case OPENVPN_PLUGIN_FUNC_SUCCESS:
-                    break;
-
-                case OPENVPN_PLUGIN_FUNC_DEFERRED:
-                    deferred = true;
-                    break;
-
-                default:
-                    error = true;
+                pr->n = i + 1;
+            }
+            if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)
+            {
+                deferred = true;
+            }
+            else if (status != OPENVPN_PLUGIN_FUNC_SUCCESS)
+            {
+                error = true;
+                if (type == OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY)
+                {
                     break;
+                }
             }
         }

-        if (pr)
-        {
-            pr->n = i;
-        }
-
         gc_free(&gc);

         if (error)
Gert Doering Nov. 11, 2021, 1:34 p.m. | #3
Hi,

On Thu, Nov 11, 2021 at 01:23:43PM +0000, Pete Nelson wrote:
> Thank you, Frank.  Amended patch so it applies only to auth attempts.
> 
> Not sure how patchwork handles this.. my intent is this amended patch
> overwrites the patch from the OP.
> -- Pete

The best way to do that is to send the patch with "git send-email" and
add a "-v2" switch to that - it won't overwrite the patch in PW, but
we can see "ah, it's a v2".

Please add a note to the commit message as well what is new on v2,
like

 v2: only apply shortcut logic to authentication plugin calls

(if that's what is new)

Please do also do "git commit -s", so we get the signed-off-by:
line.

gert
Pete Nelson Nov. 11, 2021, 2:04 p.m. | #4
When evaluating authentication plugins, stop further evaluation
once the first failure is detected.

implementation note: refactoring from a switch-case to an
if-else block allows the break statement to break out of the
outer for loop without additional control variables.

v2: add check for auth plugin before breaking out of loop

Signed-off-by: Peter Nelson <petiepooo@gmail.com>
---
 src/openvpn/plugin.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index d5704e07..02b17378 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -818,26 +818,24 @@ plugin_call_ssl(const struct plugin_list *pl,
                                                 certdepth,
                                                 current_cert
                                                 );
-            switch (status)
+            if (pr)
             {
-                case OPENVPN_PLUGIN_FUNC_SUCCESS:
-                    break;
-
-                case OPENVPN_PLUGIN_FUNC_DEFERRED:
-                    deferred = true;
-                    break;
-
-                default:
-                    error = true;
+                pr->n = i + 1;
+            }
+            if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)
+            {
+                deferred = true;
+            }
+            else if (status != OPENVPN_PLUGIN_FUNC_SUCCESS)
+            {
+                error = true;
+                if (type == OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY)
+                {
                     break;
+                }
             }
         }

-        if (pr)
-        {
-            pr->n = i;
-        }
-
         gc_free(&gc);

         if (error)
Pete Nelson Nov. 11, 2021, 2:26 p.m. | #5
Should that have shown up as a separate entry in patchworks?  I'm having to
cut/paste the results of git format-patch as I can't git send-email from my
dev box, and it looks like I may have missed editing the subject line.
-- Pete

On Thu, Nov 11, 2021 at 1:34 PM Gert Doering <gert@greenie.muc.de> wrote:

> Hi,
>
> On Thu, Nov 11, 2021 at 01:23:43PM +0000, Pete Nelson wrote:
> > Thank you, Frank.  Amended patch so it applies only to auth attempts.
> >
> > Not sure how patchwork handles this.. my intent is this amended patch
> > overwrites the patch from the OP.
> > -- Pete
>
> The best way to do that is to send the patch with "git send-email" and
> add a "-v2" switch to that - it won't overwrite the patch in PW, but
> we can see "ah, it's a v2".
>
> Please add a note to the commit message as well what is new on v2,
> like
>
>  v2: only apply shortcut logic to authentication plugin calls
>
> (if that's what is new)
>
> Please do also do "git commit -s", so we get the signed-off-by:
> line.
>
> gert
> --
> "If was one thing all people took for granted, was conviction that if you
>  feed honest figures into a computer, honest figures come out. Never
> doubted
>  it myself till I met a computer with a sense of humor."
>                              Robert A. Heinlein, The Moon is a Harsh
> Mistress
>
> Gert Doering - Munich, Germany
> gert@greenie.muc.de
>
<div dir="ltr"><div dir="ltr">Should that have shown up as a separate entry in patchworks?  I&#39;m having to cut/paste the results of git format-patch as I can&#39;t git send-email from my dev box, and it looks like I may have missed editing the subject line.</div><div>-- Pete<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Nov 11, 2021 at 1:34 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</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,<br>
<br>
On Thu, Nov 11, 2021 at 01:23:43PM +0000, Pete Nelson wrote:<br>
&gt; Thank you, Frank.  Amended patch so it applies only to auth attempts.<br>
&gt; <br>
&gt; Not sure how patchwork handles this.. my intent is this amended patch<br>
&gt; overwrites the patch from the OP.<br>
&gt; -- Pete<br>
<br>
The best way to do that is to send the patch with &quot;git send-email&quot; and<br>
add a &quot;-v2&quot; switch to that - it won&#39;t overwrite the patch in PW, but<br>
we can see &quot;ah, it&#39;s a v2&quot;.<br>
<br>
Please add a note to the commit message as well what is new on v2,<br>
like<br>
<br>
 v2: only apply shortcut logic to authentication plugin calls<br>
<br>
(if that&#39;s what is new)<br>
<br>
Please do also do &quot;git commit -s&quot;, so we get the signed-off-by:<br>
line.<br>
<br>
gert<br>
-- <br>
&quot;If was one thing all people took for granted, was conviction that if you <br>
 feed honest figures into a computer, honest figures come out. Never doubted <br>
 it myself till I met a computer with a sense of humor.&quot;<br>
                             Robert A. Heinlein, The Moon is a Harsh Mistress<br>
<br>
Gert Doering - Munich, Germany                             <a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a><br>
</blockquote></div></div>
Antonio Quartulli Nov. 11, 2021, 2:34 p.m. | #6
Hi,

On 11/11/2021 15:26, Pete Nelson wrote:
> Should that have shown up as a separate entry in patchworks?  I'm having 
> to cut/paste the results of git format-patch as I can't git send-email 
> from my dev box, and it looks like I may have missed editing the subject 
> line.

It shows up when it is detected as a proper new patch (maybe using the 
exact same subject fooled the detection?).

You can pass -v2 also to git format-patch, so it will edit the subject 
for you.

In any case, I'd suggest fixing git-send-email as copy/paste may 
introduce all kind of formatting bugs and may make the patch unusable.

Cheers,
Gert Doering Nov. 11, 2021, 2:39 p.m. | #7
Hi,

On Thu, Nov 11, 2021 at 03:34:19PM +0100, Antonio Quartulli wrote:
> In any case, I'd suggest fixing git-send-email as copy/paste may 
> introduce all kind of formatting bugs and may make the patch unusable.

Indeed.  Line wraps, tab/space issues, "helpful mail clients", so
git-send-email is really the magic tool.

As a side note, it needs not run on the dev box - so "git format-patch",
copy the result to something that has e-mail, then "git send-email $file"
on that second box - works fine.

("git send-email HEAD" is obviously more convenient, though :-) )

gert

Patch

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index d5704e07..c6c9a63f 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -818,24 +818,19 @@  plugin_call_ssl(const struct plugin_list *pl,
                                                 certdepth,
                                                 current_cert
                                                 );
-            switch (status)
+            if (pr)
             {
-                case OPENVPN_PLUGIN_FUNC_SUCCESS:
-                    break;
-
-                case OPENVPN_PLUGIN_FUNC_DEFERRED:
-                    deferred = true;
-                    break;
-
-                default:
-                    error = true;
-                    break;
+                pr->n = i + 1;
+            }
+            if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)
+            {
+                deferred = true;
+            }
+            else if (status != OPENVPN_PLUGIN_FUNC_SUCCESS)
+            {
+                error = true;
+                break;
             }
-        }
-
-        if (pr)
-        {
-            pr->n = i;
         }

         gc_free(&gc);