| Message ID | 20170929162458.16514-1-steffan@karger.me |
|---|---|
| State | Superseded |
| Headers | show |
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 */ >
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 --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 */
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(-)