From 2a453ec2f66ea4681283df5a4d9e99410a974507 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Fri, 6 Oct 2017 12:39:08 +0200 Subject: [PATCH] [5377] DHCPv4 server now respects the outbound-interface setting. --- src/bin/dhcp4/dhcp4_srv.cc | 92 ++++++++++++++--------- src/bin/dhcp4/dhcp4_srv.h | 10 +-- src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 84 ++++++++++++++++++++- src/lib/dhcp/pkt.h | 14 +++- src/lib/dhcp/pkt_filter_inet.cc | 19 ++++- src/lib/dhcpsrv/cfg_iface.cc | 2 +- src/lib/dhcpsrv/cfg_iface.h | 5 ++ 7 files changed, 179 insertions(+), 47 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 218ede3683..cd79e1b3b7 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -1139,13 +1140,20 @@ Dhcpv4Srv::srvidToString(const OptionPtr& srvid) { void Dhcpv4Srv::appendServerID(Dhcpv4Exchange& ex) { - // The source address for the outbound message should have been set already. - // This is the address that to the best of the server's knowledge will be - // available from the client. - /// @todo: perhaps we should consider some more sophisticated server id - /// generation, but for the current use cases, it should be ok. + + // Use local address on which the packet has been received as a + // server identifier. In some cases it may be a different address, + // e.g. broadcast packet or DHCPv4o6 packet. + IOAddress local_addr = ex.getQuery()->getLocalAddr(); + Pkt4Ptr query = ex.getQuery(); + + if (local_addr.isV4Bcast() || query->isDhcp4o6()) { + SocketInfo sock_info = IfaceMgr::instance().getSocket(*query); + local_addr = sock_info.addr_; + } + OptionPtr opt_srvid(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, - ex.getResponse()->getLocalAddr())); + local_addr)); ex.getResponse()->addOption(opt_srvid); } @@ -2027,38 +2035,52 @@ Dhcpv4Srv::adjustIfaceData(Dhcpv4Exchange& ex) { response->setRemotePort(DHCP4_SERVER_PORT); } - IOAddress local_addr = query->getLocalAddr(); + CfgIfacePtr cfg_iface = CfgMgr::instance().getCurrentCfg()->getCfgIface(); + if (query->isRelayed() && + (cfg_iface->getSocketType() == CfgIface::SOCKET_UDP) && + (cfg_iface->getOutboundIface() == CfgIface::USE_ROUTING)) { - // In many cases the query is sent to a broadcast address. This address - // appears as a local address in the query message. We can't simply copy - // this address to a response message and use it as a source address. - // Instead we will need to use the address assigned to the interface - // on which the query has been received. In other cases, we will just - // use this address as a source address for the response. - // Do the same for DHCPv4-over-DHCPv6 exchanges. - if (local_addr.isV4Bcast() || query->isDhcp4o6()) { - SocketInfo sock_info = IfaceMgr::instance().getSocket(*query); - local_addr = sock_info.addr_; + response->setLocalAddr(IOAddress::IPV4_ZERO_ADDRESS()); + response->setIface(""); + response->resetIndex(); + + } else { + + IOAddress local_addr = query->getLocalAddr(); + + // In many cases the query is sent to a broadcast address. This address + // appears as a local address in the query message. We can't simply copy + // this address to a response message and use it as a source address. + // Instead we will need to use the address assigned to the interface + // on which the query has been received. In other cases, we will just + // use this address as a source address for the response. + // Do the same for DHCPv4-over-DHCPv6 exchanges. + if (local_addr.isV4Bcast() || query->isDhcp4o6()) { + SocketInfo sock_info = IfaceMgr::instance().getSocket(*query); + local_addr = sock_info.addr_; + } + + // We assume that there is an appropriate socket bound to this address + // and that the address is correct. This is safe assumption because + // the local address of the query is set when the query is received. + // The query sent to an incorrect address wouldn't have been received. + // However, if socket is closed for this address between the reception + // of the query and sending a response, the IfaceMgr should detect it + // and return an error. + response->setLocalAddr(local_addr); + // In many cases the query is sent to a broadcast address. This address + // appears as a local address in the query message. Therefore we can't + // simply copy local address from the query and use it as a source + // address for the response. Instead, we have to check what address our + // socket is bound to and use it as a source address. This operation + // may throw if for some reason the socket is closed. + /// @todo Consider an optimization that we use local address from + /// the query if this address is not broadcast. + response->setIface(query->getIface()); + response->setIndex(query->getIndex()); } - // We assume that there is an appropriate socket bound to this address - // and that the address is correct. This is safe assumption because - // the local address of the query is set when the query is received. - // The query sent to an incorrect address wouldn't have been received. - // However, if socket is closed for this address between the reception - // of the query and sending a response, the IfaceMgr should detect it - // and return an error. - response->setLocalAddr(local_addr); - // In many cases the query is sent to a broadcast address. This address - // appears as a local address in the query message. Therefore we can't - // simply copy local address from the query and use it as a source - // address for the response. Instead, we have to check what address our - // socket is bound to and use it as a source address. This operation - // may throw if for some reason the socket is closed. - /// @todo Consider an optimization that we use local address from - /// the query if this address is not broadcast. + response->setLocalPort(DHCP4_SERVER_PORT); - response->setIface(query->getIface()); - response->setIndex(query->getIndex()); } void diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 9d197aca73..0f07121a85 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -681,12 +681,10 @@ protected: /// @brief Adds server identifier option to the server's response. /// - /// This method adds a server identifier to the DHCPv4 message. It expects - /// that the local (source) address is set for this message. If address is - /// not set, it will throw an exception. This method also expects that the - /// server identifier option is not present in the specified message. - /// Otherwise, it will throw an exception on attempt to add a duplicate - /// server identifier option. + /// This method adds a server identifier to the DHCPv4 message. This is set + /// to the local address on which the client's query has been received with + /// the exception of broadcast traffic and DHCPv4o6 query for which a socket + /// on the particular interface is found and its address is used as server id. /// /// @note This method doesn't throw exceptions by itself but the underlying /// classes being used my throw. The reason for this method to not sanity diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 85897604b1..9c3b7b0e7c 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -204,6 +204,88 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelay) { EXPECT_EQ("192.0.1.50", resp->getRemoteAddr().toText()); } +// This test verifies that it is possible to configure the server to use +// routing information to determine the right outbound interface to sent +// responses to a relayed client. +TEST_F(Dhcpv4SrvTest, adjustIfaceDataUseRouting) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + // Create configuration for interfaces. It includes the outbound-interface + // setting which indicates that the responses aren't neccessarily sent + // over the same interface via which a request has been received, but routing + // information is used to determine this interface. + CfgMgr::instance().clear(); + CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface(); + cfg_iface->useSocketType(AF_INET, CfgIface::SOCKET_UDP); + cfg_iface->use(AF_INET, "eth0"); + cfg_iface->use(AF_INET, "eth1"); + cfg_iface->setOutboundIface(CfgIface::USE_ROUTING); + CfgMgr::instance().commit();; + + // Create the instance of the incoming packet. + boost::shared_ptr req(new Pkt4(DHCPDISCOVER, 1234)); + // Set the giaddr to non-zero address and hops to non-zero value + // as if it was relayed. + req->setGiaddr(IOAddress("192.0.1.1")); + req->setHops(2); + // Set ciaddr to zero. This simulates the client which applies + // for the new lease. + req->setCiaddr(IOAddress("0.0.0.0")); + // Clear broadcast flag. + req->setFlags(0x0000); + + // Set local address, port and interface. + req->setLocalAddr(IOAddress("192.0.2.5")); + req->setLocalPort(1001); + req->setIface("eth1"); + req->setIndex(1); + + // Create the exchange using the req. + Dhcpv4Exchange ex = createExchange(req); + + Pkt4Ptr resp = ex.getResponse(); + resp->setYiaddr(IOAddress("192.0.1.100")); + // Clear the remote address. + resp->setRemoteAddr(IOAddress("0.0.0.0")); + // Set hops value for the response. + resp->setHops(req->getHops()); + + // This function never throws. + ASSERT_NO_THROW(NakedDhcpv4Srv::adjustIfaceData(ex)); + + // Now the destination address should be relay's address. + EXPECT_EQ("192.0.1.1", resp->getRemoteAddr().toText()); + // The query has been relayed, so the response must be sent to the port 67. + EXPECT_EQ(DHCP4_SERVER_PORT, resp->getRemotePort()); + + // The local port is always DHCPv4 server port 67. + EXPECT_EQ(DHCP4_SERVER_PORT, resp->getLocalPort()); + + + // No specific interface is selected as outbound interface and no specific + // local address is provided. The IfaceMgr will figure out which interface to use. + EXPECT_TRUE(resp->getLocalAddr().isV4Zero()); + EXPECT_TRUE(resp->getIface().empty()); + EXPECT_FALSE(resp->indexSet()); + + // Another test verifies that setting outbound interface to same as inbound will + // cause the server to set interface and local address as expected. + + cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface(); + cfg_iface->useSocketType(AF_INET, CfgIface::SOCKET_UDP); + cfg_iface->use(AF_INET, "eth0"); + cfg_iface->use(AF_INET, "eth1"); + cfg_iface->setOutboundIface(CfgIface::SAME_AS_INBOUND); + CfgMgr::instance().commit(); + + ASSERT_NO_THROW(NakedDhcpv4Srv::adjustIfaceData(ex)); + + EXPECT_EQ("192.0.2.5", resp->getLocalAddr().toText()); + EXPECT_EQ("eth1", resp->getIface()); + EXPECT_EQ(1, resp->getIndex()); +} + // This test verifies that the destination address of the response message // is set to ciaddr when giaddr is set to zero and the ciaddr is set to // non-zero address in the received message. This is the case when the @@ -487,7 +569,7 @@ TEST_F(Dhcpv4SrvTest, appendServerID) { // Set a local address. It is required by the function under test // to create the Server Identifier option. - response->setLocalAddr(IOAddress("192.0.3.1")); + query->setLocalAddr(IOAddress("192.0.3.1")); // Append the Server Identifier. ASSERT_NO_THROW(NakedDhcpv4Srv::appendServerID(ex)); diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h index a86d4ce437..91448ff024 100644 --- a/src/lib/dhcp/pkt.h +++ b/src/lib/dhcp/pkt.h @@ -468,6 +468,11 @@ public: ifindex_ = ifindex; }; + /// @brief Resets interface index to negative value. + void resetIndex() { + ifindex_ = -1; + } + /// @brief Returns interface index. /// /// @return interface index @@ -475,6 +480,13 @@ public: return (ifindex_); }; + /// @brief Checks if interface index has been set. + /// + /// @return true if interface index set, false otherwise. + bool indexSet() const { + return (ifindex_ >= 0); + } + /// @brief Returns interface name. /// /// Returns interface name over which packet was received or is @@ -697,7 +709,7 @@ protected: /// Each network interface has assigned an unique ifindex. /// It is a functional equivalent of a name, but sometimes more useful, e.g. /// when using odd systems that allow spaces in interface names. - int ifindex_; + int64_t ifindex_; /// @brief Local IP (v4 or v6) address. /// diff --git a/src/lib/dhcp/pkt_filter_inet.cc b/src/lib/dhcp/pkt_filter_inet.cc index b780c49406..4311caafac 100644 --- a/src/lib/dhcp/pkt_filter_inet.cc +++ b/src/lib/dhcp/pkt_filter_inet.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2017 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 @@ -255,8 +255,21 @@ PktFilterInet::send(const Iface&, uint16_t sockfd, cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_pktinfo)); struct in_pktinfo* pktinfo =(struct in_pktinfo *)CMSG_DATA(cmsg); memset(pktinfo, 0, sizeof(struct in_pktinfo)); - pktinfo->ipi_ifindex = pkt->getIndex(); - pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr().toUint32()); // set the source IP address + + // In some cases the index of the outbound interface is not set. This + // is a matter of configuration. When the server is configured to + // determine the outbound interface based on routing information, + // the index is left unset (negative). + if (pkt->indexSet()) { + pktinfo->ipi_ifindex = pkt->getIndex(); + } + + // When the DHCP server is using routing to determine the outbound + // interface, the local address is also left unset. + if (!pkt->getLocalAddr().isV4Zero()) { + pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr().toUint32()); + } + m.msg_controllen = CMSG_SPACE(sizeof(struct in_pktinfo)); #endif diff --git a/src/lib/dhcpsrv/cfg_iface.cc b/src/lib/dhcpsrv/cfg_iface.cc index 049407154f..8aad5eca52 100644 --- a/src/lib/dhcpsrv/cfg_iface.cc +++ b/src/lib/dhcpsrv/cfg_iface.cc @@ -261,7 +261,7 @@ CfgIface::textToOutboundIface(const std::string& txt) { void CfgIface::setOutboundIface(const OutboundIface& outbound_iface) { - outbound_iface_ = traffic_type; + outbound_iface_ = outbound_iface; } void diff --git a/src/lib/dhcpsrv/cfg_iface.h b/src/lib/dhcpsrv/cfg_iface.h index f591745194..57344c2911 100644 --- a/src/lib/dhcpsrv/cfg_iface.h +++ b/src/lib/dhcpsrv/cfg_iface.h @@ -233,6 +233,11 @@ public: void useSocketType(const uint16_t family, const std::string& socket_type_name); + /// @brief Returns DHCP socket type used by the server. + SocketType getSocketType() const { + return (socket_type_); + } + /// @brief Returns the socket type in the textual format. std::string socketTypeToText() const; -- 2.47.3