[Bugfix] make RevokeCommissioning tear down PASESession (project-chip#41715)

* Adding callback to FailSafeContext that is called after the failsafe timer is disarmed

* rregister a callback to close commissioning window after fail-safe is disarmed when RevokeCommissioning is invoked

* Adding Integration Test: Ensure PASE is cleared when Revoke Commissioning is invoked

* Adding Unit Test to make sure RevokeCommissioning properly clears PASESession

* RevokeCommissioning: Stop calling ForceFailSafeTimerExpiry if FailSafe is not armed

* changing fix approach: modify the locations of AddEventHAndler and RemoveEventHandler for CommissioningWindowManager

* Restyled by whitespace

* Restyled by clang-format

* Fix Integration test

* Fix integration test and add a case

* Restyled by autopep8

* Integrating comments

* integrating comments

---------

Co-authored-by: Restyled.io <[email protected]>

Author: Alami-Amine

src/app/server/CommissioningWindowManager.cpp

@@ -138,8 +138,6 @@ void CommissioningWindowManager::ResetState()
 
     DeviceLayer::SystemLayer().CancelTimer(HandleCommissioningWindowTimeout, this);
     mCommissioningTimeoutTimerArmed = false;
-
-    DeviceLayer::PlatformMgr().RemoveEventHandler(OnPlatformEventWrapper, reinterpret_cast<intptr_t>(this));
 }
 
 void CommissioningWindowManager::Cleanup()
@@ -212,8 +210,6 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess
         mAppDelegate->OnCommissioningSessionStarted();
     }
 
-    TEMPORARY_RETURN_IGNORED DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper, reinterpret_cast<intptr_t>(this));
-
     TEMPORARY_RETURN_IGNORED StopAdvertisement(/* aShuttingDown = */ false);
 
     auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
@@ -244,6 +240,8 @@ void CommissioningWindowManager::OnSessionEstablished(const SessionHandle & sess
         // When the now-armed fail-safe is disarmed or expires it will handle
         // clearing out mPASESession.
         mPASESession.Grab(session);
+        TEMPORARY_RETURN_IGNORED DeviceLayer::PlatformMgr().AddEventHandler(OnPlatformEventWrapper,
+                                                                            reinterpret_cast<intptr_t>(this));
     }
 }
 
@@ -591,6 +589,8 @@ void CommissioningWindowManager::OnSessionReleased()
     // session, since we arm it when the PASE session is set up, and anything
     // that disarms the fail-safe would also tear down the PASE session.
     ExpireFailSafeIfArmed();
+
+    DeviceLayer::PlatformMgr().RemoveEventHandler(OnPlatformEventWrapper, reinterpret_cast<intptr_t>(this));
 }
 
 void CommissioningWindowManager::ExpireFailSafeIfArmed()

src/app/server/CommissioningWindowManager.h

@@ -130,6 +130,8 @@ class CommissioningWindowManager : public Messaging::UnsolicitedMessageHandler,
     // commissioning window timeout.
     void OverrideMinCommissioningTimeout(System::Clock::Seconds32 timeout) { mMinCommissioningTimeoutOverride.SetValue(timeout); }
 
+    Optional<SessionHandle> GetPASESession() const { return mPASESession.Get(); }
+
 private:
     //////////// SessionDelegate Implementation ///////////////
     void OnSessionReleased() override;

src/app/tests/TestCommissioningWindowManager.cpp

@@ -36,13 +36,37 @@
 #include <lib/support/tests/ExtraPwTestMacros.h>
 #include <pw_unit_test/framework.h>
 
+#include <lib/support/UnitTestUtils.h>
+#include <messaging/tests/MessagingContext.h>
+
+using namespace chip;
 using namespace chip::Crypto;
+using namespace chip::Messaging;
+using namespace System::Clock::Literals;
 
 using chip::CommissioningWindowAdvertisement;
 using chip::CommissioningWindowManager;
 using chip::Server;
 
 namespace {
+
+// Test Set #01 of Spake2p Parameters (PIN Code, Iteration Count, Salt, and matching Verifier):
+constexpr uint32_t sTestSpake2p01_PinCode        = 20202021;
+constexpr uint32_t sTestSpake2p01_IterationCount = 1000;
+constexpr uint8_t sTestSpake2p01_Salt[]          = { 0x53, 0x50, 0x41, 0x4B, 0x45, 0x32, 0x50, 0x20,
+                                                     0x4B, 0x65, 0x79, 0x20, 0x53, 0x61, 0x6C, 0x74 };
+Spake2pVerifier sTestSpake2p01_PASEVerifier = { .mW0 = {
+    0xB9, 0x61, 0x70, 0xAA, 0xE8, 0x03, 0x34, 0x68, 0x84, 0x72, 0x4F, 0xE9, 0xA3, 0xB2, 0x87, 0xC3,
+    0x03, 0x30, 0xC2, 0xA6, 0x60, 0x37, 0x5D, 0x17, 0xBB, 0x20, 0x5A, 0x8C, 0xF1, 0xAE, 0xCB, 0x35,
+},
+    .mL  = {
+    0x04, 0x57, 0xF8, 0xAB, 0x79, 0xEE, 0x25, 0x3A, 0xB6, 0xA8, 0xE4, 0x6B, 0xB0, 0x9E, 0x54, 0x3A,
+    0xE4, 0x22, 0x73, 0x6D, 0xE5, 0x01, 0xE3, 0xDB, 0x37, 0xD4, 0x41, 0xFE, 0x34, 0x49, 0x20, 0xD0,
+    0x95, 0x48, 0xE4, 0xC1, 0x82, 0x40, 0x63, 0x0C, 0x4F, 0xF4, 0x91, 0x3C, 0x53, 0x51, 0x38, 0x39,
+    0xB7, 0xC0, 0x7F, 0xCC, 0x06, 0x27, 0xA1, 0xB8, 0x57, 0x3A, 0x14, 0x9F, 0xCD, 0x1F, 0xA4, 0x66,
+    0xCF
+} };
+
 bool sAdminFabricIndexDirty = false;
 bool sAdminVendorIdDirty    = false;
 bool sWindowStatusDirty     = false;
@@ -111,11 +135,24 @@ static void StopEventLoop(intptr_t context)
     EXPECT_SUCCESS(chip::DeviceLayer::PlatformMgr().StopEventLoopTask());
 }
 
