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:[email protected]] Sent: Wednesday, July 30, 2014 7:43 PM To: Vyronas Tsingaras Cc: [email protected] 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
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Dev mailing list [email protected] https://lists.strongswan.org/mailman/listinfo/dev
