BIND 10 #2165: update Message::addRRset() to be unaware of signedness

[BIND 10 Development]

#2165: update Message::addRRset() to be unaware of signedness
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:         |  Sprint-20120904
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  libdns++                           |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  5
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
  scalable inmemory                  |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Hi vorner

 Replying to [comment:13 vorner]:
 > Replying to [comment:12 muks]:
 > > I hope I follow what you are asking for correctly. I see two cases of
 it:
 > >
 > > * In this one, I think `setTTL()` is called eventually on the `RRset`
 passed to `Message::addRRset()`.
 > > {{{
 > > bin/auth/query.cc-        message.addRRset(section,
 > > bin/auth/query.cc:
 boost::const_pointer_cast<AbstractRRset>(rrset));
 > > bin/auth/query.cc-        added_.push_back(rrset.get());
 > > }}}
 >
 > I think the first one. The message needs it only to render it, why would
 it need a non-const pointer. The second one is obvious why it is needed,
 but probably wrong. Anyway, the second one will be deprecated soon, won't
 it?

 `setTTL()` is eventually called by the `Message` class on the passed
 `RRset`.

 > > This is so that the files are present in the tarball for any
 downloader to view/use. In projects I have worked in, such extra stuff in
 the tree became a part of the tarball. As you see, it was added to
 `EXTRA_DIST` on purpose. The real question here is if the test files
 should be in the tree anymore. When it goes from the tree, it can go from
 the tarball too.
 >
 > Well, I don't know about the policy (let's talk about it somewhere), I
 think it should not be in git either. I don't like to see unused or
 commented-out code through the current version. If someone wants to dig
 for it, it can always be extracted from history again.

 We can bring it up during the weekly meeting if you like. I'd like these
 to be removed too if they are not going to be used again.

 > > > Looking at the code of `toWire(AbstractMessageRenderer, …)`, can it
 happen that the real RR is not rendered, because it does not fit (64k long
 TXT record, for example), but then the signatures fit, so they are
 included? If so, is it OK?
 > >
 > > Nice one. :) I added a test for it first, and then updated the code to
 not render the signatures if truncation would happen with the records
 firstly.
 >
 > The test seems to have a copy-pasted commented-out code in it. I think
 it should be removed:
 > {{{#!c++
 > TEST_F(MessageTest, toWireSignedAndTruncated) {
 >     // EDNSPtr edns(new EDNS());
 >     // edns->setUDPSize(256);
 >     // message_render.setEDNS(edns);
 > }}}

 It was left there on purpose along with the corresponding additional EDNS
 record in the wiredata for use with an EDNS test. But I've removed it now.

 Replying to [comment:14 jinmei]:
 > Replying to [comment:13 vorner]:
 >
 > Just happened to notice this:
 >
 > > Commit `3c20e037be36b32640289bb61f45d555649bad12`, right? Well, I'm
 little bit worried about one thing there. What happens if someone sends a
 query asking directly for RRSIG type without dnssec enabled? And,
 actually, that query makes sense ‒ I think unbound uses that trick to
 tunnel DNSSEC data if there's something not liking EDNS or a non-DNSSEC-
 aware cache in between.
 >
 > I didn't follow the context of this point, but regarding direct RRSIG
 > queries: my understanding is that it's a kind of spec hole what an
 > authoritative server should do for such queries.  I vaguely recall a
 > discussion at the IETF on this point - (IIRC) some said RRSIGs are
 > only meaningful with records they are signed for (and it's legitimate
 > for auth servers to not return the RRSIGs (and RRSIGs only) for such
 > queries); some said RRSIGs are just one type of RRs in one sense and
 > shouldn't be treated differently when explicitly asked.  I don't know
 > if there was a consensus about this.
 >
 > In any case, our current implementation doesn't return RRSIGs to
 > direct RRSIGs queries anyway (try dig @ns.jinmei.org jinmei.org
 > rrsig).  We can discuss if we want to change that (maybe a tomorrow's
 > topic?), but I think it's outside the scope of this ticket.

 We can discuss this during the weekly meeting. However, I want to add that
 the datasrc behaviour is that RRSIGs are returned when explicitly queried
 for as `RRType::RRSIG()` or any, even if `FIND_DNSSEC` is not specified.
 This behavior has not changed. What changed was that no signatures are
 _attached_ to the `RRset`s if `FIND_DNSSEC` is not specified. Another
 unittest `DatabaseClientTest.findRRSIGsWithoutDNSSEC` has been added for
 this so that the behaviour is clear and tested.

-- 
Ticket URL: <http://bind10.isc.org/ticket/2165#comment:15>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development