Persist and reload message counters on the controller

src/controller/CHIPDevice.cpp

@@ -46,9 +46,12 @@
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/ErrorStr.h>
+#include <support/PersistentStorageUtils.h>
 #include <support/SafeInt.h>
 #include <support/logging/CHIPLogging.h>
 #include <system/TLVPacketBufferBackingStore.h>
+#include <transport/MessageCounter.h>
+#include <transport/PeerMessageCounter.h>
 
 using namespace chip::Inet;
 using namespace chip::System;
@@ -150,8 +153,10 @@ CHIP_ERROR Device::SendCommands(app::CommandSender * commandObj)
 
 CHIP_ERROR Device::Serialize(SerializedDevice & output)
 {
-    CHIP_ERROR error       = CHIP_NO_ERROR;
-    uint16_t serializedLen = 0;
+    CHIP_ERROR error             = CHIP_NO_ERROR;
+    uint16_t serializedLen       = 0;
+    uint32_t localMessageCounter = 0;
+    uint32_t peerMessageCounter  = 0;
     SerializableDevice serializable;
 
     static_assert(BASE64_ENCODED_LEN(sizeof(serializable)) <= sizeof(output.inner),
@@ -164,6 +169,14 @@ CHIP_ERROR Device::Serialize(SerializedDevice & output)
     serializable.mDevicePort = Encoding::LittleEndian::HostSwap16(mDeviceAddress.GetPort());
     serializable.mAdminId    = Encoding::LittleEndian::HostSwap16(mAdminId);
 
+    Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+    VerifyOrExit(connectionState != nullptr, error = CHIP_ERROR_INCORRECT_STATE);
+    localMessageCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter().Value();
+    peerMessageCounter  = connectionState->GetSessionMessageCounter().GetPeerMessageCounter().GetCounter();
+
+    serializable.mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(localMessageCounter);
+    serializable.mPeerMessageCounter  = Encoding::LittleEndian::HostSwap32(peerMessageCounter);
+
     serializable.mCASESessionKeyId           = Encoding::LittleEndian::HostSwap16(mCASESessionKeyId);
     serializable.mDeviceProvisioningComplete = (mDeviceProvisioningComplete) ? 1 : 0;
 
@@ -215,10 +228,12 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input)
         IPAddress::FromString(Uint8::to_const_char(serializable.mDeviceAddr), sizeof(serializable.mDeviceAddr) - 1, ipAddress),
         error = CHIP_ERROR_INVALID_ADDRESS);
 
-    mPairing  = serializable.mOpsCreds;
-    mDeviceId = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId);
-    port      = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort);
-    mAdminId  = Encoding::LittleEndian::HostSwap16(serializable.mAdminId);
+    mPairing             = serializable.mOpsCreds;
+    mDeviceId            = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId);
+    port                 = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort);
+    mAdminId             = Encoding::LittleEndian::HostSwap16(serializable.mAdminId);
+    mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(serializable.mLocalMessageCounter);
+    mPeerMessageCounter  = Encoding::LittleEndian::HostSwap32(serializable.mPeerMessageCounter);
 
     mCASESessionKeyId           = Encoding::LittleEndian::HostSwap16(serializable.mCASESessionKeyId);
     mDeviceProvisioningComplete = (serializable.mDeviceProvisioningComplete != 0);
@@ -262,6 +277,17 @@ void Device::OnNewConnection(SecureSessionHandle session)
 {
     mState         = ConnectionState::SecureConnected;
     mSecureSession = session;
+
+    // Reset the message counters here because this is the first time we get a handle to the secure session.
+    Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+    VerifyOrReturn(connectionState != nullptr);
+    MessageCounter & localCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter();
+    if (localCounter.SetCounter(mLocalMessageCounter))
+    {
+        ChipLogError(Controller, "Unable to restore local counter to %d", mLocalMessageCounter);
+    };
+    Transport::PeerMessageCounter & peerCounter = connectionState->GetSessionMessageCounter().GetPeerMessageCounter();
+    peerCounter.SetCounter(mPeerMessageCounter);
 }
 
 void Device::OnConnectionExpired(SecureSessionHandle session)
