Skip to content
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

AddTrustedRootCertificate is missing validations from spec #17208

Closed
tcarmelveilleux opened this issue Apr 8, 2022 · 3 comments
Closed

AddTrustedRootCertificate is missing validations from spec #17208

tcarmelveilleux opened this issue Apr 8, 2022 · 3 comments
Assignees
@emargolis @bzbarsky-apple
Labels
bug Something isn't working commissioning Involves placing devices on the network, initial setup spec Mismatch between spec and implementation V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

The following TODOs exists in AddTrustedRootCertificate handler:

    // TODO: Ensure we do not duplicate roots in storage, and detect "same key, different cert" errors
    // TODO: Validate cert signature prior to setting.
    VerifyOrExit(gFabricBeingCommissioned.SetRootCert(RootCertificate) == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_INVALID_FIELD);

Both the duplicate check and signature validation are required. Furthermore, the assumption that the root should be stored in the "gFabricBeingCommissioned" means we cannot reuse roots for multiple clients without re-adding the root (which is not required by spec)

Proposed Solution

  • Implement signature verification check of root
  • Implement validity checks required by spec on AddTrustedRootCertificate
@tcarmelveilleux tcarmelveilleux added V1.0 spec Mismatch between spec and implementation commissioning Involves placing devices on the network, initial setup bug Something isn't working labels Apr 8, 2022
This was referenced Apr 8, 2022
Closed
Closed
@tcarmelveilleux tcarmelveilleux self-assigned this Jun 13, 2022
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jun 15, 2022
@tcarmelveilleux
Introduce fail-safe compliant Operational Cert storage
5c8d60b 
- This PR is an intermediate fully unit-tested step towards replacing
  all NOC/ICAC/RCAC storage from Fabric-Table to allow spec-compliant
  fail-safe implementation
- Right now, we have hacky code in opcreds cluster server and in FabricTable
  to attempt to affect fail-safe properly. It doesn't actually work and
  prevents the TrustedRootCertificates, NOCs and Fabrics attributes from
  being implemented properly, and prevents UpdateNOC from being implemented
  to spec

Issue project-chip#18633
Issue project-chip#17208
Issue project-chip#15585

This PR implements an operational cert storage interface and provides
an implementation based on exact storage from existing FabricTable
for backwards compatibility. Its usage in FabricTable and via
opcreds cluster is a follow-up.

Testing done:
- Added large-scale new code unit test
- Unit tests pass
- Not hooked-up to device software yet, so just the unit tests need to pass
@tcarmelveilleux tcarmelveilleux mentioned this issue Jun 15, 2022
Merged
andy31415 pushed a commit that referenced this issue Jun 16, 2022
@tcarmelveilleux @restyled-commits
Introduce fail-safe compliant Operational Cert storage (#19643)
22888b0 
* Introduce fail-safe compliant Operational Cert storage

- This PR is an intermediate fully unit-tested step towards replacing
  all NOC/ICAC/RCAC storage from Fabric-Table to allow spec-compliant
  fail-safe implementation
- Right now, we have hacky code in opcreds cluster server and in FabricTable
  to attempt to affect fail-safe properly. It doesn't actually work and
  prevents the TrustedRootCertificates, NOCs and Fabrics attributes from
  being implemented properly, and prevents UpdateNOC from being implemented
  to spec

Issue #18633
Issue #17208
Issue #15585

This PR implements an operational cert storage interface and provides
an implementation based on exact storage from existing FabricTable
for backwards compatibility. Its usage in FabricTable and via
opcreds cluster is a follow-up.

Testing done:
- Added large-scale new code unit test
- Unit tests pass
- Not hooked-up to device software yet, so just the unit tests need to pass

* Restyled by clang-format

* Remove 2 unused variables in unit test to fix CI

Co-authored-by: Restyled.io <[email protected]>
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jun 21, 2022
@tcarmelveilleux
Implement shadow fail-safe data in FabricTable
54b18f9 
- 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 tcarmelveilleux mentioned this issue Jun 21, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Jun 24, 2022
@tcarmelveilleux @restyled-commits
@msandstedt @bzbarsky-apple
Implement shadow fail-safe data in FabricTable (#19819)
e5e09f5 
* 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]>
@emargolis emargolis mentioned this issue Jul 11, 2022
Merged
@woody-apple
Copy link
Contributor

Spec Issue Review: Assigning @bzbarsky-apple to confirm if there's any remaining work, and close if not.

@emargolis
Copy link
Contributor

The first TODO became obsolete and was removed in #21060
The second TODO was addressed in #20562
This ticket can be closed

@bzbarsky-apple
Copy link
Contributor

Yep, that matches my understanding. Thank you @emargolis !

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

@emargolis emargolis

@bzbarsky-apple bzbarsky-apple

Labels
bug Something isn't working commissioning Involves placing devices on the network, initial setup spec Mismatch between spec and implementation V1.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tcarmelveilleux @emargolis @woody-apple @bzbarsky-apple