Implement CSR generation from first principles to support commissioners #18631
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
tcarmelveilleux
added
crypto
security
commissioning
|
|
- 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
force-pushed
the
first-principles-csr
branch
from
7dabd88
to
5995c2c
Compare
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
harimau-qirex,
hawk248,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs and
jmartinez-silabs
pullapprove
bot
requested review from
rgoliver,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple and
xylophone21
andy31415
approved these changes
May 19, 2022
Co-authored-by: Boris Zbarsky <[email protected]>
msandstedt
approved these changes
May 19, 2022
|
PR #18631: Size comparison from 3ef81a8 to 7ff899a Increases (34 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, p6)
Decreases (9 builds for cc13x2_26x2, esp32, linux)
Full report (34 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, p6)
|
balducci-apple
approved these changes
May 19, 2022
|
PR #18631: Size comparison from 3ef81a8 to 463ccfc Increases above 0.2%:
Increases (34 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (19 builds for cc13x2_26x2, esp32, linux, telink)
Full report (37 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
emargolis
approved these changes
May 19, 2022
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
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.
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.
is no easy way to write code interfacing with an external CA to
provide a CSR for a natively bridged keypair.
Issue #18444
Change overview
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.
Testing
both mbedTLS and OpenSSL
users.