Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-10 Thread Klemens Nanni
On Wed, Nov 10, 2021 at 07:35:26AM +0100, Bjorn Ketelaars wrote:
> On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> > Diff below does two things:
> > 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> >sppp(4)
> > 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> >RTM_PROPOSAL_STATIC route messages.
> 
> 
> Updated diff below, based on feedback from kn@ and claudio@:
> 
> - fix forgotten parentheses with `sizeof`
> - instead of using `u_int32_t` use `struct in_addr` for holding dns
>   addresses. Makes it more clear what the data is
> - decouple `IPCP_OPT` definitions from the bitmask values to
>   enable/disable an option. Makes the code look a bit better
> - use `memcpy`
> - fit code within 80 columns
> 
> While here add RFC to sppp(4)'s STANDARDS section.
> 
> @kn, is this still OK for you?

I agree with claudio's feedback on sizeof and semarie's point of using
a dedicated message, but these can be done as their own commits.

OK kn

> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..5850a6da963 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + struct in_addr dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */

Double space in comment.

>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-10 Thread Claudio Jeker
On Wed, Nov 10, 2021 at 07:35:26AM +0100, Bjorn Ketelaars wrote:
> On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> > Diff below does two things:
> > 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> >sppp(4)
> > 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> >RTM_PROPOSAL_STATIC route messages.
> 
> 
> Updated diff below, based on feedback from kn@ and claudio@:
> 
> - fix forgotten parentheses with `sizeof`
> - instead of using `u_int32_t` use `struct in_addr` for holding dns
>   addresses. Makes it more clear what the data is
> - decouple `IPCP_OPT` definitions from the bitmask values to
>   enable/disable an option. Makes the code look a bit better
> - use `memcpy`
> - fit code within 80 columns
> 
> While here add RFC to sppp(4)'s STANDARDS section.
> 
> @kn, is this still OK for you?
> 
> Other OK's?
> 
> 
> diff --git share/man/man4/sppp.4 share/man/man4/sppp.4
> index 5ca10285953..bccb41eec15 100644
> --- share/man/man4/sppp.4
> +++ share/man/man4/sppp.4
> @@ -230,6 +230,13 @@ take place.
>  .Re
>  .Pp
>  .Rs
> +.%A S. Cobb
> +.%D December 1995
> +.%R RFC 1877
> +.%T PPP Internet Protocol Control Protocol Extensions for Name Server 
> Addresses
> +.Re
> +.Pp
> +.Rs
>  .%A W. Simpson
>  .%D August 1996
>  .%R RFC 1994
> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..5850a6da963 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + struct in_addr dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */
>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ac1dc9a709d..e460703c089 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -132,6 +132,14 @@
>  #define IPCP_OPT_ADDRESSES   1   /* both IP addresses; deprecated */
>  #define IPCP_OPT_COMPRESSION 2   /* IP compression protocol (VJ) */
>  #define IPCP_OPT_ADDRESS 3   /* local IP address */
> +#define IPCP_OPT_PRIMDNS 129 /* primary remote dns address */
> +#define IPCP_OPT_SECDNS  131 /* secondary remote dns address 
> */
> +
> +#define SPPP_IPCP_OPT_ADDRESSES  1   /* bitmask value */
> +#define SPPP_IPCP_OPT_COMPRESSION2   /* bitmask value */
> +#define SPPP_IPCP_OPT_ADDRESS3   /* bitmask value */
> +#define SPPP_IPCP_OPT_PRIMDNS4   /* bitmask value */
> +#define SPPP_IPCP_OPT_SECDNS 5   /* bitmask value */

Instead of repeating the /* bitmask value */ just add a comment at the
top. Something like "bitmask value to enable or disable individual IPCP
options"

