D15093: Add WireGuard capability.

andersonbruce created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. andersonbruce requested review of this revision. REPOSITORY R116 Plasma Network Management Applet BRANCH Feature/AddWireGuardVPN REVISION DETAIL https://phabricator.kde.org/D15093

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#315875 , @ngraham wrote: > Wow, what a first patch! And you even used `arc` too, how nice. > > Since this fixes https://bugs.kde.org/show_bug.cgi?id=397572, can you indicate it as such in the Summary

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#315877 , @lbeltrame wrote: > Some smallish nitpicks I noticed there. Also, does this mean that NM has WG support now? Yes there is an add-on available. The original is at

D15093: Add WireGuard capability.

andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in wireguard.cpp:71 > You can use simpleipv[4,6]validator we have in plasma-nm instead of using > everything below. Or maybe QHostAddress can validate it for you? The problem with using the simpleipv[4,6]validator is that

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#320599 , @jgrulich wrote: > I think you can completely remove WireguardAuth dialog if there is no use for it. I also spotted few trailing spaces in the patch, please remove them. The 'askUser'

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 41241. andersonbruce added a comment. - Fixed comment - Expand capabilities of SimpleIP validators. - Changes per review comments - Update for updated ip validators REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D15093: Add WireGuard capability.

andersonbruce marked 37 inline comments as done. andersonbruce added a comment. I think I've gotten most of the fixes in. I made changes to simpleipv4addressvalidator and simpleipv6addressvalidator to add checking of addresses with CIDR and Port suffixes. Please check these carefully, I

D15093: Add WireGuard capability.

andersonbruce added a comment. I've used KDE for years but this is the first time I've written code using Qt so it doesn't surprise me that I didn't use some of the preferred methods of doing things. I have a few questions below and hopefully you will have a little patience with me if any

D15093: Add WireGuard capability.

andersonbruce marked an inline comment as done. andersonbruce added a comment. In D15093#320577 , @jgrulich wrote: > > Does wg-quick support both, like simple commands and script files? If so, we should support both as well, if it

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#323095 , @pino wrote: > Thanks Bruce for all the updates, and patience! In return, I have more notes :) > > - I'd send the changes to the existing SimpleIpV4AddressValidator in an own review request,

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 41451. andersonbruce added a comment. - Correct capitalization - Remove HTML from tooltip strings - Update some tooltip strings for clarity REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D15093: Add WireGuard capability.

andersonbruce added a comment. Well for the first time since July there has been activity on network-manager-wireguard including the addition of an additional parameter supported so I am going to get going on adding it to this. Also, it looks like the pre- and post- options now do something

D15093: Add WireGuard capability.

andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in wireguardwidget.cpp:60 > Do not use KColorScheme. Use QPalette instead and you don't need to keeping > it as class variable. My apologies if I sound a little frustrated on this but I spent more than 4 hours trying to

D15093: Add WireGuard capability.

andersonbruce marked 9 inline comments as done. andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in plasmanetworkmanagement_wireguardui.desktop:16 > Name could be just WireGuard I guess I left this alone because it is consistent with the names of other VPNs. > jgrulich

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 44198. andersonbruce added a comment. - Address review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15093?vs=42290=44198 BRANCH Feature/AddWireGuardVPN REVISION DETAIL

D15093: Add WireGuard capability.

andersonbruce added a comment. A couple of notes on the latest upload: 1. I left the background color change code alone. Doing searches on changing widget background color, it looks like the preferred method is to use setPalette() which requires a QPalette, and after trying a couple of

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 45001. andersonbruce marked an inline comment as done. andersonbruce added a comment. - Allocate palettes in advanced widget rather than passing them from main interface REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#354899 , @jgrulich wrote: > I still don't like the way how to get QPalette in the advanced dialog, can you please just simply construct it the same way you do it in the standard dialog? Fair

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#355860 , @ngraham wrote: > Very cool! Does this actually implement full native WireGuard support, or only add support for the NetworkManager plugin available at

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#355941 , @ngraham wrote: > Ah, thanks. And presumably shipping that networkmanager plugin would be up to distros, right? I assume this is the case. There are several NetworkManager VPN

