[trac893] overall document updates

src/lib/dns/tests/tsig_unittest.cc

@@ -171,7 +171,7 @@ TSIGTest::createMessageAndSign(uint16_t id, const Name& qname,
 
     TSIGContext::State expected_new_state =
         (ctx->getState() == TSIGContext::INIT) ?
-        TSIGContext::WAIT_RESPONSE : TSIGContext::SENT_RESPONSE;
+        TSIGContext::SENT_REQUEST : TSIGContext::SENT_RESPONSE;
     ConstTSIGRecordPtr tsig = ctx->sign(id, renderer.getData(),
                                         renderer.getLength());
     EXPECT_EQ(expected_new_state, ctx->getState());
@@ -729,7 +729,7 @@ TEST_F(TSIGTest, badkeyForResponse) {
         SCOPED_TRACE("Verify a response resulting in BADKEY");
         commonVerifyChecks(*tsig_ctx, &dummy_record, &dummy_data[0],
                            dummy_data.size(), TSIGError::BAD_KEY(),
-                           TSIGContext::WAIT_RESPONSE);
+                           TSIGContext::SENT_REQUEST);
     }
 
     // A similar case with a different algorithm
@@ -742,7 +742,7 @@ TEST_F(TSIGTest, badkeyForResponse) {
         SCOPED_TRACE("Verify a response resulting in BADKEY due to bad alg");
         commonVerifyChecks(*tsig_ctx, &dummy_record2, &dummy_data[0],
                            dummy_data.size(), TSIGError::BAD_KEY(),
-                           TSIGContext::WAIT_RESPONSE);
+                           TSIGContext::SENT_REQUEST);
     }
 }
 
@@ -760,7 +760,7 @@ TEST_F(TSIGTest, badsigThenValidate) {
         SCOPED_TRACE("Verify a response that should fail due to BADSIG");
         commonVerifyChecks(*tsig_ctx, message.getTSIGRecord(),
                            &received_data[0], received_data.size(),
-                           TSIGError::BAD_SIG(), TSIGContext::WAIT_RESPONSE);
+                           TSIGError::BAD_SIG(), TSIGContext::SENT_REQUEST);
     }
 
     createMessageFromFile("tsig_verify5.wire");
@@ -784,7 +784,7 @@ TEST_F(TSIGTest, nosigThenValidate) {
         SCOPED_TRACE("Verify a response without TSIG that should exist");
         commonVerifyChecks(*tsig_ctx, NULL, &dummy_data[0],
                            dummy_data.size(), TSIGError::FORMERR(),
-                           TSIGContext::WAIT_RESPONSE);
+                           TSIGContext::SENT_REQUEST);
     }
 
     createMessageFromFile("tsig_verify5.wire");
@@ -810,7 +810,7 @@ TEST_F(TSIGTest, badtimeThenValidate) {
         SCOPED_TRACE("Verify resulting in BADTIME due to expired SIG");
         commonVerifyChecks(*tsig_ctx, tsig.get(), &dummy_data[0],
                            dummy_data.size(), TSIGError::BAD_TIME(),
-                           TSIGContext::WAIT_RESPONSE);
+                           TSIGContext::SENT_REQUEST);
     }
 
     // revert the clock again.

src/lib/dns/tsig.cc

@@ -16,7 +16,6 @@
 
 #include <stdint.h>
 
-#include <cassert>              // for the tentative verifyTentative()
 #include <vector>
 
 #include <boost/shared_ptr.hpp>
@@ -62,12 +61,18 @@ struct TSIGContext::TSIGContextImpl {
         state_(INIT), key_(key), error_(Rcode::NOERROR()),
         previous_timesigned_(0)
     {}
+
+    // This helper method is used from verify().  It's expected to be called
+    // just before verify() returns.  It updates internal state based on
+    // the verification result and return the TSIGError to be returned to
+    // the caller of verify(), so that verify() can call this method within
+    // its 'return' statement.
     TSIGError postVerifyUpdate(TSIGError error, const void* digest,
                                size_t digest_len)
     {
         if (state_ == INIT) {
             state_ = RECEIVED_REQUEST;
-        } else if (state_ == WAIT_RESPONSE && error == TSIGError::NOERROR()) {
+        } else if (state_ == SENT_REQUEST && error == TSIGError::NOERROR()) {
             state_ = VERIFIED_RESPONSE;
         }
         if (digest != NULL) {
@@ -78,6 +83,17 @@ struct TSIGContext::TSIGContextImpl {
         error_ = error;
         return (error);
     }
+
+    // The following three are helper methods to compute the digest for
+    // TSIG sign/verify in order to unify the common code logic for sign()
+    // and verify() and to keep these callers concise.
+    // All methods take OutputBuffer as a local work space, which will be
+    // cleared at the beginning of the methods (and the resulting content
+    // of the buffer is not expected to be used by the caller), so it could
+    // be instantiated inside the method.  We reuse the same instance just
+    // for efficiency reasons.
+    // The methods also take an HMAC object, which will be updated with the
+    // calculated digest.
     void digestPreviousMAC(OutputBuffer& buffer, HMACPtr hmac) const;
     void digestTSIGVariables(OutputBuffer& buffer, HMACPtr hmac,
                              uint16_t rrclass, uint32_t rrttl,
@@ -139,16 +155,20 @@ TSIGContext::TSIGContextImpl::digestTSIGVariables(
     }
 }
 
+// In digestDNSMessage, we exploit some minimum knowledge of DNS message
+// format:
+// - the header section has a fixed length of 12 octets (MESSAGE_HEADER_LEN)
+// - the offset in the header section to the ID field is 0
+// - the offset in the header section to the ARCOUNT field is 10 (and the field
+//   length is 2 octets)
+// We could construct a separate Message object from the given data, adjust
+// fields via the Message interfaces and then render it back to a separate
+// buffer, but that would be overkilling.  The DNS message header has a
+// fixed length and necessary modifications are quite straightforward, so
+// we do the job using lower level interfaces.
 namespace {
-// We exploit some minimum knowledge of DNS message format:
-// the header section has a fixed length of 12 octets
-// the offset in the header section to the ID field is 0 (and the field length
-// is 2 octets)
-// the offset in the header section to the ARCOUNT field is 10 (and the field
-// length is 2 octets)
 const size_t MESSAGE_HEADER_LEN = 12;
 }
-
 void
 TSIGContext::TSIGContextImpl::digestDNSMessage(OutputBuffer& buffer,
                                                HMACPtr hmac,
@@ -168,9 +188,7 @@ TSIGContext::TSIGContextImpl::digestDNSMessage(OutputBuffer& buffer,
 
     // Install the adjusted ARCOUNT (we don't care even if the value is bogus
     // and it underflows; it would simply result in verification failure)
-    InputBuffer b(msgptr, sizeof(uint16_t));
-    const uint16_t arcount = b.readUint16();
-    buffer.writeUint16(arcount - 1);
+    buffer.writeUint16(InputBuffer(msgptr, sizeof(uint16_t)).readUint16() - 1);
     msgptr += 2;
 
     // Digest the header and the rest of the DNS message
@@ -299,7 +317,7 @@ TSIGContext::sign(const uint16_t qid, const void* const data,
                                           otherdata)));
     // Exception free from now on.
     impl_->previous_digest_.swap(digest);
-    impl_->state_ = (impl_->state_ == INIT) ? WAIT_RESPONSE : SENT_RESPONSE;
+    impl_->state_ = (impl_->state_ == INIT) ? SENT_REQUEST : SENT_RESPONSE;
     return (tsig);
 }
 
@@ -345,6 +363,10 @@ TSIGContext::verify(const TSIGRecord* const record, const void* const data,
     // Check time: the current time must be in the range of
     // [time signed - fudge, time signed + fudge].  Otherwise verification
     // fails with BADTIME. (RFC2845 Section 4.6.2)
+    // Note: for simplicity we don't explicitly catch the case of too small
+    // current time causing underflow.  With the fact that fudge is quite
+    // small and (for now) non configurable, it shouldn't be a real concern
+    // in practice.
     const uint64_t now = getTSIGTime();
     if (tsig_rdata.getTimeSigned() + DEFAULT_FUDGE < now ||
         tsig_rdata.getTimeSigned() - DEFAULT_FUDGE > now) {

src/lib/dns/tsig.h

@@ -74,8 +74,7 @@ public:
 /// in this mode will identify the appropriate TSIG key (or internally record
 /// an error if it doesn't find a key).  The server will then verify the
 /// query with the context, and generate a signed response using the same
-/// same context.  (Note: this mode is not yet implemented and may change,
-/// see below).
+/// same context.
 ///
 /// When multiple messages belong to the same TSIG session, either side
 /// (signer or verifier) will keep using the same context.  It records
@@ -83,8 +82,65 @@ public:
 /// calls to \c sign() or \c verify() work correctly in terms of the TSIG
 /// protocol.
 ///
-/// \note The \c verify() method is not yet implemented.  The implementation
-/// and documentation should be updated in the corresponding task.
+/// \b Examples
+///
+/// This is a typical client application that sends a TSIG signed query
+/// and verifies the response.
+///
+/// \code
+///    // "renderer" is of MessageRenderer to render the message.
+///    Message message(Message::RENDER);
+///    message.addQuestion(Question(Name("www.example.com"), RRClass::IN(),
+///                                 RRType::A()));
+///    message.toWire(renderer, ctx);
+///
+///    // sendto, then recvfrom.  received result in (data, data_len)
+///
+///    message.clear(Message::PARSE);
+///    InputBuffer buffer(data, data_len);
+///    message.fromWire(buffer);
+///    TSIGError tsig_error = ctx.verify(message.getTSIGRecord(),
+///                                      data, data_len);
+///    if (tsig_error == TSIGError::NOERROR()) {
+///        // okay.  ctx can be continuously used if it's receiving subsequent
+///        // signed responses from a TCP stream.
+///    } else if (message.getRcode() == Rcode::NOTAUTH()) {
+///        // hard error.  give up this transaction per RFC2845 4.6.
+///    } else {
+///        // keep waiting for further response with the same ctx.
+///    } \endcode
+///
+/// And this is a typical server application that authenticates a signed
+/// query and returns a response according to the result.
+///
+/// \code
+///    // Assume "message" is of type Message for query handling and
+///    // "renderer" is of MessageRenderer to render responses.
+///    Message message(Message::RENDER);
+///
+///    TSIGKeyRing keyring; // this must be configured with keys somewhere
+///
+///    // Receive a query and store it in (data, data_len)
+///    InputBuffer buffer(data, data_len);
+///    message.clear(Message::PARSE);
+///    message.fromWire(buffer);
+///
+///    const TSIGRecord* tsig = message.getTSIGRecord();
+///    if (tsig != NULL) {
+///        TSIGContext ctx(tsig->getName(), tsig->getRdata().getAlgorithm(),
+///                        keyring);
+///        ctx.verify(tsig, data, data_len);
+///
+///        // prepare response
+///        message.makeResponse();
+///        //...
+///        message.toWire(renderer, ctx);
+///
+///        // send the response data back to the client.
+///        // If this is a beginning of a signed session over a TCP and
+///        // server has more data to send to the client, this ctx
+///        // will be used to sign subsequent messages.
+///    } \endcode
 ///
 /// <b>TCP Consideration</b>
 ///
@@ -125,10 +181,10 @@ public:
     /// directly.
     enum State {
         INIT,                   ///< Initial state
-        WAIT_RESPONSE,          /// TODO: document update
-        RECEIVED_REQUEST,
-        SENT_RESPONSE,
-        VERIFIED_RESPONSE
+        SENT_REQUEST, ///< Client sent a signed request, waiting response
+        RECEIVED_REQUEST,       ///< Server received a signed request
+        SENT_RESPONSE,          ///< Server sent a signed response
+        VERIFIED_RESPONSE       ///< Client successfully verified a response
     };
 
     /// \name Constructors and destructor
@@ -162,6 +218,13 @@ public:
     /// complete TSIG RR into the message that has been signed so that it
     /// will become a complete TSIG-signed message.
     ///
+    /// In general, this method is called once by a client to send a
+    /// signed request or one more times by a server to sign
+    /// response(s) to a signed request.  To avoid allowing accidental
+    /// misuse, if this method is called after a "client" validates a
+    /// response, an exception of class \c TSIGContextError will be
+    /// thrown.
+    ///
     /// \note Normal applications are not expected to call this method
     /// directly; they will usually use the \c Message::toWire() method
     /// with a \c TSIGContext object being a parameter and have the
@@ -201,16 +264,89 @@ public:
     ConstTSIGRecordPtr sign(const uint16_t qid, const void* const data,
                             const size_t data_len);
 
-    /// record can be NULL so that we can transparently check the case where
-    /// we sent a signed request but have received an unsigned response.
+    /// Verify a DNS message.
+    ///
+    /// This method verifies given data along with the context and a given
+    /// TSIG in the form of a \c TSIGRecord object.  The data to be verified
+    /// is generally expected to be a complete, wire-format DNS message,
+    /// exactly as received by the host, and ending with a TSIG RR.
+    /// After verification process this method updates its internal state,
+    /// and returns the result in the form of a \c TSIGError object.
+    /// Possible return values are (see the \c TSIGError class description
+    /// for the mnemonics):
     ///
-    /// One unexpected case that is not covered by this method:
-    /// receive a signed reply to an unsigned query
+    /// - \c NOERROR: The data has been verified correctly.
+    /// - \c FORMERR: \c TSIGRecord is not given (see below).
+    /// - \c BAD_KEY: Appropriate key is not found or specified key doesn't
+    ///               match for the data.
+    /// - \c BAD_TIME: The current time doesn't fall in the range specified
+    ///                in the TSIG.
+    /// - \c BAD_SIG: The signature given in the TSIG doesn't match against
+    ///               the locally computed digest or is the signature is
+    ///               invalid in other way.
     ///
-    /// TODO: Note about the overflow + BADTIME case.
+    /// If this method is called by a DNS client waiting for a signed
+    /// response and the result is not \c NOERROR, the context can be used
+    /// to try validating another signed message as described in RFC2845
+    /// Section 4.6.
+    ///
+    /// If this method is called by a DNS server that tries to authenticate
+    /// a signed request, and if the result is not \c NOERROR, the
+    /// corresponding error condition is recorded in the context so that
+    /// the server can return a response indicating what was wrong by calling
+    /// \c sign() with the updated context.
+    ///
+    /// In general, this method is called once by a server for
+    /// authenticating a signed request or one more times by a client to
+    /// validate signed response(s) to a signed request.  To avoid allowing
+    /// accidental misuse, if this method is called after a "server" signs
+    /// a response, an exception of class \c TSIGContextError will be thrown.
+    ///
+    /// The \c record parameter can be NULL; in that case this method simply
+    /// returns \c FORMERR as the case described in Section 4.6 of RFC2845,
+    /// i.e., receiving an unsigned response to a signed request.  This way
+    /// a client can transparently pass the result of
+    /// \c Message::getTSIGRecord() without checking whether it's non NULL
+    /// and take an appropriate action based on the result of this method.
+    ///
+    /// This method handles the given data mostly as opaque.  It digests
+    /// the data assuming it begins with a DNS header and ends with a TSIG
+    /// RR whose length is given by calling \c TSIGRecord::getLength() on
+    /// \c record, but otherwise it doesn't parse the data to confirm the
+    /// assumption.  It's caller's responsibility to ensure the data is
+    /// valid and consistent with \c record.  To avoid disruption, this
+    /// method performs minimal validation on the given \c data and \c record:
+    /// \c data must not be NULL; \c data_len must not be smaller than the
+    /// sum of the DNS header length (fixed, 12 octets) and the length of
+    /// the TSIG RR.  If this check fails it throws an \c InvalidParameter
+    /// exception.
+    ///
+    /// One unexpected case that is not covered by this method is that a
+    /// client receives a signed response to an unsigned request.  RFC2845 is
+    /// silent about such cases; BIND 9 explicitly identifies the case and
+    /// reject it.  With this implementation, the client can know that the
+    /// response contains a TSIG via the result of
+    /// \c Message::getTSIGRecord() and that it is an unexpected TSIG due to
+    /// the fact that it doesn't have a corresponding \c TSIGContext.
+    /// It's up to the client implementation whether to react to such a case
+    /// explicitly (for example, it could either ignore the TSIG and accept
+    /// the response or drop it).
+    ///
+    /// This method provides the strong exception guarantee; unless the method
+    /// returns (without an exception being thrown), the internal state of
+    /// the \c TSIGContext won't be modified.
+    ///
+    /// \todo Support intermediate TCP DNS messages without TSIG (RFC2845 4.4)
+    /// \todo Signature truncation support based on RFC4635
     ///
     /// \exception TSIGContextError Context already signed a response.
-    /// \exception InvalidParameter
+    /// \exception InvalidParameter \c data is NULL or \c data_len is too small.
+    ///
+    /// \param record The \c TSIGRecord to be verified with \c data
+    /// \param data Points to the wire-format data (exactly as received) to
+    /// be verified
+    /// \param data_len The length of \c data in bytes
+    /// \return The \c TSIGError that indicates verification result
     TSIGError verify(const TSIGRecord* const record, const void* const data,
                      const size_t data_len);