>  
>  #define IPV6CP_OPT_IFID  1   /* interface identifier */
>  #define IPV6CP_OPT_COMPRESSION   2   /* IPv6 compression protocol */
> @@ -338,6 +346,8 @@ void sppp_update_gw(struct ifnet *ifp);
>  void sppp_set_ip_addrs(void *);
>  void sppp_clear_ip_addrs(void *);
>  void sppp_set_phase(struct sppp *sp);
> +void sppp_update_dns(struct ifnet *ifp);
> +void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
>  
>  /* our control protocol descriptors */
>  static const struct cp lcp = {
> @@ -701,6 +711,7 @@ sppp_attach(struct ifnet *ifp)
>  
>   sp->pp_if.if_type = IFT_PPP;
>   sp->pp_if.if_output = sppp_output;
> + sp->pp_if.if_rtrequest = sppp_rtrequest;
>   ifq_set_maxlen(>pp_if.if_snd, 50);
>   mq_init(>pp_cpq, 50, IPL_NET);
>   sp->pp_loopcnt = 0;
> @@ -2512,13 +2523,19 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header 
> *h, int len)
>* Peer doesn't grok address option.  This is
>* bad.  XXX  Should we better give up here?
>*/
> - sp->ipcp.opts &= ~(1 << IPCP_OPT_ADDRESS);
> + sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_ADDRESS);
>   break;
>  #ifdef notyet
>   case IPCP_OPT_COMPRESS:
> - sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
> + sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_COMPRESS);
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_PRIMDNS);
> + break;
> + case IPCP_OPT_SECDNS:
> + sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_SECDNS);
> + break;
>   }
>   }
>   if (debug)
> @@ -2559,7 +2576,7 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header 
> *h, int len)
>   if (len >= 6 && p[1] == 6) {
>   wantaddr = p[2] << 24 

Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-10 Thread Claudio Jeker
On Wed, Nov 10, 2021 at 08:22:52AM +0100, Sebastien Marie wrote:
> On Wed, Nov 10, 2021 at 07:35:26AM +0100, Bjorn Ketelaars wrote:
> > On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> > > Diff below does two things:
> > > 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> > >sppp(4)
> > > 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> > >RTM_PROPOSAL_STATIC route messages.
> > 
> > 
> > Updated diff below, based on feedback from kn@ and claudio@:
> > 
> > - fix forgotten parentheses with `sizeof`
> > - instead of using `u_int32_t` use `struct in_addr` for holding dns
> >   addresses. Makes it more clear what the data is
> > - decouple `IPCP_OPT` definitions from the bitmask values to
> >   enable/disable an option. Makes the code look a bit better
> > - use `memcpy`
> > - fit code within 80 columns
> > 
> > While here add RFC to sppp(4)'s STANDARDS section.
> > 
> > @kn, is this still OK for you?
> > 
> > Other OK's?
> 
> There is one point which bother me a bit: you are using
> RTP_PROPOSAL_STATIC for sending the proposal, whereas all others
> sources (dhcpleased/dhclient, slaacd, umb) are using a specific value.
> 
> By using RTP_PROPOSAL_STATIC, it means also that route(8) nameserver
> subcommand might interfere with it.
> 
> Using a new specific value (like RTP_PROPOSAL_SPPP) would make sense
> to me. But no objection if RTM_PROPOSAL_STATIC is preferred.

Agreed, This should use its own prio. I would suggest to call it
RTP_PROPOSAL_PPP. This is something that can be done independently.

-- 
:wq Claudio



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-09 Thread Sebastien Marie
On Wed, Nov 10, 2021 at 07:35:26AM +0100, Bjorn Ketelaars wrote:
> On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> > Diff below does two things:
> > 1. add PPP IPCP extensions for name server addresses (rfc1877) to
> >sppp(4)
> > 2. propose negotiated name servers from sppp(4) to resolvd(8) using
> >RTM_PROPOSAL_STATIC route messages.
> 
> 
> Updated diff below, based on feedback from kn@ and claudio@:
> 
> - fix forgotten parentheses with `sizeof`
> - instead of using `u_int32_t` use `struct in_addr` for holding dns
>   addresses. Makes it more clear what the data is
> - decouple `IPCP_OPT` definitions from the bitmask values to
>   enable/disable an option. Makes the code look a bit better
> - use `memcpy`
> - fit code within 80 columns
> 
> While here add RFC to sppp(4)'s STANDARDS section.
> 
> @kn, is this still OK for you?
> 
> Other OK's?

There is one point which bother me a bit: you are using
RTP_PROPOSAL_STATIC for sending the proposal, whereas all others
sources (dhcpleased/dhclient, slaacd, umb) are using a specific value.

By using RTP_PROPOSAL_STATIC, it means also that route(8) nameserver
subcommand might interfere with it.

Using a new specific value (like RTP_PROPOSAL_SPPP) would make sense
to me. But no objection if RTM_PROPOSAL_STATIC is preferred.

> +void
> +sppp_update_dns(struct ifnet *ifp)
> +{
> + struct rt_addrinfo info;
> + struct sockaddr_rtdns rtdns;
> + struct sppp *sp = ifp->if_softc;
> + size_t sz = 0;
> + int i, flag = 0;
> +
> + memset(, 0, sizeof(rtdns));
> + memset(, 0, sizeof(info));
> +
> + for (i = 0; i < IPCP_MAX_DNSSRV; i++) {
> + if (sp->ipcp.dns[i].s_addr == INADDR_ANY)
> + break;
> + sz = sizeof(sp->ipcp.dns[i].s_addr);
> + memcpy(rtdns.sr_dns + i * sz, >ipcp.dns[i].s_addr, sz);
> + flag = RTF_UP;
> + }
> +
> + rtdns.sr_family = AF_INET;
> + rtdns.sr_len = 2 + i * sz;
> + info.rti_info[RTAX_DNS] = srtdnstosa();
> +
> + rtm_proposal(ifp, , flag, RTP_PROPOSAL_STATIC);
> +}

Thanks.
-- 
Sebastien Marie



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-09 Thread Bjorn Ketelaars
On Mon 08/11/2021 11:52, Bjorn Ketelaars wrote:
> Diff below does two things:
> 1. add PPP IPCP extensions for name server addresses (rfc1877) to
>sppp(4)
> 2. propose negotiated name servers from sppp(4) to resolvd(8) using
>RTM_PROPOSAL_STATIC route messages.


Updated diff below, based on feedback from kn@ and claudio@:

- fix forgotten parentheses with `sizeof`
- instead of using `u_int32_t` use `struct in_addr` for holding dns
  addresses. Makes it more clear what the data is
- decouple `IPCP_OPT` definitions from the bitmask values to
  enable/disable an option. Makes the code look a bit better
- use `memcpy`
- fit code within 80 columns

While here add RFC to sppp(4)'s STANDARDS section.

@kn, is this still OK for you?

Other OK's?


diff --git share/man/man4/sppp.4 share/man/man4/sppp.4
index 5ca10285953..bccb41eec15 100644
--- share/man/man4/sppp.4
+++ share/man/man4/sppp.4
@@ -230,6 +230,13 @@ take place.
 .Re
 .Pp
 .Rs
+.%A S. Cobb
+.%D December 1995
+.%R RFC 1877
+.%T PPP Internet Protocol Control Protocol Extensions for Name Server Addresses
+.Re
+.Pp
+.Rs
 .%A W. Simpson
 .%D August 1996
 .%R RFC 1994
diff --git sys/net/if_sppp.h sys/net/if_sppp.h
index ff559fcc369..5850a6da963 100644
--- sys/net/if_sppp.h
+++ sys/net/if_sppp.h
@@ -132,6 +132,8 @@ struct sipcp {
  * original one here, in network byte order */
u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
u_int32_t req_myaddr;   /* local address requested (IPv4) */
+#define IPCP_MAX_DNSSRV2
+   struct in_addr dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */
 #ifdef INET6
struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
 #endif
diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
index ac1dc9a709d..e460703c089 100644
--- sys/net/if_spppsubr.c
+++ sys/net/if_spppsubr.c
@@ -132,6 +132,14 @@
 #define IPCP_OPT_ADDRESSES 1   /* both IP addresses; deprecated */
 #define IPCP_OPT_COMPRESSION   2   /* IP compression protocol (VJ) */
 #define IPCP_OPT_ADDRESS   3   /* local IP address */
+#define IPCP_OPT_PRIMDNS   129 /* primary remote dns address */
+#define IPCP_OPT_SECDNS131 /* secondary remote dns address 
*/
+
+#define SPPP_IPCP_OPT_ADDRESSES1   /* bitmask value */
+#define SPPP_IPCP_OPT_COMPRESSION  2   /* bitmask value */
+#define SPPP_IPCP_OPT_ADDRESS  3   /* bitmask value */
+#define SPPP_IPCP_OPT_PRIMDNS  4   /* bitmask value */
+#define SPPP_IPCP_OPT_SECDNS   5   /* bitmask value */
 
 #define IPV6CP_OPT_IFID1   /* interface identifier */
 #define IPV6CP_OPT_COMPRESSION 2   /* IPv6 compression protocol */
@@ -338,6 +346,8 @@ void sppp_update_gw(struct ifnet *ifp);
 void sppp_set_ip_addrs(void *);
 void sppp_clear_ip_addrs(void *);
 void sppp_set_phase(struct sppp *sp);
+void sppp_update_dns(struct ifnet *ifp);
+void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
 
 /* our control protocol descriptors */
 static const struct cp lcp = {
@@ -701,6 +711,7 @@ sppp_attach(struct ifnet *ifp)
 
sp->pp_if.if_type = IFT_PPP;
sp->pp_if.if_output = sppp_output;
+   sp->pp_if.if_rtrequest = sppp_rtrequest;
ifq_set_maxlen(>pp_if.if_snd, 50);
mq_init(>pp_cpq, 50, IPL_NET);
sp->pp_loopcnt = 0;
@@ -2512,13 +2523,19 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header 
*h, int len)
 * Peer doesn't grok address option.  This is
 * bad.  XXX  Should we better give up here?
 */
-   sp->ipcp.opts &= ~(1 << IPCP_OPT_ADDRESS);
+   sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_ADDRESS);
break;
 #ifdef notyet
case IPCP_OPT_COMPRESS:
-   sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
+   sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_COMPRESS);
break;
 #endif
