accel-ppp-users — This mailing list is for all users and developers.

Re: [Accel-ppp-users] [PATCH 0/7] iputils: stricter route deletion and cleanup

[Dmitry K. <xe...@ma...>]

>This series brings the ability to use the same parameters in
>iproute_del() as in iproute_add(). The objective is to ensure that
>we only delete routes that have the same properties as the ones we
>originally inserted.
>
>The rest of the series makes some cleanup in the rest of iputils.c,
>simplifying a bit netlink header generation.
>
>Patch 1 adds the 'src' and 'gw' parameters to iproute_del().
>
>Patch 2 uses the new 'gw' parameter in radius.c.
>
>Patch 7 uses 'src' in ipoe.c. I put this patch at the end of the series
>and mark it RFC as I don't have any working IPoE environment to test
>it. The route insertion and deletion process looks quite clear, and
>setting 'src' there seems to fit properly.
>
>Then patches 3 to 6 simplify a little bit the way netlink messages are
>generated, without modifying their semantic.
>
>Guillaume Nault (7):
>  iputils: add 'src' and 'gw' parameters to iproute_del()
>  radius: specify gateway in iproute_del()
>  iputils: set scope depending on gateway in iproute_{add,del}()
>  iputils: always set scope to RT_SCOPE_UNIVERSE in ip6route_{add,del}()
>  iputils: remove NLM_F_CREATE flag from ip6{route,addr}_del()
>  iputils: remove unnecessary NLM_F_ACK
>  ipoe: stricter route deletion
>
> accel-pppd/ctrl/ipoe/ipoe.c     | 10 +++++-----
> accel-pppd/libnetlink/iputils.c | 29 ++++++++++++++++-------------
> accel-pppd/libnetlink/iputils.h |  2 +-
> accel-pppd/radius/radius.c      |  2 +-
> 4 files changed, 23 insertions(+), 20 deletions(-)
>
>-- 
>2.20.1
>

Applied, thanks

-- 
Dmitry Kozlov

[Accel-ppp-users] [RFC PATCH 7/7] ipoe: stricter route deletion

[Guillaume N. <g....@al...>]

Rework the conditionals to make __ipoe_session_activate() and
ipoe_session_finished() follow the same logic:

  * Drop the second '!serv->opt_ifcfg' test in __ipoe_session_activate(),
    which is is already checked by the parent conditional.

  * Invert the order of the tests in ipoe_session_finished(), so that
    it uses the same conditions as __ipoe_session_activate().

Finally, set the 'src' parameter in iproute_del(), so that we can be
sure that the deleted route matches the one added by
__ipoe_session_activate().

Signed-off-by: Guillaume Nault <g....@al...>
---

Sending as RFC, as I couldn't test this patch beyond compilation.
Setting the source here looks quite logical, but an external review
would be in order.

 accel-pppd/ctrl/ipoe/ipoe.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index 0fe16ae1..eb06e7b7 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -981,9 +981,9 @@ static void __ipoe_session_activate(struct ipoe_session *ses)
 	ap_session_activate(&ses->ses);
 
 	if (ses->ifindex == -1 && !serv->opt_ifcfg) {
-		if (serv->opt_ip_unnumbered == 0)
+		if (!serv->opt_ip_unnumbered)
 			iproute_add(serv->ifindex, ses->router, ses->yiaddr, 0, conf_proto, ses->mask, 0);
-		else if (!serv->opt_ifcfg)
+		else
 			iproute_add(serv->ifindex, serv->opt_src ?: ses->router, ses->yiaddr, 0, conf_proto, 32, 0);
 	}
 
@@ -1166,10 +1166,10 @@ static void ipoe_session_finished(struct ap_session *s)
 			ipoe_nl_delete(ses->ifindex);
 	} else if (ses->started) {
 		if (!serv->opt_ifcfg) {
-			if (serv->opt_ip_unnumbered)
-				iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, 32, 0);
+			if (!serv->opt_ip_unnumbered)
+				iproute_del(serv->ifindex, ses->router, ses->yiaddr, 0, conf_proto, ses->mask, 0);
 			else
-				iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, ses->mask, 0);
+				iproute_del(serv->ifindex, serv->opt_src ?: ses->router, ses->yiaddr, 0, conf_proto, 32, 0);
 		}
 	}
 
-- 
2.20.1

[Accel-ppp-users] [PATCH 5/7] iputils: remove NLM_F_CREATE flag from ip6{route, addr}_del()

[Guillaume N. <g....@al...>]

These are deletion requests. NLM_F_CREATE is confusing for readers and
ignored by kernel.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/libnetlink/iputils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index f04f3489..5dfd097b 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -600,7 +600,7 @@ int __export ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len,
 	memset(&req, 0, sizeof(req) - 4096);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE;
+	req.n.nlmsg_flags = NLM_F_REQUEST;
 	req.n.nlmsg_type = RTM_DELROUTE;
 	req.i.rtm_family = AF_INET6;
 	req.i.rtm_table = RT_TABLE_MAIN;
@@ -708,7 +708,7 @@ int __export ip6addr_del(int ifindex, struct in6_addr *addr, int prefix_len)
 	memset(&req, 0, sizeof(req) - 4096);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifaddrmsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE;
+	req.n.nlmsg_flags = NLM_F_REQUEST;
 	req.n.nlmsg_type = RTM_DELADDR;
 	req.i.ifa_family = AF_INET6;
 	req.i.ifa_index = ifindex;
-- 
2.20.1

[Accel-ppp-users] [PATCH 6/7] iputils: remove unnecessary NLM_F_ACK

[Guillaume N. <g....@al...>]

Using NLM_F_ACK in these functions is confusing because they don't
parse any netlink response.

In fact, NLM_F_ACK is only required internally by rtnl_talk(), which
already adds it when its 'answer' parameter is NULL. Therefore it's
useless to manually set it in functions that don't set 'answer'.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/libnetlink/iputils.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index 5dfd097b..a1ededbf 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -169,7 +169,7 @@ int __export iplink_set_mtu(int ifindex, int mtu)
 	memset(&req, 0, sizeof(req) - 1024);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
+	req.n.nlmsg_flags = NLM_F_REQUEST;
 	req.n.nlmsg_type = RTM_SETLINK;
 	req.i.ifi_family = AF_UNSPEC;
 	req.i.ifi_index = ifindex;
@@ -200,7 +200,7 @@ int __export iplink_vlan_add(const char *ifname, int ifindex, int vid)
 	memset(&req, 0, sizeof(req) - 4096);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK | NLM_F_CREATE | NLM_F_EXCL;
+	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_CREATE | NLM_F_EXCL;
 	req.n.nlmsg_type = RTM_NEWLINK;
 	req.i.ifi_family = AF_UNSPEC;
 
@@ -243,7 +243,7 @@ int __export iplink_vlan_del(int ifindex)
 	memset(&req, 0, sizeof(req) - 4096);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct ifinfomsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
+	req.n.nlmsg_flags = NLM_F_REQUEST;
 	req.n.nlmsg_type = RTM_DELLINK;
 	req.i.ifi_family = AF_UNSPEC;
 	req.i.ifi_index = ifindex;
@@ -516,7 +516,7 @@ int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw
 	memset(&req, 0, sizeof(req) - 4096);
 
 	req.n.nlmsg_len = NLMSG_LENGTH(sizeof(struct rtmsg));
-	req.n.nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK;
+	req.n.nlmsg_flags = NLM_F_REQUEST;
 	req.n.nlmsg_type = RTM_DELROUTE;
 	req.i.rtm_family = AF_INET;
 	req.i.rtm_table = RT_TABLE_MAIN;
-- 
2.20.1

[Accel-ppp-users] [PATCH 4/7] iputils: always set scope to RT_SCOPE_UNIVERSE in ip6route_{add, del}()

[Guillaume N. <g....@al...>]

No need to be clever here. All IPv6 routes have global scope (kernel
ignores rtm_scope for IPv6 and always reports RT_SCOPE_UNIVERSE when
dumping such routes).

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/libnetlink/iputils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index ffa57267..f04f3489 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -563,7 +563,7 @@ int __export ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len,
 	req.n.nlmsg_type = RTM_NEWROUTE;
 	req.i.rtm_family = AF_INET6;
 	req.i.rtm_table = RT_TABLE_MAIN;
-	req.i.rtm_scope = (pref_len == 128) ? RT_SCOPE_HOST : RT_SCOPE_LINK;
+	req.i.rtm_scope = RT_SCOPE_UNIVERSE;
 	req.i.rtm_protocol = proto;
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = pref_len;
@@ -604,7 +604,7 @@ int __export ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len,
 	req.n.nlmsg_type = RTM_DELROUTE;
 	req.i.rtm_family = AF_INET6;
 	req.i.rtm_table = RT_TABLE_MAIN;
-	req.i.rtm_scope = (pref_len == 128) ? RT_SCOPE_HOST : RT_SCOPE_LINK;
+	req.i.rtm_scope = RT_SCOPE_UNIVERSE;
 	req.i.rtm_protocol = proto;
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = pref_len;
-- 
2.20.1

[Accel-ppp-users] [PATCH 3/7] iputils: set scope depending on gateway in iproute_{add, del}()

[Guillaume N. <g....@al...>]

>From a logical point of view, we have link scope if no gateway is
present, and global scope otherwise. Therefore it makes more sense
to set rtm_scope depending on 'gw' rather than on 'ifindex'.

Currently, callers of iproute_add() and iproute_del() either set
'ifindex' or 'gw', but never both. So even if confusing, the current
code results in right scope selection. However one can't figure this
out without analysing every caller.

We should set rtm_scope based on the presence of the gateway instead.
Given the current code base, that doesn't change the end result, but
that better maches the scope concept. Also, that's the way iproute2
does its selection.

Furthermore, it'd be perfectly valid to have both 'iface' and 'gw' set.
In that case, scope should be RT_SCOPE_UNIVERSE instead of
RT_SCOPE_LINK. Basing scope selection on 'gw' makes this case work
correctly.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/libnetlink/iputils.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index 5f830f09..ffa57267 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -477,7 +477,7 @@ int __export iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw
 	req.n.nlmsg_type = RTM_NEWROUTE;
 	req.i.rtm_family = AF_INET;
 	req.i.rtm_table = RT_TABLE_MAIN;
-	req.i.rtm_scope = ifindex ? RT_SCOPE_LINK : RT_SCOPE_UNIVERSE;
+	req.i.rtm_scope = gw ? RT_SCOPE_UNIVERSE : RT_SCOPE_LINK;
 	req.i.rtm_protocol = proto;
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = mask;
@@ -520,7 +520,7 @@ int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw
 	req.n.nlmsg_type = RTM_DELROUTE;
 	req.i.rtm_family = AF_INET;
 	req.i.rtm_table = RT_TABLE_MAIN;
-	req.i.rtm_scope = ifindex ? RT_SCOPE_LINK : RT_SCOPE_UNIVERSE;
+	req.i.rtm_scope = gw ? RT_SCOPE_UNIVERSE : RT_SCOPE_LINK;
 	req.i.rtm_protocol = proto;
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = mask;
-- 
2.20.1

[Accel-ppp-users] [PATCH 2/7] radius: specify gateway in iproute_del()

[Guillaume N. <g....@al...>]

Be more specific about which route we want to remove. By not specifying
the gateway we could remove a different route than the one we
originally inserted.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/radius/radius.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index abcb2e5d..d910c9ce 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -628,7 +628,7 @@ static void ses_finishing(struct ap_session *ses)
 
 	for (fr = rpd->fr; fr; fr = fr->next) {
 		if (fr->gw)
-			iproute_del(0, 0, fr->dst, 0, 3, fr->mask, fr->prio);
+			iproute_del(0, 0, fr->dst, fr->gw, 3, fr->mask, fr->prio);
 	}
 
 	if (rpd->acct_started || rpd->acct_req)
-- 
2.20.1

[Accel-ppp-users] [PATCH 1/7] iputils: add 'src' and 'gw' parameters to iproute_del()

[Guillaume N. <g....@al...>]

Rework iproute_del() to have the same parameters as iproute_add().
This will allow callers to specify more precisely the route they want
to delete.

Callers will later be converted to make use of these parameters to
ensure that the removed route precisely matches the one that was
originaly inserted.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ctrl/ipoe/ipoe.c     | 4 ++--
 accel-pppd/libnetlink/iputils.c | 9 ++++++---
 accel-pppd/libnetlink/iputils.h | 2 +-
 accel-pppd/radius/radius.c      | 2 +-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/accel-pppd/ctrl/ipoe/ipoe.c b/accel-pppd/ctrl/ipoe/ipoe.c
index ed79b656..0fe16ae1 100644
--- a/accel-pppd/ctrl/ipoe/ipoe.c
+++ b/accel-pppd/ctrl/ipoe/ipoe.c
@@ -1167,9 +1167,9 @@ static void ipoe_session_finished(struct ap_session *s)
 	} else if (ses->started) {
 		if (!serv->opt_ifcfg) {
 			if (serv->opt_ip_unnumbered)
-				iproute_del(serv->ifindex, ses->yiaddr, conf_proto, 32, 0);
+				iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, 32, 0);
 			else
-				iproute_del(serv->ifindex, ses->yiaddr, conf_proto, ses->mask, 0);
+				iproute_del(serv->ifindex, 0, ses->yiaddr, 0, conf_proto, ses->mask, 0);
 		}
 	}
 
diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index 343088f3..5f830f09 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -500,7 +500,7 @@ int __export iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw
 	return r;
 }
 
-int __export iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32_t prio)
+int __export iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio)
 {
 	struct ipaddr_req {
 		struct nlmsghdr n;
@@ -525,12 +525,15 @@ int __export iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = mask;
 
-	addattr32(&req.n, sizeof(req), RTA_DST, dst);
-
 	if (ifindex)
 		addattr32(&req.n, sizeof(req), RTA_OIF, ifindex);
+	if (src)
+		addattr32(&req.n, sizeof(req), RTA_PREFSRC, src);
+	if (gw)
+		addattr32(&req.n, sizeof(req), RTA_GATEWAY, gw);
 	if (prio)
 		addattr32(&req.n, sizeof(req), RTA_PRIORITY, prio);
+	addattr32(&req.n, sizeof(req), RTA_DST, dst);
 
 	if (rtnl_talk(rth, &req.n, 0, 0, NULL, NULL, NULL, 0) < 0)
 		r = -1;
diff --git a/accel-pppd/libnetlink/iputils.h b/accel-pppd/libnetlink/iputils.h
index 15104b16..78224745 100644
--- a/accel-pppd/libnetlink/iputils.h
+++ b/accel-pppd/libnetlink/iputils.h
@@ -21,7 +21,7 @@ int ipaddr_del(int ifindex, in_addr_t addr, int mask);
 int ipaddr_del_peer(int ifindex, in_addr_t addr, in_addr_t peer);
 
 int iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio);
-int iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32_t prio);
+int iproute_del(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int proto, int mask, uint32_t prio);
 in_addr_t iproute_get(in_addr_t dst, in_addr_t *gw);
 
 int ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio);
diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index 062e3b72..abcb2e5d 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -628,7 +628,7 @@ static void ses_finishing(struct ap_session *ses)
 
 	for (fr = rpd->fr; fr; fr = fr->next) {
 		if (fr->gw)
-			iproute_del(0, fr->dst, 3, fr->mask, fr->prio);
+			iproute_del(0, 0, fr->dst, 0, 3, fr->mask, fr->prio);
 	}
 
 	if (rpd->acct_started || rpd->acct_req)
-- 
2.20.1

[Accel-ppp-users] [PATCH 0/7] iputils: stricter route deletion and cleanup

[Guillaume N. <g....@al...>]

This series brings the ability to use the same parameters in
iproute_del() as in iproute_add(). The objective is to ensure that
we only delete routes that have the same properties as the ones we
originally inserted.

The rest of the series makes some cleanup in the rest of iputils.c,
simplifying a bit netlink header generation.

Patch 1 adds the 'src' and 'gw' parameters to iproute_del().

Patch 2 uses the new 'gw' parameter in radius.c.

Patch 7 uses 'src' in ipoe.c. I put this patch at the end of the series
and mark it RFC as I don't have any working IPoE environment to test
it. The route insertion and deletion process looks quite clear, and
setting 'src' there seems to fit properly.

Then patches 3 to 6 simplify a little bit the way netlink messages are
generated, without modifying their semantic.

Guillaume Nault (7):
  iputils: add 'src' and 'gw' parameters to iproute_del()
  radius: specify gateway in iproute_del()
  iputils: set scope depending on gateway in iproute_{add,del}()
  iputils: always set scope to RT_SCOPE_UNIVERSE in ip6route_{add,del}()
  iputils: remove NLM_F_CREATE flag from ip6{route,addr}_del()
  iputils: remove unnecessary NLM_F_ACK
  ipoe: stricter route deletion

 accel-pppd/ctrl/ipoe/ipoe.c     | 10 +++++-----
 accel-pppd/libnetlink/iputils.c | 29 ++++++++++++++++-------------
 accel-pppd/libnetlink/iputils.h |  2 +-
 accel-pppd/radius/radius.c      |  2 +-
 4 files changed, 23 insertions(+), 20 deletions(-)

-- 
2.20.1

[Accel-ppp-users] [PATCH] radius: rework Framed-Route attribute handling

[Guillaume N. <g....@al...>]

Parse Framed-Route attributes using u_parse_*() functions. This is
heavily based on the Framed-IPv6-Route parser. As a consequence, prefix
length is now mandatory.

While there, re-arrange struct frame_route to make it more like the
IPv6 version.

I haven't tried to keep the optional prefix length because the current
implementation contradicts RFC 2865's recommendation, which is to
respect the original classfull network addressing scheme (accel-ppp
always considers prefix length to be 32 when not given explicitly).

There's no point in handling classfull networks anymore (CIDR was
introduced more than 25 years ago...), but having our own special and
incompatible interpretation can be troublesome for operators
(especially since the behaviours differ in a subtle way that isn't
immediately visible).

Signed-off-by: Guillaume Nault <g....@al...>
---

Although my preference goes into making the prefix length explicit, I
can look into making it optional to keep the current behaviour. Just
let me know.

 accel-pppd/radius/radius.c   | 237 ++++++++++++++++++++---------------
 accel-pppd/radius/radius_p.h |  12 +-
 2 files changed, 145 insertions(+), 104 deletions(-)

diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index 062e3b72..da68667e 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -60,94 +60,6 @@ static struct ipdb_t ipdb;
 static mempool_t rpd_pool;
 static mempool_t auth_ctx_pool;
 
-static void parse_framed_route(struct radius_pd_t *rpd, const char *attr)
-{
-	char str[32];
-	char *ptr;
-	long int prio = 0;
-	in_addr_t dst;
-	in_addr_t gw;
-	int mask;
-	struct framed_route *fr;
-
-	ptr = strchr(attr, '/');
-	if (ptr && ptr - attr > 16)
-		goto out_err;
-
-	if (ptr) {
-		memcpy(str, attr, ptr - attr);
-		str[ptr - attr] = 0;
-	} else {
-		ptr = strchr(attr, ' ');
-		if (ptr) {
-			memcpy(str, attr, ptr - attr);
-			str[ptr - attr] = 0;
-		} else
-			strcpy(str, attr);
-	}
-
-	dst = inet_addr(str);
-	if (dst == INADDR_NONE)
-		goto out_err;
-
-	if (ptr) {
-		if (*ptr == '/') {
-			char *ptr2;
-			for (ptr2 = ++ptr; *ptr2 && *ptr2 != '.' && *ptr2 != ' '; ptr2++);
-			if (*ptr2 == '.' && ptr2 - ptr <= 16) {
-				in_addr_t a;
-				memcpy(str, ptr, ptr2 - ptr);
-				str[ptr2 - ptr] = 0;
-				a = ntohl(inet_addr(str));
-				if (a == INADDR_NONE)
-					goto out_err;
-				mask = 33 - htonl(inet_addr(str));
-				if (~((1<<(32 - mask)) - 1) != a)
-					goto out_err;
-			} else if (*ptr2 == ' ' || *ptr2 == 0) {
-				char *ptr3;
-				mask = strtol(ptr, &ptr3, 10);
-				if (mask < 0 || mask > 32 || ptr3 != ptr2)
-					goto out_err;
-			} else
-				goto out_err;
-		} else
-			mask = 32;
-
-		for (++ptr; *ptr && *ptr != ' '; ptr++);
-		if (*ptr == ' ')
-			gw = inet_addr(ptr + 1);
-		else if (*ptr == 0)
-			gw = 0;
-		else
-			goto out_err;
-
-		/* Parse priority, if any */
-		if (*ptr) {
-			for (++ptr; *ptr && *ptr != ' '; ptr++);
-			if (*ptr == ' ')
-				if (u_readlong(&prio, ptr + 1, 0, UINT32_MAX) < 0)
-					goto out_err;
-		}
-	} else {
-		mask = 32;
-		gw = 0;
-	}
-
-	fr = _malloc(sizeof (*fr));
-	fr->dst = dst;
-	fr->mask = mask;
-	fr->gw = gw;
-	fr->prio = prio;
-	fr->next = rpd->fr;
-	rpd->fr = fr;
-
-	return;
-
-out_err:
-	log_ppp_warn("radius: failed to parse Framed-Route=%s\n", attr);
-}
-
 /* Parse a RADIUS Framed-IPv6-Route string.
  *
  * Full format is like: "2001:db8::/32 fc00::1 2000"
@@ -245,6 +157,103 @@ static int parse_framed_ipv6_route(const char *str,
 	return 0;
 }
 
+/* Parse a RADIUS Framed-Route string.
+ *
+ * Full format is like: "192.0.2.0/24 203.0.113.1 8"
+ *
+ *   * "192.0.2.0/24" is the network prefix
+ *   * "203.0.113.1" is the gateway address
+ *   * "8" is the route metric (priority)
+ *
+ * The route metric can be omitted, in which case it is set to 0. This let the
+ * kernel use its own default route metric.
+ * If the route metric is not set, the gateway address can be omitted too. In
+ * this case, it's set to the unspecified address ('0.0.0.0'). This makes the
+ * route use the session's network interface directly rather than an IP
+ * gateway.
+ */
+static int parse_framed_route(const char *str, struct framed_route *fr)
+{
+	const char *ptr;
+	size_t len;
+
+	/* Skip leading spaces */
+	ptr = str + u_parse_spaces(str);
+
+	/* Get network prefix and prefix length */
+	len = u_parse_ip4cidr(ptr, &fr->prefix, &fr->plen);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting an IPv4 network prefix in CIDR notation\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* Check separator, unless string ends here */
+	len = u_parse_spaces(ptr);
+	if (!len && *ptr != '\0') {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " missing space after network prefix\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* If end of string, use no gateway and default metric */
+	if (*ptr == '\0') {
+		fr->gw.s_addr = INADDR_ANY;
+		fr->prio = 0;
+		return 0;
+	}
+
+	/* Get the gateway address */
+	len = u_parse_ip4addr(ptr, &fr->gw);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting a gateway IPv4 address\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* Again, separator or end of string required */
+	len = u_parse_spaces(ptr);
+	if (!len && *ptr != '\0') {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " missing space after gateway address\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* If end of string, use default metric */
+	if (*ptr == '\0') {
+		fr->prio = 0;
+		return 0;
+	}
+
+	/* Get route metric */
+	len = u_parse_u32(ptr, &fr->prio);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting a route metric between 0 and %u\n",
+			     str, ptr, UINT32_MAX);
+		return -1;
+	}
+	ptr += len;
+
+	/* Now this must be the end of the string */
+	if (!u_parse_endstr(ptr)) {
+		log_ppp_warn("radius: parsing Framed-Route attribute \"%s\" failed at \"%s\":"
+			     " unexpected data after route metric\n",
+			     str, ptr + u_parse_spaces(ptr));
+		return -1;
+	}
+
+	return 0;
+}
+
 static int rad_add_framed_ipv6_route(const char *str, struct radius_pd_t *rpd)
 {
 	struct framed_ip6_route *fr6;
@@ -267,6 +276,28 @@ err:
 	return -1;
 }
 
