[2325] Sanity checks for DHCPv6 server implemented.

src/bin/dhcp6/dhcp6_messages.mes

@@ -81,6 +81,12 @@ This message indicates that the server failed to grant (in response to
 received REQUEST) a lease for a given client. There may be many reasons for
 such failure. Each specific failure is logged in a separate log entry.
 
+% DHCP6_REQUIRED_OPTIONS_CHECK_FAIL %1 message received from %2 failed the following check: %3
+This message indicates that received message has invalid number of options:
+mandatory client-id option is missing, server-id forbidden in that particular
+type of message is present, there is more than one instance of client-id
+or server-id etc. Exact reason for rejecting the packed is printed.
+
 % DHCP6_NOT_RUNNING IPv6 DHCP server is not running
 A warning message is issued when an attempt is made to shut down the
 IPv6 DHCP server but it is not running.

src/bin/dhcp6/dhcp6_srv.cc

@@ -132,44 +132,51 @@ bool Dhcpv6Srv::run() {
                       .arg(query->getBuffer().getLength())
                       .arg(query->toText());
 
-            switch (query->getType()) {
-            case DHCPV6_SOLICIT:
-                rsp = processSolicit(query);
-                break;
+            try {
+                switch (query->getType()) {
+                case DHCPV6_SOLICIT:
+                    rsp = processSolicit(query);
+                    break;
 
-            case DHCPV6_REQUEST:
-                rsp = processRequest(query);
-                break;
+                case DHCPV6_REQUEST:
+                    rsp = processRequest(query);
+                    break;
 
-            case DHCPV6_RENEW:
-                rsp = processRenew(query);
-                break;
+                case DHCPV6_RENEW:
+                    rsp = processRenew(query);
+                    break;
 
-            case DHCPV6_REBIND:
-                rsp = processRebind(query);
-                break;
+                case DHCPV6_REBIND:
+                    rsp = processRebind(query);
+                    break;
 
-            case DHCPV6_CONFIRM:
-                rsp = processConfirm(query);
-                break;
+                case DHCPV6_CONFIRM:
+                    rsp = processConfirm(query);
+                    break;
 
-            case DHCPV6_RELEASE:
+                case DHCPV6_RELEASE:
                 rsp = processRelease(query);
                 break;
 
-            case DHCPV6_DECLINE:
-                rsp = processDecline(query);
-                break;
+                case DHCPV6_DECLINE:
+                    rsp = processDecline(query);
+                    break;
 
-            case DHCPV6_INFORMATION_REQUEST:
-                rsp = processInfRequest(query);
-                break;
+                case DHCPV6_INFORMATION_REQUEST:
+                    rsp = processInfRequest(query);
+                    break;
 
-            default:
-                // Only action is to output a message if debug is enabled,
-                // and that will be covered by the debug statement before
-                // the "switch" statement.
-                ;
+                default:
+                    // Only action is to output a message if debug is enabled,
+                    // and that will be covered by the debug statement before
+                    // the "switch" statement.
+                    ;
+                }
+            } catch (const RFCViolation& e) {
+                LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_REQUIRED_OPTIONS_CHECK_FAIL)
+                    .arg(query->getName())
+                    .arg(query->getRemoteAddr())
+                    .arg(e.what());
             }
 
             if (rsp) {
@@ -195,9 +202,6 @@ bool Dhcpv6Srv::run() {
                 }
             }
         }
-
-        // TODO add support for config session (see src/bin/auth/main.cc)
-        //      so this daemon can be controlled from bob
     }
 
     return (true);
@@ -357,6 +361,54 @@ OptionPtr Dhcpv6Srv::createStatusCode(uint16_t code, const std::string& text) {
     return (status);
 }
 