+   case IPCP_OPT_PRIMDNS:
+   sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_PRIMDNS);
+   break;
+   case IPCP_OPT_SECDNS:
+   sp->ipcp.opts &= ~(1 << SPPP_IPCP_OPT_SECDNS);
+   break;
}
}
if (debug)
@@ -2559,7 +2576,7 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header *h, 
int len)
if (len >= 6 && p[1] == 6) {
wantaddr = p[2] << 24 | p[3] << 16 |
p[4] << 8 | p[5];
-   sp->ipcp.opts |= (1 << IPCP_OPT_ADDRESS);
+   sp->ipcp.opts |= (1 << SPPP_IPCP_OPT_ADDRESS);
if (debug)
addlog("[wantaddr %s] ",
   

Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-08 Thread Claudio Jeker
On Mon, Nov 08, 2021 at 11:52:52AM +0100, Bjorn Ketelaars wrote:
> Diff below does two things:
> 1. add PPP IPCP extensions for name server addresses (rfc1877) to
>sppp(4)
> 2. propose negotiated name servers from sppp(4) to resolvd(8) using
>RTM_PROPOSAL_STATIC route messages.
> 
> With this I'm able to use DNS servers as provided by my ISP who uses
> PPPoE. resolv.conf is updated by resolvd(8) as function of status
> changes of pppoe(4).
> 
> rfc1877 implementation is derived from code from NetBSD, and inspired by
> [0] and [1]. Borrowed code from umb(4) for the route messages.
> 
> Opinions/comments/OK?
> 
> 
> [0] https://marc.info/?l=openbsd-tech=134943767022961=2
> [1] https://marc.info/?l=openbsd-tech=159405677416423=2
> 
> 
> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..f2dab61e46b 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + u_int32_t dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */

I would very much prefer to use struct in_addr here (or maybe in_addr_t).
It makes it more clear what the data is.

>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ac1dc9a709d..225ad8f5a3e 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -132,6 +132,10 @@
>  #define IPCP_OPT_ADDRESSES   1   /* both IP addresses; deprecated */
>  #define IPCP_OPT_COMPRESSION 2   /* IP compression protocol (VJ) */
>  #define IPCP_OPT_ADDRESS 3   /* local IP address */
> +#define IPCP_OPT_PRIMDNS 129 /* primary remote dns address */
> +#define _IPCP_OPT_PRIMDNS_   4   /* set/check option */
> +#define IPCP_OPT_SECDNS  131 /* secondary remote dns address 
> */
> +#define _IPCP_OPT_SECDNS_5   /* set/check option */

As kn@ noted this underscore version is a bit rough.
I wonder if IPCP_OPT definitions (from the standard) should be decoupled
from the bitmask values to enable/disable a feature.

  
>  #define IPV6CP_OPT_IFID  1   /* interface identifier */
>  #define IPV6CP_OPT_COMPRESSION   2   /* IPv6 compression protocol */
> @@ -338,6 +342,8 @@ void sppp_update_gw(struct ifnet *ifp);
>  void sppp_set_ip_addrs(void *);
>  void sppp_clear_ip_addrs(void *);
>  void sppp_set_phase(struct sppp *sp);
> +void sppp_update_dns(struct ifnet *ifp);
> +void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
>  
>  /* our control protocol descriptors */
>  static const struct cp lcp = {
> @@ -701,6 +707,7 @@ sppp_attach(struct ifnet *ifp)
>  
>   sp->pp_if.if_type = IFT_PPP;
>   sp->pp_if.if_output = sppp_output;
> + sp->pp_if.if_rtrequest = sppp_rtrequest;
>   ifq_set_maxlen(>pp_if.if_snd, 50);
>   mq_init(>pp_cpq, 50, IPL_NET);
>   sp->pp_loopcnt = 0;
> @@ -2519,6 +2526,12 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header 
> *h, int len)
>   sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_PRIMDNS_);
> + break;
> + case IPCP_OPT_SECDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_SECDNS_);
> + break;
>   }
>   }
>   if (debug)
> @@ -2584,6 +2597,16 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header 
> *h, int len)
>*/
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[0] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8 | p[5];