D15093: Add WireGuard capability.

andersonbruce added a comment. I've uploaded everything I wanted to get done now. Are there any new comments? And by the way, what is the process once the all the comments do get cleared? REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL

D15093: Add WireGuard capability.

andersonbruce added inline comments. INLINE COMMENTS > pino wrote in wireguard.cpp:175-178 > KConfig already supports comma-separated lists -- just pass QStringList() as > `default` value to readEntry(), so KConfigGroup knows the value is a list. The problem with using the KConfig method is if

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#319253 , @pino wrote: > Much better now! > > - regarding the UI for all the pre/post scripts: since they are file paths, better use a KUrlRequester widget (limited to local existing files only, no

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 40885. andersonbruce added a comment. Updates from review comments - Add copyright notices - Delete unneeded files - Formatting changed to agree with coding standards - Changed validation method removing color overrides - Removed hard

D15093: Add WireGuard capability.

andersonbruce added a comment. I'm not sure if the author or the reviewer is supposed to check the "Done" box on the inline comments but I think that I have addressed all the various comments made, both inline and separately. REPOSITORY R116 Plasma Network Management Applet REVISION

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce abandoned this revision. andersonbruce added a comment. Changes for this revision were merged into D15520 REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D15521 To: andersonbruce, jgrulich,

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 42118. andersonbruce marked an inline comment as done. andersonbruce added a comment. - Update from more (but not all) review comments - Change 'setting' functions to use blank NMStringMap rather than incoming data. REPOSITORY R116 Plasma

D15093: Add WireGuard capability.

andersonbruce added a comment. Okay, I just uploaded changes for most of the rest of the comments but I would like to revisit a couple of earlier issues. The first is the SpinBoxes. As I said in another comment, I don't think that the SpinBoxes are appropriate for any of the entries and

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

andersonbruce updated this revision to Diff 42281. andersonbruce added a comment. - Fix a case that was being reported as Acceptable rather than Intermediate REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15520?vs=41878=42281

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 42290. andersonbruce added a comment. - Add entry widget background color change based on entry validity - Change QSpinBoxes back to QLineEdits REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

andersonbruce updated this revision to Diff 42297. andersonbruce added a comment. - Fix review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15520?vs=42281=42297 BRANCH IP4/6validatorsUpdate REVISION DETAIL

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce created this revision. andersonbruce added reviewers: jgrulich, pino. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. andersonbruce requested review of this revision. REVISION SUMMARY CCBUG: 397572 FIXED-IN: 5.14.0 REPOSITORY R116 Plasma Network

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidatorto optionally handle a CIDR or port suffix. Added capabilitiesimplemented via defaulted parameter to maintain compatibilitywith

andersonbruce created this revision. andersonbruce added reviewers: jgrulich, pino. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. andersonbruce requested review of this revision. REVISION SUMMARY ...updated code. Opened as a separate review per comment from Pino on

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce updated this revision to Diff 41733. andersonbruce added a comment. - Correct overlooked comments from review D15093 Updating D15521: Add validator for lists of IP addresses

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 41736. andersonbruce added a comment. - Add tests for new and updated ip validators - Update ip validators - Add validator for WireGuard keys - Update per review comments - Add error messages for failure cases REPOSITORY R116 Plasma

D15093: Add WireGuard capability.

andersonbruce marked 45 inline comments as done. andersonbruce added inline comments. INLINE COMMENTS > pino wrote in wireguard.cpp:121 > better use qt's foreach here: > > Q_FOREACH (const QString , valueList) > > and then using `address` instead of `valueList[i]` Used plain 'for' instead

D15093: Add WireGuard capability.

andersonbruce updated this revision to Diff 41874. andersonbruce marked an inline comment as done. andersonbruce added a comment. - Remove changes moved to review D15520 REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

andersonbruce updated this revision to Diff 41875. andersonbruce added a comment. - Merge new IP list validator into this branch - Update from review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15520?vs=41677=41875

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce added a comment. In D15521#327222 , @jgrulich wrote: > Maybe merge this review with D15520 . I think they should go together. I believe that I have now moved all the changes from this

