From 213aede48e0f7df3623ba83b4ef0275b8f1b0886 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Fri, 17 May 2024 08:50:30 -0700 Subject: [PATCH] [dataset] move Active Dataset replacement check in `PendingDatasetManager` (#10217) This commit refactors the logic that determines whether to replace the current Active Dataset with a Pending Dataset upon its delay timer expiration. Instead of performing the check within the more general-purpose `DatasetManager::Save()` method, the logic is now moved directly into `PendingDatasetManager::HandleDelayTimer()`. This change makes the code more specific and prevents unintended checks in other situations where `Save()` is used (e.g., when Active Dataset is directly set). The conditions remain the same: The Pending Dataset's Active Timestamp must be newer, or the Pending Dataset must contain a different network key. This commit also adds `test-029-pending-dataset-key-change.py` to validate the roll-back of Active Timestamp using Pending Dataset. --- src/core/meshcop/dataset.cpp | 26 ---- src/core/meshcop/dataset.hpp | 20 --- src/core/meshcop/dataset_manager.cpp | 45 +++++-- src/core/meshcop/dataset_manager.hpp | 7 +- .../test-029-pending-dataset-key-change.py | 127 ++++++++++++++++++ tests/toranj/openthread-core-toranj-config.h | 1 + tests/toranj/start.sh | 1 + 7 files changed, 172 insertions(+), 55 deletions(-) create mode 100755 tests/toranj/cli/test-029-pending-dataset-key-change.py diff --git a/src/core/meshcop/dataset.cpp b/src/core/meshcop/dataset.cpp index 6ed478e93..f63b41254 100644 --- a/src/core/meshcop/dataset.cpp +++ b/src/core/meshcop/dataset.cpp @@ -586,13 +586,6 @@ void Dataset::RemoveTlv(Tlv *aTlv) } Error Dataset::ApplyConfiguration(Instance &aInstance) const -{ - bool isNetworkKeyUpdated; - - return ApplyConfiguration(aInstance, isNetworkKeyUpdated); -} - -Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdated) const { Mac::Mac &mac = aInstance.Get(); KeyManager &keyManager = aInstance.Get(); @@ -600,8 +593,6 @@ Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdate SuccessOrExit(error = ValidateTlvs()); - aIsNetworkKeyUpdated = false; - for (const Tlv *cur = GetTlvsStart(); cur < GetTlvsEnd(); cur = cur->GetNext()) { switch (cur->GetType()) @@ -634,19 +625,8 @@ Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdate break; case Tlv::kNetworkKey: - { - NetworkKey networkKey; - - keyManager.GetNetworkKey(networkKey); - - if (cur->ReadValueAs() != networkKey) - { - aIsNetworkKeyUpdated = true; - } - keyManager.SetNetworkKey(cur->ReadValueAs()); break; - } #if OPENTHREAD_FTD case Tlv::kPskc: @@ -671,12 +651,6 @@ exit: return error; } -void Dataset::ConvertToActive(void) -{ - RemoveTlv(Tlv::kPendingTimestamp); - RemoveTlv(Tlv::kDelayTimer); -} - const char *Dataset::TypeToString(Type aType) { return (aType == kActive) ? "Active" : "Pending"; } #if OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE diff --git a/src/core/meshcop/dataset.hpp b/src/core/meshcop/dataset.hpp index a35d98ea2..485b109ff 100644 --- a/src/core/meshcop/dataset.hpp +++ b/src/core/meshcop/dataset.hpp @@ -647,26 +647,6 @@ public: */ Error ApplyConfiguration(Instance &aInstance) const; - /** - * Applies the Active or Pending Dataset to the Thread interface. - * - * @param[in] aInstance A reference to the OpenThread instance. - * @param[out] aIsNetworkKeyUpdated Variable to return whether network key was updated. - * - * @retval kErrorNone Successfully applied configuration, @p aIsNetworkKeyUpdated is changed. - * @retval kErrorParse The dataset has at least one TLV with invalid format. - * - */ - Error ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdated) const; - - /** - * Converts a Pending Dataset to an Active Dataset. - * - * Removes the Delay Timer and Pending Timestamp TLVs. - * - */ - void ConvertToActive(void); - /** * Returns a pointer to the start of Dataset TLVs sequence. * diff --git a/src/core/meshcop/dataset_manager.cpp b/src/core/meshcop/dataset_manager.cpp index 6c3f0b6c7..f07497d59 100644 --- a/src/core/meshcop/dataset_manager.cpp +++ b/src/core/meshcop/dataset_manager.cpp @@ -143,11 +143,10 @@ exit: return error; } -Error DatasetManager::Save(const Dataset &aDataset) +Error DatasetManager::Save(const Dataset &aDataset, bool aAllowOlderTimestamp) { Error error = kErrorNone; int compare; - bool isNetworkKeyUpdated = false; if (aDataset.ReadTimestamp(GetType(), mTimestamp) == kErrorNone) { @@ -155,13 +154,13 @@ Error DatasetManager::Save(const Dataset &aDataset) if (IsActiveDataset()) { - SuccessOrExit(error = aDataset.ApplyConfiguration(GetInstance(), isNetworkKeyUpdated)); + SuccessOrExit(error = aDataset.ApplyConfiguration(GetInstance())); } } compare = Timestamp::Compare(mTimestampValid ? &mTimestamp : nullptr, mLocal.GetTimestamp()); - if (isNetworkKeyUpdated || compare > 0) + if ((compare > 0) || aAllowOlderTimestamp) { mLocal.Save(aDataset); @@ -669,15 +668,45 @@ exit: void PendingDatasetManager::HandleDelayTimer(void) { - Dataset dataset; + Dataset dataset; + Timestamp activeTimestamp; + bool shouldReplaceActive = false; IgnoreError(Read(dataset)); - LogInfo("pending delay timer expired"); + LogInfo("Pending delay timer expired"); - dataset.ConvertToActive(); + // Determine whether the Pending Dataset should replace the + // current Active Dataset. This is allowed if the Pending + // Dataset's Active Timestamp is newer, or the Pending Dataset + // contains a different key. - IgnoreError(Get().Save(dataset)); + SuccessOrExit(dataset.Read(activeTimestamp)); + if (Timestamp::Compare(&activeTimestamp, Get().GetTimestamp()) > 0) + { + shouldReplaceActive = true; + } + else + { + NetworkKey newKey; + NetworkKey currentKey; + + SuccessOrExit(dataset.Read(newKey)); + Get().GetNetworkKey(currentKey); + shouldReplaceActive = (currentKey != newKey); + } + + VerifyOrExit(shouldReplaceActive); + + // Convert Pending Dataset to Active by removing the Pending + // Timestamp and the Delay Timer TLVs. + + dataset.RemoveTlv(Tlv::kPendingTimestamp); + dataset.RemoveTlv(Tlv::kDelayTimer); + + IgnoreError(Get().Save(dataset, /* aAllowOlderTimestamp */ true)); + +exit: Clear(); } diff --git a/src/core/meshcop/dataset_manager.hpp b/src/core/meshcop/dataset_manager.hpp index eb0b8c5d0..e28a7b432 100644 --- a/src/core/meshcop/dataset_manager.hpp +++ b/src/core/meshcop/dataset_manager.hpp @@ -51,8 +51,12 @@ namespace ot { namespace MeshCoP { +class PendingDatasetManager; + class DatasetManager : public InstanceLocator { + friend class PendingDatasetManager; + public: /** * Callback function pointer, invoked when a response to a MGMT_SET request is received or times out. @@ -149,7 +153,7 @@ public: * @retval kErrorParse The dataset has at least one TLV with invalid format. * */ - Error Save(const Dataset &aDataset); + Error Save(const Dataset &aDataset) { return Save(aDataset, /* aAllowOlderTimestamp */ false); } /** * Sets the Operational Dataset for the partition read from a given message. @@ -347,6 +351,7 @@ private: bool IsActiveDataset(void) const { return GetType() == Dataset::kActive; } bool IsPendingDataset(void) const { return GetType() == Dataset::kPending; } + Error Save(const Dataset &aDataset, bool aAllowOlderTimestamp); void SignalDatasetChange(void) const; void SyncLocalWithLeader(const Dataset &aDataset); Error SendSetRequest(const Dataset &aDataset); diff --git a/tests/toranj/cli/test-029-pending-dataset-key-change.py b/tests/toranj/cli/test-029-pending-dataset-key-change.py new file mode 100755 index 000000000..2fbd50dd8 --- /dev/null +++ b/tests/toranj/cli/test-029-pending-dataset-key-change.py @@ -0,0 +1,127 @@ +#!/usr/bin/env python3 +# +# Copyright (c) 2024, The OpenThread Authors. +# All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# 3. Neither the name of the copyright holder nor the +# names of its contributors may be used to endorse or promote products +# derived from this software without specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" +# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE +# LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR +# CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF +# SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS +# INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN +# CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) +# ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE +# POSSIBILITY OF SUCH DAMAGE. + +from cli import verify +from cli import verify_within +import cli +import time + +# ----------------------------------------------------------------------------------------------------------------------- +# Test description: +# +# This test validates the rollback of the Active Timestamp when +# applying a Pending Dataset. This rollback is permitted only when +# the Network Key changes. + +test_name = __file__[:-3] if __file__.endswith('.py') else __file__ +print('-' * 120) +print('Starting \'{}\''.format(test_name)) + +# ----------------------------------------------------------------------------------------------------------------------- +# Creating `cli.Node` instances + +speedup = 20 +cli.Node.set_time_speedup_factor(speedup) + +leader = cli.Node() +router = cli.Node() + +# ----------------------------------------------------------------------------------------------------------------------- +# Form topology + +leader.form('network') +router.join(leader) + +verify(leader.get_state() == 'leader') +verify(router.get_state() == 'router') + +# ----------------------------------------------------------------------------------------------------------------------- +# Test Implementation + +# Use Pending Dataset to change network name and update +# Active Timestamp to 10. + +router.cli('dataset init active') +router.cli('dataset activetimestamp 10') +router.cli('dataset pendingtimestamp 10') +router.cli('dataset networkname new') +router.cli('dataset delaytimer 1000') +router.cli('dataset commit pending') + +time.sleep(1.2 / speedup) + +# Validate that Active Dataset is updated. + +leader.cli('dataset init active') +verify(leader.get_network_name() == 'new') +verify(leader.cli('dataset activetimestamp') == ['10']) + +#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +# Use Pending Dataset with an older Active Timestamp. +# Ensure that the new dataset is not accepted. + +router.cli('dataset init active') +router.cli('dataset activetimestamp 5') +router.cli('dataset pendingtimestamp 15') +router.cli('dataset networkname shouldfail') +router.cli('dataset delaytimer 1000') +router.cli('dataset commit pending') + +time.sleep(1.2 / speedup) + +leader.cli('dataset init active') +verify(leader.get_network_name() == 'new') +verify(leader.cli('dataset activetimestamp') == ['10']) + +#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - +# Use Pending Dataset with an older Active Timestamp but +# also change the Network Key. + +router.cli('dataset init active') +router.cli('dataset activetimestamp 7') +router.cli('dataset pendingtimestamp 20') +router.cli('dataset networkkey 00112233445566778899aabbccddeeff') +router.cli('dataset delaytimer 1000') +router.cli('dataset commit pending') + +time.sleep(1.2 / speedup) + +# Validate that the Active Dataset is updated and +# Active Timestamp is rolled back. + +leader.cli('dataset init active') +leader.cli('dataset') +verify(leader.cli('dataset activetimestamp') == ['7']) +verify(leader.get_network_key() == '00112233445566778899aabbccddeeff') + +# ----------------------------------------------------------------------------------------------------------------------- +# Test finished + +cli.Node.finalize_all_nodes() + +print('\'{}\' passed.'.format(test_name)) diff --git a/tests/toranj/openthread-core-toranj-config.h b/tests/toranj/openthread-core-toranj-config.h index c1a968916..33686ca7a 100644 --- a/tests/toranj/openthread-core-toranj-config.h +++ b/tests/toranj/openthread-core-toranj-config.h @@ -161,6 +161,7 @@ // For `toranj` test script the value is decreased so that the tests can be run faster. #define OPENTHREAD_CONFIG_TMF_PENDING_DATASET_MINIMUM_DELAY 1000 +#define OPENTHREAD_CONFIG_TMF_PENDING_DATASET_DEFAULT_DELAY 1000 #define OPENTHREAD_CONFIG_NCP_ENABLE_MCU_POWER_STATE_CONTROL 1 diff --git a/tests/toranj/start.sh b/tests/toranj/start.sh index 04cef966c..ebbbdd700 100755 --- a/tests/toranj/start.sh +++ b/tests/toranj/start.sh @@ -193,6 +193,7 @@ if [ "$TORANJ_CLI" = 1 ]; then run cli/test-026-coaps-conn-limit.py run cli/test-027-slaac-address.py run cli/test-028-border-agent-ephemeral-key.py + run cli/test-029-pending-dataset-key-change.py run cli/test-400-srp-client-server.py run cli/test-401-srp-server-address-cache-snoop.py run cli/test-500-two-brs-two-networks.py