[E2E][JF] Implemented ICAC CSR generation and ICAC issuance #39362
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Code Review
This pull request introduces new functionality for Intermediate CA (ICAC) Certificate Signing Request (CSR) generation and ICAC issuance, along with corresponding updates to the OperationalCredentialsDelegate interface and the ExampleOperationalCredentialsIssuer implementation. The changes appear to correctly implement the intended features, and new tests cover these additions.
I've identified a couple of areas for potential improvement related to memory allocation consistency and test design, which are detailed in the comments. The overall structure and integration of the new features seem well-executed.
No specific style guide was provided, so the review is based on general C++ best practices, consistency with the existing codebase, and the Google C++ Style Guide as a general reference.
Summary of Findings
- Memory Allocation Consistency: There's an inconsistency in how large certificate buffers (
kMaxDERCertLength) are allocated:rcacinSignICACAfterValidationuses stack allocation, whileicacinSignICAC(for a similar purpose) uses heap allocation viaScopedMemoryBuffer. This should be harmonized for safety and clarity. - Test State Management: Global variables (
gIcac,gIcacSpan) are used inTestExampleOperationalCredentialsIssuer.cppto share state between tests, which can lead to test interdependencies and reduce maintainability. Prefer self-contained tests or fixture-based state management. - Variable Initialization (Low Severity - Not Commented): In
SignICACAfterValidation(line 419), theerrvariable is initialized toCHIP_NO_ERRORand then immediately overwritten by the result ofmStorage->SyncGetKeyValuewithin thePERSISTENT_KEY_OPmacro. It could be declared without an initial value closer to its first assignment, or directly initialized by the operation if the macro structure allows, for minor conciseness.
Merge Readiness
The pull request implements significant new functionality for ICAC management. While the core logic appears sound, there are a couple of medium-severity issues regarding memory allocation consistency and test design that should be addressed to improve robustness and maintainability. I recommend making these changes before merging. As an AI reviewer, I am not authorized to approve pull requests; please ensure further review and approval from authorized team members.
src/controller/tests/TestExampleOperationalCredentialsIssuer.cpp
Outdated
Show resolved
Hide resolved
|
PR #39362: Size comparison from 0db703a to bf3abb4 Increases above 0.2%:
Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Jun 3, 2025
doru91
reviewed
Jun 6, 2025
doru91
reviewed
Jun 6, 2025
doru91
reviewed
Jun 6, 2025
doru91
reviewed
Jun 6, 2025
1593bf0 to
ef2a875
Compare
|
PR #39362: Size comparison from f38f1de to ef2a875 Full report (42 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, nrfconnect, nxp, psoc6, qpg, stm32)
|
|
PR #39362: Size comparison from 6aafb9d to bd61c3e Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39362: Size comparison from ae1474c to 6028994 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #39362: Size comparison from 0bac726 to 6b64f4d Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
doru91
reviewed
Jun 25, 2025
examples/jf-control-app/commands/example/ExampleOperationalCredentialsIssuer.cpp
Outdated
Show resolved
Hide resolved
doru91
reviewed
Jun 25, 2025
examples/jf-control-app/commands/example/ExampleOperationalCredentialsIssuer.cpp
Show resolved
Hide resolved
|
PR #39362: Size comparison from 0cda620 to 462fb97 Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
doru91
approved these changes
Jun 27, 2025
doru91
reviewed
Jun 27, 2025
examples/jf-control-app/commands/example/ExampleOperationalCredentialsIssuer.cpp
Show resolved
Hide resolved
bzbarsky-apple
previously requested changes
Jul 1, 2025
< 10000 div class="d-flex flex-items-center"> src/controller/OperationalCredentialsDelegate.h Outdated Show resolved Hide resolved
|
PR #39362: Size comparison from b6f0333 to 13d247e Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
There are no longer core SDK changes here, and I have no opinions on what jf-control-app does internally.
…AC issuance, and NOC issuance
|
PR #39362: Size comparison from c4c6bea to be6d62d Full report (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
chrisdecenzo
approved these changes
Jul 3, 2025
robertfarnum
approved these changes
Jul 4, 2025
|
/geminireview |
|
Acknowledged! I am now performing a comprehensive code review for this pull request. The review comments and summary will appear shortly as a new review. |
Gemini shouldn't request changes
dsavitsky-dsr
pushed a commit
to dsavitsky-dsr/connectedhomeip
that referenced
this pull request
Jul 7, 2025
…AC issuance, and NOC issuance (project-chip#39362)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses Issue #38197
Added implementation for ICAC CSR generation and ICAC issuance.
Testing