BIND 10 #2066: general description on ACL in bind10 guide

[BIND 10 Development]

#2066: general description on ACL in bind10 guide
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:         |             Milestone:
  defect                             |  Sprint-20120821
                   Priority:         |            Resolution:
  medium                             |             Sensitive:  0
                  Component:         |           Sub-Project:  Core
  documentation                      |  Estimated Difficulty:  4
                   Keywords:         |           Total Hours:  0
            Defect Severity:  N/A    |
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:8 vorner]:

 First off, considering this:

 > Also, I'm going to the vacations tomorrow (my) afternoon, so unless
 there's
 > only very little to do, I can't finish the ticket now. Maybe someone can
 take
 > it over?

 I basically suggest we merge the update and close the ticket as soon
 as obvious errors are fixed.

 I made a couple of minor editorial fixes.

 The only other thing that can block the move seems to be this one:

 > However, I noticed other thing. It rejected the example with a new
 error,
 > complaining that something is not a string and I found out it doesn't
 like the
 > syntax with listing multiple possible values in a list. I believe that
 thing is
 > a bug and possibly easy to fix (I guess there's false returned from
 somewhere
 > instead of true in the definition of the IP check). OK to fix it, or
 should we
 > just scratch the part about the lists?

 I don't know specifically which case you are referring to, but based
 on the lack of time my suggestion is to defer the fix to a separate
 ticket, and either
 - disable (remove or comment-out) the relevant part of the doc, or
 - keep the doc but add a note that it currently doesn't work,
   referring to the corresponding bug-fix ticket number.

 Other points below can be left open or deferred to a separate ticket.

 > > - As noted in a commented-out "TODO", I think we need some description
 > >   of keyring and a reference to it.
 >
 > Yes. But I believe this should go to a separate ticket.

 Then please open one.

 > I think I made this note irrelevant in one of the commits. The specs
 > were missing default values for the items, which was wrong, so I
 > fixed that instead of returning the note. Also, when #2184
 > (currently in review) is merged, the ACLs will act mostly sanely (so
 > you will be able to do config add followed by config set action and
 > config set address).

 If you confirmed the examples actually work, I'm okay with this.

 But this makes me wonder about some other things regarding the default
 ACL.  You now explicitly specify the default ACL for each module.
 This is probably a good practice, but some modules hardcode the
 default separately from the config (either applying the implicit
 default of the ACL library or really hardcoding the default rule in
 the source code).  If these two mismatch some confusion will happen
 (which could even be a security issue).  If we want to rely on the
 default ACL in the config, we should make sure that is really used as
 the default for the corresponding module.  But this will be a subject
 of a separate ticket - as far as I can see they are consistent right
 now.

 A couple of other points:

 - This sentence is still not very understandable to me:
   'You can configure it in the same way as any ACL (Section 8.1,
   "ACLs")'.  I think I can now understand what it tries to say after
   this discussion, but if this is my first time to read it I'm not
   sure if I can smoothly understand it.  My suggestion is to move the
   line after "Both b10-xfrout and b10-auth...sign responses", and
   rephrase it to: 'For further details on ACL configuration, see
   Section 8.1, "ACLs".'

 - The resolver example in Section 13.1 is not very understandable to
   me for several reasons.
   - The phrase of 'queries on the "192.168.1.1" interface' is
     confusing in that corresponding rule is about the source
   - related to this, the match expression "192.168.1.1/24" looks
     awkward because "/24" effectively makes the last "1" meaningless.
     Maybe the intent is that the server has 192.168.1.1 on some of its
     interfaces, and its subnet prefix is /24, and the rule is to try
     to accept queries only from that subnet.  If so, it's far from
     obvious just from the given description and the configuration.
   - a minor point, but I'd like to use 192.0.2.0/24 for document
     purposes unless we use others for a specific reason (which I don't
     see in this particular case)
   - it's not really clear what "these commands (could be issued)"
     refers to due to the additional sentence between this one and the
     example.
   - "the third rule" is ambiguous.  Is it acl[2] or acl[3]?
   - it was not very clear to me what "not needed" means in "ast final
     rule is not needed".
   I suggest revising it to:
 {{{
 The following session is an example of extending the ACL to also
 allow queries from 192.0.2.0/24:

 <example, replacing 192.168.1.1/24 with 192.0.2.0/24>

 Note that we didn't set the value of the last final rule
 (query_acl[3]) -- in the case of resolver, rejecting all queries is
 the default value of a new rule.  In fact, this rule can even be
 omitted completely, as the default, when a query falls off the list,
 is rejection.
 }}}

 I'd leave it to you whether to include these suggestions.

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