New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use node ID provided in the NOC #8826

Merged

bzbarsky-apple merged 7 commits into project-chip:master from pan-apple:cleanup-nodeid-assigment

Aug 11, 2021

Contributor

pan-apple commented Aug 5, 2021 •

edited

Loading

Problem

Node ID and Fabric ID are assigned via operational certificates. Currently, the code is inferring these IDs from the received messages.

Change overview

Get node ID and fabric ID (combined as PeerID aka Operational ID) from the node operational certificate.
This change resulted in a lot of other cleanup

Addition of new fabric to Fabric Table
Use of fabric ID and node ID in SecureSessionMgr
Not sending source and destination node ID in encrypted messages
Controller and Server usage of Fabric Table to store the new fabrics

Testing

CASE, PASE and IM tests run as part of CI.
Also tested end to end flow using chip-tool, Python controller and iOS controller app.

boring-cyborg bot added the examples label

pan-apple force-pushed the cleanup-nodeid-assigment branch from 929504f to b52b72f Compare

August 6, 2021 00:19

Contributor

bzbarsky-apple commented Aug 6, 2021

Due to changes in operational-credentials-cluster, ZAP files are regenerated, and hence changes in the generated code.

If possible, please break those out into a separate changeset.

tcarmelveilleux reviewed

View reviewed changes

src/app/server/Server.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/app/server/Server.cpp Outdated Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

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

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.h Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/crypto/CHIPCryptoPAL.cpp Outdated Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/protocols/secure_channel/CASEServer.cpp Show resolved Hide resolved

Contributor Author

pan-apple commented Aug 6, 2021

Due to changes in operational-credentials-cluster, ZAP files are regenerated, and hence changes in the generated code.

If possible, please break those out into a separate changeset.

I tried splitting it into two PRs. That was before I got it to a working state. Let me try again and see if it could be split.
The main issue is that some of the OpCreds cluster commands were not using fabric index correctly. And, this PR updates FabricTable implementation quite a bit. That spiraled down into updating OpCreds cluster.

tcarmelveilleux reviewed

View reviewed changes

src/transport/FabricTable.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/transport/FabricTable.cpp Show resolved Hide resolved

pan-apple added 4 commits

August 6, 2021 13:17


          Use node ID and fabric ID provided in the NOC

563ee1b


          update CHIPTool for modified opcreds cluster

cc17ec4


          remove more checks for NodeID

80d7fce


          split cluster changes from the current PR

f5e7db2

pan-apple force-pushed the cleanup-nodeid-assigment branch from 62d2dee to f5e7db2 Compare

August 6, 2021 20:22

boring-cyborg bot added app controller crypto darwin transport labels


          rename functions and variables

e7df255

msandstedt reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

pan-apple mentioned this pull request

Update OpCreds cluster to not use global fabric table #8905

Closed

msandstedt reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Outdated Show resolved Hide resolved

msandstedt reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Outdated Show resolved Hide resolved

pan-apple mentioned this pull request

app::CommandHandler should provide an accessor for the corresponding ExchangeContext #8906

Closed

tcarmelveilleux reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/app/server/Server.cpp Show resolved Hide resolved

tcarmelveilleux reviewed

View reviewed changes

src/controller/CHIPDeviceController.cpp Show resolved Hide resolved

msandstedt reviewed

View reviewed changes

src/transport/FabricTable.cpp Outdated Show resolved Hide resolved

This was referenced Aug 11, 2021

Rename chip::app::Mdns to chip::app::dnssd #8908

Closed

Need clarity on requirements and design of AdvertiseOperational() API #8909

Open

tcarmelveilleux reviewed

View reviewed changes

src/transport/FabricTable.cpp Outdated Show resolved Hide resolved

bzbarsky-apple approved these changes

View reviewed changes

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Outdated Show resolved Hide resolved

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp Show resolved Hide resolved

src/transport/FabricTable.cpp Outdated Show resolved Hide resolved

src/transport/FabricTable.h Outdated Show resolved Hide resolved

tcarmelveilleux approved these changes

View reviewed changes

Contributor

tcarmelveilleux left a comment

Technical review complete. This is a great step in the right direction. Minor nits present and some follow-up issues were called-out as needing to be filed/fixed.

msandstedt approved these changes

View reviewed changes

pan-apple mentioned this pull request

Move FabricTable to a better namespace than Transport #8911

Closed


          address review comments

6a3d8c5

bzbarsky-apple added the technical review - DONE label

pan-apple requested review from andy31415 and removed request for bhaskar-apple

August 11, 2021 19:24


          Fix build issues

7ab24da

pan-apple changed the title ~~Use node ID and fabric ID provided in the NOC~~ Use node ID provided in the NOC

jmartinez-silabs approved these changes

View reviewed changes

pullapprove bot added review - approved and removed review - pending labels

bzbarsky-apple merged commit 6a36256 into project-chip:master

pan-apple deleted the cleanup-nodeid-assigment branch

August 11, 2021 21:49

nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request


          Use node ID provided in the NOC (project-chip#8826)

f208c24

* Use node ID and fabric ID provided in the NOC

* update CHIPTool for modified opcreds cluster

* remove more checks for NodeID

* split cluster changes from the current PR

* rename functions and variables

* address review comments

* Fix build issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tcarmelveilleux tcarmelveilleux approved these changes

msandstedt msandstedt approved these changes

bzbarsky-apple bzbarsky-apple approved these changes

woody-apple woody-apple approved these changes

jmartinez-silabs jmartinez-silabs approved these changes

chrisdecenzo Awaiting requested review from chrisdecenzo

jelderton Awaiting requested review from jelderton

LuDuda Awaiting requested review from LuDuda

mspang Awaiting requested review from mspang

sagar-apple Awaiting requested review from sagar-apple

saurabhst Awaiting requested review from saurabhst

andy31415 Awaiting requested review from andy31415

Labels

app controller crypto darwin examples review - approved transport