-class TestCommissioningWindowManager : public ::testing::Test
+class TestSecurePairingDelegate : public SessionEstablishmentDelegate
+{
+public:
+    void OnSessionEstablishmentError(CHIP_ERROR error) override { mNumPairingErrors++; }
+
+    void OnSessionEstablished(const SessionHandle & session) override { mNumPairingComplete++; }
+
+    uint32_t mNumPairingErrors   = 0;
+    uint32_t mNumPairingComplete = 0;
+};
+
+class TestCommissioningWindowManager : public chip::Test::LoopbackMessagingContext
 {
 public:
     static void SetUpTestSuite()
     {
+        LoopbackMessagingContext::SetUpTestSuite();
+
         ASSERT_EQ(chip::Platform::MemoryInit(), CHIP_NO_ERROR);
         ASSERT_EQ(chip::DeviceLayer::PlatformMgr().InitChipStack(), CHIP_NO_ERROR);
 
@@ -152,7 +189,9 @@ class TestCommissioningWindowManager : public ::testing::Test
         RETURN_SAFELY_IGNORED mdnsAdvertiser.RemoveServices();
         mdnsAdvertiser.Shutdown();
 
-        // Server shudown will be called in TearDownTask
+        LoopbackMessagingContext::TearDownTestSuite();
+
+        // Server shutdown will be called in TearDownTask
 
         // TODO: At this point UDP endpoits still seem leaked and the sanitizer
         // builds will attempt a memory free. As a result, we keep Memory initialized
@@ -163,6 +202,83 @@ class TestCommissioningWindowManager : public ::testing::Test
         //
         // chip::Platform::MemoryShutdown();
     }
+
+    void SetUp() override
+    {
+        ConfigInitializeNodes(false);
+        chip::Test::LoopbackMessagingContext::SetUp();
+    }
+
+    void EstablishPASEHandshake(SessionManager & sessionManager, PASESession & pairingCommissioner,
+                                TestSecurePairingDelegate & delegateCommissioner);
+
+    void ServiceEvents();
+};
+
+void TestCommissioningWindowManager::ServiceEvents()
+{
+    DrainAndServiceIO();
+
+    chip::DeviceLayer::PlatformMgr().ScheduleWork([](intptr_t) -> void { chip::DeviceLayer::PlatformMgr().StopEventLoopTask(); },
+                                                  (intptr_t) nullptr);
+    chip::DeviceLayer::PlatformMgr().RunEventLoop();
+}
+
+void TestCommissioningWindowManager::EstablishPASEHandshake(SessionManager & sessionManager, PASESession & pairingCommissioner,
+                                                            TestSecurePairingDelegate & delegateCommissioner)
+{
+
+    auto & loopback            = GetLoopback();
+    loopback.mSentMessageCount = 0;
+
+    ExchangeContext * contextCommissioner = NewUnauthenticatedExchangeToBob(&pairingCommissioner);
+
+    EXPECT_EQ(GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::PBKDFParamRequest,
+                                                                            &Server::GetInstance().GetCommissioningWindowManager()),
+              CHIP_NO_ERROR);
+
+    DrainAndServiceIO();
+
+    EXPECT_EQ(pairingCommissioner.Pair(sessionManager, sTestSpake2p01_PinCode, Optional<ReliableMessageProtocolConfig>::Missing(),
+                                       contextCommissioner, &delegateCommissioner),
+              CHIP_NO_ERROR);
+    DrainAndServiceIO();
+
+    EXPECT_EQ(delegateCommissioner.mNumPairingErrors, 0u);
+    EXPECT_EQ(delegateCommissioner.mNumPairingComplete, 1u);
+}
+
+class TemporarySessionManager
+{
+public:
+    TemporarySessionManager(TestCommissioningWindowManager & ctx) : mCtx(ctx)
+    {
+        EXPECT_EQ(CHIP_NO_ERROR,
+                  mSessionManager.Init(&ctx.GetSystemLayer(), &ctx.GetTransportMgr(), &ctx.GetMessageCounterManager(), &mStorage,
+                                       &ctx.GetFabricTable(), ctx.GetSessionKeystore()));
+        // The setup here is really weird: we are using one session manager for
+        // the actual messages we send (the PASE handshake, so the
+        // unauthenticated sessions) and a different one for allocating the PASE
+        // sessions.  Since our Init() set us up as the thing to handle messages
+        // on the transport manager, undo that.
+        mCtx.GetTransportMgr().SetSessionManager(&mCtx.GetSecureSessionManager());
+    }
+
+    ~TemporarySessionManager()
+    {
+        mSessionManager.Shutdown();
+        // Reset the session manager on the transport again, just in case
+        // shutdown messed with it.
+        mCtx.GetTransportMgr().SetSessionManager(&mCtx.GetSecureSessionManager());
+    }
+
+    operator SessionManager &() { return mSessionManager; }
+
+    TestCommissioningWindowManager & mCtx;
+
+private:
+    TestPersistentStorageDelegate mStorage;
+    SessionManager mSessionManager;
 };
 
 void CheckCommissioningWindowManagerBasicWindowOpenCloseTask(intptr_t context)
