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      |
-------------------------------------+-------------------------------------
Changes (by stephen):

 * owner:  stephen => jinmei


Comment:

 > Maybe a matter of preference, but I'd use the term "prefix" instead of
 (address+)netmask unless we want to support non contiguous network masks.
 Done.

 > maybe we want to have a keyword "any" (or perhaps "any4"/"any6")
 Good point.  I missed the "match any address from this family".  The
 string constructor now accepts "any4" and "any6".

 > on a related note, I guess we'd probably want to use 0-length prefix to
 indicate an "any" match.
 Done.

 > createNetmask: should this be public? ...
 Renamed to "createMask" (to get rid of the "netmask" connotation) and
 placed in the "internal" namespace (which seemed more to convey the idea
 that this is an internal function than "detail" would).

 > I suspect this should be "w-m < w" (at least the current expression
 doesn't make sense to me):
 It was a typo - fixed.

 > For exception, I'd use \exception markup (that would also be helpful for
 my work-in-progress...
 Done.


 > splitIPAddress: This implementation accepts (e.g.) "/1" or "1/", which
 should be properly rejected with an exception.
 Sigh.  That's what you get when you try to use something that does not
 *exactly* do the thing you're after.  The method now does its own parsing
 and this should be fixed.

 > like createNetmask, should this be public? Moreover, since this isn't
 templated it will result in duplicate symbols.
 Well-noticed.  Moved to ip_check.cc

 > I'd revise the documentation using the term "prefix" (see the general
 comment), e.g.:
 Done.

 > Likewise, I'd rename the parameter "addrmask" to "ipprefix"
 Done.

 > Isn't it better to use the max prefix length instead of the magic number
 of -1?
 No. At the point where that is returned, all that is known is that the
 string passed (presumably) comprises an address and no prefix length.  At
 that point, the maximum valid prefix length is not yet known.  As a value
 of 0 is now being interpreted as a request to match any address of this
 family, -1 is the easiest way of passing back information about the
 absence of a prefix length.  (Note that explicitly specifying a negative
 number as the prefix length will cause an exception to be thrown, so the
 only way -1 can be returned is if no prefix length is given.)

 > setNetmask (which I'd call setPrefixLength...
 I've renamed it to setMask - as it's creating a mask used in a logical
 operation, this seems appropriate.

 > 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.
 I've not come across that before - I guess that this allows compiler some
 optimisation and we may fall foul of that and you
 would need to use a "volatile" keyword to avoid it.  OK, rewritten using a
 vector<uint8_t>.

 > 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_.
 Removed.  A toText() method can be added if we need it, and getPrefixlen()
 now calculates the prefix length on the fly.

 > 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.
 vector<uint8_t> is now used.

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

 > Do we really need the default constructor? It allows an IPCheck object
 to have a mostly invalid state, opening up a possibility of bugs.
 Depends if it is needed.  I added the default constructor to facilitate
 its use in vectors and the like (e.g. initializing a vector of IPCheck
 objects to a known size), but it's trivial to remove it. (Note that
 compare() for an acl constructed with the default constructor will always
 return false for both IPV4 and IPV6 addresses).

 > constructor from string: inet_pton() could fail with -1, too.
 This is only with an invalid address family, but the code now checks for
 the explicit success status.

 > constructor from IPvX numeric addresses: I'd pass in_addr&/in6_addr&
 instead of the lower level primitives.
 Unless we really need them, let's leave it for now.  Its just too much
 effort to modify the tests.

 > copy constructor: couldn't we simply substitute address_ and netmask_?
 i.e., address_ = other.address_, etc. Same for the assignment operator.
 Yes you can.  I'm confusing it with assignment, where array copies are not
 allowed.  However, I've removed the copy constructor and assignment
 operator as they're now not needed.

 > 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)
 getPrefixlen() has been provided.  getNetmask() (now renamed getMask()) is
 retained as it allows inspection of the internal mask, which is helpful in
 testing.

 > getFamily: if we don't need to have the default constructor, maybe we
 don't have to worry about the asserts
 If we remove the default constructor, we'll remove the asserts.  They're
 static asserts, so have no run-time impact.

 > 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).
 Added a "family" argument.

 > match(uint32_t): I'd remove this at least for now.
 Removed.

 > setNetmask: to be very sure, I'd check it doesn't overrun with netmask
 Modified to ensure this.

 One final thing.  At present the address and comparison bitmask are both
 equal to the size of addresses of the relevant family. A possibly
 premature optimization would be to truncate them to the size dictated by
 the prefix length (e.g. is the prefix length were specified as /8, just
 store one byte).  However, this will take a little time to do and you did
 suggest that you are waiting on these changes.

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