git: 74ae3f3e33b8 - main - if_wg: import latest fixup work from the wireguard-freebsd project

[Scott Long]

I’m sorry that you feel that I was “immediately aggressive”.  I have the IRC
logs to back up my statement that I was supportive but concerned, and
what I objected to.  If you feel that I was immediately aggressive or that I
deserved the scorn you hurled, then I really have nothing more to add to
this conversation, other than I’ll be looking for other ways for Netgate to
operate that don’t overlap with Wireguard.

Scott


> On Mar 15, 2021, at 9:27 AM, Kyle Evans <kevans at freebsd.org> wrote:
> 
> On Mon, Mar 15, 2021 at 9:57 AM Scott Long <scottl at samsco.org> wrote:
>> 
>> Here is the response I sent to you and Donenfeld in private.  I won’t include
>> my direct conversation with you from Slack/IRC, but I made my concerns
>> and objections pretty clear.  This commit is quite disappointing.  See below:
>> 
> 
> I'm sorry that you feel that way, but I've been pretty vocal about my
> intentions to merge this work as soon as we were done with internal
> review due to the pre-existing state of the driver.
> 
>> The good:
>> [... snip ...]
>> 
>> The bad:
>> - I want to be pragmatic about code APIs.  Maybe iflib isn’t ready for
>> pseudo interfaces yet, and fixing it is non-trivial and out of scope.
>> Going back to ifnet puts it back in line with openbsd and likely does
>> fix the vnet problems.  However, it’s a radical change to the driver, and
>> not something that I can support as a last-minute action for FreeBSD
>> 13.0 or for pfSense.  Therefore, I’m advising against its immediate
>> inclusion.  The final choice is not mine to make for FreeBSD, but
>> that’s my recommendation.  For pfSense, I’ll be discussing this with
>> the rest of the engineering staff on Monday.
>> 
> 
> iflib is definitely not ready for pseudo interfaces, and I'd like to
> see the technical justification for using iflib here in the first
> place. if_wg used exactly 0 of its real features because the queueing
> system was bypassed by setting if_transmit in its IFDI_ATTACH_POST
> implementation. In other words, we gained all of its complexity and
> pseudo interfaces immaturity without any benefit that we found while
> exploring the possibilities.
> 
>> - The LKML wouldn’t accept this kind of submission, they’d insist that
>> it be broken down into consumable pieces, and that bug fixes be
>> considered and provided that don’t rely on massive re-writes.  I’ve
>> been dealing with linux for 20+ years and BSD for almost 30 years,
>> and I’ve got to say that despite my distaste for how the LKML is run,
>> they get results.  Does fixing a segfault or packet drop/reorder
>> require the removal of iflib?
>> 
> 
> The review process on FreeBSD breaks down, as you yourself have noted.
> mmacy has not been involved in if_wg since dropping it in the tree
> AFAICT, and grehan claimed to only be involved because it was passed
> to him at Netgate and that he didn't mind where it goes. Thus, I used
> developer discretion and sought review from the person that wrote the
> OpenBSD implementation and the person that designed the protocol. We
> iterated on this for days to fix the numerous issues we were presented
> with.
> 
>> The Ugly:
>> - An accusation was made, tonight, to me, that the code Netgate
>> sponsored was not reviewed and was shoved into the tree at the last
>> minute.  This grossly ignores the actual history to the point of
>> weakening my tolerance for this entire discussion.  It shows a pretty
>> irrational bias against mmacy and Netgate and a gross ignorance of
>> the history and provenience of the work.
>> 
> 
> I'm sorry that I got heated here, your tone was immediately aggressive
> after I just spent five days cleaning up the mess that was left
> behind. At least one of the security issues I found was a small
> configuration tweak and near-immediate destruction of the system when
> applying any real load to the driver.
> 
>> - The Netgate copyright was unilaterally removed.  Yes, it was re-
>> instated a few minutes ago, and with good reason.  Please don’t
>> do that again.
>> 
> 
> I won't touch on this, other than "ack".
> 
>> - The removal of the ASM crypto bits really confuses me.  An
>> accusation was made, again tonight, that Netgate merely paid to
>> benchmark the code, that we contributed nothing to it, and that it’s
>> bad code that can’t be audited.  What I see is that the meat of the
>> implementation is copyrighted by Jason and others.  There’s a
>> modest but non-trivial amount of glue code that has (or had) the
>> Netgate copyright. I’m not going to touch the audit nonsense.
>> I need data here, and I’m not seeing it.
>> 
> 
> I would have appreciated a pointer to the copyrighted 63 lines in
> include/, because this was obviously ignorance on my part. You
> suggested functional testing and bug fixing, the former of which I
> inherently include in 'benchmarking' since you can't benchmark
> something that isn't functional, and received no pointer of the
> latter.
> 
>> There seems to be a lot of bad blood, poor understanding, and
>> misinformation going on.  I need all of this bullshit to stop.  This
>> is 5000-ish lines of a nice way to get a point-to-point VPN going
>> without the hassle of IPSec or the performance problems of
>> OpenVPN.  It’s consuming way more time and energy than it’s
>> worth to me, and I’d much rather spend time and money to
>> implement OpenVPN DCO and work on profiling and optimizing
>> IPSec.  Wireguard is important to Netgate because we recognize
>> that it’s a feature that customers want, we’re committed to making
>> security accessible, and we strongly believe in open source and
>> FreeBSD. It’s not perfect code, not by a long shot, but I expect
>> better collaboration and communication than what I’m seeing here.
>> 
> 
> It's important to me that we do what's right for the community, and
> the if_wg module that was in-tree was not right for the community. I
> just want something secure and usable in the tree, the latter point
> being the packet loss complaints from the Netgate support forum that I
> pointed you at as well as the kernel not handling allowed-ips
> conflicts that I had mentioned, among various protocol violations and
> other things the module did not handle w.r.t. configuration. The
> former point being at least the buffer overflow I mentioned, but there
> are more.
> 
> Thanks,
> 
> Kyle Evans
> 
>> Scott
>> 
>> 
>>> On Mar 14, 2021, at 10:52 PM, Kyle Evans <kevans at FreeBSD.org> wrote:
>>> 
>>> The branch main has been updated by kevans:
>>> 
>>> URL: https://cgit.FreeBSD.org/src/commit/?id=74ae3f3e33b810248da19004c58b3581cd367843
>>> 
>>> commit 74ae3f3e33b810248da19004c58b3581cd367843
>>> Author:     Kyle Evans <kevans at FreeBSD.org>
>>> AuthorDate: 2021-03-15 02:25:40 +0000
>>> Commit:     Kyle Evans <kevans at FreeBSD.org>
>>> CommitDate: 2021-03-15 04:52:04 +0000
>>> 
>>>   if_wg: import latest fixup work from the wireguard-freebsd project
>>> 
>>>   This is the culmination of about a week of work from three developers to
>>>   fix a number of functional and security issues.  This patch consists of
>>>   work done by the following folks:
>>> 
>>>   - Jason A. Donenfeld <Jason at zx2c4.com>
>>>   - Matt Dunwoodie <ncon at noconroy.net>
>>>   - Kyle Evans <kevans at FreeBSD.org>
>>> 
>>>   Notable changes include:
>>>   - Packets are now correctly staged for processing once the handshake has
>>>     completed, resulting in less packet loss in the interim.
>>>   - Various race conditions have been resolved, particularly w.r.t. socket
>>>     and packet lifetime (panics)
>>>   - Various tests have been added to assure correct functionality and
>>>     tooling conformance
>>>   - Many security issues have been addressed
>>>   - if_wg now maintains jail-friendly semantics: sockets are created in
>>>     the interface's home vnet so that it can act as the sole network
>>>     connection for a jail
>>>   - if_wg no longer fails to remove peer allowed-ips of 0.0.0.0/0
>>>   - if_wg now exports via ioctl a format that is future proof and
>>>     complete.  It is additionally supported by the upstream
>>>     wireguard-tools (which we plan to merge in to base soon)
>>>   - if_wg now conforms to the WireGuard protocol and is more closely
>>>     aligned with security auditing guidelines
>>> 
>>>   Note that the driver has been rebased away from using iflib.  iflib
>>>   poses a number of challenges for a cloned device trying to operate in a
>>>   vnet that are non-trivial to solve and adds complexity to the
>>>   implementation for little gain.
>>> 
>>>   The crypto implementation that was previously added to the tree was a
>>>   super complex integration of what previously appeared in an old out of
>>>   tree Linux module, which has been reduced to crypto.c containing simple
>>>   boring reference implementations.  This is part of a near-to-mid term
>>>   goal to work with FreeBSD kernel crypto folks and take advantage of or
>>>   improve accelerated crypto already offered elsewhere.
>>> 
>>>   There's additional test suite effort underway out-of-tree taking
>>>   advantage of the aforementioned jail-friendly semantics to test a number
>>>   of real-world topologies, based on netns.sh.
>>> 
>>>   Also note that this is still a work in progress; work going further will
>>>   be much smaller in nature