Hi, On Fri, Jan 22, 2021 at 08:02:42AM +0100, Arne Schwabe wrote: > I agree to make this "fixed" in a way that doesn't involve refactoring > of pf code that is later removed anyway. I don't think the refactoring > early affects this. It has been probably broken even earlier.
So I got myself nerdsniped into understanding when and how this broke.
Tested v2.4.10, and that works nicely when ENABLE_PF fails - it logs a
warning, and that's it. No instance restart, no crash.
The instance restart is introduced here:
commit 492e42d35f141346fe21b3e984ed1bd86e5aac40
Author: Steffan Karger <stef...@karger.me>
Date: Wed Nov 1 23:03:40 2017 +0100
pf: reject client if PF plugin is configured, but init fails
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.
... which, back then, actually worked. Sort of - it aborts the client
connection, but since it does so very early (before TLS_FINAL), the client
is never told, and times out eventually.
Sat Jan 23 12:45:48 2021 us=726018 2001:608:0:814::f000:21 PLUGIN_CALL: plugin
function PLUGIN_ENABLE_PF failed with status 1:
/home/gert/openvpn-pftest.git/sample/sample-plugins/defer/simple.so
Sat Jan 23 12:45:48 2021 us=726033 2001:608:0:814::f000:21 WARNING: failed to
init PF plugin, rejecting client.
So, git bisect time... and that leads to
$ git bisect good
c252dcc073155567c1982611ec6f065342909287 is the first bad commit
commit c252dcc073155567c1982611ec6f065342909287
Author: Arne Schwabe <a...@rfc2549.org>
Date: Fri Jul 3 11:55:06 2020 +0200
Remove did_open_context, defined and connection_established_flag
...
which is somewhat nonobvious to me - but the call chain for the pf*context()
stuff is sufficiently "out of the norm" that it could very well be, changing
assumptions about "how do we know the state of a multi_instance" which
work well for all well-behaved paths, but not for the pf stuff.
That said, I think a "proper" fix might be to move the pf_init_context()
call to "when the instance is fully initialized and TLS has succeeded"
(so we can proper shut down the instance again). Somewhere close to the
other plugin calls like "client-connect", and fail "as they do", if
needed.
But I do not intend to do that, just write up thoughts for reference if
we ever ask ourselves "what was the result of that investigation?" :-)
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 g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
