Update OpCreds cluster to not use global fabric table #8905
Assignees
Labels
Comments
lzgrablic02
added
commissioning
|
@tcarmelveilleux Is this still relevant? |
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this issue
Jun 21, 2022
- FabricTable management during commissioning did not properly handle the fact that committing needs to only be done on CommissioningComplete, which prevented the AddTrustedRootCertificate, UpdateNOC and AddNOC command semantics to be implemented properly and prevented proper state observation of operational credential clusters server Fixes project-chip#7695 Issue project-chip#8905 Fixes project-chip#18633 Issue project-chip#17208 Fixes project-chip#15585 This PR: - Removes direct access to FabricInfo from everywhere, which caused possibly stale FabricInfo references during commissioning. - Remove immediate committing of fabric table on UpdateNOC. - Make Fabrics, NOCs and TrustedRootCertificates attributes reflect proper partial state during fail-safe, by using the shadow data capabilities of OperationalCertificateStore and by updates to FabricInfo - Make it possible to unit test fabric table by providing the necessary lifecycle public APIs to test every modality of the commissioning flow - Make Server and DeviceController use OperationalCertificateStore to allow proper external lifecycle management of the operational cert chain. - Update all examples/controller code to new API - Remove dangerous internal APIs from FabricTable and replace with direct accessors where needed - Add more of the necessary spec validations to the UpdateNOC and AddNOC flows Testing done: - Updated all unit tests, all pass - Cert tests still pass as before - Working on further integration tests and unit tests as a follow-up noting that current state has not regressed on existing test coverage, and that new usage of OperationalCertificateStore class in FabricTable gains a large amount of additional coverage transitively via some of the existing tests making use of FabricTable.
tcarmelveilleux
added a commit
that referenced
this issue
Jun 24, 2022
* Implement shadow fail-safe data in FabricTable - FabricTable management during commissioning did not properly handle the fact that committing needs to only be done on CommissioningComplete, which prevented the AddTrustedRootCertificate, UpdateNOC and AddNOC command semantics to be implemented properly and prevented proper state observation of operational credential clusters server Fixes #7695 Issue #8905 Fixes #18633 Issue #17208 Fixes #15585 This PR: - Removes direct access to FabricInfo from everywhere, which caused possibly stale FabricInfo references during commissioning. - Remove immediate committing of fabric table on UpdateNOC. - Make Fabrics, NOCs and TrustedRootCertificates attributes reflect proper partial state during fail-safe, by using the shadow data capabilities of OperationalCertificateStore and by updates to FabricInfo - Make it possible to unit test fabric table by providing the necessary lifecycle public APIs to test every modality of the commissioning flow - Make Server and DeviceController use OperationalCertificateStore to allow proper external lifecycle management of the operational cert chain. - Update all examples/controller code to new API - Remove dangerous internal APIs from FabricTable and replace with direct accessors where needed - Add more of the necessary spec validations to the UpdateNOC and AddNOC flows Testing done: - Updated all unit tests, all pass - Cert tests still pass as before - Working on further integration tests and unit tests as a follow-up noting that current state has not regressed on existing test coverage, and that new usage of OperationalCertificateStore class in FabricTable gains a large amount of additional coverage transitively via some of the existing tests making use of FabricTable. * Restyled by clang-format * Fixes after merging master * Rename callback of FabricTable::Delegate - Remove needless ones - Make the callbacks do nothing by default to avoid more "Intentionally left blank" - Renamed to actually reflect what is happening * Rekick restyle * restyle * Align last known good time commit / revert to shadow fail-safe approach Fabric commit / revert is being refactored to only persist fabric data when we actually commit on receipt of CommissioningComplete. This reworks Last known Good Time to use the same strategy. Now, instead of persisting both last known good time and a fail-safe backup at the time of certificate installation, a single, updated last known good time is stored in RAM at the time of certificate installation. Then, on commit, this is persisted, but never before. * Fix CI * Fix TV app * Reduce flash usage on K32W0 by disabling detail (verbose) logs * Disable progress logging in TI platform lock example to save flash * Disable progress logging in TI platform shell example to save flash * Reduce stack usage of TestEventLogging * Apply review comments from @msandstedt * Save stack for Nordic build * Save stack for Nordic build, some more * Added unit tests for all basic operations * Fix merge conflict * Restyle * Change nrf connect stack limit up by 512 bytes for fake unit test comparisons * Revert "Save stack for Nordic build" This reverts commit 52699c4. * Revert "Save stack for Nordic build, some more" This reverts commit e62391e. * Fix UpdateLabel * Apply review comments, add ACL fabric removal * Apply review comments * per bzbarsky, clear in-memory Last Known Good Time if revert fails * Apply fixes from @bzbarsky-apple * Apply more review comments * Restyled * Fix tests * Add unit test for iterator * Iterator now works * Fix semantic merge conflict * Restyled * Fix predicates for storage presence temporarily issue #16958 * Fix Darwin build * Add missing docs, move methods to private that shouldn't be public * Change stack warning temporarily to pass on nRFConnect * Make MatterControllerFactory use const FabricInfo * Restyle * Fix semantic conflict on SessionManager * Fix an init ordering issue in TestSessionManager.cpp * Fix SessionManager shutdown * Restyled Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Michael Sandstedt <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
|
The remaining issue of the fact that all built-in cluster servers use singleton access will be fixed post-1.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Problem
The current implementation of operational credentials cluster uses a global variable to access fabric table. This makes it impossible to write tests where a same cluster server could be used to commission multiple nodes. Other similar use cases are also impacted by this design.
Proposed Solution
Find a way to pass the fabric table as a context to the cluster server.
Reference: #8826 (comment)
The text was updated successfully, but these errors were encountered: