Opened 7 years ago

Closed 7 years ago

#2165 closed task (fixed)

update Message::addRRset() to be unaware of signedness

Reported by: jinmei Owned by: muks
Priority: medium Milestone: Sprint-20120904
Component: libdns++ Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: scalable inmemory
Estimated Difficulty: 5 Add Hours to Ticket: 0
Total Hours: 2.91 Internal?: no

Description (last modified by jinmei)

This is one building block of #2163.

What we do is:

  • adjust counts_[section] for any RRSIGs using RRset::getSIGRdataCount() (introduced in #2164)
  • remove the 'sign' parameter of Message::addRRset()
  • stop extracting the RRSIG

Also:

  • update the following part of Zonefinder::find() doc:
        /// - \c FIND_DNSSEC Request that DNSSEC data (like NSEC, RRSIGs) are
        ///   returned with the answer. It is allowed for the data source to
        ///   include them even when not requested.
    
    saying DNSSEC data are only returned when FIND_DNSSEC is specified (the current implementation should already meet this requirement). Note: without this condition we cannot remove the 'sign' parameter of Message::addRRset()

Finally: update affected tests.

This will be an incomplete update: we also need to update the
rendering logic so the RRSIGs will be correctly rendered. But
combining these will be too big a task, so I suggest we stop here, not
merging (temporarily disabling any failed tests due to this particular
point). The rest of the work will go to a separate ticket (#2166).

Depends on #2164.

Subtickets

Change History (19)

comment:1 Changed 7 years ago by jinmei

  • Description modified (diff)

comment:2 Changed 7 years ago by shane

  • Milestone changed from New Tasks to Next-Sprint-Proposed

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120821

comment:4 Changed 7 years ago by muks

  • Owner changed from UnAssigned to muks
  • Status changed from new to assigned

Picking

comment:5 follow-up: Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from assigned to reviewing

The bug is up for review.

Please note the following points:

  • The work is in the trac2165_2 branch.
  • Whoever picks up this bug for review will also have to assign #2166 to themselves (for review) as both are in the same branch. Please first assign both bugs to yourself before beginning review.
  • Some precursor tests were added in #2164 so that #2165 and #2166 could be tested.
  • Jinmei has pointed out an issue with commit ef85853043e2d5f1fe3c2d494cd4af3b553d88fa which remains to be addressed, but you can review the rest of the work in parallel as it's another review comment: https://lists.isc.org/pipermail/bind10-dev/2012-August/003731.html
  • The commits are cleanly separated. Only ace794f6a528370d1533790512836870a89b4344 is large as it involved removing the sign argument from the call tree.
  • I have verified that lettuce passes here.
  • One important note: for every change that involved fixing a testcase or testdata, please check carefully based on the testcase whether that was the correct thing to do. I think some of the tests expected incorrect results before.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 7 years ago by muks

Replying to muks:

This is now fixed too. More tests needed adjusting. But now tests in the Python bindings fail due to this change:

@@ -866,7 +868,8 @@ DatabaseClient::Finder::findOnNameResult(const Name& name,
         // includes the case where we were explicitly querying for a CNAME and
         // found it.  It also includes the case where we were querying for an
         // NS RRset and found it at the apex of the zone.)
-        return (ResultContext(SUCCESS, wti->second, flags));
+        return (ResultContext(SUCCESS, stripRRsigs(wti->second, options),
+                              flags));
     }

It throws an exception that "database is locked" in SQLite3Accessor::startUpdateZone(). I am trying to figure out why that happens.

comment:7 in reply to: ↑ 6 ; follow-up: Changed 7 years ago by jinmei

Replying to muks:

It throws an exception that "database is locked" in SQLite3Accessor::startUpdateZone(). I am trying to figure out why that happens.

It can be reproduced with the following simplified tests:

class DataSrcUpdater(unittest.TestCase):

    def setUp(self):
        # Make a fresh copy of the writable database with all original content
        shutil.copyfile(READ_ZONE_DB_FILE, WRITE_ZONE_DB_FILE)

    def test_update_delete_commit(self):

        dsc = isc.datasrc.DataSourceClient("sqlite3", WRITE_ZONE_DB_CONFIG)
        updater = dsc.get_updater(isc.dns.Name("example.com"), True)

    def test_update_delete_abort(self):

        # we don't do enything with this one, just making sure loading two
        # datasources
        dsc = isc.datasrc.DataSourceClient("sqlite3", WRITE_ZONE_DB_CONFIG)
        updater = dsc.get_updater(isc.dns.Name("example.com"), True)
        raise Exception('foo')

(note that deleter_abort will be tested first)

I suspect the test framework keeps the local objects (in particular
updater) alive on raise (maybe to print debug info?), and the
zombie 'updater' object from test_update_delete_abort prevents from
the new one in test_update_delete_commit from working.

Running it with B10_LOGGER_DESTINATION=stdout seems to support this
theory. You'll see this after test reports:

2012-08-22 09:41:42.959 INFO  [bind10.datasrc] DATASRC_DATABASE_UPDATER_ROLLBACK zone updates roll-backed for 'example.com./IN' on sqlite3_rwtest.sqlite3.copied

It indicates the updater from delete_abort rolls back its transaction
only after the tests are completed. If you remove the raise, you'll
see the ROLLBACK logs (there are two of them) before the output from
the test framework.

One other thing: this won't work because rrset_to_delete now wouldn't
have RRSIGs.

        self.assertRaises(isc.datasrc.Error, updater.delete_rrset,
                          rrset_to_delete)

And one final thing at this moment: I think in database.cc we should
avoid including RRSIGs from the beginning if they are not required.
I believe we can do the trick in getRRsets(). Then we won't have to
do expensive stripRRsigs() (in general, if we need explicitly do
things like in any other place than in the current in-memory
implementation I think it's not the correct way).

comment:8 in reply to: ↑ 7 Changed 7 years ago by muks

Replying to jinmei:

And one final thing at this moment: I think in database.cc we should
avoid including RRSIGs from the beginning if they are not required.
I believe we can do the trick in getRRsets(). Then we won't have to
do expensive stripRRsigs() (in general, if we need explicitly do
things like in any other place than in the current in-memory
implementation I think it's not the correct way).

This has been fixed now. :)

One other thing: this won't work because rrset_to_delete now wouldn't
have RRSIGs.

        self.assertRaises(isc.datasrc.Error, updater.delete_rrset,
                          rrset_to_delete)

Thanks for noticing this, esp. the tip that tests are run out of order. The results are printed out of order too and I was looking at a later failure assuming it was the cause. Running the unittests with verbosity=2 made it more obvious what was going on. We should bump minimum required Python to 3.2 and add verbosity=2 to all unittests so it prints the test names as it's executing them.

The bug has been fixed now. :)

comment:9 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:10 Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

I'll answer both tickets here, I reviewed it as one diff.

Regarding style, you use rr as the name for an RRset in many places. Isn't that misleading little bit? RR usually means single Resource Record. Or is it really so the RRset contains only one?

It is not related to this ticket much, but it reminded me of the problem. There are many calls like this:

boost::const_pointer_cast<AbstractRRset>(rrset));

This is bad, but there was some problem with some method of rrset not being const even if it was a getter method and there was some technical reason for it inside the implementation. I think it was related to the signatures. Would you know by any chance if it is still a problem?

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

if (found_rrset != found_domain->second.end()) {
    ConstRRsetPtr rrset;
    // Strip whatever signature there is in case DNSSEC is not required
    // Just to make sure the Query asks for it when it is needed
    if ((options & ZoneFinder::FIND_DNSSEC) != 0 &&
        found_rrset->second->getRRsig()) {
        rrset = found_rrset->second;
    } else {
        RRsetPtr noconst(new RRset(found_rrset->second->getName(),
                                   found_rrset->second->getClass(),
                                   found_rrset->second->getType(),
                                   found_rrset->second->getTTL()));
        for (RdataIteratorPtr
             i(found_rrset->second->getRdataIterator());
             !i->isLast(); i->next()) {
            noconst->addRdata(i->getCurrent());
        }
        rrset = noconst;
    }
    return (createContext(options, SUCCESS, rrset));

You disabled the tests for the old API. If I leave aside the fact we still have the API in our tree, even if it is not used (I think it is not used), why are the files listed in EXTRA_DIST? If the test files are not compiled, is there a reason to include them in the tarball?

The stripRRsigs function, should the whole (nontrivial) implementation be in header file? And, is the ZoneFinder? the best place for it? Doesn't the function work on any RRset?

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?

Both toWire methods would probably deserve a description about what is being returned. If I didn't read the code, I'd ask if it was number of bytes, number of records or some status code.

This comment isn't true any more (or, it is probably true in some sense, but still very misleading) ‒ the removed one lowers the count by 2, not 1:

// Locate the AAAA RRset and remove it; this has one RR in it.

Regarding this:

#if (0)
        // Disabled by #2165. The RRSIG RRsets are no longer directly
        // stored in the Message's rrsets, so the iterator will not find
        // them. The expected text used in many tests are flattened,
        // where the RRSIGs are inline. In other words, RRSIGs may occur
        // between (expected_begin, expected_end) but not between
        // (actual_begin, actual_end).

        // make sure rrsets only contains expected RRsets
        EXPECT_EQ(std::distance(expected_begin, expected_end),
                  std::distance(actual_begin, actual_end));
#endif

First, I don't like to see any code commented out without any FIXME or TODO mark besides it (these allow for easy grepping for problems).

However, the more serious concern is, when do we fix the test to test what it did before? Is there a ticket for it? Will it be deprecated? If not, maybe rendering it to text, sorting both texts by lines and comparing as strings? Would that work? Or, we need to see they are the same size, so actually comparing only the lengths of the strings?

Thank you

comment:11 follow-up: Changed 7 years ago by 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:

    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:

    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.

Last edited 7 years ago by jinmei (previous) (diff)

comment:12 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi vorner

Replying to vorner:

Hello

I'll answer both tickets here, I reviewed it as one diff.

Regarding style, you use rr as the name for an RRset in many places. Isn't that misleading little bit? RR usually means single Resource Record. Or is it really so the RRset contains only one?

This was unintentional. It was used simply as a tiny temporary name, such as i for example. I'll change them to r or rp. :)

It is not related to this ticket much, but it reminded me of the problem. There are many calls like this:

boost::const_pointer_cast<AbstractRRset>(rrset));

This is bad, but there was some problem with some method of rrset not being const even if it was a getter method and there was some technical reason for it inside the implementation. I think it was related to the signatures. Would you know by any chance if it is still a problem?

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());
    
  • In this one, RRset::addRRsig() is not const and would modify the covered_rrset.
    lib/datasrc/memory_datasrc.cc-        // cases.
    lib/datasrc/memory_datasrc.cc:        boost::const_pointer_cast<AbstractRRset>(covered_rrset)->addRRsig(sig_rrset);
    lib/datasrc/memory_datasrc.cc-
    

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

