Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#1431 closed enhancement (complete)

NSEC3: closest provable encloser proof

Reported by: stephen Owned by: jinmei
Priority: medium Milestone: Sprint-20120124
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket: NSEC3
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 14.35 Internal?: no

Description

Given a domain name, update the new data source interface to include a method that returns the NSEC3 RR or RRs that prove the existence of the closest encloser (or closest provable encloser). (See RFC 5155 section 7.2.1.)

Subtickets

Change History (24)

comment:1 Changed 8 years ago by stephen

  • Type changed from defect to enhancement

comment:2 Changed 8 years ago by shane

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

comment:3 Changed 8 years ago by shane

  • Feature Depending on Ticket set to NSEC3

comment:4 Changed 8 years ago by jelte

  • Estimated Difficulty changed from 0 to 6

comment:5 Changed 8 years ago by jelte

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

comment:6 Changed 8 years ago by jinmei

Note: if this task intends to update DatabaseClient::Finder::find(),
it's better to hold off until #1198 is completed.

comment:7 Changed 8 years ago by jelte

  • Milestone changed from Sprint-20111220 to Year 3 Task Backlog

comment:8 Changed 8 years ago by jelte

  • Milestone changed from Year 3 Task Backlog to Sprint-20120124

comment:9 Changed 8 years ago by jinmei

Please hold off for this task for a while. As I look at #1562 I now think it would be better
to start this task after clarifying what we should specifically do along with #1562.
I expect we (I) can clarify that within a day or so.

comment:10 Changed 8 years ago by jinmei

After considering the whole picture of supporting NSEC3 in in-memory
data source, I now propose the following for this ticket:

Define the interface of a new method of ZoneFinder, findNSEC3, and
implement it in auth/tests/query_unittest.cc:MockZoneFinder.

The description of the method is:

std::pair<bool, ConstRRsetPtr>
ZoneFinder::findNSEC3(const Name& name, bool recursive);

It searches the (conceptual) auxiliary table of zone that stores NSEC3
RRsets for exact or covering NSEC3 for the given name.
if recursive is true, it will continue the search up toward the
zone apex until it finds a provable enclosure NSEC3.
If it cannot find a provable enclosure even at the apex, or
if it cannot find any NSEC3 when recursive is false, it throws.

It returns std::pair<bool, ConstRRsetPtr>. The first element is
true if the found NSEC3 is an exact match (this should always be
the case when recursive is true); it's false if it's a covering
NSEC3. The second element points to the found NSEC3 RRset. This
must never be null.

We'll need to implement this for in memory version of the finder, but
it's better to do that after #1574 and #1575. Also, by providing it
in the auth tests we can start developing the higher level support
(i.e. updates to auth::Query) independently from lower level support.

comment:11 Changed 8 years ago by jinmei

One more suggestion, assuming the expected task is quite easy:
add the same change as #1578 for the MockZoneFinder

comment:12 Changed 8 years ago by jinmei

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

comment:13 Changed 8 years ago by jinmei

trac1431 is ready for review.

On thinking about the interface further as I wrote the mock
implementation, I thought the interface could be (hopefully) better
with some modifications. So the resulting interface is slightly
different from the very original one. See the documentation for
details.

One possible controversial point is whether to allow 'known_encloser'
parameter for possible optimization. Right now we don't necessarily
need it because the underlying data source won't be optimized so soon.
I thought it's quite clear we'll eventually want to provide some
information that encapsulates data source specific knowledge to
findNSEC3() once we reach the point of optimizing things and it would
be nice if the auth code expects it from this stage, but if this
specific feature is considered premature, I'm okay with removing it
for now.

While this is a new API, it's not provide any user visible feature, so
I don't plan to add changelog for this ticket.

comment:14 Changed 8 years ago by jinmei

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

comment:15 Changed 8 years ago by vorner

  • Owner changed from UnAssigned to vorner

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

  • Owner changed from vorner to jinmei

Hello

