[master] Merge branch 'trac3279'

Conflicts:
	src/lib/dhcp/iface_mgr.cc
	src/lib/dhcp/iface_mgr.h
	src/lib/dhcp/tests/iface_mgr_unittest.cc

src/bin/dhcp4/dhcp4_messages.mes

-# Copyright (C) 2012-2013  Internet Systems Consortium, Inc. ("ISC")
+# Copyright (C) 2012-2014  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
@@ -175,6 +175,13 @@ IPv4 DHCP server but it is not running.
 A debug message issued during startup, this indicates that the IPv4 DHCP
 server is about to open sockets on the specified port.
 
+% DHCP4_PACKET_NOT_FOR_US received DHCPv4 message (transid=%1, iface=%2) dropped because it contains foreign server identifier
+This debug message is issued when received DHCPv4 message is dropped because
+it is addressed to a different server, i.e. a server identifier held by
+this message doesn't match the identifier used by our server. The arguments
+of this message hold the name of the transaction id and interface on which
+the message has been received.
+
 % DHCP4_OPEN_SOCKET_FAIL failed to create socket: %1
 A warning message issued when IfaceMgr fails to open and bind a socket. The reason
 for the failure is appended as an argument of the log message.

src/bin/dhcp4/dhcp4_srv.cc

-// Copyright (C) 2011-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 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
@@ -259,6 +259,15 @@ Dhcpv4Srv::run() {
             }
         }
 
+        // Check if the DHCPv4 packet has been sent to us or to someone else.
+        // If it hasn't been sent to us, drop it!
+        if (!acceptServerId(query)) {
+            LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_NOT_FOR_US)
+                .arg(query->getTransid())
+                .arg(query->getIface());
+            continue;
+        }
+
         // When receiving a packet without message type option, getType() will
         // throw. Let's set type to -1 as default error indicator.
         int type = -1;
@@ -1525,6 +1534,57 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& question) {
     return (subnet);
 }
 
+bool
+Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const {
+    // This function is meant to be called internally by the server class, so
+    // we rely on the caller to sanity check the pointer and we don't check
+    // it here.
+
+    // Check if server identifier option is present. If it is not present
+    // we accept the message because it is targetted to all servers.
+    // Note that we don't check cases that server identifier is mandatory
+    // but not present. This is meant to be sanity checked in other
+    // functions.
+    OptionPtr option = pkt->getOption(DHO_DHCP_SERVER_IDENTIFIER);
+    if (!option) {
+        return (true);
+    }
+    // Server identifier is present. Let's convert it to 4-byte address
+    // and try to match with server identifiers used by the server.
+    OptionCustomPtr option_custom =
+        boost::dynamic_pointer_cast<OptionCustom>(option);
+    // Unable to convert the option to the option type which encapsulates it.
+    // We treat this as non-matching server id.
+    if (!option_custom) {
+        return (false);
+    }
+    // The server identifier option should carry exactly one IPv4 address.
+    // If the option definition for the server identifier doesn't change,
+    // the OptionCustom object should have exactly one IPv4 address and
+    // this check is somewhat redundant. On the other hand, if someone
+    // breaks option it may be better to check that here.
+    if (option_custom->getDataFieldsNum() != 1) {
+        return (false);
+    }
+
+    // The server identifier MUST be an IPv4 address. If given address is
+    // v6, it is wrong.
+    IOAddress server_id = option_custom->readAddress();
+    if (!server_id.isV4()) {
+        return (false);
+    }
+
+    // This function iterates over all interfaces on which the
+    // server is listening to find the one which has a socket bound
+    // to the address carried in the server identifier option.
+    // This has some performance implications. However, given that
+    // typically there will be just a few active interfaces the
+    // performance hit should be acceptable. If it turns out to
+    // be significant, we will have to cache server identifiers
+    // when sockets are opened.
+    return (IfaceMgr::instance().hasOpenSocket(server_id));
+}
+
 void
 Dhcpv4Srv::sanityCheck(const Pkt4Ptr& pkt, RequirementLevel serverid) {
     OptionPtr server_id = pkt->getOption(DHO_DHCP_SERVER_IDENTIFIER);

src/bin/dhcp4/dhcp4_srv.h

@@ -167,6 +167,21 @@ public:
 
 protected:
 
+    /// @brief Verifies if the server id belongs to our server.
+    ///
+    /// This function checks if the server identifier carried in the specified
+    /// DHCPv4 message belongs to this server. If the server identifier option
+    /// is absent or the value carried by this option is equal to one of the
+    /// server identifiers used by the server, the true is returned. If the
+    /// server identifier option is present, but it doesn't match any server
+    /// identifier used by this server, the false value is returned.
+    ///
+    /// @param pkt DHCPv4 message which server identifier is to be checked.
+    ///
+    /// @return true, if the server identifier is absent or matches one of the
+    /// server identifiers that the server is using; false otherwise.
+    bool acceptServerId(const Pkt4Ptr& pkt) const;
+
     /// @brief verifies if specified packet meets RFC requirements
     ///
     /// Checks if mandatory option is really there, that forbidden option

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

-// Copyright (C) 2011-2013  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 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
@@ -1031,6 +1031,56 @@ TEST_F(Dhcpv4SrvFakeIfaceTest, RenewBasic) {
     EXPECT_TRUE(LeaseMgrFactory::instance().deleteLease(addr));
 }
 
+// This test verifies that the logic which matches server identifier in the
+// received message with server identifiers used by a server works correctly:
+// - a message with no server identifier is accepted,
+// - a message with a server identifier which matches one of the server
+// identifiers used by a server is accepted,
+// - a message with a server identifier which doesn't match any server
+// identifier used by a server, is not accepted.
+TEST_F(Dhcpv4SrvFakeIfaceTest, acceptServerId) {
+    NakedDhcpv4Srv srv(0);
+
+    Pkt4Ptr pkt(new Pkt4(DHCPREQUEST, 1234));
+    // If no server identifier option is present, the message is always
+    // accepted.
+    EXPECT_TRUE(srv.acceptServerId(pkt));
+
+    // Create definition of the server identifier option.
+    OptionDefinition def("server-identifier", DHO_DHCP_SERVER_IDENTIFIER,
+                         "ipv4-address", false);
+
+    // Add a server identifier option which doesn't match server ids being
+    // used by the server. The accepted server ids are the IPv4 addresses
+    // configured on the interfaces. The 10.1.2.3 is not configured on
+    // any interfaces.
+    OptionCustomPtr other_serverid(new OptionCustom(def, Option::V6));
+    other_serverid->writeAddress(IOAddress("10.1.2.3"));
+    pkt->addOption(other_serverid);
+    EXPECT_FALSE(srv.acceptServerId(pkt));
+
+    // Remove the server identifier.
+    ASSERT_NO_THROW(pkt->delOption(DHO_DHCP_SERVER_IDENTIFIER));
+
+    // Add a server id being an IPv4 address configured on eth0 interface.
+    // A DHCPv4 message holding this server identifier should be accepted.
+    OptionCustomPtr eth0_serverid(new OptionCustom(def, Option::V6));
+    eth0_serverid->writeAddress(IOAddress("192.0.3.1"));
+    ASSERT_NO_THROW(pkt->addOption(eth0_serverid));
+    EXPECT_TRUE(srv.acceptServerId(pkt));
+
+    // Remove the server identifier.
+    ASSERT_NO_THROW(pkt->delOption(DHO_DHCP_SERVER_IDENTIFIER));
+
+    // Add a server id being an IPv4 address configured on eth1 interface.
+    // A DHCPv4 message holding this server identifier should be accepted.
+    OptionCustomPtr eth1_serverid(new OptionCustom(def, Option::V6));
+    eth1_serverid->writeAddress(IOAddress("10.0.0.1"));
+    ASSERT_NO_THROW(pkt->addOption(eth1_serverid));
+    EXPECT_TRUE(srv.acceptServerId(pkt));
+
+}
+
 // @todo: Implement tests for rejecting renewals
 
 // This test verifies if the sanityCheck() really checks options presence.

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -438,6 +438,7 @@ public:
     using Dhcpv4Srv::processClientName;
     using Dhcpv4Srv::computeDhcid;
     using Dhcpv4Srv::createNameChangeRequests;
+    using Dhcpv4Srv::acceptServerId;
     using Dhcpv4Srv::sanityCheck;
     using Dhcpv4Srv::srvidToString;
     using Dhcpv4Srv::unpackOptions;

src/lib/dhcp/iface_mgr.cc

-// Copyright (C) 2011-2014  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 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
@@ -321,6 +321,24 @@ IfaceMgr::hasOpenSocket(const uint16_t family) const {
     return (false);
 }
 
+bool
+IfaceMgr::hasOpenSocket(const IOAddress& addr) const {
+    // Iterate over all interfaces and search for open sockets.
+    for (IfaceCollection::const_iterator iface = ifaces_.begin();
+         iface != ifaces_.end(); ++iface) {
+        const Iface::SocketCollection& sockets = iface->getSockets();
+        for (Iface::SocketCollection::const_iterator sock = sockets.begin();
+             sock != sockets.end(); ++sock) {
+            // Check if the socket address matches the specified address.
+            if (sock->addr_ == addr) {
+                return (true);
+            }
+        }
+    }
+    // There are no open sockets found for the specified family.
+    return (false);
+}
+
 void IfaceMgr::stubDetectIfaces() {
     string ifaceName;
     const string v4addr("127.0.0.1"), v6addr("::1");

src/lib/dhcp/iface_mgr.h

-// Copyright (C) 2011-2014  Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2011-2014 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
@@ -862,6 +862,24 @@ public:
         ifaces_.push_back(iface);
     }
 
+    /// @brief Checks if there is at least one socket of the specified family
+    /// open.
+    ///
+    /// @param family A socket family.
+    ///
+    /// @return true if there is at least one socket open, false otherwise.
+    bool hasOpenSocket(const uint16_t family) const;
+
+    /// @brief Checks if there is a socket open and bound to an address.
+    ///
+    /// This function checks if one of the sockets opened by the IfaceMgr is
+    /// bound to the IP address specified as the method parameter.
+    ///
+    /// @param addr Address of the socket being searched.
+    ///
+    /// @return true if there is a socket bound to the specified address.
+    bool hasOpenSocket(const isc::asiolink::IOAddress& addr) const;
+
     /// A value of socket descriptor representing "not specified" state.
     static const int INVALID_SOCKET = -1;
 
@@ -983,13 +1001,6 @@ private:
     getLocalAddress(const isc::asiolink::IOAddress& remote_addr,
                     const uint16_t port);
 
-    /// @brief Checks if there is at least one socket of the specified family
-    /// open.
-    ///
-    /// @param family A socket family.
-    ///
-    /// @return true if there is at least one socket open, false otherwise.
-    bool hasOpenSocket(const uint16_t family) const;
 
     /// Holds instance of a class derived from PktFilter, used by the
     /// IfaceMgr to open sockets and send/receive packets through these

src/lib/dhcp/tests/iface_mgr_unittest.cc

@@ -269,6 +269,7 @@ public:
             }
         }
     }
