git: 74ae3f3e33b8 - main - if_wg: import latest fixup work from the wireguard-freebsd project
Scott Long
scottl at samsco.org
Mon Mar 15 15:46:54 UTC 2021
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
More information about the dev-commits-src-all
mailing list