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

 Hello

 Replying to [comment:5 jinmei]:
 > 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.

 This will come with some technical issues, because the keyring is created
 every time it is loaded, but generally I agree that something like this is
 a good idea.

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

 I don't want to worry about views, at last not now. The code is not ready
 for views at all yet and we didn't decide how we'll handle them.

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

 Thanks. I forgot to remove the TODO when I discovered they are not counted
 in the count RRs.

 >   I'd either say "tsig != NULL" or ASSERT_NE(static_cast<void*>(NULL),
 tsg).
 >   (unfortunately static_cast is necessary for some deviant compilers)

 OK.

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

 Bad copy-paste when writing the tests. I accidentally switched them, it
 should be correct now.

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

 That's why the tests didn't fail when I switched them. Thanks for
 clarification, this is not needed then. I assumed it is returned in the
 extended RCODE.

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

 You mean like another test for it, right?

 So, how is it now? Should I create a follow-up ticket for the cleanups
 after the merge and say it's still subtask of #875?

 Thanks

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