From: Thomas Markwalder Date: Mon, 15 Apr 2019 15:06:10 +0000 (-0400) Subject: [#484] Expands supported hex literal formats in option data X-Git-Tag: Kea-1.6.0-beta~255 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=251efcd5f518a215173845b22555276df0e0ffc6;p=thirdparty%2Fkea.git [#484] Expands supported hex literal formats in option data src/bin/dhcp4/tests/config_parser_unittest.cc TEST_F(Dhcp4ParserTest, optionDataInvalidHexLiterals) TEST_F(Dhcp4ParserTest, optionDataValidHexLiterals) - new tests src/bin/dhcp6/tests/config_parser_unittest.cc TEST_F(Dhcp6ParserTest, optionDataInvalidHexLiterals) TEST_F(Dhcp6ParserTest, optionDataValidHexLiterals) - new tests src/lib/dhcpsrv/parsers/option_data_parser.* OptionDataParser::createOption() - modified to use util::str::decodeFormattedHexString() src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc TEST_F(ParseConfigTest, hexOptionData) - new test src/lib/util/strutil.* decodeSeparatedHexString() - new function which accepts the octet separator as a parameter decodeFormattedHexString() - now detects either colons or or spaces as octet separators --- diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 1ee586fa2d..d492e43fb0 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -3357,55 +3357,60 @@ TEST_F(Dhcp4ParserTest, optionCodeZero) { testInvalidOptionParam("0", "code"); } -// Verify that option data which contains non hexadecimal characters -// is rejected by the configuration. -TEST_F(Dhcp4ParserTest, optionDataInvalidChar) { - // Option code 0 is reserved and should not be accepted - // by configuration parser. - testInvalidOptionParam("01020R", "data"); +// Verify that invalid hex literals for option data are detected. +TEST_F(Dhcp4ParserTest, optionDataInvalidHexLiterals) { + testInvalidOptionParam("01020R", "data"); // non hex digit + testInvalidOptionParam("0x01:02", "data"); // 0x prefix with colon separator + testInvalidOptionParam("0x01 02", "data"); // 0x prefix with space separator + testInvalidOptionParam("0X0102", "data"); // 0X upper case X in prefix + testInvalidOptionParam("01.02", "data"); // invalid separator } -// Verify that option data containing '0x' prefix is rejected -// by the configuration. -TEST_F(Dhcp4ParserTest, optionDataUnexpectedPrefix) { - // Option code 0 is reserved and should not be accepted - // by configuration parser. - testInvalidOptionParam("0x0102", "data"); -} +// Verify the valid forms hex literals in option data are supported. +TEST_F(Dhcp4ParserTest, optionDataValidHexLiterals) { -// Verify that either lower or upper case characters are allowed -// to specify the option data. -TEST_F(Dhcp4ParserTest, optionDataLowerCase) { - ConstElementPtr x; - std::string config = createConfigWithOption("0a0b0C0D", "data"); - ConstElementPtr json; - ASSERT_NO_THROW(json = parseDHCP4(config)); + std::vector valid_hexes = + { + "0a0b0C0D", // upper and lower case + "0A:0B:0C:0D", // colon seperator + "0A 0B 0C 0D", // space seperator + "A0B0C0D", // odd number of digits + "0xA0B0C0D" // 0x prefix + }; - EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); - checkResult(x, 0); + for (auto valid_hex : valid_hexes) { + ConstElementPtr x; + std::string config = createConfigWithOption(valid_hex, "data"); + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP4(config)); - Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()-> - getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.5")); - ASSERT_TRUE(subnet); - OptionContainerPtr options = subnet->getCfgOption()->getAll(DHCP4_OPTION_SPACE); - ASSERT_EQ(1, options->size()); + EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json)); + checkResult(x, 0); - // Get the search index. Index #1 is to search using option code. - const OptionContainerTypeIndex& idx = options->get<1>(); + Subnet4Ptr subnet = CfgMgr::instance().getStagingCfg()-> + getCfgSubnets4()->selectSubnet(IOAddress("192.0.2.5")); + ASSERT_TRUE(subnet); + OptionContainerPtr options = subnet->getCfgOption()->getAll(DHCP4_OPTION_SPACE); + ASSERT_EQ(1, options->size()); - // Get the options for specified index. Expecting one option to be - // returned but in theory we may have multiple options with the same - // code so we get the range. - std::pair range = - idx.equal_range(56); - // Expect single option with the code equal to 100. - ASSERT_EQ(1, std::distance(range.first, range.second)); - const uint8_t foo_expected[] = { - 0x0A, 0x0B, 0x0C, 0x0D - }; - // Check if option is valid in terms of code and carried data. - testOption(*range.first, 56, foo_expected, sizeof(foo_expected)); + // Get the search index. Index #1 is to search using option code. + const OptionContainerTypeIndex& idx = options->get<1>(); + + // Get the options for specified index. Expecting one option to be + // returned but in theory we may have multiple options with the same + // code so we get the range. + std::pair range = idx.equal_range(56); + // Expect single option with the code equal to 100. + ASSERT_EQ(1, std::distance(range.first, range.second)); + const uint8_t foo_expected[] = { 0x0A, 0x0B, 0x0C, 0x0D }; + + // Check if option is valid in terms of code and carried data. + testOption(*range.first, 56, foo_expected, sizeof(foo_expected)); + + // Clear configuration for the next pass. + resetConfiguration(); + } } // Verify that specific option object is returned for standard diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index e8a0d46319..64e1c95f26 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -3561,55 +3561,62 @@ TEST_F(Dhcp6ParserTest, optionCodeZero) { testInvalidOptionParam("0", "code"); } -// Verify that option data which contains non hexadecimal characters -// is rejected by the configuration. -TEST_F(Dhcp6ParserTest, optionDataInvalidChar) { - // Option code 0 is reserved and should not be accepted - // by configuration parser. - testInvalidOptionParam("01020R", "data"); +// Verify that invalid hex literals for option data are detected. +TEST_F(Dhcp6ParserTest, optionDataInvalidHexLiterals) { + testInvalidOptionParam("01020R", "data"); // non hex digit + testInvalidOptionParam("0x01:02", "data"); // 0x prefix with colon separator + testInvalidOptionParam("0x01 02", "data"); // 0x prefix with space separator + testInvalidOptionParam("0X0102", "data"); // 0X upper case X in prefix + testInvalidOptionParam("01.02", "data"); // invalid separator } -// Verify that option data containing '0x' prefix is rejected -// by the configuration. -TEST_F(Dhcp6ParserTest, optionDataUnexpectedPrefix) { - // Option code 0 is reserved and should not be accepted - // by configuration parser. - testInvalidOptionParam("0x0102", "data"); -} +// Verify the valid forms hex literals in option data are supported. +TEST_F(Dhcp6ParserTest, optionDataValidHexLiterals) { -// Verify that either lower or upper case characters are allowed -// to specify the option data. -TEST_F(Dhcp6ParserTest, optionDataLowerCase) { - ConstElementPtr x; - std::string config = createConfigWithOption("0a0b0C0D", "data"); - ConstElementPtr json = parseDHCP6(config); + std::vector valid_hexes = + { + "0a0b0C0D", // upper and lower case + "0A:0B:0C:0D", // colon seperator + "0A 0B 0C 0D", // space seperator + "A0B0C0D", // odd number of digits + "0xA0B0C0D" // 0x prefix + }; - EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); - checkResult(x, 0); + for (auto valid_hex : valid_hexes) { + ConstElementPtr x; + std::string config = createConfigWithOption(valid_hex, "data"); + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP6(config)); - Subnet6Ptr subnet = CfgMgr::instance().getStagingCfg()->getCfgSubnets6()-> - selectSubnet(IOAddress("2001:db8:1::5"), classify_); - ASSERT_TRUE(subnet); - OptionContainerPtr options = subnet->getCfgOption()->getAll(DHCP6_OPTION_SPACE); - ASSERT_EQ(1, options->size()); + EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json)); + checkResult(x, 0); - // Get the search index. Index #1 is to search using option code. - const OptionContainerTypeIndex& idx = options->get<1>(); + Subnet6Ptr subnet = CfgMgr::instance().getStagingCfg()->getCfgSubnets6()-> + selectSubnet(IOAddress("2001:db8:1::5"), classify_); + ASSERT_TRUE(subnet); + OptionContainerPtr options = subnet->getCfgOption()->getAll(DHCP6_OPTION_SPACE); + ASSERT_EQ(1, options->size()); - // Get the options for specified index. Expecting one option to be - // returned but in theory we may have multiple options with the same - // code so we get the range. - std::pair range = - idx.equal_range(D6O_SUBSCRIBER_ID); - // Expect single option with the code equal to 38. - ASSERT_EQ(1, std::distance(range.first, range.second)); - const uint8_t subid_expected[] = { - 0x0A, 0x0B, 0x0C, 0x0D - }; - // Check if option is valid in terms of code and carried data. - testOption(*range.first, D6O_SUBSCRIBER_ID, subid_expected, - sizeof(subid_expected)); + // Get the search index. Index #1 is to search using option code. + const OptionContainerTypeIndex& idx = options->get<1>(); + + // Get the options for specified index. Expecting one option to be + // returned but in theory we may have multiple options with the same + // code so we get the range. + std::pair range = + idx.equal_range(D6O_SUBSCRIBER_ID); + + // Expect single option with the code equal to 38. + ASSERT_EQ(1, std::distance(range.first, range.second)); + const uint8_t subid_expected[] = { 0x0A, 0x0B, 0x0C, 0x0D }; + + // Check if option is valid in terms of code and carried data. + testOption(*range.first, D6O_SUBSCRIBER_ID, subid_expected, sizeof(subid_expected)); + + // Clear configuration for the next pass. + resetConfiguration(); + } } // Verify that specific option object is returned for standard diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.cc b/src/lib/dhcpsrv/parsers/option_data_parser.cc index f447a9f757..d6cb037a3f 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.cc +++ b/src/lib/dhcpsrv/parsers/option_data_parser.cc @@ -296,13 +296,7 @@ OptionDataParser::createOption(ConstElementPtr option_data) { // Otherwise, the option data is specified as a string of // hexadecimal digits that we have to turn into binary format. try { - // The decodeHex function expects that the string contains an - // even number of digits. If we don't meet this requirement, - // we have to insert a leading 0. - if (!data_param.empty() && ((data_param.length() % 2) != 0)) { - data_param = data_param.insert(0, "0"); - } - util::encode::decodeHex(data_param, binary); + util::str::decodeFormattedHexString(data_param, binary); } catch (...) { isc_throw(DhcpConfigError, "option data is not a valid" << " string of hexadecimal digits: " << data_param diff --git a/src/lib/dhcpsrv/parsers/option_data_parser.h b/src/lib/dhcpsrv/parsers/option_data_parser.h index 75d8191e3b..d9897751af 100644 --- a/src/lib/dhcpsrv/parsers/option_data_parser.h +++ b/src/lib/dhcpsrv/parsers/option_data_parser.h @@ -84,13 +84,27 @@ private: template OptionDefinitionPtr findOptionDefinition(const std::string& option_space, const SearchKey& search_key) const; - /// @brief Create option instance. /// /// Creates an instance of an option and adds it to the provided /// options storage. If the option data parsed by \ref build function /// are invalid or insufficient this function emits an exception. /// + /// If the option data is given as a string containing a hexadecimal + /// literal, then it is converted into binary format. These literals + /// may contain upper and lower case digits. They may be octets + /// delimited by colons or spaces (octets may be 1 or 2 digits) + /// If not delimited octets then they must be a continous string of + /// digits with or without a "0x" prefix. Examples: + /// + /// -# ab:cd:ef - colon delimited + /// -# ab cd ef - space delimited + /// -# 0xabcdef - 0x prefixed (no delimiters) + /// -# abcdef - no prefix or delimeters + /// + /// A leading zero is assumed for odd number of digits + /// in an octet or continuous string. + /// /// @param option_data An element holding data for a single option being /// created. /// diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index 89d020e9ea..8f0d47c751 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -165,7 +166,6 @@ public: ParseConfigTest() :family_(AF_INET6) { reset_context(); - CfgMgr::instance().clear(); } ~ParseConfigTest() { @@ -452,10 +452,11 @@ public: /// @brief Wipes the contents of the context to allowing another parsing /// during a given test if needed. - void reset_context(){ + /// @param family protocol family to use durin the test, defaults to AF_INET6 + void reset_context(uint16_t family = AF_INET6){ // Note set context universe to V6 as it has to be something. CfgMgr::instance().clear(); - family_ = AF_INET6; + family_ = family; // Ensure no hooks libraries are loaded. HooksManager::unloadLibraries(); @@ -1443,6 +1444,49 @@ TEST_F(ParseConfigTest, commaCSVFormatOptionData) { cfg.runCfgOptionsTest(family_, expected); } +// Verifies that hex literals can support a variety of formats. +TEST_F(ParseConfigTest, hexOptionData) { + + // All of the following variants should parse correctly + // into the same two IPv4 addresses: 12.0.3.1 and 192.0.3.2 + std::vector valid_hexes = { + "0C000301C0000302", // even number + "C000301C0000302", // odd number + "0C 00 03 01 C0 00 03 02", // spaces + "0C:00:03:01:C0:00:03:02", // colons + "0x0C000301C0000302", // 0x + "C 0 3 1 C0 0 3 02", // odd or or even octets + "0x0c000301C0000302" // upper or lower case digits + }; + + for (auto hex_str : valid_hexes) { + ostringstream os; + os << + "{ \n" + " \"option-data\": [ { \n" + " \"name\": \"domain-name-servers\", \n" + " \"code \": 6, \n" + " \"space\": \"dhcp4\", \n" + " \"csv-format\": false, \n" + " \"data\": \"" << hex_str << "\" \n" + " } ] \n" + "} \n"; + + reset_context(AF_INET); + int rcode = 0; + ASSERT_NO_THROW(rcode = parseConfiguration(os.str(), true)); + EXPECT_EQ(0, rcode); + + Option4AddrLstPtr opt = boost::dynamic_pointer_cast + (getOptionPtr(DHCP4_OPTION_SPACE, 6)); + ASSERT_TRUE(opt); + ASSERT_EQ(2, opt->getAddresses().size()); + EXPECT_EQ("12.0.3.1", opt->getAddresses()[0].toText()); + EXPECT_EQ("192.0.3.2", opt->getAddresses()[1].toText()); + } +} + + /// The next set of tests check basic operation of the HooksLibrariesParser. // // Convenience function to set a configuration of zero or more hooks @@ -2638,6 +2682,8 @@ TEST_F(ParseConfigTest, defaultSharedNetwork6) { EXPECT_FALSE(network->getRapidCommit().get()); } + + // There's no test for ControlSocketParser, as it is tested in the DHCPv4 code // (see CtrlDhcpv4SrvTest.commandSocketBasic in // src/bin/dhcp4/tests/ctrl_dhcp4_srv_unittest.cc). diff --git a/src/lib/util/strutil.cc b/src/lib/util/strutil.cc index 71b8cf90ee..5c57275218 100644 --- a/src/lib/util/strutil.cc +++ b/src/lib/util/strutil.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2019 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 @@ -214,8 +214,14 @@ quotedStringToBinary(const std::string& quoted_string) { void decodeColonSeparatedHexString(const std::string& hex_string, std::vector& binary) { + decodeSeparatedHexString(hex_string, ":", binary); +} + +void +decodeSeparatedHexString(const std::string& hex_string, const std::string& sep, + std::vector& binary) { std::vector split_text; - boost::split(split_text, hex_string, boost::is_any_of(":"), + boost::split(split_text, hex_string, boost::is_any_of(sep), boost::algorithm::token_compress_off); std::vector binary_vec; @@ -225,7 +231,7 @@ decodeColonSeparatedHexString(const std::string& hex_string, // means that two consecutive colons were specified. This is not // allowed. if ((split_text.size() > 1) && split_text[i].empty()) { - isc_throw(isc::BadValue, "two consecutive colons specified in" + isc_throw(isc::BadValue, "two consecutive separators ('" << sep << "') specified in" " a decoded string '" << hex_string << "'"); // Between a colon we expect at most two characters. @@ -262,14 +268,16 @@ decodeColonSeparatedHexString(const std::string& hex_string, binary.swap(binary_vec); } + void decodeFormattedHexString(const std::string& hex_string, std::vector& binary) { // If there is at least one colon we assume that the string // comprises octets separated by colons (e.g. MAC address notation). if (hex_string.find(':') != std::string::npos) { - decodeColonSeparatedHexString(hex_string, binary); - + decodeSeparatedHexString(hex_string, ":", binary); + } else if (hex_string.find(' ') != std::string::npos) { + decodeSeparatedHexString(hex_string, " ", binary); } else { std::ostringstream s; diff --git a/src/lib/util/strutil.h b/src/lib/util/strutil.h index 11e0e8d12b..ead5fd21ba 100644 --- a/src/lib/util/strutil.h +++ b/src/lib/util/strutil.h @@ -1,4 +1,4 @@ -// Copyright (C) 2011-2018 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2019 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 @@ -219,17 +219,32 @@ tokenToNum(const std::string& num_token) { std::vector quotedStringToBinary(const std::string& quoted_string); -/// \brief Converts a string of hexadecimal digits with colons into -/// a vector. +/// \brief Converts a string of separated hexadecimal digits. +/// into a vector. /// -/// This function supports the following formats: -/// - yy:yy:yy:yy:yy -/// - y:y:y:y:y -/// - y:yy:yy:y:y +/// Octets may contain 1 or 2 digits: +/// +/// - yyyyyyyyyy +/// - yyyyy +/// - yyyyyyy /// /// If the decoded string doesn't match any of the supported formats, /// an exception is thrown. /// +/// \param hex_string Input string. +/// \param binary Vector receiving converted string into binary. +/// \throw isc::BadValue if the format of the input string is invalid. +void +decodeSeparatedHexString(const std::string& hex_string, + const std::string& sep, + std::vector& binary); + +/// \brief Converts a string of hexadecimal digits with colons into +/// a vector. +/// +/// Convenience method which calls @c decodeSeparatedHexString() passing +/// in a colon for the separator. + /// \param hex_string Input string. /// \param binary Vector receiving converted string into binary. /// \throw isc::BadValue if the format of the input string is invalid. @@ -240,9 +255,11 @@ decodeColonSeparatedHexString(const std::string& hex_string, /// \brief Converts a formatted string of hexadecimal digits into /// a vector. /// -/// This function supports formats supported by -/// @ref decodeColonSeparatedHexString and the following additional -/// formats: +/// This function supports the following formats: +/// +/// - yyyyyyyy where is either a colon or space, see +/// @c decodeSeparatedHexString +/// /// - yyyyyyyyyy /// - 0xyyyyyyyyyy /// diff --git a/src/lib/util/tests/strutil_unittest.cc b/src/lib/util/tests/strutil_unittest.cc index 2672521fb4..07f3d84121 100644 --- a/src/lib/util/tests/strutil_unittest.cc +++ b/src/lib/util/tests/strutil_unittest.cc @@ -431,6 +431,8 @@ void testFormatted(const std::string& input, TEST(StringUtilTest, decodeFormattedHexString) { // Colon separated. testFormatted("1:A7:B5:4:23", "01A7B50423"); + // Space separated. + testFormatted("1 A7 B5 4 23", "01A7B50423"); // No colons, even number of digits. testFormatted("17a534", "17A534"); // Odd number of digits. @@ -443,18 +445,27 @@ TEST(StringUtilTest, decodeFormattedHexString) { testFormatted("", ""); std::vector decoded; - // Whitespace. + // Dangling colon. + EXPECT_THROW(decodeFormattedHexString("0a:", decoded), + isc::BadValue); + // Dangling space. EXPECT_THROW(decodeFormattedHexString("0a ", decoded), isc::BadValue); - // Whitespace within a string. - EXPECT_THROW(decodeFormattedHexString("01 02", decoded), + // '0x' prefix and spaces. + EXPECT_THROW(decodeFormattedHexString("x01 02", decoded), isc::BadValue); // '0x' prefix and colons. EXPECT_THROW(decodeFormattedHexString("0x01:02", decoded), isc::BadValue); + // colon and spaces mixed + EXPECT_THROW(decodeFormattedHexString("01:02 03", decoded), + isc::BadValue); // Missing colon. EXPECT_THROW(decodeFormattedHexString("01:0203", decoded), isc::BadValue); + // Missing space. + EXPECT_THROW(decodeFormattedHexString("01 0203", decoded), + isc::BadValue); // Invalid prefix. EXPECT_THROW(decodeFormattedHexString("x0102", decoded), isc::BadValue);