Use exchange for session establishment and provisioning #5938

pan-apple · 2021-04-10T20:13:15Z

Problem

Session establishment doesn't use exchange or reliable messaging.

Summary of Changes

Add concept of ExchangeTransport to distinguish between encrypted
and unencrypted message exchanges
Update PASE to use exchange manager to send/receive messages
Update network provisioning code to use exchange manager

Fixes #2240, Fixes #2243

todo · 2021-04-10T20:13:16Z

This is needed for time being, as ServiceProvisioning is used for opening pairing window.

connectedhomeip/src/app/server/Server.cpp

Lines 499 to 505 in 33af7ef

    
               // TODO: This is needed for time being, as ServiceProvisioning is used for opening pairing window. 
        
               err = gExchangeMgr.RegisterUnsolicitedMessageHandlerForProtocol(Protocols::ServiceProvisioning::Id, &gCallbacks); 
        
               SuccessOrExit(err); 
        
           #if defined(CHIP_APP_USE_ECHO) 
        
               err = InitEchoHandler(&gExchangeMgr); 
        
               SuccessOrExit(err);

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

todo · 2021-04-10T20:13:19Z

Change this check to only include the protocol and message types that are allowed

connectedhomeip/src/messaging/ApplicationExchangeTransport.cpp

Lines 44 to 54 in 33af7ef

    
           // TODO: Change this check to only include the protocol and message types that are allowed 
        
           switch (protocol) 
        
           { 
        
           case Protocols::SecureChannel::Id.GetProtocolId(): 
        
               switch (static_cast<Protocols::SecureChannel::MsgType>(type)) 
        
               { 
        
               case Protocols::SecureChannel::MsgType::PBKDFParamRequest: 
        
               case Protocols::SecureChannel::MsgType::PBKDFParamResponse: 
        
               case Protocols::SecureChannel::MsgType::PASE_Spake2p1: 
        
               case Protocols::SecureChannel::MsgType::PASE_Spake2p2: 
        
               case Protocols::SecureChannel::MsgType::PASE_Spake2p3:

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

todo · 2021-04-10T20:13:22Z

Figure out which channel to use for the received message

connectedhomeip/src/messaging/ExchangeMgr.cpp

Lines 286 to 289 in 33af7ef

    
               // TODO: Figure out which channel to use for the received message 
        
               ec = AllocContext(payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), nullptr); 
        
           } 
        
           else

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

todo · 2021-04-10T20:13:25Z

Register for specific message types, as CASE and PASE share the same protocol ID

connectedhomeip/src/messaging/MessageCounterSync.cpp

Lines 45 to 49 in 33af7ef

    
               // TODO: Register for specific message types, as CASE and PASE share the same protocol ID 
        
               return mExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::SecureChannel::Id, this); 
        
           } 
        
           void MessageCounterSyncMgr::Shutdown()

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

pan-apple · 2021-04-12T23:39:00Z

rebased

src/messaging/ApplicationExchangeTransport.cpp

src/messaging/ExchangeTransport.cpp

src/protocols/secure_channel/NetworkProvisioning.cpp

src/protocols/secure_channel/NetworkProvisioning.h

src/protocols/secure_channel/SessionEstablishmentTransport.cpp

src/transport/PairingSession.h

yunhanw-google · 2021-04-13T17:25:15Z

@pan-apple the im cirque test is faling after sending several im commands, in case you wanna locally reproduce it, you can use
To start the Server, run the built executable.

$ ./chip-im-responder
To start the Client, run the built executable and pass it the IP address of the server to talk to.

$ ./chip-im-initiator <Server's IPv4 address>

src/app/server/Server.cpp

src/messaging/ExchangeContext.h

src/messaging/ExchangeDelegate.h

src/messaging/ExchangeMgr.cpp

src/messaging/ExchangeTransport.h

yufengwangca · 2021-04-13T18:15:25Z

src/messaging/MessageCounterSync.cpp

+    // TODO: Register for specific message types, as CASE and PASE share the same protocol ID
+    return mExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::SecureChannel::Id, this);


We should use RegisterUnsolicitedMessageHandlerForType here now to register only MCSP messages, otherwise, all PASE/CASE messages will be received by MessageCounterSync class

The existing code is registering for the protocol type. I tried changing to only MCSP messages, but some tests are failing. That's why I added TODO so this can be tracked in an issue and fixed in a separate PR.

I think that indicates a problem, by design, each protocol should only have one protocol delegate, since the message is moved not copied.

Now, there are two entities both register to receive SecureChannel message, it is really depending on registration order, the first one will be overridden by the second one.

Registering by different message type is also a workaround, but it should works, ideally, we should use SecureChannel protocol delegate to dispatch the messages, but we can leave it later as TODO

Lets have an issue ticket to follow up here if the PASE messages are consumed by MCSP first, then PASE protocol should not receive the message.