D15093: Add WireGuard capability.

andersonbruce added a comment. Since I added a validator function for the WireGuard style keys, is there any way to assign a validator to the PasswordField widget without a fairly substantial rewrite of that class? REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL

D15520: Upgrade SimpleIpV4AddressValidator and SimpleIpV6AddressValidator

andersonbruce updated this revision to Diff 41878. andersonbruce added a comment. - Remove unnecessary includes and member functions REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15520?vs=41875=41878 BRANCH

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#327917 , @pino wrote: > note there are still few "not done" comments around (eg using QSpinBox for fwMark) Yes I know, I'll get to the spinbox in the next day or so. I've only been putting it off

D15093: Add WireGuard capability.

andersonbruce added a comment. In D15093#328246 , @pino wrote: > In D15093#328241 , @andersonbruce wrote: > > > In D15093#327917 , @pino wrote: > > > >

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce updated this revision to Diff 41794. andersonbruce added a comment. - Correct CMakefile I screwed up Updating D15521: Add validator for lists of IP addresses Added as separate review per comment from Pino on

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce marked 3 inline comments as done. andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in CMakeLists.txt:92 > I'm sure you don't need to add all these and why did you remove PUBLIC and > PRIVATE keywords? Damn, copied the wrong file. Sorry about that. REPOSITORY

D15093: Add WireGuard capability.

andersonbruce marked an inline comment as done. andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in wireguardadvancedwidget.cpp:104 > I would prefer having just an empty map with data where you just set > everything the user configured in UI, removing options from existing

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce added a comment. In D15521#327222 , @jgrulich wrote: > Maybe merge this review with D15520 . I think they should go together. Can you please discuss this with Pino, he was the one who

D15521: Add validator for lists of IP addressesAdded as separate review per comment from Pino onreview D15093. This code will not compile withoutthe updated code in review D15520. Also includesunit te

andersonbruce added inline comments. INLINE COMMENTS > jgrulich wrote in simpleiplistvalidator.cpp:39 > In both validator constructors (mean also for IPv6 one), you can directly > pass "style" param from contructor, given both enums seem to be identical. They are the same now but initially

D19435: Remove redundant parameter fields

andersonbruce created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added a reviewer: jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY BUG: 403547 Remove entry of parameters which are specific to the wg-quick

D19434: Change entry of Endpoint UI

andersonbruce created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added a reviewer: jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY BUG: 403548 Entry of the Endpoint was originally done in only one LineEdit

D19434: Change entry of Endpoint UI

andersonbruce added a comment. In D19434#423458 , @jgrulich wrote: > Shouldn't we maybe use QSpinBox for the port? With the spinbox you can also set min and max values so you don't need to validate it. My preference is to only use a

D19434: Change entry of Endpoint UI

andersonbruce updated this revision to Diff 53328. andersonbruce added a comment. - Add validator for Endpoint Port field REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D19434?vs=52875=53328 BRANCH EndPointUIChange REVISION

D18867: Remove redundant DNS field per bug 403546

andersonbruce created this revision. andersonbruce added a reviewer: jgrulich. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added 1 blocking reviewer(s): jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY A DNS field was included on

D18867: Remove redundant DNS field per bug 403546

andersonbruce added a comment. Any connection created by the previous version or the basic NetworkManager plugin should continue to work even though the DNS server list will not be viewable in the new interface. I did not provide an automatic conversion from a previously created

D19434: Change entry of Endpoint UI

