BIND 10 #2027: ddns: move acl up

[BIND 10 Development]

#2027: ddns: move acl up
-------------------------------------+-------------------------------------
                   Reporter:  jelte  |                 Owner:  jinmei
                       Type:         |                Status:  reviewing
  enhancement                        |             Milestone:
                   Priority:         |  Sprint-20120619
  medium                             |            Resolution:
                  Component:  DDNS   |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  3
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 The code and test look okay.

 But I have comments on the rationale comment:
 {{{#!python
             # Contrary to what RFC2136 specifies, we do ACL checks before
             # prerequisites. Following the spec, information could leak,
             # and we decided not to do so (as do other implementations)
 }}}

 First, "as do other implementations" is a bit ambiguous to me.  Does
 this mean "other implementations do follow the check but we decided
 not to do so", or "other implementations do not follow the check and
 we decided not to do so either"?  I guess the intent is the former,
 and maybe grammar-wise that can be the only valid interpretation, but
 at least to me it was not super clear.

 Secondly, in my understanding the information leak is not the only
 reason why the RFC2136 ordering is bad.  My understanding is that it
 primarily simply doesn't make sense in that ACL should generally be
 checked as soon as possible, and in practice we have had other
 security issues (in BIND 9) that could have been more easily worked
 around if the ACL check could take place before prerequisite check.

 And, as an related point to the above two points, BIND 9 actually
 prevents information leak by checking the general "query ACL" before
 prerequisite check (but it still checks the update ACL after
 prerequisite handling).  So, if this comment means "other
 implementations follow the spec but we won't because it would leak
 information", it doesn't make perfect sense because "other
 implementations actually wouldn't be considered to follow the spec if
 it's mainly about information leak (btw what are "other
 implementations"?  Obviously it should include BIND 9, but are there
 others?  I think Nominum's auth server supports ddns and I guess it
 actually follows the spec on this point, but I'm not sure).

 So, if we want to be super precise (at least in terms of my
 understanding), why we reverse the order is:

 - there's a general consensus that what RFC literally says doesn't
   make sense
 - doing the ACL check after prerequisite can cause various troubles in
   practice, such as information leak or making the defense more
   difficult if and when a vulnerability is found in the prerequisite
   check.
 - while BIND 9 prevents the information leak by introducing two sets
   of ACLs, it still has other problems, and so just avoiding leak this
   way wouldn't be worth the complexity of maintaining and performing
   two sets of ACLs.

 The code comment won't have to be in this detail, but if I were to
 write it, I'd say something like:

 {{{#!python
             # Contrary to what RFC2136 specifies, we do ACL checks before
             # prerequisites. It's now generally considered to be a bad
             # idea, and actually does harm such as information
             # leak. It should make more sense to prevent any security
 issues
             # by performing ACL check as early as possible.
 }}}

 (this example comment doesn't mention "other implementations".  It's
 not necessarily because I don't think we have to, but because I was
 not sure how we mention it).

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