Separate device attestation and operational cert provisioning into separate steps #12915
Conversation
This file contains 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
|
PR #12915: Size comparison from a5a7d92 to d288cec Increases (2 builds for linux)
Decreases (4 builds for linux, p6)
Full report (15 builds for cyw30739, efr32, k32w, linux, p6, qpg, telink)
|
pan-apple
approved these changes
Jan 21, 2022
There was a problem hiding this comment.
It looks good to me. The code is moved within the controller API, from Controller.cpp and CommissioneeDeviceProxy to AutoCommissioner, and CommissioningDelegate.
It does change the state machine that drives the commissioning process. So some testing for all use-cases will be helpful. A lot of the use cases are tested via CI. There are some controller/commissioner apps that are not in CI.
|
PR #12915: Size comparison from 1b49aee to 51b8892 Increases (1 build for linux)
Decreases (1 build for linux)
Full report (22 builds for cyw30739, efr32, esp32, k32w, linux, mbed, p6, qpg, telink)
|
andy31415
approved these changes
Jan 24, 2022
|
fast track: this has been up for quite a while now and has had several review passes. |
msandstedt
approved these changes
Jan 24, 2022
|
PR #12915: Size comparison from 1b49aee to 90ecc76 Increases (1 build for linux)
Decreases (29 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
tcarmelveilleux
approved these changes
Jan 24, 2022
|
PR #12915: Size comparison from 87d3698 to 3b9971b Increases above 0.2%:
Increases (26 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (7 builds for esp32, linux, nrfconnect, p6)
Full report (33 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 24, 2022
This amendment takes advantage project-chip#12915, which makes available both the root certificate and NOC in memory at the same time during commissioning. Because of this, the commissioner no longer needs to extract and store the Root CA public key to generate the compressed fabric ID.
This amendment takes advantage project-chip#12915, which makes available both the root certificate and NOC in memory at the same time during commissioning. Because of this, the commissioner no longer needs to extract and store the Root CA public key to generate the compressed fabric ID.
cleanup operational ID extraction from certificates
|
PR #12915: Size comparison from 87d3698 to 8a88412 Increases above 0.2%:
Increases (32 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
Decreases (10 builds for esp32, linux, nrfconnect)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, qpg, telink)
|
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 25, 2022
msandstedt
added a commit
to msandstedt/connectedhomeip
that referenced
this pull request
Jan 25, 2022
woody-apple
pushed a commit
that referenced
this pull request
Jan 26, 2022
…13830) * Add interfaces to pass ipk and adminSubject from op creds delegate The operational credentials delegate provides interfaces to pass certificates back to the commissioner, but cannot fully specify the contents of the AddNOC command. Missing are the IPK and AdminSubject. Without these, those can only be managed locally. This commit adds optional interfaces to pass these from the delegate. Where AdminSubject is not passed, the commissioner uses its own node ID as before. And where IPK is not passed, the commissioner writes a 0-length IPK, which is also as before and is what we are using until GroupKeyManagement and other dependencies are in place. Fixes #13503 * fix android build * fix sanitization test * fix iOS build * per tcarmelveilleux, do not enclose 0-length IPK ind AddNOC * remove Has methods from CommissioningParameters for brevity * restyle * Fix #12915 merge
selissia
pushed a commit
to selissia/connectedhomeip
that referenced
this pull request
Jan 28, 2022
…parate steps (project-chip#12915) * DAC and PAI storage/control to AutoCommissioner. * Move attestation step to commissioning delegate * Move signing request and NOC chain gen to AutoCommissioner. * Remaining steps pulled out into external commissioner. * Fix up error handling * Add some additional logging I can't repro the cirque failure locally for some reason. Adding logging to figure out what's up. * I'm dumb. * Error checking on parameter setting. * Fix sizing on thread data set * Use Variant instead of union. * Simplify error handling. * Get rid of some debug cruft. * cleanup operational ID extraction from certificates This amendment takes advantage project-chip#12915, which makes available both the root certificate and NOC in memory at the same time during commissioning. Because of this, the commissioner no longer needs to extract and store the Root CA public key to generate the compressed fabric ID. Co-authored-by: Michael Sandstedt <michael.sandstedt@gmail.com>
selissia
pushed a commit
to selissia/connectedhomeip
that referenced
this pull request
Jan 28, 2022
…roject-chip#13830) * Add interfaces to pass ipk and adminSubject from op creds delegate The operational credentials delegate provides interfaces to pass certificates back to the commissioner, but cannot fully specify the contents of the AddNOC command. Missing are the IPK and AdminSubject. Without these, those can only be managed locally. This commit adds optional interfaces to pass these from the delegate. Where AdminSubject is not passed, the commissioner uses its own node ID as before. And where IPK is not passed, the commissioner writes a 0-length IPK, which is also as before and is what we are using until GroupKeyManagement and other dependencies are in place. Fixes project-chip#13503 * fix android build * fix sanitization test * fix iOS build * per tcarmelveilleux, do not enclose 0-length IPK ind AddNOC * remove Has methods from CommissioningParameters for brevity * restyle * Fix project-chip#12915 merge
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.
Problem
Device attestation and operational certificate provisioning are currently implemented as a long chain of cluster commands in the device commissioner. While this works well currently, it means that all the information required for attestation and op cert provisioning needs to be available when the command chain is triggered. it is somewhat limiting for commissioners that have more complex certificate provisioning requirements where they may need to use the information provided by earlier steps, then perform external steps to retrieve or generate certs before continuing. This PR breaks these long chains into individual commissioning steps with callbacks and drives the chain from the auto commissioner class. This means that a commissioner could replace the auto commissioner delegate with a delegate that performs additional work between the phases of the op cert provisioning. This will also allow us to do a bit of code cleanup to handle the error cases in a more centralized way (later PR).
Change overview
Separates the device attestation and certification provisioning command chains into individual commissioning steps and drives these from the external autocommissioner class.
NOTE: There was a fair amount of code moved in this change. The changes are broken up into the individual steps as separate commits to make the review easier to parse.
Testing
BLE testing: chip-tool with M5 all clusters app.
IP testing: linux lighting app with chip-tool.
also covered by cirque tests.