BIND 10 #772: Update xfrout to use ACL checking library

[BIND 10 Development]

#772: Update xfrout to use ACL checking library
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  stephen                            |                Status:  reviewing
                       Type:         |             Milestone:
  enhancement                        |  Sprint-20110802
                   Priority:  major  |            Resolution:
                  Component:         |             Sensitive:  0
  xfrout                             |           Sub-Project:  DNS
                   Keywords:         |  Estimated Difficulty:  3.0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 First, I've made a few trivial changes/cleanups and directly pushed them.

 '''general'''
 - I'm not sure if we should reject incoming AXFR request by default.
   At least it's different from BIND 9's default.  It will also break
   current deployment of BIND 10 (although I think it's acceptable at
   the current maturity of BIND 10 as long as we warn it in a release
   note or something).

 '''misc in xfrout.py.in'''
 - parse_query_message(): shouldn't we skip performing ACL if TSIG
   check fails?  In fact, (although quite unlikely) if the fail is MAC
   mismatch, it may mean the message is broken, so performing ACL
   itself may not even make sense especially if and when we introduce
   ACl based on message content.  We may want to apply a "DROP" rule
   only by remote address even before the TSIG check, but as we
   discussed in the resolver case, that would probably be done by a
   separate ACL.
 - parse_query_message(): not sure if it's intentional, but you can
   omit "isc.acl.acl" below:
 {{{
             if acl_result == isc.acl.acl.DROP:
 ...
             elif acl_result == isc.acl.acl.REJECT:
 }}}
 - It seems we can merge reply_query_with_error_rcode() and
   reply_query_with_format_error().
 - update_config_data: we should probably log first so that it's logged
   even if load() fails with exception:
 {{{
         if 'ACL' in new_config:
             self._acl = REQUEST_LOADER.load(new_config['ACL'])
         logger.info(XFROUT_NEW_CONFIG)
 }}}
 - update_config_data: just checking, is it okay to not to protect it
   by lock?  maybe we should add comment about that.

 - config_handler: technically "Bad configuration" may be too
   restricted, because exception would happen for other (rare) reasons
   such as memory allocation failure.  maybe simply say "Failed to
   handle new configuration" or something?
 {{{
             except Exception as e:
                 answer = create_answer(1, "Bad configuration: " + str(e))
 }}}

 '''guess_remote'''
 - in the doc, "The socket must be a socket" sounds awkward.  Maybe
   "sock_fd must refer to a socket" or something?
 - The trick employed in this function seems to be the best current
   practice, but I'm afraid it's fragile.  I wouldn't be surprised
   there's a system that is more strict about the AF matching, for
   example.  For longer term, I guess we should pass some auxiliary
   information (at least the AF, socktype and protocol, and possibly
   local/remote addresses) when we pass a socket FD from one process to
   another.
 - As for this:
 {{{
             # To make it work even on hosts without IPv6 support
             # (Any idea how to simulate this in test?)
 }}}
   It seems we can override socket.has_ipv6.  So a test could tweak it
   before testing.  (but we might not bother to do this minor case -
   today's many system already supports IPv6, and if we implement the
   "longer term" solution in not far future, the problem will be gone
   anyway)

 '''xfrout_test'''
 - maybe a matter of taste, but you should be able to use assertEqual here:
 {{{
         self.assertTrue(rcode is None)
         #self.assertEqual(None, rcode)
 }}}
 - you mean "REFUSED"?
 {{{
 +        # This should be rejected, therefore NOTAUTH
 }}}

 '''xfrout_message.mes'''

 I'd explain %1 and %2 such as in '<zone name>/<zone class>'.  '%3:%4'
 to represent address+port is not a good way when it's IPv6 address.
 Ideally we should resolve the unification of the notation (should be a
 ticket for this, don't remember the number), but assuming it's not
 done yet, I'd suggest either %3#%4 (BIND 9 way) or [%3]:%4.

 '''changelog'''
 As discussed in 'general', if we keep rejecting requests by default,
 we should note it more explicitly that it's a backward incompatible
 change (and we should mark it with '*').

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