flimflam: Add L2TP/IPsec VPN plugin

flimflam: Add L2TP/IPsec VPN plugin

Launches and interacts with the l2tpipsec_vpn manager which we have added in chromeos-base/vpn-manager.

Adds capability of creating providers which handle their own tunnel device creation (aka pppd) by introducing VPN_FLAG_NO_TUN.  l2tpipsec is one such provider.

Supports allowing provider tasks receive dbus messages to which they can return a response in order to enable username/password requests from pppd.

Change-Id: I7dda866bd2969d691ac67a0830a9dc45f055509c

BUG=chromium-os:11814
TEST=run connect-vpn with openvpn and l2tpipsec kind

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=52600f8

[kmixter1, 2011-03-03 17:16:41]

PTAL at this early code - not fully functional yet, but appreciate any comments
on general approach and handling of interfacing coming up.

[kmixter1, 2011-03-04 02:06:02]

Some updates... gets to li_notify but fails there now.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c
File plugins/l2tpipsec.c (right):

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode117
plugins/l2tpipsec.c:117: ipaddr.peer = g_strdup(value);
I'm getting 0.0.0.0 from the pppd plugin.  Trying to get at a real peer address.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode147
plugins/l2tpipsec.c:147: connman_provider_ipconfig_set(provider, &ipaddr);
So this is failing... it fails because ipv4config is NULL.  I believe it's NULL
because much earlier during vpn_connect connman_ipconfig_create failed because
the index it was passed was negative.  That index was provider->element.index
(in __connman_provider_ipconfig_init).  I suspect that's because
connman_provider_set_index was not called.  Probably it would have been called
as part of vpn_create_tun but of course I hav vpn_driver.flags =
VPN_FLAGS_NO_TUN.  The ppp tunnel would actually be known at this point (ifname
== "ppp0") but it's too late.  So I'm not sure how this was supposed to work
upstream...

[Sam Leffler, 2011-03-04 17:47:45]

