[Darwin][AttestationVerifier] Expose a mechanism to customise cd signing keys and use it in darwin #22338
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
10de793 to
e10cc58
Compare
bzbarsky-apple
approved these changes
Sep 2, 2022
src/credentials/attestation_verifier/FileAttestationTrustStore.h
Outdated
Show resolved
Hide resolved
e10cc58 to
6f8c4b1
Compare
tcarmelveilleux
requested changes
Sep 6, 2022
There was a problem hiding this comment.
The CD trust store changes cause a model which forces using files to provide the official CDs in the default implementation. This is confusing and also overloads usage of AttestationTrustStore for 2 separate usages that are semantically different.
Provided proposed changes in vivien-apple#7 that enable the required CD files support while not overloading existing interfaces
…thod to allow controllers passing in some CD certs
…tionTrustStore::GetCertificationDeclarationCert
…initialize the test ArrayTrustStore store with the test CD cert
…ts for certification declaration verification
- Remove CD stuff from FileAttestationTrustStore - Refactor FileAttestationTrustStore to allow loading of any X.509 cert directory - Add a command line to chip-tool to disallow test keys (`only-allow-trusted-cd-keys`) - Add plumbing to enable CD keys lookup properly without mixing-up with PAA semantics - Add official CD verifying key and official SDK CD test key in the default CD trust store as-is
6f8c4b1 to
a9e5662
Compare
|
PR #22338: Size comparison from e535710 to a9e5662 Increases above 0.2%:
Increases (8 builds for bl602, cc13x2_26x2, linux, nrfconnect, qpg)
Decreases (13 builds for bl602, cc13x2_26x2, k32w, linux, nrfconnect, psoc6, telink)
Full report (45 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
tcarmelveilleux
approved these changes
Sep 6, 2022
|
PR #22338: Size comparison from e535710 to 62f8c70 Increases above 0.2%:
Increases (10 builds for cc13x2_26x2, esp32, linux, nrfconnect, psoc6, qpg)
Decreases (10 builds for cc13x2_26x2, k32w, linux, psoc6, qpg, telink)
Full report (45 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
emargolis
reviewed
Sep 7, 2022
| @@ -166,6 +165,9 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer | |||
| VerifyOrExit(info.attestationElementsBuffer.size() <= kMaxResponseLength, | |||
| attestationError = AttestationVerificationResult::kInvalidArgument); | |||
|
|
|||
| // Ensure PAI is present | |||
There was a problem hiding this comment.
If you are adding this check here for PAI then it also makes sense to add it for DAC and PAA. Regardless, the ExtractVIDPIDFromX509Cert() calls below should return errors in this case.
|
|
||
| CHIP_ERROR CsaCdKeysTrustStore::AddTrustedKey(const ByteSpan & derCertBytes) | ||
| { | ||
| // TODO: Verify cert against CD root of trust (i.e. verify signatures). To do so, |
There was a problem hiding this comment.
Interesting that we do the check here but not in the AddTrustedKey(const ByteSpan & kid, const Crypto::P256PublicKey & pubKey) method above. Is that a loophole to bypass security?
| * @returns CHIP_NO_ERROR on success, CHIP_INVALID_ARGUMENT if `kid` or `pubKey` arguments | ||
| * are not usable, CHIP_ERROR_KEY_NOT_FOUND if no key is found that matches `kid`. | ||
| */ | ||
| virtual CHIP_ERROR LookupVerifyingKey(const ByteSpan & kid, Crypto::P256PublicKey & outPubKey) const = 0; |
There was a problem hiding this comment.
Suggested change
| virtual CHIP_ERROR LookupVerifyingKey(const ByteSpan & kid, Crypto::P256PublicKey & outPubKey) const = 0; | |
| virtual CHIP_ERROR LookupCdVerifyingKey(const ByteSpan & kid, Crypto::P256PublicKey & outPubKey) const = 0; |
Otherwise, it is not very clear if the method is looking for the CD Signing Cert or CD Root of Trust.
| return CHIP_NO_ERROR; | ||
| } | ||
|
|
||
| CHIP_ERROR CsaCdKeysTrustStore::AddTrustedKey(const ByteSpan & derCertBytes) |
There was a problem hiding this comment.
Suggested change
| CHIP_ERROR CsaCdKeysTrustStore::AddTrustedKey(const ByteSpan & derCertBytes) | |
| CHIP_ERROR CsaCdKeysTrustStore::AddCdSigningKey(const ByteSpan & derCertBytes) |
Was thinking that it is clearer. Also, terms CdSigningKey and CdVerifyingKey are synonyms - maybe stick with only one of them.
| } | ||
|
|
||
| CHIP_ERROR CsaCdKeysTrustStore::AddTrustedKey(const ByteSpan & derCertBytes) | ||
| { |
There was a problem hiding this comment.
Where CD root of trust is stored? Is it one of the certificates in the PAA Trust Store?
isiu-apple
pushed a commit
to isiu-apple/connectedhomeip
that referenced
this pull request
Sep 16, 2022
…ing keys and use it in darwin (project-chip#22338) * Add AttestationTrustStore::GetCertificationDeclarationCert virtual method to allow controllers passing in some CD certs * Add cdCerts member to MTRControllerFactoryparams and override AttestationTrustStore::GetCertificationDeclarationCert * Implement ArrayTrustStore::GetCertificationDeclarationSigningKey and initialize the test ArrayTrustStore store with the test CD cert * Update the FileAttestationTrustStore to read a directory with der certs for certification declaration verification * Add credentials/development/cd-certs/ and update chip-tool to use it if desired * Update API to match conversation - Remove CD stuff from FileAttestationTrustStore - Refactor FileAttestationTrustStore to allow loading of any X.509 cert directory - Add a command line to chip-tool to disallow test keys (`only-allow-trusted-cd-keys`) - Add plumbing to enable CD keys lookup properly without mixing-up with PAA semantics - Add official CD verifying key and official SDK CD test key in the default CD trust store as-is * Update src/darwin to take into account the proposed changes * Add unit test for `CsaCdKeysTrustStore` Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
emargolis
pushed a commit
to emargolis/connectedhomeip-1
that referenced
this pull request
Oct 17, 2022
…ing keys and use it in darwin (project-chip#22338) * Add AttestationTrustStore::GetCertificationDeclarationCert virtual method to allow controllers passing in some CD certs * Add cdCerts member to MTRControllerFactoryparams and override AttestationTrustStore::GetCertificationDeclarationCert * Implement ArrayTrustStore::GetCertificationDeclarationSigningKey and initialize the test ArrayTrustStore store with the test CD cert * Update the FileAttestationTrustStore to read a directory with der certs for certification declaration verification * Add credentials/development/cd-certs/ and update chip-tool to use it if desired * Update API to match conversation - Remove CD stuff from FileAttestationTrustStore - Refactor FileAttestationTrustStore to allow loading of any X.509 cert directory - Add a command line to chip-tool to disallow test keys (`only-allow-trusted-cd-keys`) - Add plumbing to enable CD keys lookup properly without mixing-up with PAA semantics - Add official CD verifying key and official SDK CD test key in the default CD trust store as-is * Update src/darwin to take into account the proposed changes * Add unit test for `CsaCdKeysTrustStore` Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
andy31415
added a commit
that referenced
this pull request
Oct 20, 2022
…ntation (#23239) * Add missing pthread header (#22833) * [build] Fix #21255 - allow circular initialization of SimpleStateMachine test. (#22461) * [build] Fix #21255 - allow circular initialization of SimpleStateMachine test. * [build] Add comment per review feedback. * [Darwin][AttestationVerifier] Expose a mechanism to customise cd signing keys and use it in darwin (#22338) * Add AttestationTrustStore::GetCertificationDeclarationCert virtual method to allow controllers passing in some CD certs * Add cdCerts member to MTRControllerFactoryparams and override AttestationTrustStore::GetCertificationDeclarationCert * Implement ArrayTrustStore::GetCertificationDeclarationSigningKey and initialize the test ArrayTrustStore store with the test CD cert * Update the FileAttestationTrustStore to read a directory with der certs for certification declaration verification * Add credentials/development/cd-certs/ and update chip-tool to use it if desired * Update API to match conversation - Remove CD stuff from FileAttestationTrustStore - Refactor FileAttestationTrustStore to allow loading of any X.509 cert directory - Add a command line to chip-tool to disallow test keys (`only-allow-trusted-cd-keys`) - Add plumbing to enable CD keys lookup properly without mixing-up with PAA semantics - Add official CD verifying key and official SDK CD test key in the default CD trust store as-is * Update src/darwin to take into account the proposed changes * Add unit test for `CsaCdKeysTrustStore` Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com> * [Attestation] Updated to Use CD Signed by a Valid CSA Cert (#22685) * Updated CSA Official CD Signing Certificates (#23027) * restyled. * Remove fixed versioning for git in cirque (#23257) Co-authored-by: Gene Harvey <gene.harvey@smartthings.com> Co-authored-by: Martin Turon <mturon@google.com> Co-authored-by: Vivien Nicolas <vnicolas@apple.com> Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com> Co-authored-by: Andrei Litvin <andy314@gmail.com>
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
This PR implements some parts of "Certificate Declaration verification need to consume real signing keys (aka #21802)" by exposing a virtual method
AttestationTrustStore::GetCertificationDeclarationCertand updating the code so it uses this method if it is implemented or returns the default test certificate otherwise.In this PR this method is only implemented for the darwin controller, but it could be easily extended to the
ArrayAttestationTrustStoreand theFileAttestationTrustStore.Change overview
AttestationTrustStore::GetCertificationDeclarationCertand makes it returnCHIP_ERROR_NOT_IMPLEMENTEDfor some of the implementations.AttestationTrustStore::GetCertificationDeclarationCertin the darwin implementationTesting
It has been tested by manually toying with startup params in the
darwin-framework-toolcommand line tool.For example here is the diff for using the default test keys