Jeremie Courreges-Anglas([email protected]) on 2020.07.25 14:01:06 +0200: > On Fri, Jul 17 2020, Jesper Wallin <[email protected]> wrote: > > Hi all, > > > > I recently decided to add SSHFP records for my servers, since I never > > memorize or write down my key fingerprints. I learned that if I > > want ssh(1) to trust these records, DNSSEC needs to be enabled for my > > zone. To validate these records, ssh(1) is using getrrsetbyname(3), > > which checks if the AD (Authentic Data) flag is set in the response. > > > > To get a response with the AD flag set, the request itself also needs > > to have the AD flag set. It turns out that getrrsetbyname(3) doesn't > > set this and will therefore never get a validated response, unless the > > resolver is configured to always send it, no matter what the client > > requested. > > > > It seems like unwind(8) behaves this way but it also responds with the > > RRSIG records, which is extra overhead and ignored by getrrsetbyname(3). > > > > This was mentioned a few years ago [0] and the solution suggested was > > to add the RES_USE_DNSSEC to _res.options, which also makes the resolver > > respond with the extra RRSIG records. > > > > Instead, by only setting the AD flag, both the request and the response > > has the same size as without the flag set. The patch below will add > > RES_USE_AD as an option to _res.options and set it by default. > > This is also the default behaviour in dig(1), which I understand is a > > bit different, but that sure added some confusion while debugging this. > > > > This let you run unbound(8) or any other validating resolver on your > > local network and getrrsetbyname(3) will trust it. Do read the CAVEATS > > in the manual of getrrsetbyname(3) though. > > > > As a side note, I noticed that the default value of _res.options was the > > same value as RES_DEFAULT, so I changed it to RES_DEFAULT instead, for > > the sake of consistency. > > > > Thoughts? > > Thanks for addressing this longstanding issue. > > I think using the AD bit in queries is a good idea. IIRC Peter J. > Philipp (cc'd) suggested using it but I was not thrilled because: > > 1. the meaning of the AD bit in queries is relatively recent (2013 > I think[0]) > 2. getrrsetbyname also collects signature records, and for this you need > EDNS + the DO bit set. Implementing this in was not 100% trivial, > I think we had something working but Eric or I were not 100% happy > with it. > > 1. is probably not a concern, after all you're supposed to use > a trustworthy resolver, which should be modern and understand the > purpose of the AD bit in queries. > > 2. is probably not a concern either. I guess that all getrrsetbyname(3) > callers only care about the target records and the AD bit, not about the > signature records. (Why would they use it for anyway?). In the base > system, only ssh(1) and traceroute(8) use getrrsetbyname(3). > AFAIK no other system provides getrrsetbyname(3), and ISC has removed > getrrsetbyname and the whole lwres API in 2017[1]. So I'd say we're > free to improve our version of getrrsetbyname(3) as we see fit.
This is a concern for the stub resolver, because edns and AD does not work everywhere. Indeed when we switched unbound to validate by default we learned that this is not a good idea for everyone - which lead to the development of unwind(8). > Now let's discuss the implementation. There are two main things > I dislike with your diff: > - it affects all DNS queries done through libc, even if the user doesn't > care at all about DNSSEC. I suspect there's potential for fallout > here, but I have no data to back this. > - trusting the AD bit by default looks a bit too easy. The > documentation says that people should only trust the AD bit / > RRSET_VALIDATED if the communication between libc and the resolver is > secure, but who actually reads the documentation? > > For those two reasons I think the feature should be opt-in. That's what > Florian Weimer did in glibc, with the implementation of RES_TRUSTAD[2][3]. > The diff below implements the same semantics: > - by default, don't send queries the AD flag, and don't trust its value > in replies > - if "options trust-ad" is set in resolv.conf(5), set the AD flag in > queries and allow applications to look at the AD flag in replies. > > There's one more thing I'd like to check in the res_* API, but I'm > sitting on this idea since too long already, and I'm happy with my test > results so far. > > Thoughts? Comments? Your observation that this should not be on by default is probably correct. If you enable trust-ad on a system that moves around, e.g. your laptop, you will experience failures, which is why unwind tests for this and falls back to non-trusting dhcp learned resolvers in such cases. In fact it also falls back to the stub resolver, i.e. our libc resolver. Which makes me think that trust-ad should not be used when you run unwind (because the whole point of the fallback is that dnssec does not work). But thats a documentation issue. The patch reads ok, fwiw. i'm not familiar with the code. > [0] https://tools.ietf.org/html/rfc6840#section-5.7 > [1] > https://gitlab.isc.org/isc-projects/bind9/-/commit/8eb88aafee951859264e36c315b1289cd8c2088b > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1164339 > [3] > https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=446997ff1433d33452b81dfa9e626b8dccf101a4 > > > > Index: include/resolv.h > =================================================================== > RCS file: /d/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 23 Jul 2020 13:28:15 -0000 > @@ -186,6 +186,8 @@ struct __res_state_ext { > #define RES_INSECURE2 0x00000800 /* type 2 security disabled */ > #define RES_NOALIASES 0x00001000 /* shuts off HOSTALIASES > feature */ > #define RES_USE_INET6 0x00002000 /* use/map IPv6 in > gethostbyname() */ > +/* GNU extension: use higher bit to avoid conflict with ISC use */ > +#define RES_TRUSTAD 0x80000000 /* query with AD, trust AD in > reply */ > /* KAME extensions: use higher bit to avoid conflict with ISC use */ > #define RES_USE_EDNS0 0x40000000 /* use EDNS0 */ > /* DNSSEC extensions: use higher bit to avoid conflict with ISC use */ > Index: lib/libc/net/res_init.3 > =================================================================== > RCS file: /d/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 25 Jul 2020 02:09:58 -0000 > @@ -179,6 +179,17 @@ 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 (this flag is stripped > +by default). > +Useful when applications want to ensure that the DNS resources they > +look up have been signed with DNSSEC and validated by the name server. > +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 . > .It Dv RES_USE_INET6 > With this option > .Xr gethostbyname 3 > Index: share/man/man5/resolv.conf.5 > =================================================================== > RCS file: /d/cvs/src/share/man/man5/resolv.conf.5,v > retrieving revision 1.60 > diff -u -p -r1.60 resolv.conf.5 > --- share/man/man5/resolv.conf.5 25 Apr 2020 14:22:04 -0000 1.60 > +++ share/man/man5/resolv.conf.5 25 Jul 2020 02:08:17 -0000 > @@ -285,6 +285,15 @@ 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 > +Sets the AD flag in DNS queries, and preserve the value of the AD flag > +in DNS replies (this flag is stripped by default). > +Useful when applications want to ensure that the DNS resources they > +look up have been signed with DNSSEC and validated by the > +.Ic nameserver . > +This option should be used only if the transport between the > +.Ic nameserver > +and the resolver is secure. > .El > .El > .Pp > Index: lib/libc/asr/asr.c > =================================================================== > RCS file: /d/cvs/src/lib/libc/asr/asr.c,v > retrieving revision 1.64 > diff -u -p -r1.64 asr.c > --- lib/libc/asr/asr.c 6 Jul 2020 13:33:05 -0000 1.64 > +++ lib/libc/asr/asr.c 23 Jul 2020 23:36:06 -0000 > @@ -640,6 +640,8 @@ pass0(char **tok, int n, struct asr_ctx > ac->ac_options |= RES_USEVC; > else if (!strcmp(tok[i], "edns0")) > ac->ac_options |= RES_USE_EDNS0; > + else if (!strcmp(tok[i], "trust-ad")) > + ac->ac_options |= RES_TRUSTAD; > else if ((!strncmp(tok[i], "ndots:", 6))) { > e = NULL; > d = strtonum(tok[i] + 6, 1, 16, &e); > Index: lib/libc/asr/asr_debug.c > =================================================================== > RCS file: /d/cvs/src/lib/libc/asr/asr_debug.c,v > retrieving revision 1.26 > diff -u -p -r1.26 asr_debug.c > --- lib/libc/asr/asr_debug.c 3 Jul 2019 03:24:03 -0000 1.26 > +++ lib/libc/asr/asr_debug.c 23 Jul 2020 23:36:06 -0000 > @@ -285,6 +285,7 @@ _asr_dump_config(FILE *f, struct asr *a) > PRINTOPT(RES_NOALIASES, "NOALIASES"); > PRINTOPT(RES_USE_EDNS0, "USE_EDNS0"); > PRINTOPT(RES_USE_DNSSEC, "USE_DNSSEC"); > + PRINTOPT(RES_TRUSTAD, "TRUSTAD"); > if (o) > fprintf(f, " 0x%08x", o); > fprintf(f, "\n"); > Index: lib/libc/asr/getrrsetbyname_async.c > =================================================================== > RCS file: /d/cvs/src/lib/libc/asr/getrrsetbyname_async.c,v > retrieving revision 1.11 > diff -u -p -r1.11 getrrsetbyname_async.c > --- lib/libc/asr/getrrsetbyname_async.c 23 Feb 2017 17:04:02 -0000 > 1.11 > +++ lib/libc/asr/getrrsetbyname_async.c 23 Jul 2020 23:36:06 -0000 > @@ -32,7 +32,8 @@ > #include "asr_private.h" > > static int getrrsetbyname_async_run(struct asr_query *, struct asr_result *); > -static void get_response(struct asr_result *, const char *, int); > +static void get_response(struct asr_query *, struct asr_result *, const char > *, > + int); > > struct asr_query * > getrrsetbyname_async(const char *hostname, unsigned int rdclass, > @@ -150,7 +151,7 @@ getrrsetbyname_async_run(struct asr_quer > break; > } > > - get_response(ar, ar->ar_data, ar->ar_datalen); > + get_response(as, ar, ar->ar_data, ar->ar_datalen); > free(ar->ar_data); > async_set_state(as, ASR_STATE_HALT); > break; > @@ -255,7 +256,8 @@ static void free_dns_response(struct dns > static int count_dns_rr(struct dns_rr *, u_int16_t, u_int16_t); > > static void > -get_response(struct asr_result *ar, const char *pkt, int pktlen) > +get_response(struct asr_query *as, struct asr_result *ar, const char *pkt, > + int pktlen) > { > struct rrsetinfo *rrset = NULL; > struct dns_response *response = NULL; > @@ -287,7 +289,7 @@ get_response(struct asr_result *ar, cons > rrset->rri_nrdatas = response->header.ancount; > > /* check for authenticated data */ > - if (response->header.ad == 1) > + if (as->as_ctx->ac_options & RES_TRUSTAD && response->header.ad == 1) > rrset->rri_flags |= RRSET_VALIDATED; > > /* copy name from answer section */ > Index: lib/libc/asr/res_mkquery.c > =================================================================== > RCS file: /d/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 24 Jul 2020 12:34:27 -0000 > @@ -62,6 +62,9 @@ res_mkquery(int op, const char *dname, i > h.flags |= RD_MASK; > if (ac->ac_options & RES_USE_CD) > h.flags |= CD_MASK; > + 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: /d/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 24 Jul 2020 13:02:38 -0000 > @@ -378,6 +378,9 @@ setup_query(struct asr_query *as, const > h.flags |= RD_MASK; > if (as->as_ctx->ac_options & RES_USE_CD) > h.flags |= CD_MASK; > + 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; > @@ -700,6 +703,9 @@ validate_packet(struct asr_query *as) > /* Must be a response */ > if ((h.flags & QR_MASK) == 0) > goto inval; > + /* Zap the AD flag unless the server is considered trusted. */ > + if (!(as->as_ctx->ac_options & RES_TRUSTAD)) > + h.flags &= ~(AD_MASK); > > as->as.dns.rcode = RCODE(h.flags); > as->as.dns.ancount = h.ancount; > > > -- > jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE >
