Fix sequence of root cert and op cert provisioning #8550

pan-apple · 2021-07-21T19:38:36Z

Problem

Current device commissioning flow is sending root certificate before getting the CSR from the device. As per specifications, the following should be the flow

Request OpCSR
Receive OpCSR -> Send trusted root certs
Receive response for root cert provisioning -> Send OpCert

Change overview

Updated the certificate provisioning flow to match the specifications.

Testing

CI runs end to end commissioning tests.
Also manually tested commissioning using Python and chip-tool controller applications.

src/controller/CHIPDeviceController.cpp

src/controller/CHIPDeviceController.h

src/controller/CHIPDevice.h

todo · 2021-07-29T22:55:10Z

- Write attestation signature using attestation key

connectedhomeip/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

Lines 491 to 495 in 9291538

    
               // TODO - Write attestation signature using attestation key 
        
               SuccessOrExit(err = writer->Put(TLV::ContextTag(1), ByteSpan())); 
        
               SuccessOrExit(err = commandObj->FinishCommand()); 
        
           exit:

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

todo · 2021-07-29T22:55:13Z

- Verify that the generated root cert matches with commissioner's root cert

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 1254 to 1264 in 9291538

    
           // TODO - Verify that the generated root cert matches with commissioner's root cert 
        
           device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired]; 
        
           { 
        
               MutableByteSpan rootCert = device->GetMutableNOCChain(); 
        
               uint32_t certLen = (rootCert.size() > UINT32_MAX) ? UINT32_MAX : static_cast<uint32_t>(rootCert.size()); 
        
               err = ConvertX509CertToChipCert(rcac, rootCert.data(), certLen, certLen); 
        
               SuccessOrExit(err);

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

todo · 2021-07-29T22:55:16Z

Refactor this API to match latest spec, so that GenerateNodeOperationalCertificate receives the full CSR Elements data

connectedhomeip/src/controller/java/AndroidDeviceControllerWrapper.cpp

Lines 64 to 74 in 9291538

    
           // TODO Refactor this API to match latest spec, so that GenerateNodeOperationalCertificate receives the full CSR Elements data 
        
           // payload. 
        
           CHIP_ERROR AndroidDeviceControllerWrapper::GenerateNOCChain(const Optional<NodeId> & nodeId, FabricId fabricId, 
        
                                                                       const ByteSpan & csrElements, const ByteSpan & attestationSignature, 
        
                                                                       const ByteSpan & DAC, const ByteSpan & PAI, const ByteSpan & PAA, 
        
                                                                       Callback::Callback<OnNOCChainGeneration> * onCompletion) 
        
           { 
        
               jmethodID method; 
        
               CHIP_ERROR err = CHIP_NO_ERROR; 
        
               err            = JniReferences::GetInstance().FindMethod(JniReferences::GetInstance().GetEnvForCurrentThread(), mJavaObjectRef, 
        
                                                             "onOpCSRGenerationComplete", "([B)V", &method);

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

pan-apple · 2021-07-29T22:58:49Z

@tcarmelveilleux the latest commit tries to address your comments. Please take a look when you get a chance.
The changes are

Single API to request certificate chain from the CA
The CA is provided the NOCSRElements, attestation signatures and attestation related information.
The device object stores the ICAC and NOC in CHIPCert format for the duration of root certificate provisioning.

Also updated operational credentials server to generate the NOCSR in the correct format.

todo · 2021-07-30T00:30:56Z

- Need a mechanism to generate CSRNonce for commissioner's CSR

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 295 to 305 in 7193a5b

    
           // TODO - Need a mechanism to generate CSRNonce for commissioner's CSR 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(2), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(3), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(4), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(5), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.EndContainer(containerType)); 
        
           ReturnErrorOnFailure(csrElementWriter.Finalize()); 
        
           mOperationalCredentialsDelegate->SetNodeIdForNextNOCRequest(mLocalDeviceId); 
        
           mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(0);

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

todo · 2021-07-30T00:30:59Z

- Need a mechanism to generate signature for commissioner's CSR

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 306 to 312 in 7193a5b

    
               // TODO - Need a mechanism to generate signature for commissioner's CSR 
        
               ReturnErrorOnFailure(mOperationalCredentialsDelegate->GenerateNOCChain( 
        
                   ByteSpan(csrElements.Get(), csrElementWriter.GetLengthWritten()), ByteSpan(), ByteSpan(), ByteSpan(), ByteSpan(), 
        
                   &mLocalNOCChainCallback)); 
        
               ReturnErrorOnFailure(mFabrics.Store(fabric->GetFabricIndex())); 
        
           }

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

todo · 2021-07-30T00:31:02Z

- Verify that the generated root cert matches with commissioner's root cert

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 1277 to 1287 in 7193a5b

    
           // TODO - Verify that the generated root cert matches with commissioner's root cert 
        
           device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired]; 
        
           { 
        
               MutableByteSpan rootCert = device->GetMutableNOCChain(); 
        
               uint32_t certLen = (rootCert.size() > UINT32_MAX) ? UINT32_MAX : static_cast<uint32_t>(rootCert.size()); 
        
               err = ConvertX509CertToChipCert(rcac, rootCert.data(), certLen, certLen); 
        
               SuccessOrExit(err);

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

github-actions · 2021-07-30T08:17:59Z

Size increase report for "esp32-example-build" from 1d420e1

File	Section	File	VM
chip-lock-app.elf	.flash.text	108	108
chip-all-clusters-app.elf	.flash_rodata_dummy	0	65536
chip-all-clusters-app.elf	.flash.rodata	176	176
chip-all-clusters-app.elf	.flash.text	168	168
chip-temperature-measurement-app.elf	.flash.text	32	32

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_line,0,652
.debug_info,0,550
.debug_loc,0,292
.flash.text,108,108
.debug_str,0,40
.xt.prop._ZN26OpCredsFabricTableDelegate26OnFabricPersistedToStorageEPN4chip9Transport10FabricInfoE,0,40
.debug_abbrev,0,38
.debug_frame,0,24
.debug_aranges,0,8
.shstrtab,0,3
.strtab,0,1
.xt.prop._ZNK4chip11CASESession17GetI2RSessionInfoEv,0,-40
.debug_ranges,0,-48
[Unmapped],0,-108

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

sections,vmsize,filesize
.flash_rodata_dummy,65536,0
[LOAD #0 [RW]],0,65536
[Unmapped],0,3752
.debug_line,0,640
.debug_info,0,460
.debug_loc,0,328
.flash.rodata,176,176
.flash.text,168,168
.strtab,0,166
.debug_abbrev,0,116
.debug_frame,0,40
.debug_str,0,40
.debug_ranges,0,32
.symtab,0,32
.debug_aranges,0,8
.shstrtab,0,-2

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_line,0,646
.debug_info,0,435
.debug_loc,0,223
.debug_str,0,42
.xt.prop._ZN26OpCredsFabricTableDelegate26OnFabricPersistedToStorageEPN4chip9Transport10FabricInfoE,0,40
.debug_abbrev,0,38
.flash.text,32,32
.debug_frame,0,24
.debug_aranges,0,8
.shstrtab,0,3
.strtab,0,1
[Unmapped],0,-32
.xt.prop._ZNK4chip11CASESession17GetI2RSessionInfoEv,0,-40
.debug_ranges,0,-48

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

sections,vmsize,filesize

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

sections,vmsize,filesize

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

sections,vmsize,filesize

github-actions · 2021-07-30T08:18:48Z

Size increase report for "nrfconnect-example-build" from 1d420e1

File	Section	File	VM
chip-lock.elf	text	132	132
chip-lock.elf	device_handles	-4	-4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_info,0,1108
.debug_loc,0,636
.debug_line,0,338
text,132,132
.debug_ranges,0,56
.debug_str,0,40
.debug_abbrev,0,38
.debug_frame,0,24
.debug_aranges,0,8
device_handles,-4,-4

todo · 2021-07-30T15:36:39Z

- Need a mechanism to generate CSRNonce for commissioner's CSR

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 295 to 305 in 50580e7

    
           // TODO - Need a mechanism to generate CSRNonce for commissioner's CSR 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(2), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(3), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(4), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.Put(TLV::ContextTag(5), ByteSpan())); 
        
           ReturnErrorOnFailure(csrElementWriter.EndContainer(containerType)); 
        
           ReturnErrorOnFailure(csrElementWriter.Finalize()); 
        
           mOperationalCredentialsDelegate->SetNodeIdForNextNOCRequest(mLocalDeviceId); 
        
           mOperationalCredentialsDelegate->SetFabricIdForNextNOCRequest(0);

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.

todo · 2021-07-30T15:36:42Z

- Need a mechanism to generate signature for commissioner's CSR

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 306 to 312 in 50580e7

    
               // TODO - Need a mechanism to generate signature for commissioner's CSR 
        
               ReturnErrorOnFailure(mOperationalCredentialsDelegate->GenerateNOCChain( 
        
                   ByteSpan(csrElements.Get(), csrElementWriter.GetLengthWritten()), ByteSpan(), ByteSpan(), ByteSpan(), ByteSpan(), 
        
                   &mLocalNOCChainCallback)); 
        
               ReturnErrorOnFailure(mFabrics.Store(fabric->GetFabricIndex())); 
        
           }

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.

todo · 2021-07-30T15:36:45Z

- Verify that the generated root cert matches with commissioner's root cert

connectedhomeip/src/controller/CHIPDeviceController.cpp

Lines 1277 to 1287 in 50580e7

    
           // TODO - Verify that the generated root cert matches with commissioner's root cert 
        
           device = &commissioner->mActiveDevices[commissioner->mDeviceBeingPaired]; 
        
           { 
        
               MutableByteSpan rootCert = device->GetMutableNOCChain(); 
        
               uint32_t certLen = (rootCert.size() > UINT32_MAX) ? UINT32_MAX : static_cast<uint32_t>(rootCert.size()); 
        
               err = ConvertX509CertToChipCert(rcac, rootCert.data(), certLen, certLen); 
        
               SuccessOrExit(err);

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.

* Fix sequence of root cert and op cert provisioning * address review comments * Update operational credentials delegate to use single API * cleanup delegate API * Fix Android build

boring-cyborg bot added the controller label Jul 21, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, sagar-apple, saurabhst and woody-apple July 21, 2021 19:39

pullapprove bot added the review - pending label Jul 21, 2021

tcarmelveilleux reviewed Jul 21, 2021

View reviewed changes

src/controller/CHIPDeviceController.cpp Outdated Show resolved Hide resolved

pan-apple requested a review from tcarmelveilleux July 22, 2021 00:36

woody-apple added the SDK Approved label Jul 22, 2021

woody-apple approved these changes Jul 22, 2021

View reviewed changes

tcarmelveilleux reviewed Jul 22, 2021

View reviewed changes

src/controller/CHIPDeviceController.h Outdated Show resolved Hide resolved

jmartinez-silabs approved these changes Jul 24, 2021

View reviewed changes

andy31415 approved these changes Jul 26, 2021

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels Jul 26, 2021

tcarmelveilleux reviewed Jul 28, 2021

View reviewed changes

src/controller/CHIPDevice.h Outdated Show resolved Hide resolved

pan-apple requested a review from tcarmelveilleux July 28, 2021 16:58

pan-apple added 3 commits July 29, 2021 15:22

Fix sequence of root cert and op cert provisioning

24612f1

address review comments

c284965

Update operational credentials delegate to use single API

9291538

pan-apple force-pushed the fix-opcert-flow branch from c21dfd2 to 9291538 Compare July 29, 2021 22:55

boring-cyborg bot added app darwin examples labels Jul 29, 2021

pullapprove bot requested a review from selissia July 29, 2021 22:55

cleanup delegate API

7193a5b

Fix Android build

50580e7

bzbarsky-apple merged commit 84c369c into project-chip:master Jul 30, 2021

pan-apple deleted the fix-opcert-flow branch July 30, 2021 19:10

pan-apple mentioned this pull request Jul 30, 2021

Refactor OperationalCredentialsDelegate.h so that GetIntermediateCACertificate and GetRootCACertificate are asynchronous #8353

Closed

samadDotDev mentioned this pull request Aug 24, 2021

Persist NOC and ICA after async retrieval #8090

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sequence of root cert and op cert provisioning #8550

Fix sequence of root cert and op cert provisioning #8550

pan-apple commented Jul 21, 2021

todo bot commented Jul 29, 2021

todo bot commented Jul 29, 2021

todo bot commented Jul 29, 2021

pan-apple commented Jul 29, 2021

todo bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

github-actions bot commented Jul 30, 2021

github-actions bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

Fix sequence of root cert and op cert provisioning #8550

Fix sequence of root cert and op cert provisioning #8550

Conversation

pan-apple commented Jul 21, 2021

Problem

Change overview

Testing

todo bot commented Jul 29, 2021

- Write attestation signature using attestation key

This comment was generated by todo based on a TODO comment in 9291538 in #8550. cc @pan-apple.

todo bot commented Jul 29, 2021

- Verify that the generated root cert matches with commissioner's root cert

This comment was generated by todo based on a TODO comment in 9291538 in #8550. cc @pan-apple.

todo bot commented Jul 29, 2021

Refactor this API to match latest spec, so that GenerateNodeOperationalCertificate receives the full CSR Elements data

This comment was generated by todo based on a TODO comment in 9291538 in #8550. cc @pan-apple.

pan-apple commented Jul 29, 2021

todo bot commented Jul 30, 2021

- Need a mechanism to generate CSRNonce for commissioner's CSR

This comment was generated by todo based on a TODO comment in 7193a5b in #8550. cc @pan-apple.

todo bot commented Jul 30, 2021

- Need a mechanism to generate signature for commissioner's CSR

This comment was generated by todo based on a TODO comment in 7193a5b in #8550. cc @pan-apple.

todo bot commented Jul 30, 2021

- Verify that the generated root cert matches with commissioner's root cert

This comment was generated by todo based on a TODO comment in 7193a5b in #8550. cc @pan-apple.

github-actions bot commented Jul 30, 2021

github-actions bot commented Jul 30, 2021

todo bot commented Jul 30, 2021

- Need a mechanism to generate CSRNonce for commissioner's CSR

This comment was generated by todo based on a TODO comment in 50580e7 in #8550. cc @pan-apple.

todo bot commented Jul 30, 2021

- Need a mechanism to generate signature for commissioner's CSR

This comment was generated by todo based on a TODO comment in 50580e7 in #8550. cc @pan-apple.

todo bot commented Jul 30, 2021

- Verify that the generated root cert matches with commissioner's root cert

This comment was generated by todo based on a TODO comment in 50580e7 in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `9291538` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `7193a5b` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.

This comment was generated by todo based on a `TODO` comment in `50580e7` in #8550. cc @pan-apple.