[tcat] Fix dataset-write authorization/callback for Decommission command (#13182)

Previously, the case of the TCAT Commissioner doing a decommission
operation did not restore its authorization to perform
active-dataset-writes, if that authorization was already revoked due
to an 'external-triggered' active dataset change.

This PR fixes it: a decommission by the TCAT Commissioner will now
restore its authorization for active-dataset-writes within the same
session. It renames the related flag to `mIsSourceOfDatasetChange`,
making it more generic, and adds a test to check the new behavior for
multiple decommission operations invoked. To be able to test this, the
HandleDecommission() function needed to be split into 2 parts such that
the unit test mock function can do Decommission() also.

The callback to the application for HandleDecommission() is also fixed
to be called in unsuccessful error cases, as required by the API.

src/core/common/settings.cpp

@@ -226,7 +226,7 @@ void Settings::DeleteOperationalDataset(MeshCoP::Dataset::Type aType)
 }
 
 #if OPENTHREAD_CONFIG_BLE_TCAT_ENABLE
-void Settings::SaveTcatCommissionerCertificate(uint8_t *aCert, uint16_t aCertLen)
+void Settings::SaveTcatCommissionerCertificate(const uint8_t *aCert, uint16_t aCertLen)
 {
     Error error = Get<SettingsDriver>().Set(kKeyTcatCommrCert, aCert, aCertLen);
 

src/core/common/settings.hpp

@@ -770,7 +770,7 @@ public:
      * @param[in]  aCert            The DER-encoded X509 end-entity certificate to store.
      * @param[in]  aCertLen         Certificate length.
      */
-    void SaveTcatCommissionerCertificate(uint8_t *aCert, uint16_t aCertLen);
+    void SaveTcatCommissionerCertificate(const uint8_t *aCert, uint16_t aCertLen);
 
     /**
      * Reads the Tcat Commissioner certificate.

src/core/meshcop/tcat_agent.cpp

@@ -82,7 +82,7 @@ void TcatAgent::ClearCommissionerState(void)
     mInstallCodeVerified           = false;
     mCanOverwriteDataset           = false;
     mApplicationResponsePending    = false;
-    mHasWrittenActiveDataset       = false;
+    mIsSourceOfDatasetChange       = false;
 }
 
 Error TcatAgent::Start(AppDataReceiveCallback aAppDataReceiveCallback, JoinCallback aJoinHandler, void *aContext)
@@ -602,7 +602,7 @@ Error TcatAgent::HandleSetActiveOperationalDataset(const Message &aIncomingMessa
     // Flag lets HandleNotifierEvents() know that the Agent is the source of the change. In theory, this could be
     // coalesced with another module's dataset write at exactly the same time, but this is in practice impossible
     // to exploit as an attack vector by the TCAT Commissioner.
-    mHasWrittenActiveDataset = true;
+    mIsSourceOfDatasetChange = true;
 
 exit:
     return error;
@@ -697,6 +697,15 @@ Error TcatAgent::HandleDecommission(void)
     VerifyOrExit(IsCommandClassAuthorized(kDecommissioning), error = kErrorRejected);
     SuccessOrExit(error = Get<Ble::BleSecure>().GetPeerCertificateDer(buf, &bufLen, bufLen));
 
+    Decommission(buf, static_cast<uint16_t>(bufLen));
+
+exit:
+    mJoinCallback.InvokeIfSet(&GetInstance(), /* aIsJoin */ false, error);
+    return error;
+}
+
+void TcatAgent::Decommission(const uint8_t *aCommissionerCert, uint16_t aCertLength)
+{
     Get<Mle::Mle>().Stop();
 
     if (!mVendorInfo->mDoNotActivateAfterLeaving)
@@ -708,7 +717,7 @@ Error TcatAgent::HandleDecommission(void)
     Get<PendingDatasetManager>().Clear();
 
     IgnoreReturnValue(Get<Instance>().ErasePersistentInfo());
-    Get<Settings>().SaveTcatCommissionerCertificate(buf, static_cast<uint16_t>(bufLen));
+    Get<Settings>().SaveTcatCommissionerCertificate(aCommissionerCert, aCertLength);
 
 #if !OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE
     {
@@ -718,11 +727,8 @@ Error TcatAgent::HandleDecommission(void)
     }
 #endif
 
-    mJoinCallback.InvokeIfSet(&GetInstance(), /* aIsJoin */ false, error);
-    mCanOverwriteDataset = true; // enable repeated commissioning/decommissioning cycles in a session
-
-exit:
-    return error;
+    mCanOverwriteDataset     = true; // enable repeated commissioning/decommissioning cycles in a session
+    mIsSourceOfDatasetChange = true; // record that we made the dataset change (causing callback event later)
 }
 
 Error TcatAgent::HandlePing(const Message     &aIncomingMessage,
@@ -1119,11 +1125,11 @@ void TcatAgent::HandleNotifierEvents(Events aEvents)
 
     // Change of network key or ExtPanId by another process: it wrote a dataset for *another* Thread Network.
     // This event revokes the Commissioner's existing authorization (if any) to rewrite datasets.
-    if (!mHasWrittenActiveDataset && aEvents.ContainsAny(kEventNetworkKeyChanged | kEventThreadExtPanIdChanged))
+    if (!mIsSourceOfDatasetChange && aEvents.ContainsAny(kEventNetworkKeyChanged | kEventThreadExtPanIdChanged))
     {
         mCanOverwriteDataset = false;
     }
-    mHasWrittenActiveDataset = false;
+    mIsSourceOfDatasetChange = false;
 
     if (aEvents.Contains(kEventPskcChanged))
     {

src/core/meshcop/tcat_agent.hpp

@@ -459,6 +459,7 @@ private:
                                   const OffsetRange &aOffsetRange,
                                   bool              &aResponse);
     Error HandleDecommission(void);
+    void  Decommission(const uint8_t *aCommissionerCert, uint16_t aCertLength);
     Error HandlePing(const Message     &aIncomingMessage,
                      Message           &aOutgoingMessage,
                      const OffsetRange &aOffsetRange,
@@ -525,7 +526,7 @@ private:
     bool                             mInstallCodeVerified : 1;
     bool                             mCanOverwriteDataset : 1;
     bool                             mApplicationResponsePending : 1;
-    bool                             mHasWrittenActiveDataset : 1;
+    bool                             mIsSourceOfDatasetChange : 1;
     using ExpireTimer = TimerMilliIn<TcatAgent, &TcatAgent::HandleTimer>;
     ExpireTimer     mActiveOrStandbyTimer;
     uint32_t        mTcatActiveDurationMs;

tests/unit/test_tcat.cpp

@@ -606,7 +606,7 @@ private:
     {
         aInstance->Get<Mle::Mle>().Disable();
         aInstance->Get<ActiveDatasetManager>().SaveLocal(aDataset);
-        aInstance->Get<TcatAgent>().mHasWrittenActiveDataset = true;
+        aInstance->Get<TcatAgent>().mIsSourceOfDatasetChange = true;
         otTaskletsProcess(aInstance);
     }
 
@@ -626,6 +626,22 @@ private:
         otTaskletsProcess(aInstance);
     }
 
+    // Mock operation: TCAT Commissioner sends the Decommission command. `HandleDecommission()` itself can't be
+    // called here because it reads the peer certificate from a real (mbedtls) TLS session, which these unit
+    // tests don't set up. So its authorization check is mimicked and the rest of it - `Decommission()` - is
+    // invoked with a stand-in commissioner certificate.
+    static void MockDecommission(Instance *aInstance)
+    {
+        static uint8_t sCommissionerCert[] = {0x30, 0x82, 0x01, 0x00};
+
+        TcatAgent &agent = aInstance->Get<TcatAgent>();
+
+        VerifyOrQuit(agent.IsCommandClassAuthorized(TcatAgent::kDecommissioning));
+        agent.Decommission(sCommissionerCert, sizeof(sCommissionerCert));
+        agent.mJoinCallback.InvokeIfSet(aInstance, /* aIsJoin */ false, kErrorNone);
+        otTaskletsProcess(aInstance);
+    }
+
     // Mock operation: the device attaches to a Thread network. Implemented by directly becoming Leader,
     // which is synchronous (no need to drive the attach state machine) and signals `kEventThreadRoleChanged`
     // just like a real attach. Requires a complete Active Dataset to be present.
@@ -1250,6 +1266,59 @@ public:
         testFreeInstance(instance);
     }
 
