Add OTP support for openconnect VPN
ClosedPublic

Authored by enriquem on Jan 19 2019, 7:49 PM.

Details

Summary

With this patch, One Time Password (OTP) support is added to the openconnect VPN settings and code. It draws the specifications from NetworkManager-openconnect, and presents the same functionality.
The config dialog is modified to include settings for the OTP options
The auth dialog reads and sets the OTP option, updating tokens as needed

Diff Detail

Repository
R116 Plasma Network Management Applet
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
enriquem created this revision.Jan 19 2019, 7:49 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJan 19 2019, 7:49 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
enriquem requested review of this revision.Jan 19 2019, 7:49 PM
enriquem updated this revision to Diff 49906.Jan 19 2019, 9:44 PM
pino added a subscriber: pino.Jan 19 2019, 10:31 PM

Hi Enrique,

I'm not a plasma-nm developer, however I provide some tips & hints regarding your patch.

Other than what I noted already, there are few more things that apply in general:

  • make sure to respect the indentation: each level by 4 spaces with no tabs, space after a comma for function arguments, etc
  • make sure to use curly brackets {...} also for blocks with only 1 line
  • nullptr instead of NULL
vpn/openconnect/openconnectauth.cpp
60–66

IMHO it is better to define the macros in the same way for all the cases, specifying their parameters

225

extra empty line

279

this is needed only in the block within the if in the line below, so move it inside that block; also, make tokenSecret const

298

if tokenSecret is not empty, then its length will always be greater than 0, so the second condition is redundant

301

d->token.tokenSecret is a QByteArray, so if you want to clear it just call clear()

305

it looks like all the other messages for addFormInfo() do not have a trailing newline, so please remove it

360

extra empty line

374

extra empty line

380

extra empty line

vpn/openconnect/openconnectauth.h
58

unrelated change

66

extra empty line

vpn/openconnect/openconnectauthworkerthread.h
95

extra empty line

vpn/openconnect/openconnectprop.ui
181–182

this seems a bit too long as label for a checkbox

208

usually buttons are actions, so IMHO "Show Tokens" is more appropriate

vpn/openconnect/openconnecttoken.ui
14–16

remove these lines (or invoke on this .ui file the fixuifiles script available in kde-dev-scripts)

86

"YubiKey"; also, is "OATH" correct?

vpn/openconnect/openconnectwidget.cpp
48

this include should be placed together with the other Qt includes some lines above

56

are you sure it needs to be mutable?

78

extra empty line

80

tokenWid is not needed, you can just setupUi on the dialog

85

the button box can be added directly to the ui file, and avoid the need to add it manually

122–136

never ever compare to ui strings! they are translatable, so these checks break when using a translation;
a better way is to use the itemData mechanism of a combobox to associate arbitrary stuff (better some values of an enum) to easily get back the information on the selected item;
also, considering that the items are added statically in the ui file and here removed at runtime depending on the available modes, then IMHO it would be better to directly only add the supported modes at runtime here

150

extra empty line

163

const

241

missing empty line after this

enriquem updated this revision to Diff 49911.Jan 19 2019, 11:48 PM

I addressed your comments, except the ones regarding macro definitions, since I am unsure thar applicable versions of openconnect have compatible parameters. I will find out and revisit the comment later

enriquem updated this revision to Diff 49920.Jan 20 2019, 10:03 AM

Amended to remove global tokenModeList

enriquem updated this revision to Diff 49921.Jan 20 2019, 10:04 AM
enriquem updated this revision to Diff 49946.Jan 20 2019, 4:05 PM

Changed the buildup of the "tokens" ComoBox

enriquem updated this revision to Diff 49949.Jan 20 2019, 5:44 PM
enriquem updated this revision to Diff 50279.Jan 25 2019, 7:33 PM

Corrects minor bug in one call to updateLog. However, all calls to updateLog have to be reviewed if this goes to production

