[strongSwan-dev] [Patch] support dynamic IP adresses

[Mirko Parthey]

Hello Martin,
thanks for reviewing and improving my patch.

On Mon, Aug 29, 2011 at 10:12:51AM +0200, Martin Willi wrote:
> The patch looks fine, might be a little more complicated than required,
> though:
> 
> > +               char            *dnsname;
> > +               bool            has_dnsname;
> 
> Using "host" as member is probably easier, as it refers to the KW_HOST
> keyword. The has_dnsname attribute could be eliminated by just setting
> dnsname/host to NULL(?).

OK.

> > -       plog("# bad addr: %s=%s [%s]", name, value, ugh);
> > -       if (streq(ugh, "does not look numeric and name lookup failed"))
> > -       {
> > -               end->dns_failed = TRUE;
> 
> I think it would be a good idea to accept name resolution failures and
> just pass on the DNS name to charon. This would allow us to use the
> connection even if the name is not resolvable during startup.

I agree, but could not come up with an elegant way to limit this to charon.
Your checking for IKEv2 looks like the right thing to do.

> I've attached a patch containing these modifications. I'll push it if it
> looks fine to you.

Please see my comments below.

> -	{ ARG_MISC, 0, NULL  /* KW_HOST */                                             },
> +	{ ARG_STR,  offsetof(starter_end_t, host), NULL                                },

You dropped the /* KW_HOST */ comment - intention or accident?

> +		else
> +		{
> +			/* check for allow_any prefix */
> +			if (value[0] == '%')
> +			{
> +				end->allow_any = TRUE;
> +				value++;
> +			}
> +			conn->addr_family = ip_version(value);
> +			ugh = ttoaddr(value, 0, conn->addr_family, &end->addr);
> +			if (ugh != NULL)
> +			{
> +				plog("# bad addr: %s=%s [%s]", name, value, ugh);
> +				if (streq(ugh, "does not look numeric and name lookup failed"))
> +				{
> +					end->dns_failed = TRUE;
> +					anyaddr(conn->addr_family, &end->addr);
> +				}
> +				else
> +				{
> +					goto err;
> +				}
> +			}
> +			if (!end->allow_any)
> +			{
> +				end->host = clone_str(value);
> +			}

DNS names can be given with or without a '%' prefix, and I would expect
> +				end->host = clone_str(value);
to be executed in either case. The (!end->allow_any) conditional looks
superfluous to me.


> @@ -464,7 +470,7 @@ static void handle_dns_failure(const char *label, starter_end_t *end,
>  			plog("# fallback to %s=%%any due to '%%' prefix or %sallowany=yes",
>  				label, label);
>  		}
> -		else
> +		else if (!end->host || conn->keyexchange != KEY_EXCHANGE_IKEV2)
>  		{
>  			/* declare an error */
>  			cfg->err++;

What is your intention behind the !end->host conditional here?
Is it related to the !end->allow_any discussed above?

Regards,
Mirko