[3432] Addressed review comments

Consolidated DNSClient::doUpdate variants into a single method which
accepts a smart pointer to a TSIGKey instead of TSIGKey reference.
Simplified some unit tests.
Added missing commentary.
Corrected typos and copyright dates.

src/bin/d2/d2_config.cc

@@ -50,24 +50,19 @@ const dns::Name&
 TSIGKeyInfo::stringToAlgorithmName(const std::string& algorithm_id) {
     if (boost::iequals(algorithm_id, HMAC_MD5_STR)) {
         return (dns::TSIGKey::HMACMD5_NAME());
-    }
-    if (boost::iequals(algorithm_id, HMAC_SHA1_STR)) {
+    } else if (boost::iequals(algorithm_id, HMAC_SHA1_STR)) {
         return (dns::TSIGKey::HMACSHA1_NAME());
-    }
-    if (boost::iequals(algorithm_id, HMAC_SHA224_STR)) {
+    } else if (boost::iequals(algorithm_id, HMAC_SHA224_STR)) {
         return (dns::TSIGKey::HMACSHA224_NAME());
-    }
-    if (boost::iequals(algorithm_id, HMAC_SHA256_STR)) {
+    } else if (boost::iequals(algorithm_id, HMAC_SHA256_STR)) {
         return (dns::TSIGKey::HMACSHA256_NAME());
-    }
-    if (boost::iequals(algorithm_id, HMAC_SHA384_STR)) {
+    } else if (boost::iequals(algorithm_id, HMAC_SHA384_STR)) {
         return (dns::TSIGKey::HMACSHA384_NAME());
-    }
-    if (boost::iequals(algorithm_id, HMAC_SHA512_STR)) {
+    } else if (boost::iequals(algorithm_id, HMAC_SHA512_STR)) {
         return (dns::TSIGKey::HMACSHA512_NAME());
     }
 
-    isc_throw(BadValue, "Unknown TSIG Key algorithm:" << algorithm_id);
+    isc_throw(BadValue, "Unknown TSIG Key algorithm: " << algorithm_id);
 }
 
 void
@@ -577,8 +572,8 @@ DdnsDomainParser::build(isc::data::ConstElementPtr domain_config) {
         }
 
         if (!tsig_key_info) {
-            isc_throw(D2CfgError, "DdnsDomain :" << name <<
-                     " specifies an undefined key:" << key_name);
+            isc_throw(D2CfgError, "DdnsDomain " << name <<
+                     " specifies an undefined key: " << key_name);
         }
     }
 

src/bin/d2/d2_config.h

@@ -175,9 +175,10 @@ public:
     /// -# "HMAC-SHA384"
     /// -# "HMAC-SHA512"
     ///
-    /// @param secret the base-64 encoded secret component for this key
-    /// such as one would typically see in a key file for the entry "Key"
-    /// in the example below:
+    /// @param secret  The base-64 encoded secret component for this key.
+    /// (A suitable string for use here could be obtained by running the
+    /// BIND 9 dnssec-keygen program; the contents of resulting key file
+    /// will look similar to:
     /// @code
     ///   Private-key-format: v1.3
     ///   Algorithm: 157 (HMAC_MD5)
@@ -187,6 +188,7 @@ public:
     ///   Publish: 20140515143700
     ///   Activate: 20140515143700
     /// @endcode
+    /// where the value the "Key:" entry is the secret component of the key.)
     ///
     /// @throw D2CfgError if values supplied are invalid:
     /// name cannot be blank, algorithm must be a supported value,
@@ -237,7 +239,7 @@ public:
     /// -# "HMAC-SHA384"
     /// -# "HMAC-SHA512"
     ///
-    /// @return const reference to a dns::Name containing the alogorithm name
+    /// @return const reference to a dns::Name containing the algorithm name
     /// @throw BadValue if ID isn't recognized.
     static const dns::Name& stringToAlgorithmName(const std::string&
                                                   algorithm_id);
@@ -419,7 +421,7 @@ public:
         return (name_);
     }
 
-    /// @brief Conveneince method which returns the domain's TSIG key name.
+    /// @brief Convenience method which returns the domain's TSIG key name.
     ///
     /// @return returns the key name in an std::string. If domain has no
     /// TSIG key, the string will empty.
@@ -432,6 +434,10 @@ public:
         return (servers_);
     }
 
+    /// @brief Getter which returns the domain's TSIGKey info
+    ///
+    /// @return returns the pointer to the server storage.  If the domain
+    /// is not configured to use TSIG the pointer will be empty.
     const TSIGKeyInfoPtr& getTSIGKeyInfo() {
         return (tsig_key_info_);
     }
@@ -443,7 +449,8 @@ private:
     /// @brief The list of server(s) supporting this domain.
     DnsServerInfoStoragePtr servers_;
 
-    /// @brief
+    /// @brief Pointer to domain's the TSIGKeyInfo.
+    /// Value is empty if the domain is not configured for TSIG.
     TSIGKeyInfoPtr tsig_key_info_;
 };
 

src/bin/d2/d2_update_message.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/bin/d2/d2_update_message.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/bin/d2/dns_client.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -81,14 +81,7 @@ public:
                   const uint16_t ns_port,
                   D2UpdateMessage& update,
                   const unsigned int wait,
-                  const dns::TSIGKey& tsig_key);
-
-    // Starts asynchronous DNS Update.
-    void doUpdate(asiolink::IOService& io_service,
-                  const asiolink::IOAddress& ns_addr,
-                  const uint16_t ns_port,
-                  D2UpdateMessage& update,
-                  const unsigned int wait);
+                  const dns::TSIGKeyPtr& tsig_key);
 
     // This function maps the IO error to the DNSClient error.
     DNSClient::Status getStatus(const asiodns::IOFetch::Result);
@@ -194,17 +187,7 @@ DNSClientImpl::doUpdate(asiolink::IOService& io_service,
                         const uint16_t ns_port,
                         D2UpdateMessage& update,
                         const unsigned int wait,
-                        const dns::TSIGKey& tsig_key) {
-    tsig_context_.reset(new TSIGContext(tsig_key));
-    doUpdate(io_service, ns_addr, ns_port, update, wait);
-}
-
-void
-DNSClientImpl::doUpdate(asiolink::IOService& io_service,
-                        const IOAddress& ns_addr,
-                        const uint16_t ns_port,
-                        D2UpdateMessage& update,
-                        const unsigned int wait) {
+                        const dns::TSIGKeyPtr& tsig_key) {
     // The underlying implementation which we use to send DNS Updates uses
     // signed integers for timeout. If we want to avoid overflows we need to
     // respect this limitation here.
@@ -214,6 +197,15 @@ DNSClientImpl::doUpdate(asiolink::IOService& io_service,
                   << ". Provided timeout value is '" << wait << "'");
     }
 
+    // Create a TSIG context if we have a key, otherwise clear the context
+    // pointer.  Message marshalling uses non-null context is the indicator
+    // that TSIG should be used.
+    if (tsig_key) {
+        tsig_context_.reset(new TSIGContext(*tsig_key));
+    } else {
+        tsig_context_.reset();
+    }
+
     // A renderer is used by the toWire function which creates the on-wire data
     // from the DNS Update message. A renderer has its internal buffer where it
     // renders data by default. However, this buffer can't be directly accessed.
@@ -244,7 +236,6 @@ DNSClientImpl::doUpdate(asiolink::IOService& io_service,
     io_service.post(io_fetch);
 }
 
