Update L2TP to NetworkManager-l2tp 1.8.0 features
ClosedPublic

Authored by dkosovic on Mar 1 2020, 2:06 PM.

Details

Summary
  • Update to NetworkManager-l2tp 1.8.0 features which include:
    • NetworkManager-l2tp 1.8.0 user certificate support.
    • Machine certificate support.
    • Rename Gateway ID to Remote ID.
    • Phase 1 & Phase 2 Lifetime TimeEdits.
    • Use IP Compression CheckBox.
    • Disable PFS CheckBox.
  • Synchronize nm-l2tp-service.h with NetworkManager-l2tp 1.8.0's nm-service-defines.h.
  • Base new user certificate UI on openvpn UI.
  • Delete l2tpauth.ui and replace with programmatically generated l2tpauthwidget.
  • Add user and machine private key / certificate password support to l2tpauthwidget.
  • Detect if libreswan or strongswan is being used and change UI based on which one is detected, e.g. disable "Disable PFS" check box with strongswan, Phase 1 & Phase 2 Lifetime show default strongswan and libreswan values when unchecked.
  • Disable IPSec settings button if libreswan or strongswan not detected.
  • Remove visibility of PPP Authentication group box if user certificate authentication is selected.
  • Support loading *SWAN Base64 encoded PSK, but for backwards compatibility use unencoded for saving settings.
  • For backwards compatibility with older NetworkManager-l2tp use NM_L2TP_KEY_IPSEC_GATEWAY_ID instead of NM_L2TP_KEY_IPSEC_REMOTE_ID for saving connection settings.

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Lint Skipped
Unit
Unit Tests Skipped
dkosovic created this revision.Mar 1 2020, 2:06 PM
Restricted Application added a project: Plasma. · View Herald TranscriptMar 1 2020, 2:06 PM
Restricted Application added a reviewer: jgrulich. · View Herald Transcript
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
dkosovic requested review of this revision.Mar 1 2020, 2:06 PM
dkosovic added a comment.EditedMar 1 2020, 10:50 PM

This patch depends on D27763 being applied first.

Old l2tp:

Old l2tp IPsec Settings :

New l2tp (username/password) :

New l2tp (certificate) :

New l2tp IPsec Settings (PSK) :

New l2tp IPsec Settings (certificate) :

jgrulich accepted this revision.Mar 2 2020, 3:11 PM

Only nitpicking, otherwise it looks good to me.

vpn/l2tp/l2tpauth.cpp
34

This include doesn't seem to be used.

vpn/l2tp/l2tpipsecwidget.cpp
127

const? Same for variables below.

133

Missing indentation.

143

const? Same for variables below.

170

This can be simplified using:

m_ui->cbIPComp->setChecked(dataMap[NM_L2TP_KEY_IPSEC_IPCOMP] == yesString)

Same above.

259

const?

265

const?

304

const?

vpn/l2tp/l2tppppwidget.cpp
60

const? Same for variables below.

This revision is now accepted and ready to land.Mar 2 2020, 3:11 PM

@dkosovic will you update the review to address my comments?

@dkosovic will you update the review to address my comments?

I agree with all your comments and they are great nitpicks and suggestions.

Sorry I didn't have time tonight to work on testing everything and submitting an updated patch. I'm guessing submitting an updated patch is what I need to do for the review?

@dkosovic will you update the review to address my comments?

I agree with all your comments and they are great nitpicks and suggestions.

Sorry I didn't have time tonight to work on testing everything and submitting an updated patch. I'm guessing submitting an updated patch is what I need to do for the review?

No problem, take your time. I don't know the way you submitted this change, whether you used arc to do so or you just uploaded a patch. With arc you just create another commit and run again:

arc diff

If you uploaded just a patch, then I guess you will need to update it the same way.

ngraham added a subscriber: ngraham.Mar 4 2020, 5:12 AM

-1 for using group boxes. :) I don't think they're needed at all. Just change the "Type:" label to "Authentication:" and that section is clear enough, then just separate the logical sections with whitespace

I'm of mixed minds on the group boxes.

With L2TP/IPsec connection setup instructions for macOS and iOS, they typically have screenshots of the "User Authentication" and "Machine Authentication" settings. Many Linux users try to match what they see for other platforms to what they need to enter. A typical user probably wouldn't know that machine authentication is IPsec authentication, although if it is a pre-shared key it doesn't need to be spelt out, for certificates it can get a bit more confusing between user and machine certificates. But having said that, I did explicitly use "User Certificate" and "Machine Certificate" labels, perhaps most users would realize "Machine Certificate" corresponds to "Machine Authentication"?

I got the idea of using a checkable group box as the existing PPP settings dialog used one, but granted it didn't have nested group boxes. I was trying to imply with the Advanced group box that it is optional and should only be touched if you know what you are doing.

dkosovic updated this revision to Diff 76933.Mar 4 2020, 1:34 PM

updates based on review comments

When I was updating this client's UI, I was pretty conflicted about using group boxes, many of the other VPN clients were using group boxes for their main window and at one stage I was thinking of introducing group boxes for the main window of this client. It was definitely this client's PPP settings dialog which uses multiple group boxes that initially convinced me to use group boxes with the IPsec settings dialog.

I don't know what is best for the UI.

It's just that in general we're trying to move away from group boxes and towards the use of whitespace to logically group sections. It's not a huge deal, but it would be appreciated. :)

I would say it's not a big deal as we use groupboxes quite a lot in plasma-nm. We can get rid of them in future when rewriting the KCM to QML.

This revision was automatically updated to reflect the committed changes.