BIND 10 #1104: support TSIG in DNS (Request) ACL

[BIND 10 Development]

#1104: support TSIG in DNS (Request) ACL
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  jinmei                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110802
                  Component:         |            Resolution:
  xfrout                             |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  5
Feature Depending on Ticket:         |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 Replying to [comment:12 stephen]:
 > > ...one possible solution is to enhance the TSIGRecord class so that it
 can store a state indicating the record has been verified, and have
 TSIGContext::verify() change the state to "verified" on success (to make
 it possible we need to stop constifying the 'record' parameter,
 > though).
 > As part of a solution to avoid avoid multiple verifications of the same
 record, that would work.  To avoid the overhead involved with
 "unconstifying" TSIGContext, we could make the internal "verified" flag
 mutable.

 Firs off: in case I was not clear, what I suggested is to introduce a
 "verified" flag to the TSIGRecord class, not TSIGContext.  And as a
 result we'll have to change from:
 {{{
 TSIGError
 TSIGContext::verify(const TSIGRecord* const record, const void* const
 data,
                     const size_t data_len)
 }}}
 to:
 {{{
 TSIGError
 TSIGContext::verify(TSIGRecord* const record, const void* const data,
                     const size_t data_len)
 }}}
 (where the "verified" flag of 'record' may be modified).

 I don't think it a good idea to deceive the compiler with mutable in
 this case.  When we set the "verified" flag of TSIGRecord, it actually
 changes externally observable behavior.  IMO hiding the fact with
 mutable goes beyond the acceptable usage of it.

 Another approach would be to pass TSIGContext instead of TSIGRecord to
 RequestContext.  TSIGContext (internally) contains information of the
 result of the latest verification and the associated TSIG key, so we
 could expose these to RequestContext as read only attributes.  One
 possible drawback is that this approach would be a bit more error
 prone than the approach of passing TSIGRecord.  For example, an
 extremely buggy implementation may reuse a TSIGContext that has
 validated a signature multiple times for different ACL checks.  Such a
 bug can happen with TSIGRecord, too, but it will be less likely if the
 expected usage is to pass the result of Message::getTSIGRecord() to
 the RequestContext constructor (then the application never holds a
 reference to the TSIG record itself).

 > > Then we can change !RequestContext so that it will ignore the given
 TSIG record unless its state is "verified".
 > I'm not sure what you meant here.  If an ACL requires a check on a TSIG
 record, that check needs to be carried out; you can't ignore the record
 just because it has not been verified.

 For the purpose of ACL we only use the TSIG key name (at least in the
 current implementation, and as in BIND 9 and NSD).  So the proposed
 logic would be to change this:
 {{{
 template<>
 bool
 NameCheck<RequestContext>::matches(const RequestContext& request) const {
     return (request.tsig != NULL && request.tsig->getName() == name_);
 }
 }}}
 to this:
 {{{
 template<>
 bool
 NameCheck<RequestContext>::matches(const RequestContext& request) const {
     return (request.tsig != NULL && request.tsig->isVerified() &&
             request.tsig->getName() == name_);
 }
 }}}

 > Using your idea, I was more envisaging TSIGContext::verify() being
 called during the ACL checking procedure (and in other places), performing
 the verification if called for the first time and returning the stored
 verification state on subsequent calls.

 I was aware of that possibility, but didn't think it the best away to
 handle this issue.  IMO, if an application wants to handle TSIG, it
 should explicitly do it at an appropriate position of the code.  On
 one hand it may look convenient if we merge TSIG verification in ACL
 checks (in addition to doing verification separately), but it will
 make the ACL module have tighter coupling with DNS specific logic,
 which will subsequently lead to reducing clarity.  It would also
 require that RequestContext be modifiable in matches(), which is
 against an assumption of the method.  Further, deeper inspection
 before performing TSIG verification could be even dangerous, because
 some of the checks may depend on the integrity of the message content
 (e.g., whether a particular header flag is on/off) but we cannot rely
 on the integrity until we perform the signature.

 I still tend to agree on providing a safety net for naive/buggy
 implementations that don't correctly perform TSIG check but still
 do pass a TSIG record to RequestContext (with which, as you pointed
 out, an ACL with TSIG key would cause a confusing/insecure result).
 But I believe it should be a minimal guard that is sufficient to
 prevent the harmful result, and shouldn't try to be too smart.

 Now, as this point seems to be not so trivial, my current suggestion
 is to create a new ticket as an open enhancement, referring to this
 discussion, and close this one.  Would that be okay for you?  (If you
 like it I don't mind with keeping this ticket open instead of creating
 a new one).

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