[master] Merged trac3773 (always call SanityCheck)

src/bin/dhcp6/dhcp6_srv.cc

@@ -259,12 +259,7 @@ Dhcpv6Srv::testServerID(const Pkt6Ptr& pkt) {
     /// @todo Currently we always check server identifier regardless if
     /// it is allowed in the received message or not (per RFC3315).
     /// If the server identifier is not allowed in the message, the
-    /// sanityCheck function should deal with it. We may rethink this
-    /// design if we decide that it is appropriate to check at this stage
-    /// of message processing that the server identifier must or must not
-    /// be present. In such case however, the logic checking server id
-    /// will have to be removed from sanityCheck and placed here instead,
-    /// to avoid duplicate checks.
+    /// sanityCheck function should deal with it.
     OptionPtr server_id = pkt->getOption(D6O_SERVERID);
     if (server_id){
         // Let us test received ServerID if it is same as ServerID
@@ -1970,16 +1965,9 @@ Dhcpv6Srv::extendLeases(const Pkt6Ptr& query, Pkt6Ptr& reply,
     // Renew or Rebind message.
     /// @todo add support for IA_TA
 
-    /// @todo - assignLeases() drops the packet as RFC violation, shouldn't
-    /// we do that here? Shouldn't sanityCheck defend against this? Maybe
-    /// this should treated as a code error instead. If we're this far with
-    /// no duid that seems wrong.
-    if (!ctx.duid_) {
-        // This should not happen. We have checked this before.
-        reply->addOption(createStatusCode(*query, STATUS_UnspecFail,
-                         "You did not include mandatory client-id"));
-        return;
-    }
+    // For the lease extension it is critical that the client has sent
+    // DUID. There is no need to check for the presence of the DUID here
+    // because we have already checked it in the sanityCheck().
 
     for (OptionCollection::iterator opt = query->options_.begin();
          opt != query->options_.end(); ++opt) {
@@ -2466,6 +2454,8 @@ Dhcpv6Srv::processRenew(const Pkt6Ptr& renew) {
 Pkt6Ptr
 Dhcpv6Srv::processRebind(const Pkt6Ptr& rebind) {
 
+    sanityCheck(rebind, MANDATORY, FORBIDDEN);
+
     // Let's create a simplified client context here.
     AllocEngine::ClientContext6 ctx = createContext(rebind);
 
@@ -2486,6 +2476,8 @@ Dhcpv6Srv::processRebind(const Pkt6Ptr& rebind) {
 Pkt6Ptr
 Dhcpv6Srv::processConfirm(const Pkt6Ptr& confirm) {
 
+    sanityCheck(confirm, MANDATORY, FORBIDDEN);
+
     // Let's create a simplified client context here.
     AllocEngine::ClientContext6 ctx = createContext(confirm);
 
@@ -2591,6 +2583,9 @@ Dhcpv6Srv::processRelease(const Pkt6Ptr& release) {
 
 Pkt6Ptr
 Dhcpv6Srv::processDecline(const Pkt6Ptr& decline) {
+
+    sanityCheck(decline, MANDATORY, MANDATORY);
+
     /// @todo: Implement this
     Pkt6Ptr reply(new Pkt6(DHCPV6_REPLY, decline->getTransid()));
     return (reply);
@@ -2599,6 +2594,8 @@ Dhcpv6Srv::processDecline(const Pkt6Ptr& decline) {
 Pkt6Ptr
 Dhcpv6Srv::processInfRequest(const Pkt6Ptr& inf_request) {
 
+    sanityCheck(inf_request, OPTIONAL, OPTIONAL);
+
     // Let's create a simplified client context here.
     AllocEngine::ClientContext6 ctx = createContext(inf_request);
 

src/bin/dhcp6/tests/confirm_unittest.cc

@@ -18,6 +18,7 @@
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcp6/json_config_parser.h>
 #include <dhcp6/tests/dhcp6_message_test.h>
+#include <dhcpsrv/utils.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -98,6 +99,24 @@ public:
 };
 
 
+// Test that client-id is mandatory and server-id forbidden for Confirm messages
+TEST_F(ConfirmTest, sanityCheck) {
+    NakedDhcpv6Srv srv(0);
+
+    // A message with no client-id should fail
+    Pkt6Ptr confirm = Pkt6Ptr(new Pkt6(DHCPV6_CONFIRM, 1234));
+    EXPECT_THROW(srv.processConfirm(confirm), RFCViolation);
+
+    // A message with a single client-id should succeed
+    OptionPtr clientid = generateClientId();
+    confirm->addOption(clientid);
+    EXPECT_NO_THROW(srv.processConfirm(confirm));
+
+    // A message with server-id present should fail
+    confirm->addOption(srv.getServerID());
+    EXPECT_THROW(srv.processConfirm(confirm), RFCViolation);
+}
+
 // Test that directly connected client's Confirm message is processed and Reply
 // message is sent back. In this test case, the client sends Confirm for two
 // addresses that belong to the same IAID and are sent within the same IA_NA

src/bin/dhcp6/tests/dhcp6_test_utils.h

@@ -98,7 +98,11 @@ public:
     using Dhcpv6Srv::processSolicit;
     using Dhcpv6Srv::processRequest;
     using Dhcpv6Srv::processRenew;
+    using Dhcpv6Srv::processRebind;
+    using Dhcpv6Srv::processConfirm;
     using Dhcpv6Srv::processRelease;
+    using Dhcpv6Srv::processDecline;
+    using Dhcpv6Srv::processInfRequest;
     using Dhcpv6Srv::processClientFqdn;
     using Dhcpv6Srv::createNameChangeRequests;
     using Dhcpv6Srv::createRemovalNameChangeRequest;

src/bin/dhcp6/tests/rebind_unittest.cc

@@ -18,6 +18,7 @@
 #include <dhcp/tests/iface_mgr_test_config.h>
 #include <dhcp6/json_config_parser.h>
 #include <dhcp6/tests/dhcp6_message_test.h>
+#include <dhcpsrv/utils.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -244,6 +245,24 @@ public:
     }
 };
 
+// Test that client-id is mandatory and server-id forbidden for Rebind messages
+TEST_F(RebindTest, sanityCheck) {
+    NakedDhcpv6Srv srv(0);
+
+    // A message with no client-id should fail
+    Pkt6Ptr rebind = Pkt6Ptr(new Pkt6(DHCPV6_REBIND, 1234));
+    EXPECT_THROW(srv.processRebind(rebind), RFCViolation);
+
+    // A message with a single client-id should succeed
+    OptionPtr clientid = generateClientId();
+    rebind->addOption(clientid);
+    EXPECT_NO_THROW(srv.processRebind(rebind));
+
+    // A message with server-id present should fail
+    rebind->addOption(srv.getServerID());
+    EXPECT_THROW(srv.processRebind(rebind), RFCViolation);
+}
+
 // Test that directly connected client's Rebind message is processed and Reply
 // message is sent back.
 TEST_F(RebindTest, directClient) {