-
 DNSClient::DNSClient(D2UpdateMessagePtr& response_placeholder,
                      Callback* callback, const DNSClient::Protocol proto)
     : impl_(new DNSClientImpl(response_placeholder, callback, proto)) {
@@ -266,19 +257,9 @@ DNSClient::doUpdate(asiolink::IOService& io_service,
                     const uint16_t ns_port,
                     D2UpdateMessage& update,
                     const unsigned int wait,
-                    const dns::TSIGKey& tsig_key) {
+                    const dns::TSIGKeyPtr& tsig_key) {
     impl_->doUpdate(io_service, ns_addr, ns_port, update, wait, tsig_key);
 }
 
-void
-DNSClient::doUpdate(asiolink::IOService& io_service,
-                    const IOAddress& ns_addr,
-                    const uint16_t ns_port,
-                    D2UpdateMessage& update,
-                    const unsigned int wait) {
-    impl_->doUpdate(io_service, ns_addr, ns_port, update, wait);
-}
-
 } // namespace d2
 } // namespace isc
-

src/bin/d2/dns_client.h

@@ -146,41 +146,15 @@ public:
     /// @param wait A timeout (in milliseconds) for the response. If a response
     /// is not received within the timeout, exchange is interrupted. This value
     /// must not exceed maximal value for 'int' data type.
-    /// @param tsig_key An @c isc::dns::TSIGKey object representing TSIG
-    /// context which will be used to render the DNS Update message.
-    ///
-    /// @todo Implement TSIG Support. Currently any attempt to call this
-    /// function will result in exception.
+    /// @param tsig_key A pointer to an @c isc::dns::TSIGKey object that will
+    /// (if not null) be used to sign the DNS Update message and verify the
+    /// response.
     void doUpdate(asiolink::IOService& io_service,
                   const asiolink::IOAddress& ns_addr,
                   const uint16_t ns_port,
                   D2UpdateMessage& update,
                   const unsigned int wait,
-                  const dns::TSIGKey& tsig_key);
-
-    /// @brief Start asynchronous DNS Update without TSIG.
-    ///
-    /// This function starts asynchronous DNS Update and returns. The DNS Update
-    /// will be executed by the specified IO service. Once the message exchange
-    /// with a DNS server is complete, timeout occurs or IO operation is
-    /// interrupted, the caller-supplied callback function will be invoked.
-    ///
-    /// An address and port of the DNS server is specified through the function
-    /// arguments so as the same instance of the @c DNSClient can be used to
-    /// initiate multiple message exchanges.
-    ///
-    /// @param io_service IO service to be used to run the message exchange.
-    /// @param ns_addr DNS server address.
-    /// @param ns_port DNS server port.
-    /// @param update A DNS Update message to be sent to the server.
-    /// @param wait A timeout (in milliseconds) for the response. If a response
-    /// is not received within the timeout, exchange is interrupted. This value
-    /// must not exceed maximal value for 'int' data type.
-    void doUpdate(asiolink::IOService& io_service,
-                  const asiolink::IOAddress& ns_addr,
-                  const uint16_t ns_port,
-                  D2UpdateMessage& update,
-                  const unsigned int wait);
+                  const dns::TSIGKeyPtr& tsig_key = dns::TSIGKeyPtr());
 
 private:
     DNSClientImpl* impl_;  ///< Pointer to DNSClient implementation.

src/bin/d2/nc_trans.cc

@@ -172,18 +172,11 @@ NameChangeTransaction::sendUpdate(const std::string& comment) {
 
         // @todo time out should ultimately be configurable, down to
         // server level?
-        if (tsig_key_) {
-            dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(),
-                                  current_server_->getPort(),
-                                  *dns_update_request_,
-                                  DNS_UPDATE_DEFAULT_TIMEOUT,
-                                  *tsig_key_);
-        } else {
-            dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(),
-                                  current_server_->getPort(),
-                                  *dns_update_request_,
-                                  DNS_UPDATE_DEFAULT_TIMEOUT);
-        }
+        dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(),
+                              current_server_->getPort(),
+                              *dns_update_request_,
+                              DNS_UPDATE_DEFAULT_TIMEOUT,
+                              tsig_key_);
 
         // Message is on its way, so the next event should be NOP_EVT.
         postNextEvent(NOP_EVT);

src/bin/d2/nc_trans.h

@@ -471,12 +471,6 @@ public:
     /// @return A const pointer reference to the DNSClient
     const DNSClientPtr& getDNSClient() const;
 
