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

[Vyronas Tsingaras]

Hi Martin,

How does it look now? The "eap" : "ike" part is to make it easy to scripts to 
differentiate what type of identity it is. Eg. We have a conn that uses 
rightauth=pubkey (IKE id that is the DN of the X.509 cert) and a conn for 
windows 7 that uses rightauth=eap-tls (EAP id that is only the email). So our 
script can parse for the email when argv[1] is ike and it knows it already is 
an email if argv[1] is eap

https://github.com/strongswan/strongswan/pull/5



-----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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5588 bytes
Desc: not available
URL: <http://lists.strongswan.org/pipermail/dev/attachments/20140730/6d17bde2/attachment.bin>