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

Cannot provide externally obtained NOC and externally managed key pair to Darwin framework on controller init #18444

Closed
tcarmelveilleux opened this issue May 13, 2022 · 1 comment · Fixed by #18519
Closed

Cannot provide externally obtained NOC and externally managed key pair to Darwin framework on controller init #18444

tcarmelveilleux opened this issue May 13, 2022 · 1 comment · Fixed by #18519
Assignees
@bzbarsky-apple
Labels
darwin V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

When managing your own fabric for multi-fabric use-cases on iOS, there is currently no API that allows providing an NOC for a controller, and associated key pair protocol implementation.

This has the following side-effects:

  • Clients have to provide the root CA key pair (including private key) and let the framework decide of NOC issuance for the local controller, which is impossible if the key pair resides outside the app calling the framework (e.g. a cloud-based CA).
  • There is no way to provide a Keychain-based or other secured non-native version of the implementation of the operational key used during CASE, to safeguard access to the private key bits, which increases attack surface to operational keys that may underpins administrator controller applications in a fabric.

Proposed Solution

  • Allow providing an optional already-generated NOC and associated keypair if the issuance of controller NOCs is delegated to outside the framework
  • Allow the P256Keypair* in the fabric table to be backed by a bridge to externally provided crypto implementation with the following minimal API which suffices for both initiator and responder roles in CASE:
    • GetPublicKey()
    • SignMessage()
    • ECDHDeriveSecret()
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 16, 2022
@tcarmelveilleux
Allow external ownership of CHIPDeviceController operational key
ef8a02e 
- For controllers that make use of their own key management,
  such as OS-aided key management, it is currently impossible
  to pass-in the Operational Key pair to use for a given controller.

This PR adds APIs to avoid FabricTable from trying to manage
the lifecycle of the operational key, and allow the caller/initializer
of CHIPDeviceControllerFactory to properly inject its own operational
key that it manages, without attempts of storage or dynamic memory
management.

Issue project-chip#18444
Issue project-chip#7695

Testing done:
- Unit tests still pass, cert tests as well
- This was tested internally by us using the APIs to
  manage a key for Android. That change will be a follow-up
@tcarmelveilleux tcarmelveilleux mentioned this issue May 16, 2022
Merged
@bzbarsky-apple bzbarsky-apple self-assigned this May 16, 2022
@bzbarsky-apple
Copy link
Contributor

ECDHDeriveSecret()

As discussed today, this is not needed, because secrets are derived from ephemeral keys during CASE; the operational key is only used to sign things.

The rest can be done once #18482 merges.

tcarmelveilleux added a commit that referenced this issue May 17, 2022
@tcarmelveilleux @restyled-commits @bzbarsky-apple
Allow external ownership of CHIPDeviceController operational key (#18482
51cbb53 
)

* Allow external ownership of CHIPDeviceController operational key

- For controllers that make use of their own key management,
  such as OS-aided key management, it is currently impossible
  to pass-in the Operational Key pair to use for a given controller.

This PR adds APIs to avoid FabricTable from trying to manage
the lifecycle of the operational key, and allow the caller/initializer
of CHIPDeviceControllerFactory to properly inject its own operational
key that it manages, without attempts of storage or dynamic memory
management.

Issue #18444
Issue #7695

Testing done:
- Unit tests still pass, cert tests as well
- This was tested internally by us using the APIs to
  manage a key for Android. That change will be a follow-up

* Restyled by clang-format

* Apply suggestions from code review

Co-authored-by: Boris Zbarsky <[email protected]>

* Address review comments

* Restyled by clang-format

* Address review comments

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue May 17, 2022
@bzbarsky-apple
Allow Darwin framework consumers to provide a controller NOC and keyp…
bb2e6ef 
…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
@bzbarsky-apple bzbarsky-apple mentioned this issue May 17, 2022
Merged
andy31415 pushed a commit that referenced this issue May 18, 2022
@bzbarsky-apple
Allow Darwin framework consumers to provide a controller NOC and keyp…
5c70127 
…air (#18519)

* Allow Darwin framework consumers to provide a controller NOC and keypair.

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

* Address review comments.
tcarmelveilleux pushed a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 19, 2022
@tennessee-google
Implement CSR generation from first principles to support commissioners
5a81a48 
- 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 tcarmelveilleux mentioned this issue May 19, 2022
Merged
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 19, 2022
@tcarmelveilleux
Implement CSR generation from first principles to support commissioners
5995c2c 
- 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 issue May 20, 2022
@tcarmelveilleux @bzbarsky-apple @restyled-commits
Implement CSR generation from first principles to support commissione…
f7a321d 
…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
Assignees

@bzbarsky-apple bzbarsky-apple

Labels
darwin V1.0
Projects
None yet
Milestone
No milestone
2 participants
@tcarmelveilleux @bzbarsky-apple