BIND 10 #1771: database datasource incorrectly rejects "on zonecut" glue

[BIND 10 Development]

#1771: database datasource incorrectly rejects "on zonecut" glue
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  vorner
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120612
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:  data   |           Sub-Project:  DNS
  source                             |  Estimated Difficulty:  3
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by muks):

 * owner:  muks => vorner


Comment:

 Replying to [comment:14 vorner]:
 > > I didn't follow what you mean here by any A/AAAA. Can you give me an
 example of what case you mean?
 >
 > Well, let's say we have this in the same domain name domain.example.org:
 > NS sub.domain.example.org.
 > NS sub2.domain.example.org.
 > A 192.0.2.1
 >
 > The A record is not the glue, it is not referenced by the NS. But this
 will not complain. However, such checks would be crazy to code and mostly
 useless anyway. I just wanted to note it.

 `may_have_glue` doesn't allow all in-bailiwick cases, but only the exact
 match. i.e., `may_have_glue` is only `true` if `NS domain.example.org`
 exists and then the A RR would be allowed. In the above case,
 `may_have_glue` would be `false`.

 > > > And, as this is a publicly-observable bug, I think it should have a
 changelog entry.
 > >
 > > How about this:
 > > {{{
 > > XYZ.    [bug]           muks
 > >         The database datasource has been fixed to not reject glue
 records.
 > > }}}
 >
 > Not all glue records were rejected before, were they?

 You are right. Before, there was a reject coded only for the cases where a
 NS record existed and the delegated zone name had other RRs. In-domain
 cases where the NS record did not equal the delegated zone name (such as
 the example you've provided above) would have passed. I'll update the
 ChangeLog message for it.

 > Replying to [comment:12 muks]:
 > This raises another question. What is the situation when the exception
 is thrown? Can it happen at all? And if so, in what circumstances? Can it
 be a false positive too?
 >
 > {{{#!c++
 >     if (check_ns && seen_ns && (!may_have_glue && seen_other)) {
 >         isc_throw(DataSourceError, "NS shares domain " << name <<
 >                   " with something else");
 >     }
 > }}}

 In the example that you provided above, this exception would be thrown
 now.


 Replying to [comment:15 jinmei]:
 > And, actually, I'd rather basically omit the "NS + seen_other" check,
 > maybe except for specific cases like NS + DNAME that are prohibited by
 > the protocol.
 >
 > We already ensure that records on and under a zone cut are hidden at
 > the caller side of getRRsets() (unless GLUE_OK is specified), and, as
 > long as it's ensured I think it's too restrictive to reject all other
 > cases.  IMO this layer should be flexible about what kind of data can
 > be a glue (except for those already prohibited by the protocol), and
 > let the higher layer such as auth::Query class make that decision.

 Removing that check (or replacing with NS+DNAME check) causes some tests
 to fail. I have to check why it does (and read the datasrc code more
 completely). But for now, for this bug, can we use the code in this branch
 which does not change behavior significantly?

 > In any case the current implementation seems quite tricky:
 >
 > {{{#!cpp
 >                 seen_ns = true;
 >
 >                 if (Name(columns[DatabaseAccessor::RDATA_COLUMN]) ==
 Name(name))
 >                     may_have_glue = true;
 > }}}
 >
 > I cannot understand from a glance what this condition really means, in
 > which case it doesn't hold, and for what purpose we do this (besides,
 > it needs editorial fixes: it's too long, and misses braces).  Even if
 > we still want to selectively reject some cases, I'd like to see more
 > intuitive (and if possible more efficient - it requires text-to-name
 > conversions twice and comparison of two names, which are not very
 > cheap) implementation; and, even if the end result is still this
 > check, I think we need to explain it in comments.

 This specifically checks for the in-bailiwick case where the NS name
 equals the delegated zone name exactly, and allows glue. Before these
 changes, the code in this method allowed other kinds of _valid glue_
 anyway, just not the NS name == delegated zone name case. But in something
 like the following, the A record is NOT glue:

 {{{
 foo.example.org. NS 3600 ns.example.com.
 foo.example.org. A  3600 192.0.2.1
 }}}

 The Name(name) has been replaced with a Name object outside the loop.

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