+static int rad_add_framed_route(const char *str, struct radius_pd_t *rpd)
+{
+	struct framed_route *fr;
+
+	fr = _malloc(sizeof(*fr));
+	if (!fr)
+		goto err;
+
+	if (parse_framed_route(str, fr) < 0)
+		goto err_fr;
+
+	fr->next = rpd->fr;
+	rpd->fr = fr;
+
+	return 0;
+
+err_fr:
+	_free(fr);
+err:
+	return -1;
+}
+
 int rad_proc_attrs(struct rad_req_t *req)
 {
 	struct ev_wins_t wins = {};
@@ -368,7 +399,7 @@ int rad_proc_attrs(struct rad_req_t *req)
 				rpd->ses->ifname_rename[attr->len] = 0;
 				break;
 			case Framed_Route:
-				parse_framed_route(rpd, attr->val.string);
+				rad_add_framed_route(attr->val.string, rpd);
 				break;
 			case Framed_IPv6_Route:
 				rad_add_framed_ipv6_route(attr->val.string, rpd);
@@ -578,10 +609,11 @@ static void ses_started(struct ap_session *ses)
 
 	for (fr6 = rpd->fr6; fr6; fr6 = fr6->next) {
 		bool gw_spec = !IN6_IS_ADDR_UNSPECIFIED(&fr6->gw);
-		char nbuf[INET6_ADDRSTRLEN];
-		char gwbuf[INET6_ADDRSTRLEN];
 
 		if (ip6route_add(gw_spec ? 0 : rpd->ses->ifindex, &fr6->prefix, fr6->plen, gw_spec ? &fr6->gw : NULL, 3, fr6->prio)) {
+			char gwbuf[INET6_ADDRSTRLEN];
+			char nbuf[INET6_ADDRSTRLEN];
+
 			log_ppp_warn("radius: failed to add route %s/%hhu %s %u\n",
 				     u_ip6str(&fr6->prefix, nbuf), fr6->plen,
 				     u_ip6str(&fr6->gw, gwbuf), fr6->prio);
@@ -589,11 +621,15 @@ static void ses_started(struct ap_session *ses)
 	}
 
 	for (fr = rpd->fr; fr; fr = fr->next) {
-		if (iproute_add(fr->gw ? 0 : rpd->ses->ifindex, 0, fr->dst, fr->gw, 3, fr->mask, fr->prio)) {
-			char dst[17], gw[17];
-			u_inet_ntoa(fr->dst, dst);
-			u_inet_ntoa(fr->gw, gw);
-			log_ppp_warn("radius: failed to add route %s/%i %s %u\n", dst, fr->mask, gw, fr->prio);
+		bool gw_spec = fr->gw.s_addr != INADDR_ANY;
+
+		if (iproute_add(gw_spec ? 0 : rpd->ses->ifindex, 0, fr->prefix.s_addr, fr->gw.s_addr, 3, fr->plen, fr->prio)) {
+			char gwbuf[INET_ADDRSTRLEN];
+			char nbuf[INET_ADDRSTRLEN];
+
+			log_ppp_warn("radius: failed to add route %s/%hhu %s %u\n",
+				     u_ip4str(&fr->prefix, nbuf), fr->plen,
+				     u_ip4str(&fr->gw, gwbuf), fr->prio);
 		}
 	}
 
@@ -627,8 +663,9 @@ static void ses_finishing(struct ap_session *ses)
 	}
 
 	for (fr = rpd->fr; fr; fr = fr->next) {
-		if (fr->gw)
-			iproute_del(0, fr->dst, 3, fr->mask, fr->prio);
+		/* Again, no need to remove routes with unspecified gateway. */
+		if (fr->gw.s_addr != INADDR_ANY)
+			iproute_del(0, fr->prefix.s_addr, 3, fr->plen, fr->prio);
 	}
 
 	if (rpd->acct_started || rpd->acct_req)
@@ -639,8 +676,8 @@ static void ses_finished(struct ap_session *ses)
 {
 	struct radius_pd_t *rpd = find_pd(ses);
 	struct ipv6db_addr_t *a;
-	struct framed_route *fr = rpd->fr;
 	struct framed_ip6_route *fr6;
+	struct framed_route *fr;
 
 	pthread_rwlock_wrlock(&sessions_lock);
 	pthread_mutex_lock(&rpd->lock);
@@ -699,8 +736,10 @@ static void ses_finished(struct ap_session *ses)
 		fr6 = next;
 	}
 
+	fr = rpd->fr;
 	while (fr) {
 		struct framed_route *next = fr->next;
+
 		_free(fr);
 		fr = next;
 	}
diff --git a/accel-pppd/radius/radius_p.h b/accel-pppd/radius/radius_p.h
index db8d277f..8ee30f6c 100644
--- a/accel-pppd/radius/radius_p.h
+++ b/accel-pppd/radius/radius_p.h
@@ -26,19 +26,21 @@ struct radius_auth_ctx {
 };
 
 struct framed_route {
-	in_addr_t dst;
-	int mask;
-	in_addr_t gw;
-	uint32_t prio;
 	struct framed_route *next;
+
+	struct in_addr prefix;
+	struct in_addr gw;
+	uint32_t prio;
+	uint8_t plen;
 };
 
 struct framed_ip6_route {
+	struct framed_ip6_route *next;
+
 	struct in6_addr prefix;
 	struct in6_addr gw;
 	uint32_t prio;
 	uint8_t plen;
-	struct framed_ip6_route *next;
 };
 
 struct radius_pd_t {
-- 
2.20.0

[Accel-ppp-users] [PATCH 3/3] iprange: rework range parsing using u_parse_*() functions

[Guillaume N. <g....@al...>]

Now that we have primitives for parsing IPv4 ranges, let's use them to
simplify parse_iprange().

Try u_parse_ip4cidr() first. In case of failure, try u_parse_ip4range().
If any of them succeeds, verify that there aren't spurious data
following the range definition. If everything is valid, either load the
range or disable the module (if the range is 0.0.0.0/0).

The diff is a bit ugly, but the implementation should be much clearer.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/iprange.c | 158 +++++++++++++++++--------------------------
 1 file changed, 63 insertions(+), 95 deletions(-)

diff --git a/accel-pppd/iprange.c b/accel-pppd/iprange.c
index 40c938e2..58321bd3 100644
--- a/accel-pppd/iprange.c
+++ b/accel-pppd/iprange.c
@@ -29,12 +29,6 @@ static pthread_mutex_t iprange_lock = PTHREAD_MUTEX_INITIALIZER;
 static bool conf_disable = false;
 static LIST_HEAD(client_ranges);
 
-/* Maximum IPv4 address length with CIDR notation but no extra 0,
- * e.g. "0xff.0xff.0xff.0xff/32".
- */
-#define CIDR_MAXLEN 22
-
-
 static void free_ranges(struct list_head *head)
 {
 	struct iprange_t *range;
@@ -46,115 +40,89 @@ static void free_ranges(struct list_head *head)
 	}
 }
 
+/* Parse a [client-ip-iprange] configuration entry.
+ * Ranges can be defined in CIDR notation ("192.0.2.0/24") or by specifying an
+ * upper bound for the last IPv4 byte, after a '-' character ("192.0.2.0-255").
+ * For simplicity, only mention the CIDR notation in error messages.
+ */
 static int parse_iprange(const char *str, struct iprange_t **range)
 {
-	char ipstr[CIDR_MAXLEN + 1] = { 0 };
-	struct iprange_t *_range;
-	struct in_addr addr;
-	char *suffix_str;
-	uint32_t ipmin;
-	uint32_t ipmax;
-	bool is_cidr;
-
-	/* Extra spaces and comments must have already been removed */
-	if (strpbrk(str, " \t#")) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " invalid space or comment character found\n",
-			  str);
-		return -1;
-	}
+	struct iprange_t *new_range;
+	struct in_addr base_addr;
+	const char *ptr;
+	uint32_t ip_min;
+	uint32_t ip_max;
+	uint8_t suffix;
+	size_t len;
 
 	if (!strcmp(str, "disable"))
 		goto disable;
 
-	strncpy(ipstr, str, CIDR_MAXLEN + 1);
-	if (ipstr[CIDR_MAXLEN] != '\0') {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " line too long\n",
-			  str);
-		return -1;
-	}
-
-	suffix_str = strpbrk(ipstr, "-/");
-	if (!suffix_str) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " unrecognised range format\n",
-			  str);
-		return -1;
-	}
-
-	is_cidr = *suffix_str == '/';
-	*suffix_str = '\0';
-	++suffix_str;
-
-	if (!u_parse_ip4addr(ipstr, &addr)) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " invalid IPv4 address \"%s\"\n",
-			  str, ipstr);
-		return -1;
-	}
-	ipmin = ntohl(addr.s_addr);
+	ptr = str;
 
