On 2021-11-21 21:34 +01, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:
> On Sat, Nov 20 2021, Florian Obser <flor...@openbsd.org> wrote:
>> On 2021-11-20 17:05 +01, Jeremie Courreges-Anglas <j...@wxcvbn.org> wrote:

>> do you remember why you check for !RES_USE_DNSSEC?
>> I'd like to leave it out.
>
> First, here's my understanding of RES_USE_DNSSEC: it's supposed to
> activate EDNS and set the DO bit.  The server is then supposed to reply
> with AD set only if the data has been validated (with or without
> DNSSEC), and possibly append add DNSSEC data if available.

I didn't go throught the 3000+ pages of DNS RFC but I don't see a way
an RFC says AD MUST be set withouth performing DNSSEC validation. That
just doesn't make sense.

So DO requests that the resolver performes DNSSEC validation **AND**
informs us about the outcome by setting the AD flag and giving us DNSSEC
data (i.e. RRSIGs most of the time). Now, that data is not actually
helping the stub (asr) because it can't build a chain of trust without
querying for a bunch of more data, and well, having a trust anchor configured.

So this is why setting AD in queries was introduced in 6840, I want to
know that DNSSEC validation was performed, but I don't want all the
(for me) useless junk.

> Since I didn't know the semantics of both setting AD and DO in a query
> (I would expect such semantics to be sane) I wrote those more
> conservative checks instead.  Does this make sense?  If so, maybe
> a comment would help?

Everything half way decent handles AD + DO set at the same time
correctly. All the quad-x public resolvers do, as well as bind, knot,
powerdns-recursor and unbound.

The result is the same as if only DO was set, you get AD set in the
answer and the junk DNSSEC data. Btw. dig(1) sets AD per default on
queries and does so when running with +dnssec, too.

RFC 8906 says that there are servers out there that choke on AD=1 in
queries. We shouldn't care:
1) we enable trust-ad per default for localhost. If someone runs
something this broken on localhost they need to fix their system
2) If the operator of the system sets options trust-ad in resolv.conf
they hopefully know what they are doing. If their nameserver(s) are this
broken they shouldn't (and wouldn't) set trust-ad in the first place.
So this is a non-issue and just complicates the code.

>
>       /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
>       if ((ac->ac_options & RES_TRUSTAD) &&
>           !(ac->ac_options & RES_USE_DNSSEC))
>

With that said, I'd really prefer to leave !RES_USE_DNSSEC out, it is
only a complication. But if you insist, I'll accept it.

[...]

>> +Authentic Data (AD) flag in the answer can only be trusted if the
>> +nameserver itself is trusted and the network path is trusted.
>> +Generally this is not the case and the AD flag is cleared in the answer.
>> +The
>> +.Cm trust-ad
>> +flag lets the system administrator indicate that the nameserver and the
>> +network path are trusted.
>> +Nameservers on localhost can be trusted and
>> +.Cm trust-ad is enabled per default.
>
> Already mentioned above, this last sentence doesn't really describe the
> precise behavior you proposa.  We *could* have a mix of localhost and
> non-localhost servers in resolv.conf, and use/trust AD=1 only for
> localhost servers. But the automatic detection code you wrote enables

[For the archives, in case someone wants to implement this:

yeah, you kinda can't. On the one hand we don't have per-nameserver
options, they are global. So if you do options trust-ad this applies to
all nameservers listed. No way around that.

Now, about localhost. If we have a mix of nameserver lines in
/etc/resolv.conf, some localhost, some not we run into the problem that
query packets are not rebuild per-server. So if the first one -
localhost - doesn't respond, we are sending the same query to the 2nd
one. Again with AD=1. So instead of clearing AD depending on
RES_TRUSTAD being set we could clear AD if the answer is not comming
from localhost. But, we are sending an AD=1 query to a server that might
choke on it (see RFC 89060) and nobody said we can do this by setting
options trust-ad. I wouldn't want that behaviour on may laptop that
roams to crappy networks all the time.]


> trust-ad if resolv.conf(5) *only lists localhost entries*.  Which is
> perfectly fine as far as I'm concerned, and lead to my first proposal:
>

Indeed.

>   +It is enabled by default if
>   +.Nm resolv.conf
>   +only lists nameservers on localhost.
>
> Here's an updated diff with the proposals mentioned.

OK florian@, I also got a testing & OK phessler off-list.

