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

[Vyronas Tsingaras]

I just saw it errors at a warning, will fix this (WIP functionality)

On 29 July 2014 21:09:07 EEST, Vyronas Tsingaras <vtsingaras at it.auth.gr> wrote:
>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.
>
>------------------------------------------------------------------------
>
>_______________________________________________
>Dev mailing list
>Dev at lists.strongswan.org
>https://lists.strongswan.org/mailman/listinfo/dev

-- 
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/c2d285c2/attachment-0001.html>