just a light read...

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c
File plugins/l2tpipsec.c (right):

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode56
plugins/l2tpipsec.c:56: passwd == NULL || strlen(passwd) == 0)
debug msg or connman_error?

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode89
plugins/l2tpipsec.c:89: if (!provider) {
prevailing style is to compare ptr's to NULL

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode90
plugins/l2tpipsec.c:90: connman_error("No provider found");
uniquely identify; e.g. connman_error("%s: no provider", __func__);

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode111
plugins/l2tpipsec.c:111: DBG(DBG_VPN, "%s = %s", key, value);
_DBG_VPN

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode139
plugins/l2tpipsec.c:139: No routes come through? */
should not matter because the ipconfig settings are not installed until you
return from this routine

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode147
plugins/l2tpipsec.c:147: connman_provider_ipconfig_set(provider, &ipaddr);
Can't comment on upstream.  We allocate ipv4config when the provider enters the
"CONNECT" state which should happen before your plugin initiates the connection
and you get notified.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c
File plugins/vpn.c (right):

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode94
plugins/vpn.c:94: if (vpn_driver_data && vpn_driver_data->vpn_driver &&
check ptr's against NULL

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode165
plugins/vpn.c:165: if (ifname == NULL || data == NULL)
need debug/error msgs

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode169
plugins/vpn.c:169: if (index < 0)
need msg unless connman_inet_ifindex already complains

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode307
plugins/vpn.c:307: if (data != NULL)
please add debug msg

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode325
plugins/vpn.c:325: if (vpn_driver_data && vpn_driver_data->vpn_driver &&
ptr cmp to NULL

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode412
plugins/vpn.c:412: data = connman_provider_get_data(provider);
not sure i follow why it's ok to drop set_data call

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c
File scripts/libppp-plugin.c (right):

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:20: */
need a comment explaining this is built into a .so that's loaded into ppp; I
kept looking for main until I read the plugin code and saw it was passed as a
dso

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:81: static int pptp_get_secret (char *username, char
*password)
s/secret (/secret(/ (seems inconsistent--but perhaps this is prevailing style)

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:88: if (username == NULL && password == NULL)
seems like you want some error msgs here; maybe sent via syslog unless they can
be captured some other way

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:214: append(&dict, "MTU", "1400");
say what?

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:226: static void pptp_exit (void *data, int arg)
s/ (/(/ or something to make style consistent

[kmixter1, 2011-03-05 09:30:51]

PTAL.  Most of your style comments were on upstream code changes.  I was torn
between keeping the code in sync and fixing style, but chose to fix style.

For reference, the changes have not yet landed upstream, but were originally
from Mohamed Abbas:

http://lists.connman.net/pipermail/connman/2011-February/003825.html
http://lists.connman.net/pipermail/connman/2011-February/003826.html
http://lists.connman.net/pipermail/connman/2011-February/003827.html

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c
File plugins/l2tpipsec.c (right):

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode56
plugins/l2tpipsec.c:56: passwd == NULL || strlen(passwd) == 0)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> debug msg or connman_error?

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode89
plugins/l2tpipsec.c:89: if (!provider) {
On 2011/03/04 17:47:45, Sam Leffler wrote:
> prevailing style is to compare ptr's to NULL

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode90
plugins/l2tpipsec.c:90: connman_error("No provider found");
On 2011/03/04 17:47:45, Sam Leffler wrote:
> uniquely identify; e.g. connman_error("%s: no provider", __func__);

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode111
plugins/l2tpipsec.c:111: DBG(DBG_VPN, "%s = %s", key, value);
On 2011/03/04 17:47:45, Sam Leffler wrote:
> _DBG_VPN

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode117
plugins/l2tpipsec.c:117: ipaddr.peer = g_strdup(value);
On 2011/03/04 02:06:02, kmixter1 wrote:
> I'm getting 0.0.0.0 from the pppd plugin.  Trying to get at a real peer
address.

FWIW, I found the peer address and it's now returned.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode139
plugins/l2tpipsec.c:139: No routes come through? */
On 2011/03/04 17:47:45, Sam Leffler wrote:
> should not matter because the ipconfig settings are not installed until you
> return from this routine

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/l2tpipsec.c#newcode147
plugins/l2tpipsec.c:147: connman_provider_ipconfig_set(provider, &ipaddr);
On 2011/03/04 17:47:45, Sam Leffler wrote:
> Can't comment on upstream.  We allocate ipv4config when the provider enters
the
> "CONNECT" state which should happen before your plugin initiates the
connection
> and you get notified.

Sort of - it tries to enter that state but ultimately the ipconfig init fails
because there is no device index yet (we don't know the device, since it's not a
tun device flimflam itself opens before initiating the connection).

I think is see now upstream doesn't have the problem because it's related to the
ways we've forked the ipconfig code.

In any case I've found a workaround.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c
File plugins/vpn.c (right):

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode94
plugins/vpn.c:94: if (vpn_driver_data && vpn_driver_data->vpn_driver &&
On 2011/03/04 17:47:45, Sam Leffler wrote:
> check ptr's against NULL

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode165
plugins/vpn.c:165: if (ifname == NULL || data == NULL)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> need debug/error msgs

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode169
plugins/vpn.c:169: if (index < 0)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> need msg unless connman_inet_ifindex already complains

It doesn't, so added.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode307
plugins/vpn.c:307: if (data != NULL)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> please add debug msg

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode325
plugins/vpn.c:325: if (vpn_driver_data && vpn_driver_data->vpn_driver &&
On 2011/03/04 17:47:45, Sam Leffler wrote:
> ptr cmp to NULL

Done.

http://codereview.chromium.org/6513009/diff/8001/plugins/vpn.c#newcode412
plugins/vpn.c:412: data = connman_provider_get_data(provider);
On 2011/03/04 17:47:45, Sam Leffler wrote:
> not sure i follow why it's ok to drop set_data call

This is from upstream, but I agree.  I see why it couldn't be NULLed out before
kill_tun.  I added it below kill_tun.

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c
File scripts/libppp-plugin.c (right):

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:20: */
On 2011/03/04 17:47:45, Sam Leffler wrote:
> need a comment explaining this is built into a .so that's loaded into ppp; I
> kept looking for main until I read the plugin code and saw it was passed as a
> dso

Done.

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:81: static int pptp_get_secret (char *username, char
*password)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> s/secret (/secret(/ (seems inconsistent--but perhaps this is prevailing style)

Done.

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:88: if (username == NULL && password == NULL)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> seems like you want some error msgs here; maybe sent via syslog unless they
can
> be captured some other way

Done.  Now I'm logging like error messages are free.

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:214: append(&dict, "MTU", "1400");
On 2011/03/04 17:47:45, Sam Leffler wrote:
> say what?

yeah. upstream. I'll remove it since it's obvious bogus and we're not using it
on the flimflam side.

http://codereview.chromium.org/6513009/diff/8001/scripts/libppp-plugin.c#newc...
scripts/libppp-plugin.c:226: static void pptp_exit (void *data, int arg)
On 2011/03/04 17:47:45, Sam Leffler wrote:
> s/ (/(/ or something to make style consistent

Done.

http://codereview.chromium.org/6513009/diff/15001/plugins/vpn.c
File plugins/vpn.c (right):

http://codereview.chromium.org/6513009/diff/15001/plugins/vpn.c#newcode184
plugins/vpn.c:184: connman_provider_set_state(provider,
CONNMAN_PROVIDER_STATE_CONNECT);
So this "fixes" the problem of ipconfig failing its earlier initialization
during __connman_provider_ipconfig_init (at connect time there is no index).  It
just reattempts the initialization once we know the device (at notify time). 
I'm certainly open to better solutions, but with it, flimflam says the
connection was successful.

[kmixter1, 2011-03-11 07:54:00]

PTAL - working now, moved local address computing code into l2tpipsec_vpn.

See http://codereview.chromium.org/6513009/ if interested.

[kmixter1, 2011-03-11 07:57:19]

(I meant see http://codereview.chromium.org/6508016/ if interested)

On Thu, Mar 10, 2011 at 11:54 PM, <kmixter@chromium.org> wrote:

> PTAL - working now, moved local address computing code into l2tpipsec_vpn.
>
> See http://codereview.chromium.org/6513009/ if interested.
>
>
> http://codereview.chromium.org/6513009/
>

[Sam Leffler, 2011-03-11 17:49:55]

Looks good, just some small stuff.

Please fill in your Description; e.g. describe changes to task support.

Need to verify openvpn still works.

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c
File plugins/l2tpipsec.c (right):

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c#newcode55
plugins/l2tpipsec.c:55: _DBG_VPN("User or password not set");
if you print both user+passwd then you can see which isn't set

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c#newcode85
plugins/l2tpipsec.c:85: dbus_message_iter_next(&iter);
might want _DBG_VPN("reason %s", reason) here; not sure what notifications come
in

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c#newcode146
plugins/l2tpipsec.c:146: g_strfreev(ipaddr.dns_servers);  /* TODO(kmixter): OK
to free? */
you own the storage so you need to free (note const'ness of parameter); looks
like you can do away with all the strdup's above and also just use a stack array
for dns_servers

otherwise it looks like you leak ipaddr.local and ipaddr.peer

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c
File scripts/libppp-plugin.c (right):

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c#new...
scripts/libppp-plugin.c:333: if (!busname || !interface || !path) {
maybe add syslog msg (would be a pain to figure out if you bail here)

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c#new...
scripts/libppp-plugin.c:355: add_notifier (&ip_up_notifier, ppptp_up, NULL);
s/ (/(/ (seems out of sync w/ prevailing style)

[kmixter1, 2011-03-12 00:52:31]

Addressed the small stuff... also see the separate CL to fix the openvpn memory
problem.  Pushing...

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c
File plugins/l2tpipsec.c (right):

http://codereview.chromium.org/6513009/diff/18001/plugins/l2tpipsec.c#newcode85
plugins/l2tpipsec.c:85: dbus_message_iter_next(&iter);
On 2011/03/11 17:49:55, Sam Leffler wrote:
> might want _DBG_VPN("reason %s", reason) here; not sure what notifications
come
> in

Done.

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c
File scripts/libppp-plugin.c (right):

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c#new...
scripts/libppp-plugin.c:333: if (!busname || !interface || !path) {
On 2011/03/11 17:49:55, Sam Leffler wrote:
> maybe add syslog msg (would be a pain to figure out if you bail here)

Done - though I think it's unlikely (impossible?) for strdup to actually return
NULL.

http://codereview.chromium.org/6513009/diff/18001/scripts/libppp-plugin.c#new...
scripts/libppp-plugin.c:355: add_notifier (&ip_up_notifier, ppptp_up, NULL);
On 2011/03/11 17:49:55, Sam Leffler wrote:
> s/ (/(/ (seems out of sync w/ prevailing style)

Done.

[kmixter1, 2011-03-17 03:36:44]

Taylor - please see changes to connect-vpn.  Will push when tree goes green.

[thutt, 2011-03-17 15:02:21]

I took a look at connect-vpn, and made a few comment.
I also looked at vpn.c, but my comments by be disregardable because I don't
understand the details of the system at all...

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c
File plugins/vpn.c (right):

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode99
plugins/vpn.c:99: strncpy(ifr.ifr_name, data->if_name, sizeof(ifr.ifr_name));
(nit) It looks like you are adding tabs in a file which does not have them.
Or, if it does, then lines 81..96 do not use tabs.

(That is, if my interpretation of the red chevrons indicate tabs)

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode204
plugins/vpn.c:204: static DBusMessage *vpn_notify(struct connman_task *task,
I don't understand why you changed this function to return a DBusMessage*, but
then always return NULL.

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode249
plugins/vpn.c:249: if (data == NULL)
I'm not a networking person, but is this returning an error stating that you're
already connected if you don't have any data?

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode318
plugins/vpn.c:318: if (data != NULL) {
I'm puzzled by the condition -vs- the message here.  If you have data, you
report an error that you could not access the data.  Maybe it's just that I
don't this stuff very much, but the condition & the message seem to contradict
each other.

It's particularly confusing because line 249 seems to do the opposite.

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn
File test/connect-vpn (right):

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn#newcode60
test/connect-vpn:60: if options.complzo: # "complzo" can only be enabled.
I think the single quotes on complzo here are ok, as it's just a comment.  No
problem if you don't want to change it, though.

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn#newcode66
test/connect-vpn:66: print ("\n--remote-cert-tls argument ('%s') "
Would it be better to remove the space preceding '(', since the rest of the file
does it that way?

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn#newcode72
test/connect-vpn:72: show_usage(parser, params["Type"])
I look forward to seeing what affect this has on the output in this case, as I
didn't know about this (and practically anything Python).

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn#newcode73
test/connect-vpn:73: elif params["Type"] == "l2tpipsec":
Just noticed that it looks like your editor is putting tabs in the file.  Can
you please make them spaces?

Also, as I've aligned the multiple assignments, would it be objectionable to
align these too (or unalign mine in 53..58), for consistency.

http://codereview.chromium.org/6513009/diff/25001/test/connect-vpn#newcode87
test/connect-vpn:87: print "Unknown VPN type: '%s'" % params["Type"]
Should this be replaced with a show_usage() like at line 72?
If so, and if that function exits, then line 88 can also be removed.

[kmixter1, 2011-03-17 17:52:20]

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c
File plugins/vpn.c (right):

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode99
plugins/vpn.c:99: strncpy(ifr.ifr_name, data->if_name, sizeof(ifr.ifr_name));
On 2011/03/17 15:02:21, thutt wrote:
> (nit) It looks like you are adding tabs in a file which does not have them.
> Or, if it does, then lines 81..96 do not use tabs.
> 
> (That is, if my interpretation of the red chevrons indicate tabs)

It's the latter.  The file is mostly tabs but I didn't add them consistently.
Running tabify.

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode204
plugins/vpn.c:204: static DBusMessage *vpn_notify(struct connman_task *task,
On 2011/03/17 15:02:21, thutt wrote:
> I don't understand why you changed this function to return a DBusMessage*, but
> then always return NULL.

This is the default notification function if the provider doesn't override it. 
The change makes notification functions be able to return dbus responses, but
this one doesn't need to.

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode249
plugins/vpn.c:249: if (data == NULL)
On 2011/03/17 15:02:21, thutt wrote:
> I'm not a networking person, but is this returning an error stating that
you're
> already connected if you don't have any data?

Done and added a message.

http://codereview.chromium.org/6513009/diff/25001/plugins/vpn.c#newcode318
plugins/vpn.c:318: if (data != NULL) {
On 2011/03/17 15:02:21, thutt wrote:
> I'm puzzled by the condition -vs- the message here.  If you have data, you
> report an error that you could not access the data.  Maybe it's just that I
> don't this stuff very much, but the condition & the message seem to contradict
> each other.
> 
> It's particularly confusing because line 249 seems to do the opposite.

Updated the error message - I copy and pasted wrong when adding error messages
in response to an earlier review.

[kmixter1, 2011-03-17 18:49:27]

PTAL - also added GATEWAY_ADDRESS string passed up by libppp so that we know
whether or not to set a default gateway/route.

[Sam Leffler, 2011-03-17 18:52:51]

LGTM