-
-	/* If is_cidr is set, range is given with CIDR notation,
-	 * e.g. "192.0.2.0/24".
-	 * If unset, range is an IP address where the last octet is replaced by
-	 * an octet range, e.g. "192.0.2.0-255".
-	 */
-	if (is_cidr) {
-		long int prefix_len;
+	/* Try IPv4 CIDR notation first */
+	len = u_parse_ip4cidr(ptr, &base_addr, &suffix);
+	if (len) {
+		uint32_t addr_hbo;
 		uint32_t mask;
 
-		if (u_readlong(&prefix_len, suffix_str, 0, 32)) {
-			log_error("iprange: impossible to parse range \"%s\":"
-				  " invalid CIDR prefix length \"/%s\"\n",
-				  str, suffix_str);
-			return -1;
-		}
+		/* Cast to uint64_t to avoid undefined 32 bits shift on 32 bits
+		 * integer if 'suffix' is 0.
+		 */
+		mask = (uint64_t)0xffffffff << (32 - suffix);
+		addr_hbo = ntohl(base_addr.s_addr);
+		ip_min = addr_hbo & mask;
+		ip_max = addr_hbo | ~mask;
+
+		if (ip_min != addr_hbo) {
+			struct in_addr min_addr = { .s_addr = htonl(ip_min) };
+			char ipbuf[INET_ADDRSTRLEN];
 
-		/* Interpret /0 as disable request */
-		if (prefix_len == 0) {
-			if (ipmin != INADDR_ANY)
-				log_warn("iprange: %s is equivalent to 0.0.0.0/0 and disables the iprange module\n",
-					 str);
-			goto disable;
+			log_warn("iprange: network %s is equivalent to %s/%hhu\n",
+				 str, u_ip4str(&min_addr, ipbuf), suffix);
 		}
+		goto addrange;
+	}
 
-		mask = INADDR_BROADCAST << (32 - prefix_len);
-		if (ipmin != (ipmin & mask)) {
-			char buf[INET_ADDRSTRLEN] = { 0 };
+	/* Not an IPv4 CIDR, try the IPv4 range notation */
+	len = u_parse_ip4range(ptr, &base_addr, &suffix);
+	if (len) {
+		ip_min = ntohl(base_addr.s_addr);
+		ip_max = (ip_min & 0xffffff00) | suffix;
+		goto addrange;
+	}
 
-			ipmin &= mask;
-			addr.s_addr = htonl(ipmin);
-			log_warn("iprange: first IP of range %s will be %s\n",
-				 str, inet_ntop(AF_INET, &addr, buf,
-						sizeof(buf)));
-		}
+	log_error("iprange: parsing range \"%s\" failed:"
+		  " expecting an IPv4 network prefix in CIDR notation\n",
+		  str);
 
-		ipmax = ipmin | ~mask;
-	} else {
-		long int max;
+	return -1;
 
-		if (u_readlong(&max, suffix_str, ipmin & 0xff, 255)) {
-			log_error("iprange: impossible to parse range \"%s\":"
-				  " invalid upper bound \"-%s\"\n",
-				  str, suffix_str);
-			return -1;
-		}
+addrange:
+	ptr += len;
 
-		ipmax = (ipmin & 0xffffff00) | max;
+	if (!u_parse_endstr(ptr)) {
+		log_error("iprange: parsing range \"%s\" failed:"
+			  " unexpected data at \"%s\"\n",
+			  str, ptr);
+		return -1;
 	}
 
-	_range = _malloc(sizeof(*_range));
-	if (!_range) {
-		log_error("iprange: impossible to allocate range \"%s\":"
-			  " memory allocation failed\n", str);
+	if (ip_min == INADDR_ANY && ip_max == INADDR_BROADCAST)
+		goto disable;
+
+	new_range = _malloc(sizeof(*new_range));
+	if (!new_range) {
+		log_error("iprange: impossible to load range \"%s\":"
+			  " memory allocation failed\n",
+			  str);
 		return -1;
 	}
 
-	_range->begin = ipmin;
-	_range->end = ipmax;
-	*range = _range;
+	new_range->begin = ip_min;
+	new_range->end = ip_max;
+
+	*range = new_range;
 
 	return 0;
 
@@ -175,7 +143,7 @@ static bool load_ranges(struct list_head *list, const char *conf_sect)
 
 	list_for_each_entry(opt, &s->items, entry) {
 		/* Ignore parsing errors, parse_iprange() already logs suitable
-		 * error message.
+		 * error messages.
 		 */
 		if (parse_iprange(opt->name, &r) < 0)
 			continue;
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 1/3] utils: rework u_parse_ip4addr()

[Guillaume N. <g....@al...>]

Redefine u_parse_ip4addr() to match the behaviour of other u_parse_*()
functions:

  * Drop the err_msg parameter.
  * Return the number of bytes parsed instead of an error number.
  * Remove support for fancy IPv4 address notations.

There is currently only one user of u_parse_ip4addr() (in iprange.c).
Dropping the fancy IPv4 address representations is probably not going
to harm anyone (quite the opposite as many users don't realise that
leading 0 means octal and that plain integers can be considered IPv4
addresses).

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/iprange.c |  3 +--
 accel-pppd/utils.c   | 51 ++++++++++++++++++++++----------------------
 accel-pppd/utils.h   |  4 ++--
 3 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/accel-pppd/iprange.c b/accel-pppd/iprange.c
index f7b77a27..40c938e2 100644
--- a/accel-pppd/iprange.c
+++ b/accel-pppd/iprange.c
@@ -51,7 +51,6 @@ static int parse_iprange(const char *str, struct iprange_t **range)
 	char ipstr[CIDR_MAXLEN + 1] = { 0 };
 	struct iprange_t *_range;
 	struct in_addr addr;
-	const char *errmsg;
 	char *suffix_str;
 	uint32_t ipmin;
 	uint32_t ipmax;
@@ -88,7 +87,7 @@ static int parse_iprange(const char *str, struct iprange_t **range)
 	*suffix_str = '\0';
 	++suffix_str;
 
-	if (u_parse_ip4addr(ipstr, &addr, &errmsg)) {
+	if (!u_parse_ip4addr(ipstr, &addr)) {
 		log_error("iprange: impossible to parse range \"%s\":"
 			  " invalid IPv4 address \"%s\"\n",
 			  str, ipstr);
diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c
index 544c59e7..b1590544 100644
--- a/accel-pppd/utils.c
+++ b/accel-pppd/utils.c
@@ -1,7 +1,6 @@
 #include <arpa/inet.h>
 #include <ctype.h>
 #include <errno.h>
-#include <netdb.h>
 #include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -167,31 +166,6 @@ size_t __export u_parse_u32(const char *str, uint32_t *val)
 	return endptr - str;
 }
 
-int __export u_parse_ip4addr(const char *src, struct in_addr *addr,
-			     const char **err_msg)
-{
-	struct addrinfo hint = {
-		.ai_flags = AI_NUMERICHOST,
-		.ai_family = AF_INET,
-		.ai_socktype = 0,
-		.ai_protocol = 0,
-	};
-	struct addrinfo *ainfo;
-	int err;
-
-	err = getaddrinfo(src, NULL, &hint, &ainfo);
-	if (err) {
-		*err_msg = gai_strerror(err);
-		return err;
-	}
-
-	*addr = ((struct sockaddr_in *)ainfo->ai_addr)->sin_addr;
-
-	freeaddrinfo(ainfo);
-
-	return 0;
-}
-
 /* Parse an IPv6 address (for example "2001:db8::1").
  * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv6
  * address.
@@ -214,6 +188,31 @@ size_t __export u_parse_ip6addr(const char *str, struct in6_addr *addr)
 	return len;
 }
 
+/* Parse an IPv4 address in dotted-decimal format (for example "198.51.100.1").
+ * Other formats (hex "0xc6.0x33.0x64.0x1", octal "0306.063.0144.01", mixed
+ * "0xc6.51.0144.1", non dotted-quad "198.51.25601"...) are rejected.
+ *
+ * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv4
+ * address.
+ */
+size_t __export u_parse_ip4addr(const char *str, struct in_addr *addr)
+{
+	char buf[INET_ADDRSTRLEN];
+	size_t len;
+
+	len = strspn(str, ".0123456789");
+	if (!len || len >= sizeof(buf))
+		return 0;
+
+	memcpy(buf, str, len);
+	buf[len] = '\0';
+
+	if (inet_pton(AF_INET, buf, addr) != 1)
+		return 0;
+
+	return len;
+}
+
 /* Parse an IPv6 network prefix in CIDR notation (for example "2001:db8::/32").
  * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv6
  * network prefix.
diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h
index d3e06083..a7c38973 100644
--- a/accel-pppd/utils.h
+++ b/accel-pppd/utils.h
@@ -16,9 +16,9 @@ size_t u_parse_u8(const char *str, uint8_t *val);
 size_t u_parse_u16(const char *str, uint16_t *val);
 size_t u_parse_u32(const char *str, uint32_t *val);
 
-int u_parse_ip4addr(const char *src, struct in_addr *addr,
-		    const char **err_msg);
 size_t u_parse_ip6addr(const char *str, struct in6_addr *addr);
+size_t u_parse_ip4addr(const char *str, struct in_addr *addr);
+
 size_t u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t *plen);
 
 int u_randbuf(void *buf, size_t buf_len, int *err);
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 2/3] utils: add IPv4 string parsing helpers

[Guillaume N. <g....@al...>]

Define the IPv4 counterparts of u_ip6str() and u_parse_ip6cidr().
Also add the special u_parse_ip4range() which will be useful for
parsing the [client-ip-range] section of accel-ppp.conf.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/utils.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++
 accel-pppd/utils.h |  3 ++
 2 files changed, 82 insertions(+)

diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c
index b1590544..dd928c1a 100644
--- a/accel-pppd/utils.c
+++ b/accel-pppd/utils.c
@@ -30,6 +30,21 @@ char __export *u_ip6str(const struct in6_addr *addr, char *buf)
 	return buf;
 }
 
+/* Convenient wrapper around inet_ntop() to print IPv4 addresses.
+ * It stores a string representation of addr into buf, which must be at
+ * least INET_ADDRSTRLEN bytes long.
+ *
+ * Returns buf, which is guaranteed to contain a valid string even if an error
+ * occured.
+ */
+char __export *u_ip4str(const struct in_addr *addr, char *buf)
+{
+	if (!inet_ntop(AF_INET, addr, buf, INET_ADDRSTRLEN))
+		snprintf(buf, INET_ADDRSTRLEN, "< ERROR! >");
+
+	return buf;
+}
+
 void __export u_inet_ntoa(in_addr_t addr, char *str)
 {
 	addr = ntohl(addr);
@@ -242,6 +257,70 @@ size_t __export u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t
 	return ptr - str;
 }
 
+/* Parse an IPv4 network prefix in CIDR notation (for example "192.0.2.0/24").
+ * The IP address must be in dotted-decimal format.
+ * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv4
+ * network prefix.
+ */
+size_t __export u_parse_ip4cidr(const char *str, struct in_addr *netp, uint8_t *plen)
+{
+	const char *ptr = str;
+	size_t len;
+
+	len = u_parse_ip4addr(ptr, netp);
+	if (!len)
+		return 0;
+
+	ptr += len;
+	if (*ptr != '/')
+		return 0;
+
+	len = u_parse_u8(++ptr, plen);
+	if (!len)
+		return 0;
+
+	if (*plen > 32)
+		return 0;
+
+	ptr += len;
+
+	return ptr - str;
+}
+
+/* Parse an IPv4 address range (for example "192.0.2.0-255").
+ * The IP address must be in dotted-decimal format. The number following '-'
+ * is the upper bound of the address' least significant byte (the lower bound
+ * is given by the address itself). The upper bound must be bigger or equal
+ * than the lower bound.
+ *
+ * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv4
+ * address range.
+ */
+size_t __export u_parse_ip4range(const char *str, struct in_addr *base_ip, uint8_t *max)
+{
+	const char *ptr = str;
+	size_t len;
+
+	len = u_parse_ip4addr(ptr, base_ip);
+	if (!len)
+		return 0;
+
+	ptr += len;
+	if (*ptr != '-')
+		return 0;
+
+	len = u_parse_u8(++ptr, max);
+	if (!len)
+		return 0;
+
+	if (*max < (ntohl(base_ip->s_addr) & 0xff))
+		return 0;
+
+	ptr += len;
+
+	return ptr - str;
+}
+
 int __export u_randbuf(void *buf, size_t buf_len, int *err)
 {
 	uint8_t *u8buf = buf;
diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h
index a7c38973..06859a6b 100644
--- a/accel-pppd/utils.h
+++ b/accel-pppd/utils.h
@@ -5,6 +5,7 @@
 #include <stdint.h>
 
 char *u_ip6str(const struct in6_addr *addr, char *buf);
+char *u_ip4str(const struct in_addr *addr, char *buf);
 
 void u_inet_ntoa(in_addr_t, char *str);
 int u_readlong(long int *dst, const char *src, long int min, long int max);
@@ -20,6 +21,8 @@ size_t u_parse_ip6addr(const char *str, struct in6_addr *addr);
 size_t u_parse_ip4addr(const char *str, struct in_addr *addr);
 
 size_t u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t *plen);
+size_t u_parse_ip4cidr(const char *str, struct in_addr *netp, uint8_t *plen);
+size_t u_parse_ip4range(const char *str, struct in_addr *base_ip, uint8_t *max);
 
 int u_randbuf(void *buf, size_t buf_len, int *err);
 
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 0/3] utils: implement IPv4 parsing primitive for iprange

[Guillaume N. <g....@al...>]

This series extends utils.c with IPv4 parsing functions. The objective
is to simplify the code having to deal with IPv4 text representations
(for parsing and for printing).

Patch 1 redefines u_parse_ip4addr() to make it work like its IPv6
counterpart.

Patch 2 defines u_ip4str(), to easily print an IPv4 from a printf()-like
function. It also adds u_parse_ip4cidr() and u_parse_ip4range() to
simplify the parsing of IPv4 addresse ranges.

Patch 3 uses these new functions to simplify the parsing of the
[client-ip-range] section (accel-ppp.conf).

Next, I'm planning to rework the RADIUS Framed-Route attribute parsing
if this approach is considered useful.

Guillaume Nault (3):
  utils: rework u_parse_ip4addr()
  utils: add IPv4 string parsing helpers
  iprange: rework range parsing using u_parse_*() functions

 accel-pppd/iprange.c | 159 +++++++++++++++++--------------------------
 accel-pppd/utils.c   | 130 ++++++++++++++++++++++++++++-------
 accel-pppd/utils.h   |   7 +-
 3 files changed, 172 insertions(+), 124 deletions(-)

-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH] utils: fix typo in description of u_parse_endstr()

[Guillaume N. <g....@al...>]

u_parse_endstr() used to be u_parse_eos() in my internal repository.
I forgot to update the documentation when I renamed it.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/utils.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c
index 544c59e7..0295934f 100644
--- a/accel-pppd/utils.c
+++ b/accel-pppd/utils.c
@@ -70,7 +70,7 @@ size_t __export u_parse_spaces(const char *str)
  * Reads a sequence of space characters, followed by the end-of-string
  * mark ('\0').
  * Returns the number of characters parsed on success (that is, the number of
- * space characters plus one for '\0'). Beware that 'str + u_parse_eos(str)'
+ * space characters plus one for '\0'). Beware that 'str + u_parse_endstr(str)'
  * points to the next byte after the end of the string in this case.
  * Returns 0 if parsing fails (that is, if unexpected characers are found
  * before the end of the string).
-- 
2.20.0.rc1

Re: [Accel-ppp-users] [PATCH 0/3] radius: handle Framed-IPv6-Route attributes

[Dmitry K. <xe...@ma...>]

>This series adds support for the Framed-IPv6-Route RADIUS attribute.
>This is the IPv6 counterpart of the Framed-Route attribute.
>
>Patch 1 modifies ip6route_add() and ip6route_del() to handle optional
>gateway and metric.
>
>Patch 2 adds some string helpers to simplify parsing of the attribute.
>
>Patch 3 does the real job of supporting Framed-IPv6-Route.
>
>Helpers added in patch 2 would probably be helpful in other places
>too, as they simplify string handling and make such code more
>readable (IMO). If that's welcome, I can convert some other string
>parsing code to use them.
>
>Guillaume Nault (3):
>  libnetlink: add gateway and priority parameters to ip6route_*()
>  utils: add string parsing helpers
>  radius: implement Framed-IPv6-Route attribute

Applied, thanks

-- 
Dmitry Kozlov

[Accel-ppp-users] [PATCH 0/3] radius: handle Framed-IPv6-Route attributes

[Guillaume N. <g....@al...>]

This series adds support for the Framed-IPv6-Route RADIUS attribute.
This is the IPv6 counterpart of the Framed-Route attribute.

Patch 1 modifies ip6route_add() and ip6route_del() to handle optional
gateway and metric.

Patch 2 adds some string helpers to simplify parsing of the attribute.

Patch 3 does the real job of supporting Framed-IPv6-Route.

Helpers added in patch 2 would probably be helpful in other places
too, as they simplify string handling and make such code more
readable (IMO). If that's welcome, I can convert some other string
parsing code to use them.

Guillaume Nault (3):
  libnetlink: add gateway and priority parameters to ip6route_*()
  utils: add string parsing helpers
  radius: implement Framed-IPv6-Route attribute

 accel-pppd/ifcfg.c              |   2 +-
 accel-pppd/ipv6/dhcpv6.c        |   2 +-
 accel-pppd/libnetlink/iputils.c |  18 +++-
 accel-pppd/libnetlink/iputils.h |   4 +-
 accel-pppd/radius/radius.c      | 157 +++++++++++++++++++++++++++
 accel-pppd/radius/radius_p.h    |   9 ++
 accel-pppd/utils.c              | 181 ++++++++++++++++++++++++++++++++
 accel-pppd/utils.h              |  14 +++
 8 files changed, 380 insertions(+), 7 deletions(-)

-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 2/3] utils: add string parsing helpers