if (found_rrset != found_domain->second.end()) {
    ConstRRsetPtr rrset;
    // Strip whatever signature there is in case DNSSEC is not required
    // Just to make sure the Query asks for it when it is needed
    if ((options & ZoneFinder::FIND_DNSSEC) != 0 &&
        found_rrset->second->getRRsig()) {
        rrset = found_rrset->second;
    } else {
        RRsetPtr noconst(new RRset(found_rrset->second->getName(),
                                   found_rrset->second->getClass(),
                                   found_rrset->second->getType(),
                                   found_rrset->second->getTTL()));
        for (RdataIteratorPtr
             i(found_rrset->second->getRdataIterator());
             !i->isLast(); i->next()) {
            noconst->addRdata(i->getCurrent());
        }
        rrset = noconst;
    }
    return (createContext(options, SUCCESS, rrset));

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.

You disabled the tests for the old API. If I leave aside the fact we still have the API in our tree, even if it is not used (I think it is not used), why are the files listed in EXTRA_DIST? If the test files are not compiled, is there a reason to include them in the tarball?

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.

The stripRRsigs function, should the whole (nontrivial) implementation be in header file? And, is the ZoneFinder? the best place for it? Doesn't the function work on any RRset?

ZoneFinder is probably not the best place for all of that code, but it checks the options argument against FIND_DNSSEC which is why it's there. Otherwise we'll have to check these inline in code and conditionally call stripRRsigs() as it doesn't make sense for RRset::stripRRsigs() to take a bool argument (whether to strip or not). A part of it can be moved to RRset, but if we want to use it optimally by not creating RRsetPtr unless required, it'll result in ugly API as AbstractRRset::stripRRsigs() could either return (this) or a new RRset depending on whether RRSIGs are present.

The code has been moved to zone_finder.cc. It's used by users of datasrc, so unless it becomes a problem I think we can keep it as it is.

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.

Both toWire methods would probably deserve a description about what is being returned. If I didn't read the code, I'd ask if it was number of bytes, number of records or some status code.

API functions are properly documented in the header files. RRset::toWire() inherits from AbstractRRset::toWire() and says so in its API documentation. AbstractRRset::toWire() has detailed documentation. It returns the number of records rendered.

This comment isn't true any more (or, it is probably true in some sense, but still very misleading) ‒ the removed one lowers the count by 2, not 1:

// Locate the AAAA RRset and remove it; this has one RR in it.

This comment has been updated. :)

