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

[Martin Willi]

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