Silence resolver logging for DNAME records when DNSSEC is enabled
Commit Message
On Fri, Feb 20, 2015 at 09:10:41AM +0100, Florian Weimer wrote:
> Can we remove the logging altogether? Or at least for the
> RES_USE_DNSSEC case?
>
> The DO bit essentially means, “I'm fine with receiving unknown RR
> types”, it's not really related to DNSSEC. The reason for that is the
> fact that the DNSSEC protocol was changed twice (once for DNSSECbis,
> which is completely unrecognizable to the previous implementation, and
> once for NSEC3), and the flag was reused.
>
> So unless there is a compelling reason for logging this information,
> I'd say just remove it.
Thanks for the context. I wasn't sure about removing the logging
altogether, but if it is going to be such a pain for DNSSEC, we might
as well silence it. How is this then?
Siddhesh
[BZ #14841]
* resolv/gethnamaddr.c (getanswer): Skip logging if
RES_USE_DNSSEC is set.
* resolv/nss_dns/dns-host.c (getanswer_r): Likewise.
Comments
On 02/20/2015 11:13 AM, Siddhesh Poyarekar wrote:
> On Fri, Feb 20, 2015 at 09:10:41AM +0100, Florian Weimer wrote:
>> Can we remove the logging altogether? Or at least for the
>> RES_USE_DNSSEC case?
>>
>> The DO bit essentially means, “I'm fine with receiving unknown
>> RR types”, it's not really related to DNSSEC. The reason for
>> that is the fact that the DNSSEC protocol was changed twice (once
>> for DNSSECbis, which is completely unrecognizable to the previous
>> implementation, and once for NSEC3), and the flag was reused.
>>
>> So unless there is a compelling reason for logging this
>> information, I'd say just remove it.
>
> Thanks for the context. I wasn't sure about removing the logging
> altogether, but if it is going to be such a pain for DNSSEC, we
> might as well silence it. How is this then?
You are inconsistent about the space after p_class and p_type.
I noticed there is an existing bug in p_type which affects the code:
<https://sourceware.org/bugzilla/show_bug.cgi?id=18004>
Your patch looks fine, but I suggest to be more explicit the comment,
saying that DNSSEC uses many different types in responses which do not
match the QTYPE.
On Fri, Feb 20, 2015 at 02:54:18PM +0100, Florian Weimer wrote:
> You are inconsistent about the space after p_class and p_type.
You mean between gethnamaddr.c and dns-host.c? I left that untouched
because the gethnamaddr uses a completely different code formatting,
likely from the bind source.
> I noticed there is an existing bug in p_type which affects the code:
>
> <https://sourceware.org/bugzilla/show_bug.cgi?id=18004>
>
> Your patch looks fine, but I suggest to be more explicit the comment,
> saying that DNSSEC uses many different types in responses which do not
> match the QTYPE.
Ack, I'll expand the comment. I'll wait for more comments and commit
it on Tuesday since my Monday comes earlier than most people here.
Siddhesh
On 02/20/2015 04:52 PM, Siddhesh Poyarekar wrote:
> On Fri, Feb 20, 2015 at 02:54:18PM +0100, Florian Weimer wrote:
>> You are inconsistent about the space after p_class and p_type.
>
> You mean between gethnamaddr.c and dns-host.c? I left that
> untouched because the gethnamaddr uses a completely different code
> formatting, likely from the bind source.
Oh, I suspected as much. Fine then.
On 02/20/2015 05:13 AM, Siddhesh Poyarekar wrote:
> Thanks for the context. I wasn't sure about removing the logging
> altogether, but if it is going to be such a pain for DNSSEC, we might
> as well silence it. How is this then?
>
> Siddhesh
>
> [BZ #14841]
> * resolv/gethnamaddr.c (getanswer): Skip logging if
> RES_USE_DNSSEC is set.
> * resolv/nss_dns/dns-host.c (getanswer_r): Likewise.
Looks good to me, and simplifies the code.
I guess given the churn we're going to have to wait a while before
this standardizes to be able to provide useful logging.
Cheers,
Carlos.
@@ -331,23 +331,16 @@ getanswer (const querybuf *answer, int anslen, const char *qname, int qtype)
buflen -= n;
continue;
}
- if ((type == T_SIG) || (type == T_KEY) || (type == T_NXT)) {
- /* We don't support DNSSEC yet. For now, ignore
- * the record and send a low priority message
- * to syslog.
- */
- syslog(LOG_DEBUG|LOG_AUTH,
- "gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
- qname, p_class(C_IN), p_type(qtype),
- p_type(type));
- cp += n;
- continue;
- }
if (type != qtype) {
- syslog(LOG_NOTICE|LOG_AUTH,
+ /* Log a low priority message if we get an unexpected
+ * record, but skip it if we are using DNSSEC.
+ */
+ if ((_res.options & RES_USE_DNSSEC) == 0) {
+ syslog(LOG_NOTICE|LOG_AUTH,
"gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
- qname, p_class(C_IN), p_type(qtype),
- p_type(type));
+ qname, p_class(C_IN), p_type(qtype),
+ p_type(type));
+ }
cp += n;
continue; /* XXX - had_error++ ? */
}
@@ -820,26 +820,18 @@ getanswer_r (const querybuf *answer, int anslen, const char *qname, int qtype,
linebuflen -= n;
continue;
}
- if (__builtin_expect (type == T_SIG, 0)
- || __builtin_expect (type == T_KEY, 0)
- || __builtin_expect (type == T_NXT, 0))
- {
- /* We don't support DNSSEC yet. For now, ignore the record
- and send a low priority message to syslog. */
- syslog (LOG_DEBUG | LOG_AUTH,
- "gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
- qname, p_class (C_IN), p_type(qtype), p_type (type));
- cp += n;
- continue;
- }
if (type == T_A && qtype == T_AAAA && map)
have_to_map = 1;
else if (__glibc_unlikely (type != qtype))
{
- syslog (LOG_NOTICE | LOG_AUTH,
- "gethostby*.getanswer: asked for \"%s %s %s\", got type \"%s\"",
- qname, p_class (C_IN), p_type (qtype), p_type (type));
+ /* Log a low priority message if we get an unexpected record, but
+ skip it if we are using DNSSEC. */
+ if ((_res.options & RES_USE_DNSSEC) == 0)
+ syslog (LOG_NOTICE | LOG_AUTH,
+ "gethostby*.getanswer: asked for \"%s %s %s\", "
+ "got type \"%s\"",
+ qname, p_class (C_IN), p_type (qtype), p_type (type));
cp += n;
continue; /* XXX - had_error++ ? */
}