BIND 10 #998: IP based ACL check

[BIND 10 Development]

#998: IP based ACL check
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110628
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5.0
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 (continued)

 I've completed my review on ip_check.h.  I've not fully reviewed the
 tests yet, but I guess it's better to give the ticket back to Stephen
 at this point.

 == splitIPAddress ==
 - This implementation accepts (e.g.) "/1" or "1/", which should be
   properly rejected with an exception.
 - like createNetmask, should this be public?  Moreover, since this
   isn't templated it will result in duplicate symbols.
 - I'd revise the documentation using the term "prefix" (see the
   general comment), e.g.:
 {{{
 /// Splits an IP prefix (given in the form of "xxxxxx/n" or "xxxxx") into
 a
 /// string representing its address part and the prefix length in bits.
 /// An exception...
 }}}
   "(In the latter case...)" was removed because it's obvious from the
   definition of the prefix length.
 - Likewise, I'd rename the parameter "addrmask" to "ipprefix"
 - Isn't it better to use the max prefix length instead of the magic
   number of -1?  Then we can eliminate the special case consideration
   for negative 'requested' in setNetmask (which I'd call
   setPrefixLength), and we could even make the parameter unsigned int.

 == IPCheck class ==
 - The union usage for AddressData seems dangerous in that it breaks
   the basic rule of union: don't read a member that's different from a
   member that was last written.  Especially in a bit more advanced
   languages like C++ we should generally be able to find a better way
   than doing this type of game using union.  Also, the use of uint32_t
   version basically seems to be for performance optimization.  If
   that's the main purpose, it seems to me premature.  We should
   consider optimization after we are sure this is a heavy bottleneck,
   and after we know a specific optimization really improves the
   performance significantly, especially because it uses a dangerous
   hack like tweaking union.  Actually, I'd expect this will eventually
   be a major optimization point considering some operators use very
   large ACLs, but at that point we'll probably need more aggressive
   reconstruction such as introducing a radix/trie, which would make
   this type of micro optimization moot.

 - at the risk of sounding self contradicting, I'd be a bit concerned
   about the redundant member variables like masksize_ (which I'd call
   prefixlen_) and straddr_.  These can be generated on demand from
   other members, and, as commented, these wouldn't be needed in
   performance sensitive path.  On the other hand, we may have a very
   large number of ACL entries, so the contribution of the additional
   redundant member might be significant in terms of memory usage.

 - Likewise, I wonder whether we might want to allocate spaces for
   address and mask dynamically, and only for the necessary amount
   depending on the address family.

 - as we already discussed, I'd suggest removing "inverse".

 - Do we really need the default constructor?  It allows an IPCheck
   oject to have a mostly invalid state, opening up a possibility of
   bugs.

 - constructor from string: inet_pton() could fail with -1, too.

 - constructor from IPvX numeric addresses: I'd pass in_addr&/in6_addr&
   instead of the lower level primitives.

 - copy constructor: couldn't we simply substitute address_ and
   netmask_? i.e., address_ = other.address_, etc.  Same for the
   assignment operator.

 - getNetmask(): I'd instead define a method returning a prefix length
   (int) because conceptually this class works with prefix lengths;
   netmasks are just internal representation (probably for faster
   matching)

 - getFamily: if we don't need to have the default constructor, maybe
   we don't have to worry about the asserts (IMO this is another reason
   why it would be better to not have it if we don't have a meaningful
   way to use it).

 - match(const uint8_t*): I'd like this method to support multiple
   families of addresses (and simply return false if address families
   don't match) either by having an additional "family" parameter or by
   taking an IPAddress structure as I defined in trac999preview branch
   (which is now in the central repo for reference).  Otherwise, each
   specialization of matches() needs to deal with the address family
   mismatches (as I did in trac999review).

 - match(uint32_t): I'd remove this at least for now.  If it's for
   optimization, it seems to me to be premature.  Also, it's not really
   well defined when constructed for an IPv6 prefix.

 - setNetmask: to be very sure, I'd check it doesn't overrun with
   netmask (or we might want to use something like boost::array and use
   at() instead []).

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