Update WireGuard to match NetworkManager 1.16 interface
ClosedPublic

Authored by andersonbruce on May 1 2019, 2:03 AM.

Details

Summary

In NetworkManager 1.16 handling of WireGuard interfaces was changed
from a VPN add-on to a core interface type with a different API. This
plasma-nm update is intended to match that change including (but not
limited to) moving address specification to the IPv4 and IPv6 tabs and
the ability to add multiple Peers to an interface.

BUG: 405501

Diff Detail

Repository
R116 Plasma Network Management Applet
Branch
TwoTabs
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11786
Build 11804: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

Can we maybe have some assistence from anyone from VDG? @ngraham maybe?

CMakeLists.txt
11 ↗(On Diff #57297)

Please remove.

kcm/kcm.cpp
263

Please remove.

libs/declarative/connectionicon.cpp
403

Do you want me to add this to NMQT?

libs/editor/connectioneditorbase.cpp
149–150

Please remove

190

Please remove.

254

Why the "2" at the end?

448

Please remove.

454

Please remove.

libs/editor/settings/wireguardinterfacewidget.cpp
115

Coding style.

Should be:

WireGuardInterfaceWidget::WireGuardInterfaceWidget(const NetworkManager::Setting::Ptr &setting, QWidget* parent, Qt::WindowFlags f)
    : SettingWidget(setting, parent, f)
    , d(new Private)
183

I would preffer:

if (wireGuardSetting->listenPort() != 0)

It's more readable. Same for code below.

251

Remove empty line

425

Coding style. The "else if" should be on the same line as the "}" bracket.

483

Same here and below.

569

The "{" bracket should be on the same line as the "else if". Same below.

634

Please remove

libs/editor/settings/wireguardpeerswidget.cpp
82 ↗(On Diff #57297)

Coding style.

ngraham added a reviewer: VDG.May 2 2019, 1:45 PM

When VDG assistance is requested, screenshots of the current/proposed UI are appreciated. :)

  • Correct CamelCase of Wireguard to WireGuard
  • Disable adding WireGuard for NetworkManager < 1.16
  • Correct review comments
andersonbruce marked an inline comment as done.
  • Correct review comment
andersonbruce marked 12 inline comments as done.May 3 2019, 3:27 AM

Requested changes are made.

libs/declarative/connectionicon.cpp
403

Yes, please. This is part of the device type enum we discussed earlier where not all of the values defined in NM have been folded into NMQT yet.

When VDG assistance is requested, screenshots of the current/proposed UI are appreciated. :)


In the dialog shown, there can be several different "Peers" defined. The question involves the use of the SpinBox as the means of switching between the various entries. Each Peer has the same set of parameters to display/modify and changing the SpinBox value changes which one of them is being currently displayed. The "Add" and "Remove" buttons add a new Peer or delete an existing Peer and change the "of x peers" label next to the SpinBox to correspond to the current total number.

I have not seen this exact method of changing a dialog before and while it seems intuitive to me, I don't know if it violates any KDE standards and I think both jgrulich and I just wanted to get input from the VDG before this is released.

Thanks!

Seems like a tab bar would be the ideal UI here, with each peer having its own tab. That communicates the idea that the settings within the tab are specific to that tab/peer. Then the add and remove button should go below the tab bar's frame and say "Add peer" and "Remove peer" so it's clear what they refer to. Finally, the group box with the title "Peers" should be removed since the window's own title adequately communicates what you're looking at (and if it doesn't, that's what should be improved).

Does that help?

Change to tab bar interface for peers

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.

I'm afraid I don't have the ability to test this; could you post a new screenshot? Thanks!

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:

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 applies to, and change the text to say, "Remove this Peer". The Remove button can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" the correct verb? Would "Connect to" be more appropriate?

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 applies to, and change the text to say, "Remove this Peer". The Remove button can stay where it is, but maybe should say, "Add New Peer". Actually, is "Add" the correct verb? Would "Connect to" be more appropriate?

Here is a proposed version of what you asked for. It also contains a little more context showing the parent dialog where the rest of the configuration is entered, behind the Peers dialog under discussion.