-    /// @brief Pointer to the TSIG key which should be used (if any).
-    ///
-    /// @return A const pointer reference to the current TSIG key. Pointer
-    /// will be empty if TSIG is not being used.
-    const dns::TSIGKeyPtr& getTSIGKey() const;
-
     /// @brief Fetches the current DNS update request packet.
     ///
     /// @return A const pointer reference to the current D2UpdateMessage

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -108,9 +108,6 @@ bool checkServer(DnsServerInfoPtr server, const char* hostname,
 /// @brief Convenience function which compares the contents of the given
 /// TSIGKeyInfo against the given set of values.
 ///
-/// It is structured in such a way that each value is checked, and output
-/// is generate for all that do not match.
-///
 /// @param key is a pointer to the key to check against.
 /// @param name is the value to compare against key's name_.
 /// @param algorithm is the string value to compare against key's algorithm.
@@ -119,39 +116,13 @@ bool checkServer(DnsServerInfoPtr server, const char* hostname,
 /// @return returns true if there is a match across the board, otherwise it
 /// returns false.
 bool checkKey(TSIGKeyInfoPtr key, const std::string& name,
-                 const std::string& algorithm, const std::string& secret)
-{
+                 const std::string& algorithm, const std::string& secret) {
     // Return value, assume its a match.
-    bool result = true;
-    if (!key) {
-        EXPECT_TRUE(key);
-        return false;
-    }
-
-    // Check name.
-    if (key->getName() != name) {
-        EXPECT_EQ(name, key->getName());
-        result = false;
-    }
-
-    // Check algorithm.
-    if (key->getAlgorithm() != algorithm) {
-        EXPECT_EQ(algorithm, key->getAlgorithm());
-        result = false;
-    }
-
-    // Check secret.
-    if (key->getSecret() !=  secret) {
-        EXPECT_EQ (secret, key->getSecret());
-        result = false;
-    }
-
-    if (!key->getTSIGKey()) {
-        EXPECT_TRUE (key->getTSIGKey());
-        return false;
-    }
-
-    return (result);
+    return (((key) &&
+        (key->getName() == name) &&
+        (key->getAlgorithm() == algorithm)  &&
+        (key->getSecret() ==  secret)  &&
+        (key->getTSIGKey())));
 }
 
 /// @brief Test fixture class for testing DnsServerInfo parsing.

src/bin/d2/tests/d2_process_unittests.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/bin/d2/tests/d2_update_message_unittests.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -591,7 +591,7 @@ TEST_F(D2UpdateMessageTest, toWireInvalidQRFlag) {
 // TSIG test
 TEST_F(D2UpdateMessageTest, validTSIG) {
     // Create a TSIG Key and context
-    std::string secret ("this key will match");
+    std::string secret("this key will match");
     TSIGKeyPtr right_key;
     ASSERT_NO_THROW(right_key.reset(new
                                     TSIGKey(Name("right.com"),

src/bin/d2/tests/dns_client_unittests.cc

@@ -188,6 +188,29 @@ public:
                         *remote);
     }
 
+    // @brief Request handler for testing clients using TSIG
+    //
+    // This callback handler is installed when performing async read on a
+    // socket to emulate reception of the DNS Update request with TSIG by a
+    // server.  As a result, this handler will send an appropriate DNS Update
+    // response message back to the address from which the request has come.
+    //
+    // @param socket A pointer to a socket used to receive a query and send a
+    // response.
+    // @param remote A pointer to an object which specifies the host (address
+    // and port) from which a request has come.
+    // @param receive_length A length (in bytes) of the received data.
+    // @param corrupt_response A bool value which indicates that the server's
+    // response should be invalid (true) or valid (false)
+    // @param client_key TSIG key the server should use to verify the inbound
+    // request.  If the pointer is NULL, the server will not attempt to
+    // verify the request.
+    // @param server_key TSIG key the server should use to sign the outbound
+    // request. If the pointer is NULL, the server will not sign the outbound
+    // response.  If the pointer is not NULL and not the same value as the
+    // client_key, the server will use a new context to sign the response then
+    // the one used to verify it.  This allows us to simulate the server
+    // signing with the wrong key.
     void TSIGReceiveHandler(udp::socket* socket, udp::endpoint* remote,
                             size_t receive_length,
                             TSIGKeyPtr client_key,
@@ -428,13 +451,8 @@ public:
         // response.
         const int timeout = 500;
         expected_++;
-        if (client_key) {
-            dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS), TEST_PORT,
-                                  message, timeout, *client_key);
-        } else {
-            dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS), TEST_PORT,
-                                  message, timeout);
-        }
+        dns_client_->doUpdate(service_, IOAddress(TEST_ADDRESS), TEST_PORT,
+                              message, timeout, client_key);
 
         // Kick of the message exchange by actually running the scheduled
         // "send" and "receive" operations.

src/bin/d2/tests/nc_test_utils.cc

@@ -88,11 +88,11 @@ FauxServer::requestHandler(const asio::error_code& error,
                            std::size_t bytes_recvd,
                            const ResponseMode& response_mode,
                            const dns::Rcode& response_rcode) {
+    receive_pending_ = false;
     // If we encountered an error or received no data then fail.
     // We expect the client to send good requests.
     if (error.value() != 0 || bytes_recvd < 1) {
         ADD_FAILURE() << "FauxServer receive failed: " << error.message();
-        receive_pending_ = false;
         return;
     }
 
@@ -127,7 +127,6 @@ FauxServer::requestHandler(const asio::error_code& error,
         // If the request cannot be parsed, then fail the test.
         // We expect the client to send good requests.
         ADD_FAILURE() << "FauxServer request is corrupt:" << ex.what();
-        receive_pending_ = false;
         return;
     }
 
@@ -185,7 +184,6 @@ FauxServer::requestHandler(const asio::error_code& error,
         ADD_FAILURE() << "FauxServer send failed: " << ex.what();
     }
 
-    receive_pending_ = false;
     if (perpetual_receive_) {
         // Schedule the next receive
         receive (response_mode, response_rcode);
@@ -433,9 +431,9 @@ dhcp_ddns::NameChangeRequestPtr makeNcrFromString(const std::string& ncr_str) {
 
 DdnsDomainPtr makeDomain(const std::string& zone_name,
                          const std::string& key_name) {
-    DdnsDomainPtr domain;
     DnsServerInfoStoragePtr servers(new DnsServerInfoStorage());
-    domain.reset(new DdnsDomain(zone_name, servers, makeTSIGKeyInfo(key_name)));
+    DdnsDomainPtr domain(new DdnsDomain(zone_name, servers,
+                         makeTSIGKeyInfo(key_name)));
     return (domain);
 }
 

src/bin/d2/tests/nc_test_utils.h

@@ -118,11 +118,11 @@ public:
         return receive_pending_;
     }
 
-    /// @breif Sets the TSIG key to the given value.
+    /// @brief Sets the TSIG key to the given value.
     ///
     /// @param tsig_key Pointer to the TSIG key to use.  If the pointer is
     /// empty, TSIG will not be used.
-    void setTSIGKey (const dns::TSIGKeyPtr tsig_key) {
+    void setTSIGKey (const dns::TSIGKeyPtr& tsig_key) {
         tsig_key_ = tsig_key;
     }
 };

src/bin/d2/tests/nc_trans_unittests.cc

@@ -1120,6 +1120,7 @@ TEST_F(NameChangeTransactionTest, tsigAllValid) {
     algorithms.push_back(TSIGKeyInfo::HMAC_SHA512_STR);
 
     for (int i = 0; i < algorithms.size(); ++i) {
+        SCOPED_TRACE (algorithms[i]);
         TSIGKeyInfoPtr key;
         ASSERT_NO_THROW(key.reset(new TSIGKeyInfo("test_key",
                                                   algorithms[i],