Skip to content

Commit 5448491

Browse files
jepenven-silabspull[bot]
jepenven-silabs
authored and
pull[bot]
committed
Apply comments for PR 10985 (#11189)
* Apply comments for PR 10985
1 parent c9d8780 commit 5448491

File tree

4 files changed

+48
-63
lines changed

4 files changed

+48
-63
lines changed

‎src/transport/SessionManager.cpp

+28-56
Original file line numberDiff line numberDiff line change
@@ -401,49 +401,17 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea
401401
VerifyOrExit(CHIP_NO_ERROR == SecureMessageCodec::Decrypt(session, payloadHeader, packetHeader, msg),
402402
ChipLogError(Inet, "Secure transport received message, but failed to decode/authenticate it, discarding"));
403403

404-
// Verify message counter
405-
if (packetHeader.IsSecureSessionControlMsg())
404+
err = session->GetSessionMessageCounter().GetPeerMessageCounter().Verify(packetHeader.GetMessageCounter());
405+
if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED)
406406
{
407-
// TODO: control message counter is not implemented yet
407+
isDuplicate = SessionManagerDelegate::DuplicateMessage::Yes;
408+
err = CHIP_NO_ERROR;
408409
}
409-
else
410+
if (err != CHIP_NO_ERROR)
410411
{
411-
if (!session->GetSessionMessageCounter().GetPeerMessageCounter().IsSynchronized())
412-
{
413-
// Queue and start message sync procedure
414-
err = mMessageCounterManager->QueueReceivedMessageAndStartSync(
415-
packetHeader,
416-
SessionHandle(session->GetPeerNodeId(), session->GetLocalSessionId(), session->GetPeerSessionId(),
417-
session->GetFabricIndex()),
418-
session, peerAddress, std::move(msg));
419-
420-
if (err != CHIP_NO_ERROR)
421-
{
422-
ChipLogError(Inet,
423-
"Message counter synchronization for received message, failed to "
424-
"QueueReceivedMessageAndStartSync, err = %" CHIP_ERROR_FORMAT,
425-
err.Format());
426-
}
427-
else
428-
{
429-
ChipLogDetail(Inet, "Received message have been queued due to peer counter is not synced");
430-
}
431-
432-
return;
433-
}
434-
435-
err = session->GetSessionMessageCounter().GetPeerMessageCounter().Verify(packetHeader.GetMessageCounter());
436-
if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED)
437-
{
438-
isDuplicate = SessionManagerDelegate::DuplicateMessage::Yes;
439-
err = CHIP_NO_ERROR;
440-
}
441-
if (err != CHIP_NO_ERROR)
442-
{
443-
ChipLogError(Inet, "Message counter verify failed, err = %" CHIP_ERROR_FORMAT, err.Format());
444-
}
445-
SuccessOrExit(err);
412+
ChipLogError(Inet, "Message counter verify failed, err = %" CHIP_ERROR_FORMAT, err.Format());
446413
}
414+
SuccessOrExit(err);
447415

448416
mSecureSessions.MarkSessionActive(session);
449417

@@ -461,14 +429,7 @@ void SessionManager::SecureUnicastMessageDispatch(const PacketHeader & packetHea
461429
}
462430
}
463431

464-
if (packetHeader.IsSecureSessionControlMsg())
465-
{
466-
// TODO: control message counter is not implemented yet
467-
}
468-
else
469-
{
470-
session->GetSessionMessageCounter().GetPeerMessageCounter().Commit(packetHeader.GetMessageCounter());
471-
}
432+
session->GetSessionMessageCounter().GetPeerMessageCounter().Commit(packetHeader.GetMessageCounter());
472433

473434
// TODO: once mDNS address resolution is available reconsider if this is required
474435
// This updates the peer address once a packet is received from a new address
@@ -502,6 +463,24 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
502463

503464
VerifyOrExit(!msg.IsNull(), ChipLogError(Inet, "Secure transport received NULL packet, discarding"));
504465

466+
// MCSP check
467+
if (packetHeader.IsSecureSessionControlMsg())
468+
{
469+
if (packetHeader.GetDestinationNodeId().HasValue() && packetHeader.HasPrivacyFlag())
470+
{
471+
// TODO
472+
// if (packetHeader.GetDestinationNodeId().Value() == ThisDeviceNodeID)
473+
// {
474+
// MCSP processing..
475+
// }
476+
}
477+
else
478+
{
479+
ChipLogError(Inet, "Invalid condition found in packet header");
480+
ExitNow(err = CHIP_ERROR_INCORRECT_STATE);
481+
}
482+
}
483+
505484
// TODO: Handle Group message counter here spec 4.7.3
506485
// spec 4.5.1.2 for msg counter
507486

@@ -523,21 +502,14 @@ void SessionManager::SecureGroupMessageDispatch(const PacketHeader & packetHeade
523502
}
524503
}
525504

526-
if (packetHeader.IsSecureSessionControlMsg())
527-
{
528-
// TODO: control message counter is not implemented yet
529-
}
530-
else
531-
{
532-
// TODO: Commit Group Message Counter
533-
}
505+
// TODO: Commit Group Message Counter
534506