+    // Verifies that a Decommission command restores the Commissioner's authorization to (over)write the Active
+    // Dataset within the same TCAT session, so that commissioning/decommissioning cycles can be repeated.
+    static void TestTcatDecommissionRestoresDatasetWrite(void)
+    {
+        Instance        *instance = TestInitInstanceTcat();
+        TcatAgent       *agent    = &instance->Get<TcatAgent>();
+        TcatJoinCounters counters = {};
+
+        // A device that was commissioned by some other entity (not this TCAT Agent) before the session started.
+        MockActiveDatasetChanged(instance, sFullDataset);
+        VerifyOrQuit(instance->Get<ActiveDatasetManager>().IsCommissioned());
+
+        // The Commissioner connects: it is not authorized to overwrite the existing Active Dataset, but it is
+        // authorized to decommission the device.
+        MockCommissionerConnected(agent, sCommAuth, sDeviceAuth, /* aIsCommissionedAtStart */ true);
+        agent->mJoinCallback.Set(HandleTcatJoin, &counters);
+        VerifyOrQuit(CommandClassesAuthorized(agent, kClassGeneral | kClassCommissioning | kClassExtraction |
+                                                         kClassDecommissioning | kClassApplication));
+        VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sFullDataset));
+        VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sPartialDataset));
+
+        for (int i = 0; i < 3; i++)
+        {
+            // Decommission erases the Active Dataset and reports a successful 'leave'.
+            MockDecommission(instance);
+            VerifyOrQuit(!instance->Get<ActiveDatasetManager>().IsCommissioned());
+            VerifyOrQuit(instance->Get<Mle::Mle>().IsDisabled());
+            VerifyOrQuit(counters.mLeaveCount == static_cast<uint32_t>(i + 1) && counters.mJoinCount == 0 &&
+                         counters.mLastError == kErrorNone);
+
+            // Regression test: decommissioning clears the Network Key, which signals a Notifier event that would
+            // otherwise be mistaken for an external dataset change and immediately revoke the authorization again.
+            VerifyOrQuit(agent->mCanOverwriteDataset, "Decommission must restore dataset-write authorization");
+            VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sFullDataset));
+            VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sPartialDataset));
+
+            // The Commissioner recommissions the device in the same session, and may still overwrite afterwards.
+            MockWriteActiveDataset(instance, sFullDataset);
+            VerifyOrQuit(instance->Get<ActiveDatasetManager>().IsCommissioned());
+            VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sFullDataset));
+            VerifyOrQuit(IsSetActiveDatasetSuccessful(agent, sPartialDataset));
+        }
+
+        // A dataset change by another module still revokes the authorization after a Decommission.
+        MockDecommission(instance);
+        VerifyOrQuit(agent->mCanOverwriteDataset);
+        MockActiveDatasetChanged(instance, sFullDataset);
+        VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sFullDataset));
+        VerifyOrQuit(!IsSetActiveDatasetSuccessful(agent, sPartialDataset));
+
+        testFreeInstance(instance);
+    }
+
     // Verifies the OpenThread Notifier assumption that TcatAgent::HandleNotifierEvents() relies on:
     // multiple state changes that occur back-to-back are coalesced into a single notification carrying all of
     // the corresponding event flags.
