|
|
Subscribe / Log in / New account

Progress on Zinc (thus WireGuard)

By Jake Edge
September 26, 2018

When last we looked at the WireGuard VPN code and its progress toward mainline inclusion, said progress was impeded by disagreements about the new "Zinc" cryptographic library that is added by the WireGuard patches. Since that August look, several more versions of WireGuard and Zinc have been posted; it would seem that Zinc is getting closer to being accepted. Once that happens, the networking developers are poised to review that portion of the code, which likely will lead to WireGuard in the kernel some time in the next development cycle or two.

Jason Donenfeld posted Zinc v3 as part of an updated WireGuard posting on September 10. Of the versions he has posted since our article (up to v6 as of this writing), v3 has gotten most of the comments. One of the main complaints about Zinc is that it creates a new crypto API in the kernel without really addressing why the existing one would not work for WireGuard. As Ard Biesheuvel put it:

In spite of the wall of text, you fail to point out exactly why the existing AEAD [authenticated encryption with associated data] API in unsuitable, and why fixing it is not an option.

As I pointed out in a previous version, I don't think we need a separate crypto API/library in the kernel, and I don't think you have convinced anyone else yet either.

Perhaps you can devote /your/ rare talent and energy to improving what we already have for everybody's sake, rather than providing a completely separate crypto stack that only benefits WireGuard (unless you yourself port the existing crypto API software algorithms to this crypto stack first and present *that* work as a convincing case in itself)

But Greg Kroah-Hartman said that the current crypto API is too hard to use for many parts of the kernel, which leads to simpler, private implementations of crypto primitives sprinkled all over the kernel tree. He suggested that the existing crypto API be switched over time to use the Zinc primitives where that is possible. But Eric Biggers was concerned that no conversions of that sort have been done, which means there could be undiscovered problems in the Zinc API that will make it difficult to do so: "I don't think it makes sense to merge all this stuff without doing the conversions, or at the very least demonstrating how they will be done".

Donenfeld said that he is willing to do those conversions, but wants to get the series merged first. "I'd really prefer to land this series through net-next, and then after we can turn our attention to integrating this into the existing crypto API". But, as Andrew Lunn pointed out, that may be putting the cart ahead of the horse. He noted that the networking developers have not had a serious look at the WireGuard patches and won't "until the controversial part of the code, Zinc, has been sorted out". He also predicted that networking maintainer David Miller would not take the code into his tree without an Acked-by from the crypto maintainers.

Miller confirmed that assessment and clarified that even though he is listed as one of the two crypto maintainers, he would be looking for an ack from the other maintainer, Herbert Xu, as "I haven't done a serious review of crypto code in ages". Xu has been quiet, so far, on the Zinc patches, with one exception.

Donenfeld feels that Biesheuvel is not pleased with the existence of Zinc. When Biesheuvel listed the additional items that he thinks need to be addressed in Zinc, Donenfeld's response is prefaced by a number of worries about Biesheuvel: that he is "generally hostile to this whole initiative", is trying to "stall it indefinitely", and perhaps will just keep bikeshedding "until Zinc copies lots of the same design decisions from the present crypto API". Donenfeld did also say that he hoped these were all just fears and did not truly reflect what was happening. But that was when Xu stepped in to make it clear that he values the review that Biesheuvel has been doing:

That may be your view but from what I've read Ard has been very constructive in pointing out issues in your submission. If your response to criticism is to dismiss them as hostile then I'm afraid that we will not be able to progress on this patch series.

Please keep in mind that this is a large project that has to support multiple users on one hand (not just WireGuard) and complex hardware acceleration drivers on the other. Ard has been one of the most prolific contributors to the crypto code and his review should be taken seriously.

For his part, Biesheuvel tried to make his intentions clear to Donenfeld in another part of the thread:

I am not an 'entrenched crypto API guy that is out to get you'. I am a core ARM developer that takes an interest in crypto, shares your concern about the usability of the crypto API, and tries to ensure that what we end up is maintainable and usable for everybody.