enriquem updated this revision to Diff 50665.EditedFeb 1 2019, 6:39 PM
  • Removed updateLog calls
  • Properly initialize d->tokens
  • Change the config dialog so that (a) 'Apply' does not become active when the user clicks on the 'Show Tokens' button, makes no changes, and closes the dialog or clicks Cancel, and (b) when the user clicks 'Cancel', inputs read from the config file are restored
  • Added minimal tooltips (should be improved)
Restricted Application added 1 blocking reviewer(s): jgrulich. · View Herald TranscriptFeb 1 2019, 6:39 PM
enriquem updated this revision to Diff 50698.Feb 2 2019, 7:36 AM

Changed tooltips
No need for a new enum

enriquem updated this revision to Diff 50749.Feb 2 2019, 8:56 PM

Changed tooltips

  1. I'm not sure if the UI for openconnect tokens is correct, I think the QLineEdit for token secret should be on the same line, and you should probably use PasswordField instead? It can be our PasswordField widget from libs/editor/widgets/. Or it's not secret in the same sense as other secrets and it will not need to be saved by secret agent, like rest of passwords? I would also follow nm-connection-editor and make tokens options visible in the main UI, not under specific button.
  2. Your code is full of trailing spaces
  3. How can I try this? Is there any public Openconnect server which I can use to test this?
vpn/openconnect/nm-openconnect-service.h
41

Leftover from the other review?

vpn/openconnect/openconnectauth.cpp
659

Also leftover from the other review.

vpn/openconnect/openconnectwidget.cpp
95

Both disconnects can be removed if you move this all to the main widget.

199

Leftover from the other review.

256

Also leftover from the other review.

enriquem added a comment.EditedApr 7 2019, 11:44 AM
  1. I'm not sure if the UI for openconnect tokens is correct, I think the QLineEdit for token secret should be on the same line, and you should probably use PasswordField instead? It can be our PasswordField widget from libs/editor/widgets/. Or it's not secret in the same sense as other secrets and it will not need to be saved by secret agent, like rest of passwords? I would also follow nm-connection-editor and make tokens options visible in the main UI, not under specific button.

a) I don't see any need for the QComboBox and theQLineEdit to be in the same line, but that's a matter of taste, not functionality. Both fields are sort of independent: same key works with different OTP options.
b) I agree on the PasswordField, although this being an OTP it really does not matter if anyone sees it.
c) No need to save it. It is used and discarded
d) I tried putting all options in the main UI. This made the window too tall for the allocated space, so that resizing was necessary or the main window initial size ought be changed. It looked ugly to me. That's why I opted for a separate dialog. I can change it if you think it is important, but, again, it looks ugly to me.

  1. Your code is full of trailing spaces

Ah, well, what a curse! I'll get rid of them

  1. How can I try this? Is there any public Openconnect server which I can use to test this?

I set up a server in my own Fedora box with ocserv. With some tweaking of the pam modules along the lines of http://ocserv.gitlab.io/www/recipes-ocserv-2fa.html, https://www.nongnu.org/oath-toolkit/pam_oath.html and http://www.infradead.org/openconnect/token.html I was able to test HOTP and TOTP (that is, I pick a random key and use oathtool or FreeOTP). Yubikeys were tricky, since I couldn't validate the OTP. But I modified ocserv to show that the connection scripts were actually providing the correct OTP key. As for RSA, I have no clue as to how to test them, and keys are too expensive for me.

enriquem updated this revision to Diff 55653.Apr 7 2019, 12:02 PM

Hopefully there are no more trailing spaces.

enriquem added inline comments.Apr 7 2019, 12:04 PM
vpn/openconnect/nm-openconnect-service.h
41

Not really. It's a feature in NetworkManager. If the functionality is not needed, it can be dropped.

  1. I'm not sure if the UI for openconnect tokens is correct, I think the QLineEdit for token secret should be on the same line, and you should probably use PasswordField instead? It can be our PasswordField widget from libs/editor/widgets/. Or it's not secret in the same sense as other secrets and it will not need to be saved by secret agent, like rest of passwords? I would also follow nm-connection-editor and make tokens options visible in the main UI, not under specific button.