@@ -470,5 +496,29 @@ void Device::AddReportHandler(EndpointId endpoint, ClusterId cluster, AttributeI
     mCallbacksMgr.AddReportCallback(mDeviceId, endpoint, cluster, attribute, onReportCallback);
 }
 
+Device::~Device()
+{
+    if (mExchangeMgr)
+    {
+        // Ensure that any exchange contexts we have open get closed now,
+        // because we don't want them to call back in to us after this
+        // point.
+        mExchangeMgr->CloseAllContextsForDelegate(this);
+    }
+
+    if (mStorageDelegate)
+    {
+        // Store the current device in persistent storage so we have the latest
+        // message counters available next time.
+        Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+        VerifyOrReturn(connectionState != nullptr);
+        SerializedDevice serialized;
+        Serialize(serialized);
+        // TODO: no need to base-64 the serialized values AGAIN
+        PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key,
+                          mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner)));
+    }
+}
+
 } // namespace Controller
 } // namespace chip

src/controller/CHIPDevice.h

@@ -73,11 +73,11 @@ using DeviceTransportMgr = TransportMgr<Transport::UDP /* IPv6 */
 
 struct ControllerDeviceInitParams
 {
-    DeviceTransportMgr * transportMgr        = nullptr;
-    SecureSessionMgr * sessionMgr            = nullptr;
-    Messaging::ExchangeManager * exchangeMgr = nullptr;
-    Inet::InetLayer * inetLayer              = nullptr;
-
+    DeviceTransportMgr * transportMgr                   = nullptr;
+    SecureSessionMgr * sessionMgr                       = nullptr;
+    Messaging::ExchangeManager * exchangeMgr            = nullptr;
+    Inet::InetLayer * inetLayer                         = nullptr;
+    PersistentStorageDelegate * storageDelegate         = nullptr;
     Credentials::OperationalCredentialSet * credentials = nullptr;
 #if CONFIG_NETWORK_LAYER_BLE
     Ble::BleLayer * bleLayer = nullptr;
@@ -87,16 +87,7 @@ struct ControllerDeviceInitParams
 class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEstablishmentDelegate
 {
 public:
-    ~Device()
-    {
-        if (mExchangeMgr)
-        {
-            // Ensure that any exchange contexts we have open get closed now,
-            // because we don't want them to call back in to us after this
-            // point.
-            mExchangeMgr->CloseAllContextsForDelegate(this);
-        }
-    }
+    ~Device();
 
     enum class PairingWindowOption
     {
@@ -173,13 +164,14 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
      */
     void Init(ControllerDeviceInitParams params, uint16_t listenPort, Transport::AdminId admin)
     {
-        mTransportMgr   = params.transportMgr;
-        mSessionManager = params.sessionMgr;
-        mExchangeMgr    = params.exchangeMgr;
-        mInetLayer      = params.inetLayer;
-        mListenPort     = listenPort;
-        mAdminId        = admin;
-        mCredentials    = params.credentials;
+        mTransportMgr    = params.transportMgr;
+        mSessionManager  = params.sessionMgr;
+        mExchangeMgr     = params.exchangeMgr;
+        mInetLayer       = params.inetLayer;
+        mListenPort      = listenPort;
+        mAdminId         = admin;
+        mStorageDelegate = params.storageDelegate;
+        mCredentials     = params.credentials;
 #if CONFIG_NETWORK_LAYER_BLE
         mBleLayer = params.bleLayer;
 #endif
@@ -406,6 +398,10 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
 
     uint8_t mSequenceNumber = 0;
 
+    // Message counts start at 1
+    uint32_t mLocalMessageCounter = 1;
+    uint32_t mPeerMessageCounter  = 1;
+
     app::CHIPDeviceCallbacksMgr & mCallbacksMgr = app::CHIPDeviceCallbacksMgr::GetInstance();
 
     /**
@@ -440,6 +436,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
     uint16_t mCASESessionKeyId = 0;
 
     Credentials::OperationalCredentialSet * mCredentials = nullptr;
+
+    PersistentStorageDelegate * mStorageDelegate = nullptr;
 };
 
 /**
@@ -494,6 +492,8 @@ typedef struct SerializableDevice
     uint8_t mDeviceTransport;
     uint8_t mDeviceProvisioningComplete;
     uint8_t mInterfaceName[kMaxInterfaceName];
+    uint32_t mLocalMessageCounter; /* This field is serialized in LittleEndian byte order */
+    uint32_t mPeerMessageCounter;  /* This field is serialized in LittleEndian byte order */
 } SerializableDevice;
 
 typedef struct SerializedDevice

src/controller/CHIPDeviceController.cpp

@@ -53,6 +53,7 @@
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/ErrorStr.h>
+#include <support/PersistentStorageUtils.h>
 #include <support/SafeInt.h>
 #include <support/ScopedBuffer.h>
 #include <support/TimeUtils.h>
@@ -89,10 +90,6 @@ namespace Controller {
 
 using namespace chip::Encoding;
 
-constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices";
-constexpr const char kPairedDeviceKeyPrefix[]     = "PairedDevice";
-constexpr const char kNextAvailableKeyID[]        = "StartKeyID";
-
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
 constexpr uint16_t kMdnsPort = 5353;
 #endif
@@ -103,20 +100,6 @@ constexpr uint32_t kMaxCHIPOpCertLength = 1024;
 constexpr uint32_t kMaxCHIPCSRLength    = 1024;
 constexpr uint32_t kOpCSRNonceLength    = 32;
 
-// This macro generates a key using node ID an key prefix, and performs the given action
-// on that key.
-#define PERSISTENT_KEY_OP(node, keyPrefix, key, action)                                                                            \
-    do                                                                                                                             \
-    {                                                                                                                              \
-        constexpr size_t len = std::extent<decltype(keyPrefix)>::value;                                                            \
-        nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time");                                          \
-        /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */                                                \
-        char key[len + 2 * sizeof(NodeId) + 1];                                                                                    \
-        nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected");                          \
-        snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node);                                                                 \
-        action;                                                                                                                    \
-    } while (0)
-
 DeviceController::DeviceController()
 {
     mState                    = State::NotInitialized;

src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m

@@ -392,10 +392,9 @@ - (void)onPairingComplete:(NSError *)error
     if (error.code != CHIPSuccess) {
         NSLog(@"Got pairing error back %@", error);
     } else {
-        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, DISPATCH_TIME_NOW), dispatch_get_main_queue(), ^{
+        dispatch_async(dispatch_get_main_queue(), ^{
             [self->_deviceList refreshDeviceList];
             [self retrieveAndSendWifiCredentials];
-            [self setVendorIDOnAccessory];
         });
     }
 }
@@ -623,6 +622,7 @@ - (void)onAddressUpdated:(NSError *)error
         NSLog(@"Error retrieving device informations over Mdns: %@", error);
         return;
     }
+    [self setVendorIDOnAccessory];
 }
 
 - (void)updateUIFields:(CHIPSetupPayload *)payload decimalString:(nullable NSString *)decimalString

src/lib/support/BUILD.gn

@@ -77,6 +77,7 @@ static_library("support") {
     "LifetimePersistedCounter.h",
     "PersistedCounter.cpp",
     "PersistedCounter.h",
+    "PersistentStorageUtils.h",
     "Pool.cpp",
     "Pool.h",
     "PrivateHeap.cpp",

src/lib/support/PersistentStorageUtils.h

@@ -0,0 +1,48 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+/**
+ *    @file
+ *      This file defines and implements some utlities for generating persistent storage keys
+ *
+ */
+
+#pragma once
+
+#include <support/CodeUtils.h>
+
+namespace chip {
+
+constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices";
+constexpr const char kPairedDeviceKeyPrefix[]     = "PairedDevice";
+constexpr const char kNextAvailableKeyID[]        = "StartKeyID";
+
+// This macro generates a key for storage using a node ID and a key prefix, and performs the given action
+// on that key.
+#define PERSISTENT_KEY_OP(node, keyPrefix, key, action)                                                                            \
+    do                                                                                                                             \
+    {                                                                                                                              \
+        constexpr size_t len = std::extent<decltype(keyPrefix)>::value;                                                            \
+        nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time");                                          \
+        /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */                                                \
+        char key[len + 2 * sizeof(NodeId) + 1];                                                                                    \
+        nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected");                          \
+        snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node);                                                                 \
+        action;                                                                                                                    \
+    } while (0)
+
+} // namespace chip

