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):

 Replying to [comment:10 vorner]:

 I made one small change directly.  (Apparently I was not clear enough
 on this one in my previous comment, and the different part than I
 meant was changed.)

 > > '''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).
 >
 > I'm not sure about that either. I'm OK with any reasonable default, I
 just thought this one is the safest one. What do you think should be the
 default?

 Personally, I'd accept by default because conceptually xfrout would be
 part of auth, and we'd accept queries by default in auth.  But I may
 be biased because while I know there are some paranoid people who
 never want to answer xfr queries except those from the "authorized
 secondaries", I never agree with them (I see some valid cases such as
 a very big zone where xfr queries could be a DoS, but that's an
 exceptional case, not a reason to set the default).

 But now that you're leaving, I'm okay, e.g., with leaving this open
 and deferring it to a separate ticket.

 > > - update_config_data: just checking, is it okay to not to protect it
 > >   by lock?  maybe we should add comment about that.
 >
 > To be honest, I have no idea here. XfrOut hits some kind of mental block
 in my mind when trying to understand what part of code does exactly what,
 what does run in which thread, etc. It wasn't in one before, though.
 >
 > I'm quite sure the update can happen only one at a time, since the CC
 loop runs only one, but it might be possible some other thread reads them
 while being updated. Should we open a ticket to investigate it?

 From a bit closer look at it, it's probably safe as ACL is always
 replaced as a whole and specific sessions have their own copy of the
 entire ACL.

 For longer term, as you indicated I also think the current design and
 implementation of xfrout are mess.  It's a naive mixture of threads
 and events, taking bad sides of these.  So, for a longer term, I'd
 rather redesign it (hopefully via incremental refactoring) than
 "investigating" each of these problems that come from the bad design.
 And, in either case, creating a ticket would be nice.
 >
 > > - 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)
 >
 > Overriding it doesn't really test anything. Such system still has IPv6
 support, I'd like to test it with something that really doesn't have IPv6
 support. Maybe we have a buildbot without IPv6?

 You can at least test the 'else' case (if I read it correctly that
 part isn't tested at all right now).  But I'd leave it to you.

 > And with the addresses for logs, I think it might be nice thing to have,
 but I'd rather like to put it into a different task.

 Fair enough.

 Other points look okay.

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