Enable CASE session establishment #7666

pan-apple · 2021-06-15T21:40:53Z

Problem

We are currently using secure sessions established via PASE for non commissioning cluster messages. This needs to be changed to CASE sessions.

Change overview

Update CASESession code to use array of certs as operational certificates.
Trigger CASE session establishment after the device has been discovered on operational network.
Move session key ID assignment to a common class (as session ID space is shared by PASE and CASE sessions).
Expire the secure session established via PASE while triggering the CASE sessions.
Use secure sessions established via CASE for further messaging.

Testing

Tested flow using Python controller, chip-tool, and iOS CHIP Tool applications.

todo · 2021-06-15T21:40:55Z

- Match the generated cert against CSR and operational keypair

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 307 to 317 in 3b7cd86

    
                   // TODO - Match the generated cert against CSR and operational keypair 
        
                   //        Make sure it chains back to the trusted root. 
        
                   ChipLogProgress(Controller, "Generating operational certificate for the controller"); 
        
                   chipCertLen = 0; 
        
                   ReturnErrorOnFailure(GenerateOperationalCertificates(ByteSpan(CSR.Get(), csrLength), mLocalDeviceId, chipCert.Get(), 
        
                                                                        chipCertAllocatedLen, chipCertLen)); 
        
                   ReturnErrorOnFailure(admin->SetOperationalCert(ByteSpan(chipCert.Get(), chipCertLen))); 
        
               } 
        
               ReturnErrorOnFailure(mAdmins.Store(admin->GetAdminId())); 
        
           }

This comment was generated by todo based on a `TODO` comment in `3b7cd86` in #7666. cc @pan-apple.

todo · 2021-06-15T21:40:58Z

- Update SessionID allocator to use freed session IDs

connectedhomeip/src/protocols/secure_channel/SessionIDAllocator.cpp

Lines 29 to 39 in 3b7cd86

    
               // TODO - Update SessionID allocator to use freed session IDs 
        
               mNextAvailable++; 
        
               return CHIP_NO_ERROR; 
        
           } 
        
           CHIP_ERROR SessionIDAllocator::Free(uint16_t id) 
        
           { 
        
               if (mNextAvailable == id && mNextAvailable > 0) 
        
               { 
        
                   mNextAvailable--;

This comment was generated by todo based on a `TODO` comment in `3b7cd86` in #7666. cc @pan-apple.

woody-apple · 2021-06-16T04:01:31Z

/rebase

andy31415 · 2021-06-16T18:29:25Z

examples/chip-tool/commands/clusters/ModelCommand.cpp

+
+    for (uint32_t i = 0; i < kIterationCount && device->IsSessionSetupInProgress(); i++)
+    {
+        nanosleep(&sleep_time, nullptr);


could we instead have some sort of notification from the device of 'session done'?

May not care much for test tools, however I imagine most other commisioners want to avoid sleeps.

Added a TODO for this. This is big enough change to handle in a separate PR.

There is missing synchronization here. We can show this with the following argument:

Calling IsSessionSetupInProgress loads shared controller & protocol state

The loop performs no stores that could change the state

The loop synchronizes with no other thread that could change the state

Assuming those propositions, one of the following is true:

Every iteration loads the same state (i.e., IsSessionSetupInProgress remains true)

This code relies on a data race

And since data races are specified as undefined behavior in C++, all bets are off as to whether the resulting program does anything useful.

One correct implementation of waiting on a 2nd thread to do something is to synchronize access to the state with a mutex and use a condition variable for the "wait". Not to sleep; sleeping is not a useful primitive for waiting on a 2nd thread because it does not avoid the race that arises.

Fixing errors like this is not something that should be left as a TODO.

examples/chip-tool/commands/clusters/ModelCommand.cpp

src/app/server/RendezvousServer.cpp

src/app/server/RendezvousServer.h

src/controller/CHIPDevice.cpp

src/controller/CHIPDeviceController.cpp

src/protocols/secure_channel/SessionIDAllocator.h

src/protocols/secure_channel/SessionIDAllocator.cpp

src/protocols/secure_channel/CASEServer.cpp

src/app/server/RendezvousServer.cpp

src/controller/CHIPDevice.cpp

todo · 2021-06-16T20:58:52Z

- Implement notification from device object when the secure session is available.

connectedhomeip/examples/chip-tool/commands/clusters/ModelCommand.cpp

Lines 76 to 85 in 95aba4d

    
               // TODO - Implement notification from device object when the secure session is available. 
        
               // Current code is polling the device object to check for secure session availability. This should 
        
               // be updated to a notification/callback mechanism. 
        
               err = WaitForSessionSetup(mDevice); 
        
               VerifyOrExit(err == CHIP_NO_ERROR, 
        
                            ChipLogError(chipTool, "Timed out while waiting for session setup for device: %" PRIu64, localId)); 
        
               err = SendCommand(mDevice, mEndPointId); 
        
               VerifyOrExit(err == CHIP_NO_ERROR, ChipLogError(chipTool, "Failed to send message: %s", ErrorStr(err))); 
        
           }

This comment was generated by todo based on a `TODO` comment in `95aba4d` in #7666. cc @pan-apple.

todo · 2021-06-16T20:58:55Z

- Check if ID is already allocated in SessionIDAllocator::Reserve()

connectedhomeip/src/protocols/secure_channel/SessionIDAllocator.cpp

Lines 52 to 62 in 95aba4d

    
               // TODO - Check if ID is already allocated in SessionIDAllocator::Reserve() 
        
               return CHIP_NO_ERROR; 
        
           } 
        
           uint16_t SessionIDAllocator::Peek() 
        
           { 
        
               return mNextAvailable; 
        
           } 
        
           } // namespace chip