+
 };
 
 /// @brief A test fixture class for IfaceMgr.
@@ -1503,6 +1504,43 @@ TEST_F(IfaceMgrTest, openSocket4ErrorHandler) {
 
 }
 
+// This test verifies that the function correctly checks that the v4 socket is
+// open and bound to a specific address.
+TEST_F(IfaceMgrTest, hasOpenSocketForAddress4) {
+    NakedIfaceMgr ifacemgr;
+
+    // Remove all real interfaces and create a set of dummy interfaces.
+    ifacemgr.createIfaces();
+
+    // Use the custom packet filter object. This object mimics the socket
+    // opening operation - the real socket is not open.
+    boost::shared_ptr<TestPktFilter> custom_packet_filter(new TestPktFilter());
+    ASSERT_TRUE(custom_packet_filter);
+    ASSERT_NO_THROW(ifacemgr.setPacketFilter(custom_packet_filter));
+
+    // Simulate opening sockets using the dummy packet filter.
+    ASSERT_NO_THROW(ifacemgr.openSockets4(DHCP4_SERVER_PORT, true, NULL));
+
+    // Expect that the sockets are open on both eth0 and eth1.
+    ASSERT_EQ(1, ifacemgr.getIface("eth0")->getSockets().size());
+    ASSERT_EQ(1, ifacemgr.getIface("eth1")->getSockets().size());
+    // Socket shouldn't have been opened on loopback.
+    ASSERT_TRUE(ifacemgr.getIface("lo")->getSockets().empty());
+
+    // Check that there are sockets bound to addresses that we have
+    // set for interfaces.
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(IOAddress("192.0.2.3")));
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(IOAddress("10.0.0.1")));
+    // Check that there is no socket for the address which is not
+    // configured on any interface.
+    EXPECT_FALSE(ifacemgr.hasOpenSocket(IOAddress("10.1.1.1")));
+
+    // Check that v4 sockets are open, but no v6 socket is open.
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(AF_INET));
+    EXPECT_FALSE(ifacemgr.hasOpenSocket(AF_INET6));
+
+}
+
 // This test checks that the sockets are open and bound to link local addresses
 // only, if unicast addresses are not specified.
 TEST_F(IfaceMgrTest, openSockets6LinkLocal) {
@@ -1830,32 +1868,6 @@ TEST_F(IfaceMgrTest, openSockets6NoIfaces) {
     EXPECT_FALSE(socket_open);
 }
 
-// Test that exception is thrown when trying to bind a new socket to the port
-// and address which is already in use by another socket.
-TEST_F(IfaceMgrTest, openSockets6NoErrorHandler) {
-    NakedIfaceMgr ifacemgr;
-
-    // Remove all real interfaces and create a set of dummy interfaces.
-    ifacemgr.createIfaces();
-
-    boost::shared_ptr<PktFilter6Stub> filter(new PktFilter6Stub());
-    ASSERT_TRUE(filter);
-    ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
-
-    // Open socket on eth0. The openSockets6 should detect that this
-    // socket has been already open and an attempt to open another socket
-    // and bind to this address and port should fail.
-    ASSERT_NO_THROW(ifacemgr.openSocket("eth0",
-                                        IOAddress("fe80::3a60:77ff:fed5:cdef"),
-                                        DHCP6_SERVER_PORT));
-
-    // The function throws an exception when it tries to open a socket
-    // and bind it to the address in use.
-    EXPECT_THROW(ifacemgr.openSockets6(DHCP6_SERVER_PORT),
-                 isc::dhcp::SocketConfigError);
-
-}
-
 // Test that the external error handler is called when trying to bind a new
 // socket to the address and port being in use. The sockets on the other
 // interfaces should open just fine.
@@ -1897,6 +1909,41 @@ TEST_F(IfaceMgrTest, openSocket6ErrorHandler) {
 
 }
 
+// This test verifies that the function correctly checks that the v6 socket is
+// open and bound to a specific address.
+TEST_F(IfaceMgrTest, hasOpenSocketForAddress6) {
+    NakedIfaceMgr ifacemgr;
+
+    // Remove all real interfaces and create a set of dummy interfaces.
+    ifacemgr.createIfaces();
+
+    boost::shared_ptr<PktFilter6Stub> filter(new PktFilter6Stub());
+    ASSERT_TRUE(filter);
+    ASSERT_NO_THROW(ifacemgr.setPacketFilter(filter));
+
+    // Simulate opening sockets using the dummy packet filter.
+    bool success = false;
+    ASSERT_NO_THROW(success = ifacemgr.openSockets6(DHCP6_SERVER_PORT));
+    EXPECT_TRUE(success);
+
+    // Make sure that the sockets are bound as expected.
+    ASSERT_TRUE(ifacemgr.isBound("eth0", "fe80::3a60:77ff:fed5:cdef"));
+    EXPECT_TRUE(ifacemgr.isBound("eth1", "fe80::3a60:77ff:fed5:abcd"));
+
+    // There should be v6 sockets only, no v4 sockets.
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(AF_INET6));
+    EXPECT_FALSE(ifacemgr.hasOpenSocket(AF_INET));
+
+    // Check that there are sockets bound to the addresses we have configured
+    // for interfaces.
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(IOAddress("fe80::3a60:77ff:fed5:cdef")));
+    EXPECT_TRUE(ifacemgr.hasOpenSocket(IOAddress("fe80::3a60:77ff:fed5:abcd")));
+    // Check that there is no socket bound to the address which hasn't been
+    // configured on any interface.
+    EXPECT_FALSE(ifacemgr.hasOpenSocket(IOAddress("fe80::3a60:77ff:feed:1")));
+
+}
+
 // Test the Iface structure itself
 TEST_F(IfaceMgrTest, iface) {
     boost::scoped_ptr<Iface> iface;