Re: relayd ipv6 ttl check_icmp / check_tcp

[Jeremie Courreges-Anglas]

Kapetanakis Giannis  writes:

> On 12/07/17 22:00, Jeremie Courreges-Anglas wrote:
>> The tweak I had in mind: consistently use "ttl" for all the
>> get/setsockopt calls.
>>
>> ok?
>
> nice,
> you can also replace sizeof(int) to sizeof(ttl) on the else{} block of
> case AF_INET6

Indeed, thanks.  Patch committed.

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: relayd ipv6 ttl check_icmp / check_tcp

[Kapetanakis Giannis]

On 12/07/17 22:00, Jeremie Courreges-Anglas wrote:

The tweak I had in mind: consistently use "ttl" for all the
get/setsockopt calls.

ok?


nice,
you can also replace sizeof(int) to sizeof(ttl) on the else{} block of 
case AF_INET6



G




Index: check_icmp.c
===
RCS file: /d/cvs/src/usr.sbin/relayd/check_icmp.c,v
retrieving revision 1.46
diff -u -p -p -u -r1.46 check_icmp.c
--- check_icmp.c11 Jul 2017 19:41:30 -  1.46
+++ check_icmp.c12 Jul 2017 18:57:52 -
@@ -220,11 +220,12 @@ send_icmp(int s, short event, void *arg)
sizeof(packet));
}
  
+			ttl = host->conf.ttl;

switch(cie->af) {
case AF_INET:
-   if ((ttl = host->conf.ttl) > 0) {
+   if (ttl > 0) {
if (setsockopt(s, IPPROTO_IP, IP_TTL,
-   >conf.ttl, sizeof(int)) == -1)
+   , sizeof(ttl)) == -1)
log_warn("%s: setsockopt",
__func__);
} else {
@@ -243,10 +244,10 @@ send_icmp(int s, short event, void *arg)
}
break;
case AF_INET6:
-   if ((ttl = host->conf.ttl) > 0) {
+   if (ttl > 0) {
if (setsockopt(s, IPPROTO_IPV6,
-   IPV6_UNICAST_HOPS, >conf.ttl,
-   sizeof(int)) == -1)
+   IPV6_UNICAST_HOPS, ,
+   sizeof(ttl)) == -1)
log_warn("%s: setsockopt",
__func__);
} else {

Re: relayd ipv6 ttl check_icmp / check_tcp

[Sebastian Benoit]

Jeremie Courreges-Anglas(j...@wxcvbn.org) on 2017.07.12 21:00:44 +0200:
> Jeremie Courreges-Anglas  writes:
> 
> > Kapetanakis Giannis  writes:
> >
> >> On 10/07/17 17:22, Jeremie Courreges-Anglas wrote:
> >>> Using -1 for IPV6_UNICAST_HOPS is correct.
> >>> 
> >>> Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
> >>> out there don't support it.
> >>> 
>  comments?
> >>> 
> >>> ok jca@ with the nits below.
> >>> 
> >>> It would be nice to factor this out in a helper function and use it
> >>> elsewhere in relayd.
> >>
> >> Thanks for the comments.
> >>
> >> My guess is that the helper function should go outside of relayd so it can 
> >> be used by others as well?
> >> I leave that to a more competent programmer.
> >>
> >> Would you like me to set -1 to IP_TTL as well and drop the call to 
> >> getsockopt(2)?
> >
> > I'm not sure about this.  Maybe it should be discussed separately?
> >
> >> updated diff bellow (in case not) with jca@ recommendations.
> >
> > I have tweaks, but this already looks fine to me.  ok jca@
> 
> The tweak I had in mind: consistently use "ttl" for all the
> get/setsockopt calls.
> 
> ok?

yes please

> 
> 
> Index: check_icmp.c
> ===
> RCS file: /d/cvs/src/usr.sbin/relayd/check_icmp.c,v
> retrieving revision 1.46
> diff -u -p -p -u -r1.46 check_icmp.c
> --- check_icmp.c  11 Jul 2017 19:41:30 -  1.46
> +++ check_icmp.c  12 Jul 2017 18:57:52 -
> @@ -220,11 +220,12 @@ send_icmp(int s, short event, void *arg)
>   sizeof(packet));
>   }
>  
> + ttl = host->conf.ttl;
>   switch(cie->af) {
>   case AF_INET:
> - if ((ttl = host->conf.ttl) > 0) {
> + if (ttl > 0) {
>   if (setsockopt(s, IPPROTO_IP, IP_TTL,
> - >conf.ttl, sizeof(int)) == -1)
> + , sizeof(ttl)) == -1)
>   log_warn("%s: setsockopt",
>   __func__);
>   } else {
> @@ -243,10 +244,10 @@ send_icmp(int s, short event, void *arg)
>   }
>   break;
>   case AF_INET6:
> - if ((ttl = host->conf.ttl) > 0) {
> + if (ttl > 0) {
>   if (setsockopt(s, IPPROTO_IPV6,
> - IPV6_UNICAST_HOPS, >conf.ttl,
> - sizeof(int)) == -1)
> + IPV6_UNICAST_HOPS, ,
> + sizeof(ttl)) == -1)
>   log_warn("%s: setsockopt",
>   __func__);
>   } else {
> 
> 
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
> 

Re: relayd ipv6 ttl check_icmp / check_tcp

[Jeremie Courreges-Anglas]

Jeremie Courreges-Anglas  writes:

> Kapetanakis Giannis  writes:
>
>> On 10/07/17 17:22, Jeremie Courreges-Anglas wrote:
>>> Using -1 for IPV6_UNICAST_HOPS is correct.
>>> 
>>> Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
>>> out there don't support it.
>>> 
 comments?
>>> 
>>> ok jca@ with the nits below.
>>> 
>>> It would be nice to factor this out in a helper function and use it
>>> elsewhere in relayd.
>>
>> Thanks for the comments.
>>
>> My guess is that the helper function should go outside of relayd so it can 
>> be used by others as well?
>> I leave that to a more competent programmer.
>>
>> Would you like me to set -1 to IP_TTL as well and drop the call to 
>> getsockopt(2)?
>
> I'm not sure about this.  Maybe it should be discussed separately?
>
>> updated diff bellow (in case not) with jca@ recommendations.
>
> I have tweaks, but this already looks fine to me.  ok jca@

The tweak I had in mind: consistently use "ttl" for all the
get/setsockopt calls.

ok?


Index: check_icmp.c
===
RCS file: /d/cvs/src/usr.sbin/relayd/check_icmp.c,v
retrieving revision 1.46
diff -u -p -p -u -r1.46 check_icmp.c
--- check_icmp.c11 Jul 2017 19:41:30 -  1.46
+++ check_icmp.c12 Jul 2017 18:57:52 -
@@ -220,11 +220,12 @@ send_icmp(int s, short event, void *arg)
sizeof(packet));
}
 
