From: Thomas Markwalder Date: Thu, 26 Jun 2014 14:13:17 +0000 (-0400) Subject: [3436] D2 TSIGKeyInfo configuration errors now include position info. X-Git-Tag: trac3473_base~5^2~7 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=2d89d22a3f1cbbe7c1f0e6a116abfa96733586fc;p=thirdparty%2Fkea.git [3436] D2 TSIGKeyInfo configuration errors now include position info. Modified DCfgContextBase::getParam() variants to return the parameter's Element::Position. This makes it available during parsing. Modified TSIGKeyInfoParser::build to validate parameters and use position in error messages. --- diff --git a/src/bin/d2/d2_config.cc b/src/bin/d2/d2_config.cc index 45edc59895..7256092b05 100644 --- a/src/bin/d2/d2_config.cc +++ b/src/bin/d2/d2_config.cc @@ -356,36 +356,59 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) { std::string name; std::string algorithm; std::string secret; + std::vector pos; - // Fetch the key configuration's parsed scalar values from parser's - // local storage. - local_scalars_.getParam("name", name); - local_scalars_.getParam("algorithm", algorithm); - local_scalars_.getParam("secret", secret); + // Fetch the key's parsed scalar values from parser's local storage. + // All are required, if any are missing we'll throw. + try { + pos.push_back(local_scalars_.getParam("name", name)); + pos.push_back(local_scalars_.getParam("algorithm", algorithm)); + pos.push_back(local_scalars_.getParam("secret", secret)); + } catch (const std::exception& ex) { + isc_throw(D2CfgError, "TSIG Key incomplete : " << ex.what() + << " : " << key_config->getPosition()); + } // Name cannot be blank. if (name.empty()) { - isc_throw(D2CfgError, "TSIG Key Info must specify name"); + isc_throw(D2CfgError, "TSIG key must specify name : " << pos[0]); } - // Algorithm cannot be blank. - if (algorithm.empty()) { - isc_throw(D2CfgError, "TSIG Key Info must specify algorithm"); + // Currently, the premise is that key storage is always empty prior to + // parsing so we are always adding keys never replacing them. Duplicates + // are not allowed and should be flagged as a configuration error. + if (keys_->find(name) != keys_->end()) { + isc_throw(D2CfgError, "Duplicate TSIG key name specified : " << name + << " : " << pos[0]); + } + + // Algorithm must be valid. + try { + TSIGKeyInfo::stringToAlgorithmName(algorithm); + } catch (const std::exception& ex) { + isc_throw(D2CfgError, "TSIG key invalid algorithm : " + << algorithm << " : " << pos[1]); } // Secret cannot be blank. + // Cryptolink lib doesn't offer anyway to validate these. As long as it + // isn't blank we'll accept it. If the content is bad, the call to in + // TSIGKeyInfo::remakeKey() made in the TSIGKeyInfo ctor will throw. + // We'll deal with that below. if (secret.empty()) { - isc_throw(D2CfgError, "TSIG Key Info must specify secret"); + isc_throw(D2CfgError, "TSIG key must specify secret : " << pos[2]); } - // Currently, the premise is that key storage is always empty prior to - // parsing so we are always adding keys never replacing them. Duplicates - // are not allowed and should be flagged as a configuration error. - if (keys_->find(name) != keys_->end()) { - isc_throw(D2CfgError, "Duplicate TSIG key specified:" << name); - } + // Everything should be valid, so create the key instance. + // It is possible for the asiodns::dns::TSIGKey create to fail such as + // with an invalid secret content. + TSIGKeyInfoPtr key_info; + try { + key_info.reset(new TSIGKeyInfo(name, algorithm, secret)); + } catch (const std::exception& ex) { + isc_throw(D2CfgError, ex.what() << " : " << key_config->getPosition()); - TSIGKeyInfoPtr key_info(new TSIGKeyInfo(name, algorithm, secret)); + } // Add the new TSIGKeyInfo to the key storage. (*keys_)[name]=key_info; diff --git a/src/bin/d2/d_cfg_mgr.cc b/src/bin/d2/d_cfg_mgr.cc index 7743b4291e..a6e61bf0e2 100644 --- a/src/bin/d2/d_cfg_mgr.cc +++ b/src/bin/d2/d_cfg_mgr.cc @@ -52,43 +52,51 @@ DCfgContextBase::DCfgContextBase(const DCfgContextBase& rhs): string_values_(new StringStorage(*(rhs.string_values_))) { } -void +const data::Element::Position& DCfgContextBase::getParam(const std::string& name, bool& value, bool optional) { try { value = boolean_values_->getParam(name); + return (boolean_values_->getPosition(name)); } catch (DhcpConfigError& ex) { // If the parameter is not optional, re-throw the exception. if (!optional) { throw; } } -} + return (data::Element::ZERO_POSITION()); +} -void +const data::Element::Position& DCfgContextBase::getParam(const std::string& name, uint32_t& value, bool optional) { try { value = uint32_values_->getParam(name); + return (uint32_values_->getPosition(name)); } catch (DhcpConfigError& ex) { // If the parameter is not optional, re-throw the exception. if (!optional) { throw; } } + + return (data::Element::ZERO_POSITION()); } -void +const data::Element::Position& DCfgContextBase::getParam(const std::string& name, std::string& value, bool optional) { try { value = string_values_->getParam(name); + return (string_values_->getPosition(name)); } catch (DhcpConfigError& ex) { // If the parameter is not optional, re-throw the exception. if (!optional) { throw; } } + + return (data::Element::ZERO_POSITION()); } DCfgContextBase::~DCfgContextBase() { diff --git a/src/bin/d2/d_cfg_mgr.h b/src/bin/d2/d_cfg_mgr.h index eee07e89a4..9d5817b4dd 100644 --- a/src/bin/d2/d_cfg_mgr.h +++ b/src/bin/d2/d_cfg_mgr.h @@ -80,9 +80,14 @@ public: /// will not throw if the parameter is not found in the context. The /// contents of the output parameter, value, will not be altered. /// It defaults to false if not specified. + /// + /// @return The parameter's element's position information if found, + /// otherwise it returns isc::data::Element::ZERO_POSITION(). + /// /// @throw throws DhcpConfigError if the context does not contain the /// parameter and optional is false. - void getParam(const std::string& name, bool& value, bool optional=false); + const data::Element::Position& + getParam(const std::string& name, bool& value, bool optional=false); /// @brief Fetches the value for a given uint32_t configuration parameter /// from the context. @@ -93,9 +98,14 @@ public: /// @param optional if true, the parameter is optional and the method /// will not throw if the parameter is not found in the context. The /// contents of the output parameter, value, will not be altered. + /// + /// @return The parameter's element's position information if found, + /// otherwise it returns isc::data::Element::ZERO_POSITION(). + /// /// @throw throws DhcpConfigError if the context does not contain the /// parameter and optional is false. - void getParam(const std::string& name, uint32_t& value, + const data::Element::Position& + getParam(const std::string& name, uint32_t& value, bool optional=false); /// @brief Fetches the value for a given string configuration parameter @@ -107,9 +117,14 @@ public: /// @param optional if true, the parameter is optional and the method /// will not throw if the parameter is not found in the context. The /// contents of the output parameter, value, will not be altered. + /// + /// @return The parameter's element's position information if found, + /// otherwise it returns isc::data::Element::ZERO_POSITION(). + /// /// @throw throws DhcpConfigError if the context does not contain the /// parameter and optional is false. - void getParam(const std::string& name, std::string& value, + const data::Element::Position& + getParam(const std::string& name, std::string& value, bool optional=false); /// @brief Fetches the Boolean Storage. Typically used for passing diff --git a/src/bin/d2/tests/d_cfg_mgr_unittests.cc b/src/bin/d2/tests/d_cfg_mgr_unittests.cc index d67d15e8d4..f57f2f7640 100644 --- a/src/bin/d2/tests/d_cfg_mgr_unittests.cc +++ b/src/bin/d2/tests/d_cfg_mgr_unittests.cc @@ -439,4 +439,50 @@ TEST_F(DStubCfgMgrTest, rollBackTest) { EXPECT_TRUE(object); } +// Tests that configuration element position is returned by getParam variants. +TEST_F(DStubCfgMgrTest, paramPosition) { + // Create a configuration with one of each scalar types. We end them + // with line feeds so we can test position value. + string config = "{ \"bool_test\": true , \n" + " \"uint32_test\": 77 , \n" + " \"string_test\": \"hmmm chewy\" }"; + ASSERT_TRUE(fromJSON(config)); + + // Verify that the configuration parses without error. + answer_ = cfg_mgr_->parseConfig(config_set_); + ASSERT_TRUE(checkAnswer(0)); + DStubContextPtr context = getStubContext(); + ASSERT_TRUE(context); + + // Verify that the boolean parameter was parsed correctly by retrieving + // its value from the context. + bool actual_bool = false; + isc::data::Element::Position pos; + EXPECT_NO_THROW(pos = context->getParam("bool_test", actual_bool)); + EXPECT_EQ(true, actual_bool); + EXPECT_EQ(1, pos.line_); + + // Verify that the uint32 parameter was parsed correctly by retrieving + // its value from the context. + uint32_t actual_uint32 = 0; + EXPECT_NO_THROW(pos = context->getParam("uint32_test", actual_uint32)); + EXPECT_EQ(77, actual_uint32); + EXPECT_EQ(2, pos.line_); + + // Verify that the string parameter was parsed correctly by retrieving + // its value from the context. + std::string actual_string = ""; + EXPECT_NO_THROW(pos = context->getParam("string_test", actual_string)); + EXPECT_EQ("hmmm chewy", actual_string); + EXPECT_EQ(3, pos.line_); + + // Verify that an optional parameter that is not defined, returns the + // zero position. + pos = isc::data::Element::ZERO_POSITION(); + EXPECT_NO_THROW(pos = context->getParam("bogus_value", + actual_string, true)); + EXPECT_EQ(pos.file_, isc::data::Element::ZERO_POSITION().file_); +} + + } // end of anonymous namespace