But the main technical objections that Biggers and Biesheuvel have raised were still being hashed out in the thread. Andy Lutomirski suggested that Donenfeld add a conversion of one of the algorithms in the existing crypto API to use Zinc as part of the patch set. After a bit of resistance, Donenfeld agreed. Conversions of the Poly1305 hash and ChaCha20 cipher (which are what WireGuard uses) in the existing crypto subsystem were part of the WireGuard v4 patch set.

Along the way, there have also been discussions about the OpenSSL implementations of some of the primitives (i.e. CRYPTOGAMS) that were incorporated into Zinc. These are written in assembly, but are actually generated from Perl scripts. Donenfeld has modified the assembly output in order to make it comply with kernel coding standards but has also made some other adjustments. That makes it difficult to keep the implementation in sync with any changes that OpenSSL might make, Biesheuvel noted. "Dumping 10,000s of lines of generated assembler in the kernel tree like that is really unacceptable IMO." Donenfeld said that he disagrees with the characterization of the code, but that getting his changes into the OpenSSL upstream is desirable.

Beyond that, there were some minor licensing concerns (and the resulting SPDX identifiers) with some files, which have seemingly been resolved. Similarly, some performance problems were noted and addressed. In short, Zinc is starting to look like something that could be merged. Donenfeld posted v6 of the WireGuard patch set on September 25.

Zinc is still awaiting an ack from Xu, though it is not clear how much he has scrutinized the code at this point. Once that happens, though, the networking side of the patch set can be reviewed by Miller and other networking developers. If all goes well, it will end up in the mainline before too long—but that still means at least four, or more likely seven, months from now. Whenever it comes, it is clear that WireGuard is eagerly anticipated by many.


Index entries for this article
KernelCryptography
KernelNetwork/Virtual private networks
SecurityCryptography
SecurityNetwork/Virtual private network (VPN)


(Log in to post comments)

Progress on Zinc (thus WireGuard)

