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 <kev...@freebsd.org> wrote: > > On Mon, Mar 15, 2021 at 9:57 AM Scott Long <sco...@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 <kev...@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 <kev...@freebsd.org> >>> AuthorDate: 2021-03-15 02:25:40 +0000 >>> Commit: Kyle Evans <kev...@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 <ja...@zx2c4.com> >>> - Matt Dunwoodie <n...@noconroy.net> >>> - Kyle Evans <kev...@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 _______________________________________________ dev-commits-src-main@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/dev-commits-src-main To unsubscribe, send any mail to "dev-commits-src-main-unsubscr...@freebsd.org"