andersonbruce added a comment. In D19434#424235 , @jgrulich wrote: > In D19434#423613 , @andersonbruce wrote: > > > In D19434#423458 , @jgrulich wrote:

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57970. andersonbruce added a comment. - Don't allow save of configuration with "AlwaysAsk" flag for keys REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57969=57970 BRANCH

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. Updated SecretAgent class to always try to get the secrets from kwallet even if the 'allow-interaction' flag is set. The keys WireGuard uses are 43 random characters long and we don't expect the user to enter these manually when trying to make a connection. If

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce marked an inline comment as done. andersonbruce added a comment. In D20930#464387 , @jgrulich wrote: > In D20930#464384 , @jgrulich wrote: > > > In D20930#464377

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce marked an inline comment as done. andersonbruce added a comment. In D20930#464412 , @jgrulich wrote: > In D20930#464403 , @andersonbruce wrote: > > > In D20930#464387

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57983. andersonbruce added a comment. - Disable "AlwaysAsk" option in key password fields Caution: will not compile without changes to passwordfield.cpp and passwordfield.h made in the current master branch. REPOSITORY R116 Plasma

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57972. andersonbruce added a comment. - Return non-WireGuard logic to original state REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57970=57972 BRANCH TwoTabs REVISION DETAIL

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57969. andersonbruce marked an inline comment as done. andersonbruce added a comment. - Always get encrypted data from kwallet Don't ask user for key values. REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. The changes to the key passwordfield menus was made to remove the AlwaysAsk option. This is the last change I am aware of necessary to release this. REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D20930 To:

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce marked 8 inline comments as done. andersonbruce added a comment. Update to fix some comments. INLINE COMMENTS > jgrulich wrote in connectionicon.cpp:403 > You can now use NetworkManager::Device::WireGuard. There is still a discrepancy between the enum defined by NetworkManager

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57888. andersonbruce added a comment. - Correct review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57790=57888 BRANCH TwoTabs REVISION DETAIL

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. In D20930#462547 , @ngraham wrote: > So much better! > > I have one more little suggestion for the buttons: move the "Remove Peer" button into the bottom of the tab for that peer to make it clear what it

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. In D20930#462775 , @ngraham wrote: > In D20930#462774 , @andersonbruce wrote: > > > Personally I like the Add and Remove buttons to be together where they were, and

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57790. andersonbruce added a comment. - Update labels on Add and Remove peer buttons REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57741=57790 BRANCH TwoTabs REVISION DETAIL

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce created this revision. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added a reviewer: jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY In NetworkManager 1.16 handling of WireGuard interfaces was changed from a VPN

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. A fairly large update was made to change the input of peer data to a use a QTabWidget interface to switch between peers rather than a spin box. REPOSITORY R116 Plasma Network Management Applet REVISION DETAIL https://phabricator.kde.org/D20930 To:

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57741. andersonbruce added a comment. Change to tab bar interface for peers REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57434=57741 BRANCH TwoTabs REVISION DETAIL

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. In D20930#462365 , @ngraham wrote: > I'm afraid I don't have the ability to test this; could you post a new screenshot? Thanks! Here's a screenshot of the dialog with three peers:F6814829: Tabs.png

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce marked 12 inline comments as done. andersonbruce added a comment. Requested changes are made. INLINE COMMENTS > jgrulich wrote in connectionicon.cpp:403 > Do you want me to add this to NMQT? Yes, please. This is part of the device type enum we discussed earlier where not all

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce added a comment. In D20930#459505 , @ngraham wrote: > When #VDG assistance is requested, screenshots of the current/proposed UI are appreciated. :) F6804483: PeersDialog.png

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57433. andersonbruce added a comment. - Correct CamelCase of Wireguard to WireGuard - Disable adding WireGuard for NetworkManager < 1.16 - Correct review comments REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE

D20930: Update WireGuard to match NetworkManager 1.16 interface

andersonbruce updated this revision to Diff 57434. andersonbruce marked an inline comment as done. andersonbruce added a comment. - Correct review comment REPOSITORY R116 Plasma Network Management Applet CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D20930?vs=57433=57434 BRANCH

D22283: Change validator for Endpoint Address entry field

andersonbruce created this revision. andersonbruce added a reviewer: jgrulich. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added 1 blocking reviewer(s): jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY BUG: 408670 Change

D29469: Add NetworkMannager::primaryConnection() call for WireGuard connections.

andersonbruce created this revision. andersonbruce added a reviewer: jgrulich. Herald added a project: Plasma. Herald added a subscriber: plasma-devel. Herald added 1 blocking reviewer(s): jgrulich. andersonbruce requested review of this revision. REVISION SUMMARY If a WireGuard connection is