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

[Vyronas Tsingaras]

I added the initializations because I was getting broken tests on Travis on "LEAK_DETECTIVE" tests so I just tried everything.

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

You are right here, it's the same logic as afterwards here "> 			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);" will do everything in the suggestions, thanks a lot for your time!

Regards,
Vyronas Tsingaras

-----Original Message-----
From: Martin Willi [mailto:martin at strongswan.org] 
Sent: Wednesday, July 30, 2014 7:43 PM
To: Vyronas Tsingaras
Cc: dev at lists.strongswan.org
Subject: Re: [strongSwan-dev] Pull request for external-authorization plugin


> 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