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 without AddNOC is broken #15311

Closed
bzbarsky-apple opened this issue Feb 17, 2022 · 1 comment · Fixed by #18965
Closed

AddTrustedRootCertificate without AddNOC is broken #15311

bzbarsky-apple opened this issue Feb 17, 2022 · 1 comment · Fixed by #18965
Assignees
@emargolis
Labels
security spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

AddTrustedRootCertificate sets the new root cert in gFabricBeingCommissioned... but then nothing ever happens to get it anywhere else. This means:

  1. If we never make an AddNOC call, the new root cert will never get added to the list in any way.
  2. Until we make an AddNOC call the root cert list as viewed by the attribute will not show the new root. Which also means our MatterReportingAttributeChangeCallback call there is nonsense.

Proposed Solution

Implement something closer to the spec.
@bzbarsky-apple bzbarsky-apple added spec Mismatch between spec and implementation security V1.0 and removed V1.0 labels Feb 17, 2022
@bzbarsky-apple bzbarsky-apple changed the title AddTrustedRootCertificate outside commissioning is broken AddTrustedRootCertificate without AddNOC is broken Feb 17, 2022
@bzbarsky-apple
Copy link
Contributor Author

Maybe in practice no one will add root certs without doing AddNOC so this is not a problem?

@bzbarsky-apple bzbarsky-apple mentioned this issue Feb 17, 2022
Merged
@yufengwangca yufengwangca self-assigned this Feb 18, 2022
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 30, 2022
@tcarmelveilleux
Multiple spec compliance fixes to NOC cluster
fe67e05 
- Add all missing IM-level checks that would lead to
  INVALID_COMMAND error on incorrectly provided inputs
- Normalize logging of errors some more
- Add missing interlocks on fail-safe
- Remove RemoveTrustedRootCertificate command
- Ensure error-handling in AddTrustedRootCertificate
  does not destroy in-progress state on innocuous errors
- Move some IM structural checks before fail-safe checks
  in AddNOC/UpdateNOC

Issue project-chip#18633
Fixes project-chip#15311
Fixes project-chip#17209
@tcarmelveilleux tcarmelveilleux mentioned this issue May 30, 2022
Merged
tcarmelveilleux added a commit that referenced this issue Jun 1, 2022
@tcarmelveilleux @restyled-commits
Multiple spec compliance fixes to NOC cluster (#18965)
50d1c8e 
* Multiple spec compliance fixes to NOC cluster

- Add all missing IM-level checks that would lead to
  INVALID_COMMAND error on incorrectly provided inputs
- Normalize logging of errors some more
- Add missing interlocks on fail-safe
- Remove RemoveTrustedRootCertificate command
- Ensure error-handling in AddTrustedRootCertificate
  does not destroy in-progress state on innocuous errors
- Move some IM structural checks before fail-safe checks
  in AddNOC/UpdateNOC

Issue #18633
Fixes #15311
Fixes #17209

* ZAP changes

* Restyled by clang-format

* regen zap

* Restyled by clang-format

* Add missing root cert size validation

* Fix CI

* Restyled by clang-format

* Fix CI

* Apply review comments

* ZAP regen

* Restyled by whitespace

* Restyled by clang-format

* Fix a constant move to wrong context

* Add FAILSAFE_REQUIRED and TIMED_REQUEST_MISMATCH

* ZAP regen

* Restyled by whitespace

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@emargolis emargolis

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

Successfully merging a pull request may close this issue.

Multiple spec compliance fixes to NOC cluster tcarmelveilleux/connectedhomeip
3 participants
@emargolis @yufengwangca @bzbarsky-apple