Yes, TODO bot will file the ticket once this PR is merged.

yufengwangca · 2021-04-13T18:24:08Z

I noticed there are more than 300 bytes increase on BSS after this refactor, I guess this is mainly caused by moving Transport to ExchangeContext, it should go back to normal if we keep the transport handling all within ExchangeMgr.

src/messaging/ExchangeContext.cpp

src/protocols/secure_channel/SessionEstablishmentTransport.h

src/messaging/ExchangeTransport.h

src/messaging/ExchangeTransport.cpp

src/messaging/ExchangeContext.cpp

yufengwangca · 2021-04-14T04:22:23Z

src/messaging/ExchangeTransport.cpp

+#endif
+    }
+
+    if (!IsTransportReliable() && reliableMessageContext.AutoRequestAck() && mReliableMessageMgr != nullptr &&


It looks isReliableTransmission is just translated from it is requested, we should also check the low level Transport type. CRMP is not needed for TCP

The expectation is that IsTransportReliable() method for TCP based exchange transport (e.g. SessionEstablishmentExchangeTransportTCP) will return True.

ApplicationExchangeDispatch should support both TCP and UDP right? if I run chip-shell ping -t , the message should also go through ApplicationExchangeDispatch, right?

ApplicationExchangeDispatch will be used by IM messages. Those should all be UDP only.

Lets have a ticket to follow up here, since all of our examples still support TCP option now

src/protocols/secure_channel/SessionEstablishmentTransport.h

src/messaging/ExchangeMgr.h

src/messaging/ExchangeContext.h

github-actions · 2021-04-14T23:38:24Z

Size increase report for "nrfconnect-example-build" from 6130e4a

File	Section	File	VM
chip-lighting.elf	text	1016	1016
chip-lighting.elf	rodata	584	584
chip-lighting.elf	bss	0	137
chip-lighting.elf	[LOAD #3 [RW]]	0	23
chip-lighting.elf	devices	4	4
chip-lighting.elf	device_handles	-8	-8
chip-lighting.elf	datas	-12	-12
chip-shell.elf	text	1316	1316
chip-shell.elf	rodata	784	788
chip-shell.elf	[LOAD #3 [RW]]	0	23
chip-shell.elf	bss	0	9
chip-shell.elf	device_handles	-4	-4
chip-lock.elf	text	1016	1016
chip-lock.elf	rodata	584	584
chip-lock.elf	bss	0	129
chip-lock.elf	[LOAD #3 [RW]]	0	31
chip-lock.elf	devices	-4	-4
chip-lock.elf	device_handles	-8	-8
chip-lock.elf	datas	-12	-12

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,179495
.debug_line,0,15953
.debug_abbrev,0,12056
.debug_str,0,5217
.strtab,0,3281
.symtab,0,1456
text,1016,1016
.debug_frame,0,996
.debug_loc,0,699
rodata,584,584
.debug_ranges,0,512
.debug_aranges,0,344
bss,137,0
[LOAD #3 [RW]],23,0
devices,4,4
.shstrtab,0,3
device_handles,-8,-8
datas,-12,-12

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.debug_info,0,129412
.debug_line,0,14575
.debug_abbrev,0,11101
.debug_str,0,4857
.debug_loc,0,4091
.strtab,0,3035
.symtab,0,1424
text,1316,1316
.debug_frame,0,1068
.debug_ranges,0,1024
rodata,788,784
.debug_aranges,0,376
[LOAD #3 [RW]],23,0
bss,9,0
.shstrtab,0,1
device_handles,-4,-4

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,179459
.debug_line,0,15937
.debug_abbrev,0,12056
.debug_str,0,5206
.strtab,0,3281
.symtab,0,1456
text,1016,1016
.debug_frame,0,996
.debug_loc,0,698
rodata,584,584
.debug_ranges,0,512
.debug_aranges,0,344
bss,129,0
[LOAD #3 [RW]],31,0
.shstrtab,0,-1
devices,-4,-4
device_handles,-8,-8
datas,-12,-12

…ect-chip#5938)" This reverts commit 362c5f2.

boring-cyborg bot added app controller examples labels Apr 10, 2021

pan-apple mentioned this pull request Apr 10, 2021

Use exchange for session establishment and provisioning #5674

Closed

pan-apple force-pushed the pase-over-exchange branch from aa4c502 to 142a310 Compare April 10, 2021 23:00

pan-apple marked this pull request as ready for review April 12, 2021 19:11

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, saurabhst, vivien-apple, wehale, woody-apple and yufengwangca April 12, 2021 19:11

jmartinez-silabs approved these changes Apr 12, 2021

View reviewed changes

woody-apple approved these changes Apr 12, 2021

View reviewed changes

pan-apple force-pushed the pase-over-exchange branch 2 times, most recently from 2972729 to b84ebb7 Compare April 12, 2021 23:38

pan-apple force-pushed the pase-over-exchange branch from 616802b to 4426904 Compare April 13, 2021 16:02

mspang reviewed Apr 13, 2021

View reviewed changes

yufengwangca requested changes Apr 13, 2021

View reviewed changes

yufengwangca requested changes Apr 14, 2021

View reviewed changes

pan-apple added 17 commits April 14, 2021 16:07

fix build errors

94c969e

update data model gni file

8b38ec9

rebase fixes

0eb8217

release exchange transport on free

ae2fbb8

fix failing test

0ebc8d9

fix cirque failure

50e7b08

cleanup

17a9952

use oneshot exchange for network provisioning

6b73bf9

fix softap based pairing

833c9b8

address some review comments

2f64a19

fix build issue

2d7dc03

reduce exchange context memory requirement

6764bc4

rename ExchangeTransport to ExchangeMessageDispatch

9c646b8

use payload header where possible

95ba2b5

reduce RAM usage

9708009

further memory optimizations

a47e5cf

remove unused code

dab0b68

pan-apple force-pushed the pase-over-exchange branch from 23288cd to dab0b68 Compare April 14, 2021 23:08

yufengwangca approved these changes Apr 14, 2021

View reviewed changes

yufengwangca merged commit 362c5f2 into project-chip:master Apr 14, 2021

pan-apple deleted the pase-over-exchange branch April 14, 2021 23:38

pan-apple mentioned this pull request Apr 14, 2021

Set exchange delegate for app message send #6027

Merged

yufengwangca mentioned this pull request Apr 15, 2021

Register MCSP messages by type instead of protocol #6031

Merged

yunhanw-google added a commit to yunhanw-google/connectedhomeip that referenced this pull request Apr 15, 2021

Revert "Use exchange for session establishment and provisioning (proj…

efc2312

…ect-chip#5938)" This reverts commit 362c5f2.

This was referenced Apr 15, 2021

crash in exchange context when sending multiple messages #6032

Closed

revert_PR5938 which crash all cirque integration test #6036

Closed

yunhanw-google added a commit to yunhanw-google/connectedhomeip that referenced this pull request Apr 15, 2021

Revert "Use exchange for session establishment and provisioning (proj…

14afee0

…ect-chip#5938)" This reverts commit 362c5f2.

erjiaqing added a commit to erjiaqing/connectedhomeip that referenced this pull request Apr 15, 2021

Revert "Use exchange for session establishment and provisioning (proj…

9170ff2

…ect-chip#5938)" This reverts commit 362c5f2.

Damian-Nordic mentioned this pull request Apr 16, 2021

Broken rendezvous between the Python controller and devices #6092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use exchange for session establishment and provisioning #5938

Use exchange for session establishment and provisioning #5938

pan-apple commented Apr 10, 2021

todo bot commented Apr 10, 2021

todo bot commented Apr 10, 2021

todo bot commented Apr 10, 2021

todo bot commented Apr 10, 2021

pan-apple commented Apr 12, 2021

yunhanw-google commented Apr 13, 2021

yufengwangca Apr 13, 2021

pan-apple Apr 14, 2021

yufengwangca Apr 14, 2021

yufengwangca Apr 14, 2021

pan-apple Apr 14, 2021

yufengwangca commented Apr 13, 2021

yufengwangca Apr 14, 2021

pan-apple Apr 14, 2021

yufengwangca Apr 14, 2021

pan-apple Apr 14, 2021

yufengwangca Apr 14, 2021

pan-apple Apr 14, 2021

github-actions bot commented Apr 14, 2021

		// TODO: Register for specific message types, as CASE and PASE share the same protocol ID
		return mExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::SecureChannel::Id, this);

Use exchange for session establishment and provisioning #5938

Use exchange for session establishment and provisioning #5938

Conversation

pan-apple commented Apr 10, 2021

Problem

Summary of Changes

todo bot commented Apr 10, 2021

This is needed for time being, as ServiceProvisioning is used for opening pairing window.

This comment was generated by todo based on a TODO comment in 33af7ef in #5938. cc @pan-apple.

todo bot commented Apr 10, 2021

Change this check to only include the protocol and message types that are allowed

This comment was generated by todo based on a TODO comment in 33af7ef in #5938. cc @pan-apple.

todo bot commented Apr 10, 2021

Figure out which channel to use for the received message

This comment was generated by todo based on a TODO comment in 33af7ef in #5938. cc @pan-apple.

todo bot commented Apr 10, 2021

Register for specific message types, as CASE and PASE share the same protocol ID

This comment was generated by todo based on a TODO comment in 33af7ef in #5938. cc @pan-apple.

pan-apple commented Apr 12, 2021

yunhanw-google commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yufengwangca commented Apr 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2021

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `33af7ef` in #5938. cc @pan-apple.