[Guillaume N. <g....@al...>]

Define parsers for IPv6 addresses and CIDR notations, unsigned
integers, separators (variable number of space characters) and end of
strings (variable number of spaces followed by '\0').

All of these functions work on constant string and return the number
bytes parsed. If the input string doesn't have the expected format,
these functions return 0 (no forward progress).

Also implement a convenient wrapper around inet_ntop() that can be used
easily in printf-like functions.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/utils.c | 181 +++++++++++++++++++++++++++++++++++++++++++++
 accel-pppd/utils.h |  14 ++++
 2 files changed, 195 insertions(+)

diff --git a/accel-pppd/utils.c b/accel-pppd/utils.c
index 3b87ee16..544c59e7 100644
--- a/accel-pppd/utils.c
+++ b/accel-pppd/utils.c
@@ -1,7 +1,11 @@
+#include <arpa/inet.h>
+#include <ctype.h>
 #include <errno.h>
 #include <netdb.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <unistd.h>
 #include <netinet/in.h>
 
@@ -12,6 +16,21 @@
 
 extern int urandom_fd;
 
+/* Convenient wrapper around inet_ntop() to print IPv6 addresses.
+ * It stores a string representation of addr into buf, which must be at
+ * least INET6_ADDRSTRLEN bytes long.
+ *
+ * Returns buf, which is guaranteed to contain a valid string even if an error
+ * occured.
+ */
+char __export *u_ip6str(const struct in6_addr *addr, char *buf)
+{
+	if (!inet_ntop(AF_INET6, addr, buf, INET6_ADDRSTRLEN))
+		snprintf(buf, INET6_ADDRSTRLEN, "< ERROR! >");
+
+	return buf;
+}
+
 void __export u_inet_ntoa(in_addr_t addr, char *str)
 {
 	addr = ntohl(addr);
@@ -37,6 +56,117 @@ int __export u_readlong(long int *dst, const char *src,
         }
 }
 
+/* Parse spaces.
+ * Returns the number of leading space characters in str.
+ * This is a convenient function around strspn() which preserves the look and
+ * feel of other u_parse_*() functions.
+ */
+size_t __export u_parse_spaces(const char *str)
+{
+	return strspn(str, " ");
+}
+
+/* Parse end of string.
+ * Reads a sequence of space characters, followed by the end-of-string
+ * mark ('\0').
+ * Returns the number of characters parsed on success (that is, the number of
+ * space characters plus one for '\0'). Beware that 'str + u_parse_eos(str)'
+ * points to the next byte after the end of the string in this case.
+ * Returns 0 if parsing fails (that is, if unexpected characers are found
+ * before the end of the string).
+ */
+size_t __export u_parse_endstr(const char *str)
+{
+	const char *end;
+
+	end = str + strspn(str, " ");
+	if (*end != '\0')
+		return 0;
+
+	++end;
+
+	return end - str;
+}
+
+/* Parse an 8 bits unsigned integer in base 10.
+ * Returns the number of bytes parsed on success.
+ * Returns 0 if str doesn't start with a valid number or if this number doesn't
+ * fit in 8 bits.
+ */
+size_t __export u_parse_u8(const char *str, uint8_t *val)
+{
+	char *endptr;
+	unsigned long ul;
+
+	/* strtoul() handles leading signs (+/-) and white spaces. Make sure we
+	 * parse raw numbers.
+	 */
+	if (!isdigit(*str))
+		return 0;
+
+	ul = strtoul(str, &endptr, 10);
+	if (ul > UINT8_MAX)
+		return 0;
+
+	*val = ul;
+
+	return endptr - str;
+}
+
+/* Parse a 16 bits unsigned integer in base 10.
+ * Returns the number of bytes parsed on success.
+ * Returns 0 if str doesn't start with a valid number or if this number doesn't
+ * fit in 16 bits.
+ */
+size_t __export u_parse_u16(const char *str, uint16_t *val)
+{
+	char *endptr;
+	unsigned long ul;
+
+	/* strtoul() handles leading signs (+/-) and white spaces. Make sure we
+	 * parse raw numbers.
+	 */
+	if (!isdigit(*str))
+		return 0;
+
+	ul = strtoul(str, &endptr, 10);
+	if (ul > UINT16_MAX)
+		return 0;
+
+	*val = ul;
+
+	return endptr - str;
+}
+
+/* Parse a 32 bits unsigned integer in base 10.
+ * Returns the number of bytes parsed on success.
+ * Returns 0 if str doesn't start with a valid number or if this number doesn't
+ * fit in 32 bits.
+ */
+size_t __export u_parse_u32(const char *str, uint32_t *val)
+{
+	char *endptr;
+	unsigned long ul;
+
+	/* strtoul() handles leading signs (+/-) and white spaces. Make sure we
+	 * parse raw numbers.
+	 */
+	if (!isdigit(*str))
+		return 0;
+
+	errno = 0;
+	ul = strtoul(str, &endptr, 10);
+	/* On platforms where unsigned longs are 32 bits wide, overflows would
+	 * return a valid UINT32_MAX value. So we need to check for ERANGE too.
+	 */
+	if (errno == ERANGE || ul > UINT32_MAX)
+		return 0;
+
+	*val = ul;
+
+	return endptr - str;
+}
+
 int __export u_parse_ip4addr(const char *src, struct in_addr *addr,
 			     const char **err_msg)
 {
@@ -62,6 +192,57 @@ int __export u_parse_ip4addr(const char *src, struct in_addr *addr,
 	return 0;
 }
 
+/* Parse an IPv6 address (for example "2001:db8::1").
+ * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv6
+ * address.
+ */
+size_t __export u_parse_ip6addr(const char *str, struct in6_addr *addr)
+{
+	char buf[INET6_ADDRSTRLEN];
+	size_t len;
+
+	len = strspn(str, ":0123456789abcdef");
+	if (!len || len >= sizeof(buf))
+		return 0;
+
+	memcpy(buf, str, len);
+	buf[len] = '\0';
+
+	if (inet_pton(AF_INET6, buf, addr) != 1)
+		return 0;
+
+	return len;
+}
+
+/* Parse an IPv6 network prefix in CIDR notation (for example "2001:db8::/32").
+ * Returns the number of bytes parsed, or 0 if str doesn't start with an IPv6
+ * network prefix.
+ */
+size_t __export u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t *plen)
+{
+	const char *ptr = str;
+	size_t len;
+
+	len = u_parse_ip6addr(ptr, netp);
+	if (!len)
+		return 0;
+
+	ptr += len;
+	if (*ptr != '/')
+		return 0;
+
+	len = u_parse_u8(++ptr, plen);
+	if (!len)
+		return 0;
+
+	if (*plen > 128)
+		return 0;
+
+	ptr += len;
+
+	return ptr - str;
+}
+
 int __export u_randbuf(void *buf, size_t buf_len, int *err)
 {
 	uint8_t *u8buf = buf;
diff --git a/accel-pppd/utils.h b/accel-pppd/utils.h
index 87582648..d3e06083 100644
--- a/accel-pppd/utils.h
+++ b/accel-pppd/utils.h
@@ -2,11 +2,25 @@
 #define __UTILS_H
 
 #include <netinet/in.h>
+#include <stdint.h>
+
+char *u_ip6str(const struct in6_addr *addr, char *buf);
 
 void u_inet_ntoa(in_addr_t, char *str);
 int u_readlong(long int *dst, const char *src, long int min, long int max);
+
+size_t u_parse_spaces(const char *str);
+size_t u_parse_endstr(const char *str);
+
+size_t u_parse_u8(const char *str, uint8_t *val);
+size_t u_parse_u16(const char *str, uint16_t *val);
+size_t u_parse_u32(const char *str, uint32_t *val);
+
 int u_parse_ip4addr(const char *src, struct in_addr *addr,
 		    const char **err_msg);
+size_t u_parse_ip6addr(const char *str, struct in6_addr *addr);
+size_t u_parse_ip6cidr(const char *str, struct in6_addr *netp, uint8_t *plen);
+
 int u_randbuf(void *buf, size_t buf_len, int *err);
 
 #endif
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 3/3] radius: implement Framed-IPv6-Route attribute