The idea of passing a hint through the code seems very reasonable (so we won't need to change the interface in future, if we're not going to use the hint itself for now too much). However, looking at the code and the interface, the fact we are returning the empty NSEC3 RRset to signal it is signed with NSEC3 and pass the hint feels like a misuse. It is both not obvious and inconsistent with what the NSEC case does.

What I would propose would be extending the FindResult? to include the following:

  • Some flags (eg. „zone is signed“, „it is signed with NSEC3“, possibly removing the flag of „The result is wildcard“ from the main status and including it as a separate flag).
  • A shared_ptr<Hints> (or scoped_ptr, or whatever). The Hints class would be either a base class or only declared and a implementation-dependant definition would be present in each different data source (the first one needs dynamic_cast on the accepting end, but is „by the handbook“, the other is little bit misuse, having multiple real types with the same name, so it's effectively a hidden anonymous pointer).

Also, I'd have few code-level details:

Indentation:

+        /// TBD
+       virtual FindNSEC3Result
+        findNSEC3(const isc::dns::Name& name, bool recursive,
+                  const isc::dns::ConstRRsetPtr known_encloser);
+

I believe it expects there's NSEC3PARAMS RR in the apex, not NSEC3, because the matching NSEC3 is hard to guess without knowing the salt and algorithm:

    /// In general, this method expects the zone is properly signed with NSEC3
    /// RRs.  Specifically, it assumes at least the apex node has a matching
    /// NSEC3 RR.  So the search must always succeed; if the assumption isn't
    /// met, \c DataSourceError exception will be thrown

It should be said that the known_encloser should be an RRset coming from the same data source as a result of find, not that a user would be allowed to construct it directly (eg. the caller shouldn't guess a name, create the RRset himself and pass it, because the findNSEC3 method might expect it to be a derived class with hints).

The DataSourceError exception is usually raised on database errors and such, as well as bad data. However, this comment indicates that if the exception is raised, it always means it is improperly signed, but it might be a low-level database error in reality:

    /// \exception DataSourceError The zone isn't properly signed with NSEC3
    /// (NSEC3 parameters cannot be found; no NSEC3s are available, etc).

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

Replying to vorner:

Thanks for the prompt review.

The idea of passing a hint through the code seems very reasonable (so we won't need to change the interface in future, if we're not going to use the hint itself for now too much). However, looking at the code and the interface, the fact we are returning the empty NSEC3 RRset to signal it is signed with NSEC3 and pass the hint feels like a misuse. It is both not obvious and inconsistent with what the NSEC case does.

Point taken. I admit it's not clean or intuitive to use the returned
RRset as an optimization hint.

What I would propose would be extending the FindResult? to include the following:

  • Some flags (eg. „zone is signed“, „it is signed with NSEC3“, possibly removing the flag of „The result is wildcard“ from the main status and including it as a separate flag).
  • A shared_ptr<Hints> (or scoped_ptr, or whatever). The Hints class would be either a base class or only declared and a implementation-dependant definition would be present in each different data source (the first one needs dynamic_cast on the accepting end, but is „by the handbook“, the other is little bit misuse, having multiple real types with the same name, so it's effectively a hidden anonymous pointer).

I also agree it makes sense to introduce "flags" information. In fact
I've been wondering whether the information of "wildcard match" should
be integrated to the result code (because it will make it harder to
combine processing common to wildcard cases, such as for
WILDCARD_CNAME and normal WILDCARD).

As for the explicit Hints class, on thinking about it further, I guess
we might (eventually) do something like this:

  • Define FindContext? (or "Hints", naming is not important). It's a base class, and actual data source implementation would encapsulate useful internal information in it for possible post-find processing
  • ZoneFinder::find() can return (a (shared) pointer to) FindContext? in FindResult? if it want to allow optimization in later processing. It's optional and can be NULL.
  • FindContext? has methods like "findNSEC3()" or "findAdditional()", which are generally optimized version of ZoneFinder?'s counter part, and should be generally optimized exploiting the encapsulated information linked to its internal details.
  • The application may use the FindContext? version of these methods if they want and it's returned in the hope that it may be more efficient.

This way we can also avoid the need for down cast (dynamic_cast) and
can make the interface type-safer.

Would something like this make sense? If so, what I'd propose for
this ticket is:

  • introduce the flag field to FindResult? and update the find() interface so that if the zone is signed with NSEC/NSEC3 the corresponding flags are set, and same for wildcard.
  • deprecate WILDCARD_xxx and have the caller refer to this flag
  • remove the idea of returning NSEC3 RRset from find() and allowing the caller to use it for findNSEC3() for now. The idea of FindContext? will be big enough (and non urgent), so it's probably better to postpone it.

Also, I'd have few code-level details:

Indentation:

+        /// TBD
+       virtual FindNSEC3Result
+        findNSEC3(const isc::dns::Name& name, bool recursive,
+                  const isc::dns::ConstRRsetPtr known_encloser);
+

Oops, actually this "TBD" should have been updated (with indentation
fix as a side effect), but I forgot to commit/push it. Now it should
look okay.

I believe it expects there's NSEC3PARAMS RR in the apex, not NSEC3, because the matching NSEC3 is hard to guess without knowing the salt and algorithm:

    /// In general, this method expects the zone is properly signed with NSEC3
    /// RRs.  Specifically, it assumes at least the apex node has a matching
    /// NSEC3 RR.  So the search must always succeed; if the assumption isn't
    /// met, \c DataSourceError exception will be thrown

This was indeed (intended to be) about NSEC3. The point in this
context is that in the recursive mode the search should always
succeed. I omitted that the parameters should be available, too,
because it's already noted in the documentation, but for clarity I
added that too.

It should be said that the known_encloser should be an RRset coming from the same data source as a result of find, not that a user would be allowed to construct it directly (eg. the caller shouldn't guess a name, create the RRset himself and pass it, because the findNSEC3 method might expect it to be a derived class with hints).

As I proposed above, I think I'd cancel the idea of using RRset for
this purpose for now, so I didn't update the doc on this point.

The DataSourceError exception is usually raised on database errors and such, as well as bad data. However, this comment indicates that if the exception is raised, it always means it is improperly signed, but it might be a low-level database error in reality:

    /// \exception DataSourceError The zone isn't properly signed with NSEC3
    /// (NSEC3 parameters cannot be found; no NSEC3s are available, etc).

Okay, updated. Hopefully the new version addresses the concern.

comment:18 Changed 8 years ago by jinmei

  • Owner changed from jinmei to vorner

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

Replying to jinmei:

Would something like this make sense? If so, what I'd propose for
this ticket is:

  • introduce the flag field to FindResult? and update the find() interface so that if the zone is signed with NSEC/NSEC3 the corresponding flags are set, and same for wildcard.
  • deprecate WILDCARD_xxx and have the caller refer to this flag
  • remove the idea of returning NSEC3 RRset from find() and allowing the caller to use it for findNSEC3() for now. The idea of FindContext? will be big enough (and non urgent), so it's probably better to postpone it.

I've updated the branch toward this direction, but not completely
deprecated WILDCARD_xxx as it would require rather big changes to the
existing database data source implementation. For now, I introduced a
quick hack wrapper in the auth Query code to convert the old format to
the new one.

If this approach is okay I'll create a separate ticket for the
subsequent refactoring.

comment:20 Changed 8 years ago by jinmei

I've noticed we need one more extension, especially with canceling the
idea of returning RRsetPtr for the closest encloser from find():
the caller would now need to identify the closest encloser of a given
name. I've updated findNSEC3() and FindNSEC3Result so that
the caller can know the # of labels of the closest encloser (which
will be much cheaper than name) and, if necessary, can construct
the encloser name.

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

  • Owner changed from vorner to jinmei

Hello

Replying to jinmei:

As for the explicit Hints class, on thinking about it further, I guess
we might (eventually) do something like this:

[...]

This way we can also avoid the need for down cast (dynamic_cast) and
can make the interface type-safer.

This would indeed make the code slightly cleaner design-wise. However, I fear that it would be harder to use (as the caller would need to distinguish if a NULL was returned and if not, call the method on hints instead of the data source). Also, the code inside the data source would get duplicated and we would need to unify it, adding another layer the reader would need to get through. I'm not sure if avoiding one dynamic cast is worth this much.

Would something like this make sense? If so, what I'd propose for
this ticket is:

  • introduce the flag field to FindResult? and update the find() interface so that if the zone is signed with NSEC/NSEC3 the corresponding flags are set, and same for wildcard.
  • deprecate WILDCARD_xxx and have the caller refer to this flag
  • remove the idea of returning NSEC3 RRset from find() and allowing the caller to use it for findNSEC3() for now. The idea of FindContext? will be big enough (and non urgent), so it's probably better to postpone it.

ACK, this direction looks good, no matter if we do it the context or hints way.

Also, I'd have few code-level details:

Indentation:

+        /// TBD
+       virtual FindNSEC3Result
+        findNSEC3(const isc::dns::Name& name, bool recursive,
+                  const isc::dns::ConstRRsetPtr known_encloser);
+

Actually, one indentation problem was left (there were tabs for some reason), and I fixed it.

Replying to jinmei:

Replying to jinmei:

Would something like this make sense? If so, what I'd propose for
this ticket is:

  • introduce the flag field to FindResult? and update the find() interface so that if the zone is signed with NSEC/NSEC3 the corresponding flags are set, and same for wildcard.
  • deprecate WILDCARD_xxx and have the caller refer to this flag
  • remove the idea of returning NSEC3 RRset from find() and allowing the caller to use it for findNSEC3() for now. The idea of FindContext? will be big enough (and non urgent), so it's probably better to postpone it.

I've updated the branch toward this direction, but not completely
deprecated WILDCARD_xxx as it would require rather big changes to the
existing database data source implementation. For now, I introduced a
quick hack wrapper in the auth Query code to convert the old format to
the new one.

If this approach is okay I'll create a separate ticket for the
subsequent refactoring.

The wrapper looks kludge to me, but provided the ticket is handled soon enough (possibly in the next sprint), I think it is OK. The ticket should be relatively small, the only place that used WILDCARD_xxx is the database datasource and the compiler should be able to find all the occurrences if the definitions are removed.

The code itself looks good enough to merge, so please do. We can discuss the inclusion of hints or whatever in the result later and outside of this ticket, possibly on the next non-planning call? Or the mailing list?

Thank you

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

Replying to vorner:

Indentation:

+        /// TBD
+       virtual FindNSEC3Result
+        findNSEC3(const isc::dns::Name& name, bool recursive,
+                  const isc::dns::ConstRRsetPtr known_encloser);
+

Actually, one indentation problem was left (there were tabs for some reason), and I fixed it.

Oops, thanks.

Replying to jinmei:

I've updated the branch toward this direction, but not completely
deprecated WILDCARD_xxx as it would require rather big changes to the
existing database data source implementation. For now, I introduced a
quick hack wrapper in the auth Query code to convert the old format to
the new one.

If this approach is okay I'll create a separate ticket for the
subsequent refactoring.

The wrapper looks kludge to me, but provided the ticket is handled soon enough (possibly in the next sprint), I think it is OK. The ticket should be relatively small, the only place that used WILDCARD_xxx is the database datasource and the compiler should be able to find all the occurrences if the definitions are removed.

I know it's a kludge, and while the cleanup fix is essentially
straightforward, the diff is actually not small (right now git diff on
my trac1611 branch is over 1000 lines, containing 78 chunks. This
work is mostly done except for documentation update, so I'm quite sure
we can clean them up in the next sprint.

The code itself looks good enough to merge, so please do. We can discuss the inclusion of hints or whatever in the result later and outside of this ticket, possibly on the next non-planning call? Or the mailing list?

Both mailing list and biweekly call sound good to me.

For the moment I'll merge the current branch and close this ticket.
Thanks.

comment:23 Changed 8 years ago by jinmei

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

comment:24 Changed 8 years ago by jinmei

  • Total Hours changed from 0 to 14.35
Note: See TracTickets for help on using tickets.