Regarding this:

#if (0)
        // Disabled by #2165. The RRSIG RRsets are no longer directly
        // stored in the Message's rrsets, so the iterator will not find
        // them. The expected text used in many tests are flattened,
        // where the RRSIGs are inline. In other words, RRSIGs may occur
        // between (expected_begin, expected_end) but not between
        // (actual_begin, actual_end).

        // make sure rrsets only contains expected RRsets
        EXPECT_EQ(std::distance(expected_begin, expected_end),
                  std::distance(actual_begin, actual_end));
#endif

First, I don't like to see any code commented out without any FIXME or TODO mark besides it (these allow for easy grepping for problems).

However, the more serious concern is, when do we fix the test to test what it did before? Is there a ticket for it? Will it be deprecated? If not, maybe rendering it to text, sorting both texts by lines and comparing as strings? Would that work? Or, we need to see they are the same size, so actually comparing only the lengths of the strings?

This test doesn't make sense anymore as the RRSIGs are no longer "inline" in Message's rrsets vector, and the test measures vector index distances. I couldn't quickly think of a way to modify it in place to get the same effect. It could mean writing fresh tests for such calls. I've created #2223 and added a TODO pointing to that bug in the comment. :)

Replying to 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:

    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:

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

comment:13 in reply to: ↑ 12 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks

Hello

Replying to 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:

TEST_F(MessageTest, toWireSignedAndTruncated) {
    // EDNSPtr edns(new EDNS());
    // edns->setUDPSize(256);
    // message_render.setEDNS(edns);

Replying to 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:

    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:

    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.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by jinmei

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

comment:15 in reply to: ↑ 14 ; follow-up: Changed 7 years ago by muks

  • Owner changed from muks to vorner

Hi vorner

Replying to vorner:

Replying to 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:

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 jinmei:

Replying to 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 RRsets if FIND_DNSSEC is not specified. Another unittest DatabaseClientTest.findRRSIGsWithoutDNSSEC has been added for this so that the behaviour is clear and tested.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to muks
  • Total Hours changed from 0 to 2.91

Hello

Replying to muks:

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.

Hmm, OK. Not related to this ticket. But it should be fixed sometime.

So, I think it can be merged.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 7 years ago by jinmei

Replying to vorner:

Hello

Replying to muks:

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.

How exactly? At least in the new API Message shouldn't have to
modify the passed RRsets for responses. As for setTTL(), I cannot
even find a code path in Message where setTTL() happens in the
rendering mode.

comment:18 in reply to: ↑ 17 Changed 7 years ago by muks

Replying to jinmei:

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

How exactly? At least in the new API Message shouldn't have to
modify the passed RRsets for responses. As for setTTL(), I cannot
even find a code path in Message where setTTL() happens in the
rendering mode.

Ah, my bad. Both fromWire() and toWire() use the same rrsets_ array, and this happens in the fromWire() case. We can refactor this code to const and non-const parts so Message::addRRset() takes const, but it would be larger than a trivial change and outside the scope of this bug. Let's open another bug for it.

comment:19 Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged to master in commit 5d1e19947ce5875cd950f1b1750dad5076909446:

* 79f1c5b (origin/trac2165_2) [2165] Remove effects that were used during EDNS testing
* 1159e5b [2165] Test RRType::RRSIG() query when FIND_DNSSEC is not passed
* abcd7d9 [2165] Fix answer section count in comment
* 2fabaed [2165] Don't continue and render RRSIGs if data is truncated
* 11af4c1 [2165] Add TODO to commented out test code
* 33ab70a [2165] Update comment
* a173297 [2165] Move ZoneFinder::stripRRsigs() into a .cc file
* 3afbf54 [2165] Rename variables
* 552c578 [2165] Remove another block of redundant code
* 3c20e03 [2165] Ignore RRSIG database records if sigs argument is false
* 32dd44a [2165] Rename member variable to contain underscore
* a8d00ea [2165] Fix typo in comment
* 94d0fd2 [2165] updater.delete_rrset no longer raises (as there are no attached RRSIGs now)
* 1fbbeb3 [2165] Stop RRSIGs from being added in getRRsets() itself
* 7f00956 [2165] Don't strip RRSIGs from DNSSEC rrsets
* 9a13449 [2165] Remove RRSIG filtering code from getAdditionalAddrs()
* 2a22df2 [2165] Check for NULL rrsets in stripRRsigs() itself
* 52f1d2c [2165] Remove multiple copies of the same code
* 18324f0 [2165] Fix NSEC3 tests to expect RRSIGs in some more cases
* 98a0dad [2165] Fix NSEC3 tests to expect RRSIGs in some cases
* f060129 [2165] Strip RRSIGs in MockZoneFinder when DNSSEC find option is not specified
* ef85853 [2165] Strip RRSIGs from addditional records in DB datasource when DNSSEC is not asked
* 4ecd5fb [2165] Don't add another copy of RRSIGs to the actual rrsets
* cdfbc35 [2165] Strip RRSIGs from addditional records in memory datasource when DNSSEC is not asked
* d057be8 [2165] Add the attached RRSIG when dumping RRsets
* 525a74d [2165] Disable test code that is no longer correct
* 8cc84c6 [2165] Add back deleted dnssec flag in some places
* 5438673 [2165] Update prepareRRset() to filter RRSIGs when ZoneFinder::FIND_DNSSEC is not passed
* 4c86091 [2165] Disable some datasrc tests which cover obsolete API
* c4500a6 [2166] Update RRset::toWire() so it will render RRSIGs too
* ace794f [2165] Make Message::addRRset() to be unaware of signedness

Resolving as fixed. Thank you for the reviews Michael and Jinmei.

Note: See TracTickets for help on using tickets.