[Guillaume N. <g....@al...>]

Framed-IPv6-Route is the IPv6 counterpart of Framed-Route. It's only
used for defining routes to be added locally by accel-ppp. Routes that
should be announced to the peer using Router Advertisements should be
defined in the Route-IPv6-Information attribute (but that's currently
not implemented).

Framed-IPv6-Route format is:
<network in CIDR notation> [<gateway IPv6 address> [<route metric>]]

The gateway address and the route metric are optionals, but the metric
can only be set if a gateway address is given. One can use the
unspecified address '::' to define a route with no gateway and a
non-default route metric.

When no gateway address is defined, the session's network interface is
used directly.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/radius/radius.c   | 157 +++++++++++++++++++++++++++++++++++
 accel-pppd/radius/radius_p.h |   9 ++
 2 files changed, 166 insertions(+)

diff --git a/accel-pppd/radius/radius.c b/accel-pppd/radius/radius.c
index dd05b83f..062e3b72 100644
--- a/accel-pppd/radius/radius.c
+++ b/accel-pppd/radius/radius.c
@@ -1,5 +1,7 @@
+#include <netinet/in.h>
 #include <stdlib.h>
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <string.h>
 #include <unistd.h>
@@ -146,6 +148,125 @@ out_err:
 	log_ppp_warn("radius: failed to parse Framed-Route=%s\n", attr);
 }
 
+/* Parse a RADIUS Framed-IPv6-Route string.
+ *
+ * Full format is like: "2001:db8::/32 fc00::1 2000"
+ *
+ *   * "2001:db8::/32" is the network prefix
+ *   * "fc00::1" is the gateway address
+ *   * "2000" is the route metric (priority)
+ *
+ * The route metric can be omitted, in which case it is set to 0. This let the
+ * kernel use its own default route metric.
+ * If the route metric is not set, the gateway address can be omitted too. In
+ * this case, it's set to the unspecified address ('::'). This makes the route
+ * use the session's network interface directly rather than an IP gateway.
+ */
+static int parse_framed_ipv6_route(const char *str,
+				   struct framed_ip6_route *fr6)
+{
+	const char *ptr;
+	size_t len;
+
+	/* Skip leading spaces */
+	ptr = str + u_parse_spaces(str);
+
+	/* Get network prefix and prefix length */
+	len = u_parse_ip6cidr(ptr, &fr6->prefix, &fr6->plen);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting an IPv6 network prefix in CIDR notation\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* Check separator, unless string ends here */
+	len = u_parse_spaces(ptr);
+	if (!len && *ptr != '\0') {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " missing space after network prefix\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* If end of string, use no gateway and default metric */
+	if (*ptr == '\0') {
+		fr6->gw = in6addr_any;
+		fr6->prio = 0;
+		return 0;
+	}
+
+	/* Get the gateway address */
+	len = u_parse_ip6addr(ptr, &fr6->gw);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting a gateway IPv6 address\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* Again, separator or end of string required */
+	len = u_parse_spaces(ptr);
+	if (!len && *ptr != '\0') {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " missing space after gateway address\n",
+			     str, ptr);
+		return -1;
+	}
+	ptr += len;
+
+	/* If end of string, use default metric */
+	if (*ptr == '\0') {
+		fr6->prio = 0;
+		return 0;
+	}
+
+	/* Get route metric */
+	len = u_parse_u32(ptr, &fr6->prio);
+	if (!len) {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " expecting a route metric between 0 and %u\n",
+			     str, ptr, UINT32_MAX);
+		return -1;
+	}
+	ptr += len;
+
+	/* Now this must be the end of the string */
+	if (!u_parse_endstr(ptr)) {
+		log_ppp_warn("radius: parsing Framed-IPv6-Route attribute \"%s\" failed at \"%s\":"
+			     " unexpected data after route metric\n",
+			     str, ptr + u_parse_spaces(ptr));
+		return -1;
+	}
+
+	return 0;
+}
+
+static int rad_add_framed_ipv6_route(const char *str, struct radius_pd_t *rpd)
+{
+	struct framed_ip6_route *fr6;
+
+	fr6 = _malloc(sizeof(*fr6));
+	if (!fr6)
+		goto err;
+
+	if (parse_framed_ipv6_route(str, fr6) < 0)
+		goto err_fr6;
+
+	fr6->next = rpd->fr6;
+	rpd->fr6 = fr6;
+
+	return 0;
+
+err_fr6:
+	_free(fr6);
+err:
+	return -1;
+}
+
 int rad_proc_attrs(struct rad_req_t *req)
 {
 	struct ev_wins_t wins = {};
@@ -249,6 +370,9 @@ int rad_proc_attrs(struct rad_req_t *req)
 			case Framed_Route:
 				parse_framed_route(rpd, attr->val.string);
 				break;
+			case Framed_IPv6_Route:
+				rad_add_framed_ipv6_route(attr->val.string, rpd);
+				break;
 		}
 	}
 
