[strongSwan-dev] Pull request for external-authorization plugin
vtsingaras at it.auth.gr
Wed Jul 30 21:07:09 CEST 2014
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 is ike and it knows it already is
an email if argv is eap
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 :
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",
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
> 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.
> /* 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 ;-).
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 5588 bytes
Desc: not available
More information about the Dev