Hi²,
On 26/09/17 00:05, Steffan Karger wrote:
> Hi,
>
> On 21-09-17 19:15, Antonio Quartulli wrote:
>> On 15/09/17 05:34, Steffan Karger wrote:
>>> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
>>> index 5cb002b..5fe1734 100644
>>> --- a/src/openvpn/pf.c
>>> +++ b/src/openvpn/pf.c
>>> @@ -639,10 +639,10 @@ pf_init_context(struct context *c)
>>> }
>>> #endif
>>> }
>>> - else
>>> - {
>>> - msg(M_WARN, "WARNING: OPENVPN_PLUGIN_ENABLE_PF disabled");
>>> - }
>>
>> Why removing this chunk above? Isn't that supposed to be printed when
>> the init function of the plugin fails?
>
> Because it's no longer true; instead of disabling the firewall, we will
> now reject the connection. I tried to explain that in the commit message:
>
>> On 15/09/17 05:34, Steffan Karger wrote:
>>> Note that 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.
>
> I think rejecting a client is the sensible thing to do when initializing
> the firewall failed. (Rather than accepting the client and not
> enforcing the firewall rules.)
Right.
This makes sense, but don't you think this is worth its own commit
instead of hiding a behavioural change like this behind "Don't throw
fatal errors from create_temp_file()" ? :-)
It could be done in a patch preceding this one, imho.
>
>>> + }
>>> + if (!c->c2.pf.enabled)
>>> + {
>>> + register_signal(c, SIGUSR1, "plugin-pf-init-failed");
>>> }
>>
>> Maybe it's just me missing how the signaling part really works, but what
>> happens on the server when the SIGUSR1 is delivered? And why registering
>> the signal when PF is disabled? I am missing something I think...
>
> This registers a SIGUSR1 on the instance context, not the top-level
> context. This leads to an instance reset. See e.g. check_tls_errors(),
> which does the same in case of an error (or even uses SIGTERM, if
> --tls-exit is used).
>
> Registering a signal is how error reporting is this part of the code works.
I see, we basically use signals as they were flags. Thanks for
clarifying that. I can see this is handled in init_instance() where we
check for a signal and possibly abort the initialization of the child
context. I think it makes sense and as you are saying this is similar to
other error handling procedures.
Cheers,
>
>>> }
>>> #endif /* ifdef PLUGIN_PF */
>>> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
>>> index 9cd36d7..df2736c 100644
>>> --- a/src/openvpn/ssl_verify.c
>>> +++ b/src/openvpn/ssl_verify.c
>>> @@ -547,14 +547,14 @@ verify_cert_export_cert(openvpn_x509_cert_t
>>> *peercert, const char *tmp_dir, stru
>>> FILE *peercert_file;
>>> const char *peercert_filename = "";
>>>
>>> - if (!tmp_dir)
>>> + /* create tmp file to store peer cert */
>>> + if (!tmp_dir ||
>>> + !(peercert_filename = create_temp_file(tmp_dir, "pcf", gc)))
>>
>> || at the beginning of the next line (as mentioned in my previous email)
>
> Will do, once we agree on the above discussion points.
>
> Hope this clarifies things.
>
> -Steffan
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
