From d31b67b97e11abf3c66b7f961d7750cfd9c9776f Mon Sep 17 00:00:00 2001 From: Sagar Dhawan Date: Thu, 27 May 2021 14:46:05 -0700 Subject: [PATCH] Persist and reload message counters on the controller (#7106) * Persist and reload message counters on the controller * Refactor CHIPDevice storage into an API on CHIPDevice * Rename PersistentStorageUtils -> PersistentStorageMacros * Fix the cluster tests * Fix ordering of initializers * Update comment to describe why counters are being stored * Moved comments around * Fix typo * Address review comments * remove unnecessary semicolon Co-authored-by: Boris Zbarsky --- src/controller/CHIPDevice.cpp | 81 +++++++++++++++++-- src/controller/CHIPDevice.h | 50 +++++++----- src/controller/CHIPDeviceController.cpp | 50 ++++-------- .../QRCode/QRCodeViewController.m | 4 +- src/lib/support/BUILD.gn | 1 + src/lib/support/PersistentStorageMacros.h | 48 +++++++++++ src/messaging/ExchangeMgr.cpp | 4 +- src/transport/MessageCounter.h | 22 ++++- src/transport/PeerMessageCounter.h | 1 - src/transport/SecureSessionMgr.cpp | 6 +- 10 files changed, 190 insertions(+), 77 deletions(-) create mode 100644 src/lib/support/PersistentStorageMacros.h diff --git a/src/controller/CHIPDevice.cpp b/src/controller/CHIPDevice.cpp index 41bd381dd6ca14..d0d0c3a70727b9 100644 --- a/src/controller/CHIPDevice.cpp +++ b/src/controller/CHIPDevice.cpp @@ -46,9 +46,12 @@ #include #include #include +#include #include #include #include +#include +#include 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); @@ -258,10 +273,43 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input) return error; } +CHIP_ERROR Device::Persist() +{ + CHIP_ERROR error = CHIP_NO_ERROR; + if (mStorageDelegate != nullptr) + { + SerializedDevice serialized; + SuccessOrExit(error = Serialize(serialized)); + + // TODO: no need to base-64 the serialized values AGAIN + PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key, + error = mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + if (error != CHIP_NO_ERROR) + { + ChipLogError(Controller, "Failed to persist device %d", error); + } + } +exit: + return error; +} + 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. + // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session + // we need to restore the session counters along with the session information. + 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 +518,26 @@ 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 != nullptr && mSessionManager != nullptr) + { + // If a session can be found, persist the device so that we track the newest message counter values + Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession); + if (connectionState != nullptr) + { + Persist(); + } + } +} + } // namespace Controller } // namespace chip diff --git a/src/controller/CHIPDevice.h b/src/controller/CHIPDevice.h index 41b54d8d5f1868..ef8e8cd917e030 100644 --- a/src/controller/CHIPDevice.h +++ b/src/controller/CHIPDevice.h @@ -73,11 +73,11 @@ using DeviceTransportMgr = TransportMgrCloseAllContextsForDelegate(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 @@ -226,6 +218,13 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta **/ CHIP_ERROR Deserialize(const SerializedDevice & input); + /** + * @brief Serialize and store the Device in persistent storage + * + * @return Returns a CHIP_ERROR if either serialization or storage fails + */ + CHIP_ERROR Persist(); + /** * @brief * Called when a new pairing is being established @@ -406,6 +405,9 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta uint8_t mSequenceNumber = 0; + uint32_t mLocalMessageCounter = 0; + uint32_t mPeerMessageCounter = 0; + app::CHIPDeviceCallbacksMgr & mCallbacksMgr = app::CHIPDeviceCallbacksMgr::GetInstance(); /** @@ -440,6 +442,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta uint16_t mCASESessionKeyId = 0; Credentials::OperationalCredentialSet * mCredentials = nullptr; + + PersistentStorageDelegate * mStorageDelegate = nullptr; }; /** @@ -494,6 +498,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 diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 93e57b072f6868..71856925135fee 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -57,6 +57,7 @@ #include #include #include +#include #include #include #include @@ -93,10 +94,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 @@ -107,20 +104,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::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; @@ -486,19 +469,13 @@ CHIP_ERROR DeviceController::UpdateDevice(Device * device, uint64_t fabricId) void DeviceController::PersistDevice(Device * device) { - // mStorageDelegate would not be null for a typical pairing scenario, as Pair() - // requires a valid storage delegate. However, test pairing usecase, that's used - // mainly by test applications, do not require a storage delegate. This is to - // reduce overheads on these tests. - // Let's make sure the delegate object is available before calling into it. - if (mStorageDelegate != nullptr && mState == State::Initialized) + if (mState == State::Initialized) { - SerializedDevice serialized; - device->Serialize(serialized); - - // TODO: no need to base-64 the serialized values AGAIN - PERSISTENT_KEY_OP(device->GetDeviceId(), kPairedDeviceKeyPrefix, key, - mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner))); + device->Persist(); + } + else + { + ChipLogError(Controller, "Failed to persist device. Controller not initialized."); } } @@ -778,11 +755,14 @@ void DeviceController::OnCommissionableNodeFound(const chip::Mdns::Commissionabl ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams() { - return ControllerDeviceInitParams{ .transportMgr = mTransportMgr, - .sessionMgr = mSessionMgr, - .exchangeMgr = mExchangeMgr, - .inetLayer = mInetLayer, - .credentials = &mCredentials }; + return ControllerDeviceInitParams{ + .transportMgr = mTransportMgr, + .sessionMgr = mSessionMgr, + .exchangeMgr = mExchangeMgr, + .inetLayer = mInetLayer, + .storageDelegate = mStorageDelegate, + .credentials = &mCredentials, + }; } DeviceCommissioner::DeviceCommissioner() : diff --git a/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m b/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m index 834d6adf23138d..8816c86b42717c 100644 --- a/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m +++ b/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 diff --git a/src/lib/support/BUILD.gn b/src/lib/support/BUILD.gn index f88bbc0603e18b..72b29d92093350 100644 --- a/src/lib/support/BUILD.gn +++ b/src/lib/support/BUILD.gn @@ -77,6 +77,7 @@ static_library("support") { "LifetimePersistedCounter.h", "PersistedCounter.cpp", "PersistedCounter.h", + "PersistentStorageMacros.h", "Pool.cpp", "Pool.h", "PrivateHeap.cpp", diff --git a/src/lib/support/PersistentStorageMacros.h b/src/lib/support/PersistentStorageMacros.h new file mode 100644 index 00000000000000..3b79004b5344b1 --- /dev/null +++ b/src/lib/support/PersistentStorageMacros.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 + +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::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 diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index e585fe9ee930e2..4613d9025e4e35 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -203,8 +203,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; diff --git a/src/transport/MessageCounter.h b/src/transport/MessageCounter.h index 49d044dc7b4f0e..84ecce104a7419 100644 --- a/src/transport/MessageCounter.h +++ b/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; diff --git a/src/transport/PeerMessageCounter.h b/src/transport/PeerMessageCounter.h index 476150938f4171..2048c98b0af26d 100644 --- a/src/transport/PeerMessageCounter.h +++ b/src/transport/PeerMessageCounter.h @@ -148,7 +148,6 @@ class PeerMessageCounter mSynced.mWindow.reset(); } - /* Test-only */ uint32_t GetCounter() { return mSynced.mMaxCounter; } private: diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index 85cc40c9ddd8c2..cda9a7bc0e6562 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/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());