- Secondary servers in a failover pair will now perform ddns removals if

  they had performed ddns updates on a lease that is expiring, or was
  released through the primary.  As part of the same fix, stale binding scopes
  will now be removed if a change in identity of a lease's active client is
  detected, rather than simply if a lease is noticed to have expired (which it
  may have expired without a failover server noticing in some situations).
  [ISC-Bugs #19826b]

diff --git a/RELNOTES b/RELNOTES
index b7e516e80e4fff7439edfcacd9b314943d7ce184..d7d1d649c997a5129858ce5e5595b99b29650aa6 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -171,6 +171,13 @@ work on other platforms. Please report any problems and suggested fixes to
 - Don't look for IPv6 interfaces on Linux when running in DHCPv4 mode.
   Thanks to patches from Matthew Newton and David Cantrell.
 
+- Secondary servers in a failover pair will now perform ddns removals if
+  they had performed ddns updates on a lease that is expiring, or was
+  released through the primary.  As part of the same fix, stale binding scopes
+  will now be removed if a change in identity of a lease's active client is
+  detected, rather than simply if a lease is noticed to have expired (which it
+  may have expired without a failover server noticing in some situations).
+
                        Changes since 4.1.0b1
 
 - A missing "else" in dhcrelay.c could have caused an interface not to

diff --git a/server/failover.c b/server/failover.c
index 78728c1ca7f9fb5ef425ef98ee3d1908c87936c7..3793faddcf8fb67ff62608398a06528cecc105ed 100644 (file)
--- a/server/failover.c
+++ b/server/failover.c
@@ -5016,6 +5016,8 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
        int new_binding_state;
        int send_to_backup = 0;
        int required_options;
+       isc_boolean_t chaddr_changed = ISC_FALSE;
+       isc_boolean_t ident_changed = ISC_FALSE;
 
        /* Validate the binding update. */
        required_options = FTB_ASSIGNED_IP_ADDRESS | FTB_BINDING_STATUS;
@@ -5085,6 +5087,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
                        message = "chaddr too long";
                        goto bad;
                }
+
+               if ((lt->hardware_addr.hlen != msg->chaddr.count) ||
+                   (memcmp(lt->hardware_addr.hbuf, msg->chaddr.data,
+                           msg->chaddr.count) != 0))
+                       chaddr_changed = ISC_TRUE;
+
                lt -> hardware_addr.hlen = msg -> chaddr.count;
                memcpy (lt -> hardware_addr.hbuf, msg -> chaddr.data,
                        msg -> chaddr.count);
@@ -5095,6 +5103,7 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
                reason = FTR_MISSING_BINDINFO;
                goto bad;
        } else if (msg->binding_status == FTS_ABANDONED) {
+               chaddr_changed = ISC_TRUE;
                lt->hardware_addr.hlen = 0;
                if (lt->scope)
                        binding_scope_dereference(&lt->scope, MDL);
@@ -5110,6 +5119,12 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
                        goto bad;
                }
 
+               if ((lt->uid_len != msg->client_identifier.count) ||
+                   (lt->uid == NULL) || /* Sanity; should never happen. */
+                   (memcmp(lt->uid, msg->client_identifier.data,
+                           lt->uid_len) != 0))
+                       ident_changed = ISC_TRUE;
+
                lt->uid_len = msg->client_identifier.count;
 
                /* Allocate the lt->uid buffer if we haven't already, or
@@ -5138,15 +5153,45 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state,
        } else if (lt->uid && msg->binding_status != FTS_RESET &&
                   msg->binding_status != FTS_FREE &&
                   msg->binding_status != FTS_BACKUP) {
+               ident_changed = ISC_TRUE;
                if (lt->uid != lt->uid_buf)
                        dfree (lt->uid, MDL);
                lt->uid = NULL;
                lt->uid_max = lt->uid_len = 0;
        }
 
-       /* If the lease was expired, also remove the stale binding scope. */
-       if (lt->scope && lt->ends < cur_time)
-               binding_scope_dereference(&lt->scope, MDL);
+       /*
+        * A server's configuration can assign a 'binding scope';
+        *
+        *      set var = "value";
+        *
+        * The problem with these binding scopes is that they are refreshed
+        * when the server processes a client's DHCP packet.  A local binding
+        * scope is trash, then, when the lease has been assigned by the
+        * partner server.  There is no real way to detect this, a peer may
+        * be updating us (as through potential conflict) with a binding we
+        * sent them, but we can trivially detect the /problematic/ case;
+        *
+        *      lease is free.
+        *      primary allocates lease to client A, assigns ddns name A.
+        *      primary fails.
+        *      secondary enters partner down.
+        *      lease expires, and is set free.
+        *      lease is allocated to client B and given ddns name B.
+        *      primary recovers.
+        *
+        * The binding update in this case will be active->active, but the
+        * client identification on the lease will have changed.  The ddns
+        * update on client A will have leaked if we just remove the binding
+        * scope blindly.
+        */
+       if (msg->binding_status == FTS_ACTIVE &&
+           (chaddr_changed || ident_changed)) {
+               ddns_removals(lease, NULL);
+
+               if (lease->scope != NULL)
+                       binding_scope_dereference(&lease->scope, MDL);
+       }
 
        /* XXX Times may need to be adjusted based on clock skew! */
        if (msg -> options_present & FTB_STOS) {

diff --git a/server/mdb.c b/server/mdb.c
index 2f04aa3c4103df4955126630c902f352ae9b4a8f..5e333555a843bfc95154e9cd8c7554b34b91bfe9 100644 (file)
--- a/server/mdb.c
+++ b/server/mdb.c
@@ -1462,6 +1462,21 @@ void make_binding_state_transition (struct lease *lease)
              lease -> binding_state == FTS_ACTIVE &&
              lease -> next_binding_state == FTS_RELEASED))) {
 #if defined (NSUPDATE)
+               /*
+                * Note: ddns_removals() is also iterated when the lease
+                * enters state 'released' in 'release_lease()'.  The below
+                * is caught when a peer receives a BNDUPD from a failover
+                * peer; it may not have received the client's release (it
+                * may have been offline).
+                *
+                * We could remove the call from release_lease() because
+                * it will also catch here on the originating server after the
+                * peer acknowledges the state change.  However, there could
+                * be many hours inbetween, and in this case we /know/ the
+                * client is no longer using the lease when we receive the
+                * release message.  This is not true of expiry, where the
+                * peer may have extended the lease.
+                */
                ddns_removals(lease, NULL);
 #endif
                if (lease -> on_release) {