[strongSwan-dev] Pull request for external-authorization plugin

[Vyronas Tsingaras]

Hi Martin and everyone else

My second attempt.

https://github.com/strongswan/strongswan/pull/4

Comments are greatly appreciated. 

On 29 July 2014 15:16:08 EEST, Martin Willi <martin at strongswan.org> wrote:
>Hi Vyronas,
>
>> https://github.com/strongswan/strongswan/pull/4
>> Please send me any problems you see/suggestions you may have.
>
>Thanks for your patch. Some notes:
>
>> +ARG_ENABL_SET([external-authorization], [enable the use of an
>external authorization program (like OpenVPN).
>
>That line misses two closing brackets. Travis failed to ./configure
>your
>sources, see [1].
>
>Also, the name is rather long; something like ext-auth might be simpler
>for all these identifiers?
>
>At the end of the file, you'll have to add your Makefile to
>AC_CONFIG_FILES to generate it during ./configure.
>
>> +	-DIPSEC_PIDDIR=\"${piddir}\"
>
>You probably don't need that PIDDIR define in your source?
>
>> +++
>b/src/libcharon/plugins/external_authorization/external_authorization_listener.c
>> @@ -0,0 +1,121 @@
>> +#include "external_authorization_listener.h"
>
>Can you add a license header to all your new files? GPLv2 is possible,
>MIT preferable, see [2].
>
>> +	if(!this->enabled)
>> +	{
>> +		DBG2(DBG_CFG, "external-authorization is disabled");
>> +		*success = TRUE;
>> +		return TRUE;
>> +	}
>> +	/* check if path is empty string or NULL */
>> +	if (!strcmp(this->path, "") || this->path == NULL)
>> +	{
>> +		DBG1(DBG_CFG, "ERROR: no authorization script specified");
>> +		*success = FALSE;
>> +		return FALSE;
>> +	}
>
>I think it would be simpler to check these values during plugin
>registration, and register the plugin in your plugin_cb only if these
>are set. You could add all that logic to _plugin.c, and pass the path
>as
>constructor argument to your listener.
>
>Also, most likely you won't need the "enabled" flag at all, as with
>5.2.0 we can now globally enable/disable plugins conveniently.
>
>> +			DBG2(DBG_CFG, "peer identity received: '%Y'", peer_id);
>> +			char buf[255];
>> +			snprintf(buf, 255, "%Y", peer_id);
>
>Please define variables at the beginning of the block. Use of sizeof()
>is preferable.
>
>> +			setenv("IPSEC_IDENTITY", (char*)buf, 1);
>
>As most likely that list of variables grows, we probably should
>consider
>some consistent naming scheme. IPSEC_IDENTITY is not very specific;
>what
>about IKE_REMOTE_EAP_ID?
>
>> +			DBG2(DBG_CFG, "calling script: %s", this->path);
>> +			authorized = WEXITSTATUS(system(this->path));
>
>While the invocation of the authorize() hook is currently serialized,
>this is not set in stone. If we change that behavior, this code has
>some
>serious multi-threading issues. Preferably you'd set that variable for
>the invocation only, i.e. pass it to the system() call.
>
>You also might consider using popen(), which allows you to redirect the
>output to the strongSwan log system. This allows your authorization
>script to log failure information. Refer to the updown plugin for
>details.
>
>> +METHOD(external_authorization_listener_t, set_active, void,
>> +METHOD(external_authorization_listener_t, set_path, void,
>
>Are there any users of these methods? If not you probably should drop
>these for now.
>
>> +		.path = lib->settings->get_str(lib->settings,
>"%s.plugins.external-authorization.path", "", lib->ns),
>
>Please wrap lines around ~80 characters.
>
>Kind regards
>Martin
>
>[1]https://travis-ci.org/strongswan/strongswan/builds/31042895
>[2]https://wiki.strongswan.org/projects/strongswan/wiki/Contributions

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strongswan.org/pipermail/dev/attachments/20140729/b3bb43f9/attachment.html>