src/messaging/ExchangeMgr.cpp

@@ -198,8 +198,8 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
     UnsolicitedMessageHandler * matchingUMH = nullptr;
     bool sendAckAndCloseExchange            = false;
 
-    ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d", payloadHeader.GetMessageType(),
-                    payloadHeader.GetProtocolID());
+    ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d on exchange %d", payloadHeader.GetMessageType(),
+                    payloadHeader.GetProtocolID(), payloadHeader.GetExchangeID());
 
     // Search for an existing exchange that the message applies to. If a match is found...
     bool found = false;

src/transport/MessageCounter.h

@@ -47,10 +47,11 @@ class MessageCounter
 
     virtual ~MessageCounter() = 0;
 
-    virtual Type GetType()       = 0;
-    virtual void Reset()         = 0;
-    virtual uint32_t Value()     = 0; /** Get current value */
-    virtual CHIP_ERROR Advance() = 0; /** Advance the counter */
+    virtual Type GetType()                        = 0;
+    virtual void Reset()                          = 0;
+    virtual uint32_t Value()                      = 0; /** Get current value */
+    virtual CHIP_ERROR Advance()                  = 0; /** Advance the counter */
+    virtual CHIP_ERROR SetCounter(uint32_t count) = 0; /** Set the counter to the specified value */
 };
 
 inline MessageCounter::~MessageCounter() {}
@@ -71,6 +72,12 @@ class GlobalUnencryptedMessageCounter : public MessageCounter
         ++value;
         return CHIP_NO_ERROR;
     }
+    CHIP_ERROR SetCounter(uint32_t count) override
+    {
+        Reset();
+        value = count;
+        return CHIP_NO_ERROR;
+    }
 
 private:
     uint32_t value;
@@ -89,6 +96,7 @@ class GlobalEncryptedMessageCounter : public MessageCounter
     }
     uint32_t Value() override { return persisted.GetValue(); }
     CHIP_ERROR Advance() override { return persisted.Advance(); }
+    CHIP_ERROR SetCounter(uint32_t count) override { return CHIP_ERROR_NOT_IMPLEMENTED; }
 
 private:
 #if CONFIG_DEVICE_LAYER
@@ -127,6 +135,12 @@ class LocalSessionMessageCounter : public MessageCounter
         ++value;
         return CHIP_NO_ERROR;
     }
+    CHIP_ERROR SetCounter(uint32_t count) override
+    {
+        Reset();
+        value = count;
+        return CHIP_NO_ERROR;
+    }
 
 private:
     uint32_t value;

src/transport/PeerMessageCounter.h

@@ -148,7 +148,6 @@ class PeerMessageCounter
         mSynced.mWindow.reset();
     }
 
-    /* Test-only */
     uint32_t GetCounter() { return mSynced.mMaxCounter; }
 
 private:

src/transport/SecureSessionMgr.cpp

@@ -370,11 +370,7 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
         {
             ChipLogError(Inet, "Message counter verify failed, err = %d", err);
         }
-        // TODO - Enable exit on error for message counter verification failure.
-        //        We are now using IM messages in commissioner class to provision op creds and
-        //        other device commissioning steps. This is somehow causing issues with message counter
-        //        verification. Disabling this check for now. Enable it after debugging the cause.
-        // SuccessOrExit(err);
+        SuccessOrExit(err);
     }
 
     admin = mAdmins->FindAdminWithId(state->GetAdminId());