Posted Sep 26, 2018 17:35 UTC (Wed) by zx2c4 (subscriber, #82519) [Link]

Donenfeld said that he is willing to do those conversions, but wants to get the series merged first. "I'd really prefer to land this series through net-next, and then after we can turn our attention to integrating this into the existing crypto API".
For what it's worth, I wound up doing those conversions anyway, and they're a part of v6. This pattern applies for most other things -- I didn't want to do them, in order to keep the Zinc patchset small, but Ard has insisted, and I've relented. So hopefully piece by piece things are being done as needed. So it goes.
but that getting his changes into the OpenSSL upstream is desirable.
And indeed Andy Polyakov and I have already started talking about this stuff and things are moving nicely on this front.

Progress on Zinc (thus WireGuard)

Posted Sep 26, 2018 17:39 UTC (Wed) by GennaroReinger (guest, #127208) [Link]

I would prefer for Jason to stop making Ad hominem[0] digressions towards his reviewers. It's OK to disagree with someone but not implying their motivation is sinister. Things like below were repeated several times during this series.

https://lore.kernel.org/lkml/CAHmME9p5b=L0FSL72gCszhvut-k...

[0] Ad hominem, short for argumentum ad hominem, is a fallacious argumentative strategy whereby genuine discussion of the topic at hand is avoided by instead attacking the character, motive, or other attribute of the person making the argument, or persons associated with the argument, rather than attacking the substance of the argument itself. https://en.wikipedia.org/wiki/Ad_hominem

Progress on Zinc (thus WireGuard)

Posted Sep 26, 2018 18:48 UTC (Wed) by johannbg (subscriber, #65743) [Link]

Or he could just as well be following the communication standard which has been set by the kernel community leader through his response and thus acts as an example for others to follow and thinks this is how the community prefers to communicate, could he not? ( if you give him the benefit of doubt )

It can also be a good practise in communities for other reviewers to chime in once certain number of reviews from the same reviewer is exceeded ( like 3 or something ), to prevent the submitter getting the feeling that no matter how many times he rewrites things at the reviewer behest, it never is or will be good enough.

Then there are legit case to what he is implying is taking place ( the I own the code syndrom which is similar when administrators self claim ownership of servers ) but as you point out there are better ways to deal with this instead of starting ad hominen attack, like for example just saying it's starting to feel like i'm swimming upstream, can I get a second opinion from someone else ( or something along those lines ).

Progress on Zinc (thus WireGuard)

Posted Sep 26, 2018 18:54 UTC (Wed) by zx2c4 (subscriber, #82519) [Link]

I agree, I should have written this differently and toned this down a bit. I apologized in a follow-up email.

The patchset is very much benefitting from the reviews of many, who have all sorts of interesting perspectives to offer. In many areas it's improved tremendously thanks to those. However, I'm pretty wary of directions that lead to bloating the core concepts of Zinc and making the same mistakes as the current crypto API. On those matters, I've been particularly standoffish, especially when I've seen many repeated instances of this. But indeed that should be handled more diplomatically.

Progress on Zinc (thus WireGuard)

Posted Jun 7, 2020 8:10 UTC (Sun) by scientes (guest, #83068) [Link]

I frankly agree with the anger. Ard is repeatably asking for an explanation of what is wrong with the old API, when that explanation has been provided over and over. What is going on here is a fear of admitting fault, and we have a worse kernel because of it.

"If, now, your right eye is making you stumble, tear it out and throw it away from you. For it is better for you to lose one of your members than for your whole body to be pitched into Gehenna. Also, if your right hand is making you stumble, cut it off and throw it away from you. For it is better for you to lose one of your members than for your whole body to land in Gehenna."--Jesus, Matthew 5:29-30

(Yes, I know this is an old thread)

Progress on Zinc (thus WireGuard)

Posted Sep 26, 2018 19:24 UTC (Wed) by rweikusat2 (subscriber, #117920) [Link]

After about a week of digging through a sizable amount of essentially legacy Java code mostly created via mindless copy'n'paste and based on meanwhile seriously ancient version of JBoss, Seam, JSF and RichFaces in order to identifty a certain problem with it, find a way to fix it which works with "the stack" and carefully trying to get rid of at least some ancient horrors encountered during the journey, this "My API!" "His API!" squabbling make for some delightful, light social comedy :-)

Progress on Zinc (thus WireGuard)

Posted Sep 27, 2018 0:55 UTC (Thu) by pabs (subscriber, #43278) [Link]

The generated assembly thing sounds like a violation of the spirit if not the letter of the GPL.

Progress on Zinc (thus WireGuard)

Posted Sep 27, 2018 9:09 UTC (Thu) by moltonel (guest, #45207) [Link]

Would it make sense and be possible to commit the generation code rather than the generated result ?

Progress on Zinc (thus WireGuard)

Posted Sep 27, 2018 9:14 UTC (Thu) by epa (subscriber, #39769) [Link]

Often it's best to commit both. Then you have a safety check in case the generator starts doing something weird.

Progress on Zinc (thus WireGuard)

Posted Sep 27, 2018 11:34 UTC (Thu) by ardbiesheuvel (subscriber, #89747) [Link]

That is exactly what I have been doing for the past 5 years [1] [2], after working with Andy Polyakov to upstream the #ifdef __KERNEL__ changes to OpenSSL first.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/...
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/...

Progress on Zinc (thus WireGuard)

Posted Sep 27, 2018 18:13 UTC (Thu) by JoeBuck (subscriber, #2330) [Link]

Including just the generated assembly, and not the generator, would violate GPL2 (both the spirit and the letter), which require source code and says "The source code for a work means the preferred form of the work for making modifications to it. " That's the same reason that obfuscated C code does not count as "source code". In this case, to fix bugs or add features a developer would not edit the assembly code, s/he would edit the generator or the input to the generator, so that has to be included. Fortunately it appears that this is being done.


Copyright © 2018, Eklektix, Inc.
This article may be redistributed under the terms of the Creative Commons CC BY-SA 4.0 license
Comments and public postings are copyrighted by their creators.
Linux is a registered trademark of Linus Torvalds