From d5d0fe2ee6c18a261a6d6ea34848eff562183e73 Mon Sep 17 00:00:00 2001 From: chrisdecenzo Date: Tue, 16 Aug 2022 07:07:05 -0700 Subject: [PATCH 1/5] Feedback from Android commissioning fixes --- src/controller/CHIPDeviceController.h | 9 +++++++ .../AndroidOperationalCredentialsIssuer.cpp | 1 - .../java/CHIPDeviceController-JNI.cpp | 15 ++++++++++- .../ChipDeviceController.java | 26 +++++++++++++++---- .../DacOnlyPartialAttestationVerifier.cpp | 6 ++++- .../DacOnlyPartialAttestationVerifier.h | 26 +++++++++++++++++++ 6 files changed, 75 insertions(+), 8 deletions(-) diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 78581dca42c657..53371ef3894a78 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -561,6 +561,15 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, */ CHIP_ERROR NetworkCredentialsReady(); + /** + * @brief + * This function returns the current CommissioningStage for this commissioner. + */ + CommissioningStage GetCommissioningStage() + { + return mCommissioningStage; + } + #if CONFIG_NETWORK_LAYER_BLE #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE /** diff --git a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp index 249921ae17c923..59872b01486b52 100644 --- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp +++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp @@ -205,7 +205,6 @@ CHIP_ERROR AndroidOperationalCredentialsIssuer::CallbackGenerateNOCChain(const B P256PublicKey pubkey; ReturnErrorOnFailure(VerifyCertificateSigningRequest(csr.data(), csr.size(), pubkey)); - // TODO: verify signed by DAC creds? ChipLogProgress(chipTool, "VerifyCertificateSigningRequest"); jobject csrInfo; diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp index 2020eb27e45d8a..2f2ccb58cd9d5a 100644 --- a/src/controller/java/CHIPDeviceController-JNI.cpp +++ b/src/controller/java/CHIPDeviceController-JNI.cpp @@ -524,7 +524,9 @@ JNI_METHOD(void, setUseJavaCallbackForNOCRequest) if (useCallback) { - // if we are assigning a callback, then make the device commissioner delegate verification to the cloud + // if we are assigning a callback, then make the device commissioner delegate verification to the + // PartialDACVerifier so that DAC chain and CD validation can be performed by custom code + // triggered by ChipDeviceController.NOCChainIssuer.onNOCChainGenerationNeeded(). wrapper->Controller()->SetDeviceAttestationVerifier(wrapper->GetPartialDACVerifier()); } else @@ -554,6 +556,17 @@ JNI_METHOD(void, updateCommissioningNetworkCredentials) ChipLogError(Controller, "UpdateCommissioningParameters failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); } + + // Only invoke NetworkCredentialsReady when called in response to NetworkScan result + if (wrapper->Controller()->GetCommissioningStage() == CommissioningStage::kNeedsNetworkCreds) + { + err = wrapper->Controller()->NetworkCredentialsReady(); + if (err != CHIP_NO_ERROR) + { + ChipLogError(Controller, "NetworkCredentialsReady failed. Err = %" CHIP_ERROR_FORMAT, err.Format()); + JniReferences::GetInstance().ThrowError(env, sChipDeviceControllerExceptionCls, err); + } + } } JNI_METHOD(jbyteArray, convertX509CertToMatterCert) diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index 541f83e40aba99..e50b3466829f3c 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -63,10 +63,18 @@ public void setScanNetworksListener(ScanNetworksListener listener) { } /** - * Sets this DeviceController to use the given issuer for issuing operational certs. By default, - * the DeviceController uses an internal, OperationalCredentialsDelegate (see - * AndroidOperationalCredentialsIssuer) + * Sets this DeviceController to use the given issuer for issuing operational certs and + * verifying the DAC. By default, the DeviceController uses an internal, OperationalCredentialsDelegate + * (see AndroidOperationalCredentialsIssuer). * + * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be + * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials + * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform + * DAC verification and the CSR signing. + * + *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used + * rather than the DefaultDACVerifier. + * * @param issuer */ public void setNOCChainIssuer(NOCChainIssuer issuer) { @@ -692,8 +700,12 @@ protected void finalize() throws Throwable { public interface NOCChainIssuer { /** * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be - * called when the NOC CSR needs to be signed. This allows for custom credentials issuer - * implementations, for example, when a proprietary cloud API will perform the CSR signing. + * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials + * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform + * DAC verification and the CSR signing. + * + *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used + * rather than the DefaultDACVerifier. * *

