From: Francis Dupont Date: Wed, 7 Nov 2018 15:01:09 +0000 (+0100) Subject: [66-authoritative-flag-in-kea] Cleanup code, added legacy unit test X-Git-Tag: 259-libyang-adapt-authoritative_base~7 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=2f272cd336614a90d8c89f89dc525a4e86f96b83;p=thirdparty%2Fkea.git [66-authoritative-flag-in-kea] Cleanup code, added legacy unit test --- diff --git a/ChangeLog b/ChangeLog index ac6bd80160..d0e69f7bec 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,7 +1,8 @@ 1475. [func] sebschrader - Authoritative flag for DHCPv4 has been added. When enabled, it - alters how DHCPv4 server treats packets from unknown clients. This - lets two servers cooperate on the same link easier. + Add authoritative feature for DHCPv4 from ISC DHCP: requests from + unknown clients are dropped (default/previous behavior) or + answered with DHCPNAK (new behevior with new authoritative flag + set to true for the subnet). Patch proposed by Sebastian Schrader. (Gitlab #66, git tbd) 1474. [doc] godfryd diff --git a/doc/examples/kea4/advanced.json b/doc/examples/kea4/advanced.json index cee0f43273..00380e59e7 100644 --- a/doc/examples/kea4/advanced.json +++ b/doc/examples/kea4/advanced.json @@ -166,7 +166,7 @@ }, { // This particular subnet has the authoritative value changed. - // This casuses Kea to reply to requests with unknown IP addresses + // This causes Kea to reply to requests for unknown IP addresses // with a DHCPNAK message. "pools": [ { "pool": "192.0.5.100 - 192.0.5.200" } ], "subnet": "192.0.5.0/24", diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index c5c3c43054..c89f83b396 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -3232,15 +3232,16 @@ It is merely echoed by the server
Authoritative DHCPv4 Server Behavior The original DHCPv4 specification - (RFC 2131) + (RFC 2131) states that if a clients requests an address in the INIT-REBOOT state of which, the server has no knowledge of, the server must remain silent, - except if the server knows that the client requests an IP address from the - wrong network. - By default Kea follows the behavior of the ISC dhcpd instead of the - specification and also remains silent, if the client requests an IP - address from the wrong network, - because configuration information about a given network segment is not + except if the server knows that the client requests an IP + address from the wrong network. + By default Kea follows the behavior of the ISC dhcpd instead of + the specification and also remains silent, if the client + requests an IP address from the wrong network, because + configuration information about a given network segment is not known to be correct. Kea only rejects a client's DHCPREQUEST with a DHCPNAK message, if it already has a lease for the client, but with a different IP address. diff --git a/src/bin/dhcp4/dhcp4_log.h b/src/bin/dhcp4/dhcp4_log.h index 34a790cb39..2131f76225 100644 --- a/src/bin/dhcp4/dhcp4_log.h +++ b/src/bin/dhcp4/dhcp4_log.h @@ -105,7 +105,7 @@ extern isc::log::Logger packet4_logger; /// @brief Logger for options parser. /// /// This logger is used to issue log messages related to processing of the -/// DHCP options +/// DHCP options extern isc::log::Logger options4_logger; /// @brief Logger for Hostname or FQDN processing. diff --git a/src/bin/dhcp4/dhcp4_parser.cc b/src/bin/dhcp4/dhcp4_parser.cc index a6c16aac04..f875091cb2 100644 --- a/src/bin/dhcp4/dhcp4_parser.cc +++ b/src/bin/dhcp4/dhcp4_parser.cc @@ -3217,7 +3217,7 @@ namespace isc { namespace dhcp { case 577: #line 1971 "dhcp4_parser.yy" // lalr1.cc:856 { - yylhs.value.as< ElementPtr > () = ElementPtr(new StringElement("when-present", ctx.loc2pos(yystack_[0].location))); + yylhs.value.as< ElementPtr > () = ElementPtr(new StringElement("when-present", ctx.loc2pos(yystack_[0].location))); } #line 3223 "dhcp4_parser.cc" // lalr1.cc:856 break; diff --git a/src/bin/dhcp4/dhcp4_parser.yy b/src/bin/dhcp4/dhcp4_parser.yy index 1a90aeb793..2f409b25a1 100644 --- a/src/bin/dhcp4/dhcp4_parser.yy +++ b/src/bin/dhcp4/dhcp4_parser.yy @@ -1969,7 +1969,7 @@ replace_client_name: REPLACE_CLIENT_NAME { replace_client_name_value: WHEN_PRESENT { - $$ = ElementPtr(new StringElement("when-present", ctx.loc2pos(@1))); + $$ = ElementPtr(new StringElement("when-present", ctx.loc2pos(@1))); } | NEVER { $$ = ElementPtr(new StringElement("never", ctx.loc2pos(@1))); diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index f3f79c7b59..7aa26f2faf 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -2004,9 +2004,9 @@ Dhcpv4Srv::assignLease(Dhcpv4Exchange& ex) { } // If we know this client, check if his notion of the IP address is - // correct, if we don't know him check, if we are authoritative. - if ((known_client && (lease->addr_ != hint)) - || (!known_client && authoritative)) { + // correct, if we don't know him, check if we are authoritative. + if ((known_client && (lease->addr_ != hint)) || + (!known_client && authoritative)) { LOG_DEBUG(bad_packet4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_NAK_0002) .arg(query->getLabel()) diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index d81da588c8..93159a9c16 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -215,8 +215,10 @@ public: if (authoritative != (*subnet)->getAuthoritative()) { isc_throw(DhcpConfigError, "Subnet " << (*subnet)->toText() - << " has different authoritative setting " << (*subnet)->getAuthoritative() - << " than the shared-network itself: " << authoritative); + << " has different authoritative setting " + << (*subnet)->getAuthoritative() + << " than the shared-network itself: " + << authoritative); } // Let's collect the subnets in case we later find out the diff --git a/src/bin/dhcp4/tests/classify_unittest.cc b/src/bin/dhcp4/tests/classify_unittest.cc index 227abd7fa5..713fd8f4df 100644 --- a/src/bin/dhcp4/tests/classify_unittest.cc +++ b/src/bin/dhcp4/tests/classify_unittest.cc @@ -50,7 +50,7 @@ namespace { /// - 1 pool: 10.0.0.10-10.0.0.100 /// - the following classes defined: /// not (option[93].hex == 0x0009) -/// not member(), next-server set to 1.2.3.4 +/// not member(), next-server set to 1.2.3.4 /// option[93].hex == 0x0006 /// option[93].hex == 0x0001 /// or member(), set boot-file-name to pxelinux.0 diff --git a/src/bin/dhcp4/tests/dora_unittest.cc b/src/bin/dhcp4/tests/dora_unittest.cc index 102d6611f4..7ae1424c97 100644 --- a/src/bin/dhcp4/tests/dora_unittest.cc +++ b/src/bin/dhcp4/tests/dora_unittest.cc @@ -138,8 +138,15 @@ namespace { /// - Use for testing authoritative flag /// - 1 subnet: 10.0.0.0/24 /// - 1 pool: 10.0.0.10-10.0.0.100 -/// - authoritative flag is set to false, thus the server responds -/// with DHCPNAK to requests with unknown subnets. +/// - authoritative flag is set to true, thus the server responds +/// with DHCPNAK to requests from unknown clients. +/// +/// - Configuration 16: +/// - Use for testing authoritative flag +/// - 1 subnet: 10.0.0.0/24 +/// - 1 pool: 10.0.0.10-10.0.0.100 +/// - authoritative flag is set to false, thus the server does not +/// respond to requests from unknown clients. /// const char* DORA_CONFIGS[] = { // Configuration 0 @@ -506,10 +513,27 @@ const char* DORA_CONFIGS[] = { " \"interfaces\": [ \"*\" ]" "}," "\"valid-lifetime\": 600," + "\"authoritative\": true," + "\"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\"" + " } ]" + " } ]" + "}", + +// Configuration 16 + "{ \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + "}," + "\"valid-lifetime\": 600," + "\"authoritative\": false," "\"subnet4\": [ { " " \"subnet\": \"10.0.0.0/24\", " " \"id\": 1," - " \"authoritative\": true," " \"pools\": [ { \"pool\": \"10.0.0.10-10.0.0.100\" } ]," " \"option-data\": [ {" " \"name\": \"routers\"," @@ -823,7 +847,7 @@ TEST_F(DORATest, initRebootRequest) { // Test that the client in the INIT-REBOOT state can request the IP // address it has and the address is returned. Also, check that if -// if the client requests invalid address the server sends a DHCPNAK. +// if the client is unknown the server sends a DHCPNAK. TEST_F(DORATest, authoritative) { Dhcp4Client client(Dhcp4Client::SELECTING); // Configure DHCP server. @@ -891,14 +915,107 @@ TEST_F(DORATest, authoritative) { EXPECT_EQ(DHCPNAK, static_cast(resp->getType())); // Now let's fix the IP address. The client identifier is still - // invalid so the message should be dropped. + // invalid so the server still responds with DHCPNAK. + + ASSERT_NO_THROW(client.doRequest()); + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + resp = client.getContext().response_; + EXPECT_EQ(DHCPNAK, static_cast(resp->getType())); + + // Restore original client identifier. + client.includeClientId("11:22"); client.config_.lease_.addr_ = IOAddress("10.0.0.50"); + + // Try to request from a different HW address. This should be successful + // because the client identifier matches. + client.modifyHWAddr(); + ASSERT_NO_THROW(client.doRequest()); + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + resp = client.getContext().response_; + // Make sure that the server has responded with DHCPACK. + ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + // Make sure that the client has got the lease with the requested address. + ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText()); +} + +// Test that the client in the INIT-REBOOT state can request the IP +// address it has and the address is returned. Also, check that if +// if the client is unknown the request is dropped. +TEST_F(DORATest, notAuthoritative) { + Dhcp4Client client(Dhcp4Client::SELECTING); + // Configure DHCP server. + configure(DORA_CONFIGS[16], *client.getServer()); + client.includeClientId("11:22"); + // Obtain a lease from the server using the 4-way exchange. + ASSERT_NO_THROW(client.doDORA(boost::shared_ptr< + IOAddress>(new IOAddress("10.0.0.50")))); + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + Pkt4Ptr resp = client.getContext().response_; + // Make sure that the server has responded with DHCPACK. + ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + // Response must not be relayed. + EXPECT_FALSE(resp->isRelayed()); + // Make sure that the server id is present. + EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText()); + // Make sure that the client has got the lease with the requested address. + ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText()); + + // Client has a lease in the database. Let's transition the client + // to the INIT_REBOOT state so as the client can request the cached + // lease using the DHCPREQUEST message. + client.setState(Dhcp4Client::INIT_REBOOT); + ASSERT_NO_THROW(client.doRequest()); + + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + resp = client.getContext().response_; + // Make sure that the server has responded with DHCPACK. + ASSERT_EQ(DHCPACK, static_cast(resp->getType())); + // Response must not be relayed. + EXPECT_FALSE(resp->isRelayed()); + // Make sure that the server id is present. + EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText()); + // Make sure that the client has got the lease with the requested address. + ASSERT_EQ("10.0.0.50", client.config_.lease_.addr_.toText()); + + // Try to request a different address than the client has. The server + // should respond with DHCPNAK. + client.config_.lease_.addr_ = IOAddress("10.0.0.30"); ASSERT_NO_THROW(client.doRequest()); // Make sure that the server responded. ASSERT_TRUE(client.getContext().response_); resp = client.getContext().response_; EXPECT_EQ(DHCPNAK, static_cast(resp->getType())); + // Try to request another different address from an unknown subnet. + // The server should respond with DHCPNAK. + client.config_.lease_.addr_ = IOAddress("10.1.0.30"); + ASSERT_NO_THROW(client.doRequest()); + // Make sure that the server responded. + ASSERT_TRUE(client.getContext().response_); + resp = client.getContext().response_; + ASSERT_EQ(DHCPNAK, static_cast(resp->getType())); + + // Change client identifier. The server should treat the request + // as a request from unknown client and not respond (no DHCPNAK). + // Changed behavior vs authoritative! + client.includeClientId("12:34"); + client.config_.lease_.addr_ = IOAddress("10.1.0.30"); + ASSERT_NO_THROW(client.doRequest()); + // Make sure that the server did not respond. + EXPECT_FALSE(client.getContext().response_); + + // Now let's fix the IP address. The client identifier is still + // invalid so the message should be dropped (no DHCPNAK). + // Changed behavior vs authoritative! + client.config_.lease_.addr_ = IOAddress("10.0.0.50"); + ASSERT_NO_THROW(client.doRequest()); + // Make sure that the server did not respond. + EXPECT_FALSE(client.getContext().response_); + // Restore original client identifier. client.includeClientId("11:22"); client.config_.lease_.addr_ = IOAddress("10.0.0.50"); @@ -2117,7 +2234,7 @@ public: /// Recreates PgSQL schema for a test. DORAPgSQLTest() : DORATest() { db::test::destroyPgSQLSchema(); - db::test::createPgSQLSchema(); + db::test::createPgSQLSchema(); } /// @brief Destructor. diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 1d01fc690b..d03988ac56 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/src/lib/dhcpsrv/cfgmgr.h @@ -92,19 +92,6 @@ public: /// @param datadir New data directory. void setDataDir(const std::string& datadir); - /// @brief Sets whether server should NAK unknown clients in DHCPv4 - /// - /// @param echo should unknown clients be rejected or not - void authoritative(const bool enabled) { - authoritative_ = enabled; - } - - /// @brief Returns whether server should NAK requests for unknown leases - /// @return true if requests for unknown leases should be NAKed, false otherwise - bool authoritative() const { - return (authoritative_); - } - /// @brief Updates the DHCP-DDNS client configuration to the given value. /// /// Passes the new configuration to the D2ClientMgr instance, @@ -283,9 +270,6 @@ private: /// @brief directory where data files (e.g. server-id) are stored std::string datadir_; - /// Indicates whether v4 server should NAK requests for unknown addresses - bool authoritative_; - /// @brief Manages the DHCP-DDNS client and its configuration. D2ClientMgr d2_client_mgr_;