>
> Index: include/resolv.h
> ===================================================================
> RCS file: /home/cvs/src/include/resolv.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 resolv.h
> --- include/resolv.h  14 Jan 2019 06:23:06 -0000      1.22
> +++ include/resolv.h  21 Nov 2021 19:15:30 -0000
> @@ -191,6 +191,7 @@ struct __res_state_ext {
>  /* DNSSEC extensions: use higher bit to avoid conflict with ISC use */
>  #define      RES_USE_DNSSEC  0x20000000      /* use DNSSEC using OK bit in 
> OPT */
>  #define      RES_USE_CD      0x10000000      /* set Checking Disabled flag */
> +#define      RES_TRUSTAD     0x80000000      /* Request AD, keep it in 
> responses. */
>  
>  #define RES_DEFAULT  (RES_RECURSE | RES_DEFNAMES | RES_DNSRCH)
>  
> Index: share/man/man5/resolv.conf.5
> ===================================================================
> RCS file: /home/cvs/src/share/man/man5/resolv.conf.5,v
> retrieving revision 1.62
> diff -u -p -r1.62 resolv.conf.5
> --- share/man/man5/resolv.conf.5      24 Aug 2021 07:30:32 -0000      1.62
> +++ share/man/man5/resolv.conf.5      21 Nov 2021 20:22:49 -0000
> @@ -259,6 +259,18 @@ first as an absolute name before any sea
>  .It Cm tcp
>  Forces the use of TCP for queries.
>  Normal behaviour is to query via UDP but fall back to TCP on failure.
> +.It Cm trust-ad
> +A name server indicating that it performed DNSSEC validation by setting the
> +Authentic Data (AD) flag in the answer can only be trusted if the
> +name server itself is trusted and the network path is trusted.
> +Generally this is not the case and the AD flag is cleared in the answer.
> +The
> +.Cm trust-ad
> +option lets the system administrator indicate that the name server and the
> +network path are trusted.
> +This option is automatically enabled if
> +.Nm resolv.conf
> +only lists name servers on localhost.
>  .El
>  .El
>  .Pp
> Index: lib/libc/asr/asr.c
> ===================================================================
> RCS file: /home/cvs/src/lib/libc/asr/asr.c,v
> retrieving revision 1.66
> diff -u -p -r1.66 asr.c
> --- lib/libc/asr/asr.c        5 Nov 2021 13:08:58 -0000       1.66
> +++ lib/libc/asr/asr.c        21 Nov 2021 19:15:30 -0000
> @@ -661,7 +661,8 @@ pass0(char **tok, int n, struct asr_ctx 
>                               d = strtonum(tok[i] + 6, 1, 16, &e);
>                               if (e == NULL)
>                                       ac->ac_ndots = d;
> -                     }
> +                     } else if (!strcmp(tok[i], "trust-ad"))
> +                             ac->ac_options |= RES_TRUSTAD;
>               }
>       }
>  }
> @@ -672,7 +673,10 @@ pass0(char **tok, int n, struct asr_ctx 
>  static int
>  asr_ctx_from_string(struct asr_ctx *ac, const char *str)
>  {
> -     char             buf[512], *ch;
> +     struct sockaddr_in6     *sin6;
> +     struct sockaddr_in      *sin;
> +     int                      i, trustad;
> +     char                     buf[512], *ch;
>  
>       asr_ctx_parse(ac, str);
>  
> @@ -701,6 +705,27 @@ asr_ctx_from_string(struct asr_ctx *ac, 
>                       if (ch && asr_ndots(++ch) == 0)
>                               break;
>               }
> +
> +     trustad = 1;
> +     for (i = 0; i < ac->ac_nscount && trustad; i++) {
> +             switch (ac->ac_ns[i]->sa_family) {
> +             case AF_INET:
> +                     sin = (struct sockaddr_in *)ac->ac_ns[i];
> +                     if (sin->sin_addr.s_addr != htonl(INADDR_LOOPBACK))
> +                             trustad = 0;
> +                     break;
> +             case AF_INET6:
> +                     sin6 = (struct sockaddr_in6 *)ac->ac_ns[i];
> +                     if (!IN6_IS_ADDR_LOOPBACK(&sin6->sin6_addr))
> +                             trustad = 0;
> +                     break;
> +             default:
> +                     trustad = 0;
> +                     break;
> +             }
> +     }
> +     if (trustad)
> +             ac->ac_options |= RES_TRUSTAD;
>  
>       return (0);
>  }
> Index: lib/libc/asr/asr_debug.c
> ===================================================================
> RCS file: /home/cvs/src/lib/libc/asr/asr_debug.c,v
> retrieving revision 1.27
> diff -u -p -r1.27 asr_debug.c
> --- lib/libc/asr/asr_debug.c  2 Apr 2021 07:00:30 -0000       1.27
> +++ lib/libc/asr/asr_debug.c  21 Nov 2021 19:15:30 -0000
> @@ -286,6 +286,7 @@ _asr_dump_config(FILE *f, struct asr *a)
>       PRINTOPT(RES_USE_EDNS0, "USE_EDNS0");
>       PRINTOPT(RES_USE_DNSSEC, "USE_DNSSEC");
>       PRINTOPT(RES_USE_CD, "USE_CD");
> +     PRINTOPT(RES_TRUSTAD, "TRUSTAD");
>       if (o)
>               fprintf(f, " 0x%08x", o);
>       fprintf(f, "\n");
> Index: lib/libc/asr/res_mkquery.c
> ===================================================================
> RCS file: /home/cvs/src/lib/libc/asr/res_mkquery.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 res_mkquery.c
> --- lib/libc/asr/res_mkquery.c        14 Jan 2019 06:49:42 -0000      1.13
> +++ lib/libc/asr/res_mkquery.c        21 Nov 2021 20:09:08 -0000
> @@ -62,6 +62,10 @@ res_mkquery(int op, const char *dname, i
>               h.flags |= RD_MASK;
>       if (ac->ac_options & RES_USE_CD)
>               h.flags |= CD_MASK;
> +     /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
> +     if ((ac->ac_options & RES_TRUSTAD) &&
> +         !(ac->ac_options & RES_USE_DNSSEC))
> +             h.flags |= AD_MASK;
>       h.qdcount = 1;
>       if (ac->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>               h.arcount = 1;
> Index: lib/libc/asr/res_send_async.c
> ===================================================================
> RCS file: /home/cvs/src/lib/libc/asr/res_send_async.c,v
> retrieving revision 1.39
> diff -u -p -r1.39 res_send_async.c
> --- lib/libc/asr/res_send_async.c     28 Sep 2019 11:21:07 -0000      1.39
> +++ lib/libc/asr/res_send_async.c     21 Nov 2021 20:09:41 -0000
> @@ -42,6 +42,7 @@ static int udp_recv(struct asr_query *);
>  static int tcp_write(struct asr_query *);
>  static int tcp_read(struct asr_query *);
>  static int validate_packet(struct asr_query *);
> +static void clear_ad(struct asr_result *);
>  static int setup_query(struct asr_query *, const char *, const char *, int, 
> int);
>  static int ensure_ibuf(struct asr_query *, size_t);
>  static int iter_ns(struct asr_query *);
> @@ -258,6 +259,8 @@ res_send_async_run(struct asr_query *as,
>               as->as.dns.ibuf = NULL;
>               ar->ar_errno = 0;
>               ar->ar_rcode = as->as.dns.rcode;
> +             if (!(as->as_ctx->ac_options & RES_TRUSTAD))
> +                     clear_ad(ar);
>               async_set_state(as, ASR_STATE_HALT);
>               break;
>  
> @@ -378,6 +381,11 @@ setup_query(struct asr_query *as, const 
>               h.flags |= RD_MASK;
>       if (as->as_ctx->ac_options & RES_USE_CD)
>               h.flags |= CD_MASK;
> +     /* Set the AD flag unless we already plan to set the DNSSEC DO bit. */
> +     if ((as->as_ctx->ac_options & RES_TRUSTAD) &&
> +         !(as->as_ctx->ac_options & RES_USE_DNSSEC))
> +             h.flags |= AD_MASK;
> +
>       h.qdcount = 1;
>       if (as->as_ctx->ac_options & (RES_USE_EDNS0 | RES_USE_DNSSEC))
>               h.arcount = 1;
> @@ -745,6 +753,21 @@ validate_packet(struct asr_query *as)
>      inval:
>       errno = EINVAL;
>       return (-1);
> +}
> +
> +/*
> + * Clear AD flag in the answer.
> + */
> +static void
> +clear_ad(struct asr_result *ar)
> +{
> +     struct asr_dns_header   *h;
> +     uint16_t                 flags;
> +
> +     h = (struct asr_dns_header *)ar->ar_data;
> +     flags = ntohs(h->flags);
> +     flags &= ~(AD_MASK);
> +     h->flags = htons(flags);
>  }
>  
>  /*
> Index: lib/libc/net/res_init.3
> ===================================================================
> RCS file: /home/cvs/src/lib/libc/net/res_init.3,v
> retrieving revision 1.4
> diff -u -p -r1.4 res_init.3
> --- lib/libc/net/res_init.3   25 Apr 2020 21:06:17 -0000      1.4
> +++ lib/libc/net/res_init.3   21 Nov 2021 20:20:22 -0000
> @@ -179,6 +179,18 @@ This option has no effect.
>  In the past, it turned off the legacy
>  .Ev HOSTALIASES
>  feature.
> +.It Dv RES_TRUSTAD
> +If set, the resolver routines will set the AD flag in DNS queries and
> +preserve the value of the AD flag in DNS replies.
> +If not set, the resolver routines will clear the AD flag in responses.
> +Direct use of this option to enable AD bit processing is discouraged.
> +Instead the use of trusted name servers should be annotated with
> +.Dq options trust-ad
> +in
> +.Xr resolv.conf 5 .
> +This option is automatically enabled if
> +.Xr resolv.conf 5
> +only lists name servers on localhost.
>  .It Dv RES_USE_INET6
>  With this option
>  .Xr gethostbyname 3
>
>
>
> -- 
> jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF  DDCC 0DFA 74AE 1524 E7EE
>

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

Reply via email to