diff --git a/src/lib/dhcp/duid.cc b/src/lib/dhcp/duid.cc index f1c8866c1c..4f5113ab33 100644 --- a/src/lib/dhcp/duid.cc +++ b/src/lib/dhcp/duid.cc @@ -28,15 +28,20 @@ namespace dhcp { DUID::DUID(const std::vector& duid) { if (duid.size() > MAX_DUID_LEN) { isc_throw(OutOfRange, "DUID too large"); - } else { - duid_ = duid; } + if (duid.empty()) { + isc_throw(OutOfRange, "Empty DUIDs are not allowed"); + } + duid_ = duid; } DUID::DUID(const uint8_t* data, size_t len) { if (len > MAX_DUID_LEN) { isc_throw(OutOfRange, "DUID too large"); } + if (len == 0) { + isc_throw(OutOfRange, "Empty DUIDs/Client-ids not allowed"); + } duid_ = std::vector(data, data + len); } @@ -83,11 +88,19 @@ bool DUID::operator!=(const DUID& other) const { // Constructor based on vector ClientId::ClientId(const std::vector& clientid) : DUID(clientid) { + if (clientid.size() < MIN_CLIENT_ID_LEN) { + isc_throw(OutOfRange, "client-id is too short (" << clientid.size() + << "), at least 2 is required"); + } } // Constructor based on C-style data ClientId::ClientId(const uint8_t *clientid, size_t len) : DUID(clientid, len) { + if (len < MIN_CLIENT_ID_LEN) { + isc_throw(OutOfRange, "client-id is too short (" << len + << "), at least 2 is required"); + } } // Returns a copy of client-id data diff --git a/src/lib/dhcp/duid.h b/src/lib/dhcp/duid.h index 688885b90c..efbc6e13ca 100644 --- a/src/lib/dhcp/duid.h +++ b/src/lib/dhcp/duid.h @@ -35,6 +35,11 @@ class DUID { /// As defined in RFC3315, section 9.1 static const size_t MAX_DUID_LEN = 128; + /// @brief minimum duid size + /// There's no explicit minimal DUID size specified in RFC3315, + /// so let's use absolute minimum + static const size_t MIN_DUID_LEN = 1; + /// @brief specifies DUID type typedef enum { DUID_UNKNOWN = 0, ///< invalid/unknown type @@ -88,6 +93,13 @@ typedef boost::shared_ptr DuidPtr; /// a client-id class ClientId : public DUID { public: + + /// @brief Minimum size of a client ID + /// + /// Excerpt from RFC2132, section 9.14. + /// The code for this option is 61, and its minimum length is 2. + static const size_t MIN_CLIENT_ID_LEN = 2; + /// @brief Maximum size of a client ID /// /// This is the same as the maximum size of the underlying DUID. diff --git a/src/lib/dhcp/tests/duid_unittest.cc b/src/lib/dhcp/tests/duid_unittest.cc index de20e51060..aea15d1728 100644 --- a/src/lib/dhcp/tests/duid_unittest.cc +++ b/src/lib/dhcp/tests/duid_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2011 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -37,6 +37,15 @@ using boost::scoped_ptr; namespace { +// This is a workaround for strange linking problems with gtest: +// libdhcp___unittests-duid_unittest.o: In function `Compare': +// ~/gtest-1.6.0/include/gtest/gtest.h:1353: undefined reference to `isc::dhcp::ClientId::MAX_CLIENT_ID_LE'N +// collect2: ld returned 1 exit status + +const size_t MAX_DUID_LEN = DUID::MAX_DUID_LEN; +const size_t MAX_CLIENT_ID_LEN = DUID::MAX_DUID_LEN; + + // This test verifies if the constructors are working as expected // and process passed parameters. TEST(DuidTest, constructor) { @@ -61,21 +70,20 @@ TEST(DuidTest, constructor) { // This test verifies if DUID size restrictions are implemented // properly. TEST(DuidTest, size) { - const int MAX_DUID_SIZE = 128; - uint8_t data[MAX_DUID_SIZE + 1]; + uint8_t data[MAX_DUID_LEN + 1]; vector data2; - for (uint8_t i = 0; i < MAX_DUID_SIZE + 1; ++i) { + for (uint8_t i = 0; i < MAX_DUID_LEN + 1; ++i) { data[i] = i; - if (i < MAX_DUID_SIZE) + if (i < MAX_DUID_LEN) data2.push_back(i); } - ASSERT_EQ(data2.size(), MAX_DUID_SIZE); + ASSERT_EQ(data2.size(), MAX_DUID_LEN); - scoped_ptr duidmaxsize1(new DUID(data, MAX_DUID_SIZE)); + scoped_ptr duidmaxsize1(new DUID(data, MAX_DUID_LEN)); scoped_ptr duidmaxsize2(new DUID(data2)); EXPECT_THROW( - scoped_ptr toolarge1(new DUID(data, MAX_DUID_SIZE + 1)), + scoped_ptr toolarge1(new DUID(data, MAX_DUID_LEN + 1)), OutOfRange); // that's one too much @@ -84,6 +92,16 @@ TEST(DuidTest, size) { EXPECT_THROW( scoped_ptr toolarge2(new DUID(data2)), OutOfRange); + + // empty duids are not allowed + vector empty; + EXPECT_THROW( + scoped_ptr emptyDuid(new DUID(empty)), + OutOfRange); + + EXPECT_THROW( + scoped_ptr emptyDuid2(new DUID(data, 0)), + OutOfRange); } // This test verifies if the implementation supports all defined @@ -157,6 +175,51 @@ TEST(ClientIdTest, constructor) { EXPECT_TRUE(data2 == vecdata); } +// Check that client-id sizes are reasonable +TEST(ClientIdTest, size) { + uint8_t data[MAX_CLIENT_ID_LEN + 1]; + vector data2; + for (uint8_t i = 0; i < MAX_CLIENT_ID_LEN + 1; ++i) { + data[i] = i; + if (i < MAX_CLIENT_ID_LEN) + data2.push_back(i); + } + ASSERT_EQ(data2.size(), MAX_CLIENT_ID_LEN); + + scoped_ptr duidmaxsize1(new ClientId(data, MAX_CLIENT_ID_LEN)); + scoped_ptr duidmaxsize2(new ClientId(data2)); + + EXPECT_THROW( + scoped_ptr toolarge1(new ClientId(data, MAX_CLIENT_ID_LEN + 1)), + OutOfRange); + + // that's one too much + data2.push_back(128); + + EXPECT_THROW( + scoped_ptr toolarge2(new ClientId(data2)), + OutOfRange); + + // empty client-ids are not allowed + vector empty; + EXPECT_THROW( + scoped_ptr empty_client_id1(new ClientId(empty)), + OutOfRange); + + EXPECT_THROW( + scoped_ptr empty_client_id2(new ClientId(data, 0)), + OutOfRange); + + // client-id must be at least 2 bytes long + vector shorty(1,17); // just a single byte with value 17 + EXPECT_THROW( + scoped_ptr too_short_client_id1(new ClientId(shorty)), + OutOfRange); + EXPECT_THROW( + scoped_ptr too_short_client_id1(new ClientId(data, 1)), + OutOfRange); +} + // This test checks if the comparison operators are sane. TEST(ClientIdTest, operators) { uint8_t data1[] = {0, 1, 2, 3, 4, 5, 6}; diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index bcb7851552..9c5bdeb298 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -177,6 +177,14 @@ AllocEngine::allocateAddress6(const Subnet6Ptr& subnet, isc_throw(InvalidOperation, "No allocator selected"); } + if (!subnet) { + isc_throw(InvalidOperation, "Subnet is required for allocation"); + } + + if (!duid) { + isc_throw(InvalidOperation, "DUID is mandatory for allocation"); + } + // check if there's existing lease for that subnet/duid/iaid combination. Lease6Ptr existing = LeaseMgrFactory::instance().getLease6(*duid, iaid, subnet->getID()); if (existing) { @@ -284,8 +292,16 @@ AllocEngine::allocateAddress4(const SubnetPtr& subnet, isc_throw(InvalidOperation, "No allocator selected"); } + if (!subnet) { + isc_throw(InvalidOperation, "Can't allocate IPv4 address without subnet"); + } + + if (!hwaddr) { + isc_throw(InvalidOperation, "HWAddr must be defined"); + } + // Check if there's existing lease for that subnet/clientid/hwaddr combination. - Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(hwaddr->hwaddr_, subnet->getID()); + Lease4Ptr existing = LeaseMgrFactory::instance().getLease4(*hwaddr, subnet->getID()); if (existing) { // We have a lease already. This is a returning client, probably after // its reboot. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc index ddc0f62be1..25224e6853 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_unittest.cc @@ -160,7 +160,15 @@ public: EXPECT_EQ(subnet_->getT2(), lease->t2_); EXPECT_TRUE(false == lease->fqdn_fwd_); EXPECT_TRUE(false == lease->fqdn_rev_); - EXPECT_TRUE(*lease->client_id_ == *clientid_); + if (lease->client_id_ && !clientid_) { + ADD_FAILURE() << "Lease4 has a client-id, while it should have none."; + } + if (!lease->client_id_ && clientid_) { + ADD_FAILURE() << "Lease4 has no client-id, but it was expected to have one."; + } + if (lease->client_id_ && clientid_) { + EXPECT_TRUE(*lease->client_id_ == *clientid_); + } EXPECT_TRUE(lease->hwaddr_ == hwaddr_->hwaddr_); // @todo: check cltt } @@ -329,6 +337,24 @@ TEST_F(AllocEngine6Test, allocBogusHint6) { detailCompareLease(lease, from_mgr); } +// This test checks that NULL values are handled properly +TEST_F(AllocEngine6Test, allocateAddress6Nulls) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100))); + ASSERT_TRUE(engine); + + // Allocations without subnet are not allowed + Lease6Ptr lease = engine->allocateAddress6(Subnet6Ptr(), duid_, iaid_, + IOAddress("::"), false); + ASSERT_FALSE(lease); + + // Allocations without DUID are not allowed either + lease = engine->allocateAddress6(subnet_, DuidPtr(), iaid_, + IOAddress("::"), false); + ASSERT_FALSE(lease); +} + + // This test verifies that the allocator picks addresses that belong to the // pool TEST_F(AllocEngine6Test, IterativeAllocator) { @@ -702,6 +728,42 @@ TEST_F(AllocEngine4Test, allocBogusHint4) { } +// This test checks that NULL values are handled properly +TEST_F(AllocEngine4Test, allocateAddress4Nulls) { + boost::scoped_ptr engine; + ASSERT_NO_THROW(engine.reset(new AllocEngine(AllocEngine::ALLOC_ITERATIVE, 100))); + ASSERT_TRUE(engine); + + // Allocations without subnet are not allowed + Lease4Ptr lease = engine->allocateAddress4(SubnetPtr(), clientid_, hwaddr_, + IOAddress("0.0.0.0"), false); + EXPECT_FALSE(lease); + + // Allocations without HW address are not allowed + lease = engine->allocateAddress4(subnet_, clientid_, HWAddrPtr(), + IOAddress("0.0.0.0"), false); + EXPECT_FALSE(lease); + + // Allocations without client-id are allowed + clientid_ = ClientIdPtr(); + lease = engine->allocateAddress4(subnet_, ClientIdPtr(), hwaddr_, + IOAddress("0.0.0.0"), false); + // Check that we got a lease + ASSERT_TRUE(lease); + + // Do all checks on the lease + checkLease4(lease); + + // Check that the lease is indeed in LeaseMgr + Lease4Ptr from_mgr = LeaseMgrFactory::instance().getLease4(lease->addr_); + ASSERT_TRUE(from_mgr); + + // Now check that the lease in LeaseMgr has the same parameters + detailCompareLease(lease, from_mgr); +} + + + // This test verifies that the allocator picks addresses that belong to the // pool TEST_F(AllocEngine4Test, IterativeAllocator) { diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc index 1fc4f0b5dd..9b8ec63c57 100644 --- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc @@ -317,8 +317,7 @@ public: } else if (address == straddress4_[7]) { lease->hwaddr_ = vector(); // Empty - lease->client_id_ = ClientIdPtr( - new ClientId(vector())); // Empty + lease->client_id_ = ClientIdPtr(); // Empty lease->valid_lft_ = 7975; lease->cltt_ = 213876; lease->subnet_id_ = 19; @@ -1048,9 +1047,10 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientId) { ASSERT_EQ(1, returned.size()); detailCompareLease(leases[3], *returned.begin()); - // Check that an empty vector is valid - EXPECT_TRUE(leases[7]->client_id_->getClientId().empty()); - returned = lmptr_->getLease4(leases[7]->hwaddr_); + // Check that client-id is NULL + EXPECT_FALSE(leases[7]->client_id_); + HWAddr tmp(leases[7]->hwaddr_, HTYPE_ETHER); + returned = lmptr_->getLease4(tmp); ASSERT_EQ(1, returned.size()); detailCompareLease(leases[7], *returned.begin()); @@ -1075,7 +1075,11 @@ TEST_F(MySqlLeaseMgrTest, getLease4ClientIdSize) { // ClientId::MAX_CLIENT_ID_LEN is used in an EXPECT_EQ. int client_id_max = ClientId::MAX_CLIENT_ID_LEN; EXPECT_EQ(128, client_id_max); - for (uint8_t i = 0; i <= client_id_max; i += 16) { + + int client_id_min = ClientId::MIN_CLIENT_ID_LEN; + EXPECT_EQ(2, client_id_min); // See RFC2132, section 9.14 + + for (uint8_t i = client_id_min; i <= client_id_max; i += 16) { vector clientid_vec(i, i); leases[1]->client_id_.reset(new ClientId(clientid_vec)); EXPECT_TRUE(lmptr_->addLease(leases[1])); @@ -1184,7 +1188,11 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSize) { // For speed, go from 0 to 128 is steps of 16. int duid_max = DUID::MAX_DUID_LEN; EXPECT_EQ(128, duid_max); - for (uint8_t i = 0; i <= duid_max; i += 16) { + + int duid_min = DUID::MIN_DUID_LEN; + EXPECT_EQ(1, duid_min); + + for (uint8_t i = duid_min; i <= duid_max; i += 16) { vector duid_vec(i, i); leases[1]->duid_.reset(new DUID(duid_vec)); EXPECT_TRUE(lmptr_->addLease(leases[1])); @@ -1249,7 +1257,11 @@ TEST_F(MySqlLeaseMgrTest, getLease6DuidIaidSubnetIdSize) { // For speed, go from 0 to 128 is steps of 16. int duid_max = DUID::MAX_DUID_LEN; EXPECT_EQ(128, duid_max); - for (uint8_t i = 0; i <= duid_max; i += 16) { + + int duid_min = DUID::MIN_DUID_LEN; + EXPECT_EQ(1, duid_min); + + for (uint8_t i = duid_min; i <= duid_max; i += 16) { vector duid_vec(i, i); leases[1]->duid_.reset(new DUID(duid_vec)); EXPECT_TRUE(lmptr_->addLease(leases[1]));