Opened 8 years ago

Closed 8 years ago

#1574 closed task (complete)

Add support for loading NSEC3 RRsets to in memory data source

Reported by: jinmei Owned by: jinmei
Priority: high Milestone: Sprint-20120207
Component: data source Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: NSEC3
Estimated Difficulty: 7 Add Hours to Ticket: 0
Total Hours: 17.25 Internal?: no

Description

We use a separate storage for NSEC3 RR sets in the in memory data
source. That could even be a straightforward STL map<Name,
(Const)RRsetPtr> (this is not efficient in terms of memory
footprint, but should be okay for the initial implementation).

For simplicity, I propose ensuring consistency of NSEC3 parameters
at load time: As we add NSEC3s to that storage, check the hash
algorithm, iteration, and salt are the same for all RRs. At the end
of loading check the existence of NSEC3PARAM RR at the apex, and
check the parameters of NSEC3PARAM and of NSEC3 RRs are the same.
Reject loading if these aren't met.

Subtickets

Change History (24)

comment:1 follow-up: Changed 8 years ago by jelte

Do we have an exit strategy for this consistency check? I have no problems with it for now, but at some point we'll have to support adding NSEC3 RRs that do not (yet) have an NSEC3PARAM, in order to support incrementally rolling entire nsec3 chains.

comment:2 in reply to: ↑ 1 Changed 8 years ago by jinmei

Replying to jelte:

Do we have an exit strategy for this consistency check? I have no problems with it for now, but at some point we'll have to support adding NSEC3 RRs that do not (yet) have an NSEC3PARAM, in order to support incrementally rolling entire nsec3 chains.

I'm not sure what "incrementally rolling" means, but the proposed
simple check algorithm doesn't rely on the existence of NSEC3PARAM
until at the very end (in the first phase it only checks all NSEC3s
have the same set of parameters). The situation will change if and
when we want to support multiple sets of parameters, though. Are you
asking about what we should do at that point?

comment:3 follow-up: Changed 8 years ago by jelte

Yes; the scenario i have in mind is when you have a large zone, and want to change the NSEC3 salt for instance. The 'incremental' way to do that would be to add the newly generated NSEC3s in batches (keeping the old, and leaving NSEC3PARAM as it is), and when they are all there, replace the NSEC3PARAM and start removing the old NSEC3 RRs in batches.

I'm not saying we should do that, when we do automatic signing we might very well replace them all at once, but we should definitely allow the scenario at some point.

I'm guessing we would have multiple NSEC3 namespaces, one of which is 'active' (as pointed out by the NSEC3PARAM record at the apex).

comment:4 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:5 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 6 to 7

comment:6 Changed 8 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120207
  • Priority changed from major to critical

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

Replying to jelte:

Yes; the scenario i have in mind is when you have a large zone, and want to change the NSEC3 salt for instance. The 'incremental' way to do that would be to add the newly generated NSEC3s in batches (keeping the old, and leaving NSEC3PARAM as it is), and when they are all there, replace the NSEC3PARAM and start removing the old NSEC3 RRs in batches.

I'm not saying we should do that, when we do automatic signing we might very well replace them all at once, but we should definitely allow the scenario at some point.

I'm guessing we would have multiple NSEC3 namespaces, one of which is 'active' (as pointed out by the NSEC3PARAM record at the apex).

I think there are a few ways to do that without requiring
fundamentally redesigning everything. Having multiple namespaces is
one way. So I believe we can find a way of evolution without breaking
deployed practices.

comment:8 Changed 8 years ago by jinmei

  • Owner set to jinmei
  • Status changed from new to accepted

comment:9 Changed 8 years ago by jinmei

This task is getting big (it was correctly given a relatively large
estimate), so I decided to divide it two subtasks.

The first part is ready for review at branch trac1574.

Feature-wise, it's mostly completed. The major remaining task is
parameter consistency check among NSEC3 RRs and NSEC3PARAM, but even
without them it should be able to load normally NSEC3-signed zone
correctly.

The second part would probably be better to be developed using #1575
(which I believe is almost ready for merge). That's another reason
why I separated the task at this point.

I don't plan to add a changelog entry for this task since it's
basically an internal update, and, until we use other features of
NSEC3 lookups it's invisible to users.