Personally I like the Add and Remove buttons to be together where they were, and to simply change the wording to "Add New Peer" and "Remove This Peer". I think this still makes it clear that the peer in the tab currently being displayed in the dialog is the one that will be removed but am willing to do it the way shown below if the VDG considers it to be more consistent with the rest of KDE.

The wording should definitely not be "Connect to" because this form is used to edit a configuration not to actually make a connection. Hitting "Connect" on a WireGuard configuration activates all the peers in that configuration at one time. I haven't set up a large multi-user server but I imagine one could contain dozens if not hundreds of peers.

Personally I like the Add and Remove buttons to be together where they were, and to simply change the wording to "Add New Peer" and "Remove This Peer". I think this still makes it clear that the peer in the tab currently being displayed in the dialog is the one that will be removed

All right, let's do that, then!

The wording should definitely not be "Connect to" because this form is used to edit a configuration not to actually make a connection.

Gotcha, thanks for explaining!

I haven't set up a large multi-user server but I imagine one could contain dozens if not hundreds of peers.

Hmm, if that's the case, then a tabbed UI may not actually be optimal. A list view, with a list of peers on the left side, a search above the list, and the peer information on the right, would be better if there may be enough peers to make access via tab view awkward. If so, I apologize for leading you down a bad path out of ignorance of the underlying technology. What do you think?

Personally I like the Add and Remove buttons to be together where they were, and to simply change the wording to "Add New Peer" and "Remove This Peer". I think this still makes it clear that the peer in the tab currently being displayed in the dialog is the one that will be removed

All right, let's do that, then!

I'll update to this.

I haven't set up a large multi-user server but I imagine one could contain dozens if not hundreds of peers.

Hmm, if that's the case, then a tabbed UI may not actually be optimal. A list view, with a list of peers on the left side, a search above the list, and the peer information on the right, would be better if there may be enough peers to make access via tab view awkward. If so, I apologize for leading you down a bad path out of ignorance of the underlying technology. What do you think?

I attempted to make a list view awhile ago and because of the length of some of the fields (in particular the key fields which are each 44 characters long) I couldn't come up with anything that looked decent. I think the tabbed interface will be fine in the vast majority of cases, especially in a desktop environment where typically there will only be one peer. Considering that the previous version only allowed one peer to be specified, this is a big improvement so I think at least for now and probably the foreseeable future, the tabbed interface is the way to go.

  • Update labels on Add and Remove peer buttons
ngraham accepted this revision.May 9 2019, 1:59 PM

Thanks for the info. I appreciate that you've taken so much time to think this through, and thanks for helping me understand how this works. LGTM from a VDG perspective!

jgrulich added inline comments.May 10 2019, 12:19 PM
kcm/kcm.cpp
527
if (!connection.isEmpty()) {
    return;
}
libs/declarative/connectionicon.cpp
403

You can now use NetworkManager::Device::WireGuard.

libs/declarative/networkstatus.cpp
175

You can use now NetworkManager::Device::WireGuard. It should be part of NMQT 5.58 so we can use it for Plasma 5.16.

libs/editor/settings/wireguardinterfacewidget.cpp
621

The "{" bracket should be on the same line with the "if"

libs/editor/settings/wireguardpeerwidget.cpp
53

You can change all the validators to take all the necessary parameters as first and then have a default value for the parent object so you don't need to pass a nullptr.

E.g.

explicit SimpleIpV4AddressValidator(AddressStyle style = AddressStyle::Base, QObject *parent = nullptr);

I should have noticed this before. Can you change all the validators you use this way?

87
WireGuardPeerWidget::WireGuardPeerWidget(const QVariantMap &peerData, QWidget *parent, Qt::WindowFlags f)
libs/editor/settings/wireguardpeerwidget.h
34–43
explicit WireGuardPeerWidget(const QVariantMap &peerData, QWidget *parent = nullptr, Qt::WindowFlags f = {});
libs/editor/settings/wireguardtabwidget.h
34–45
explicit WireGuardTabWidget(const NMVariantMapList &peerData, QWidget *parent = nullptr, Qt::WindowFlags f = {});