This comment was generated by todo based on a `TODO` comment in `95aba4d` in #7666. cc @pan-apple.

woody-apple · 2021-06-16T22:26:46Z

/rebase

github-actions · 2021-06-22T16:49:27Z

Size increase report for "gn_qpg6100-example-build" from 59e4a16

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	532	532
chip-qpg6100-lighting-example.out	.bss	0	8
chip-qpg6100-lighting-example.out	.heap	0	-8

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,24911
.debug_line,0,2798
.debug_loc,0,1334
.debug_abbrev,0,987
.debug_str,0,954
.text,532,532
.debug_ranges,0,440
.strtab,0,396
.symtab,0,288
.debug_frame,0,280
.debug_aranges,0,96
.bss,8,0
.heap,-8,0
[Unmapped],0,-532

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

github-actions · 2021-06-22T17:14:03Z

Size increase report for "esp32-example-build" from 59e4a16

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	560	560
chip-all-clusters-app.elf	.flash.rodata	64	64
chip-all-clusters-app.elf	.dram0.bss	0	16

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,29219
.debug_line,0,4083
.debug_abbrev,0,1263
.debug_str,0,953
.debug_loc,0,742
.flash.text,560,560
.shstrtab,0,462
.debug_ranges,0,352
.strtab,0,330
.debug_frame,0,232
.symtab,0,176
.xt.prop._ZN4chip9Transport15PeerConnectionsILj16ELNS_4Time6SourceE0EE23FindPeerConnectionStateEyPNS0_19PeerConnectionStateE,0,148
.debug_aranges,0,88
.xt.prop._ZN4chip9Transport19PeerConnectionStateC2EONS0_11PeerAddressE,0,76
.flash.rodata,64,64
.xt.lit._ZN4chip9Transport15PeerConnectionsILj16ELNS_4Time6SourceE0EE23FindPeerConnectionStateEyPNS0_19PeerConnectionStateE,0,48
.xt.lit._ZN4chip16SecureSessionMgr16IsControlMessageERNS_13PayloadHeaderE,0,40
.xt.prop._ZN4chip16SecureSessionMgr16IsControlMessageERNS_13PayloadHeaderE,0,40
.dram0.bss,16,0
[Unmapped],0,-64

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

github-actions · 2021-06-22T17:52:50Z

Size increase report for "nrfconnect-example-build" from 59e4a16