a) I don't see any need for the QComboBox and theQLineEdit to be in the same line, but that's a matter of taste, not functionality. Both fields are sort of independent: same key works with different OTP options.

All forms in the kcm use "Label: input" so I would like all the options to follow same pattern.

b) I agree on the PasswordField, although this being an OTP it really does not matter if anyone sees it.

Ok, then I guess leave it as it is now.

c) No need to save it. It is used and discarded

Doesn't seem to be true, when I save token secret, I see it is saved in NetworkManager so in this case PasswordField should be used and there should be an option to store it either to NM or secret agent.

d) I tried putting all options in the main UI. This made the window too tall for the allocated space, so that resizing was necessary or the main window initial size ought be changed. It looked ugly to me. That's why I opted for a separate dialog. I can change it if you think it is important, but, again, it looks ugly to me.

In that case use a different button label, something like "Token Authentication".

! In D18394#445411, @jgrulich wrote:

All forms in the kcm use "Label: input" so I would like all the options to follow same pattern.

OK.

b) I agree on the PasswordField, although this being an OTP it really does not matter if anyone sees it.

Ok, then I guess leave it as it is now.

c) No need to save it. It is used and discarded

Doesn't seem to be true, when I save token secret, I see it is saved in NetworkManager so in this case PasswordField should be used and there should be an option to store it either to NM or secret agent.

I was wrong in the two points above. The token secret is needed for automatic generation of the OTP. The OTP will not be shown in the connection dialog.
I changed the behavior, to enable password options, but that conflicts with NetworkManager (I guess this is like any other vpn setting).

d) I tried putting all options in the main UI. This made the window too tall for the allocated space, so that resizing was necessary or the main window initial size ought be changed. It looked ugly to me. That's why I opted for a separate dialog. I can change it if you think it is important, but, again, it looks ugly to me.

In that case use a different button label, something like "Token Authentication".

OK.

enriquem updated this revision to Diff 55714.Apr 7 2019, 9:28 PM

Update per comments above

Sorry for the delay, I promise that I will start reviewing this more frequently so it gets merged just in time for Plasma 5.16 (we have 2 weeks).

One more thing: When I open the "Token authentication" dialog, the "Token secret" label is not aligned with the text input

Otherwise I think it looks good, but I didn't test it as I don't have time to configure an openconnect server.

vpn/openconnect/openconnectauth.cpp
285

QStringLiteral("manual")

Same below.

288

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

vpn/openconnect/openconnectprop.ui
181

Use same description NM uses in nm-connection-editor:
"Prevent user from manually accepting invalid certificates"

vpn/openconnect/openconnectwidget.cpp
120

QStringLiteral("disabled")

Same below.

124

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

enriquem updated this revision to Diff 57294.Tue, Apr 30, 9:29 PM

I believe I addressed all of your comments. Please check your email for testing options.

jgrulich accepted this revision.Thu, May 2, 9:35 AM

Just fix those minor issues, otherwise it looks good to me, I'll verify on Monday, but I believe it's safe to push this.

vpn/openconnect/openconnectauth.cpp
287

Missing space after "==".

293

And here is one additional space after "=="

vpn/openconnect/openconnecttoken.ui
37

I think this combobox should be expanded across whole width.

vpn/openconnect/openconnectwidget.cpp
132

Missing space after "=="

This revision is now accepted and ready to land.Thu, May 2, 9:35 AM
enriquem updated this revision to Diff 57394.EditedThu, May 2, 7:03 PM

I addressed your comments. Sorry for the careless coding in what spaces are concerned. BTW, I added a missing QStringLiteral.

I believe the combobox size constraints are fixed now (although I liked it better before).

And many thanks for your time!

And I could not compile this here. I don't think I introduced any errors, but please be aware.

This revision was automatically updated to reflect the committed changes.