+void Dhcpv6Srv::sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
+                            RequirementLevel serverid) {
+    Option::OptionCollection client_ids = pkt->getOptions(D6O_CLIENTID);
+    switch (clientid) {
+    case MANDATORY:
+        if (client_ids.size() != 1) {
+            isc_throw(RFCViolation, "Exactly 1 client-id option expected in "
+                      << pkt->getName() << ", but " << client_ids.size()
+                      << " received");
+        }
+        break;
+    case OPTIONAL:
+        if (client_ids.size() > 1) {
+            isc_throw(RFCViolation, "Too many (" << client_ids.size()
+                      << ") client-id options received in " << pkt->getName());
+        }
+        break;
+
+    case FORBIDDEN:
+        // doesn't make sense - client-id is always allowed
+        break;
+    }
+
+    Option::OptionCollection server_ids = pkt->getOptions(D6O_SERVERID);
+    switch (serverid) {
+    case FORBIDDEN:
+        if (server_ids.size() > 0) {
+            isc_throw(RFCViolation, "Exactly 1 server-id option expected, but "
+                      << server_ids.size() << " received in " << pkt->getName());
+        }
+        break;
+
+    case MANDATORY:
+        if (server_ids.size() != 1) {
+            isc_throw(RFCViolation, "Invalid number of server-id options received ("
+                      << server_ids.size() << "), exactly 1 expected in message "
+                      << pkt->getName());
+        }
+        break;
+
+    case OPTIONAL:
+        if (server_ids.size() > 1) {
+            isc_throw(RFCViolation, "Too many (" << server_ids.size()
+                      << ") client-id options received in " << pkt->getName());
+        }
+    }
+}
+
 Subnet6Ptr Dhcpv6Srv::selectSubnet(const Pkt6Ptr& question) {
     Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(question->getRemoteAddr());
 
@@ -370,7 +422,7 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) {
 
     // We need to select a subnet the client is connected in.
     Subnet6Ptr subnet = selectSubnet(question);
-    if (subnet) {
+    if (!subnet) {
         // This particular client is out of luck today. We do not have
         // information about the subnet he is connected to. This likely means
         // misconfiguration of the server (or some relays). We will continue to
@@ -378,12 +430,13 @@ void Dhcpv6Srv::assignLeases(const Pkt6Ptr& question, Pkt6Ptr& answer) {
         // addresses or prefixes, no subnet specific configuration etc. The only
         // thing this client can get is some global information (like DNS
         // servers).
-        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED)
-            .arg(subnet->toText());
-    } else {
+
         // perhaps this should be logged on some higher level? This is most likely
         // configuration bug.
         LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_SUBNET_SELECTION_FAILED);
+    } else {
+        LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL_DATA, DHCP6_SUBNET_SELECTED)
+            .arg(subnet->toText());
     }
 
     // @todo: We should implement Option6Duid some day, but we can do without it
@@ -514,6 +567,14 @@ OptionPtr Dhcpv6Srv::handleIA_NA(const Subnet6Ptr& subnet, const DuidPtr& duid,
 
 Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) {
 
+    if (!sanityCheck(solicit, MANDATORY, FORBIDDEN)) {
+        return (Pkt6Ptr());
+    }
+
+Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) {
+
+    sanityCheck(solicit, MANDATORY, FORBIDDEN);
+
     Pkt6Ptr advertise(new Pkt6(DHCPV6_ADVERTISE, solicit->getTransid()));
 
     copyDefaultOptions(solicit, advertise);
@@ -526,6 +587,9 @@ Pkt6Ptr Dhcpv6Srv::processSolicit(const Pkt6Ptr& solicit) {
 }
 
 Pkt6Ptr Dhcpv6Srv::processRequest(const Pkt6Ptr& request) {
+
+    sanityCheck(request, MANDATORY, MANDATORY);
+
     Pkt6Ptr reply(new Pkt6(DHCPV6_REPLY, request->getTransid()));
 
     copyDefaultOptions(request, reply);

src/bin/dhcp6/dhcp6_srv.h

@@ -30,6 +30,23 @@
 
 namespace isc {
 namespace dhcp {
+
+/// An exception that is thrown if a DHCPv6 protocol violation occurs while
+/// processing a message (e.g. a mandatory option is missing)
+class RFCViolation : public isc::Exception {
+public:
+
+/// @brief constructor
+///
+/// @param file name of the file, where exception occurred
+/// @param line line of the file, where exception occurred
+/// @param what text description of the issue that caused exception
+RFCViolation(const char* file, size_t line, const char* what) :
+    isc::Exception(file, line, what) {}
+};
+
+
+
 /// @brief DHCPv6 server service.
 ///
 /// This class represents DHCPv6 server. It contains all
@@ -45,6 +62,12 @@ namespace dhcp {
 class Dhcpv6Srv : public boost::noncopyable {
 
 public:
+    /// @brief defines if certain option may, must or must not appear
+    typedef enum {
+        FORBIDDEN,
+        MANDATORY,
+        OPTIONAL
+    } RequirementLevel;
 
     /// @brief Minimum length of a MAC address to be used in DUID generation.
     static const size_t MIN_MAC_LEN = 6;
@@ -84,6 +107,19 @@ public:
     void shutdown();
 
 protected:
+
+    /// @brief verifies if specified packet meets RFC requirements
+    ///
+    /// Checks if mandatory option is really there, that forbidden option
+    /// is not there, and that client-id or server-id appears only once.
+    ///
+    /// @param pkt packet to be checked
+    /// @param clientid expectation regarding client-id option
+    /// @param serverid expectation regarding server-id option
+    /// @throw RFCViolation if any issues are detected
+    void sanityCheck(const Pkt6Ptr& pkt, RequirementLevel clientid,
+                     RequirementLevel serverid);
+
     /// @brief Processes incoming SOLICIT and returns response.
     ///
     /// Processes received SOLICIT message and verifies that its sender

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc

@@ -59,6 +59,7 @@ public:
     using Dhcpv6Srv::processRequest;
     using Dhcpv6Srv::createStatusCode;
     using Dhcpv6Srv::selectSubnet;
+    using Dhcpv6Srv::sanityCheck;
 };
 
 class Dhcpv6SrvTest : public ::testing::Test {
@@ -651,6 +652,9 @@ TEST_F(Dhcpv6SrvTest, RequestBasic) {
     OptionPtr clientid = generateClientId();
     req->addOption(clientid);
 
+    // server-id is mandatory in REQUEST
+    req->addOption(srv->getServerID());
+
     // Pass it to the server and hope for a REPLY
     Pkt6Ptr reply = srv->processRequest(req);
 
@@ -709,6 +713,11 @@ TEST_F(Dhcpv6SrvTest, ManyRequests) {
     req2->addOption(clientid2);
     req3->addOption(clientid3);
 
+    // server-id is mandatory in REQUEST
+    req1->addOption(srv->getServerID());
+    req2->addOption(srv->getServerID());
+    req3->addOption(srv->getServerID());
+
     // Pass it to the server and get an advertise
     Pkt6Ptr reply1 = srv->processRequest(req1);
     Pkt6Ptr reply2 = srv->processRequest(req2);
@@ -763,8 +772,8 @@ TEST_F(Dhcpv6SrvTest, StatusCode) {
     EXPECT_TRUE(status->getData() == exp);
 }
 
-// This test verifies if the selectSubnet() method works as expected.
-TEST_F(Dhcpv6SrvTest, SelectSubnet) {
+// This test verifies if the sanityCheck() really checks options presence.
+TEST_F(Dhcpv6SrvTest, sanityCheck) {
     boost::scoped_ptr<NakedDhcpv6Srv> srv;
     ASSERT_NO_THROW( srv.reset(new NakedDhcpv6Srv(0)) );
 
@@ -772,18 +781,63 @@ TEST_F(Dhcpv6SrvTest, SelectSubnet) {
 
     // check that the packets originating from local addresses can be
     pkt->setRemoteAddr(IOAddress("fe80::abcd"));
-    EXPECT_EQ(subnet_, srv->selectSubnet(pkt));
 
-    // packets originating from subnet A will select subnet A
-    pkt->setRemoteAddr(IOAddress("2001:db8:1::6789"));
-    EXPECT_EQ(subnet_, srv->selectSubnet(pkt));
+    // client-id is optional for information-request, so 
+    EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL));
+
+    // empty packet, no client-id, no server-id
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN),
+                 RFCViolation);
+
+    // This doesn't make much sense, but let's check it for completeness
+    EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::FORBIDDEN, Dhcpv6Srv::FORBIDDEN));
+
+    OptionPtr clientid = generateClientId();
+    pkt->addOption(clientid);
+
+    // client-id is mandatory, server-id is forbidden (as in SOLICIT or REBIND)
+    EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::FORBIDDEN));
+
+    pkt->addOption(srv->getServerID());
+
+    // both client-id and server-id are mandatory (as in REQUEST, RENEW, RELEASE, DECLINE)
+    EXPECT_NO_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY));
+
+    // sane section ends here, let's do some negative tests as well
+
+    pkt->addOption(clientid);
+    pkt->addOption(clientid);
+
+    // with more than one client-id it should throw, no matter what
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::OPTIONAL),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::MANDATORY),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY),
+                 RFCViolation);
+
+    pkt->delOption(D6O_CLIENTID);
+    pkt->delOption(D6O_CLIENTID);
+
+    // again we have only one client-id
+
+    // let's try different type of insanity - several server-ids
+    pkt->addOption(srv->getServerID());
+    pkt->addOption(srv->getServerID());
 
-    // packets from a subnet that is not supported will not get
-    // a subnet
-    pkt->setRemoteAddr(IOAddress("3000::faf"));
-    EXPECT_FALSE(srv->selectSubnet(pkt));
+    // with more than one server-id it should throw, no matter what
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::OPTIONAL),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::OPTIONAL),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::OPTIONAL, Dhcpv6Srv::MANDATORY),
+                 RFCViolation);
+    EXPECT_THROW(srv->sanityCheck(pkt, Dhcpv6Srv::MANDATORY, Dhcpv6Srv::MANDATORY),
+                 RFCViolation);
+    
 
-    /// @todo: expand this test once support for relays is implemented
 }
 
 }   // end of anonymous namespace