From: Thomas Markwalder Date: Tue, 10 Jul 2018 17:31:49 +0000 (-0400) Subject: [5680] kea-dhcp4 now uses hostname sanititzer when configured for it X-Git-Tag: ha_phase2~34 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1825ead45ec9aa74a5951ffd04f93d4e3c93ed88;p=thirdparty%2Fkea.git [5680] kea-dhcp4 now uses hostname sanititzer when configured for it src/bin/dhcp4/dhcp4_srv.cc Dhcpv4Srv::processHostnameOption() - sanitizes client hostname if configured to do so src/bin/dhcp4/tests/fqdn_unittest.cc TEST_F(NameDhcpv4SrvTest, sanitizeHostname) - new test to verify hostname sanitizing works as expected src/lib/dhcpsrv/d2_client_cfg.h D2ClientConfig::getHostnameSanitizer() added missing getter src/lib/dhcpsrv/tests/d2_client_unittest.cc TEST(D2ClientConfigTest, constructorsAndAccessors) - updated to verify hostname sanitizing stuff src/lib/util/strutil.cc fixed regex compilation issue --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 4f5c8fa521..f13723dfdc 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -1779,11 +1779,9 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) { .arg(ex.getQuery()->getLabel()); return; } - // Copy construct the hostname provided by the client. It is entirely - // possible that we will use the hostname option provided by the client - // to perform the DNS update and we will send the same option to him to - // indicate that we accepted this hostname. - OptionStringPtr opt_hostname_resp(new OptionString(*opt_hostname)); + + // Stores the value we eventually use, so we can send it back. + OptionStringPtr opt_hostname_resp; // The hostname option may be unqualified or fully qualified. The lab_count // holds the number of labels for the name. The number of 1 means that @@ -1792,7 +1790,6 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) { // By checking the number of labels present in the hostname we may infer // whether client has sent the fully qualified or unqualified hostname. - if ((replace_name_mode == D2ClientConfig::RCM_ALWAYS || replace_name_mode == D2ClientConfig::RCM_WHEN_PRESENT) || label_count < 2) { @@ -1805,17 +1802,28 @@ Dhcpv4Srv::processHostnameOption(Dhcpv4Exchange& ex) { /// point. /// For now, we just remain liberal and expect that the DNS will handle /// conversion if needed and possible. - opt_hostname_resp->setValue("."); - - } else if (label_count == 2) { - // If there are two labels, it means that the client has specified - // the unqualified name. We have to concatenate the unqualified name - // with the domain name. The false value passed as a second argument - // indicates that the trailing dot should not be appended to the - // hostname. We don't want to append the trailing dot because - // we don't know whether the hostname is partial or not and some - // clients do not handle the hostnames with the trailing dot. - opt_hostname_resp->setValue(d2_mgr.qualifyName(hostname, false)); + opt_hostname_resp.reset(new OptionString(Option::V4, DHO_HOST_NAME, ".")); + } else { + // Sanitize the name the client sent us, if we're configured to do so. + isc::util::str::StringSanitizerPtr sanitizer = d2_mgr.getD2ClientConfig() + ->getHostnameSanitizer(); + if (sanitizer) { + hostname = sanitizer->scrub(hostname); + } + + if (label_count == 2) { + // If there are two labels, it means that the client has specified + // the unqualified name. We have to concatenate the unqualified name + // with the domain name. The false value passed as a second argument + // indicates that the trailing dot should not be appended to the + // hostname. We don't want to append the trailing dot because + // we don't know whether the hostname is partial or not and some + // clients do not handle the hostnames with the trailing dot. + opt_hostname_resp.reset(new OptionString(Option::V4, DHO_HOST_NAME, + d2_mgr.qualifyName(hostname, false))); + } else { + opt_hostname_resp.reset(new OptionString(Option::V4, DHO_HOST_NAME, hostname)); + } } LOG_DEBUG(ddns4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_RESPONSE_HOSTNAME_DATA) diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index f8dd12866f..f13e9929da 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2018 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 @@ -29,6 +29,7 @@ namespace { /// @brief Set of JSON configurations used by the FQDN tests. const char* CONFIGS[] = { + // 0 "{ \"interfaces-config\": {" " \"interfaces\": [ \"*\" ]" "}," @@ -53,6 +54,7 @@ const char* CONFIGS[] = { "\"qualifying-suffix\": \"\"" "}" "}", + // 1 "{ \"interfaces-config\": {" " \"interfaces\": [ \"*\" ]" "}," @@ -77,6 +79,7 @@ const char* CONFIGS[] = { "\"qualifying-suffix\": \"fake-suffix.isc.org.\"" "}" "}", + // 2 // Simple config with DDNS updates disabled. Note pool is one address // large to ensure we get a specific address back. "{ \"interfaces-config\": {" @@ -93,6 +96,7 @@ const char* CONFIGS[] = { "\"qualifying-suffix\": \"fake-suffix.isc.org.\"" "}" "}", + // 3 // Simple config with DDNS updates enabled. Note pool is one address // large to ensure we get a specific address back. "{ \"interfaces-config\": {" @@ -109,6 +113,7 @@ const char* CONFIGS[] = { "\"qualifying-suffix\": \"fake-suffix.isc.org.\"" "}" "}", + // 4 // Configuration which disables DNS updates but contains a reservation // for a hostname. Reserved hostname should be assigned to a client if // the client includes it in the Parameter Request List option. @@ -136,6 +141,7 @@ const char* CONFIGS[] = { "\"qualifying-suffix\": \"\"" "}" "}", + // 5 // Configuration which disables DNS updates but contains a reservation // for a hostname and the qualifying-suffix which should be appended to // the reserved hostname in the Hostname option returned to a client. @@ -162,7 +168,35 @@ const char* CONFIGS[] = { "\"enable-updates\": false," "\"qualifying-suffix\": \"example.isc.org\"" "}" - "}" + "}", + // 6 + // Configuration which enables DNS updates and hostname sanitization + "{ \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + "}," + "\"valid-lifetime\": 3000," + "\"subnet4\": [ { " + " \"subnet\": \"10.0.0.0/24\", " + " \"id\": 1," + " \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ]," + " \"option-data\": [ {" + " \"name\": \"routers\"," + " \"data\": \"10.0.0.200,10.0.0.201\"" + " } ]," + " \"reservations\": [" + " {" + " \"hw-address\": \"aa:bb:cc:dd:ee:ff\"," + " \"hostname\": \"unique-xxx-host.example.org\"" + " }" + " ]" + " }]," + "\"dhcp-ddns\": {" + "\"enable-updates\": true," + "\"hostname-char-set\" : \"[^A-Za-z0-9.-]\"," + "\"hostname-char-replacement\" : \"x\"," + "\"qualifying-suffix\": \"example.org\"" + "}" + "}", }; class NameDhcpv4SrvTest : public Dhcpv4SrvTest { @@ -425,7 +459,8 @@ public: // Test that the server's processes the hostname (or lack thereof) // in a client request correctly, according to the replace-client-name - // mode configuration parameter. + // mode configuration parameter. We include hostname sanitizer to ensure + // it does not interfere with name replacement. // // @param mode - value to use for replace-client-name // @param client_name_flag - specifies whether or not the client request @@ -449,6 +484,8 @@ public: "\"dhcp-ddns\": {" "\"enable-updates\": true," "\"qualifying-suffix\": \"fake-suffix.isc.org.\"," + "\"hostname-char-set\": \"[^A-Za-z0-9.-]\"," + "\"hostname-char-replacement\": \"x\"," "\"replace-client-name\": \"%s\"" "}}"; @@ -1662,4 +1699,66 @@ TEST_F(NameDhcpv4SrvTest, replaceClientNameModeTest) { CLIENT_NAME_PRESENT, NAME_NOT_REPLACED); } +TEST_F(NameDhcpv4SrvTest, sanitizeHost) { + Dhcp4Client client(Dhcp4Client::SELECTING); + + // Configure DHCP server. + configure(CONFIGS[6], *client.getServer()); + + // Make sure that DDNS is enabled. + ASSERT_TRUE(CfgMgr::instance().ddnsEnabled()); + ASSERT_NO_THROW(client.getServer()->startD2()); + + struct Scenario { + std::string description_; + std::string original_; + std::string sanitized_; + }; + + std::vector scenarios = { + { + "unqualified host name with invalid characters", + "one-&$_-host", + "one-xxx-host.example.org" + }, + { + "qualified host name with invalid characters", + "two-&$_-host.other.org", + "two-xxx-host.other.org" + }, + { + "unqualified host name with all valid characters", + "three-ok-host", + "three-ok-host.example.org" + }, + { + "qualified host name with valid characters", + "four-ok-host.other.org", + "four-ok-host.other.org" + } + }; + + Pkt4Ptr resp; + OptionStringPtr hostname; + for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) { + SCOPED_TRACE((*scenario).description_); + { + // Set the hostname option. + ASSERT_NO_THROW(client.includeHostname((*scenario).original_)); + + // Send the DHCPDISCOVER and make sure that the server responded. + ASSERT_NO_THROW(client.doDiscover()); + resp = client.getContext().response_; + ASSERT_TRUE(resp); + ASSERT_EQ(DHCPOFFER, static_cast(resp->getType())); + + // Make sure the response hostname is what we expect. + hostname = boost::dynamic_pointer_cast(resp->getOption(DHO_HOST_NAME)); + ASSERT_TRUE(hostname); + EXPECT_EQ((*scenario).sanitized_, hostname->getValue()); + } + } +} + + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/d2_client_cfg.h b/src/lib/dhcpsrv/d2_client_cfg.h index 5da4af6004..e69015388f 100644 --- a/src/lib/dhcpsrv/d2_client_cfg.h +++ b/src/lib/dhcpsrv/d2_client_cfg.h @@ -209,6 +209,12 @@ public: return(hostname_char_replacement_); } + /// @brief Return pointer to compiled regular expression string sanitizer + /// Will be empty if hostname-char-set is empty. + util::str::StringSanitizerPtr getHostnameSanitizer() const { + return(hostname_sanitizer_); + } + /// @brief Compares two D2ClientConfigs for equality bool operator == (const D2ClientConfig& other) const; diff --git a/src/lib/dhcpsrv/tests/d2_client_unittest.cc b/src/lib/dhcpsrv/tests/d2_client_unittest.cc index dbd397318a..6a5a450fea 100644 --- a/src/lib/dhcpsrv/tests/d2_client_unittest.cc +++ b/src/lib/dhcpsrv/tests/d2_client_unittest.cc @@ -128,6 +128,10 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) { EXPECT_EQ(d2_client_config->getGeneratedPrefix(), generated_prefix); EXPECT_EQ(d2_client_config->getQualifyingSuffix(), qualifying_suffix); + EXPECT_EQ(d2_client_config->getHostnameCharSet(), hostname_char_set); + EXPECT_EQ(d2_client_config->getHostnameCharReplacement(), hostname_char_replacement); + EXPECT_TRUE(d2_client_config->getHostnameSanitizer()); + ASSERT_TRUE(d2_client_config->getContext()); EXPECT_EQ(d2_client_config->getContext()->str(), user_context); diff --git a/src/lib/util/strutil.cc b/src/lib/util/strutil.cc index 7d51bbcd04..71b8cf90ee 100644 --- a/src/lib/util/strutil.cc +++ b/src/lib/util/strutil.cc @@ -334,7 +334,7 @@ public: try { std::regex_replace(std::ostream_iterator(result), original.begin(), original.end(), - scrub_expr, char_replacement_); + scrub_exp_, char_replacement_); } catch (const std::exception& ex) { isc_throw(isc::BadValue, "replacing '" << char_set_ << "' with '" << char_replacement_ << "' in '" << original << "' failed: ,"