diff mbox

[Openvpn-devel,0/2] Reject client if PF plugin is configured, but init fails

Message ID 20170929162458.16514-1-steffan@karger.me
State Superseded
Headers

Commit Message

Steffan Karger Sept. 29, 2017, 6:24 a.m. UTC 
  This changes the behavior for pf plugins: instead of just not initializing
the firewall rules and happily continuing, this now rejects the client in
the case of an (unlikely) failure to initialize the pf.

Signed-off-by: Steffan Karger <steffan@karger.me>
---
 src/openvpn/pf.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Antonio Quartulli Sept. 29, 2017, 3 p.m. UTC | #1 
On 30/09/17 00:24, Steffan Karger wrote:
> This changes the behavior for pf plugins: instead of just not initializing
> the firewall rules and happily continuing, this now rejects the client in
> the case of an (unlikely) failure to initialize the pf.
> 
> Signed-off-by: Steffan Karger <steffan@karger.me>
> ---
>  src/openvpn/pf.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
> index 5cb002bf..29231b67 100644
> --- a/src/openvpn/pf.c
> +++ b/src/openvpn/pf.c
> @@ -639,10 +639,11 @@ pf_init_context(struct context *c)
>                  }
>  #endif
>              }
> -            else
> -            {
> -                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
> -            }
> +        }
> +        if (!c->c2.pf.enabled)
> +        {
> +            msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client.");
> +            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
>          }

after this part there is some code dealing with the management
interface: wouldn't it be better to have a return after registering the
signal so that we don't attempt any interaction with the management
interface? If the plugin initialization failed, we should exit right
away no?

(although this PF thing with the plugin being optional makes everything
more complicated every time :/)

Another thought: if we abort every time we can't initialize the pf
context, do you think we need the c->c2.pf.enabled member at all?

It looks to me that after this patch if PF is enabled, either a client
is connected and has a PF context, or its connection was rejected
entirely. Thus a overall state should be enough rather than a per-client
one. (Can probably be adjusted with another patch if you don't feel like
doing all this in here?)

Cheers,

>      }
>  #endif /* ifdef PLUGIN_PF */
>
Steffan Karger Nov. 1, 2017, 12:26 p.m. UTC | #2 
Hi,

On 30-09-17 03:00, Antonio Quartulli wrote:
> On 30/09/17 00:24, Steffan Karger wrote:
>> This changes the behavior for pf plugins: instead of just not initializing
>> the firewall rules and happily continuing, this now rejects the client in
>> the case of an (unlikely) failure to initialize the pf.
>>
>> Signed-off-by: Steffan Karger <steffan@karger.me>
>> ---
>>  src/openvpn/pf.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
>> index 5cb002bf..29231b67 100644
>> --- a/src/openvpn/pf.c
>> +++ b/src/openvpn/pf.c
>> @@ -639,10 +639,11 @@ pf_init_context(struct context *c)
>>                  }
>>  #endif
>>              }
>> -            else
>> -            {
>> -                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
>> -            }
>> +        }
>> +        if (!c->c2.pf.enabled)
>> +        {
>> +            msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client.");
>> +            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
>>          }
> 
> after this part there is some code dealing with the management
> interface: wouldn't it be better to have a return after registering the
> signal so that we don't attempt any interaction with the management
> interface? If the plugin initialization failed, we should exit right
> away no?
> 
> (although this PF thing with the plugin being optional makes everything
> more complicated every time :/)

Good point, I'll fix that and send a whole new patch set with all your
comments processed.  With the expansion from 2 to 4 patches, the thread
has become quite a mess.

> Another thought: if we abort every time we can't initialize the pf
> context, do you think we need the c->c2.pf.enabled member at all?
> 
> It looks to me that after this patch if PF is enabled, either a client
> is connected and has a PF context, or its connection was rejected
> entirely. Thus a overall state should be enough rather than a per-client
> one. (Can probably be adjusted with another patch if you don't feel like
> doing all this in here?)

The pf.enabled member is used in a number of quick inline checks to
determine whether to go into the whole pf code path or not.  I think
it's best to keep it.

-Steffan

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
diff mbox

Patch

diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
index 5cb002bf..29231b67 100644
--- a/src/openvpn/pf.c
+++ b/src/openvpn/pf.c
@@ -639,10 +639,11 @@  pf_init_context(struct context *c)
                 }
 #endif
             }
-            else
-            {
-                msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
-            }
+        }
+        if (!c->c2.pf.enabled)
+        {
+            msg(M_WARN, "WARNING: failed to init PF plugin, rejecting client.");
+            register_signal(c, SIGUSR1, "plugin-pf-init-failed");
         }
     }
 #endif /* ifdef PLUGIN_PF */