Duplicate MRP message handling and test (#7893)

* Duplicate CRMP message handling and test

* address review comments

* update test with review comments

src/lib/core/CHIPError.h

@@ -1785,6 +1785,14 @@ typedef CHIP_CONFIG_ERROR_TYPE CHIP_ERROR;
  */
 #define CHIP_ERROR_INTERMEDIATE_CA_NOT_REQUIRED                      _CHIP_ERROR(190)
 
+/**
+ * @def CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED
+ *
+ * @brief
+ *   The received message is a duplicate of a previously received message.
+ */
+#define CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED                      _CHIP_ERROR(191)
+
 /**
  *  @}
  */

src/messaging/ExchangeContext.cpp

@@ -370,16 +370,17 @@ void ExchangeContext::NotifyResponseTimeout()
 }
 
 CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
-                                          const Transport::PeerAddress & peerAddress, PacketBufferHandle && msgBuf)
+                                          const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
+                                          PacketBufferHandle && msgBuf)
 {
     // We hold a reference to the ExchangeContext here to
     // guard against Close() calls(decrementing the reference
     // count) by the protocol before the CHIP Exchange
     // layer has completed its work on the ExchangeContext.
     Retain();
 
-    CHIP_ERROR err =
-        mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, GetReliableMessageContext());
+    CHIP_ERROR err = mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, msgFlags,
+                                                  GetReliableMessageContext());
     SuccessOrExit(err);
 
     // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer.
@@ -388,6 +389,12 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con
         ExitNow(err = CHIP_NO_ERROR);
     }
 
+    // Since the message is duplicate, let's not forward it up the stack
+    if (msgFlags.Has(MessageFlagValues::kDuplicateMessage))
+    {
+        ExitNow(err = CHIP_NO_ERROR);
+    }
+
     // Since we got the response, cancel the response timer.
     CancelResponseTimer();
 

src/messaging/ExchangeContext.h

@@ -118,6 +118,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
      *
      *  @param[in]    peerAddress   The address of the sender
      *
+     *  @param[in]    msgFlags      The message flags corresponding to the received message
+     *
      *  @param[in]    msgBuf        A handle to the packet buffer holding the CHIP message.
      *
      *  @retval  #CHIP_ERROR_INVALID_ARGUMENT               if an invalid argument was passed to this HandleMessage API.
@@ -126,7 +128,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
      *                                                       protocol layer.
      */
     CHIP_ERROR HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
-                             const Transport::PeerAddress & peerAddress, System::PacketBufferHandle && msgBuf);
+                             const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
+                             System::PacketBufferHandle && msgBuf);
 
     ExchangeDelegate * GetDelegate() const { return mDelegate; }
     void SetDelegate(ExchangeDelegate * delegate) { mDelegate = delegate; }

src/messaging/ExchangeMessageDispatch.cpp

@@ -99,23 +99,21 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SecureSessionHandle session, uin
 }
 
 CHIP_ERROR ExchangeMessageDispatch::OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId,
-                                                      const Transport::PeerAddress & peerAddress,
+                                                      const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
                                                       ReliableMessageContext * reliableMessageContext)
 {
     ReturnErrorCodeIf(!MessagePermitted(payloadHeader.GetProtocolID().GetProtocolId(), payloadHeader.GetMessageType()),
                       CHIP_ERROR_INVALID_ARGUMENT);
 
     if (IsReliableTransmissionAllowed())
     {
-        if (payloadHeader.IsAckMsg() && payloadHeader.GetAckId().HasValue())
+        if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && payloadHeader.GetAckId().HasValue())
         {
             ReturnErrorOnFailure(reliableMessageContext->HandleRcvdAck(payloadHeader.GetAckId().Value()));
         }
 
         if (payloadHeader.NeedsAck())
         {
-            MessageFlags msgFlags;
-
             // An acknowledgment needs to be sent back to the peer for this message on this exchange,
             // Set the flag in message header indicating an ack requested by peer;
             msgFlags.Set(MessageFlagValues::kPeerRequestedAck);

src/messaging/ExchangeMessageDispatch.h

@@ -24,6 +24,7 @@
 #pragma once
 
 #include <lib/core/ReferenceCounted.h>
+#include <messaging/Flags.h>
 #include <transport/SecureSessionMgr.h>
 
 namespace chip {
@@ -59,7 +60,7 @@ class ExchangeMessageDispatch : public ReferenceCounted<ExchangeMessageDispatch>
                                            const EncryptedPacketBufferHandle & preparedMessage) const                       = 0;
 
     virtual CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId,
-                                         const Transport::PeerAddress & peerAddress,
+                                         const Transport::PeerAddress & peerAddress, MessageFlags msgFlags,
                                          ReliableMessageContext * reliableMessageContext);
 
 protected:

src/messaging/ExchangeMgr.cpp

@@ -140,7 +140,7 @@ CHIP_ERROR ExchangeManager::UnregisterUnsolicitedMessageHandlerForType(Protocols
     return UnregisterUMH(protocolId, static_cast<int16_t>(msgType));
 }
 
-void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer)
+void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source)
 {
 #if CHIP_ERROR_LOGGING
     char srcAddressStr[Transport::PeerAddress::kMaxToStringSize];
@@ -197,7 +197,7 @@ CHIP_ERROR ExchangeManager::UnregisterUMH(Protocols::Id protocolId, int16_t msgT
 
 void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader,
                                         SecureSessionHandle session, const Transport::PeerAddress & source,
-                                        System::PacketBufferHandle && msgBuf, SecureSessionMgr * msgLayer)
+                                        DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf)
 {
     CHIP_ERROR err                          = CHIP_NO_ERROR;
     UnsolicitedMessageHandler * matchingUMH = nullptr;
@@ -207,6 +207,12 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
                     payloadHeader.GetMessageType(), payloadHeader.GetProtocolID().ToFullyQualifiedSpecForm(),
                     payloadHeader.GetExchangeID());
 
