From: Thomas Markwalder Date: Thu, 19 Jul 2018 19:36:30 +0000 (-0400) Subject: [5680] kea-dhcp4 supports client FQDN name sanitizing X-Git-Tag: ha_phase2~32 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=8ac1c13d7e3c948a0134e8b74496d930bceeb806;p=thirdparty%2Fkea.git [5680] kea-dhcp4 supports client FQDN name sanitizing src/lib/dhcpsrv/d2_client_mgr.h D2ClientMgr::adjustDomainName() - added logic to sanitize the inbound FQDN name when configured to do so src/lib/dhcpsrv/tests/d2_client_unittest.cc TEST(D2ClientMgr, sanitizeFqdnV4) TEST(D2ClientMgr, sanitizeFqdnV6) - new tests src/bin/dhcp4/tests/fqdn_unittest.cc TEST_F(NameDhcpv4SrvTest, sanitizeFqdn) - new test --- diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index f13e9929da..2c1c6857fb 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/src/bin/dhcp4/tests/fqdn_unittest.cc @@ -1699,6 +1699,8 @@ TEST_F(NameDhcpv4SrvTest, replaceClientNameModeTest) { CLIENT_NAME_PRESENT, NAME_NOT_REPLACED); } +// Verifies that setting hostname-char-set sanitizes Hostname option +// values received from clients. TEST_F(NameDhcpv4SrvTest, sanitizeHost) { Dhcp4Client client(Dhcp4Client::SELECTING); @@ -1760,5 +1762,74 @@ TEST_F(NameDhcpv4SrvTest, sanitizeHost) { } } +// Verifies that setting hostname-char-set sanitizes FQDN option +// values received from clients. +TEST_F(NameDhcpv4SrvTest, sanitizeFqdn) { + 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_; + Option4ClientFqdn::DomainNameType name_type_; + std::string sanitized_; + }; + + std::vector scenarios = { + { + "unqualified FQDN with invalid characters", + "one-&*_-host", + Option4ClientFqdn::PARTIAL, + "one-xxx-host.example.org." + }, + { + "qualified FQDN with invalid characters", + "two-&*_-host.other.org", + Option4ClientFqdn::FULL, + "two-xxx-host.other.org." + }, + { + "unqualified FQDN name with all valid characters", + "three-ok-host", + Option4ClientFqdn::PARTIAL, + "three-ok-host.example.org." + }, + { + "qualified FQDN name with valid characters", + "four-ok-host.other.org", + Option4ClientFqdn::FULL, + "four-ok-host.other.org." + } + }; + + Pkt4Ptr resp; + Option4ClientFqdnPtr fqdn; + for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) { + SCOPED_TRACE((*scenario).description_); + { + // Set the hostname option. + ASSERT_NO_THROW(client.includeHostname((*scenario).original_)); + ASSERT_NO_THROW(client.includeFQDN(0, (*scenario).original_, (*scenario).name_type_)); + + // 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 fqdn is what we expect. + fqdn = boost::dynamic_pointer_cast(resp->getOption(DHO_FQDN)); + ASSERT_TRUE(fqdn); + EXPECT_EQ((*scenario).sanitized_, fqdn->getDomainName()); + } + } +} + } // end of anonymous namespace diff --git a/src/lib/dhcpsrv/d2_client_mgr.h b/src/lib/dhcpsrv/d2_client_mgr.h index 7a2de10d4c..c4b4c67b31 100644 --- a/src/lib/dhcpsrv/d2_client_mgr.h +++ b/src/lib/dhcpsrv/d2_client_mgr.h @@ -1,4 +1,4 @@ -// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-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 @@ -16,12 +16,14 @@ #include #include +#include #include #include #include #include #include +#include namespace isc { namespace dhcp { @@ -238,6 +240,12 @@ public: /// If replace-client-name is false and the supplied name is a fully /// qualified name, set the server FQDN to the supplied name. /// + /// If hostname-char-set is not empty, the inbound name will be + /// sanitized. This is done by iterating over the domain name labels, + /// sanitizing each individually, and then concatenating them into a + /// new sanitized name. It is done this way to guard against the case + /// where the hostname-char-set does not protect dots from replacement. + /// /// @param fqdn FQDN option from which to get client (inbound) name /// @param fqdn_resp FQDN option to update with the adjusted name /// @tparam T FQDN Option class containing the FQDN data such as @@ -462,9 +470,33 @@ D2ClientMgr::adjustDomainName(const T& fqdn, T& fqdn_resp) { fqdn.getDomainName().empty()) { fqdn_resp.setDomainName("", T::PARTIAL); } else { + // Sanitize the name the client sent us, if we're configured to do so. + std::string client_name = fqdn.getDomainName(); + if (d2_client_config_->getHostnameSanitizer()) { + // We do not know if the sanitizer's regexp preserves dots, so + // we'll scrub it label by label. Yeah, lucky us. + // Using boost::split is simpler than using dns::Name::split() as + // that returns Names which have trailing dots etc. + std::vector labels; + boost::algorithm::split(labels, client_name, boost::is_any_of(".")); + std::stringstream ss; + for (auto label = labels.begin(); label != labels.end(); ++label ) { + if (label != labels.begin()) { + ss << "."; + } + + ss << d2_client_config_->getHostnameSanitizer()->scrub(*label); + } + + client_name = ss.str(); + } + // If the supplied name is partial, qualify it by adding the suffix. if (fqdn.getDomainNameType() == T::PARTIAL) { - fqdn_resp.setDomainName(qualifyName(fqdn.getDomainName(),true), T::FULL); + fqdn_resp.setDomainName(qualifyName(client_name,true), T::FULL); + } + else { + fqdn_resp.setDomainName(client_name, T::FULL); } } } diff --git a/src/lib/dhcpsrv/tests/d2_client_unittest.cc b/src/lib/dhcpsrv/tests/d2_client_unittest.cc index 6a5a450fea..9087e770d5 100644 --- a/src/lib/dhcpsrv/tests/d2_client_unittest.cc +++ b/src/lib/dhcpsrv/tests/d2_client_unittest.cc @@ -11,6 +11,7 @@ #include #include +#include #include using namespace std; @@ -1127,4 +1128,166 @@ TEST(D2ClientMgr, updateDirectionsV6) { // Response S=1, N=1 isn't possible. } +/// @brief Tests v4 FQDN name sanitizing +TEST(D2ClientMgr, sanitizeFqdnV4) { + D2ClientMgr mgr; + + // Create enabled configuration. + // replace-client-name is false, client passes in empty fqdn + D2ClientConfigPtr cfg; + ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true, + isc::asiolink::IOAddress("127.0.0.1"), 477, + isc::asiolink::IOAddress("127.0.0.1"), 478, + 1024, + dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON, + false, false, false, D2ClientConfig::RCM_NEVER, + "prefix", "suffix.com", "[^A-Za-z0-9-]", "x"))); + ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg)); + ASSERT_EQ(D2ClientConfig::RCM_NEVER, cfg->getReplaceClientNameMode()); + + struct Scenario { + std::string description_; + std::string client_name_; + Option4ClientFqdn::DomainNameType name_type_; + std::string expected_name_; + }; + + std::vector scenarios = { + { + "full FQDN, name unchanged", + "One.123.example.com.", + Option4ClientFqdn::FULL, + "One.123.example.com." + }, + { + "partial FQDN, name unchanged, but qualified", + "One.123", + Option4ClientFqdn::PARTIAL, + "One.123.suffix.com." + }, + { + "full FQDN, scrubbed", + "O#n^e.123.ex&a*mple.com.", + Option4ClientFqdn::FULL, + "Oxnxe.123.exxaxmple.com." + }, + { + "partial FQDN, scrubbed and qualified", + "One.1+2|3", + Option4ClientFqdn::PARTIAL, + "One.1x2x3.suffix.com." + }, + { + // Some chars, like parens, get escaped by lib::dns::Name + // when output via Name::getDomainName(). This means they'll + // get replaced by TWO replacment chars, if the backslash "\" + // is an invalid character per hostname-char-set. + "full FQDN, scrubbed with escaped char", + "One.123.exa(mple.com.", + Option4ClientFqdn::FULL, + // expect the ( to be replaced by two x's + "One.123.exaxxmple.com." + } + }; + + Option4ClientFqdnPtr request; + Option4ClientFqdnPtr response; + for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) { + SCOPED_TRACE((*scenario).description_); + { + request.reset(new Option4ClientFqdn(0, Option4ClientFqdn::RCODE_CLIENT(), + (*scenario).client_name_, + (*scenario).name_type_)); + + response.reset(new Option4ClientFqdn(*request)); + mgr.adjustDomainName(*request, *response); + EXPECT_EQ((*scenario).expected_name_, response->getDomainName()); + EXPECT_EQ(Option4ClientFqdn::FULL, response->getDomainNameType()); + } + } +} + +/// @brief Tests v6 FQDN name sanitizing +/// @todo This test currently verifies that Option6ClientFqdn::DomainName +/// downcases strings used to construct it. For some reason, currently +/// uknown, Option4ClientFqdn preserves the case, while Option6ClientFqdn +/// downcases it (see setDomainName() in both classes. See Trac #5700. +TEST(D2ClientMgr, sanitizeFqdnV6) { + D2ClientMgr mgr; + + // Create enabled configuration. + // replace-client-name is false, client passes in empty fqdn + D2ClientConfigPtr cfg; + ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true, + isc::asiolink::IOAddress("127.0.0.1"), 477, + isc::asiolink::IOAddress("127.0.0.1"), 478, + 1024, + dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON, + false, false, false, D2ClientConfig::RCM_NEVER, + "prefix", "suffix.com", "[^A-Za-z0-9-]", "x"))); + ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg)); + ASSERT_EQ(D2ClientConfig::RCM_NEVER, cfg->getReplaceClientNameMode()); + + struct Scenario { + std::string description_; + std::string client_name_; + Option6ClientFqdn::DomainNameType name_type_; + std::string expected_name_; + }; + + std::vector scenarios = { + { + "full FQDN, name unchanged", + "One.123.example.com.", + Option6ClientFqdn::FULL, + "one.123.example.com." + }, + { + "partial FQDN, name unchanged, but qualified", + "One.123", + Option6ClientFqdn::PARTIAL, + "one.123.suffix.com." + }, + { + "full FQDN, scrubbed", + "O#n^e.123.ex&a*mple.com.", + Option6ClientFqdn::FULL, + "oxnxe.123.exxaxmple.com." + }, + { + "partial FQDN, scrubbed and qualified", + "One.1+2|3", + Option6ClientFqdn::PARTIAL, + "one.1x2x3.suffix.com." + }, + { + // Some chars, like parens, get escaped by lib::dns::Name + // when output via Name::getDomainName(). This means they'll + // get replaced by TWO replacment chars, if the backslash "\" + // is an invalid character per hostname-char-set. + "full FQDN, scrubbed with escaped char", + "One.123.exa(mple.com.", + Option6ClientFqdn::FULL, + // expect the ( to be replaced by two x's + "one.123.exaxxmple.com." + } + }; + + Option6ClientFqdnPtr request; + Option6ClientFqdnPtr response; + for (auto scenario = scenarios.begin(); scenario != scenarios.end(); ++scenario) { + SCOPED_TRACE((*scenario).description_); + { + request.reset(new Option6ClientFqdn(0, (*scenario).client_name_, + (*scenario).name_type_)); + + response.reset(new Option6ClientFqdn(*request)); + mgr.adjustDomainName(*request, *response); + EXPECT_EQ((*scenario).expected_name_, response->getDomainName()); + EXPECT_EQ(Option6ClientFqdn::FULL, response->getDomainNameType()); + } + } +} + + } // end of anonymous namespace