From: Tomek Mrugalski Date: Thu, 10 Aug 2017 11:51:47 +0000 (+0200) Subject: [5280] Changes after review: X-Git-Tag: trac5124a_base~11 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=94ff2448c8cf6e3321f4d1b3666a2e2b736f6c50;p=thirdparty%2Fkea.git [5280] Changes after review: - added missing comments for couple handlers - duplicated leaseX-update check removed, unit-test added - added missing parameters in lease_mgr_unitest.cc - added sanity check for state when adding and updating leases - not possibly anymore to sneak v4 addresses when v6 is expected (and vice versa) --- diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc index 89ae6026bc..85eb442948 100644 --- a/src/hooks/dhcp/lease_cmds/lease_cmds.cc +++ b/src/hooks/dhcp/lease_cmds/lease_cmds.cc @@ -303,16 +303,88 @@ private: static ConstElementPtr lease6DelHandler(const string& command, ConstElementPtr args); + /// @brief lease4-update handler + /// + /// This command attempts to update existing IPv4 lease. The parameters + /// specified will replace existing lease. The only condition is that + /// the IP address must not change. If you want to change the IP address, + /// please use lease4-del and lease4-add instead. + /// + /// Example command: + /// { + /// "command": "lease4-update", + /// "arguments": { + /// "subnet-id": 44, + /// "ip-address": "192.0.2.1", + /// "hw-address": "1a:1b:1c:1d:1e:1f", + /// "hostname": "newhostname.example.org" + /// } + /// }; + /// + /// @param command - should be "lease4-update", but it is ignored + /// @param args arguments that describe the lease being updated. static ConstElementPtr lease4UpdateHandler(const string& command, ConstElementPtr args); - /// @brief Not implemented yet. + /// @brief lease6-update handler + /// + /// This command attempts to update existing IPv6 lease. The parameters + /// specified will replace existing lease. The only condition is that + /// the IP address must not change. If you want to change the IP address, + /// please use lease6-del and lease6-add instead. + /// + /// Example command: + /// { + /// "command": "lease6-update", + /// "arguments": { + /// "subnet-id": 66, + /// "ip-address": "2001:db8::1", + /// "iaid": 7654321, + /// "duid": "88:88:88:88:88:88:88:88", + /// "hostname": "newhostname.example.org" + /// } + /// }"; + /// + /// @param command - should be "lease6-update" (but it is ignored) + /// @param args arguments that describe the lease being updated. static ConstElementPtr lease6UpdateHandler(const string& command, ConstElementPtr args); + /// @brief lease4-wipe handler + /// + /// This commands attempts to remove all IPv4 leases from a specific + /// subnet. Currently the leases are removed from the database, + /// without any processing (like calling hooks or doing DDNS + /// cleanups). + /// + /// Example command: + /// {\n" + /// "command": "lease4-wipe",\n" + /// "arguments": {" + /// "subnet-id": 44 + /// }\n" + /// }"; + /// @param command - should be "lease4-wipe" (but is ignored) + /// @param args arguments that describe the lease being updated. static ConstElementPtr lease4WipeHandler(const string& command, ConstElementPtr args); + /// @brief lease6-wipe handler + /// + /// This commands attempts to remove all IPv4 leases from a specific + /// subnet. Currently the leases are removed from the database, + /// without any processing (like calling hooks or doing DDNS + /// cleanups). + /// + /// Example command: + /// {\n" + /// "command": "lease4-wipe",\n" + /// "arguments": {" + /// "subnet-id": 44 + /// }\n" + /// }"; + /// @param command - should be "lease4-wipe" (but is ignored) + /// @param args arguments that describe the lease being updated. static ConstElementPtr lease6WipeHandler(const string& command, ConstElementPtr args); @@ -321,10 +393,11 @@ private: /// See @ref Parameters class for detailed description of what is expected /// in the args structure. /// - /// @param args - arguments passed to command + /// @param v6 whether addresses allowed are v4 (false) or v6 (true) + /// @param args arguments passed to command /// @return parsed parameters /// @throw BadValue if input arguments don't make sense. - static Parameters getParameters(const ConstElementPtr& args); + static Parameters getParameters(bool v6, const ConstElementPtr& args); }; LeaseCmdsImpl::LeaseCmdsImpl() { @@ -435,7 +508,7 @@ LeaseCmdsImpl::leaseAddHandler(const std::string& name, } LeaseCmdsImpl::Parameters -LeaseCmdsImpl::getParameters(const ConstElementPtr& params) { +LeaseCmdsImpl::getParameters(bool v6, const ConstElementPtr& params) { Parameters x; if (!params || params->getType() != Element::map) { @@ -470,6 +543,14 @@ LeaseCmdsImpl::getParameters(const ConstElementPtr& params) { } x.addr = IOAddress(tmp->stringValue()); + + if ((v6 && !x.addr.isV6()) || (!v6 && !x.addr.isV4())) { + stringstream txt; + txt << "Invalid " << (v6 ? "IPv6" : "IPv4") + << " address specified: " << tmp->stringValue(); + isc_throw(BadValue, txt.str()); + } + x.query_type = Parameters::TYPE_ADDR; return (x); } @@ -537,7 +618,7 @@ LeaseCmdsImpl::leaseGetHandler(const std::string& name, ConstElementPtr params) Lease6Ptr lease6; bool v4 = (name == "lease4-get"); try { - p = getParameters(params); + p = getParameters(!v4, params); switch (p.query_type) { case Parameters::TYPE_ADDR: { @@ -607,7 +688,7 @@ LeaseCmdsImpl::lease4DelHandler(const std::string& , ConstElementPtr params) { Lease4Ptr lease4; IOAddress addr(IOAddress::IPV4_ZERO_ADDRESS()); try { - p = getParameters(params); + p = getParameters(false, params); switch (p.query_type) { case Parameters::TYPE_ADDR: { @@ -661,7 +742,7 @@ LeaseCmdsImpl::lease6DelHandler(const std::string& , ConstElementPtr params) { Lease6Ptr lease6; IOAddress addr(IOAddress::IPV6_ZERO_ADDRESS()); try { - p = getParameters(params); + p = getParameters(true, params); switch (p.query_type) { case Parameters::TYPE_ADDR: { @@ -726,14 +807,6 @@ LeaseCmdsImpl::lease4UpdateHandler(const string& , ConstElementPtr params) { // subnet-id is valid, etc) lease4 = parser.parse(config, params); - // Ok, now check if there is a lease to be updated. - Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(lease4->addr_); - if (!existing) { - stringstream tmp; - tmp << "There is no lease for address " << lease4->addr_ << ", can't update."; - return (createAnswer(CONTROL_RESULT_EMPTY, tmp.str())); - } - LeaseMgrFactory::instance().updateLease4(lease4); return (createAnswer(CONTROL_RESULT_SUCCESS, "IPv4 lease updated.")); @@ -758,15 +831,6 @@ LeaseCmdsImpl::lease6UpdateHandler(const string& , ConstElementPtr params) { // subnet-id is valid, etc) lease6 = parser.parse(config, params); - // Ok, now check if there is a lease to be updated. - Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(lease6->type_, - lease6->addr_); - if (!existing) { - stringstream tmp; - tmp << "There is no lease for address " << lease6->addr_ << ", can't update."; - return (createAnswer(CONTROL_RESULT_EMPTY, tmp.str())); - } - LeaseMgrFactory::instance().updateLease6(lease6); return (createAnswer(CONTROL_RESULT_SUCCESS, "IPv6 lease updated.")); @@ -779,11 +843,10 @@ ConstElementPtr LeaseCmdsImpl::lease4WipeHandler(const string& /*cmd*/, ConstElementPtr params) { try { - // We need the lease to be specified. + // The subnet-id is a mandatory parameter. if (!params) { isc_throw(isc::BadValue, "no parameters specified for lease4-wipe command"); } - SimpleParser parser; SubnetID id = parser.getUint32(params, "subnet-id"); @@ -802,11 +865,10 @@ ConstElementPtr LeaseCmdsImpl::lease6WipeHandler(const string& /*cmd*/, ConstElementPtr params) { try { - // We need the lease to be specified. + // The subnet-id is a mandatory parameter. if (!params) { isc_throw(isc::BadValue, "no parameters specified for lease6-wipe command"); } - SimpleParser parser; SubnetID id = parser.getUint32(params, "subnet-id"); diff --git a/src/hooks/dhcp/lease_cmds/lease_parser.cc b/src/hooks/dhcp/lease_cmds/lease_parser.cc index 67b7a43c3e..b0288002d2 100644 --- a/src/hooks/dhcp/lease_cmds/lease_parser.cc +++ b/src/hooks/dhcp/lease_cmds/lease_parser.cc @@ -103,6 +103,12 @@ Lease4Parser::parse(ConstSrvConfigPtr& cfg, state = getUint8(lease_info, "state"); } + // Check if the state value is sane. + if (state > Lease::STATE_EXPIRED_RECLAIMED) { + isc_throw(BadValue, "Invalid state value: " << state << ", supported " + "values are: 0 (default), 1 (declined) and 2 (expired-reclaimed)"); + } + // Let's fabricate some data and we're ready to go. uint32_t t1 = subnet->getT1(); uint32_t t2 = subnet->getT2(); @@ -229,6 +235,12 @@ Lease6Parser::parse(ConstSrvConfigPtr& cfg, state = getUint8(lease_info, "state"); } + // Check if the state value is sane. + if (state > Lease::STATE_EXPIRED_RECLAIMED) { + isc_throw(BadValue, "Invalid state value: " << state << ", supported " + "values are: 0 (default), 1 (declined) and 2 (expired-reclaimed)"); + } + // Let's fabricate some data and we're ready to go. uint32_t t1 = subnet->getT1(); uint32_t t2 = subnet->getT2(); diff --git a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc index 957d48cf8a..30cdd0da2e 100644 --- a/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc +++ b/src/hooks/dhcp/lease_cmds/tests/lease_cmds_unittest.cc @@ -543,6 +543,21 @@ TEST_F(LeaseCmdsTest, Lease4AddBadParams) { "}"; exp_rsp = "Non-IPv4 address specified: 2001:db8::1"; testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + + // currently defined states are 0,1 and 2. 123 is junk. + txt = + "{\n" + " \"command\": \"lease4-add\",\n" + " \"arguments\": {" + " \"subnet-id\": 44,\n" + " \"ip-address\": \"192.0.2.1\",\n" + " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"state\": 123\n" + " }\n" + "}"; + exp_rsp = "Invalid state value: 123, supported values are: 0 (default), 1 " + "(declined) and 2 (expired-reclaimed)"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); } // Check that a simple, well formed lease4 can be added. @@ -761,6 +776,22 @@ TEST_F(LeaseCmdsTest, Lease6AddBadParams) { "}"; exp_rsp = "Non-IPv6 address specified: 192.0.2.1"; testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); + + // Invalid state: the only supported values are 0,1,2. + txt = + "{\n" + " \"command\": \"lease6-add\",\n" + " \"arguments\": {" + " \"subnet-id\": 66,\n" + " \"ip-address\": \"2001:db8::1\",\n" + " \"duid\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"iaid\": 1234\n," + " \"state\": 123\n" + " }\n" + "}"; + exp_rsp = "Invalid state value: 123, supported values are: 0 (default), 1 " + "(declined) and 2 (expired-reclaimed)"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); } // Check that a simple, well formed lease6 can be added. @@ -944,6 +975,35 @@ TEST_F(LeaseCmdsTest, Lease4GetMissingParams) { testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); } +// Checks that lease4-get sanitizes its input. +TEST_F(LeaseCmdsTest, Lease4GetByAddrBadParam) { + + // Initialize lease manager (false = v4, true = add a lease) + initLeaseMgr(false, true); + + // Invalid family + string cmd = + "{\n" + " \"command\": \"lease4-get\",\n" + " \"arguments\": {" + " \"ip-address\": \"2001:db8::1\"" + " }\n" + "}"; + string exp_rsp = "Invalid IPv4 address specified: 2001:db8::1"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); + + // This is way off + cmd = + "{\n" + " \"command\": \"lease4-get\",\n" + " \"arguments\": {" + " \"ip-address\": \"221B Baker St.\"" + " }\n" + "}"; + exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); +} + // Checks that lease4-get can handle a situation when the query is // valid, but the lease is not there. TEST_F(LeaseCmdsTest, Lease4GetByAddrNotFound) { @@ -1112,6 +1172,35 @@ TEST_F(LeaseCmdsTest, Lease6GetByAddr) { checkLease6(lease, "2001:db8::1", 0, 66, "77:77:77:77:77:77:77:77", false); } +// Checks that lease6-get sanitizes its input. +TEST_F(LeaseCmdsTest, Lease6GetByAddrBadParam) { + + // Initialize lease manager (true = v6, true = add a lease) + initLeaseMgr(true, true); + + // Invalid family + string cmd = + "{\n" + " \"command\": \"lease6-get\",\n" + " \"arguments\": {" + " \"ip-address\": \"192.0.2.1\"" + " }\n" + "}"; + string exp_rsp = "Invalid IPv6 address specified: 192.0.2.1"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); + + // This is way off + cmd = + "{\n" + " \"command\": \"lease6-get\",\n" + " \"arguments\": {" + " \"ip-address\": \"221B Baker St.\"" + " }\n" + "}"; + exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); +} + // Checks that lease6-get(subnet-id, type, addr6) can handle a situation when // the query is correctly formed and the lease is returned. TEST_F(LeaseCmdsTest, Lease6GetByAddrPrefix) { @@ -1283,6 +1372,31 @@ TEST_F(LeaseCmdsTest, Lease4UpdateBadParams) { testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); } +// Check that lease4-update correctly handles case when there is +// no lease to be updated. +TEST_F(LeaseCmdsTest, Lease4UpdateNoLease) { + + // Initialize lease manager (false = v4, false = don't add any lease) + initLeaseMgr(false, false); + + // Check that the lease manager pointer is there. + ASSERT_TRUE(lmptr_); + + // Now send the command. + string txt = + "{\n" + " \"command\": \"lease4-update\",\n" + " \"arguments\": {" + " \"subnet-id\": 44,\n" + " \"ip-address\": \"192.0.2.1\",\n" + " \"hw-address\": \"1a:1b:1c:1d:1e:1f\",\n" + " \"hostname\": \"newhostname.example.org\"" + " }\n" + "}"; + string exp_rsp = "failed to update the lease with address 192.0.2.1 - no such lease"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); +} + // Check that a lease4 can be updated. We're changing hw-address // and a hostname. TEST_F(LeaseCmdsTest, Lease4Update) { @@ -1458,6 +1572,33 @@ TEST_F(LeaseCmdsTest, Lease6Update) { EXPECT_EQ(7654321, l->iaid_); } + +// Check that lease6-update correctly handles case when there is +// no lease to be updated. +TEST_F(LeaseCmdsTest, Lease6UpdateNoLease) { + + // Initialize lease manager (true = v6, false = don't add any lease) + initLeaseMgr(true, false); + + // Check that the lease manager pointer is there. + ASSERT_TRUE(lmptr_); + + // Now send the command. + string txt = + "{\n" + " \"command\": \"lease6-update\",\n" + " \"arguments\": {" + " \"subnet-id\": 66,\n" + " \"ip-address\": \"2001:db8::1\",\n" + " \"iaid\": 7654321,\n" + " \"duid\": \"88:88:88:88:88:88:88:88\",\n" + " \"hostname\": \"newhostname.example.org\"" + " }\n" + "}"; + string exp_rsp = "failed to update the lease with address 2001:db8::1 - no such lease"; + testCommand(txt, CONTROL_RESULT_ERROR, exp_rsp); +} + // Checks that lease6-del can handle a situation when the query is // broken (some required parameters are missing). TEST_F(LeaseCmdsTest, Lease4DelMissingParams) { @@ -1576,6 +1717,36 @@ TEST_F(LeaseCmdsTest, Lease4DelByAddr) { EXPECT_FALSE(lmptr_->getLease4(IOAddress("192.0.2.1"))); } + +// Checks that lease4-del sanitizes its input. +TEST_F(LeaseCmdsTest, Lease4DelByAddrBadParam) { + + // Initialize lease manager (false = v4, true = add a lease) + initLeaseMgr(false, true); + + // Invalid family + string cmd = + "{\n" + " \"command\": \"lease4-del\",\n" + " \"arguments\": {" + " \"ip-address\": \"2001:db8::1\"" + " }\n" + "}"; + string exp_rsp = "Invalid IPv4 address specified: 2001:db8::1"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); + + // This is way off + cmd = + "{\n" + " \"command\": \"lease6-del\",\n" + " \"arguments\": {" + " \"ip-address\": \"221B Baker St.\"" + " }\n" + "}"; + exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); +} + // Checks that lease4-del can handle a situation when the query is // well formed, but the lease is not there. TEST_F(LeaseCmdsTest, Lease4DelByHWAddrNotFound) { @@ -1697,6 +1868,35 @@ TEST_F(LeaseCmdsTest, Lease6DelByAddr) { EXPECT_FALSE(lmptr_->getLease6(Lease::TYPE_NA, IOAddress("2001:db8::1"))); } +// Checks that lease6-del sanitizes its input. +TEST_F(LeaseCmdsTest, Lease6DelByAddrBadParam) { + + // Initialize lease manager (true = v6, true = add a lease) + initLeaseMgr(true, true); + + // Invalid family + string cmd = + "{\n" + " \"command\": \"lease6-del\",\n" + " \"arguments\": {" + " \"ip-address\": \"192.0.2.1\"" + " }\n" + "}"; + string exp_rsp = "Invalid IPv6 address specified: 192.0.2.1"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); + + // This is way off + cmd = + "{\n" + " \"command\": \"lease6-del\",\n" + " \"arguments\": {" + " \"ip-address\": \"221B Baker St.\"" + " }\n" + "}"; + exp_rsp = "Failed to convert string to address '221B Baker St.': Invalid argument"; + testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp); +} + // Checks that lease6-del(subnet-id, type, addr6) can handle a situation when // the query is correctly formed and the lease is deleted. TEST_F(LeaseCmdsTest, Lease6DelByAddrPrefix) { diff --git a/src/lib/dhcpsrv/tests/lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/lease_mgr_unittest.cc index 34b595286a..05dccca6bc 100644 --- a/src/lib/dhcpsrv/tests/lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/lease_mgr_unittest.cc @@ -223,12 +223,14 @@ public: " is not implemented"); } - /// @brief Pretends to will all IPv4 leases from a subnet + /// @brief Pretends to wipe all IPv4 leases from a subnet + /// @param subnet_id (ignored, but one day may specify the subnet) virtual size_t wipeLeases4(const SubnetID&) { isc_throw(NotImplemented, "ConreteLeaseMgr::wipeLeases4 not implemented"); } - /// @brief Pretends to will all IPv4 leases from a subnet + /// @brief Pretends to wipe all IPv4 leases from a subnet + /// @param subnet_id (ignored, but one day may specify the subnet) virtual size_t wipeLeases6(const SubnetID&) { isc_throw(NotImplemented, "ConreteLeaseMgr::wipeLeases4 not implemented"); }