+    MessageFlags msgFlags;
+    if (isDuplicate == DuplicateMessage::Yes)
+    {
+        msgFlags.Set(MessageFlagValues::kDuplicateMessage);
+    }
+
     // Search for an existing exchange that the message applies to. If a match is found...
     bool found = false;
     mContextPool.ForEachActiveObject([&](auto * ec) {
@@ -220,7 +226,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
             }
 
             // Matched ExchangeContext; send to message handler.
-            ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf));
+            ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf));
             found = true;
             return false;
         }
@@ -232,10 +238,10 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
         ExitNow(err = CHIP_NO_ERROR);
     }
 
-    // Search for an unsolicited message handler if it marked as being sent by an initiator. Since we didn't
-    // find an existing exchange that matches the message, it must be an unsolicited message. However all
+    // If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator.
+    // Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all
     // unsolicited messages must be marked as being from an initiator.
-    if (payloadHeader.IsInitiator())
+    if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator())
     {
         // Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly
         // handle the message type over handlers that handle all messages for a profile.
@@ -288,7 +294,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
 
         ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%p", ec->GetExchangeId(), ec->GetDelegate());
 
-        ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf));
+        ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf));
 
         // Close exchange if it was created only to send ack for a duplicate message.
         if (sendAckAndCloseExchange)
@@ -302,15 +308,15 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
     }
 }
 
-void ExchangeManager::OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr)
+void ExchangeManager::OnNewConnection(SecureSessionHandle session)
 {
     if (mDelegate != nullptr)
     {
         mDelegate->OnNewConnection(session, this);
     }
 }
 
-void ExchangeManager::OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr)
+void ExchangeManager::OnConnectionExpired(SecureSessionHandle session)
 {
     if (mDelegate != nullptr)
     {
@@ -328,23 +334,6 @@ void ExchangeManager::OnConnectionExpired(SecureSessionHandle session, SecureSes
     });
 }
 
-void ExchangeManager::OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf)
-{
-    PacketHeader header;
-
-    ReturnOnFailure(header.DecodeAndConsume(msgBuf));
-
-    Optional<NodeId> peer = header.GetSourceNodeId();
-    if (!peer.HasValue())
-    {
-        char addrBuffer[Transport::PeerAddress::kMaxToStringSize];
-        source.ToString(addrBuffer, sizeof(addrBuffer));
-        ChipLogError(ExchangeManager, "Unencrypted message from %s is dropped since no source node id in packet header.",
-                     addrBuffer);
-        return;
-    }
-}
-
 void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * delegate)
 {
     mContextPool.ForEachActiveObject([&](auto * ec) {

src/messaging/ExchangeMgr.h

@@ -49,7 +49,7 @@ static constexpr int16_t kAnyMessageType = -1;
  *    It works on be behalf of higher layers, creating ExchangeContexts and
  *    handling the registration/unregistration of unsolicited message handlers.
  */
-class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public TransportMgrDelegate
+class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate
 {
     friend class ExchangeContext;
 
@@ -242,21 +242,17 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans
     CHIP_ERROR RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegate * delegate);
     CHIP_ERROR UnregisterUMH(Protocols::Id protocolId, int16_t msgType);
 
-    void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer) override;
+    void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source) override;
 
     void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, SecureSessionHandle session,
-                           const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf,
-                           SecureSessionMgr * msgLayer) override;
+                           const Transport::PeerAddress & source, DuplicateMessage isDuplicate,
+                           System::PacketBufferHandle && msgBuf) override;
 
-    void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) override;
+    void OnNewConnection(SecureSessionHandle session) override;
 #if CHIP_CONFIG_TEST
 public: // Allow OnConnectionExpired to be called directly from tests.
 #endif  // CHIP_CONFIG_TEST
-    void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) override;
-
-    // TransportMgrDelegate interface for rendezvous sessions
-private:
-    void OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf) override;
+    void OnConnectionExpired(SecureSessionHandle session) override;
 };
 
 } // namespace Messaging

src/messaging/tests/TestExchangeMgr.cpp

@@ -141,7 +141,7 @@ void CheckSessionExpirationBasics(nlTestSuite * inSuite, void * inContext)
     ExchangeContext * ec1 = ctx.NewExchangeToLocal(&sendDelegate);
 
     // Expire the session this exchange is supposedly on.
-    ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession(), &ctx.GetSecureSessionManager());
+    ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession());
 
     MockAppDelegate receiveDelegate;
     CHIP_ERROR err =
@@ -171,7 +171,7 @@ void CheckSessionExpirationTimeout(nlTestSuite * inSuite, void * inContext)
 
     // Expire the session this exchange is supposedly on.  This should close the
     // exchange.
-    ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession(), &ctx.GetSecureSessionManager());
+    ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession());
     NL_TEST_ASSERT(inSuite, sendDelegate.IsOnResponseTimeoutCalled);
 }