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:

 > First, I don't see much benefit in having both Ipv4Check and Ipv6Check
 as derived classes of !IpCheck?...
 Which version of the code did you review? !IpCheck, Ipv4Check and
 Ipv6Check were all derived from 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 ...
 That's a very valid point and, on reflection, I agree with you.  I have
 rewritten the code to put everything in one class.

 > 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.
 The class as implemented here is only a single ACL match (or non-match -
 the capability to check for a non-match was trivial, which is why I added
 it to this class).  The ACL you are suggesting in the example is really a
 combined ACL:
 {{{
 allow (source matches 2001:db8:1::/64) AND (dest matches 2001:db8:2::/64)
 }}}
 I would see then implementation as being a class containing two IPCheck
 classes, one for source and one for destination. It would be the container
 class that knows the structure of the packet and distinguishes between
 source and destination address.

 However, consider the possible ACL:
 {{{
 allow (source matches 2001:db8:1::/64) AND (protocol is TCP)
 }}}
 (I don't know whether we would want to allow access based on protocol, but
 as it is a possibility so I think we should consider it.)  In this case a
 more general class allowing the AND between two ACLs is called for.
 Applying this to the IP check problem, an alternative approach to a single
 class checking both source and destination addresses could be to derive
 SourceIPCheck and DestinationIPCheck classes from IPCheck and combine them
 using the AND operator.  (All the derived classes would do is provide a
 matches() method to pass the source/destination IP address from the packet
 to the parent IPCheck::matches())

 > 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...
 The entry into compare() is through matches(), which is currently
 unspecified until it is known what structure is going to be used as
 "Context".  (The idea is that once we know, a template specialisation can
 be written.)

 > 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.
 Whoops!  I missed that.  Added.

 > 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).
 The new IPCheck is now a single class.

 > 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
 Let's discuss this further.  As noted above, I think that this can be
 implemented by the planned logical operator ACLs.

 > (maybe optionally) add an interface to Ip(vX)Check to maintain and
 return the type of addresses.
 Done.

 > I'd say "IP" instead of "Ip" because "IP" is an acronym
 I used Ip to avoid a run of capitals, but I'm not dogmatic about it and
 have renamed the class IPCheck.

 > I'd use unnamed namespace wherever possible in the test, and avoid using
 file-scope "static'
 Done.

 > can't compare() be private?
 An oversight - it is now.

 > "getNetmask(): const uint32_t" should be uint32_t. The const is
 redundant, and at least clang++ actually treats it a fatal warning.
 Treating all addresses as arrays, this now returns a vector.

 Thanks for the comprehensive review.

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