Please use memcpy and here. Since this remains in network byte order it
should just work.

> + break;
> + case IPCP_OPT_SECDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[1] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8 | p[5];

See above.

> + break;
>   }
>   }
>   if (debug)
> @@ -2612,6 +2635,7 @@ sppp_ipcp_tls(struct sppp *sp)
>   IPCP_MYADDR_DYN|IPCP_HISADDR_DYN);
>   sp->ipcp.req_myaddr = 0;
>   sp->ipcp.req_hisaddr = 0;
> + memset(>ipcp.dns, 0, sizeof sp->ipcp.dns);
>  
>   sppp_get_ip_addrs(sp, , , 0);
>   /*
> @@ -2644,6 +2668,10 @@ sppp_ipcp_tls(struct sppp *sp)
>   sp->ipcp.flags |= IPCP_HISADDR_DYN;
>   }
>  
> + /* negotiate name server addresses */
> + sp->ipcp.opts |= (1 << _IPCP_OPT_PRIMDNS_);
> + 

Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-08 Thread Stuart Henderson
On 2021/11/08 15:50, Klemens Nanni wrote:
> With this diff pppoe(4) sends two nameservers and thus wins, but that is
> expected and setups such as mine must either
> - disable resolvd
> - enable resolvd but also enable unwind
>   (unwind also learns DNS proposals but always wins in resolv.conf)
> - enable resolvd and clear pppoe(4) proposals,
>   e.g. `route nameserver pppoe0'
> - do whatever else fits their setup

This is already a problem though having additional sources of nameserver
addresses does mean that more people will run into it.

Most resolvers handed out by ISPs or on local networks are only
reachable from (or at least only give valid responses to) IPs on the
expected network; that's OK when the nameserver is in the same subnet as
the client (typical home ISP router config) but otherwise it's not (you
do get to learn how very quick resolvd is at changing the file back to
what it wants though!).

Feels like maybe it should only add a resolver if it's either on a
directly-connected interface or there's a route for the ns address
pointing at the interface that the ns was learned from. Though there's
an extra complication if you have multiple route tables..



Re: sppp(4)/pppoe(4) - DNS configuration via resolvd(8)

2021-11-08 Thread Klemens Nanni
On Mon, Nov 08, 2021 at 11:52:52AM +0100, Bjorn Ketelaars wrote:
> Diff below does two things:
> 1. add PPP IPCP extensions for name server addresses (rfc1877) to
>sppp(4)

You could add that RFC to sppp(4)'s STANDARDS section.

> 2. propose negotiated name servers from sppp(4) to resolvd(8) using
>RTM_PROPOSAL_STATIC route messages.

Needs mentioning in resolvd(8)'s sentence about umb(4).
unwind(8) should mention it as well (it does not even mention umb at
this point).

> With this I'm able to use DNS servers as provided by my ISP who uses
> PPPoE. resolv.conf is updated by resolvd(8) as function of status
> changes of pppoe(4).

Works as expected on octeon behind german VDSL2.

I use unbound(8) with a static 'nameserver ::1' entry in resolv.conf(5)
and resolvd(8) is running but does not get any DNS proposals, hence the
unbound entry wins.

With this diff pppoe(4) sends two nameservers and thus wins, but that is
expected and setups such as mine must either
- disable resolvd
- enable resolvd but also enable unwind
  (unwind also learns DNS proposals but always wins in resolv.conf)
- enable resolvd and clear pppoe(4) proposals,
  e.g. `route nameserver pppoe0'
