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):

 Replying to [comment:21 stephen]:

 First, responding to your response.  (I have no other comments on the
 points I'm not explicitly referring to below, although there can be
 further comments on the resulting code).

 > > 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).

 Apparently the intended usage is to use shared pointers of Check objects
 (see the ACL class merged in the master), so it's less likely to store
 bare Check objects in STL containers in practice.  In general, I'd
 prefer minimalist approach: don't introduce a feature due to a guessed
 (but yet non existent) usage.  But if you still prefer to keeping it,
 I won't oppose to it any more in this review cycle.

 > > constructor from string: inet_pton() could fail with -1, too.
 > This is only with an invalid address family

 I don't think it's guaranteed so (even though it may be the case for
 many actual implementations), but...

 > but the code now checks for the explicit success status.

 ...then that's okay (but see comments about the actual revised
 implementation below).

 > 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.

 Yeah I've noticed this optimization, too.  But whether or not other
 ticket is waiting on the completion of this one, I'd rather defer this
 type of optimization to a separate ticket.

 Next, comments on the revised code.  This time I've also reviewed the
 unit tests.

 '''general'''

 You seem to have intended to apply my editorial suggestion on IPv[46]
 vs IPV[46], but the actual result seems to be different from what I
 actually suggested: my suggestion is to use "IPv4/6" (lower v) in
 comments.  You applied it in the reverse direction.  Is that
 intentional?

 I've also made a few minor changes (mostly editorial, plus constify
 something where it's trivial) and pushed them directly.

 '''acl/Makefile.am'''

 Why did you disable -Werror for libacl?

 '''splitIPAddress (revised)'''

 - prefix_size: I'd personally defer defining a mutable variable until
   we really start using it (in order to effectively limit the lifetime
   of the variable), and, in this sense, I'd move the definition of it
   immediately before this part:
 {{{
     // If there is a prefixlength, attempt to convert it.
     if (!prefixlen.empty()) {
     ...
 }}}
 - so, are we (now?) allowing beginning and ending spaces (including
   '\n')?
 - mod_prefix could (should) be const.
 - prefix could simply be initialized using the default constructor
   (which would be a bit more efficient than constructing string("") and
   coping it)
 - slashpos could (should) be const

 '''ip_check.h'''

 == general ==
 - lexical_cast.hpp and scoped_ptr.hpp should be removed

 == IPCheck ==
 - I know I may sound inconsistent regarding when to worry about
   performance and when to not, but I'm afraid using vectors are
   suboptimal in terms of both comparison performance (because its
   operator[] is slower than direct memory access) and memory footprint
   (due to the overhead of the class structure).  But this only affects
   the internal data structure and we can change this later, so I'd
   propose keeping the current implementation for now, leaving this
   discussion open and creating a separate ticket for it.
 - "any4" and "any6": I'd rather use all 0 address than keeping them
   empty because having variable length data is an easy source of bugs.
   (even though it would be less efficient in terms of memory footprint)
 - constructors: address_() and mask_() could be omitted, but this is
   probably a matter of personal taste.
 - From IPv4 constructor: I don't think it a good idea to use the host
   byte order.  And, on further thought, I'd even wonder whether we
   need these "from address" constructors in the first place for now.
   In the sense of minimalist approach, all we need for the immediate
   purpose is the constructor from the string.  While I'm hesitating to
   suggest this, maybe it'd be smoother if we backed out these
   constructors from this branch for now and defer them to a separate
   ticket.
 - From IPv4 constructor: as far as I can see we don't need static_cast
   here.
 - From string constructor: this pattern doesn't seem to be the most
   common idiom in this case:
 {{{
                 std::copy(address_bytes, address_bytes + IPV6_SIZE,
                           std::back_inserter(address_));
 }}}
   I'd rewrite this to:
 {{{
                 address_.assign(address_bytes, address_bytes + IPV6_SIZE);
 }}}
   Same comment applies to the IPv4 case.
 - From string constructor: technically this is not 100% correct:
 {{{
                     isc_throw(isc::InvalidParameter, "address prefix of "
 <<
                               ipprefix << " is a not valid");
 }}}
   (oh btw, the what message looks like a typo in "is a not valid")
   because if inet_pton fails with -1 it means a system error, not
   necessarily due to the input was invalid.  Likewise, this is also
   not entire correct:
 {{{
             } else {
                 // Not IPV6, try IPV4
 }}}
   when status == -1.  To be 100% pedantic we should do something like
   the attached diff.
 - getPrefixlen(): even though it's legal, I'd avoid shadowing the same
   name of variable 'i'.
 - getPrefixlen(): this for loop can make meaningless iterations once
   the mask byte is 0.
 - getPrefixlen(): the condition seems to be wrong.  It should be "i < 8":
 {{{
                 for (int i = 0; i < 8 * sizeof(uint8_t); ++i) {
 }}}
 - compare(): (should have noticed this before) shouldn't this be non
   virtual?
 - compare(): personally I'd list 'family' first as it seems to be more
   significant, but that's purely a matter of taste, so this is just a
   comment.

 '''ip_check_unittest'''

 - As far as I understand it, this is not correct:
 {{{
     // Check on all possible 8-bit values.  Use a signed type to generate
 a
     // variable with the most significant bits set (right-shifting will
     // introduce additional bits).
 }}}
   The behavior of >> for signed integer is implementation dependent
   (at least it is so for plain C).  To be very safe, I'd use a
   uint16_t initialized with 0xff00 with >>.  This way we could also
   integrate the case of i = 0 in the for loop.

 - Same for the 32-bit version, but I now guess we need this in the
   first place, both for the main code and test (and whether createMask
   needs to be templated) because we don't use the 32-bit version.

 - for comparison tests, I'd also use prefix lengths that are not for a
   4 or 8-bit boundary, e.g. /19, /45, /127, etc.

 - MixedMode test: I don't see a point in doing EXPECT_NO_THROW
   before EXPECT_FALSE for the same expression.  I'd either remove the
   former or change it to ASSERT_NO_THROW.

 - MixedMode test: for a more meaningful test, I'd use data that would
   confuse the compare method without the explicit family check,
   e.g. an IPv4 address to be tested and IPv6 prefix in the check whose
   first 32 bits are identical to the IPv4 address.

 '''Others'''
 These are basically random thoughts on future possible extensions.  I
 wouldn't request them be incorporated to the current branch (actually
 I'd rather defer it).

 - We might want to "normalize" the address, that is, apply the mask to
   the address.  For example, if we're given "192.0.2.1/24", internally
   convert it to "192.0.2.0/24".
 - maybe we want to implement specialized toText() for IPCheck..
 - for IPv6, we might want to support link-local addresses with scope
   consideration, i.e, differentiate fe80::1%ether0 and fe80::1%ether1.
   although not very advisable, there are actually people trying to use
   DNS over IPv6 link-local addresses.

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