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