From ffcb75a19e38aa03575ce65f2556216cc94eb251 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Thu, 19 Aug 2021 00:37:26 +0200 Subject: [PATCH] [#2040] Checkpoint (todo: add stat UTs) --- src/bin/d2/d2_hooks.dox | 2 +- src/bin/d2/d2_process.cc | 4 ++ src/bin/d2/dns_client.cc | 41 +++++++++++++++++-- src/bin/d2/dns_client.h | 8 ++-- src/bin/d2/nc_trans.h | 4 +- src/bin/d2/tests/d2_simple_parser_unittest.cc | 2 +- .../d2/tests/d2_update_message_unittests.cc | 22 +++++----- src/bin/d2/tests/dns_client_unittests.cc | 24 +++++------ src/bin/d2/tests/nc_test_utils.cc | 8 ++-- src/bin/d2/tests/nc_test_utils.h | 4 +- src/bin/d2/tests/testdata/d2_cfg_tests.json | 2 +- src/lib/d2srv/d2_config.cc | 8 ++-- src/lib/d2srv/d2_config.h | 8 ++-- src/lib/d2srv/d2_tsig_key.cc | 15 +++++-- src/lib/d2srv/d2_tsig_key.h | 24 ++++++++++- src/lib/d2srv/tests/d2_tsig_key_unittest.cc | 4 +- 16 files changed, 123 insertions(+), 57 deletions(-) diff --git a/src/bin/d2/d2_hooks.dox b/src/bin/d2/d2_hooks.dox index 8ef06a9b18..0c33820b8f 100644 --- a/src/bin/d2/d2_hooks.dox +++ b/src/bin/d2/d2_hooks.dox @@ -69,7 +69,7 @@ to the end of this list. @subsection d2HooksSelectKey select_key - @b Arguments: - name: @b current_server, type: isc::d2::DnsServerInfoPtr, direction: in - - name: @b tsig_key, type: isc::dns::TSIGKeyPtr, direction: in/out + - name: @b tsig_key, type: isc::d2::D2TsigKeyPtr, direction: in/out - @b Description: this callout is executed when the D2 is selecting for a TSIG key to protect the next DNS update to the already chosen DNS diff --git a/src/bin/d2/d2_process.cc b/src/bin/d2/d2_process.cc index a741a978c7..f26bc07f0a 100644 --- a/src/bin/d2/d2_process.cc +++ b/src/bin/d2/d2_process.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,9 @@ D2Process::D2Process(const char* name, const asiolink::IOServicePtr& io_service) // Instantiate stats manager. isc::stats::StatsMgr& stats_mgr = isc::stats::StatsMgr::instance(); stats_mgr.setMaxSampleCountDefault(0); + for (const auto& name : D2TsigKey::globalStats) { + stats_mgr.setValue("global." + name, static_cast(0)); + } }; void diff --git a/src/bin/d2/dns_client.cc b/src/bin/d2/dns_client.cc index 1d4e4d62c8..a7f91182b8 100644 --- a/src/bin/d2/dns_client.cc +++ b/src/bin/d2/dns_client.cc @@ -8,6 +8,7 @@ #include #include #include +#include #include namespace isc { @@ -26,6 +27,7 @@ using namespace isc::util; using namespace isc::asiolink; using namespace isc::asiodns; using namespace isc::dns; +using namespace isc::stats; // This class provides the implementation for the DNSClient. This allows for // the separation of the DNSClient interface from the implementation details. @@ -55,6 +57,8 @@ public: DNSClient::Protocol proto_; // TSIG context used to sign outbound and verify inbound messages. dns::TSIGContextPtr tsig_context_; + // TSIG key name for stats. + std::string tsig_key_name_; // Constructor and Destructor DNSClientImpl(D2UpdateMessagePtr& response_placeholder, @@ -74,10 +78,13 @@ public: const uint16_t ns_port, D2UpdateMessage& update, const unsigned int wait, - const dns::TSIGKeyPtr& tsig_key); + const D2TsigKeyPtr& tsig_key); // This function maps the IO error to the DNSClient error. DNSClient::Status getStatus(const asiodns::IOFetch::Result); + + // This function updates statistics. + void incrStats(const std::string& stat, bool update_key = true); }; DNSClientImpl::DNSClientImpl(D2UpdateMessagePtr& response_placeholder, @@ -137,17 +144,22 @@ DNSClientImpl::operator()(asiodns::IOFetch::Result result) { try { response_->fromWire(in_buf_->getData(), in_buf_->getLength(), tsig_context_.get()); + incrStats("success"); } catch (const isc::Exception& ex) { status = DNSClient::INVALID_RESPONSE; LOG_DEBUG(d2_to_dns_logger, isc::log::DBGLVL_TRACE_DETAIL, DHCP_DDNS_INVALID_RESPONSE).arg(ex.what()); - + incrStats("error"); } if (tsig_context_) { // Context is a one-shot deal, get rid of it. tsig_context_.reset(); } + } else if (status == DNSClient::TIMEOUT) { + incrStats("timeout"); + } else { + incrStats("error"); } // Once we are done with internal business, let's call a callback supplied @@ -174,13 +186,14 @@ DNSClientImpl::getStatus(const asiodns::IOFetch::Result result) { } return (DNSClient::OTHER); } + 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) { + const D2TsigKeyPtr& 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. @@ -195,8 +208,10 @@ DNSClientImpl::doUpdate(asiolink::IOService& io_service, // that TSIG should be used. if (tsig_key) { tsig_context_.reset(new TSIGContext(*tsig_key)); + tsig_key_name_ = tsig_key->getKeyName().toText(); } else { tsig_context_.reset(); + tsig_key_name_.clear(); } // A renderer is used by the toWire function which creates the on-wire data @@ -227,6 +242,24 @@ DNSClientImpl::doUpdate(asiolink::IOService& io_service, // Post the task to the task queue in the IO service. Caller will actually // run these tasks by executing IOService::run. io_service.post(io_fetch); + + // Update sent statistics. + incrStats("sent"); + if (tsig_key) { + incrStats("signed", false); + } else { + incrStats("unsigned", false); + } +} + +void +DNSClientImpl::incrStats(const std::string& stat, bool update_key) { + StatsMgr& mgr = StatsMgr::instance(); + mgr.addValue("global." + stat, static_cast(1)); + if (update_key && !tsig_key_name_.empty()) { + mgr.addValue(StatsMgr::generateName("key", tsig_key_name_, stat), + static_cast(1)); + } } DNSClient::DNSClient(D2UpdateMessagePtr& response_placeholder, @@ -250,7 +283,7 @@ DNSClient::doUpdate(asiolink::IOService& io_service, const uint16_t ns_port, D2UpdateMessage& update, const unsigned int wait, - const dns::TSIGKeyPtr& tsig_key) { + const D2TsigKeyPtr& tsig_key) { impl_->doUpdate(io_service, ns_addr, ns_port, update, wait, tsig_key); } diff --git a/src/bin/d2/dns_client.h b/src/bin/d2/dns_client.h index 41423be889..fb2412523d 100644 --- a/src/bin/d2/dns_client.h +++ b/src/bin/d2/dns_client.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2020 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2021 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -13,7 +13,7 @@ #include #include -#include +#include namespace isc { namespace d2 { @@ -138,7 +138,7 @@ 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 A pointer to an @c isc::dns::TSIGKey object that will + /// @param tsig_key A pointer to an @c D2TsigKeyPtr object that will /// (if not null) be used to sign the DNS Update message and verify the /// response. void doUpdate(asiolink::IOService& io_service, @@ -146,7 +146,7 @@ public: const uint16_t ns_port, D2UpdateMessage& update, const unsigned int wait, - const dns::TSIGKeyPtr& tsig_key = dns::TSIGKeyPtr()); + const D2TsigKeyPtr& tsig_key = D2TsigKeyPtr()); private: DNSClientImpl* impl_; ///< Pointer to DNSClient implementation. diff --git a/src/bin/d2/nc_trans.h b/src/bin/d2/nc_trans.h index a25b5da57f..503d9a0f83 100644 --- a/src/bin/d2/nc_trans.h +++ b/src/bin/d2/nc_trans.h @@ -12,8 +12,8 @@ #include #include #include +#include #include -#include #include #include @@ -589,7 +589,7 @@ private: D2CfgMgrPtr cfg_mgr_; /// @brief Pointer to the TSIG key which should be used (if any). - dns::TSIGKeyPtr tsig_key_; + D2TsigKeyPtr tsig_key_; }; /// @brief Defines a pointer to a NameChangeTransaction. diff --git a/src/bin/d2/tests/d2_simple_parser_unittest.cc b/src/bin/d2/tests/d2_simple_parser_unittest.cc index 57777ec3f3..b236469cb9 100644 --- a/src/bin/d2/tests/d2_simple_parser_unittest.cc +++ b/src/bin/d2/tests/d2_simple_parser_unittest.cc @@ -641,7 +641,7 @@ TEST_F(TSIGKeyInfoParserTest, invalidEntry) { " \"digest-bits\": 120 , " " \"secret\": \"bogus\" " "}"; - PARSE_FAIL(config, "Cannot make TSIGKey: Incomplete input for base64:" + PARSE_FAIL(config, "Cannot make D2TsigKey: Incomplete input for base64:" " bogus (:1:1)"); } diff --git a/src/bin/d2/tests/d2_update_message_unittests.cc b/src/bin/d2/tests/d2_update_message_unittests.cc index 1320e377a3..092b23f9e6 100644 --- a/src/bin/d2/tests/d2_update_message_unittests.cc +++ b/src/bin/d2/tests/d2_update_message_unittests.cc @@ -583,18 +583,18 @@ TEST_F(D2UpdateMessageTest, toWireInvalidQRFlag) { TEST_F(D2UpdateMessageTest, validTSIG) { // Create a TSIG Key and context std::string secret("this key will match"); - TSIGKeyPtr right_key; + D2TsigKeyPtr right_key; ASSERT_NO_THROW(right_key.reset(new - TSIGKey(Name("right.com"), - TSIGKey::HMACMD5_NAME(), - secret.c_str(), secret.size()))); + D2TsigKey(Name("right.com"), + TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); - TSIGKeyPtr wrong_key; + D2TsigKeyPtr wrong_key; secret = "this key will not match"; ASSERT_NO_THROW(wrong_key.reset(new - TSIGKey(Name("wrong.com"), - TSIGKey::HMACMD5_NAME(), - secret.c_str(), secret.size()))); + D2TsigKey(Name("wrong.com"), + TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); // Build a request message @@ -658,9 +658,9 @@ TEST_F(D2UpdateMessageTest, allValidTSIG) { dns::Name key_name("test_key"); std::string secret("random text for secret"); for (int i = 0; i < algorithms.size(); ++i) { - dns::TSIGKey key(key_name, - TSIGKeyInfo::stringToAlgorithmName(algorithms[i]), - secret.c_str(), secret.size()); + D2TsigKey key(key_name, + TSIGKeyInfo::stringToAlgorithmName(algorithms[i]), + secret.c_str(), secret.size()); // Build a request message D2UpdateMessage msg; diff --git a/src/bin/d2/tests/dns_client_unittests.cc b/src/bin/d2/tests/dns_client_unittests.cc index a795f0db2e..85927cbe01 100644 --- a/src/bin/d2/tests/dns_client_unittests.cc +++ b/src/bin/d2/tests/dns_client_unittests.cc @@ -205,8 +205,8 @@ public: // signing with the wrong key. void TSIGReceiveHandler(udp::socket* socket, udp::endpoint* remote, size_t receive_length, - TSIGKeyPtr client_key, - TSIGKeyPtr server_key) { + D2TsigKeyPtr client_key, + D2TsigKeyPtr server_key) { TSIGContextPtr context; if (client_key) { @@ -413,7 +413,7 @@ public: // @param server_key TSIG key the "server" should use to sign the response. // If this is NULL, then client_key is used. // @param should_pass indicates if the test should pass. - void runTSIGTest(TSIGKeyPtr client_key, TSIGKeyPtr server_key, + void runTSIGTest(D2TsigKeyPtr client_key, D2TsigKeyPtr server_key, bool should_pass = true) { // Tell operator() method if we expect an invalid response. corrupt_response_ = !should_pass; @@ -485,18 +485,18 @@ TEST_F(DNSClientTest, invalidTimeout) { // Verifies that TSIG can be used to sign requests and verify responses. TEST_F(DNSClientTest, runTSIGTest) { std::string secret ("key number one"); - TSIGKeyPtr key_one; + D2TsigKeyPtr key_one; ASSERT_NO_THROW(key_one.reset(new - TSIGKey(Name("one.com"), - TSIGKey::HMACMD5_NAME(), - secret.c_str(), secret.size()))); + D2TsigKey(Name("one.com"), + TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); secret = "key number two"; - TSIGKeyPtr key_two; + D2TsigKeyPtr key_two; ASSERT_NO_THROW(key_two.reset(new - TSIGKey(Name("two.com"), - TSIGKey::HMACMD5_NAME(), - secret.c_str(), secret.size()))); - TSIGKeyPtr nokey; + D2TsigKey(Name("two.com"), + TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); + D2TsigKeyPtr nokey; // Should be able to send and receive with no keys. // Neither client nor server will attempt to sign or verify. diff --git a/src/bin/d2/tests/nc_test_utils.cc b/src/bin/d2/tests/nc_test_utils.cc index f4888d492a..358387bdc7 100644 --- a/src/bin/d2/tests/nc_test_utils.cc +++ b/src/bin/d2/tests/nc_test_utils.cc @@ -173,11 +173,11 @@ FauxServer::requestHandler(const boost::system::error_code& error, if (response_mode == INVALID_TSIG) { // Create a different key to sign the response. std::string secret ("key that doesn't match"); - dns::TSIGKeyPtr key; + D2TsigKeyPtr key; ASSERT_NO_THROW(key.reset(new - dns::TSIGKey(dns::Name("badkey"), - dns::TSIGKey::HMACMD5_NAME(), - secret.c_str(), secret.size()))); + D2TsigKey(dns::Name("badkey"), + dns::TSIGKey::HMACMD5_NAME(), + secret.c_str(), secret.size()))); context.reset(new dns::TSIGContext(*key)); } diff --git a/src/bin/d2/tests/nc_test_utils.h b/src/bin/d2/tests/nc_test_utils.h index 543004b627..4cfff4cb92 100644 --- a/src/bin/d2/tests/nc_test_utils.h +++ b/src/bin/d2/tests/nc_test_utils.h @@ -61,7 +61,7 @@ public: bool perpetual_receive_; // TSIG Key to use to verify requests and sign responses. If its // NULL TSIG is not used. - dns::TSIGKeyPtr tsig_key_; + D2TsigKeyPtr tsig_key_; /// @brief Constructor /// @@ -117,7 +117,7 @@ public: /// /// @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 D2TsigKeyPtr& tsig_key) { tsig_key_ = tsig_key; } }; diff --git a/src/bin/d2/tests/testdata/d2_cfg_tests.json b/src/bin/d2/tests/testdata/d2_cfg_tests.json index 7780e7913e..6cb087fa0a 100644 --- a/src/bin/d2/tests/testdata/d2_cfg_tests.json +++ b/src/bin/d2/tests/testdata/d2_cfg_tests.json @@ -702,7 +702,7 @@ #----- ,{ "description" : "D2.tsig-keys, invalid secret", -"logic-error" : "Cannot make TSIGKey: Incomplete input for base64: bogus (:1:62)", +"logic-error" : "Cannot make D2TsigKey: Incomplete input for base64: bogus (:1:62)", "data" : { "forward-ddns" : {}, diff --git a/src/lib/d2srv/d2_config.cc b/src/lib/d2srv/d2_config.cc index 40baf5f609..21c7bf4a5d 100644 --- a/src/lib/d2srv/d2_config.cc +++ b/src/lib/d2srv/d2_config.cc @@ -165,7 +165,7 @@ void TSIGKeyInfo::remakeKey() { try { // Since our secret value is base64 encoded already, we need to - // build the input string for the appropriate TSIGKey constructor. + // build the input string for the appropriate D2TsigKey constructor. // If secret isn't a valid base64 value, the constructor will throw. std::ostringstream stream; stream << dns::Name(name_).toText() << ":" @@ -175,9 +175,9 @@ TSIGKeyInfo::remakeKey() { stream << ":" << digestbits_; } - tsig_key_.reset(new dns::TSIGKey(stream.str())); + tsig_key_.reset(new D2TsigKey(stream.str())); } catch (const std::exception& ex) { - isc_throw(D2CfgError, "Cannot make TSIGKey: " << ex.what()); + isc_throw(D2CfgError, "Cannot make D2TsigKey: " << ex.what()); } } @@ -439,7 +439,7 @@ TSIGKeyInfoParser::parse(ConstElementPtr key_config) { } // Everything should be valid, so create the key instance. - // It is possible for the asiodns::dns::TSIGKey create to fail such as + // It is possible for the D2TsigKey constructor to fail such as // with an invalid secret content. TSIGKeyInfoPtr key_info; try { diff --git a/src/lib/d2srv/d2_config.h b/src/lib/d2srv/d2_config.h index 892cd9729b..6162ec5ea1 100644 --- a/src/lib/d2srv/d2_config.h +++ b/src/lib/d2srv/d2_config.h @@ -12,8 +12,8 @@ #include #include #include +#include #include -#include #include #include @@ -343,8 +343,8 @@ public: /// @brief Getter which returns the TSIG key used to sign and verify /// messages /// - /// @return const pointer reference to dns::TSIGKey. - const dns::TSIGKeyPtr& getTSIGKey() const { + /// @return const pointer reference to @c D2TsigKeyPtr + const D2TsigKeyPtr& getTSIGKey() const { return (tsig_key_); } @@ -397,7 +397,7 @@ private: uint32_t digestbits_; /// @brief The actual TSIG key. - dns::TSIGKeyPtr tsig_key_; + D2TsigKeyPtr tsig_key_; }; /// @brief Defines a pointer for TSIGKeyInfo instances. diff --git a/src/lib/d2srv/d2_tsig_key.cc b/src/lib/d2srv/d2_tsig_key.cc index 229f7c06b0..bee8b08adf 100644 --- a/src/lib/d2srv/d2_tsig_key.cc +++ b/src/lib/d2srv/d2_tsig_key.cc @@ -21,8 +21,6 @@ namespace d2 { set D2TsigKey::keyStats = { "sent", - "signed", -// "unsigned", "success", "timeout", "error" @@ -38,7 +36,18 @@ D2TsigKey::globalStats = { "error" }; -D2TsigKey::D2TsigKey(const std::string& key_name) : TSIGKey(key_name) { +D2TsigKey::D2TsigKey(const std::string& key_spec) : TSIGKey(key_spec) { + initStats(); +} + +D2TsigKey::D2TsigKey(const Name& key_name, const Name& algorithm_name, + const void* secret, size_t secret_len, size_t digestbits) + : TSIGKey(key_name, algorithm_name, secret, secret_len, digestbits) { + initStats(); +} + +void +D2TsigKey::initStats() { StatsMgr& stats_mgr = StatsMgr::instance(); const string& kname = getKeyName().toText(); for (const auto& name : keyStats) { diff --git a/src/lib/d2srv/d2_tsig_key.h b/src/lib/d2srv/d2_tsig_key.h index 615996ab91..3f37c4c078 100644 --- a/src/lib/d2srv/d2_tsig_key.h +++ b/src/lib/d2srv/d2_tsig_key.h @@ -26,8 +26,24 @@ public: /// /// Initialize the key statistics. /// - /// @param key_name Domain name of the key. - D2TsigKey(const std::string& key_name); + /// @param key_spec Specification of the key + /// (name:secret[:algorithm][:digestbits]) + explicit D2TsigKey(const std::string& key_spec); + + /// @brief Constructor. + /// + /// Initialize the key statistics. + /// + /// @param key_name The name of the key as a domain name. + /// @param algorithm_name The hash algorithm used for this key in the + /// form of domain name. + /// @param secret Point to a binary sequence of the shared secret to be + /// used for this key. + /// @param secret_len The size of the binary %data (@c secret) in bytes. + /// @param digestbits The number of bits to include in the digest + /// (0 means to include all) + D2TsigKey(const dns::Name& key_name, const dns::Name& algorithm_name, + const void* secret, size_t secret_len, size_t digestbits = 0); /// @brief Destructor. /// @@ -47,6 +63,10 @@ public: /// /// The list of global statistic names. static std::set globalStats; + +private: + /// @brief Initialize key statistics. + void initStats(); }; /// @brief Type of pointer to a D2 TSIG key. diff --git a/src/lib/d2srv/tests/d2_tsig_key_unittest.cc b/src/lib/d2srv/tests/d2_tsig_key_unittest.cc index c73ed1bb12..edb6aeba5f 100644 --- a/src/lib/d2srv/tests/d2_tsig_key_unittest.cc +++ b/src/lib/d2srv/tests/d2_tsig_key_unittest.cc @@ -42,7 +42,7 @@ public: /// @brief Check TSIG key life. TEST_F(D2TsigKeyTest, key) { // Statistics names. - ASSERT_EQ(5, D2TsigKey::keyStats.size()); + ASSERT_EQ(4, D2TsigKey::keyStats.size()); ASSERT_EQ(6, D2TsigKey::globalStats.size()); // Get the statistics manager. @@ -52,7 +52,7 @@ TEST_F(D2TsigKeyTest, key) { // Create a key. const string& key_spec = "foo.bar.::test"; D2TsigKeyPtr key(new D2TsigKey(key_spec)); - EXPECT_EQ(5, stat_mgr.count()); + EXPECT_EQ(4, stat_mgr.count()); // Get the 'sent' statistics. const string& stat_name = "key[foo.bar.].sent";