Bug 226028 - No OpenConnect VPN support
Summary: No OpenConnect VPN support
Status: RESOLVED FIXED
Alias: None
Product: knetworkmanager
Classification: Miscellaneous
Component: general (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: NOR wishlist
Target Milestone: ---
Assignee: Will Stephenson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-09 15:27 UTC by David Woodhouse
Modified: 2011-12-16 20:03 UTC (History)
11 users (show)

See Also:
Latest Commit:
Version Fixed In:


Attachments
first attempt at an openconnect plug-in (62.10 KB, patch)
2011-06-25 17:13 UTC, Ilia Kats
Details
second try (61.99 KB, patch)
2011-06-25 23:23 UTC, Ilia Kats
Details
screenshot (207.02 KB, image/png)
2011-06-26 00:11 UTC, Ilia Kats
Details
third try (62.18 KB, patch)
2011-06-26 00:42 UTC, Ilia Kats
Details
final (?) (63.24 KB, patch)
2011-06-26 21:05 UTC, Ilia Kats
Details
secrets working now (62.49 KB, patch)
2011-06-27 08:47 UTC, Ilia Kats
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Woodhouse 2010-02-09 15:27:45 UTC
Version:           0.9.20091220.fc13 (using Devel)
OS:                Linux
Installed from:    Compiled sources

OpenConnect is a client for Cisco's current 'AnyConnect' VPN product, which is replacing the older IPSec-based VPN supported by vpnc.

It looks like knetworkmanager doesn't have support for this yet, and users are asking for it.

The configuration part is relatively simple (for GNOME it was just copied from vpnc and tweaked a little).

The 'needSecrets' part is slightly more complex. Rather than just asking the user for a password, it interacts directly with the VPN/HTTPS server, using SSL certificates and asking the user to fill in arbitrary forms provided by the server, until a successful login is rewarded with an HTTP cookie. It's that cookie which is passed back to the nm-openconnect-server as the 'secret' for the session.

Under GNOME, this is handled by the nm-openconnect-auth-dialog tool which is part of the openconnect package. It reads the settings directly from gconf and will spit out the 'secrets' on stdout.
Comment 1 Jean-Baptiste Carré 2010-05-04 16:28:50 UTC
I confirm that the openconnect support is still missing :-(.
Comment 2 David Woodhouse 2010-05-04 17:00:09 UTC
It shouldn't be hard to fix -- you don't need much of a clue. The 'slightly more complex' part I mentioned above is all implemented in openconnect itself -- you can use it as a library.
Comment 3 Jean-Baptiste Carré 2010-05-04 17:18:18 UTC
Right, but I should first learn some basics in programming in order to do that -> I'm just a normal user currently ;-)
My point is just that I can't use openconnect inside knetwork-manager for now :-), can't I?
Comment 4 David Woodhouse 2010-05-29 01:06:26 UTC
*** This bug has been confirmed by popular vote. ***
Comment 5 Reuben 2010-11-12 08:36:46 UTC
I took a look at this in the source package in Ubuntu 10.10, plasma-widget-networkmanagement.

I can add my own configuration dialog and set up my openconnect VPN connection using the plasma widget.  This is the "relatively simple" part David mentioned.  Of course, I can't actually connect, because openconnect ends up being called without a VPN session cookie having been obtained first.

To see how the "slightly more complex" part might work, I studied the Gnome equivalent for comparison.  It seems that the Gnome network management tool nm-applet knows about an "auth-dialog" setting that's stored in files like /etc/NetworkManager/VPN/nm-openconnect-service.name.  The [GNOME] section of the file sets auth-dialog to point to nm-openconnect-auth-dialog so that nm-applet will use this for authentication before starting openconnect.

However, it doesn't look like the KDE plasma widget has an equivalent to this.  In fact, some of the existing VPN plugins in plasma-widget-networkmanagement have UI files for authentication dialogs, but as far as I can tell they are not compiled or installed anywhere.

I'm guessing this is something that the developers are planning to add support for later, but that a VPN plugin doesn't currently have a way to launch an authentication dialog on each connection attempt.

Unless I'm missing something...?
Comment 6 David Woodhouse 2010-11-12 11:33:58 UTC
If the existing VPN support cannot ask the user for a password, does that mean it doesn't work at all for *any* type of VPN?

Or are you required to pre-configure your password and store it on disk rather than entering it on demand? 

If it has support for entering a password on demand, you could make the "relatively simple" bits work for now by considering the login cookie to be the 'password'. Run 'openconnect --cookieonly' manually to obtain that cookie, then provide it to the NetworkManager UI with cut/paste.

Not the ideal user interface, but it would work. And then once that's working we can look at how to do it automatically.

Be *very* careful with those cookies, btw; they grant access to your VPN session.
Comment 7 Reuben 2010-11-12 15:35:50 UTC
I don't have another type of VPN to connect to, so I can't directly try it out.  However, it looks to me as if the code requires you to pre-configure the password and store it on disk.  This thread seems to confirm that for the OpenVPN plugin, there's no support for an auth dialog at connection time:

http://old.nabble.com/KNetworkManager-OpenVPN-private-key-password-td29637903.html
Comment 8 Reuben 2010-11-12 21:51:12 UTC
OK, after poking around some more, I found a hopeful hint.  There's some logic in libs/ui/connectionsecretsjob.cpp that gets called when you connect and decides whether to load stored secrets or prompt the user.  If the overall secrets storage is set to "never store, always prompt," then my settings gui gets launched again each time the VPN is connected.  So it should be possible to get an auth dialog when one is needed.
Comment 9 Andrey Borzenkov 2010-11-26 17:34:17 UTC
You could also look at bug 24416 for alternative auth dialogue framework. Works for me for vpnc.
Comment 10 Reuben 2010-11-26 17:41:11 UTC
(In reply to comment #9)

I think you must have had a different bug in mind.  The link to bug 24416 refers to message display in kmail.
Comment 11 Andrey Borzenkov 2010-11-26 17:53:58 UTC
Oops. Bug 244416 of course.
Comment 12 David Woodhouse 2011-03-10 01:16:32 UTC
OpenConnect v3.01 was released today. It splits the authentication code out into a library which can be easily used from GUI tools. The GNOME auth dialog has thus been removed from openconnect itself, and moved into NetworkManager-openconnect instead.

It should be relatively simple to use it from knetworkmanager.
Comment 13 Andrey Borzenkov 2011-03-17 17:36:48 UTC
(In reply to comment #12)
> OpenConnect v3.01 was released today. It splits the authentication code out
> into a library which can be easily used from GUI tools.
> 
> It should be relatively simple to use it from knetworkmanager.

Could you add openconnect_vpninfo_free() to the library? KNM authenticators are implemented as plugins and really need to clean up behind itself.
Comment 14 David Woodhouse 2011-03-17 20:12:03 UTC
(In reply to comment #13)
> Could you add openconnect_vpninfo_free() to the library? KNM authenticators are
> implemented as plugins and really need to clean up behind itself.

http://git.infradead.org/users/dwmw2/openconnect.git/commitdiff/b5c1f4c0 ?
Comment 15 Andrey Borzenkov 2011-03-17 20:27:14 UTC
Wow, it was fast :) 

Should not these be reversed?

+       free((void *)vpninfo->cert);
+       if (vpninfo->cert != vpninfo->sslkey)
+               free((void *)vpninfo->sslkey);

Thank you!
Comment 16 David Woodhouse 2011-03-17 21:59:02 UTC
I probably should reverse those, just for appearances' sake. But in fact it's fine as it is. I'm not *dereferencing* the cert pointer after I free it; I'm only comparing it with the sslkey pointer, so that I don't free the same memory twice if they're the same.
Comment 17 David Woodhouse 2011-04-24 16:34:24 UTC
OpenConnect v3.02 has been released, with the openconnect_vpninfo_free() function in libopenconnect.

See http://git.gnome.org/browse/network-manager-openconnect/commit/?id=fd4561ac4 for documentation on what this auth-dialog needs to do.
Comment 18 Ilia Kats 2011-06-25 17:13:29 UTC
Created attachment 61326 [details]
first attempt at an openconnect plug-in

Could you please test the attached patch? It _should_ work, but I  have no way of testing it myself. This is against the nm09 branch of KDE NM and requires openconnect >= 3.02 to build.
Comment 19 David Woodhouse 2011-06-25 21:49:27 UTC
vpnplugins/openconnect/openconnectwidget.cpp:26:32: fatal error: ui_openconnectprop.h: No such file or directory
Comment 20 Ilia Kats 2011-06-25 22:40:10 UTC
ui_openconnectprop.h should be generated from openconnectprop.ui at compile time. If that doesn't work, you can run
uic openconnectprop.ui > ui_openconnectprop.h
Comment 21 David Woodhouse 2011-06-25 22:55:15 UTC
The problem is that it's in the x86_64-redhat-linux-gnu/vpnplugins/openconnect directory, but you removed that directory from the include path.

Fix that, and there is confusion between your openconnect.h and the one from libopenconnect. Working on that now...

In the meantime, these fixes were also necessary (although perhaps not the best fix):
--- networkmanagement-0.9/vpnplugins/openconnect/openconnectauth.cpp~   2011-06-25 22:47:16.000000000 +0100
+++ networkmanagement-0.9/vpnplugins/openconnect/openconnectauth.cpp    2011-06-25 23:43:06.000000000 +0100
@@ -380,7 +380,7 @@ void OpenconnectAuthWidget::processAuthF
             widget = qobject_cast<QWidget*>(cmb);
         }
         if (widget) {
-            widget->setProperty("openconnect_opt", (int)opt);
+            widget->setProperty("openconnect_opt", (unsigned long long)opt);
             layout->addRow(text, widget);
         }
     }
@@ -389,8 +389,8 @@ void OpenconnectAuthWidget::processAuthF
     box->addButton(QDialogButtonBox::Ok)->setText(QLatin1String("Login"));
     box->addButton(QDialogButtonBox::Cancel);
     d->ui.loginBoxLayout->addWidget(box);
-    box->setProperty("loginClickedPtr", (int)loginClicked);
-    box->setProperty("openconnect_form", (int)form);
+    box->setProperty("loginClickedPtr", (unsigned long long)loginClicked);
+    box->setProperty("openconnect_form", (unsigned long long)form);
 
     connect(box, SIGNAL(accepted()), this, SLOT(formLoginClicked()));
     connect(box, SIGNAL(rejected()), this, SLOT(formCancelClicked()));
@@ -468,14 +468,14 @@ void OpenconnectAuthWidget::formLoginCli
 {
     Q_D(OpenconnectAuthWidget);
     QLayout *layout = d->ui.loginBoxLayout->itemAt(0)->layout();
-    bool *loginClicked = (bool*) d->ui.loginBoxLayout->itemAt(1)->widget()->property("loginClickedPtr").toInt();
-    struct oc_auth_form *form = (struct oc_auth_form *) d->ui.loginBoxLayout->itemAt(1)->widget()->property("o").toInt();
+    bool *loginClicked = (bool*) d->ui.loginBoxLayout->itemAt(1)->widget()->property("loginClickedPtr").toLongLong();
+    struct oc_auth_form *form = (struct oc_auth_form *) d->ui.loginBoxLayout->itemAt(1)->widget()->property("o").toLongLong();
 
     for (int i = 0; i < layout->count(); i++) {
         QLayoutItem *item = layout->itemAt(i);
         QWidget *widget = item->widget();
         if (widget && widget->property("openconnect_opt").isValid()) {
-            struct oc_form_opt *opt = (struct oc_form_opt *) widget->property("openconnect_opt").toInt();
+            struct oc_form_opt *opt = (struct oc_form_opt *) widget->property("openconnect_opt").toLongLong();
             if (opt->type == OC_FORM_OPT_PASSWORD || opt->type == OC_FORM_OPT_TEXT) {
                 KLineEdit *le = qobject_cast<KLineEdit*>(widget);
                 QByteArray text = le->text().toAscii();
@@ -503,7 +503,7 @@ void OpenconnectAuthWidget::formLoginCli
 void OpenconnectAuthWidget::formCancelClicked()
 {
     Q_D(OpenconnectAuthWidget);
-    bool *loginClicked = (bool*) d->ui.loginBoxLayout->itemAt(1)->widget()->property("loginClickedPtr").toInt();
+    bool *loginClicked = (bool*) d->ui.loginBoxLayout->itemAt(1)->widget()->property("loginClickedPtr").toLongLong();
     *loginClicked = false;
     deleteAllFromLayout(d->ui.loginBoxLayout);
     d->workerWaiting.wakeAll();
Comment 22 David Woodhouse 2011-06-25 23:00:05 UTC
I renamed your openconnect.h to knm_openconnect.h and changed openconnect.cpp to include that instead, and now I get:
/usr/lib64/ccache/c++  -fPIC -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m64 -mtune=generic  -Wnon-virtual-dtor -Wno-long-long -ansi -Wundef -Wcast-align -Wchar-subscripts -Wall -W -Wpointer-arith -Wformat-security -fno-exceptions -DQT_NO_EXCEPTIONS -fno-check-new -fno-common -Werror=return-type -Woverloaded-virtual -fno-threadsafe-statics -fvisibility=hidden -fvisibility-inlines-hidden -O2 -DNDEBUG -DQT_NO_DEBUG -Wl,--enable-new-dtags -Wl,--fatal-warnings -Wl,--no-undefined -lc  -shared -Wl,-soname,networkmanagement_openconnectui.so -o ../../lib/networkmanagement_openconnectui.so CMakeFiles/networkmanagement_openconnectui.dir/networkmanagement_openconnectui_automoc.o CMakeFiles/networkmanagement_openconnectui.dir/openconnect.o CMakeFiles/networkmanagement_openconnectui.dir/openconnectwidget.o CMakeFiles/networkmanagement_openconnectui.dir/openconnectauth.o CMakeFiles/networkmanagement_openconnectui.dir/openconnectauthworkerthread.o -L/usr/lib64/kde4/devel -L/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/x86_64-redhat-linux-gnu/lib /usr/lib64/libkio.so.5.6.0 ../../lib/libknminternals.so.4.6.0 ../../lib/libknmui.so.4.6.0 -lopenconnect -lproxy -lssl -lcrypto -ldl -lz -lxml2 -ldbus-1 -lpthread -lrt -lssl /usr/lib64/libkio.so.5.6.0 /usr/lib64/libQtNetwork.so /usr/lib64/libQtXml.so ../../lib/libknminternals.so.4.6.0 /usr/lib64/libkdeui.so.5.6.0 /usr/lib64/libQtGui.so /usr/lib64/libQtSvg.so /usr/lib64/libkdecore.so.5.6.0 /usr/lib64/libQtDBus.so /usr/lib64/libQtCore.so -lpthread -Wl,-rpath,/usr/lib64/kde4/devel:/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/x86_64-redhat-linux-gnu/lib: -Wl,-rpath-link,/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/x86_64-redhat-linux-gnu/lib 
CMakeFiles/networkmanagement_openconnectui.dir/openconnect.o: In function `void KPluginFactory::registerPlugin<OpenconnectUiPlugin>(QString const&, QObject* (*)(QWidget*, QObject*, QList<QVariant> const&))':
/usr/include/kde4/KDE/../kpluginfactory.h:401: undefined reference to `OpenconnectUiPlugin::staticMetaObject'
CMakeFiles/networkmanagement_openconnectui.dir/openconnect.o: In function `OpenconnectUiPlugin':
/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/vpnplugins/openconnect/openconnect.cpp:32: undefined reference to `vtable for OpenconnectUiPlugin'
CMakeFiles/networkmanagement_openconnectui.dir/openconnect.o: In function `~OpenconnectUiPlugin':
/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/vpnplugins/openconnect/openconnect.cpp:37: undefined reference to `vtable for OpenconnectUiPlugin'
/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/vpnplugins/openconnect/openconnect.cpp:37: undefined reference to `vtable for OpenconnectUiPlugin'
CMakeFiles/networkmanagement_openconnectui.dir/openconnect.o: In function `OpenconnectUiPlugin':
/ssd/fedora/kde-plasma-networkmanagement/f15/networkmanagement-0.9/vpnplugins/openconnect/openconnect.cpp:32: undefined reference to `vtable for OpenconnectUiPlugin'
collect2: ld returned 1 exit status
make[2]: *** [lib/networkmanagement_openconnectui.so] Error 1
Comment 23 Ilia Kats 2011-06-25 23:23:25 UTC
Created attachment 61329 [details]
second try

I've updated the patch to account for the compiling errors, however that linker line is taken straight from all the other vpn plugins (with added libopenconnect and libssl), it should work (and it works here). Can you compile, say, the vpnc plugin?
Comment 24 David Woodhouse 2011-06-25 23:50:25 UTC
I got it building with http://david.woodhou.se/openconnect2.patch

Not the nicest solution, but at least it builds :)

I'm running Fedora 15, and have applied your patch on top of the Fedora package, which is networkmanagement-0.9-nm09-20110616.tar.bz2

I see a warning that we need NetworkManager >= 0.8.9997 to work, but found only 0.8.999. Is that relevant? I'm guessing that would be there in the standard Fedora package too?

More importantly, connecting to the VPN doesn't work. It just immediately says "Connection Intel AnyConnect VPN failed". It doesn't tell me *what* failed, or how. I only see this in .xsession-errors when I try:
QPixmap::scaled: Pixmap is a null pixmap

If you don't have a VPN account you can test with, presumably you ought to be able to get at least this far, and at least the dialog box should come up if you give it a fairly random configuration? Am I doing something wrong? Do I *need* to install the latest version from git?

(Thanks for doing this, by the way)
Comment 25 David Woodhouse 2011-06-25 23:59:11 UTC
Weird. I installed the vpnc plugin and tested that *that* was working, and it was (at least, the dialog came up). So I tried openconnect again and this time it did actually come up.

Seemed to work relatively well, prompted me with the forms from the server, and then crashed just as it should have been successful:


#6  0x00000034e076b7b3 in QObject::property(char const*) const () from /usr/lib64/libQtCore.so.4
#7  0x00007f371591a3a9 in OpenconnectAuthWidget::formLoginClicked (this=0xcc8d20) at /usr/src/debug/networkmanagement-0.9/vpnplugins/openconnect/openconnectauth.cpp:471
#8  0x00007f3715913aac in OpenconnectAuthWidget::qt_metacall (this=0xcc8d20, _c=QMetaObject::InvokeMetaMethod, _id=<optimized out>, _a=0x7fff96e09460) at /usr/src/debug/networkmanagement-0.9/x86_64-redhat-linux-gnu/vpnplugins/openconnect/moc_openconnectauth.cpp:95
#9  0x00000034e076ceca in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib64/libQtCore.so.4
Comment 26 Ilia Kats 2011-06-26 00:11:03 UTC
Created attachment 61332 [details]
screenshot

The 0.8.9997 warning is because we are including (not using yet, just including in our source files) a dbus method which got added in NM 0.8.9997, but this can be ignored for now.

I can come as far as the server certificate verification window, I've attached a screenshot. Can you look at /var/log/daemon.log or whereever NM writes it's information in Fedora, if something useful is there. There haven't been that many changes in git since 2011-06-16, so I don't think that's the problem. The connection failing immediately can have one of two causes:
a) our secret agent doesn't register with NM or NM is ignoring it (which I find hard to believe)
b) KDE NM can't find the vpn plugin. You can run kbuildsycoca4 to refresh the cache for plugins, and/or restart kded4.
Comment 27 David Woodhouse 2011-06-26 00:12:22 UTC
As you'll probably have guessed, the crash is immediately on clicking the Login button; it doesn't get as far as actually sending the login details to the server.

You ought to be able to reproduce this by pointing your configuration at *any* AnyConnect server; you don't need a valid username/password. So just pointing it at something like ucbvpn.berkeley.edu should suffice.
Comment 28 David Woodhouse 2011-06-26 00:18:06 UTC
Note: in your 'second try' patch you are still using toInt() in OpenconnectAuthWidget::formCancelClicked(), and truncating a 64-bit pointer into 32 bits.
Comment 29 Ilia Kats 2011-06-26 00:42:52 UTC
Created attachment 61333 [details]
third try

Thanks, I didn't know any AnyConnect server until now.

Anyway, I've updated the pointer handling code to use Qt's quintptr type, which is portable. Also the crash should be fixed now (I forgot to take into account the messages displayes above the login form when retrieving the UI elements by index)
Comment 30 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 31 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 32 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 33 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 34 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 35 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 36 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 37 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 38 David Woodhouse 2011-06-26 01:25:02 UTC
Works with the fix below; thanks!

--- openconnectauth.cpp~	2011-06-26 02:06:05.000000000 +0100
+++ openconnectauth.cpp	2011-06-26 02:16:09.000000000 +0100
@@ -240,6 +240,6 @@ void OpenconnectAuthWidget::writeConfig(
     QStringMap secretData;
 
     QString host(openconnect_get_hostname(d->vpninfo));
-    QString port(openconnect_get_port(d->vpninfo));
+    QString port = QString::number(openconnect_get_port(d->vpninfo));
     secretData.insert(QLatin1String(NM_OPENCONNECT_KEY_GATEWAY), host + ":" + port);
 
However, none of the 'settings', like the autoconnect flag or my username or the accepted certificate signatures, ever seem to get saved. In /var/log/messages I see a lot of this:

Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying david.woodhouse@intel.com
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying 213.190.153.58
Jun 26 02:07:13 i7 NetworkManager[979]: destroy_one_secret: destroying no
Comment 39 David Woodhouse 2011-06-26 01:26:29 UTC
Crap, sorry about that; it seemed to be failing to post so I was just trying again. And shouldn't it have complained about a mid-air collision or told me I'd already submitted that comment?
Comment 40 David Woodhouse 2011-06-26 01:29:22 UTC
Are you happy with the libopenconnect API, by the way? If so, I'll go ahead and turn that into a proper *shared* library now there's a second independent user of it.
Comment 41 Ilia Kats 2011-06-26 12:53:56 UTC
Way to spam my inbox, man ;)

To be honest, I have no idea about the destroy_one_secret part. The settings clearly reach NM if the connection is successful. (I just figured out why the username didn't get saved, but this didn't affect autoconnect) If you open /etc/NetworkManager/system-connections/name_of_your_connection (as root, only root has read permissions), can you see the autoconnect entry there in the [vpn] section?

The API is useable, yes, but happy is such a strong word;) Interfacing C libraries which expect function pointers with C++ is always a fuzz, as you can't just pass methods, since these implicitly contain a pointer to the object, so the signatures of the method and what your openconnect_vpninfo_new expects don't match, so one has to create a static wrapper around the object...
But I don't see how that can be changed without making life for both auth dialogs a lot more complicated, so go ahead. One request though: if/when the API changes, could you send an email to kde-networkmanager@kde.org ? We can't keep track of every change in every component that we can't even test properly.
Comment 42 David Woodhouse 2011-06-26 20:08:15 UTC
I had been using this before with GNOME, and only switched to KDE to test this. So those secrets were already there. However, there is a new one:
     form:%1:%2=david.woodhouse@intel.com

I suspect that's what you meant when you said you figured out why the username was not saved? :)

It does at least confirm that things *are* being saved. But the dialog is not picking up *any* of the settings that exist there. I'll poke at the NM side and see what I can find out.

On the API... I note that my callback functions don't give you *any* 'private' data; only the struct openconnect_info *. You have to be able to find your own class/object/etc from that, without even the benefit of keeping a ->private pointer of your own in the openconnect_info struct. How *do* you make that work, if there are two simultaneous connections to OpenConnect VPNs in progress at the same time?

I think we can fix this for you in a way that's even binary-compatible with existing auth-dialogs. We can just add an openconnect_vpninfo_new_with_cbdata() function, which gives your 'self' pointer, and your callbacks are invoked with *that* as the first argument instead of with the vpninfo pointer.

Does http://david.woodhou.se/callback-privdata.patch seem sane?
Comment 43 Ilia Kats 2011-06-26 21:05:06 UTC
Created attachment 61349 [details]
final (?)

Yes, that's exactly what I meant.
Forget the NM side, I was trying to pick those settings up from VPNSettings, not VPNSecrets, sorry about that. Should be fixed now. Attached patch uses your libopenconnect patch, please test.
Comment 44 David Woodhouse 2011-06-26 21:50:36 UTC
Hrm, you still have the OpenconnectAuthStaticWrapper class. I thought the point was that you could just pass (for example) the address of OpenconnectAuthWorkerThread::writeNewConfig() to openconnect_vpninfo_new() and not need the wrapper at all?
Comment 45 Ilia Kats 2011-06-26 22:11:03 UTC
Tried that, won't work. For example, using
m_openconnectInfo = openconnect_vpninfo_new_with_cbdata((char*)"OpenConnect VPN Agent (NetworkManager - running on KDE)",
                                         &OpenconnectAuthWorkerThread::validatePeerCert,
                                         OpenconnectAuthStaticWrapper::writeNewConfig,
                                         OpenconnectAuthStaticWrapper::processAuthForm,
                                         OpenconnectAuthStaticWrapper::writeProgress,
                                         this);
results in:
/home/ilia/networkmanagement/vpnplugins/openconnect/openconnectauthworkerthread.cpp: In constructor ‘OpenconnectAuthWorkerThread::OpenconnectAuthWorkerThread(QMutex*, QWaitCondition*, bool*)’:
/home/ilia/networkmanagement/vpnplugins/openconnect/openconnectauthworkerthread.cpp:81:46: error: no matches converting function ‘validatePeerCert’ to type ‘openconnect_validate_peer_cert_fn {aka int (*)(void*, struct x509_st*, const char*)}’
/home/ilia/networkmanagement/vpnplugins/openconnect/openconnectauthworkerthread.h:40:10: error: candidates are: void OpenconnectAuthWorkerThread::validatePeerCert(const QString&, const QString&, const QString&, bool*)
/home/ilia/networkmanagement/vpnplugins/openconnect/openconnectauthworkerthread.h:51:9: error:                 int OpenconnectAuthWorkerThread::validatePeerCert(x509_st*, const char*)

The same with all other spellings of validatePeerCert. As far as I understand ([1], [2], and a lot more), you have to specify the class when typedef'ing a pointer to a method, like
typedef int (OpenconnectAuthWorkerThread::*openconnect_validate_peer_cert_fn) (void *privdata,
						  struct x509_st *cert,
						  const char *reason);

[1] http://en.wikipedia.org/wiki/Function_pointer#Method_pointers
[2] http://www.parashift.com/c++-faq-lite/pointers-to-members.html
Comment 46 David Woodhouse 2011-06-27 02:10:01 UTC
OK, I've pushed the openconnect changes and turned it into a shared library.

It could do with a little more vertical space by default: http://david.woodhou.se/vpncluttered.png

If I enter my username, tab, password, enter, then the 'enter' seems to be activating the 'close' button instead of the 'login' button? That means it aborts, instead of connecting :)

And most importantly: even with your latest patch, the secrets we use for configuration still aren't being loaded when the dialog opens. Is that definitely working for you? Perhaps it's because I'm applying your patch to a snapshot dated 20110616?
Comment 47 Ilia Kats 2011-06-27 08:47:17 UTC
Created attachment 61361 [details]
secrets working now

The secrets should be definitely fixed now, the problem was that the plug-in didn't event get them, as the system secrets got overwritten by an empty set that was stored by the secret agent. Also, the login button is now the default one. I know that it's cluttered, I'm still trying to figure out how to make the dialog resize automatically instead of squeezing the input boxes.
Comment 48 David Woodhouse 2011-06-27 09:31:17 UTC
Excellent; thanks. Loading secrets is working now, and this stops it asking me about the server's certificate every time. Note that we'll also need to *write* the tab-separated "certsigs" secret if we accept a new one. I haven't done that bit.

--- openconnectauth.cpp~	2011-06-27 10:08:34.000000000 +0100
+++ openconnectauth.cpp	2011-06-27 10:18:50.000000000 +0100
@@ -163,8 +163,10 @@ void OpenconnectAuthWidget::readSecrets(
 {
     Q_D(OpenconnectAuthWidget);
     d->secrets = d->setting->vpnSecrets();
-    if (!d->secrets[NM_OPENCONNECT_KEY_GWCERT].isEmpty()) {
-        d->certificateFingerprints.append(d->secrets[NM_OPENCONNECT_KEY_GWCERT]);
+    if (!d->secrets["certsigs"].isEmpty()) {
+        QStringList sl = d->secrets["certsigs"].split('\t', QString::SkipEmptyParts);
+	
+        d->certificateFingerprints.append(sl);
     }
     if (!d->secrets["xmlconfig"].isEmpty()) {
         unsigned char sha1[SHA_DIGEST_LENGTH];

NM spewing noise about destroy_one_secret() is not our problem; that's fixed upstream in NetworkManager commit 78ce08884.

My only remaining nitpicks are the dialog size as already mentioned, and perhaps it would be nice to automatically set keyboard focus to the first form input field which isn't prepopulated — which means in the common case that you can just type your password and hit enter.

There is also a complaint of 'QFormLayout::takeAt: Invalid index 0' from kded4 every time OpenconnectAuthWidget::deleteAllFromLayout() is called. This can be fixed as follows:
@@ -527,7 +529,8 @@ void OpenconnectAuthWidget::workerFinish
 
 void OpenconnectAuthWidget::deleteAllFromLayout(QLayout *layout)
 {
-    while (QLayoutItem *item = layout->takeAt(0)) {
+    while (!layout->isEmpty()) {
+        QLayoutItem *item = layout->takeAt(0);
         if (QLayout *itemLayout = item->layout()) {
             deleteAllFromLayout(itemLayout);
             itemLayout->deleteLater();
Comment 49 David Woodhouse 2011-06-27 09:45:05 UTC
Writing certsigs might look something like this, although I know little about QString handling. Can I just do that assignment in the isEmpty() case, and is there a simpler way to append the tab and the new fingerprint in one go?

@@ -454,6 +456,12 @@ void OpenconnectAuthWidget::validatePeer
         dialog.setMainWidget(widget);
         if(dialog.exec() == KDialog::Yes) {
             d->certificateFingerprints.append(fingerprint);
+	    if (d->secrets["certsigs"].isEmpty())
+		    d->secrets["certsigs"] = fingerprint;
+	    else {
+		    d->secrets["certsigs"].append("\t");
+		    d->secrets["certsigs"].append(fingerprint);
+	    }
             *accepted = true;
         } else {
             *accepted = false;
Comment 50 Ilia Kats 2011-06-27 10:03:17 UTC
The comment about an invalid layout index doesn't need to worry us, takeAt returns a null item if there are no more items left, and the while loop just stops. (actually, that is basically the sample code provided in the Qt documentation: http://doc.trolltech.com/latest/qlayout.html#takeAt , so I doubt anything can go wrong here) I'd do the certsigs bit like this:
change the type of OpenconnectAuthWidgetPrivate::certificateFingerprints to QStringList, then for writing:

secretData.insert(QLatin1String("certsigs"), d->certificateFingerprints.join("\t"));

and for reading:

if (!d->secrets["certsigs"].isEmpty()) {
    d->certificateFingerprints.append(d->secrets["cersigs"].split("\t"));
}
d->certificateFingerprints.removeDuplicates();
Comment 51 David Woodhouse 2011-06-27 14:58:26 UTC
Ilia, thank you very much for fixing this. Will you commit it to git?

Please make it depend on openconnect >= 3.03; I'll make a new release as soon as I've done some non-Linux testing of the build system changes.
Comment 52 Ilia Kats 2011-06-27 18:23:01 UTC
Yes, I will, as soon as Lamarque approves ( https://git.reviewboard.kde.org/r/101788/ ). Thanks for the API change, by the way.
Comment 53 Ilia Kats 2011-06-28 05:39:43 UTC
Git commit 7d066ff70da5b0a5d0564b947bcfe951fb2b19af by Ilia Kats.
Committed on 22/06/2011 at 00:37.
Pushed by iliakats into branch 'nm09'.

Add an OpenConnect VPN plugin. This requires OpenConnect >= 3.03
to build. 3.03 isn't released yet, the important part is commit
423eee0b51a204562d6f2ec67893133ebcf200d6 from OpenConnect git
(http://git.infradead.org/users/dwmw2/openconnect.git/ )
OpenConnect support is optional, it just won't build and install
the plugin if OpenConnect 3.03 isn't found.

REVIEW:101788
BUG: 226028

M  +3    -1    libs/internals/settings/vpnsecrets.cpp     
M  +3    -1    libs/ui/connectionsecretsjob.cpp     
M  +5    -0    libs/ui/vpnuiplugin.cpp     
M  +2    -0    libs/ui/vpnuiplugin.h     
M  +1    -1    plasma_nm_version.h     
M  +1    -0    vpnplugins/CMakeLists.txt     
A  +31   -0    vpnplugins/openconnect/CMakeLists.txt     
A  +38   -0    vpnplugins/openconnect/FindOpenConnect.cmake     
A  +89   -0    vpnplugins/openconnect/README     
A  +17   -0    vpnplugins/openconnect/networkmanagement_openconnectui.desktop     
A  +47   -0    vpnplugins/openconnect/nm-openconnect-service.h         [License: GPL (v2+)]
A  +598  -0    vpnplugins/openconnect/openconnectauth.cpp         [License: BSD]
A  +74   -0    vpnplugins/openconnect/openconnectauth.h         [License: BSD]
A  +201  -0    vpnplugins/openconnect/openconnectauth.ui     
A  +171  -0    vpnplugins/openconnect/openconnectauthworkerthread.cpp         [License: BSD]
A  +61   -0    vpnplugins/openconnect/openconnectauthworkerthread.h         [License: BSD]
A  +159  -0    vpnplugins/openconnect/openconnectprop.ui     
A  +74   -0    vpnplugins/openconnect/openconnectui.cpp         [License: BSD]
A  +48   -0    vpnplugins/openconnect/openconnectui.h         [License: BSD]
A  +105  -0    vpnplugins/openconnect/openconnectwidget.cpp         [License: BSD]
A  +51   -0    vpnplugins/openconnect/openconnectwidget.h         [License: BSD]

http://commits.kde.org/networkmanagement/7d066ff70da5b0a5d0564b947bcfe951fb2b19af
Comment 54 David Woodhouse 2011-06-30 00:08:06 UTC
I've just released OpenConnect v3.10
Comment 55 Jason Martin 2011-12-16 11:39:12 UTC
For someone that wants to have the plugin work now, can they compile the latest version of openconnect and it should work? Are there any special tweaks that need to be done in order for it to work?

Are there plans to get at least openconnect 3.10 into Ubuntu so this will be fixed by the next version of Kubuntu?

Thanks for all the hard work!
Comment 56 Rex Dieter 2011-12-16 12:39:26 UTC
OpenConnect should be supported now by recent kde networkmanagement code.

As for distro questions, well, ask your disto.
Comment 57 David Woodhouse 2011-12-16 20:03:45 UTC
This should all be working now, and OpenConnect 3.15 is available. Distributions should be shipping at *least* 3.12, which fixes a compatibility issue with current Cisco ASA firmware.

If this *isn't* working in your distribution of choice, please file bugs there.

It's certainly working in Fedora 15/16, packages are available for openSUSE 12.1, and I'm not entirely sure about Debian-derived distros.