- do whatever else fits their setup

> rfc1877 implementation is derived from code from NetBSD, and inspired by
> [0] and [1]. Borrowed code from umb(4) for the route messages.
> 
> Opinions/comments/OK?

> [0] https://marc.info/?l=openbsd-tech=134943767022961=2
> [1] https://marc.info/?l=openbsd-tech=159405677416423=2

I guess the ifconfig(8) bits to print proposed nameservers are still
useful, even nowadays where resolvd/unwind pick them up and/or print
them (`unwindctl status autoconf').


As per above, I'd like to improve the bigger picture, but this diff is
OK kn as-is;  see inline comments.

> diff --git sys/net/if_sppp.h sys/net/if_sppp.h
> index ff559fcc369..f2dab61e46b 100644
> --- sys/net/if_sppp.h
> +++ sys/net/if_sppp.h
> @@ -132,6 +132,8 @@ struct sipcp {
> * original one here, in network byte order */
>   u_int32_t req_hisaddr;  /* remote address requested (IPv4) */
>   u_int32_t req_myaddr;   /* local address requested (IPv4) */
> +#define IPCP_MAX_DNSSRV  2
> + u_int32_t dns[IPCP_MAX_DNSSRV]; /* IPv4  DNS servers (RFC 1877) */
>  #ifdef INET6
>   struct in6_aliasreq req_ifid;   /* local ifid requested (IPv6) */
>  #endif
> diff --git sys/net/if_spppsubr.c sys/net/if_spppsubr.c
> index ac1dc9a709d..225ad8f5a3e 100644
> --- sys/net/if_spppsubr.c
> +++ sys/net/if_spppsubr.c
> @@ -132,6 +132,10 @@
>  #define IPCP_OPT_ADDRESSES   1   /* both IP addresses; deprecated */
>  #define IPCP_OPT_COMPRESSION 2   /* IP compression protocol (VJ) */
>  #define IPCP_OPT_ADDRESS 3   /* local IP address */
> +#define IPCP_OPT_PRIMDNS 129 /* primary remote dns address */
> +#define _IPCP_OPT_PRIMDNS_   4   /* set/check option */
> +#define IPCP_OPT_SECDNS  131 /* secondary remote dns address 
> */
> +#define _IPCP_OPT_SECDNS_5   /* set/check option */

This _*_ looks ugly.  NetBSD has `IPCP_OPT_PRIMDNS 129' and
`SPPP_IPCP_OPT_PRIMDNS __BIT(3)' which we don't have.

Optimally, we could even out this difference, but I have not yet looked
closer into this.

>  #define IPV6CP_OPT_IFID  1   /* interface identifier */
>  #define IPV6CP_OPT_COMPRESSION   2   /* IPv6 compression protocol */
> @@ -338,6 +342,8 @@ void sppp_update_gw(struct ifnet *ifp);
>  void sppp_set_ip_addrs(void *);
>  void sppp_clear_ip_addrs(void *);
>  void sppp_set_phase(struct sppp *sp);
> +void sppp_update_dns(struct ifnet *ifp);
> +void sppp_rtrequest(struct ifnet *ifp, int req, struct rtentry *rt);
>  
>  /* our control protocol descriptors */
>  static const struct cp lcp = {
> @@ -701,6 +707,7 @@ sppp_attach(struct ifnet *ifp)
>  
>   sp->pp_if.if_type = IFT_PPP;
>   sp->pp_if.if_output = sppp_output;
> + sp->pp_if.if_rtrequest = sppp_rtrequest;
>   ifq_set_maxlen(>pp_if.if_snd, 50);
>   mq_init(>pp_cpq, 50, IPL_NET);
>   sp->pp_loopcnt = 0;
> @@ -2519,6 +2526,12 @@ sppp_ipcp_RCN_rej(struct sppp *sp, struct lcp_header 
> *h, int len)
>   sp->ipcp.opts &= ~(1 << IPCP_OPT_COMPRESS);
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_PRIMDNS_);
> + break;
> + case IPCP_OPT_SECDNS:
> + sp->ipcp.opts &= ~(1 << _IPCP_OPT_SECDNS_);
> + break;
>   }
>   }
>   if (debug)
> @@ -2584,6 +2597,16 @@ sppp_ipcp_RCN_nak(struct sppp *sp, struct lcp_header 
> *h, int len)
>*/
>   break;
>  #endif
> + case IPCP_OPT_PRIMDNS:
> + if (len >= 6 && p[1] == 6)
> + sp->ipcp.dns[0] = p[2] << 24 | p[3] << 16 |
> + p[4] << 8