From owner-dev-commits-src-main@freebsd.org Mon Mar 15 15:46:54 2021 Return-Path: Delivered-To: dev-commits-src-main@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 5364657F40F; Mon, 15 Mar 2021 15:46:54 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from out5-smtp.messagingengine.com (out5-smtp.messagingengine.com [66.111.4.29]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 4DzglL1VCzz3H6R; Mon, 15 Mar 2021 15:46:53 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from compute2.internal (compute2.nyi.internal [10.202.2.42]) by mailout.nyi.internal (Postfix) with ESMTP id DA6835C00F9; Mon, 15 Mar 2021 11:46:53 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute2.internal (MEProxy); Mon, 15 Mar 2021 11:46:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsco.org; h= content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; s=fm2; bh=v Z+SJYcYgRZdIRcmE8tdrtpaBJDOgwlbyfGElBDX8cQ=; b=MbPAEOKycrDF3FQf7 k/iCip0muRxZ6X+ZvGacSF6r/5TB6OWQLCQ2IAduriTlF2B30v9cweI29hgyXoDm RDLo73KTEPq/YIKmqCxSS9ffIMZnfCo+lVIDOCX7v6jhZUnyxRwzXRWWv/pX8unN kFdCW+/EsIZaTIt0a+WqnH2Ck4tzm3kk5E1fgWivPJ3C6YKNOzm/rs9UAPKGEQhs iF2WmArO45gkuWa56VXli90X+UQ8uwMQIHAo4N79fSOUCl0XGd+g0OBEFE+qzbvj hEwzkWdwbd2j7slNAXyKAPDHnbK1hkMuFn/FtF6dER4SqXw99WvG/aL5B7Yttlai hMdjg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm2; bh=vZ+SJYcYgRZdIRcmE8tdrtpaBJDOgwlbyfGElBDX8 cQ=; b=DfQy7dj243dQdxknJVMIo4QOdAivqMBmcCPRAeYE4hXDocqQAJqndaQra eluX3bKTg0LfzXCeHvycne6eI0CiiuIn01ngUxzTTQUnBZ16EBTxFlRovokpW7Te Mbg0OGFA8mvy7bcXKElkM5P1bW5I+68M1UKhwVCNhdyfKxPbU/Rfj8aSl6hbbh3Z DPZ/hEOUjdh2G8rnBF1lpMzuW2V7dWE+PU1yRRUXGEAsaIeaL/OYudsu9nJH18Y2 Zlk+AhHhvlXXg6tvb2K88I+NXyPL6+7k7z3DOXXzn/ssG9yLQW2SW1fzWNBzl00C KMOzf7fA8bTSdgyL6yow3fouJt66Q== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledruddvledgkedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurheptggguffhjgffgffkfhfvofesthhqmhdthhdtjeenucfhrhhomhepufgtohht thcunfhonhhguceoshgtohhtthhlsehsrghmshgtohdrohhrgheqnecuggftrfgrthhtvg hrnhepudduveekheehiedukeekleelvedufeevfeetudfgtdffteffleehheffueffgfeh necuffhomhgrihhnpehfrhgvvggsshgurdhorhhgnecukfhppeekrdegiedrkeelrddvud efnecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepshgt ohhtthhlsehsrghmshgtohdrohhrgh X-ME-Proxy: Received: from [192.168.0.114] (unknown [8.46.89.213]) by mail.messagingengine.com (Postfix) with ESMTPA id 2DEC6240057; Mon, 15 Mar 2021 11:46:53 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.60.0.2.21\)) Subject: Re: git: 74ae3f3e33b8 - main - if_wg: import latest fixup work from the wireguard-freebsd project From: Scott Long In-Reply-To: Date: Mon, 15 Mar 2021 09:46:52 -0600 Cc: "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" Content-Transfer-Encoding: quoted-printable Message-Id: <1D0606F6-73C2-42B9-9CF5-E3EBE01CE4EE@samsco.org> References: <202103150452.12F4qxjV047368@gitrepo.freebsd.org> <13F91280-2246-4A7B-BAC2-B9ABA07B561F@samsco.org> To: Kyle Evans X-Mailer: Apple Mail (2.3654.60.0.2.21) X-Rspamd-Queue-Id: 4DzglL1VCzz3H6R X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-main@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for the main branch of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Mar 2021 15:46:54 -0000 I=E2=80=99m sorry that you feel that I was =E2=80=9Cimmediately = aggressive=E2=80=9D. 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=E2=80=99ll be looking for other ways for = Netgate to operate that don=E2=80=99t overlap with Wireguard. Scott > On Mar 15, 2021, at 9:27 AM, Kyle Evans wrote: >=20 > On Mon, Mar 15, 2021 at 9:57 AM Scott Long wrote: >>=20 >> Here is the response I sent to you and Donenfeld in private. I = won=E2=80=99t 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: >>=20 >=20 > 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. >=20 >> The good: >> [... snip ...] >>=20 >> The bad: >> - I want to be pragmatic about code APIs. Maybe iflib isn=E2=80=99t = 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=E2=80=99s 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=E2=80=99m advising against its = immediate >> inclusion. The final choice is not mine to make for FreeBSD, but >> that=E2=80=99s my recommendation. For pfSense, I=E2=80=99ll be = discussing this with >> the rest of the engineering staff on Monday. >>=20 >=20 > 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. >=20 >> - The LKML wouldn=E2=80=99t accept this kind of submission, they=E2=80=99= d insist that >> it be broken down into consumable pieces, and that bug fixes be >> considered and provided that don=E2=80=99t rely on massive re-writes. = I=E2=80=99ve >> been dealing with linux for 20+ years and BSD for almost 30 years, >> and I=E2=80=99ve 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? >>=20 >=20 > 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. >=20 >> 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. >>=20 >=20 > 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. >=20 >> - The Netgate copyright was unilaterally removed. Yes, it was re- >> instated a few minutes ago, and with good reason. Please don=E2=80=99t= >> do that again. >>=20 >=20 > I won't touch on this, other than "ack". >=20 >> - 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=E2=80=99s >> bad code that can=E2=80=99t be audited. What I see is that the meat = of the >> implementation is copyrighted by Jason and others. There=E2=80=99s a >> modest but non-trivial amount of glue code that has (or had) the >> Netgate copyright. I=E2=80=99m not going to touch the audit nonsense. >> I need data here, and I=E2=80=99m not seeing it. >>=20 >=20 > 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. >=20 >> 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=E2=80=99s consuming way more time and energy than it=E2=80= =99s >> worth to me, and I=E2=80=99d 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=E2=80=99s a feature that customers want, we=E2=80=99re = committed to making >> security accessible, and we strongly believe in open source and >> FreeBSD. It=E2=80=99s not perfect code, not by a long shot, but I = expect >> better collaboration and communication than what I=E2=80=99m seeing = here. >>=20 >=20 > 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. >=20 > Thanks, >=20 > Kyle Evans >=20 >> Scott >>=20 >>=20 >>> On Mar 14, 2021, at 10:52 PM, Kyle Evans wrote: >>>=20 >>> The branch main has been updated by kevans: >>>=20 >>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D74ae3f3e33b810248da19004c58b3581= cd367843 >>>=20 >>> commit 74ae3f3e33b810248da19004c58b3581cd367843 >>> Author: Kyle Evans >>> AuthorDate: 2021-03-15 02:25:40 +0000 >>> Commit: Kyle Evans >>> CommitDate: 2021-03-15 04:52:04 +0000 >>>=20 >>> if_wg: import latest fixup work from the wireguard-freebsd project >>>=20 >>> 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: >>>=20 >>> - Jason A. Donenfeld >>> - Matt Dunwoodie >>> - Kyle Evans >>>=20 >>> 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 >>>=20 >>> 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. >>>=20 >>> 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. >>>=20 >>> 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. >>>=20 >>> Also note that this is still a work in progress; work going = further will >>> be much smaller in nature