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

Add/RemoveTrustedRootCertificate must be updated to match latest spec #17209

Closed
tcarmelveilleux opened this issue Apr 8, 2022 · 2 comments · Fixed by #18965
Closed

Add/RemoveTrustedRootCertificate must be updated to match latest spec #17209

tcarmelveilleux opened this issue Apr 8, 2022 · 2 comments · Fixed by #18965
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 RemoveTrustedRootCertificate handler in the SDK does not seem to work at all.

By inspecting the code, we find:

// TODO: Implement the logic for RemoveTrustedRootCertificate
    EmberAfStatus status = EMBER_ZCL_STATUS_FAILURE;
    emberAfSendImmediateDefaultResponse(status);

Proposed Solution

Fix the logic of RemoveTrustedRootCertificate to follow spec. This is symmetrical to #17208
@tcarmelveilleux tcarmelveilleux added bug Something isn't working V1.0 spec Mismatch between spec and implementation commissioning Involves placing devices on the network, initial setup labels Apr 8, 2022
@tcarmelveilleux tcarmelveilleux mentioned this issue Apr 8, 2022
Closed
@tcarmelveilleux
Copy link
Contributor Author

https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5269 covers a fix in spec to match SDK by removing the command

@woody-apple woody-apple added spec Mismatch between spec and implementation and removed spec Mismatch between spec and implementation labels May 26, 2022
@tcarmelveilleux tcarmelveilleux changed the title RemoveTrustedRootCertificate does not work according to spec Add/RemoveTrustedRootCertificate must be updated to match latest spec May 26, 2022
@tcarmelveilleux
Copy link
Contributor Author

Spec changes impact AddTrustedRootCertiticate as well

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
No one assigned
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

Successfully merging a pull request may close this issue.

Multiple spec compliance fixes to NOC cluster tcarmelveilleux/connectedhomeip
2 participants
@tcarmelveilleux @woody-apple