@@ -409,4 +525,72 @@ TEST_F(TestCommissioningWindowManager, TestCheckCommissioningWindowManagerEnhanc
     chip::DeviceLayer::PlatformMgr().RunEventLoop();
 }
 
+void RevokeCommissioningCommandEquivalent()
+{
+    Server::GetInstance().GetFailSafeContext().ForceFailSafeTimerExpiry();
+
+    if (!Server::GetInstance().GetCommissioningWindowManager().IsCommissioningWindowOpen())
+    {
+        ChipLogError(Zcl, "Commissioning window is currently not open");
+        return;
+    }
+
+    Server::GetInstance().GetCommissioningWindowManager().CloseCommissioningWindow();
+    ChipLogProgress(Zcl, "Commissioning window is now closed");
+}
+
+TEST_F(TestCommissioningWindowManager, SecurePairingHandshakeTestECM)
+{
+    TemporarySessionManager sessionManager(*this);
+
+    TestSecurePairingDelegate delegateCommissioner;
+    PASESession pairingCommissioner;
+    auto & loopback = GetLoopback();
+    loopback.Reset();
+
+    // Open an Enhanced Commissioning Window
+    uint16_t originDiscriminator;
+    EXPECT_EQ(chip::DeviceLayer::GetCommissionableDataProvider()->GetSetupDiscriminator(originDiscriminator), CHIP_NO_ERROR);
+    uint16_t newDiscriminator = static_cast<uint16_t>(originDiscriminator + 1);
+
+    constexpr auto fabricIndex = static_cast<chip::FabricIndex>(1);
+    constexpr auto vendorId    = static_cast<chip::VendorId>(0xFFF3);
+
+    CommissioningWindowManager & commissionMgr = Server::GetInstance().GetCommissioningWindowManager();
+    EXPECT_EQ(commissionMgr.OpenEnhancedCommissioningWindow(commissionMgr.MaxCommissioningTimeout(), newDiscriminator,
+                                                            sTestSpake2p01_PASEVerifier, sTestSpake2p01_IterationCount,
+                                                            ByteSpan(sTestSpake2p01_Salt), fabricIndex, vendorId),
+              CHIP_NO_ERROR);
+
+    EXPECT_TRUE(commissionMgr.IsCommissioningWindowOpen());
+
+    // Establish PASE Handshake to the server's CommissioningWindowManager
+    EstablishPASEHandshake(sessionManager, pairingCommissioner, delegateCommissioner);
+
+    // Ensure that a PASE Session exists for pairingCommissioner
+    auto commissionerSession = pairingCommissioner.CopySecureSession();
+    EXPECT_TRUE(commissionerSession.HasValue());
+    EXPECT_TRUE(commissionerSession.Value()->AsSecureSession()->IsPASESession());
+
+    // Ensure that a PASE Session exists for the CommissioningWindowManager
+    EXPECT_TRUE(commissionMgr.GetPASESession().HasValue());
+    EXPECT_TRUE(commissionMgr.GetPASESession().Value()->AsSecureSession()->IsPASESession());
+
+    // This is the equivalent of AdministratorCommissioningLogic::RevokeCommissioning() in the AdministratorCommissioning Cluster
+    RevokeCommissioningCommandEquivalent();
+
+    // We need to service events here to allow the Async Events to be processed and make sure that the CommissioningWindowManager
+    // successfully shutdown the PASESession
+    ServiceEvents();
+
+    EXPECT_FALSE(commissionMgr.IsCommissioningWindowOpen());
+
+    // This asserts that the CommissioningWindowManager has cleared the PASESession
+    EXPECT_FALSE(commissionMgr.GetPASESession().HasValue());
+
+    // Asserting that PASESession is still present on the Commissioner side
+    commissionerSession = pairingCommissioner.CopySecureSession();
+    EXPECT_TRUE(commissionerSession.HasValue());
+}
+
 } // namespace