+   ttl = host->conf.ttl;
switch(cie->af) {
case AF_INET:
-   if ((ttl = host->conf.ttl) > 0) {
+   if (ttl > 0) {
if (setsockopt(s, IPPROTO_IP, IP_TTL,
-   >conf.ttl, sizeof(int)) == -1)
+   , sizeof(ttl)) == -1)
log_warn("%s: setsockopt",
__func__);
} else {
@@ -243,10 +244,10 @@ send_icmp(int s, short event, void *arg)
}
break;
case AF_INET6:
-   if ((ttl = host->conf.ttl) > 0) {
+   if (ttl > 0) {
if (setsockopt(s, IPPROTO_IPV6,
-   IPV6_UNICAST_HOPS, >conf.ttl,
-   sizeof(int)) == -1)
+   IPV6_UNICAST_HOPS, ,
+   sizeof(ttl)) == -1)
log_warn("%s: setsockopt",
__func__);
} else {


-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: relayd ipv6 ttl check_icmp / check_tcp

[Florian Obser]

commited, thanks!

On Mon, Jul 10, 2017 at 06:18:03PM +0300, Kapetanakis Giannis wrote:
> On 10/07/17 17:22, Jeremie Courreges-Anglas wrote:
> > Using -1 for IPV6_UNICAST_HOPS is correct.
> > 
> > Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
> > out there don't support it.
> > 
> >> comments?
> > 
> > ok jca@ with the nits below.
> > 
> > It would be nice to factor this out in a helper function and use it
> > elsewhere in relayd.
> 
> Thanks for the comments.
> 
> My guess is that the helper function should go outside of relayd so it can be 
> used by others as well?
> I leave that to a more competent programmer.
> 
> Would you like me to set -1 to IP_TTL as well and drop the call to 
> getsockopt(2)?
> 
> updated diff bellow (in case not) with jca@ recommendations.
> 
> G
> 
> Index: check_icmp.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 check_icmp.c
> --- check_icmp.c  28 May 2017 10:39:15 -  1.45
> +++ check_icmp.c  10 Jul 2017 15:16:02 -
> @@ -220,18 +220,45 @@ send_icmp(int s, short event, void *arg)
>   sizeof(packet));
>   }
>  
> - if ((ttl = host->conf.ttl) > 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - >conf.ttl, sizeof(int));
> - else {
> - /* Revert to default TTL */
> - len = sizeof(ttl);
> - if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
> - , ) == 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - , len);
> - else
> - log_warn("%s: getsockopt",__func__);
> + switch(cie->af) {
> + case AF_INET:
> + if ((ttl = host->conf.ttl) > 0) {
> + if (setsockopt(s, IPPROTO_IP, IP_TTL,
> + >conf.ttl, sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + } else {
> + /* Revert to default TTL */
> + len = sizeof(ttl);
> + if (getsockopt(s, IPPROTO_IP,
> + IP_IPDEFTTL, , ) == 0) {
> + if (setsockopt(s, IPPROTO_IP,
> + IP_TTL, , len) == -1)
> + log_warn(
> + "%s: setsockopt",
> + __func__);
> + } else
> + log_warn("%s: getsockopt",
> + __func__);
> + }
> + break;
> + case AF_INET6:
> + if ((ttl = host->conf.ttl) > 0) {
> + if (setsockopt(s, IPPROTO_IPV6,
> + IPV6_UNICAST_HOPS, >conf.ttl,
> + sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + } else {
> + /* Revert to default hop limit */
> + ttl = -1;
> + if (setsockopt(s, IPPROTO_IPV6,
> + IPV6_UNICAST_HOPS, ,
> + sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + }
> + break;
>   }
>  
>   r = sendto(s, packet, sizeof(packet), 0, to, slen);
> 
> 
> 
> 
> 
> 
> 


-- 
I'm not entirely sure you are real.

Re: relayd ipv6 ttl check_icmp / check_tcp

[Jeremie Courreges-Anglas]

Kapetanakis Giannis  writes:

> On 10/07/17 17:22, Jeremie Courreges-Anglas wrote:
>> Using -1 for IPV6_UNICAST_HOPS is correct.
>> 
>> Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
>> out there don't support it.
>> 
>>> comments?
>> 
>> ok jca@ with the nits below.
>> 
>> It would be nice to factor this out in a helper function and use it
>> elsewhere in relayd.
>
> Thanks for the comments.
>
> My guess is that the helper function should go outside of relayd so it can be 
> used by others as well?
> I leave that to a more competent programmer.
>
> Would you like me to set -1 to IP_TTL as well and drop the call to 
> getsockopt(2)?

I'm not sure about this.  Maybe it should be discussed separately?

> updated diff bellow (in case not) with jca@ recommendations.

I have tweaks, but this already looks fine to me.  ok jca@

> G
>
> Index: check_icmp.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 check_icmp.c
> --- check_icmp.c  28 May 2017 10:39:15 -  1.45
> +++ check_icmp.c  10 Jul 2017 15:16:02 -
> @@ -220,18 +220,45 @@ send_icmp(int s, short event, void *arg)
>   sizeof(packet));
>   }
>  
> - if ((ttl = host->conf.ttl) > 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - >conf.ttl, sizeof(int));
> - else {
> - /* Revert to default TTL */
> - len = sizeof(ttl);
> - if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
> - , ) == 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - , len);
> - else
> - log_warn("%s: getsockopt",__func__);
> + switch(cie->af) {
> + case AF_INET:
> + if ((ttl = host->conf.ttl) > 0) {
> + if (setsockopt(s, IPPROTO_IP, IP_TTL,
> + >conf.ttl, sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + } else {
> + /* Revert to default TTL */
> + len = sizeof(ttl);
> + if (getsockopt(s, IPPROTO_IP,
> + IP_IPDEFTTL, , ) == 0) {
> + if (setsockopt(s, IPPROTO_IP,
> + IP_TTL, , len) == -1)
> + log_warn(
> + "%s: setsockopt",
> + __func__);
> + } else
> + log_warn("%s: getsockopt",
> + __func__);
> + }
> + break;
> + case AF_INET6:
> + if ((ttl = host->conf.ttl) > 0) {
> + if (setsockopt(s, IPPROTO_IPV6,
> + IPV6_UNICAST_HOPS, >conf.ttl,
> + sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + } else {
> + /* Revert to default hop limit */
> + ttl = -1;
> + if (setsockopt(s, IPPROTO_IPV6,
> + IPV6_UNICAST_HOPS, ,
> + sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + }
> + break;
>   }
>  
>   r = sendto(s, packet, sizeof(packet), 0, to, slen);
>
>
>
>
>
>
>

-- 
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE

Re: relayd ipv6 ttl check_icmp / check_tcp

[Kapetanakis Giannis]

On 10/07/17 17:22, Jeremie Courreges-Anglas wrote:
> Using -1 for IPV6_UNICAST_HOPS is correct.
> 
> Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
> out there don't support it.
> 
>> comments?
> 
> ok jca@ with the nits below.
> 
> It would be nice to factor this out in a helper function and use it
> elsewhere in relayd.

Thanks for the comments.

My guess is that the helper function should go outside of relayd so it can be 
used by others as well?
I leave that to a more competent programmer.

Would you like me to set -1 to IP_TTL as well and drop the call to 
getsockopt(2)?

updated diff bellow (in case not) with jca@ recommendations.

G

Index: check_icmp.c
===
RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
retrieving revision 1.45
diff -u -p -r1.45 check_icmp.c
--- check_icmp.c28 May 2017 10:39:15 -  1.45
+++ check_icmp.c10 Jul 2017 15:16:02 -
@@ -220,18 +220,45 @@ send_icmp(int s, short event, void *arg)
sizeof(packet));
}
 
-   if ((ttl = host->conf.ttl) > 0)
-   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
-   >conf.ttl, sizeof(int));
-   else {
-   /* Revert to default TTL */
-   len = sizeof(ttl);
-   if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
-   , ) == 0)
-   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
-   , len);
-   else
-   log_warn("%s: getsockopt",__func__);
+   switch(cie->af) {
+   case AF_INET:
+   if ((ttl = host->conf.ttl) > 0) {
+   if (setsockopt(s, IPPROTO_IP, IP_TTL,
+   >conf.ttl, sizeof(int)) == -1)
+   log_warn("%s: setsockopt",
+   __func__);
+   } else {
+   /* Revert to default TTL */
+   len = sizeof(ttl);
+   if (getsockopt(s, IPPROTO_IP,
+   IP_IPDEFTTL, , ) == 0) {
+   if (setsockopt(s, IPPROTO_IP,
+   IP_TTL, , len) == -1)
+   log_warn(
+   "%s: setsockopt",
+   __func__);
+   } else
+   log_warn("%s: getsockopt",
+   __func__);
+   }
+   break;
+   case AF_INET6:
+   if ((ttl = host->conf.ttl) > 0) {
+   if (setsockopt(s, IPPROTO_IPV6,
+   IPV6_UNICAST_HOPS, >conf.ttl,
+   sizeof(int)) == -1)
+   log_warn("%s: setsockopt",
+   __func__);
+   } else {
+   /* Revert to default hop limit */
+   ttl = -1;
+   if (setsockopt(s, IPPROTO_IPV6,
+   IPV6_UNICAST_HOPS, ,
+   sizeof(int)) == -1)
+   log_warn("%s: setsockopt",
+   __func__);
+   }
+   break;
}
 
r = sendto(s, packet, sizeof(packet), 0, to, slen);

Re: relayd ipv6 ttl check_icmp / check_tcp

[Jeremie Courreges-Anglas]

Kapetanakis Giannis  writes:

> On 04/07/17 23:56, Sebastian Benoit wrote:
>> Florian Obser(flor...@openbsd.org) on 2017.07.04 19:27:15 +:
>>> On Fri, Jun 23, 2017 at 01:52:52PM +0300, Kapetanakis Giannis wrote:
 Hi,

 Using relayd's redirect/forward on ipv6 addresses I discovered problems 
 relating to setting TTL.

 There is no check for address family and setsockopt tries to apply IP_TTL 
 always.

 Without ip ttl on ipv6 table, check_icmp gives
 send_icmp: getsockopt: Invalid argument

 I've removed the IP_IPDEFTTL check. Was this ok?
>>>
>>> Nope, relayd reuses the raw socket between config reloads (I think),
>>> if the ttl gets removed from the config we need to reset to the
>>> default. Don't think there is a getsockopt for v6, you can take a look
>> 
>> i think jca@ once had a diff for somethin called IPV6_MINHOPLIMIT? Unsure if
>> thats what we need here though.

IPV6_MINHOPCOUNT support has been added to the kernel and relayd.
As tsg@ points out, this is about IPV6_UNICAST_HOPS here.

>>> at the sysctl(3) song and dance in traceroute(8) how to do this
>>> somewhat AF independet.
>>>
>>> Also please make sure to not exceed 80 cols
>
> Thanks for the commit on check_tcp.
>
> My tabstop was set to 3 and not 8. fixed that, but it looks ugly.
>
> According to ip6(4):
> IPV6_UNICAST_HOPS int *
>  Get or set the default hop limit header field for outgoing
>  unicast datagrams sent on this socket.  A value of -1 resets to
>  the default value.
>
> So I changed the diff and use this. Couldn't make it work with sysctl.

Using -1 for IPV6_UNICAST_HOPS is correct.

Note that you can also use -1 for IP_TTL on OpenBSD, sadly some systems
out there don't support it.

> comments?

ok jca@ with the nits below.

It would be nice to factor this out in a helper function and use it
elsewhere in relayd.

> Giannis
> ps. There is still a patch on @tech for alternative socket name.
> Could you also have a look there when you have some time?
> thanks
>
> Index: check_icmp.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 check_icmp.c
> --- check_icmp.c  28 May 2017 10:39:15 -  1.45
> +++ check_icmp.c  5 Jul 2017 14:35:03 -
> @@ -168,6 +168,7 @@ send_icmp(int s, short event, void *arg)
>   socklen_tslen, len;
>   int  i = 0, ttl;
>   u_int32_tid;
> + int  ip6_def_hlim = -1;

No need for this extra variable, you can just force ttl to -1 in the
IPv6 case.

>   if (event == EV_TIMEOUT) {
>   icmp_checks_timeout(cie, HCE_ICMP_WRITE_TIMEOUT);
> @@ -220,18 +221,46 @@ send_icmp(int s, short event, void *arg)
>   sizeof(packet));
>   }
>  
> - if ((ttl = host->conf.ttl) > 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - >conf.ttl, sizeof(int));
> - else {
> - /* Revert to default TTL */
> - len = sizeof(ttl);
> - if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
> - , ) == 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - , len);
> - else
> - log_warn("%s: getsockopt",__func__);
> + switch(cie->af) {
> + case AF_INET:
> + if ((ttl = host->conf.ttl) > 0) {
> + if (setsockopt(s, IPPROTO_IP, IP_TTL,
> + >conf.ttl, sizeof(int)) == -1)
> + log_warn("%s: setsockopt",
> + __func__);
> + }

extra newline

> + else {
> + /* Revert to default TTL */
> + len = sizeof(ttl);
> + if (getsockopt(s, IPPROTO_IP,
> + IP_IPDEFTTL, , ) == 0) {
> + if (setsockopt(s, IPPROTO_IP,
> + IP_TTL, , len) == -1)
> + log_warn(
> + "%s: setsockopt",
> + __func__);
> + }
> + else
> + log_warn("%s: getsockopt",__func__);

4 

Re: relayd ipv6 ttl check_icmp / check_tcp

[Kapetanakis Giannis]

On 04/07/17 23:56, Sebastian Benoit wrote:
> Florian Obser(flor...@openbsd.org) on 2017.07.04 19:27:15 +:
>> On Fri, Jun 23, 2017 at 01:52:52PM +0300, Kapetanakis Giannis wrote:
>>> Hi,
>>>
>>> Using relayd's redirect/forward on ipv6 addresses I discovered problems 
>>> relating to setting TTL.
>>>
>>> There is no check for address family and setsockopt tries to apply IP_TTL 
>>> always.
>>>
>>> Without ip ttl on ipv6 table, check_icmp gives
>>> send_icmp: getsockopt: Invalid argument
>>>
>>> I've removed the IP_IPDEFTTL check. Was this ok?
>>
>> Nope, relayd reuses the raw socket between config reloads (I think),
>> if the ttl gets removed from the config we need to reset to the
>> default. Don't think there is a getsockopt for v6, you can take a look
> 
> i think jca@ once had a diff for somethin called IPV6_MINHOPLIMIT? Unsure if
> thats what we need here though.
> 
>> at the sysctl(3) song and dance in traceroute(8) how to do this
>> somewhat AF independet.
>>
>> Also please make sure to not exceed 80 cols

Thanks for the commit on check_tcp.

My tabstop was set to 3 and not 8. fixed that, but it looks ugly.

According to ip6(4):
IPV6_UNICAST_HOPS int *
 Get or set the default hop limit header field for outgoing
 unicast datagrams sent on this socket.  A value of -1 resets to
 the default value.

So I changed the diff and use this. Couldn't make it work with sysctl.

comments?

Giannis
ps. There is still a patch on @tech for alternative socket name.
Could you also have a look there when you have some time?
thanks

Index: check_icmp.c
===
RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
retrieving revision 1.45
diff -u -p -r1.45 check_icmp.c
--- check_icmp.c28 May 2017 10:39:15 -  1.45
+++ check_icmp.c5 Jul 2017 14:35:03 -
@@ -168,6 +168,7 @@ send_icmp(int s, short event, void *arg)
socklen_tslen, len;
int  i = 0, ttl;
u_int32_tid;
+   int  ip6_def_hlim = -1;
 
if (event == EV_TIMEOUT) {
icmp_checks_timeout(cie, HCE_ICMP_WRITE_TIMEOUT);
@@ -220,18 +221,46 @@ send_icmp(int s, short event, void *arg)
sizeof(packet));
}
 
-   if ((ttl = host->conf.ttl) > 0)
-   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
-   >conf.ttl, sizeof(int));
-   else {
-   /* Revert to default TTL */
-   len = sizeof(ttl);
-   if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
-   , ) == 0)
-   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
-   , len);
-   else
-   log_warn("%s: getsockopt",__func__);
+   switch(cie->af) {
+   case AF_INET:
+   if ((ttl = host->conf.ttl) > 0) {
+   if (setsockopt(s, IPPROTO_IP, IP_TTL,
+   >conf.ttl, sizeof(int)) == -1)
+   log_warn("%s: setsockopt",
+   __func__);
+   }
+   else {
+   /* Revert to default TTL */
+   len = sizeof(ttl);
+   if (getsockopt(s, IPPROTO_IP,
+   IP_IPDEFTTL, , ) == 0) {
+   if (setsockopt(s, IPPROTO_IP,
+   IP_TTL, , len) == -1)
+   log_warn(
+   "%s: setsockopt",
+   __func__);
+   }
+   else
+   log_warn("%s: getsockopt",__func__);
+   }
+   break;
+   case AF_INET6:
+   if ((ttl = host->conf.ttl) > 0) {
+   if (setsockopt(s, IPPROTO_IPV6,
+   IPV6_UNICAST_HOPS, >conf.ttl,
+   sizeof(int)) == -1)
+   log_warn("%s: setsockopt",
+   __func__);
+   }
+   else {
+   

Re: relayd ipv6 ttl check_icmp / check_tcp

[Sebastian Benoit]

Florian Obser(flor...@openbsd.org) on 2017.07.04 19:27:15 +:
> On Fri, Jun 23, 2017 at 01:52:52PM +0300, Kapetanakis Giannis wrote:
> > Hi,
> > 
> > Using relayd's redirect/forward on ipv6 addresses I discovered problems 
> > relating to setting TTL.
> > 
> > There is no check for address family and setsockopt tries to apply IP_TTL 
> > always.
> > 
> > Without ip ttl on ipv6 table, check_icmp gives
> > send_icmp: getsockopt: Invalid argument
> > 
> > With ip ttl on ipv6 table, check_tcp gives
> > hce_notify_done: fdaa:10:1:9::11 (tcp socket option)
> > 
> > is the following diff valid?
> 
> the check_tcp hunk looks good and is OK florian@

commited, thanks!
 
> > I've removed the IP_IPDEFTTL check. Was this ok?
> 
> Nope, relayd reuses the raw socket between config reloads (I think),
> if the ttl gets removed from the config we need to reset to the
> default. Don't think there is a getsockopt for v6, you can take a look

i think jca@ once had a diff for somethin called IPV6_MINHOPLIMIT? Unsure if
thats what we need here though.

> at the sysctl(3) song and dance in traceroute(8) how to do this
> somewhat AF independet.
> 
> Also please make sure to not exceed 80 cols
> 
> > 
> > regards,
> > 
> > Giannis
> > 
> > Index: check_icmp.c
> > ===
> > RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
> > retrieving revision 1.45
> > diff -u -p -r1.45 check_icmp.c
> > --- check_icmp.c28 May 2017 10:39:15 -  1.45
> > +++ check_icmp.c23 Jun 2017 10:42:30 -
> > @@ -165,7 +165,7 @@ send_icmp(int s, short event, void *arg)
> > struct icmp6_hdr*icp6;
> > ssize_t  r;
> > u_char   packet[ICMP_BUF_SIZE];
> > -   socklen_tslen, len;
> > +   socklen_tslen;
> > int  i = 0, ttl;
> > u_int32_tid;
> >  
> > @@ -221,18 +221,18 @@ send_icmp(int s, short event, void *arg)
> > }
> >  
> > if ((ttl = host->conf.ttl) > 0)
> > -   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> > -   >conf.ttl, sizeof(int));
> > -   else {
> > -   /* Revert to default TTL */
> > -   len = sizeof(ttl);
> > -   if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
> > -   , ) == 0)
> > -   (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> > -   , len);
> > -   else
> > -   log_warn("%s: getsockopt",__func__);
> > -   }
> > +   switch(cie->af) {
> > +   case AF_INET:
> > +   if (setsockopt(s, IPPROTO_IP, IP_TTL,
> > +   >conf.ttl, sizeof(int)) == -1)
> > +   log_warn("%s: 
> > setsockopt",__func__);
> > +   break;
> > +   case AF_INET6:
> > +   if (setsockopt(s, IPPROTO_IPV6, 
> > IPV6_UNICAST_HOPS,
> > +   >conf.ttl, sizeof(int)) == -1)
> > +   log_warn("%s: 
> > setsockopt",__func__);
> > +   break;
> > +   }
> >  
> > r = sendto(s, packet, sizeof(packet), 0, to, slen);
> > if (r == -1) {
> > Index: check_tcp.c
> > ===
> > RCS file: /cvs/src/usr.sbin/relayd/check_tcp.c,v
> > retrieving revision 1.54
> > diff -u -p -r1.54 check_tcp.c
> > --- check_tcp.c 28 May 2017 10:39:15 -  1.54
> > +++ check_tcp.c 23 Jun 2017 10:42:30 -
> > @@ -82,11 +82,19 @@ check_tcp(struct ctl_tcp_event *cte)
> > if (setsockopt(s, SOL_SOCKET, SO_LINGER, , sizeof(lng)) == -1)
> > goto bad;
> >  
> > -   if (cte->host->conf.ttl > 0) {
> > -   if (setsockopt(s, IPPROTO_IP, IP_TTL,
> > -   >host->conf.ttl, sizeof(int)) == -1)
> > -   goto bad;
> > -   }
> > +   if (cte->host->conf.ttl > 0)
> > +   switch (cte->host->conf.ss.ss_family) {
> > +   case AF_INET:
> > +   if (setsockopt(s, IPPROTO_IP, IP_TTL,
> > +   >host->conf.ttl, sizeof(int)) == -1)
> > +   goto bad;
> > +   break;
> > +   case AF_INET6:
> > +   if (setsockopt(s, IPPROTO_IPV6, IPV6_UNICAST_HOPS,
> > +   >host->conf.ttl, sizeof(int)) == -1)
> > +   goto bad;
> > +   break;
> > +   }
> >  
> > bcopy(>table->conf.timeout, , sizeof(tv));
> > if (connect(s, (struct 

Re: relayd ipv6 ttl check_icmp / check_tcp

[Florian Obser]

On Fri, Jun 23, 2017 at 01:52:52PM +0300, Kapetanakis Giannis wrote:
> Hi,
> 
> Using relayd's redirect/forward on ipv6 addresses I discovered problems 
> relating to setting TTL.
> 
> There is no check for address family and setsockopt tries to apply IP_TTL 
> always.
> 
> Without ip ttl on ipv6 table, check_icmp gives
> send_icmp: getsockopt: Invalid argument
> 
> With ip ttl on ipv6 table, check_tcp gives
> hce_notify_done: fdaa:10:1:9::11 (tcp socket option)
> 
> is the following diff valid?

the check_tcp hunk looks good and is OK florian@

> I've removed the IP_IPDEFTTL check. Was this ok?

Nope, relayd reuses the raw socket between config reloads (I think),
if the ttl gets removed from the config we need to reset to the
default. Don't think there is a getsockopt for v6, you can take a look
at the sysctl(3) song and dance in traceroute(8) how to do this
somewhat AF independet.

Also please make sure to not exceed 80 cols

> 
> regards,
> 
> Giannis
> 
> Index: check_icmp.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_icmp.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 check_icmp.c
> --- check_icmp.c  28 May 2017 10:39:15 -  1.45
> +++ check_icmp.c  23 Jun 2017 10:42:30 -
> @@ -165,7 +165,7 @@ send_icmp(int s, short event, void *arg)
>   struct icmp6_hdr*icp6;
>   ssize_t  r;
>   u_char   packet[ICMP_BUF_SIZE];
> - socklen_tslen, len;
> + socklen_tslen;
>   int  i = 0, ttl;
>   u_int32_tid;
>  
> @@ -221,18 +221,18 @@ send_icmp(int s, short event, void *arg)
>   }
>  
>   if ((ttl = host->conf.ttl) > 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - >conf.ttl, sizeof(int));
> - else {
> - /* Revert to default TTL */
> - len = sizeof(ttl);
> - if (getsockopt(s, IPPROTO_IP, IP_IPDEFTTL,
> - , ) == 0)
> - (void)setsockopt(s, IPPROTO_IP, IP_TTL,
> - , len);
> - else
> - log_warn("%s: getsockopt",__func__);
> - }
> + switch(cie->af) {
> + case AF_INET:
> + if (setsockopt(s, IPPROTO_IP, IP_TTL,
> + >conf.ttl, sizeof(int)) == -1)
> + log_warn("%s: 
> setsockopt",__func__);
> + break;
> + case AF_INET6:
> + if (setsockopt(s, IPPROTO_IPV6, 
> IPV6_UNICAST_HOPS,
> + >conf.ttl, sizeof(int)) == -1)
> + log_warn("%s: 
> setsockopt",__func__);
> + break;
> + }
>  
>   r = sendto(s, packet, sizeof(packet), 0, to, slen);
>   if (r == -1) {
> Index: check_tcp.c
> ===
> RCS file: /cvs/src/usr.sbin/relayd/check_tcp.c,v
> retrieving revision 1.54
> diff -u -p -r1.54 check_tcp.c
> --- check_tcp.c   28 May 2017 10:39:15 -  1.54
> +++ check_tcp.c   23 Jun 2017 10:42:30 -
> @@ -82,11 +82,19 @@ check_tcp(struct ctl_tcp_event *cte)
>   if (setsockopt(s, SOL_SOCKET, SO_LINGER, , sizeof(lng)) == -1)
>   goto bad;
>  
> - if (cte->host->conf.ttl > 0) {
> - if (setsockopt(s, IPPROTO_IP, IP_TTL,
> - >host->conf.ttl, sizeof(int)) == -1)
> - goto bad;
> - }
> + if (cte->host->conf.ttl > 0)
> + switch (cte->host->conf.ss.ss_family) {
> + case AF_INET:
> + if (setsockopt(s, IPPROTO_IP, IP_TTL,
> + >host->conf.ttl, sizeof(int)) == -1)
> + goto bad;
> + break;
> + case AF_INET6:
> + if (setsockopt(s, IPPROTO_IPV6, IPV6_UNICAST_HOPS,
> + >host->conf.ttl, sizeof(int)) == -1)
> + goto bad;
> + break;
> + }
>  
>   bcopy(>table->conf.timeout, , sizeof(tv));
>   if (connect(s, (struct sockaddr *)>host->conf.ss, len) == -1) {
> 

-- 
I'm not entirely sure you are real.