comment:10 Changed 8 years ago by jinmei

  • Owner changed from jinmei to UnAssigned
  • Status changed from accepted to reviewing

comment:11 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:12 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei

Hello

Reading throuh it, I find just one problem (provided the tmp method will be replaced by a real one in a future ticket). Why do you create the ToUpper? class? It doesn't hold any state or like that. Shouldn't transform be able to take toupper directly?

Also, I know it's not problem of this branch itself, but we require some ordering of data in the zone file (or more precisely, the input stream). When do we want to change it? Because a database might give it in any order, so can (in theory) be in the zone file.

Thank you

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

Replying to vorner:

Reading throuh it, I find just one problem (provided the tmp method will be replaced by a real one in a future ticket). Why do you create the ToUpper? class? It doesn't hold any state or like that. Shouldn't transform be able to take toupper directly?

Unfortunately not, because the signature doesn't fully match.
transform expects char(*)(char), but toupper's type is int(*)(int).
But it doesn't necessarily have to be a functor, but an ordinary
wrapper function

#ifdef instead_of this
    struct ToUpper {
        char operator()(char ch) { return (toupper(ch)); }
    };
#endif
    char ToUpper(char ch) { return (toupper(ch)); }

If you prefer it, I'm okay with changing that (hmm, and maybe in an
unnamed namespace outside the class, because it's independent from
class internals).

BTW, for now I've moved the definition of ToUpper? a bit, as it seemed
more reasonable.

Also, I know it's not problem of this branch itself, but we require some ordering of data in the zone file (or more precisely, the input stream). When do we want to change it? Because a database might give it in any order, so can (in theory) be in the zone file.

I think we should solve this either when we want to support loading
from a different data source or when we support generic (and more
correct than b10-loadzone) master file parser. Right now, in-memory
data source requires preprocessing with BIND 9's named-compilezone in
practice, and its output meets the requirement, so the assumption is
safe.

comment:14 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:15 Changed 8 years ago by jinmei

Assuming the first part is generally agreed, I've also picked up and
completed the second part. It's ready for review at branch trac1574b.

It consists of two parts: extensions to NSEC3Hash class (in
src/lib/dns) and updates to the in-memory data source implementation
(in src/lib/datasrc). The NSEC3Hash extensions are independent
changes and can be reviewed separately. The datasrc update depends on
it.

If you don't think you can review trac1574b (once trac1574 is fully
completed) please let me know. Then I'll probably create a separate
ticket to explicitly ask for additional review.

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

Replying to vorner:

Reading throuh it, I find just one problem (provided the tmp method will be replaced by a real one in a future ticket). Why do you create the ToUpper? class? It doesn't hold any state or like that. Shouldn't transform be able to take toupper directly?

Unfortunately not, because the signature doesn't fully match.
transform expects char(*)(char), but toupper's type is int(*)(int).
But it doesn't necessarily have to be a functor, but an ordinary
wrapper function

The signature doesn't matter, because transform uses a template parameter for the function/functor. That's why you can pass a functor inside anyway. The only thing needed is for the parameter/return value to be convertible automatically, which, as demonstrated by the functor, is possible.

I tried it and there's a different problem. There's one good old C toupper and another C++ std::toupper. And we have using namespace std at the top of the file, which means it can't resolve between these two (the template parameter of transform is deduced from the signature of the function and there are several). However, this seems to work, because it uniquely selects one toupper (the C one):

transform(fst_label.begin(), fst_label.end(),
          fst_label.begin(), ::toupper);

But if it wouldn't work on some compiler, I'd prefer having a function wrapper than functor, a functor is an overkill.