Sorry for being so pedantic :)

39
void loadConfig(const NMVariantMapList &peerData);
44
void slotAddPeerWithData(const QVariantMap &peerData);
libs/models/networkmodelitem.cpp
479–480

There is already WireGuard device in NMQT with KF5 5.58 which you should be able to use.

  • Correct review comments
andersonbruce marked 8 inline comments as done.May 11 2019, 6:57 AM

Update to fix some comments.

libs/declarative/connectionicon.cpp
403

There is still a discrepancy between the enum defined by NetworkManager and the one defined by NMQT. NetworkManager is returning a value of 29 but NetworkManager::Device::WireGuard has a value of 30.

libs/editor/settings/wireguardpeerwidget.cpp
53

SimpleIpV4AddressValidator and SimpleIpV6AddressValidator were intentionally done this way because they were existing functions that I didn't want to break when I added new (defaulted) parameters for use in with WireGuard functionality.

Since changing these two will involve changes to non-WireGuard code and it makes sense to do them all at the same time, can we write this up as a separate Bug?

libs/models/networkmodelitem.cpp
479–480

This was just an extraneous comment so it was removed

jgrulich added inline comments.May 12 2019, 2:43 PM
libs/declarative/connectionicon.cpp
403

This is correct, we are unfortunately +1 because back then there was a device which we add support for and later during the NM development cycle it was renamed and I couldn't just simply rename or remove an enum, because it would break ABI. You can see in NMQT that there is code converting NM device type to NMQT device type so we this coverd.

libs/editor/settings/wireguardpeerwidget.cpp
53

Ok, we can do that in a separate review.

andersonbruce marked an inline comment as done.
  • Always get encrypted data from kwallet

Don't ask user for key values.

  • Don't allow save of configuration with "AlwaysAsk" flag for keys

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

jgrulich added inline comments.May 13 2019, 6:05 AM
kded/secretagent.cpp
449

Shouldn't it be

(isVpn && !isWireguard) && allowInteraction

You don't want to display password dialog in case it's a WG connection.

Or maybe it should stay and instead we should return true and send error (like we do when a dialog has an error) if it's a WG connection, to let NetworkManager know we cannot handle that.

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

  • Return non-WireGuard logic to original state
andersonbruce marked an inline comment as done.May 13 2019, 6:36 AM

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

Great! Should I make the change in this review? If so, what is the correct procedure (with git, arc, ...) to merge that change into this review?

kded/secretagent.cpp
449

Actually what I did is move the WireGuard handling above this 'else if' so it shouldn't get to this point if it is WireGuard so I simply returned the logic to what it was originally.

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

Great! Should I make the change in this review? If so, what is the correct procedure (with git, arc, ...) to merge that change into this review?

Should be enough just to merge current master into your branch I guess.

andersonbruce marked an inline comment as done.May 13 2019, 6:58 AM

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 data is not available in kwallet then trying to make a connection will fail. Also updated the configuration screens to not allow a configuration with "AlwaysAsk" flags on either key.

You can disable "AlwaysAsk" option with PasswordField::setPasswordNotRequiredEnabled(false).

Sorry, that's not it, I added a new option to do that, you can now use PasswordField::setPasswordNotSavedEnabled(false).

Great! Should I make the change in this review? If so, what is the correct procedure (with git, arc, ...) to merge that change into this review?

Should be enough just to merge current master into your branch I guess.

Isn't 'arc diff' going to try to include the differences between the master I originally checked out and the merged current master as changes in the WireGuard review then? Sorry if these are stupid questions, I'm not a git expert and I just don't want to mess things up.

I think it doesn't matter in this case, you can just change it in your code, you don't need to have that change in your local copy, if this is merged than it's applied on top of that so it will be ok.

  • 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.

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.

jgrulich accepted this revision.May 13 2019, 9:12 AM

Good work!!

This revision is now accepted and ready to land.May 13 2019, 9:12 AM
This revision was automatically updated to reflect the committed changes.