@@ -1279,7 +1348,7 @@ public:
             changed.mNetworkKey.m8[0] ^= 0xff;
             changed.mExtendedPanId.m8[0] ^= 0xff;
             instance->Get<ActiveDatasetManager>().SaveLocal(changed);
-            agent->mHasWrittenActiveDataset = true; // model that this Agent is the source of the change
+            agent->mIsSourceOfDatasetChange = true; // model that this Agent is the source of the change
         }
 
         // repeat the tasklets processing to ensure subsequent notifer calls are not made.
@@ -1293,7 +1362,7 @@ public:
             VerifyOrQuit(observer.mExtPanIdCount == 1, "Extended PAN ID change must be reported exactly once");
             VerifyOrQuit(observer.mBothInOneCount == 1, "both changes must be coalesced into the same notification");
 
-            // Because the agent saw both events coalesced while mHasWrittenActiveDataset was set, it recognizes the
+            // Because the agent saw both events coalesced while mIsSourceOfDatasetChange was set, it recognizes the
             // change as its own and retains the Commissioner's authorization to overwrite the dataset.
             VerifyOrQuit(agent->mCanOverwriteDataset, "a self-made dataset change must not revoke authorization");
         }
@@ -1324,6 +1393,7 @@ int main(void)
     ot::MeshCoP::UnitTester::TestTcatDatasetOverwrite();
     ot::MeshCoP::UnitTester::TestTcatDatasetOverwriteAfterAttach();
     ot::MeshCoP::UnitTester::TestTcatRepeatedCommandActivation();
+    ot::MeshCoP::UnitTester::TestTcatDecommissionRestoresDatasetWrite();
     ot::MeshCoP::UnitTester::TestTcatNotifierCoalescesEvents();
     printf("All tests passed\n");
 #else