535507
if (mCB != nullptr)
536508
{
537509
// TODO: Update Session Handle for Group messages.
538510
// SessionHandle session(session->GetPeerNodeId(), session->GetLocalSessionId(), session->GetPeerSessionId(),
539511
// session->GetFabricIndex());
540-
// mCB->OnMessageReceived(packetHeader, payloadHeader, nullptr, peerAddress, isDuplicate, std::move(msg));
512+
// mCB->OnMessageReceived(packetHeader, payloadHeader, session, peerAddress, isDuplicate, std::move(msg));
541513
}
542514

543515
exit:

‎src/transport/raw/MessageHeader.cpp

+4-7
Original file line numberDiff line numberDiff line change
@@ -177,10 +177,9 @@ CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint1
177177
}
178178
else if (mMsgFlags.Has(Header::MsgFlagValues::kDestinationNodeIdPresent))
179179
{
180-
if (mSessionType != Header::SessionType::kUnicastSession)
181-
{
182-
SuccessOrExit(err = CHIP_ERROR_INTERNAL);
183-
}
180+
// No need to check if session is Unicast because for MCSP
181+
// a destination node ID is present with a group session ID.
182+
// Spec 4.9.2.4
184183
uint64_t destinationNodeId;
185184
SuccessOrExit(err = reader.Read64(&destinationNodeId).StatusCode());
186185
mDestinationNodeId.SetValue(destinationNodeId);
@@ -282,7 +281,6 @@ CHIP_ERROR PacketHeader::Encode(uint8_t * data, uint16_t size, uint16_t * encode
282281
VerifyOrReturnError(!(mDestinationNodeId.HasValue() && mDestinationGroupId.HasValue()), CHIP_ERROR_INTERNAL);
283282
VerifyOrReturnError(encode_size != nullptr, CHIP_ERROR_INTERNAL);
284283
VerifyOrReturnError(IsSessionTypeValid(), CHIP_ERROR_INTERNAL);
285-
VerifyOrReturnError(!(IsGroupSession() && !mDestinationGroupId.HasValue()), CHIP_ERROR_INTERNAL);
286284

287285
Header::MsgFlags messageFlags = mMsgFlags;
288286
messageFlags.Set(Header::MsgFlagValues::kSourceNodeIdPresent, mSourceNodeId.HasValue())
@@ -306,8 +304,7 @@ CHIP_ERROR PacketHeader::Encode(uint8_t * data, uint16_t size, uint16_t * encode
306304
{
307305
LittleEndian::Write64(p, mDestinationNodeId.Value());
308306
}
309-
310-
if (mDestinationGroupId.HasValue())
307+
else if (mDestinationGroupId.HasValue())
311308
{
312309
LittleEndian::Write16(p, mDestinationGroupId.Value());
313310
}

‎src/transport/raw/MessageHeader.h

+5
Original file line numberDiff line numberDiff line change
@@ -167,6 +167,11 @@ class PacketHeader
167167

168168
uint8_t GetSecurityFlags() const { return mSecFlags.Raw(); }
169169

170+
bool HasPrivacyFlag() const { return mSecFlags.Has(Header::SecFlagValues::kPrivacyFlag); }
171+
172+
void SetFlags(Header::SecFlagValues value) { mSecFlags.Set(value); }
173+
void SetFlags(Header::MsgFlagValues value) { mMsgFlags.Set(value); }
174+
170175
void SetMessageFlags(uint8_t flags) { mMsgFlags.SetRaw(flags); }
171176

172177
void SetSecurityFlags(uint8_t securityFlags)

‎src/transport/raw/tests/TestMessageHeader.cpp

+11
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,17 @@ void TestPacketHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)
157157
NL_TEST_ASSERT(inSuite, header.GetMessageCounter() == 234);
158158
NL_TEST_ASSERT(inSuite, header.GetDestinationGroupId() == Optional<uint16_t>::Value((uint16_t) 45));
159159
NL_TEST_ASSERT(inSuite, header.GetSourceNodeId() == Optional<uint64_t>::Value(77ull));
160+
161+
// Verify MCSP state
162+
header.ClearDestinationGroupId().SetDestinationNodeId(42).SetFlags(Header::SecFlagValues::kPrivacyFlag);
163+
NL_TEST_ASSERT(inSuite, header.Encode(buffer, &encodeLen) == CHIP_NO_ERROR);
164+
165+
// change it to verify decoding
166+
header.SetMessageCounter(222).SetSourceNodeId(1).SetDestinationGroupId(2);
167+
NL_TEST_ASSERT(inSuite, header.Decode(buffer, &decodeLen) == CHIP_NO_ERROR);
168+
NL_TEST_ASSERT(inSuite, header.GetDestinationNodeId() == Optional<uint64_t>::Value(42ull));
169+
NL_TEST_ASSERT(inSuite, !header.GetDestinationGroupId().HasValue());
170+
NL_TEST_ASSERT(inSuite, header.HasPrivacyFlag());
160171
}
161172

162173
void TestPayloadHeaderEncodeDecode(nlTestSuite * inSuite, void * inContext)

0 commit comments

Comments
 (0)