BIND 10 #1104: support TSIG in DNS (Request) ACL

[BIND 10 Development]

#1104: support TSIG in DNS (Request) ACL
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110802
                  Component:         |            Resolution:
  xfrout                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Thanks for the review.

 Replying to [comment:6 stephen]:
 > '''src/bin/xfrout/tests/xfrout_test.py.in'''
 > In the part of test_parse_query_message that does the TSIG ACL checks,
 TSIG_KEY is added to the "self.xfrsess" key ring multiple times - is this
 needed?

 Ah, good catch.  No, they are not needed (not harmful in this context, but
 simply redundant).  Removed them, and added supplemental tests to catch
 it if we do this again due to a naive copy.

 > '''src/lib/acl/tests/dnsname_check_unittest.cc'''
 > In the "match" test, the superdomain against which the check should be
 made should be "com", not "org".

 Another good catch (and it's embarassing in that this type of error
 shouldn't happen with a pure TDD).  Fixed.

 > '''src/lib/python/isc/acl/_dns.py'''
 > Is this really a good name for this file? _dns.py" is very close to
 "dns.py".

 I don't know if it's "good" or not:-) but it seems to be the common
 convention in Python libraries.  For example, socket.py uses
 _socket.py the sqlite3 package uses _sqlite3.so, etc.

 > A comment in this file refers to "log.so", which appears not to be
 relevant here.

 Yet another good catch.  In fact, the entire comment should rather be
 the same one as other .py in that directory.  So I replaced it
 as a whole.

 I also noticed some editorial errors in comment lines of a .cc file,
 so I fixed them.

 > '''Miscellaneous'''
 > The TSIG ACL check is only on the basis of record name, which prompts
 the question "can we guarantee that the TSIG data is always checked?".  In
 other words, could it be possible for a user to construct an ACL for some
 operation that includes a check on the TSIG key, but for the code for that
 operation not to check it?  In which case security could be subverted by
 sending through a key of a given name but with arbitrary data.

 We cannot guarantee that.  The assumption is that if an application uses
 a TSIG based ACL, it's application's responsibility to perform TSIG
 validation before performing the ACL check.  Maybe we should document
 it as a note somewhere (but I'm not sure what's the best place for that.
 In changelog at the moment?)

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