Re: [NM-openconnect PATCH] Allow 'lasthost' and 'autoconnect' settings.

[David Woodhouse <dwmw2 infradead org>]

On Mon, 2009-03-30 at 23:32 -0400, Dan Williams wrote:
> On Wed, 2009-03-25 at 15:02 +0000, David Woodhouse wrote:
> > I updated the auth-dialog¹ to remember the last VPN server used, and
> > also added a setting to make it automatically start connecting rather
> > than waiting for the user to choose a host (on the basis that the user
> > will probably choose the same host every time, and doesn't want to
> > wait).
> > 
> > I tried using a boolean in gconf for the latter, but NetworkManager
> > doesn't seem to work with that, and the key ends up getting deleted
> > every time I connect. So I reverted to a string containing 'yes' or
> > 'no', which seems to be what's done elsewhere. Should we just fix the
> > configuration handling to cope with real GConf booleans instead?
> 
> Not really, because the types of the variables are specific to the VPN
> daemon, and the code to handle that was pretty icky.  Yes, it means a
> bit of extra validation, but on the flip side it means that *any*
> system-settings plugin can store the VPN's data, not just GConf.  Flat
> text file formats like GKeyFile/.ini have no concept of typed values,
> and even if they did, they wouldn't know what the VPN daemon expected.
> To ensure that we didn't need to do acrobatics to get all that working,
> VPN-specific data is required to be strings and converted as necessary
> within the VPN plugin code itself.

That makes sense.

> svn r 41 (trunk) and 44 (0.7)

Thanks.

> Want to send the auth-dialog patches if you don't get back your svn
> access for a few days?

The auth-dialog isn't in GNOME SVN. It's in the openconnect package
itself. I'm not massively happy with that -- especially right now,
because I update them in lock-step to avoid it bailing out with errors
like:
 nm_vpn_connection_connect_cb(): VPN connection 'Intel VPN' failed to
connect: 'property 'lasthost' invalid or not supported'.

Unfortunately, I don't see an easy way out. Suggestions welcome...

The VPN authentication is done over HTTP. You use a certificate and/or
password to log in using an XML form, and you're rewarded with an HTTP
cookie. That cookie is what's actually used to make the VPN connection
(with an HTTP CONNECT request).

The code to handle the login process and obtain that cookie is shared
between the auth-dialog and openconnect itself. You _can_ invoke
openconnect and give it the cookie directly, as n-m-openconnect does,
but it's also capable of handling the authentication dialogue for
itself.

If it was that simple, I'd just duplicate the damn code in both places.
Unfortunately, there's an added complication: I had to use OpenSSL
instead of GNUTLS. Only OpenSSL handles SSL certificates stored in a
TPM. (And only OpenSSL supports the DTLS protocol too, although that's
more of an issue for openconnect itself and not the authentication bit).

The generic login-handling code also makes use of the nice OpenSSL 'UI'
abstraction -- which allows it to either call back into the pretty GUI
or prompt the user on the console, entirely transparently to it
according to its environment.

Since OpenSSL isn't GPL-compatible, I _can't_ just drop a copy of that
code into n-m-openconnect.

If we put the auth-dialog into the GPL'd n-m-openconnect package where
it belongs, then we'd have to change it to use gnutls (and redo the UI
callback stuff), and more importantly it'd have to lose its TPM support.

As I said... suggestions welcome.

One thing that's tempting is to make nm-openconnect-service _not_ bail
out when it sees a configuration key it doesn't like. At least it's
easier to add stuff in the auth-dialog then, and the patch in $subject
would have purely cosmetic, rather being necessary to restore
functionality:

diff --git a/src/nm-openconnect-service.c b/src/nm-openconnect-service.c
index 68fcd15..bae04ec 100644
--- a/src/nm-openconnect-service.c
+++ b/src/nm-openconnect-service.c
@@ -134,11 +134,7 @@ validate_one_property (const char *key, const char *value, gpointer user_data)
 
 	/* Did not find the property from valid_properties or the type did not match */
 	if (!info->table[i].name) {
-		g_set_error (info->error,
-		             NM_VPN_PLUGIN_ERROR,
-		             NM_VPN_PLUGIN_ERROR_BAD_ARGUMENTS,
-		             "property '%s' invalid or not supported",
-		             key);
+		nm_warning("property '%s' invalid or not supported", key);
 	}
 }
 


-- 
dwmw2