The commissioning workflow will stop upon the onNOCChainGenerationNeeded callback and * resume once onNOCChainGeneration is called. @@ -716,6 +728,10 @@ public interface NOCChainIssuer { *

Set the AttemptNetworkScanWiFi or AttemptNetworkScanThread to configure the enable/disable * WiFi or Thread network scan during commissioning in the the default CommissioningDelegate used * by the ChipDeviceCommissioner. + * + * When the callbacks onScanNetworksFailure or onScanNetworksSuccess are invoked, the commissioning + * flow has reached the kNeedsNetworkCreds and will wait to advance until this device controller's + * updateCommissioningNetworkCredentials method is called with the desired network credentials set. */ public interface ScanNetworksListener { /** Notifies when scan networks call fails. */ diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp index e37155886d61ce..4dd6ab7d35a8f2 100644 --- a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp +++ b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp @@ -36,6 +36,10 @@ namespace Credentials { // As per specifications section 11.22.5.1. Constant RESP_MAX constexpr size_t kMaxResponseLength = 900; +/** + * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks + * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h. + */ void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, Callback::Callback * onCompletion) { @@ -144,7 +148,7 @@ void PartialDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer } exit: - onCompletion->mCall(onCompletion->mContext, attestationError); // TODO: is this check getting done? + onCompletion->mCall(onCompletion->mContext, attestationError); } } // namespace Credentials diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h index acd28f66d71054..4705a5a5128d1f 100644 --- a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h @@ -21,11 +21,37 @@ namespace chip { namespace Credentials { +/** + * @brief + * This class is based upon the DefaultDACVerifier but has all checks removed which require + * local availability of public keys that are not available from the commissionee, such as the + * PAA public key and the CSA keys used to sign the Certification Declaration (CD). + * + * This class should only be used in conjunction with an OperationalCredentialsDelegate + * which performs the removed checks. For example, an OperationalCredentialsDelegate implementation + * might send the DAC chain and signed CD to custom code which obtains these keys from the DCL. + * + * Specifically, the following list of checks have been removed: + * (1) Make sure the PAA is valid and approved by CSA. + * (2) vid-scoped PAA check: if the PAA is vid scoped, then its vid must match the DAC vid. + * (3) cert chain check: verify PAI is signed by PAA, and DAC is signed by PAI. + * (4) PAA subject key id extraction: the PAA subject key must match the PAA key referenced in the PAI. + * (5) CD signature check: make sure a valid CSA CD key is used to sign the CD. + * + * Any other checks performed by the DefaultDACVerifier should be performed here too. Changes + * made to DefaultDACVerifier::VerifyAttestationInformation should be made to + * PartialDACVerifier::VerifyAttestationInformation. + */ class PartialDACVerifier : public DefaultDACVerifier { public: PartialDACVerifier() {} + /** + * @brief + * The implementation should track DefaultDACVerifier::VerifyAttestationInformation but with the checks + * disabled that are outlined at the top of DacOnlyPartialAttestationVerifier.h. + */ void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info, Callback::Callback * onCompletion) override; From 5e20aa6e6f5e8ffd8eecba482bc3991dd25de2a4 Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Tue, 16 Aug 2022 11:50:57 -0700 Subject: [PATCH 2/5] Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java Co-authored-by: Tennessee Carmel-Veilleux --- .../java/src/chip/devicecontroller/ChipDeviceController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index e50b3466829f3c..68eb51cba658a9 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -700,7 +700,7 @@ protected void finalize() throws Throwable { public interface NOCChainIssuer { /** * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be - * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials + * called when the DAC chain must be verified and NOC chain needs to be issued from a CSR. This allows for custom credentials * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform * DAC verification and the CSR signing. * From ba2bf3c15d9ceed3c765e416992f584248f65fae Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Tue, 16 Aug 2022 11:51:04 -0700 Subject: [PATCH 3/5] Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java Co-authored-by: Tennessee Carmel-Veilleux --- .../java/src/chip/devicecontroller/ChipDeviceController.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index 68eb51cba658a9..c08182e038d560 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -702,7 +702,7 @@ public interface NOCChainIssuer { * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be * called when the DAC chain must be verified and NOC chain needs to be issued from a CSR. This allows for custom credentials * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform - * DAC verification and the CSR signing. + * DAC verification and the NOC chain issuance from CSR. * *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used * rather than the DefaultDACVerifier. From fc3482d7fc2b209ae7e3a9e45076c48cc46eda65 Mon Sep 17 00:00:00 2001 From: chrisdecenzo <61757564+chrisdecenzo@users.noreply.github.com> Date: Tue, 16 Aug 2022 11:51:11 -0700 Subject: [PATCH 4/5] Update src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h Co-authored-by: Tennessee Carmel-Veilleux --- .../attestation_verifier/DacOnlyPartialAttestationVerifier.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h index 4705a5a5128d1f..a987c7c9779ce7 100644 --- a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h +++ b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h @@ -24,8 +24,8 @@ namespace Credentials { /** * @brief * This class is based upon the DefaultDACVerifier but has all checks removed which require - * local availability of public keys that are not available from the commissionee, such as the - * PAA public key and the CSA keys used to sign the Certification Declaration (CD). + * local availability of trust anchors that are not available from the commissionee, such as the + * PAA root certificates and the CSA keys used to sign the Certification Declaration (CD). * * This class should only be used in conjunction with an OperationalCredentialsDelegate * which performs the removed checks. For example, an OperationalCredentialsDelegate implementation From 40484678d4e88dd0c6e09bb5fc1c5bb9fed86b23 Mon Sep 17 00:00:00 2001 From: "restyled-io[bot]" <32688539+restyled-io[bot]@users.noreply.github.com> Date: Tue, 16 Aug 2022 11:53:19 -0700 Subject: [PATCH 5/5] Restyle Feedback from Android commissioning fixes (#21928) * Restyled by whitespace * Restyled by google-java-format * Restyled by clang-format Co-authored-by: Restyled.io --- src/controller/CHIPDeviceController.h | 5 +-- .../ChipDeviceController.java | 37 ++++++++++--------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h index 53371ef3894a78..f5a4ddf281618a 100644 --- a/src/controller/CHIPDeviceController.h +++ b/src/controller/CHIPDeviceController.h @@ -565,10 +565,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController, * @brief * This function returns the current CommissioningStage for this commissioner. */ - CommissioningStage GetCommissioningStage() - { - return mCommissioningStage; - } + CommissioningStage GetCommissioningStage() { return mCommissioningStage; } #if CONFIG_NETWORK_LAYER_BLE #if CHIP_DEVICE_CONFIG_ENABLE_BOTH_COMMISSIONER_AND_COMMISSIONEE diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java index c08182e038d560..97627b56c4ce1f 100644 --- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java +++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java @@ -63,18 +63,18 @@ public void setScanNetworksListener(ScanNetworksListener listener) { } /** - * Sets this DeviceController to use the given issuer for issuing operational certs and - * verifying the DAC. By default, the DeviceController uses an internal, OperationalCredentialsDelegate - * (see AndroidOperationalCredentialsIssuer). + * Sets this DeviceController to use the given issuer for issuing operational certs and verifying + * the DAC. By default, the DeviceController uses an internal, OperationalCredentialsDelegate (see + * AndroidOperationalCredentialsIssuer). * - * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be - * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials - * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform + *

When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be + * called when the NOC CSR needs to be signed and DAC verified. This allows for custom credentials + * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform * DAC verification and the CSR signing. - * - *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used - * rather than the DefaultDACVerifier. - * + * + *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used rather + * than the DefaultDACVerifier. + * * @param issuer */ public void setNOCChainIssuer(NOCChainIssuer issuer) { @@ -700,10 +700,10 @@ protected void finalize() throws Throwable { public interface NOCChainIssuer { /** * When a NOCChainIssuer is set for this controller, then onNOCChainGenerationNeeded will be - * called when the DAC chain must be verified and NOC chain needs to be issued from a CSR. This allows for custom credentials - * issuer and DAC verifier implementations, for example, when a proprietary cloud API will perform - * DAC verification and the NOC chain issuance from CSR. - * + * called when the DAC chain must be verified and NOC chain needs to be issued from a CSR. This + * allows for custom credentials issuer and DAC verifier implementations, for example, when a + * proprietary cloud API will perform DAC verification and the NOC chain issuance from CSR. + * *

When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used * rather than the DefaultDACVerifier. * @@ -728,10 +728,11 @@ public interface NOCChainIssuer { *

Set the AttemptNetworkScanWiFi or AttemptNetworkScanThread to configure the enable/disable * WiFi or Thread network scan during commissioning in the the default CommissioningDelegate used * by the ChipDeviceCommissioner. - * - * When the callbacks onScanNetworksFailure or onScanNetworksSuccess are invoked, the commissioning - * flow has reached the kNeedsNetworkCreds and will wait to advance until this device controller's - * updateCommissioningNetworkCredentials method is called with the desired network credentials set. + * + *

When the callbacks onScanNetworksFailure or onScanNetworksSuccess are invoked, the + * commissioning flow has reached the kNeedsNetworkCreds and will wait to advance until this + * device controller's updateCommissioningNetworkCredentials method is called with the desired + * network credentials set. */ public interface ScanNetworksListener { /** Notifies when scan networks call fails. */