Thank you (I'll return the ticket once I get through the second part).

comment:17 follow-ups: Changed 8 years ago by vorner

Hello

I think a file is missing from git, because distcheck fails with:

make[4]: Entering directory `/home/vorner/work/bind10/src/lib/datasrc/tests'
make[4]: *** No rule to make target `testdata/example.org.nsec3-signed', needed by `distdir'.  Stop.
make[4]: Leaving directory `/home/vorner/work/bind10/src/lib/datasrc/tests'

and make check with this:

[ RUN      ] InMemoryZoneFinderTest.loadNSEC3Zone
terminate called after throwing an instance of 'isc::dns::MasterLoadError'
 what():  Failed to open master file: /home/vorner/work/bind10/src/lib/datasrc/tests/testdata/example.org.nsec3-signed
/bin/sh: line 5: 27193 Aborted                 (core dumped) ${dir}$tst
FAIL: run_unittests_memory

Otherwise, I have just some nitpicking:

Should this be "does request" instead of "doesn't"?

        // NSEC3/NSEC3PARAM is not a "singleton" per protocol, but this
        // implementation doesn't request it be so at the moment.

Do it "now"?

// If we've not done any NSEC3 setup for the zone, do it know;

Could it happen we have empty origin? I mean, the zone file would not be valid, but it could happen, couldn't it? I think we should raise an exception instead of crash on assert in that situation. Or, should such case be rejected sooner than this? If so, a comment why this can't happen in practice would be nice:

// If the zone is NSEC3-signed, check if it has NSEC3PARAM
if (tmp->nsec3_data_) {
    assert(!tmp->origin_data_->isEmpty());
    if (tmp->origin_data_->getData()->find(RRType::NSEC3PARAM()) ==
        tmp->origin_data_->getData()->end()) {
        LOG_WARN(logger, DATASRC_MEM_NO_NSEC3PARAM).
            arg(getOrigin()).arg(getClass());
    }
}

The python documentation explicitly says there are no exceptions from the match method. It is true we don't have any special exceptions, but the common ones, like bad number of arguments or bad argument types can still happen.

In both the C++ and python tests, the comment „Flags doesn't matter“ should probably be „don't“.

Thank you

comment:18 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to vorner:

(This part first)

I think a file is missing from git, because distcheck fails with:

Doh, yes, sorry about that. I added them.

comment:19 in reply to: ↑ 16 Changed 8 years ago by jinmei

Replying to vorner:

The signature doesn't matter, because transform uses a template parameter for the function/functor. That's why you can pass a functor inside anyway. The only thing needed is for the parameter/return value to be convertible automatically, which, as demonstrated by the functor, is possible.

Ah, yes, you are right in that signature doesn't matter.

I tried it and there's a different problem. There's one good old C toupper and another C++ std::toupper. And we have using namespace std at the top of the file, which means it can't resolve between these two (the template parameter of transform is deduced from the signature of the function and there are several). However, this seems to work, because it uniquely selects one toupper (the C one):

With :: it worked for me, too. But the cause of this doesn't seem to
be the ambiguity between C++/C toupper. The following code compiled
even with the "ambiguity":

#include <algorithm>
#include <cctype>
#include <string>
// If we uncomment this it doesn't compile
//#include <iostream>

using namespace std;

int
main() {
    string s("abcde");
    transform(s.begin(), s.end(), s.begin(), toupper);
    return (0);
}

but if I included iostream, I saw the same error, which could be
somehow resolved by adding '::' to toupper.

It didn't happen for SunStudio? + stlport, so I suspect it's some kind
of implementation specific pitfall (if not a bug) of g++ version of
STL.

Anyway, as long as it works I agree it's better not to rely on
external helper function or functor, so I updated the code with :: and
added note about it.

I've made this change to trac1574b, not the original trac1574.

comment:20 in reply to: ↑ 17 Changed 8 years ago by jinmei

Replying to vorner:

Otherwise, I have just some nitpicking:

I believe I've addressed these points. Please see the latest version
of the branch.

In both the C++ and python tests, the comment „Flags doesn't matter“ should probably be „don't“.

Regarding this one, my intent was "(the) Flags (field) doesn't
matter", but I see it's not obvious and looks awkward. So, in the end
I've changed them to "don't" as suggested.

comment:21 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

comment:22 follow-up: Changed 8 years ago by vorner

  • Owner changed from vorner to jinmei
  • Total Hours changed from 0 to 4.25

Hello

I think it looks OK now, please merge.

Thank you

comment:23 in reply to: ↑ 22 Changed 8 years ago by jinmei

Replying to vorner:

I think it looks OK now, please merge.

Thanks, merged, closing.

comment:24 Changed 8 years ago by jinmei

  • Resolution set to complete
  • Status changed from reviewing to closed
  • Total Hours changed from 4.25 to 17.25
Note: See TracTickets for help on using tickets.