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:  muks
  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 vorner):

 * owner:  vorner => muks


Comment:

 Hello

 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?

 > > In this code, it seems to me if the RRset has no RRSIG, it is still
 „stripped“. That is probably a needless work (query unittests, somewhere
 below line 711):
 >
 > This was existing code from before #2165. This code has been replaced
 now with stripRRsigs(). :) I avoided making changes unless they affected
 something (caused tests to fail or resulted in incorrect behavior) so that
 side effects were not introduced by mistake.

 The code was changed slightly in the condition, there was
 `||!found_rrset->second->getRRsig()` before. Anyway, the new version does
 not have the problem, so it doesn't matter.

 > 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.

 > > 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);
 }}}

 > Replying to [comment:11 jinmei]:
 > > I happen to notice one other thing.
 > >
 > > In `DatabaseClient::Finder::getRRsets` I believe you can simply
 > > ignore any RRSIGs if sigs is false.  Also, if we have a simple test
 > > method to tell if `RRsigStore` is empty, this part:
 > >
 > > {{{#!cpp
 > >     if (sigs) {
 > >         // Add signatures to all found RRsets
 > >         for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
 > >              i != result.end(); ++ i) {
 > >             sig_store.appendSignatures(i->second);
 > >         }
 > >     }
 > > }}}
 > >
 > > can be like this:
 > >
 > > {{{#!cpp
 > >     if (!sig_store.empty()) {
 > >         // Add signatures to all found RRsets
 > >         for (std::map<RRType, RRsetPtr>::iterator i(result.begin());
 > >              i != result.end(); ++ i) {
 > >             sig_store.appendSignatures(i->second);
 > >         }
 > >     }
 > > }}}
 > >
 > > This works whether or not `sigs` is true, and in case the zone isn't
 > > signed we can avoid iterating over the other types of RRsets for
 > > nothing.
 >
 > This has been fixed too. :)

 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.

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