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

[Martin Willi]

> I think it's okay now. Any comments/suggestions? 

Thanks for your update. Some notes, to listener.c

> 		identification_t *my_id = NULL;
> 		identification_t *peer_id = NULL;
> 		identification_t *eap_peer_id = NULL;
> 		my_id = ike_sa->get_my_id(ike_sa);
> 		peer_id = ike_sa->get_other_id(ike_sa);
> 		eap_peer_id = ike_sa->get_other_eap_id(ike_sa);
> 		if( peer_id == NULL || eap_peer_id == NULL )

These NULL-initializations do not make much sense; you'll set these
variables just afterwards. These identities should be guaranteed to be
set, so there is no need for that NULL check.

> 		if ( (peer_id && my_id && peer_id->equals(peer_id, my_id)) || 
> 			(eap_peer_id && my_id && eap_peer_id->equals(eap_peer_id, my_id)) )

What's the intention for that check? Why should the remote identity be
equal to the local identity? And why succeed in that case?


> 			FILE* fp = NULL;

Same as above.

> 			identification_t* generic_id = 
> 				(!eap_peer_id->equals(eap_peer_id, peer_id)) ? eap_peer_id : peer_id;

Is that needed? As mentioned in my other mail, get_other_eap_id() has a
fallback to get_other_id() if it no EAP/XAuth identity has been used.
Just use get_other_eap_id().

> 			memset(id_buf, 0, sizeof(id_buf));
> 			memset(cmd_buf, 0, sizeof(cmd_buf));
> 			memset(cmd_buf, 0, sizeof(prog_out));
> 			DBG2(DBG_CFG, "peer identity received: '%Y'", generic_id);
> 			snprintf(id_buf, sizeof(id_buf) - 1, "%Y", generic_id);

No need to memset() if you snprintf() to it. -1 is not required either;
we assume snprintf() is guaranteed to null-terminate strings. A single
buffer for the command probably works as well.

> 			snprintf(cmd_buf, sizeof(cmd_buf) - 1, "\"%s\" %s \"%s\"", this->path,
> 				(!eap_peer_id->equals(eap_peer_id, peer_id)) ? "eap" : "ike", (char*)id_buf);

Is there a reason for replacing the env var with an argument? In my
earlier comment I though about something like:

snprintf(cmd, sizeof(cmd), "IKE_REMOTE_EAP_ID='%Y' %s", id, this->path);

This is easier to extend, and much easier to parse/interpret in a shell
script.

> setvbuf(fp, NULL, _IONBF, 0);

Is that needed? There is some code you can reuse from updown to convert
output lines to strongSwan debug statements, btw.

>From _plugin.c:

> 		/* check if path is empty string or NULL */
> 		if(this->path == NULL || !strcmp(this->path, ""))
> 		{

You may additionally return FALSE here, so the plugin loader knows that
this feature has not been loaded. And we have the convenient streq()
function to avoid negating strcmp().


Would you mind to squash your changes to a clean commit? When doing so,
you might consider renaming the source files (and the used C
identifiers) to ext_auth_*, so it matches the plugin name. But let me
know if I'm asking for too much, I can take care of that as well ;-).

Thanks!
Martin