BIND 10 #998: IP based ACL check

[BIND 10 Development]

#998: IP based ACL check
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  UnAssigned
  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):

 I've taken a bit more closer look at the branch, mainly for #999.

 I have some design level question/comment/suggestion.

 '''Observations'''

 First, I don't see much benefit in having both Ipv4Check and Ipv6Check
 as derived classes of IpCheck.  IpCheck is a concrete class that
 internally creates and holds IpvXCheck.  This way, applications can
 (and I guess will) only depend on IpCheck, not only when using the
 class (as in the usual usage of polymorphism) but also when creating
 it.  So IpvXCheck doesn't seem to have to be a derived class of
 IpCheck (or even a derived class of Check).

 Second, regardless of whether they are derived from IpCheck, I'm not
 so sure if it makes sense to separate classes for IPv4 and IPv6.
 Essentially they do the same thing: dealing with some binary data with
 a fixed maximum size and a variable-length effective size (in bits),
 and matching given data of the maximum size against its internal data
 (up to the effective size).  So, most of the code logic should be
 sharable, and it seems we can implement everything in a single class
 with an identifier of the address family.

 Of course, separating classes (with polymorphism) has its own
 advantages: We'll be able to benefit from stronger type checks; the
 size of the objects may be smaller (because the address family is
 embedded in the type), which can be a big plus if/when we want to a
 large number of ACL rules.  In that case, however, I'd suggest
 combining the main code logic as much as possible either via C++
 template or some design pattern techniques like template methods.

 Third: maybe this is premature at this point, but I guess Ip(vX)Check
 will need to contain the information of "which address it is", such as
 the source address or destination address.  For example, consider the
 following ACL entry:
 {{{
   allow from 2001:db8:1::/64 to 2001:db8:2::/64
 }}}
 In this case, the "Context" would be something like a "Packet" class,
 that stores the source and destination address information.  Both the
 "from 2001:db8:1::/64" and "to 2001:db8:2::/64" would be represented
 as separate Ip(v6)Check objects, and the former/latter would have to
 know it's a source/destination address; otherwise the Context cannot
 determine which address it should extract to perform the match.

 Finally: I suspect methods like "compare()" would need to take
 something more generic, at least some opaque data and the address
 family identifier.  For example, consider these rules:
 {{{
   allow from 2001:db8:1::/64 to 2001:db8:2::/64
   allow from 192.0.2.0/24 to 192.0.2.53
 }}}

 And assume that the "Packet" class is IP-version independent (just
 like our IOMessage class).  Since we'd like to keep things version
 independently as much as possible, IpCheck<Packet>::matches() would
 like to do something like this:
 {{{
     if (getAddressType() == IpCheck::SOURCE) { // see the third point
        return (compare(packet.getSourceAddr()));
    } else {
        return (compare(packet.getDestinationAddr()));
    }
 }}}

 Now, the address family of getSourceAddr() may be different from the
 IP version for this IpCheck instance, and in that case we'd be
 expected to return "false".  So, we'd like compare() to take various
 types of addresses, and return false if the family doesn't match.

 In my experimental attempt, I'm thinking about something like this:
 {{{
 struct IPAddress {
     IPAddress(const struct sockaddr& sa);
     const int family;
     const uint8_t* const data;
     const size_t length;
 }
 }}}
 with this the compare() method would take "const IPAddress&".  But
 this is just an example structure.  I'm not pushing it if there's a
 better way to implement the concept.

 '''Suggestion'''

 Considering all these, I'd suggest

 - Either make IpCheck a single "for all IP versions" check class, or
   to make IpvXCheck direct subclass of Check.  In the latter case
   IpCheck will be removed, some factory function will be introduced,
   and employ some code sharing technique (such as templates).
 - extend the compare() method (or something equivalent to that if
   addressing the first point affects this part of interface) so that
   it can take different types of addresses
 - (maybe optionally) add an interface to Ip(vX)Check to maintain and
   return the type of addresses.  The "address type" may better be
   templated (like the "Action" type of the ACL class).

 (If these suggestions are vague, I can show it via sample code)

 '''Other comments'''

 Here are some code level comments I happened to notice.

 - I'd say "IP" instead of "Ip" because "IP" is an acronym
 - I'd use unnamed namespace wherever possible in the test, and avoid
   using file-scope "static'
 - can't compare() be private?
 - "getNetmask(): const uint32_t" should be uint32_t.  The const is
   redundant, and at least clang++ actually treats it a fatal warning.

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