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
Details
Details
- Reviewers
jgrulich - Commits
- R116:8fb5c6192c15: Add OTP support for openconnect VPN
Diff Detail
Diff Detail
- Lint
Lint Skipped - Unit
Unit Tests Skipped
Comment Actions
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; | |
| 150 | extra empty line | |
| 163 | const | |
| 241 | missing empty line after this | |
Comment Actions
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
Comment Actions
Corrects minor bug in one call to updateLog. However, all calls to updateLog have to be reviewed if this goes to production
Comment Actions
- 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)
Comment Actions
- 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.
- Your code is full of trailing spaces
- 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 | ||
| 657 | 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. | |
Comment Actions
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.
- Your code is full of trailing spaces
Ah, well, what a curse! I'll get rid of them
- 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.
| 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. | |
Comment Actions
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".
Comment Actions
! 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.
Comment Actions
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 ↗ | (On Diff #55714) | QStringLiteral("manual") Same below. |
| 288 ↗ | (On Diff #55714) | Coding style. The "else if" should be on the same line as the "}" bracket. Same below. |
| vpn/openconnect/openconnectprop.ui | ||
| 181 ↗ | (On Diff #55714) | Use same description NM uses in nm-connection-editor: |
| vpn/openconnect/openconnectwidget.cpp | ||
| 120 ↗ | (On Diff #55714) | QStringLiteral("disabled") Same below. |
| 124 ↗ | (On Diff #55714) | Coding style. The "else if" should be on the same line as the "}" bracket. Same below. |
Comment Actions
I believe I addressed all of your comments. Please check your email for testing options.
Comment Actions
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 | ||
| 36 | I think this combobox should be expanded across whole width. | |
| vpn/openconnect/openconnectwidget.cpp | ||
| 132 | Missing space after "==" | |
Comment Actions
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.