@@ -444,6 +568,7 @@ static void ses_acct_start(struct ap_session *ses)
 static void ses_started(struct ap_session *ses)
 {
 	struct radius_pd_t *rpd = find_pd(ses);
+	struct framed_ip6_route *fr6;
 	struct framed_route *fr;
 
 	if (rpd->session_timeout.expire_tv.tv_sec) {
@@ -451,6 +576,18 @@ static void ses_started(struct ap_session *ses)
 		triton_timer_add(ses->ctrl->ctx, &rpd->session_timeout, 0);
 	}
 
+	for (fr6 = rpd->fr6; fr6; fr6 = fr6->next) {
+		bool gw_spec = !IN6_IS_ADDR_UNSPECIFIED(&fr6->gw);
+		char nbuf[INET6_ADDRSTRLEN];
+		char gwbuf[INET6_ADDRSTRLEN];
+
+		if (ip6route_add(gw_spec ? 0 : rpd->ses->ifindex, &fr6->prefix, fr6->plen, gw_spec ? &fr6->gw : NULL, 3, fr6->prio)) {
+			log_ppp_warn("radius: failed to add route %s/%hhu %s %u\n",
+				     u_ip6str(&fr6->prefix, nbuf), fr6->plen,
+				     u_ip6str(&fr6->gw, gwbuf), fr6->prio);
+		}
+	}
+
 	for (fr = rpd->fr; fr; fr = fr->next) {
 		if (iproute_add(fr->gw ? 0 : rpd->ses->ifindex, 0, fr->dst, fr->gw, 3, fr->mask, fr->prio)) {
 			char dst[17], gw[17];
@@ -469,6 +606,7 @@ static void ses_started(struct ap_session *ses)
 static void ses_finishing(struct ap_session *ses)
 {
 	struct radius_pd_t *rpd = find_pd(ses);
+	struct framed_ip6_route *fr6;
 	struct framed_route *fr;
 
 	if (rpd->auth_ctx) {
@@ -478,6 +616,16 @@ static void ses_finishing(struct ap_session *ses)
 		rpd->auth_ctx = NULL;
 	}
 
+	for (fr6 = rpd->fr6; fr6; fr6 = fr6->next) {
+		/* Routes that have an unspecified gateway have been defined
+		 * using the session's virtual network interface. No need to
+		 * delete those routes here: kernel automatically drops them
+		 * when the interface is removed.
+		 */
+		if (!IN6_IS_ADDR_UNSPECIFIED(&fr6->gw))
+			ip6route_del(0, &fr6->prefix, fr6->plen, &fr6->gw, 3, fr6->prio);
+	}
+
 	for (fr = rpd->fr; fr; fr = fr->next) {
 		if (fr->gw)
 			iproute_del(0, fr->dst, 3, fr->mask, fr->prio);
@@ -492,6 +640,7 @@ static void ses_finished(struct ap_session *ses)
 	struct radius_pd_t *rpd = find_pd(ses);
 	struct ipv6db_addr_t *a;
 	struct framed_route *fr = rpd->fr;
+	struct framed_ip6_route *fr6;
 
 	pthread_rwlock_wrlock(&sessions_lock);
 	pthread_mutex_lock(&rpd->lock);
@@ -542,6 +691,14 @@ static void ses_finished(struct ap_session *ses)
 		_free(a);
 	}
 
+	fr6 = rpd->fr6;
+	while (fr6) {
+		struct framed_ip6_route *next = fr6->next;
+
+		_free(fr6);
+		fr6 = next;
+	}
+
 	while (fr) {
 		struct framed_route *next = fr->next;
 		_free(fr);
diff --git a/accel-pppd/radius/radius_p.h b/accel-pppd/radius/radius_p.h
index 1ce31c03..db8d277f 100644
--- a/accel-pppd/radius/radius_p.h
+++ b/accel-pppd/radius/radius_p.h
@@ -33,6 +33,14 @@ struct framed_route {
 	struct framed_route *next;
 };
 
+struct framed_ip6_route {
+	struct in6_addr prefix;
+	struct in6_addr gw;
+	uint32_t prio;
+	uint8_t plen;
+	struct framed_ip6_route *next;
+};
+
 struct radius_pd_t {
 	struct list_head entry;
 	struct ap_private pd;
@@ -66,6 +74,7 @@ struct radius_pd_t {
 	int termination_action;
 
 	struct framed_route *fr;
+	struct framed_ip6_route *fr6;
 
 	struct radius_auth_ctx *auth_ctx;
 
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 1/3] libnetlink: add gateway and priority parameters to ip6route_*()

[Guillaume N. <g....@al...>]

Let callers set a gateway and a priority to IPv6 routes. This is
necessary for implementing the RADIUS Framed-IPv6-Route attribute.

Also let ip6route_del() configure .rtm_protocol. This is already
implemented in ip6route_add(), so we need to add the ip6route_del()
counterpart. Otherwise, we couldn't delete routes that were added using
a non-zero protocol.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ifcfg.c              |  2 +-
 accel-pppd/ipv6/dhcpv6.c        |  2 +-
 accel-pppd/libnetlink/iputils.c | 18 +++++++++++++++---
 accel-pppd/libnetlink/iputils.h |  4 ++--
 4 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/accel-pppd/ifcfg.c b/accel-pppd/ifcfg.c
index 8fdfde7d..fbd11bf5 100644
--- a/accel-pppd/ifcfg.c
+++ b/accel-pppd/ifcfg.c
@@ -222,7 +222,7 @@ void __export ap_session_ifdown(struct ap_session *ses)
 			if (!a->installed)
 				continue;
 			if (a->prefix_len > 64)
-				ip6route_del(ses->ifindex, &a->addr, a->prefix_len);
+				ip6route_del(ses->ifindex, &a->addr, a->prefix_len, NULL, 0, 0);
 			else {
 				struct in6_addr addr;
 				memcpy(addr.s6_addr, &a->addr, 8);
diff --git a/accel-pppd/ipv6/dhcpv6.c b/accel-pppd/ipv6/dhcpv6.c
index 36a5f15e..be24858c 100644
--- a/accel-pppd/ipv6/dhcpv6.c
+++ b/accel-pppd/ipv6/dhcpv6.c
@@ -156,7 +156,7 @@ static void ev_ses_finished(struct ap_session *ses)
 		if (pd->dp_active) {
 			struct ipv6db_addr_t *p;
 			list_for_each_entry(p, &ses->ipv6_dp->prefix_list, entry)
-				ip6route_del(0, &p->addr, p->prefix_len);
+				ip6route_del(0, &p->addr, p->prefix_len, NULL, 0, 0);
 		}
 
 		ipdb_put_ipv6_prefix(ses, ses->ipv6_dp);
diff --git a/accel-pppd/libnetlink/iputils.c b/accel-pppd/libnetlink/iputils.c
index ad6005f1..343088f3 100644
--- a/accel-pppd/libnetlink/iputils.c
+++ b/accel-pppd/libnetlink/iputils.c
@@ -540,7 +540,7 @@ int __export iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32
 	return r;
 }
 
-int __export ip6route_add(int ifindex, struct in6_addr *dst, int pref_len, int proto)
+int __export ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio)
 {
 	struct ipaddr_req {
 		struct nlmsghdr n;
@@ -566,7 +566,12 @@ int __export ip6route_add(int ifindex, struct in6_addr *dst, int pref_len, int p
 	req.i.rtm_dst_len = pref_len;
 
 	addattr_l(&req.n, sizeof(req), RTA_DST, dst, sizeof(*dst));
-	addattr32(&req.n, sizeof(req), RTA_OIF, ifindex);
+	if (ifindex)
+		addattr32(&req.n, sizeof(req), RTA_OIF, ifindex);
+	if (gw)
+		addattr_l(&req.n, sizeof(req), RTA_GATEWAY, gw, sizeof(*gw));
+	if (prio)
+		addattr32(&req.n, sizeof(req), RTA_PRIORITY, prio);
 
 	if (rtnl_talk(rth, &req.n, 0, 0, NULL, NULL, NULL, 0) < 0)
 		r = -1;
@@ -576,7 +581,7 @@ int __export ip6route_add(int ifindex, struct in6_addr *dst, int pref_len, int p
 	return r;
 }
 
-int __export ip6route_del(int ifindex, struct in6_addr *dst, int pref_len)
+int __export ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio)
 {
 	struct ipaddr_req {
 		struct nlmsghdr n;
@@ -597,10 +602,17 @@ int __export ip6route_del(int ifindex, struct in6_addr *dst, int pref_len)
 	req.i.rtm_family = AF_INET6;
 	req.i.rtm_table = RT_TABLE_MAIN;
 	req.i.rtm_scope = (pref_len == 128) ? RT_SCOPE_HOST : RT_SCOPE_LINK;
+	req.i.rtm_protocol = proto;
 	req.i.rtm_type = RTN_UNICAST;
 	req.i.rtm_dst_len = pref_len;
 
 	addattr_l(&req.n, sizeof(req), RTA_DST, dst, sizeof(*dst));
+	if (ifindex)
+		addattr32(&req.n, sizeof(req), RTA_OIF, ifindex);
+	if (gw)
+		addattr_l(&req.n, sizeof(req), RTA_GATEWAY, gw, sizeof(*gw));
+	if (prio)
+		addattr32(&req.n, sizeof(req), RTA_PRIORITY, prio);
 
 	if (rtnl_talk(rth, &req.n, 0, 0, NULL, NULL, NULL, 0) < 0)
 		r = -1;
diff --git a/accel-pppd/libnetlink/iputils.h b/accel-pppd/libnetlink/iputils.h
index 6c1d3878..15104b16 100644
--- a/accel-pppd/libnetlink/iputils.h
+++ b/accel-pppd/libnetlink/iputils.h
@@ -24,8 +24,8 @@ int iproute_add(int ifindex, in_addr_t src, in_addr_t dst, in_addr_t gw, int pro
 int iproute_del(int ifindex, in_addr_t dst, int proto, int mask, uint32_t prio);
 in_addr_t iproute_get(in_addr_t dst, in_addr_t *gw);
 
-int ip6route_add(int ifindex, struct in6_addr *dst, int prefix_len, int proto);
-int ip6route_del(int ifindex, struct in6_addr *dst, int prefix_len);
+int ip6route_add(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio);
+int ip6route_del(int ifindex, const struct in6_addr *dst, int pref_len, const struct in6_addr *gw, int proto, uint32_t prio);
 int ip6addr_add(int ifindex, struct in6_addr *addr, int prefix_len);
 int ip6addr_add_peer(int ifindex, struct in6_addr *addr, struct in6_addr *peer_addr);
 int ip6addr_del(int ifindex, struct in6_addr *addr, int prefix_len);
-- 
2.20.0.rc1

Re: [Accel-ppp-users] [PATCH] ppp: use random LCP (and NCP) identifiers

[Dmitry K. <xe...@ma...>]

>In DSL setups, it's common to have an intermediate equipment,
>potentially managed by a different operator, between the two PPP
>endpoints. In such setups, the client establishes a PPPoE or L2TP
>session with the intermediate equipment. They perform LCP negotiation
>and eventually get to the authentication phase. Based on the client's
>username, the intermediate equipment then establishes another L2TP
>session with the final PPP endpoint (accel-ppp). At this point, the
>intermediate equipment forwards any PPP frame received on one side to
>the other side, so that it becomes transparent to PPP frames.
>Then accel-ppp starts an LCP negotiation again, performs
>authentication, negotiates NCPs and finally forwards IP packets to and
>from the client.
>
>+--------+                        +--------------+                    +-----------+
>| Client |------------------------| Intermediate |--------------------| accel-ppp |
>|        |                        | equipment    |                    |           |
>+--------+                        +--------------+                    +-----------+
>          <-- First hop PPPoE  -->                <--  Second hop  -->
>or L2TP session                         L2TP session
>
>          <----------------- End to end PPP session ----------------->
>
>Therefore, from the client point of view, two LCP negotiations occur.
>LCP re-negotiation is explicitly handled by RFC 1661 and even
>non-conforming PPP clients generally cope with this situation well
>enough (as long as LCP re-negotiation occurs before the authentication
>phase completes).
>
>However, accel-ppp always starts its LCP negotiation with an identifier
>set to 1. If the previous LCP negotiation also used identifier 1, then
>some clients (at least MikroTik products) consider that the
>Configure-Request sent by accel-ppp is part of the previous LCP
>negotiation and refuse to return to link establishment phase as
>mandated by section 3.4 of RFC 1661.
>
>We can easily work around this problem by using random identifiers.
>This maximises the chances that accel-ppp picks a different identifier
>than the intermediate equipment and avoids falling into the MikroTik
>problem. In case of bad luck and the chosen identifier is the same as
>the one used for the original LCP negotiation, then PPP establishment
>fails and the client tries to reconnect until the intermediate
>equipment and accel-ppp pick up different numbers. So the connection
>eventually succeeds.
>
>The identifier is set in ppp_fsm_init(), so it also affects NCPs.
>Therefore, IPCP and IPv6CP now also use random identifiers.
>
>We need to define 'id' and 'recv_id' in struct ppp_fsm_t as uint8_t,
>otherwise they could be chosen larger than 255 and comparing their
>value with the 8-bits values found in received packets would fail (this
>was generally not a problem when id was initially set to 1 and wouldn't
>grow much).
>
>Also, let's seed random() at startup, so that we don't end up with the
>same sequences across restarts. This also benefits other users of
>random(), like LCP magic numbers.

Applied

-- 
Dmitry Kozlov

Re: [Accel-ppp-users] [PATCH 0/2] auth: fix negociation of authentication when requested by peer

[Dmitry K. <xe...@ma...>]

>We have no way to authenticate ourself, we only know how to
>authenticate the peer. Therefore we should reject any request made by
>the peer to authenticate ourself. Failure to do so results in stalls
>during the PPP authentication phase because accel-ppp never sends the
>credentials the peer asked for.
>
>Patch 1 rejects the Authentication-Protocol option sent by the peer and
>expands on the reasons to do so.
>
>Patch 2 removes a callback function that patch 1 made unused.
>
>I must say that I can't really follow the logic of the original
>auth_recv_conf_req() function. So it's always possible that I've missed
>something. However, the principle of this series is that we really have
>no way to authenticate ourself to the peer, so we should always reject
>such attempts. This behaviour looks sane. We've run with a similar
>patch for a few years and it helped us maximise compatibility with
>broken clients.
>
>Guillaume Nault (2):
>  lcp: reject Authentication-Protocol option in Configure-Request
>    packets
>  auth: remove .recv_conf_req from struct ppp_auth_handler_t

Applied, thanks

-- 
Dmitry Kozlov

[Accel-ppp-users] [PATCH] ppp: use random LCP (and NCP) identifiers

[Guillaume N. <g....@al...>]

In DSL setups, it's common to have an intermediate equipment,
potentially managed by a different operator, between the two PPP
endpoints. In such setups, the client establishes a PPPoE or L2TP
session with the intermediate equipment. They perform LCP negotiation
and eventually get to the authentication phase. Based on the client's
username, the intermediate equipment then establishes another L2TP
session with the final PPP endpoint (accel-ppp). At this point, the
intermediate equipment forwards any PPP frame received on one side to
the other side, so that it becomes transparent to PPP frames.
Then accel-ppp starts an LCP negotiation again, performs
authentication, negotiates NCPs and finally forwards IP packets to and
from the client.

+--------+                        +--------------+                    +-----------+
| Client |------------------------| Intermediate |--------------------| accel-ppp |
|        |                        | equipment    |                    |           |
+--------+                        +--------------+                    +-----------+
          <-- First hop PPPoE  -->                <--  Second hop  -->
	      or L2TP session                         L2TP session

          <----------------- End to end PPP session ----------------->

Therefore, from the client point of view, two LCP negotiations occur.
LCP re-negotiation is explicitly handled by RFC 1661 and even
non-conforming PPP clients generally cope with this situation well
enough (as long as LCP re-negotiation occurs before the authentication
phase completes).

However, accel-ppp always starts its LCP negotiation with an identifier
set to 1. If the previous LCP negotiation also used identifier 1, then
some clients (at least MikroTik products) consider that the
Configure-Request sent by accel-ppp is part of the previous LCP
negotiation and refuse to return to link establishment phase as
mandated by section 3.4 of RFC 1661.

We can easily work around this problem by using random identifiers.
This maximises the chances that accel-ppp picks a different identifier
than the intermediate equipment and avoids falling into the MikroTik
problem. In case of bad luck and the chosen identifier is the same as
the one used for the original LCP negotiation, then PPP establishment
fails and the client tries to reconnect until the intermediate
equipment and accel-ppp pick up different numbers. So the connection
eventually succeeds.

The identifier is set in ppp_fsm_init(), so it also affects NCPs.
Therefore, IPCP and IPv6CP now also use random identifiers.

We need to define 'id' and 'recv_id' in struct ppp_fsm_t as uint8_t,
otherwise they could be chosen larger than 255 and comparing their
value with the 8-bits values found in received packets would fail (this
was generally not a problem when id was initially set to 1 and wouldn't
grow much).

Also, let's seed random() at startup, so that we don't end up with the
same sequences across restarts. This also benefits other users of
random(), like LCP magic numbers.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/main.c        | 2 ++
 accel-pppd/ppp/ppp_fsm.c | 2 +-
 accel-pppd/ppp/ppp_fsm.h | 4 ++--
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/accel-pppd/main.c b/accel-pppd/main.c
index 2992026c..ecffb7c1 100644
--- a/accel-pppd/main.c
+++ b/accel-pppd/main.c
@@ -283,6 +283,8 @@ int main(int _argc, char **_argv)
 			sleep(1);
 	}
 
+	srandom(time(NULL));
+
 	if (triton_init(conf_file))
 		_exit(EXIT_FAILURE);
 
diff --git a/accel-pppd/ppp/ppp_fsm.c b/accel-pppd/ppp/ppp_fsm.c
index bb45d5cb..c884143c 100644
--- a/accel-pppd/ppp/ppp_fsm.c
+++ b/accel-pppd/ppp/ppp_fsm.c
@@ -36,7 +36,7 @@ void ppp_fsm_init(struct ppp_fsm_t *layer)
 	layer->max_configure = conf_max_configure;
 	layer->max_failure = conf_max_failure;
 
-	layer->id = 1;
+	layer->id = random();
 }
 void ppp_fsm_free(struct ppp_fsm_t *layer)
 {
diff --git a/accel-pppd/ppp/ppp_fsm.h b/accel-pppd/ppp/ppp_fsm.h
index 6208d9ad..60b8aadb 100644
--- a/accel-pppd/ppp/ppp_fsm.h
+++ b/accel-pppd/ppp/ppp_fsm.h
@@ -36,8 +36,8 @@ struct ppp_fsm_t
 	int max_failure;
 	int conf_failure;
 
-	int id;
-	int recv_id;
+	uint8_t id;
+	uint8_t recv_id;
 
 	//fsm handling
 	void (*layer_up)(struct ppp_fsm_t*);
-- 
2.20.0.rc1

[Accel-ppp-users] [PATCH 1/2] lcp: reject Authentication-Protocol option in Configure-Request packets

[Guillaume N. <g....@al...>]

If we receive a Configure-Request packet, that means the peer wants us
to authenticate to him. However, none of our authentication backends
(PAP, CHAP and MSCHAP v1/v2) supports authenticating ourself to the
peer. Therefore, the LCP negotiation completes, but we hang in the
authentication phase because accel-ppp never sends any credential.

We should reject the Authentication-Protocol option found in
Configure-Request packets sent by the peer. This way, the peer knows
that we won't authenticate to him. Then it's up to him to keep
connecting without authentication from our side or to drop the
connection.

This doesn't change the way we request the peer to authenticate to us.
That part of the negotiation is handled by Configure-Request packets
that are sent by us (not those sent by the peer).

In practice some PPP clients wouldn't connect with the previous
behaviour, but are perfectly happy with their Authentication-Protocol
option being rejected. They just resend their Configure-Request without
requesting authentication from our side.

Also, since the peer_auth field of struct auth_option_t is never set
anymore, we can remove the conditionals in auth_recv_conf_nak() and
auth_recv_conf_rej().

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/ppp/ppp_auth.c | 53 +--------------------------------------
 1 file changed, 1 insertion(+), 52 deletions(-)

diff --git a/accel-pppd/ppp/ppp_auth.c b/accel-pppd/ppp/ppp_auth.c
index 59248995..c475dadc 100644
--- a/accel-pppd/ppp/ppp_auth.c
+++ b/accel-pppd/ppp/ppp_auth.c
@@ -39,7 +39,6 @@ struct auth_option_t
 	struct lcp_option_t opt;
 	struct list_head auth_list;
 	struct auth_data_t *auth;
-	struct auth_data_t *peer_auth;
 	int started:1;
 };
 
@@ -142,57 +141,11 @@ static int auth_send_conf_req(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 
 static int auth_recv_conf_req(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, uint8_t *ptr)
 {
-	struct auth_option_t *auth_opt = container_of(opt,typeof(*auth_opt),opt);
-	struct lcp_opt16_t *opt16 = (struct lcp_opt16_t*)ptr;
-	struct auth_data_t *d;
-	int r;
-
-	if (auth_opt->started) {
-		if (!auth_opt->auth)
-			return LCP_OPT_REJ;
-
-		if (!ptr || ntohs(opt16->val) != auth_opt->auth->proto)
-			return LCP_OPT_NAK;
-
-		return LCP_OPT_ACK;
-	}
-
-	if (list_empty(&auth_opt->auth_list))
-		return LCP_OPT_REJ;
-
-	if (!ptr)
-		return LCP_OPT_ACK;
-
-
-	list_for_each_entry(d, &auth_opt->auth_list, entry) {
-		if (d->proto == ntohs(opt16->val)) {
-			r = d->h->recv_conf_req(lcp->ppp, d, (uint8_t*)(opt16 + 1));
-			if (r == LCP_OPT_FAIL)
-				return LCP_OPT_FAIL;
-			if (r == LCP_OPT_REJ)
-				break;
-			auth_opt->peer_auth = d;
-			return r;
-		}
-	}
-
-	list_for_each_entry(d, &auth_opt->auth_list, entry) {
-		if (d->state != LCP_OPT_NAK) {
-			auth_opt->peer_auth = d;
-			return LCP_OPT_NAK;
-		}
-	}
-
-	log_ppp_error("cann't negotiate authentication type\n");
-	return LCP_OPT_FAIL;
+	return LCP_OPT_REJ;
 }
 
 static int auth_recv_conf_ack(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, uint8_t *ptr)
 {
-	struct auth_option_t *auth_opt = container_of(opt, typeof(*auth_opt), opt);
-
-	auth_opt->peer_auth = NULL;
-
 	return 0;
 }
 
@@ -206,8 +159,6 @@ static int auth_recv_conf_nak(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 		return -1;
 	}
 	auth_opt->auth->state = LCP_OPT_NAK;
-	if (auth_opt->peer_auth)
-		auth_opt->auth = auth_opt->peer_auth;
 
 	list_for_each_entry(d, &auth_opt->auth_list, entry) {
 		if (d->state != LCP_OPT_NAK)
@@ -229,8 +180,6 @@ static int auth_recv_conf_rej(struct ppp_lcp_t *lcp, struct lcp_option_t *opt, u
 	}
 
 	auth_opt->auth->state = LCP_OPT_NAK;
-	if (auth_opt->peer_auth)
-		auth_opt->auth = auth_opt->peer_auth;
 
 	list_for_each_entry(d, &auth_opt->auth_list, entry) {
 		if (d->state != LCP_OPT_NAK)
-- 
2.19.1

[Accel-ppp-users] [PATCH 2/2] auth: remove .recv_conf_req from struct ppp_auth_handler_t

[Guillaume N. <g....@al...>]

This callback isn't used anymore. Let's remove it from all
authentication backends.

Signed-off-by: Guillaume Nault <g....@al...>
---
 accel-pppd/auth/auth_chap_md5.c  | 8 --------
 accel-pppd/auth/auth_mschap_v1.c | 8 --------
 accel-pppd/auth/auth_mschap_v2.c | 8 --------
 accel-pppd/auth/auth_pap.c       | 7 -------
 accel-pppd/ppp/ppp_auth.h        | 1 -
 5 files changed, 32 deletions(-)

diff --git a/accel-pppd/auth/auth_chap_md5.c b/accel-pppd/auth/auth_chap_md5.c
index ae062f53..de7f7418 100644
--- a/accel-pppd/auth/auth_chap_md5.c
+++ b/accel-pppd/auth/auth_chap_md5.c
@@ -185,13 +185,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == CHAP_MD5)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad)
 {
 	struct chap_failure msg = {
@@ -441,7 +434,6 @@ static struct ppp_auth_handler_t chap=
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_mschap_v1.c b/accel-pppd/auth/auth_mschap_v1.c
index 67f941c6..23b0f0e9 100644
--- a/accel-pppd/auth/auth_mschap_v1.c
+++ b/accel-pppd/auth/auth_mschap_v1.c
@@ -186,13 +186,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == MSCHAP_V1)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad, char *mschap_error)
 {
 	struct chap_hdr *hdr = _malloc(sizeof(*hdr) + strlen(mschap_error) + 1);
@@ -514,7 +507,6 @@ static struct ppp_auth_handler_t chap = {
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_mschap_v2.c b/accel-pppd/auth/auth_mschap_v2.c
index 5c82413c..66e17f24 100644
--- a/accel-pppd/auth/auth_mschap_v2.c
+++ b/accel-pppd/auth/auth_mschap_v2.c
@@ -187,13 +187,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 1;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	if (*ptr == MSCHAP_V2)
-		return LCP_OPT_ACK;
-	return LCP_OPT_NAK;
-}
-
 static void chap_send_failure(struct chap_auth_data *ad, char *mschap_error, char *reply_msg)
 {
 	struct chap_hdr *hdr = _malloc(sizeof(*hdr) + strlen(mschap_error) + strlen(reply_msg) + 4);
@@ -662,7 +655,6 @@ static struct ppp_auth_handler_t chap=
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = chap_start,
 	.finish        = chap_finish,
 	.check         = chap_check,
diff --git a/accel-pppd/auth/auth_pap.c b/accel-pppd/auth/auth_pap.c
index a290123e..a2303d12 100644
--- a/accel-pppd/auth/auth_pap.c
+++ b/accel-pppd/auth/auth_pap.c
@@ -29,7 +29,6 @@ static int conf_any_login = 0;
 static struct auth_data_t* auth_data_init(struct ppp_t *ppp);
 static void auth_data_free(struct ppp_t*, struct auth_data_t*);
 static int lcp_send_conf_req(struct ppp_t*, struct auth_data_t*, uint8_t*);
-static int lcp_recv_conf_req(struct ppp_t*, struct auth_data_t*, uint8_t*);
 static int pap_start(struct ppp_t*, struct auth_data_t*);
 static int pap_finish(struct ppp_t*, struct auth_data_t*);
 static void pap_recv(struct ppp_handler_t*h);
@@ -64,7 +63,6 @@ static struct ppp_auth_handler_t pap= {
 	.init          = auth_data_init,
 	.free          = auth_data_free,
 	.send_conf_req = lcp_send_conf_req,
-	.recv_conf_req = lcp_recv_conf_req,
 	.start         = pap_start,
 	.finish        = pap_finish,
 };
@@ -132,11 +130,6 @@ static int lcp_send_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *
 	return 0;
 }
 
-static int lcp_recv_conf_req(struct ppp_t *ppp, struct auth_data_t *d, uint8_t *ptr)
-{
-	return LCP_OPT_ACK;
-}
-
 static void pap_send_ack(struct pap_auth_data *p, int id)
 {
 	uint8_t buf[128];
diff --git a/accel-pppd/ppp/ppp_auth.h b/accel-pppd/ppp/ppp_auth.h
index c8df9fc6..e9398c29 100644
--- a/accel-pppd/ppp/ppp_auth.h
+++ b/accel-pppd/ppp/ppp_auth.h
@@ -22,7 +22,6 @@ struct ppp_auth_handler_t
 	const char *name;
 	struct auth_data_t* (*init)(struct ppp_t*);
 	int (*send_conf_req)(struct ppp_t*, struct auth_data_t*, uint8_t*);
-	int (*recv_conf_req)(struct ppp_t*, struct auth_data_t*, uint8_t*);
 	int (*start)(struct ppp_t*, struct auth_data_t*);
 	int (*finish)(struct ppp_t*, struct auth_data_t*);
 	void (*free)(struct ppp_t*,struct auth_data_t*);
-- 
2.19.1