Allow Darwin framework consumers to provide a controller NOC and keypair #18519
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
…air. Specific changes: * Rename initWithKeypair API to initWithSigningKeypair, since we can now also init with an operational keypair. * Fix FabricInfo::SetOperationalKeypair to correcty handle the mHasExternallyOwnedOperationalKey case. The old code would try to deserialize into the externally owned keypair. * Fix FabricInfo::GetOperationalKey to just return the key even it it's null, instead of allocating a random key that does not match anything. I have checked that consumers all either null-check the call or have just called SetOperationalKeypair or SetExternallyOwnedOperationalKeypair. SetFabricInfo is updated to error out if the incoming fabric info has a null operational key. This change was needed for basic API sanity in terms of not accidentally switching a fabric from an externally managed operational key to an internally managed randomly generated one. * Fix backwards boolean check in FabricInfo::Reset that was causing us to leak internally managed keys and try to delete externally managed ones. * Change Darwin CHIPDeviceControllerStartupParams to allow providing an operational keypair to be used for the NOC. * Change Darwin CHIPDeviceControllerStartupParams To allow providing a NOC instead of having one generated inside the framework. * Refactor the code for initializing CHIPDeviceControllerStartupParamsInternal to better share code and support the new functionality. * Allow initializing CHIPOperationalCredentialsDelegate without a NOC-signing keypair. This is needed because the SDK's controller init requires a credentials delegate. When initialized in this way, the delegate will just return error when asked to create a NOC. * Added tests for the new API (which caught a number of the issues listed above). Fixes project-chip#18444
|
PR #18519: Size comparison from d79a0e3 to bb2e6ef Increases (5 builds for cc13x2_26x2)
Decreases (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
pullapprove
bot
requested review from
andy31415,
anush-apple,
arkq,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple and
lazarkov
pullapprove
bot
requested review from
msandstedt,
robszewczyk,
saurabhst,
selissia,
tecimovic,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21,
yufengwangca and
yunhanw-google
tcarmelveilleux
approved these changes
May 17, 2022
woody-apple
approved these changes
May 17, 2022
|
PR #18519: Size comparison from d79a0e3 to 8c8c3c9 Increases (5 builds for cc13x2_26x2)
Decreases (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
tcarmelveilleux
pushed a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
May 19, 2022
- When a commissioner is backing their key with OS or hardware support, the built-in P256Keypair::NewCertificateSigningRequest will not be usable since it relies on internal P256Keypair base class access to key state, as opposed to just using Pubkey() and ECDSA_sign_message primitives. This is OK on some embedded usecases that make use of P256Keypair backend directly, but not for many other usecases. - On iOS/Darwin and on native Android, backing the P256Keypair * by derived classes is bridgeable to platform APIs, but those platform APIs do not offer easy/direct CSR generation, and on Darwin, there are not ASN.1 APIs anymore. - If trying to make use of Darwin APIs introduced in project-chip#18519, there is no easy way to write code interfacing with an external CA to provide a CSR for a natively bridged keypair. This PR adds a first-principle CSR generator, written and audited by Google personel, using the ASN1Writer API already used in CHIPCert.h and used by all Commissioner code making use of SDK today. This is a straightforward implementation that directly uses a P256Keypair * (or a derived class thereof!) to generate a CSR against it, without depending on direct key access like like the native version P256Keypair::NewCerticateSigningRequest does. This PR also fixes constness of operations on P256Keypair. Issue project-chip#18444 Testing done: - Added unit tests for the new primitive - Validated generated CSR with OpenSSL - Validated equivalence to generated CSR from P256Keypair, on both mbedTLS and OpenSSL - Not used by CHIP-tool but usable by Darwin and Android framework users.
tcarmelveilleux
added a commit
to tcarmelveilleux/connectedhomeip
that referenced
this pull request
May 19, 2022
- When a commissioner is backing their key with OS or hardware support, the built-in P256Keypair::NewCertificateSigningRequest will not be usable since it relies on internal P256Keypair base class access to key state, as opposed to just using Pubkey() and ECDSA_sign_message primitives. This is OK on some embedded usecases that make use of P256Keypair backend directly, but not for many other usecases. - On iOS/Darwin and on native Android, backing the P256Keypair * by derived classes is bridgeable to platform APIs, but those platform APIs do not offer easy/direct CSR generation, and on Darwin, there are not ASN.1 APIs anymore. - If trying to make use of Darwin APIs introduced in project-chip#18519, there is no easy way to write code interfacing with an external CA to provide a CSR for a natively bridged keypair. This PR adds a first-principle CSR generator, written and audited by Google personel, using the ASN1Writer API already used in CHIPCert.h and used by all Commissioner code making use of SDK today. This is a straightforward implementation that directly uses a P256Keypair * (or a derived class thereof!) to generate a CSR against it, without depending on direct key access like like the native version P256Keypair::NewCerticateSigningRequest does. This PR also fixes constness of operations on P256Keypair. Issue project-chip#18444 Testing done: - Added unit tests for the new primitive - Validated generated CSR with OpenSSL - Validated equivalence to generated CSR from P256Keypair, on both mbedTLS and OpenSSL - Not used by CHIP-tool but usable by Darwin and Android framework users.
tcarmelveilleux
added a commit
that referenced
this pull request
May 20, 2022
…rs (#18631) * Implement CSR generation from first principles to support commissioners - When a commissioner is backing their key with OS or hardware support, the built-in P256Keypair::NewCertificateSigningRequest will not be usable since it relies on internal P256Keypair base class access to key state, as opposed to just using Pubkey() and ECDSA_sign_message primitives. This is OK on some embedded usecases that make use of P256Keypair backend directly, but not for many other usecases. - On iOS/Darwin and on native Android, backing the P256Keypair * by derived classes is bridgeable to platform APIs, but those platform APIs do not offer easy/direct CSR generation, and on Darwin, there are not ASN.1 APIs anymore. - If trying to make use of Darwin APIs introduced in #18519, there is no easy way to write code interfacing with an external CA to provide a CSR for a natively bridged keypair. This PR adds a first-principle CSR generator, written and audited by Google personel, using the ASN1Writer API already used in CHIPCert.h and used by all Commissioner code making use of SDK today. This is a straightforward implementation that directly uses a P256Keypair * (or a derived class thereof!) to generate a CSR against it, without depending on direct key access like like the native version P256Keypair::NewCerticateSigningRequest does. This PR also fixes constness of operations on P256Keypair. Issue #18444 Testing done: - Added unit tests for the new primitive - Validated generated CSR with OpenSSL - Validated equivalence to generated CSR from P256Keypair, on both mbedTLS and OpenSSL - Not used by CHIP-tool but usable by Darwin and Android framework users. * Update src/crypto/CHIPCryptoPAL.h Co-authored-by: Boris Zbarsky <[email protected]> * Fix CI * Restyled by clang-format Co-authored-by: Boris Zbarsky <[email protected]> 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
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.
Specific changes:
Rename initWithKeypair API to initWithSigningKeypair, since we can
now also init with an operational keypair.
Fix FabricInfo::SetOperationalKeypair to correcty handle the
mHasExternallyOwnedOperationalKey case. The old code would try to
deserialize into the externally owned keypair.
Fix FabricInfo::GetOperationalKey to just return the key even it
it's null, instead of allocating a random key that does not match
anything. I have checked that consumers all either null-check the
call or have just called SetOperationalKeypair or
SetExternallyOwnedOperationalKeypair. SetFabricInfo is updated to
error out if the incoming fabric info has a null operational key.
This change was needed for basic API sanity in terms of not
accidentally switching a fabric from an externally managed
operational key to an internally managed randomly generated one.
Fix backwards boolean check in FabricInfo::Reset that was causing us
to leak internally managed keys and try to delete externally managed
ones.
Change Darwin CHIPDeviceControllerStartupParams to allow providing
an operational keypair to be used for the NOC.
Change Darwin CHIPDeviceControllerStartupParams To allow providing a
NOC instead of having one generated inside the framework.
Refactor the code for initializing
CHIPDeviceControllerStartupParamsInternal to better share code and
support the new functionality.
Allow initializing CHIPOperationalCredentialsDelegate without a
NOC-signing keypair. This is needed because the SDK's controller
init requires a credentials delegate. When initialized in this way,
the delegate will just return error when asked to create a NOC.
Added tests for the new API (which caught a number of the issues
listed above).
Fixes #18444
Problem
See above.
Change overview
See above.
Testing
Tests in the PR.