Feedback from Android commissioning fixes (#21927)
* Feedback from Android commissioning fixes
* Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
* Update src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
* Update src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.h
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
* Restyle Feedback from Android commissioning fixes (#21928)
* Restyled by whitespace
* Restyled by google-java-format
* Restyled by clang-format
Co-authored-by: Restyled.io <commits@restyled.io>
Co-authored-by: Tennessee Carmel-Veilleux <tennessee.carmelveilleux@gmail.com>
Co-authored-by: restyled-io[bot] <32688539+restyled-io[bot]@users.noreply.github.com>
Co-authored-by: Restyled.io <commits@restyled.io>
diff --git a/src/controller/CHIPDeviceController.h b/src/controller/CHIPDeviceController.h
index 78581dc..f5a4ddf 100644
--- a/src/controller/CHIPDeviceController.h
+++ b/src/controller/CHIPDeviceController.h
@@ -561,6 +561,12 @@
*/
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 249921a..59872b0 100644
--- a/src/controller/java/AndroidOperationalCredentialsIssuer.cpp
+++ b/src/controller/java/AndroidOperationalCredentialsIssuer.cpp
@@ -205,7 +205,6 @@
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 2020eb2..2f2ccb5 100644
--- a/src/controller/java/CHIPDeviceController-JNI.cpp
+++ b/src/controller/java/CHIPDeviceController-JNI.cpp
@@ -524,7 +524,9 @@
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 @@
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 541f83e..97627b5 100644
--- a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
+++ b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
@@ -63,9 +63,17 @@
}
/**
- * 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).
+ *
+ * <p>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.
+ *
+ * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used rather
+ * than the DefaultDACVerifier.
*
* @param issuer
*/
@@ -692,8 +700,12 @@
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 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.
+ *
+ * <p>When a NOCChainIssuer is set for this controller, the PartialDACVerifier will be used
+ * rather than the DefaultDACVerifier.
*
* <p>The commissioning workflow will stop upon the onNOCChainGenerationNeeded callback and
* resume once onNOCChainGeneration is called.
@@ -716,6 +728,11 @@
* <p>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.
+ *
+ * <p>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 e371558..4dd6ab7 100644
--- a/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp
+++ b/src/credentials/attestation_verifier/DacOnlyPartialAttestationVerifier.cpp
@@ -36,6 +36,10 @@
// 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<OnAttestationInformationVerification> * onCompletion)
{
@@ -144,7 +148,7 @@
}
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 acd28f6..a987c7c 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 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
+ * 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<OnAttestationInformationVerification> * onCompletion) override;
