From: Thomas Markwalder Date: Wed, 14 May 2014 14:39:11 +0000 (-0400) Subject: [3432] Added basic TSIG support to NameChangeTransaction X-Git-Tag: trac3434_base~20^2~9 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=a00bfe54010dc0fc49820b5ed04fbda45001783f;p=thirdparty%2Fkea.git [3432] Added basic TSIG support to NameChangeTransaction Added TSIGKeyPtr instance member to NameChangeTransaction. Modified NameChangeTransaction::sendUpdate() to do a TSIG updates if its TSIGKeyPtr is not null. Extended FauxServer test class to support TSIG if given a key. Added round trip TSIG tests to NameChangeTransaction base class tests. This does not address how the transaction's key is determined. That requires changes to configuration classes in D2Config. --- diff --git a/src/bin/d2/nc_trans.cc b/src/bin/d2/nc_trans.cc index f9319f0596..2dfaa775c8 100644 --- a/src/bin/d2/nc_trans.cc +++ b/src/bin/d2/nc_trans.cc @@ -54,7 +54,7 @@ NameChangeTransaction(IOServicePtr& io_service, dns_update_status_(DNSClient::OTHER), dns_update_response_(), forward_change_completed_(false), reverse_change_completed_(false), current_server_list_(), current_server_(), next_server_pos_(0), - update_attempts_(0) { + update_attempts_(0), tsig_key_() { // @todo if io_service is NULL we are multi-threading and should // instantiate our own if (!io_service_) { @@ -163,8 +163,7 @@ NameChangeTransaction::transactionOutcomeString() const { void -NameChangeTransaction::sendUpdate(const std::string& comment, - bool /* use_tsig_ */) { +NameChangeTransaction::sendUpdate(const std::string& comment) { try { ++update_attempts_; // @todo add logic to add/replace TSIG key info in request if @@ -173,9 +172,18 @@ NameChangeTransaction::sendUpdate(const std::string& comment, // @todo time out should ultimately be configurable, down to // server level? - dns_client_->doUpdate(*io_service_, current_server_->getIpAddress(), - current_server_->getPort(), *dns_update_request_, - DNS_UPDATE_DEFAULT_TIMEOUT); + 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); + } // Message is on its way, so the next event should be NOP_EVT. postNextEvent(NOP_EVT); @@ -420,6 +428,7 @@ NameChangeTransaction::initServerSelection(const DdnsDomainPtr& domain) { isc_throw(NameChangeTransactionError, "initServerSelection called with an empty domain"); } + current_server_list_ = domain->getServers(); next_server_pos_ = 0; current_server_.reset(); @@ -446,6 +455,11 @@ NameChangeTransaction::selectNextServer() { return (false); } +void +NameChangeTransaction::setTSIGKey(const dns::TSIGKeyPtr& tsig_key) { + tsig_key_ = tsig_key; +} + const DNSClientPtr& NameChangeTransaction::getDNSClient() const { return (dns_client_); diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index b747839e6e..10d16a27bc 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -207,14 +208,15 @@ protected: /// currently selected server. Since the send is asynchronous, the method /// posts NOP_EVT as the next event and then returns. /// + /// If tsig_key_ is not NULL, then the update will be conducted using + /// the key to sign the request and verify the response, otherwise it + /// will be conducted without TSIG. + /// /// @param comment text to include in log detail - /// @param use_tsig True if the update should be include a TSIG key. This - /// is not yet implemented. /// /// If an exception occurs it will be logged and and the transaction will /// be failed. - virtual void sendUpdate(const std::string& comment = "", - bool use_tsig = false); + virtual void sendUpdate(const std::string& comment = ""); /// @brief Adds events defined by NameChangeTransaction to the event set. /// @@ -347,6 +349,12 @@ protected: /// servers from which to select. bool selectNextServer(); + /// @brief Sets the TSIG key to be used. + /// + /// @param tsig_key TSIG Key value to be used for signing and verifying + /// messages. If set to an emtpy pointer, TSIG will not be used. + void setTSIGKey(const dns::TSIGKeyPtr& tsig_key); + /// @brief Sets the update attempt count to the given value. /// /// @param value is the new value to assign. @@ -469,6 +477,12 @@ 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 @@ -570,6 +584,9 @@ private: /// @brief Number of transmit attempts for the current request. size_t update_attempts_; + + /// @brief Pointer to the TSIG key which should be used (if any). + dns::TSIGKeyPtr tsig_key_; }; /// @brief Defines a pointer to a NameChangeTransaction. diff --git a/src/bin/d2/tests/nc_add_unittests.cc b/src/bin/d2/tests/nc_add_unittests.cc index 8b91f5a595..807499273e 100644 --- a/src/bin/d2/tests/nc_add_unittests.cc +++ b/src/bin/d2/tests/nc_add_unittests.cc @@ -55,10 +55,8 @@ public: /// the simulate_send_exception_ flag is true. /// /// @param comment Parameter is unused, but present in base class method. - /// @param use_tsig_ Parameter is unused, but present in base class method. /// - virtual void sendUpdate(const std::string& /*comment*/, - bool /* use_tsig_ = false */) { + virtual void sendUpdate(const std::string& /*comment*/) { if (simulate_send_exception_) { // Make the flag a one-shot by resetting it. simulate_send_exception_ = false; diff --git a/src/bin/d2/tests/nc_remove_unittests.cc b/src/bin/d2/tests/nc_remove_unittests.cc index 6ca4ecf87e..e90e0f04d5 100644 --- a/src/bin/d2/tests/nc_remove_unittests.cc +++ b/src/bin/d2/tests/nc_remove_unittests.cc @@ -56,10 +56,8 @@ public: /// the simulate_send_exception_ flag is true. /// /// @param comment Parameter is unused, but present in base class method - /// @param use_tsig Parameter is unused, but present in base class method. /// - virtual void sendUpdate(const std::string& /* comment */, - bool /* use_tsig = false */) { + virtual void sendUpdate(const std::string& /* comment */) { if (simulate_send_exception_) { // Make the flag a one-shot by resetting it. simulate_send_exception_ = false; diff --git a/src/bin/d2/tests/nc_test_utils.cc b/src/bin/d2/tests/nc_test_utils.cc index 957688aad6..130b35b7f1 100644 --- a/src/bin/d2/tests/nc_test_utils.cc +++ b/src/bin/d2/tests/nc_test_utils.cc @@ -31,7 +31,6 @@ namespace d2 { const char* TEST_DNS_SERVER_IP = "127.0.0.1"; size_t TEST_DNS_SERVER_PORT = 5301; -const bool HAS_RDATA = true; const bool NO_RDATA = false; //*************************** FauxServer class *********************** @@ -39,7 +38,8 @@ const bool NO_RDATA = false; FauxServer::FauxServer(asiolink::IOService& io_service, asiolink::IOAddress& address, size_t port) :io_service_(io_service), address_(address), port_(port), - server_socket_(), receive_pending_(false), perpetual_receive_(true) { + server_socket_(), receive_pending_(false), perpetual_receive_(true), + tsig_key_() { server_socket_.reset(new asio::ip::udp::socket(io_service_.get_io_service(), asio::ip::udp::v4())); @@ -53,7 +53,7 @@ FauxServer::FauxServer(asiolink::IOService& io_service, DnsServerInfo& server) :io_service_(io_service), address_(server.getIpAddress()), port_(server.getPort()), server_socket_(), receive_pending_(false), - perpetual_receive_(true) { + perpetual_receive_(true), tsig_key_() { server_socket_.reset(new asio::ip::udp::socket(io_service_.get_io_service(), asio::ip::udp::v4())); server_socket_->set_option(asio::socket_base::reuse_address(true)); @@ -95,6 +95,13 @@ FauxServer::requestHandler(const asio::error_code& error, return; } + // If TSIG key isn't NULL, create a context and use to verify the + // request and sign the response. + dns::TSIGContextPtr context; + if (tsig_key_) { + context.reset(new dns::TSIGContext(*tsig_key_)); + } + // We have a successfully received data. We need to turn it into // a request in order to build a proper response. // Note D2UpdateMessage is geared towards making requests and @@ -104,6 +111,17 @@ FauxServer::requestHandler(const asio::error_code& error, util::InputBuffer request_buf(receive_buffer_, bytes_recvd); try { request.fromWire(request_buf); + + // If contex is not NULL, then we need to verify the message. + if (context) { + dns::TSIGError error = context->verify(request.getTSIGRecord(), + receive_buffer_, + bytes_recvd); + if (error != dns::TSIGError::NOERROR()) { + isc_throw(TSIGVerifyError, "TSIG verification failed: " + << error.toText()); + } + } } catch (const std::exception& ex) { // If the request cannot be parsed, then fail the test. // We expect the client to send good requests. @@ -131,7 +149,19 @@ FauxServer::requestHandler(const asio::error_code& error, dns::MessageRenderer renderer; util::OutputBuffer response_buf(TEST_MSG_MAX); renderer.setBuffer(&response_buf); - response.toWire(renderer); + + if (response_mode == INVALID_TSIG) { + // Create a different key to sign the response. + std::string secret ("key that doesn't match"); + dns::TSIGKeyPtr key; + ASSERT_NO_THROW(key.reset(new + dns::TSIGKey(dns::Name("badkey"), + dns::TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); + context.reset(new dns::TSIGContext(*key)); + } + + response.toWire(renderer, context.get()); // If mode is to ship garbage, then stomp on part of the rendered // message. @@ -162,6 +192,7 @@ FauxServer::requestHandler(const asio::error_code& error, } + //********************** TimedIO class *********************** TimedIO::TimedIO() diff --git a/src/bin/d2/tests/nc_test_utils.h b/src/bin/d2/tests/nc_test_utils.h index a99752528c..7388f435a4 100644 --- a/src/bin/d2/tests/nc_test_utils.h +++ b/src/bin/d2/tests/nc_test_utils.h @@ -40,8 +40,9 @@ typedef boost::shared_ptr SocketPtr; class FauxServer { public: enum ResponseMode { - USE_RCODE, // Generate a response with a given RCODE - CORRUPT_RESP // Generate a corrupt response + USE_RCODE, // Generate a response with a given RCODE + CORRUPT_RESP, // Generate a corrupt response + INVALID_TSIG // Generate a repsonse with the wrong TSIG key }; // Reference to IOService to use for IO processing. @@ -63,6 +64,9 @@ public: // a receive has been completed, a new one will be automatically // initiated. bool perpetual_receive_; + // TSIG Key to use to verify requests and sign responses. If its + // NULL TSIG is not used. + dns::TSIGKeyPtr tsig_key_; /// @brief Constructor /// @@ -96,7 +100,9 @@ public: /// @brief Socket IO Completion callback /// /// This method servers as the Server's UDP socket receive callback handler. - /// When the receive completes the handler is invoked with the + /// When the receive completes the handler is invoked with the parameters + /// listed. + /// /// @param error result code of the receive (determined by asio layer) /// @param bytes_recvd number of bytes received, if any /// @param response_mode type of response the handler should produce @@ -111,6 +117,14 @@ public: bool isReceivePending() { return receive_pending_; } + + /// @breif 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) { + tsig_key_ = tsig_key; + } }; /// @brief Provides a means to process IOService IO for a finite amount of time. diff --git a/src/bin/d2/tests/nc_trans_unittests.cc b/src/bin/d2/tests/nc_trans_unittests.cc index 5784647fd2..b9168bcb77 100644 --- a/src/bin/d2/tests/nc_trans_unittests.cc +++ b/src/bin/d2/tests/nc_trans_unittests.cc @@ -264,6 +264,7 @@ public: using NameChangeTransaction::addPtrRdata; using NameChangeTransaction::responseString; using NameChangeTransaction::transactionOutcomeString; + using NameChangeTransaction::setTSIGKey; }; // Declare them so Gtest can see them. @@ -1004,6 +1005,154 @@ TEST_F(NameChangeTransactionTest, sendUpdate) { EXPECT_EQ("response.example.com.", zone->getName().toText()); } +/// @brief Tests that an unsigned response to a signed request is an error +TEST_F(NameChangeTransactionTest, tsigUnsignedResponse) { + NameChangeStubPtr name_change; + ASSERT_NO_THROW(name_change = makeCannedTransaction()); + ASSERT_NO_THROW(name_change->initDictionaries()); + ASSERT_TRUE(name_change->selectFwdServer()); + + // Create a server and start it listening. + FauxServer server(*io_service_, *(name_change->getCurrentServer())); + server.receive (FauxServer::USE_RCODE, dns::Rcode::NOERROR()); + + // Create a key and manually set it as the transaction's TSIG key + std::string secret ("key number one"); + dns::TSIGKeyPtr key_one; + ASSERT_NO_THROW(key_one.reset(new + dns::TSIGKey(dns::Name("one.com"), + dns::TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); + name_change->setTSIGKey(key_one); + ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change)); + + // Verify that next event is IO_COMPLETED_EVT and DNS status is + // INVALID_RESPONSE. + ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT, + name_change->getNextEvent()); + + ASSERT_EQ(DNSClient::INVALID_RESPONSE, name_change->getDnsUpdateStatus()); + + // When TSIG errors occur, only the message header (including Rcode) is + // unpacked. In this case, it should be NOERROR but have no other + // information. + D2UpdateMessagePtr response = name_change->getDnsUpdateResponse(); + ASSERT_TRUE(response); + ASSERT_EQ(dns::Rcode::NOERROR().getCode(), response->getRcode().getCode()); + EXPECT_FALSE(response->getZone()); +} + +/// @brief Tests that a response signed with the wrong key is an error +TEST_F(NameChangeTransactionTest, tsigInvalidResponse) { + NameChangeStubPtr name_change; + ASSERT_NO_THROW(name_change = makeCannedTransaction()); + ASSERT_NO_THROW(name_change->initDictionaries()); + ASSERT_TRUE(name_change->selectFwdServer()); + + // Create a server, tell it to sign responses with a "random" key, + // then start it listening. + FauxServer server(*io_service_, *(name_change->getCurrentServer())); + server.receive (FauxServer::INVALID_TSIG, dns::Rcode::NOERROR()); + + // Create a key and manually set it as the transaction's TSIG key + std::string secret ("key number one"); + dns::TSIGKeyPtr key_one; + ASSERT_NO_THROW(key_one.reset(new + dns::TSIGKey(dns::Name("one.com"), + dns::TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); + name_change->setTSIGKey(key_one); + ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change)); + + // Verify that next event is IO_COMPLETED_EVT and DNS status is + // INVALID_RESPONSE. + ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT, + name_change->getNextEvent()); + + ASSERT_EQ(DNSClient::INVALID_RESPONSE, name_change->getDnsUpdateStatus()); + + // When TSIG errors occur, only the message header (including Rcode) is + // unpacked. In this case, it should be NOERROR but have no other + // information. + D2UpdateMessagePtr response = name_change->getDnsUpdateResponse(); + ASSERT_TRUE(response); + ASSERT_EQ(dns::Rcode::NOERROR().getCode(), response->getRcode().getCode()); + EXPECT_FALSE(response->getZone()); +} + +/// @brief Tests that a signed response to an unsigned request is ok. +/// Currently our policy is to accept a signed response to an unsigned request +/// even though the spec says a server MUST not do that. +TEST_F(NameChangeTransactionTest, tsigUnexpectedSignedResponse) { + NameChangeStubPtr name_change; + ASSERT_NO_THROW(name_change = makeCannedTransaction()); + ASSERT_NO_THROW(name_change->initDictionaries()); + ASSERT_TRUE(name_change->selectFwdServer()); + + // Create a server, tell it to sign responses with a "random" key, + // then start it listening. + FauxServer server(*io_service_, *(name_change->getCurrentServer())); + server.receive (FauxServer::INVALID_TSIG, dns::Rcode::NOERROR()); + + // Perform an update without TSIG. + ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change)); + + // Verify that next event is IO_COMPLETED_EVT and DNS status is SUCCESS. + ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT, + name_change->getNextEvent()); + + ASSERT_EQ(DNSClient::SUCCESS, name_change->getDnsUpdateStatus()); + + D2UpdateMessagePtr response = name_change->getDnsUpdateResponse(); + ASSERT_TRUE(response); + ASSERT_EQ(dns::Rcode::NOERROR().getCode(), response->getRcode().getCode()); + D2ZonePtr zone = response->getZone(); + EXPECT_TRUE(zone); + EXPECT_EQ("response.example.com.", zone->getName().toText()); +} + + +/// @brief Tests that a TSIG udpate succeeds when client and server both use +/// the right key. +TEST_F(NameChangeTransactionTest, tsigValidExchange) { + NameChangeStubPtr name_change; + ASSERT_NO_THROW(name_change = makeCannedTransaction()); + ASSERT_NO_THROW(name_change->initDictionaries()); + ASSERT_TRUE(name_change->selectFwdServer()); + + // Create a key + std::string secret ("key number one"); + dns::TSIGKeyPtr key_one; + ASSERT_NO_THROW(key_one.reset(new + dns::TSIGKey(dns::Name("one.com"), + dns::TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); + + // Create a server, set its TSIG key, and then start it listening. + FauxServer server(*io_service_, *(name_change->getCurrentServer())); + server.setTSIGKey(key_one); + server.receive (FauxServer::USE_RCODE, dns::Rcode::NOERROR()); + + // Manually set the transaction's key to the same key as the server. + name_change->setTSIGKey(key_one); + + // Do the update. + ASSERT_NO_FATAL_FAILURE(doOneExchange(name_change)); + + // Verify that next event is IO_COMPLETED_EVT and DNS status is SUCCESS. + ASSERT_EQ(NameChangeTransaction::IO_COMPLETED_EVT, + name_change->getNextEvent()); + + ASSERT_EQ(DNSClient::SUCCESS, name_change->getDnsUpdateStatus()); + + D2UpdateMessagePtr response = name_change->getDnsUpdateResponse(); + ASSERT_TRUE(response); + ASSERT_EQ(dns::Rcode::NOERROR().getCode(), response->getRcode().getCode()); + D2ZonePtr zone = response->getZone(); + EXPECT_TRUE(zone); + EXPECT_EQ("response.example.com.", zone->getName().toText()); +} + /// @brief Tests the prepNewRequest method TEST_F(NameChangeTransactionTest, prepNewRequest) { NameChangeStubPtr name_change;