BIND 10 #931: Implement signing part in b10-auth

[BIND 10 Development]

#931: Implement signing part in b10-auth
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  jinmei
  vorner                             |                Status:  reviewing
                       Type:  task   |             Milestone:
                   Priority:  major  |  Sprint-20110531
                  Component:         |            Resolution:
  Unclassified                       |             Sensitive:  0
                   Keywords:         |           Sub-Project:  DNS
            Defect Severity:  N/A    |  Estimated Difficulty:  0.0
Feature Depending on Ticket:  tsig   |           Total Hours:  0
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by jinmei):

 (The following are comments for changes in auth, i.e., the main part
 of the branch)

 First, I've made some minor changes and pushed them to the repo.

 '''Higher level comment'''

 (I don't request it to be addressed within this ticket, but would like
 to discuss it separately.)

 I personally don't like to refer to isc::server_common::keyring (a
 global variable) so directly in various places of code. (specifically
 in this branch, both in test and auth_srv.cc).  This way we need to
 have the knowledge that the auth server internally refers to the
 global variable.

 Also, due to the dependency on the global variable, we need to be
 careful in clearing the keyring in each test:
 {{{
     isc::server_common::keyring.reset(new TSIGKeyRing);
     isc::server_common::keyring->add(key);
     server.processMessage(*io_message, parse_message, response_obuffer,
                           &dnsserv);
     isc::server_common::keyring.reset();
 }}}
 so that we won't accidentally leave temporary key in the global
 storage.

 I think we should have one level abstraction like:
  - add an explicit interface to install a keyring to a server, say,
    AuthServ::setTSIGKeyring()
  - In auth/main.cc we use this interface to specify the global key ring
  - In tests, the test fixture creates its own keyring and sets its in
    the server
  - within server, it always refers to the keyring given via
    setTSIGKeyring(), whether it's a global one or some local data.

 Also, eventually, we'll probably want to have multiple key rings (per
 view, etc), so a single initKeyring() may not suffice.  but for now,
 I'm okay with the current design.

 '''auth_srv.cc'''

 Other than the higher level point, it looks okay.

 '''auth_srv_unittest.cc'''

 === AuthSrvTest::TSIGSigned ===

 - I'm not sure what this "TODO" means, but in this case we wouldn't
   expect it to be counted.  getRRCount() doesn't count meta RRs (by
   design).
 {{{
     // TODO Is the TSIG counted here?
     headerCheck(*parse_message, default_qid, Rcode::NOERROR(),
                 opcode.getCode(), QR_FLAG | AA_FLAG, 1, 1, 1, 0);
 }}}
 - I'd avoid implicit conversion from a pointer to bool:
 {{{
     ASSERT_TRUE(tsig) << "Missing TSIG signature";
 }}}
   I'd either say "tsig != NULL" or ASSERT_NE(static_cast<void*>(NULL),
 tsg).
   (unfortunately static_cast is necessary for some deviant compilers)

 === AuthSrvTest::TSIGSignedNoKey ===

   The comment and the test name don't seem to match the actual test
   code.  It seems to test a badsig case.  BTW, assuming you meant
   TSIGBadSig, I'd suggest:
   - Change the 3rd arg to headerCheck to
     TSIGError::BAD_SIG().toRcode() even if the result is the same
 {{{
     headerCheck(*parse_message, default_qid,
 TSIGError::BAD_KEY().toRcode(),
                 opcode.getCode(), QR_FLAG, 1, 0, 0, 0);
 }}}
  - and, add a more explicit test about the TSIG error code:
 {{{
     EXPECT_EQ(TSIGError::BAD_KEY_CODE, tsig->getRdata().getError());
 }}}

 === AuthSrvTest::TSIGBadSig ===

 same mismatch for TSIGBadSig.  same sense of comments apply.

 === Both for TSIGSignedNoKey and TSIGBadSig ===

 Regardless of the previous point: I don't understand this:
 {{{
     // Make sure we get the extended error code as well
     request_message.setEDNS(ConstEDNSPtr(new EDNS));
     createRequestPacket(request_message, IPPROTO_UDP, &context);
 }}}
   For what do you want to get the extended rcode?  If you mean TSIG
   error code, it's directly coded in the TSIG RR, not in the form of
   the extended RCODE.

 === Other ===

 I'd check if TSIG is checked before anything else.  For example, give
 a request of an unsupported query with a bad sig, and confirm we have
 a bad sig (+ NOTAUTH) response rather than NOTIMP.  But the current
 code looks correct on this point, so (considering we're running out of
 time) I'm okay with deferring it to a later ticket.

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