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

Revert the state of operational key pair, NOC and ICAC on fail-safe expiration #16443

Closed
yufengwangca opened this issue Mar 18, 2022 · 1 comment · Fixed by #19277
Closed

Revert the state of operational key pair, NOC and ICAC on fail-safe expiration #16443

yufengwangca opened this issue Mar 18, 2022 · 1 comment · Fixed by #19277
Assignees
@tcarmelveilleux
Labels
spec Mismatch between spec and implementation V1.0

Comments

@yufengwangca
Copy link
Contributor

Problem

<what's wrong or missing, please include any applicable:
If an UpdateNOC command had been successfully invoked, revert the state of operational key pair, NOC and ICAC for that Fabric to the state prior to the Fail-Safe timer being armed, for the Fabric Index that was the subject of the UpdateNOC command.

if (event->CommissioningComplete.UpdateNocCommandHasBeenInvoked)
{
    // TODO: Revert the state of operational key pair, NOC and ICAC
}

Proposed Solution

<suggested fix, suggested enhancement>
@yufengwangca yufengwangca self-assigned this Mar 18, 2022
@bzbarsky-apple bzbarsky-apple added V1.0 spec Mismatch between spec and implementation labels May 22, 2022
@bzbarsky-apple bzbarsky-apple changed the title Revert the state of operational key pair, NOC and ICAC Revert the state of operational key pair, NOC and ICAC on fail-safe expiration May 22, 2022
@woody-apple
Copy link
Contributor

Spec Review: Assigning to @tcarmelveilleux

tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue Jun 7, 2022
@tcarmelveilleux
Properly manage operational key lifecycle for fail-safe
f29b087 
- Fail-safe did not properly manage the roll-back of operational keys
- Operational key storage being centralized by value in FabricTable
  prevented ability to back keys by hardware/OS and allow the rollback
  of keys on failsafe expiry
- CASE code was using "raw" FabricInfo * which could go stale on UpdateNOC
  or after fail-safe expiry.

This PR:
- Adds an OperationalKeystore interface
- Make the FabricTable use the OperationalKeystore for when
  a commissionable node (with Opcreds cluster) is being commissioned
- Retain legacy controller behavior that allows injection of operational
  keys
- Simplifies the fail-safe handling lifecycle
- Add logging to fail-safe handling
- Add logging to general commissioning cluster
- Make CASE use ScopedNodeId everywhere
- Implement IsForUpdateNOC in fail-safe and opcreds cluster

Fixes project-chip#19072
Issue project-chip#18633
Fixes project-chip#16443
@tcarmelveilleux tcarmelveilleux mentioned this issue Jun 7, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Jun 10, 2022
@tcarmelveilleux @restyled-commits
@bzbarsky-apple @woody-apple @tehampson
Properly manage operational key lifecycle for fail-safe (#19277)
4e3930a 
* Properly manage operational key lifecycle for fail-safe

- Fail-safe did not properly manage the roll-back of operational keys
- Operational key storage being centralized by value in FabricTable
  prevented ability to back keys by hardware/OS and allow the rollback
  of keys on failsafe expiry
- CASE code was using "raw" FabricInfo * which could go stale on UpdateNOC
  or after fail-safe expiry.

This PR:
- Adds an OperationalKeystore interface
- Make the FabricTable use the OperationalKeystore for when
  a commissionable node (with Opcreds cluster) is being commissioned
- Retain legacy controller behavior that allows injection of operational
  keys
- Simplifies the fail-safe handling lifecycle
- Add logging to fail-safe handling
- Add logging to general commissioning cluster
- Make CASE use ScopedNodeId everywhere
- Implement IsForUpdateNOC in fail-safe and opcreds cluster

Fixes #19072
Issue #18633
Fixes #16443

* Fix merge of upstream

* Restyled by whitespace

* Restyled by clang-format

* Revert unintended testing changes

* Add remove operation

* Fix CI and add tests to support further tests

* Fix more CI

* Restyled by clang-format

* Darwin changes to use the new setup

* Added unit test and HasOpKeypairForFabric()

* Restyled by clang-format

* Restyled by gn

* Apply review comments from @msandstedt

* Add plumbing for init of controllers

* Restyled by clang-format

* Fix darwin tests

* Fix CI and address review comments

* Fix comment typos

* Apply review comments from @bzbarsky-apple and @tehampson

* Restyled by clang-format

* Fix more comments

* Restyled by clang-format

* Fix CI

* Fix cirque

* Restyled by clang-format

* Update src/crypto/tests/TestPersistentStorageOpKeyStore.cpp

Co-authored-by: tehampson <thampson@google.com>

* Address review comments

* Fix CI

* More clang-tidy fixes

Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
Co-authored-by: Justin Wood <woody@apple.com>
Co-authored-by: tehampson <thampson@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
spec Mismatch between spec and implementation V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Properly manage operational key lifecycle for fail-safe tcarmelveilleux/connectedhomeip
4 participants
@tcarmelveilleux @yufengwangca @woody-apple @bzbarsky-apple