File	Section	File	VM
chip-lock.elf	text	456	456
chip-lock.elf	rodata	64	60
chip-lock.elf	[LOAD #3 [RW]]	0	30
chip-lock.elf	device_handles	8	8
chip-lock.elf	bss	0	2
chip-shell.elf	text	4	4
chip-shell.elf	device_handles	-4	-4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,25267
.debug_line,0,2566
.debug_loc,0,1257
.debug_abbrev,0,1011
.debug_str,0,915
text,456,456
.strtab,0,396
.debug_ranges,0,360
.debug_frame,0,280
.symtab,0,240
.debug_aranges,0,96
rodata,60,64
[LOAD #3 [RW]],30,0
device_handles,8,8
bss,2,0

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

sections,vmsize,filesize
.debug_info,0,1930
.debug_loc,0,968
.debug_str,0,456
.debug_line,0,408
.debug_ranges,0,296
.debug_frame,0,140
.strtab,0,124
.symtab,0,64
.debug_aranges,0,32
.debug_abbrev,0,18
text,4,4
device_handles,-4,-4

pan-apple · 2021-06-22T22:55:52Z

@mspang do you have any further comments?

mspang

@mspang do you have any further comments?

I do but nothing major, they could be fixed in a followup.

mspang · 2021-06-22T22:34:14Z

examples/chip-tool/commands/clusters/ModelCommand.cpp

+{
+    ModelCommand * command = reinterpret_cast<ModelCommand *>(context);
+    VerifyOrReturn(command != nullptr,
+                   ChipLogError(chipTool, "Device connected, but cannot send the command, as the context is null"));


Can this happen (how)?

If not suggest VerifyOrDie(command != nullptr) or just leave it off since it's obvious anyway due to the call below.

mspang · 2021-06-22T23:10:37Z

src/controller/CHIPDevice.cpp

+
+CHIP_ERROR Device::EstablishConnectivity(Callback::Callback<OnDeviceConnected> * onConnection,
+                                         Callback::Callback<OnDeviceConnectionFailure> * onFailure)
+{


Can we:

VerifyOrDie(!onConnection->IsRegistered()); VerifyOrDie(!onFailure->IsRegistered());

and document that the callback must not be registered?

(taking it as a Cancelable accomplishes similar but currently trades away the type safety which doesn't seem like a good tradeoff)

mspang · 2021-06-22T23:12:40Z

examples/chip-tool/commands/clusters/ModelCommand.cpp

+{
+    ModelCommand * command = reinterpret_cast<ModelCommand *>(context);
+    ChipLogError(chipTool, "Failed in connecting to the device %" PRIu64 ". Error %d", deviceId, error);
+    VerifyOrReturn(command != nullptr, ChipLogError(chipTool, "ModelCommand context is null"));


VerifyOrDie(command != nullptr) here too.

mspang · 2021-06-22T23:15:08Z

src/controller/CHIPDevice.cpp

@@ -475,35 +492,118 @@ bool Device::GetAddress(Inet::IPAddress & addr, uint16_t & port) const
    return true;
 }

-CHIP_ERROR Device::EstablishCASESession()
+void Device::OperationalCertProvisioned()


nit: Suggest OnOperationalCertProvisioned as I think this reads a bit easier and we use that convention elsewhere.

mspang · 2021-06-22T23:17:00Z

src/controller/CHIPDeviceController.h

+     *   The function updates the state of device proxy object such that all subsequent messages
+     *   will use secure session established via CASE handshake.
+     */
+    CHIP_ERROR OperationalDiscoveryComplete(NodeId remoteDeviceId);


nit: Suggest OnOperationalDiscoveryComplete

mspang · 2021-06-22T23:48:09Z

src/controller/CHIPDeviceController.cpp

+void DeviceCommissioner::OnDeviceConnectedFn(void * context, Device * device)
+{
+    DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
+    VerifyOrReturn(commissioner != nullptr, ChipLogProgress(Controller, "Device connected callback with null context. Ignoring"));


How can this happen? If it can't please use VerifyOrDie.

mspang · 2021-06-22T23:48:18Z

src/controller/CHIPDeviceController.cpp

+{
+    DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
+    ChipLogProgress(Controller, "Device connection failed. Error %s", ErrorStr(error));
+    VerifyOrReturn(commissioner != nullptr,


How can this happen? If it can't please use VerifyOrDie.

mspang · 2021-06-22T23:50:23Z

src/controller/CHIPDeviceController.h

+    /**
+     *   This function finds the device corresponding to deviceId, and establishes a secure connection with it.
+     *   Once the connection is successfully establishes (or if it's already connected), it calls `onConnectedDevice`
+     *   callback. If it fails to establish the connection, it calls `onError` callback.


.. These callbacks must not be already registered with a source (i.e., if they were used previously, they must be canceled).

mspang · 2021-06-22T23:51:02Z

src/controller/CHIPDeviceController.cpp

+                                                Callback::Callback<OnDeviceConnectionFailure> * onFailure)
+{
+    CHIP_ERROR err  = CHIP_NO_ERROR;
+    Device * device = nullptr;


VerifyOrDie(!onConnection->IsRegistered()); VerifyOrDie(!onFailure->IsRegistered());

mspang · 2021-06-22T23:56:26Z

src/protocols/secure_channel/SessionIDAllocator.h

+namespace chip {
+
+class SessionIDAllocator
+{


Please document.

pan-apple · 2021-06-23T14:56:28Z

@mspang do you have any further comments?

I do but nothing major, they could be fixed in a followup.

Thanks @mspang, I'll address these in the follow up PR.

* Enable CASE session establishment * persist counter if device is connected * enable CASE sessions for chip-tool * address review comments * add tests for SessionIDAllocator * address review comments * address review comments * add ReserveUpTo * release resouces on cleanup, and reduce message timeout * release resources on cleanup and init. * Remove sleep from ModelCommand * some cleanup

boring-cyborg bot added app controller transport labels Jun 15, 2021

woody-apple force-pushed the enable-case-session branch from 3521803 to 7a459ab Compare June 16, 2021 06:04

pan-apple force-pushed the enable-case-session branch from 7a459ab to dca2797 Compare June 16, 2021 16:11

boring-cyborg bot added the examples label Jun 16, 2021

pan-apple marked this pull request as ready for review June 16, 2021 16:13

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, kedars, LuDuda, mspang, sagar-apple, saurabhst and woody-apple June 16, 2021 16:14

pullapprove bot added the review - pending label Jun 16, 2021

andy31415 reviewed Jun 16, 2021

View reviewed changes

pan-apple requested a review from andy31415 June 16, 2021 21:24

woody-apple force-pushed the enable-case-session branch from 95aba4d to 74b9891 Compare June 16, 2021 22:59

woody-apple force-pushed the master branch from bcaa85b to a2e125b Compare June 16, 2021 23:36

pan-apple force-pushed the enable-case-session branch from 74b9891 to 9c5c6f3 Compare June 17, 2021 00:28

woody-apple approved these changes Jun 17, 2021

View reviewed changes

pan-apple added 10 commits June 22, 2021 08:26

persist counter if device is connected

31ad603

enable CASE sessions for chip-tool

7e4b792

address review comments

8fb02e0

add tests for SessionIDAllocator

481ea8f

address review comments

c15904e

address review comments

c1f513a

add ReserveUpTo

797d24d

release resouces on cleanup, and reduce message timeout

13de107

release resources on cleanup and init.

bf54cb4

Remove sleep from ModelCommand

5373ba5

pan-apple force-pushed the enable-case-session branch from 91a81b7 to 5373ba5 Compare June 22, 2021 16:21

some cleanup

6c1f43a

pan-apple requested a review from mspang June 22, 2021 17:01

mspang approved these changes Jun 22, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - changed requested labels Jun 22, 2021

woody-apple merged commit da45346 into project-chip:master Jun 23, 2021

pan-apple deleted the enable-case-session branch June 23, 2021 14:56

mspang mentioned this pull request Jun 24, 2021

chip-tool tests TestCluster is broken after "Enable CASE session establishment (#7666)" #7860

Closed

bzbarsky-apple mentioned this pull request Jun 28, 2021

Crash doing onnetwork connection with chip-device-ctrl #7982

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable CASE session establishment #7666

Enable CASE session establishment #7666

pan-apple commented Jun 15, 2021 •

edited

Loading

todo bot commented Jun 15, 2021

todo bot commented Jun 15, 2021

woody-apple commented Jun 16, 2021

andy31415 Jun 16, 2021

pan-apple Jun 16, 2021

mspang Jun 18, 2021 •

edited

Loading

todo bot commented Jun 16, 2021

todo bot commented Jun 16, 2021

woody-apple commented Jun 16, 2021

github-actions bot commented Jun 22, 2021

github-actions bot commented Jun 22, 2021

github-actions bot commented Jun 22, 2021

pan-apple commented Jun 22, 2021

mspang left a comment

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

mspang Jun 22, 2021

pan-apple commented Jun 23, 2021

Enable CASE session establishment #7666

Enable CASE session establishment #7666

Conversation

pan-apple commented Jun 15, 2021 • edited Loading

Problem

Change overview

Testing

todo bot commented Jun 15, 2021

- Match the generated cert against CSR and operational keypair

This comment was generated by todo based on a TODO comment in 3b7cd86 in #7666. cc @pan-apple.

todo bot commented Jun 15, 2021

- Update SessionID allocator to use freed session IDs

This comment was generated by todo based on a TODO comment in 3b7cd86 in #7666. cc @pan-apple.

woody-apple commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mspang Jun 18, 2021 • edited Loading

Choose a reason for hiding this comment

todo bot commented Jun 16, 2021

- Implement notification from device object when the secure session is available.

This comment was generated by todo based on a TODO comment in 95aba4d in #7666. cc @pan-apple.

todo bot commented Jun 16, 2021

- Check if ID is already allocated in SessionIDAllocator::Reserve()

This comment was generated by todo based on a TODO comment in 95aba4d in #7666. cc @pan-apple.

woody-apple commented Jun 16, 2021

github-actions bot commented Jun 22, 2021

github-actions bot commented Jun 22, 2021

github-actions bot commented Jun 22, 2021

pan-apple commented Jun 22, 2021

mspang left a 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

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

pan-apple commented Jun 23, 2021

pan-apple commented Jun 15, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `3b7cd86` in #7666. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `3b7cd86` in #7666. cc @pan-apple.

mspang Jun 18, 2021 •

edited

Loading

This comment was generated by todo based on a `TODO` comment in `95aba4d` in #7666. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `95aba4d` in #7666. cc @pan-apple.