Skip to content

Commit

Permalink
Allow end-to-end injection of DeviceAttestationVerifier (#18515)
Browse files Browse the repository at this point in the history
* Allow end-to-end injection of DeviceAttestationVerifier

- Injection exists in SetupParams, but it caused a singleton
  to be set in the end, which would not allow different
  DeviceAttestationVerifier per commissioner instance.

Fixes #18401
Issue #18445

- This PR passes the attestation verifier end-to-end and stops
  using `GetDeviceAttestationVerifier()` singleton call in
  commissioner. If no attestation verifier is injected, the one
  from `GetDeviceAttestationVerifier()` is used.
- Fix typos in the word "attestation"
- Make chip-tool use the new injection path to prove that it works,
  while chip-repl and others use previous method.

Testing done:
- Unit tests pass
- Cert tests pass

* Restyled by clang-format

* Fix lint

Co-authored-by: Restyled.io <commits@restyled.io>
  • Loading branch information
2 people authored and pull[bot] committed Oct 19, 2023
1 parent 3827139 commit 1283992
Show file tree
Hide file tree
Showing 7 changed files with 36 additions and 13 deletions.
1 change: 0 additions & 1 deletion examples/chip-tool/commands/common/CHIPCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,6 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f
chip::Controller::SetupParams commissionerParams;

ReturnLogErrorOnFailure(mCredIssuerCmds->SetupDeviceAttestation(commissionerParams, trustStore));
chip::Credentials::SetDeviceAttestationVerifier(commissionerParams.deviceAttestationVerifier);

VerifyOrReturnError(noc.Alloc(chip::Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY);
VerifyOrReturnError(icac.Alloc(chip::Controller::kMaxCHIPDERCertLength), CHIP_ERROR_NO_MEMORY);
Expand Down
32 changes: 25 additions & 7 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,6 +363,26 @@ CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
params.systemState->SessionMgr()->RegisterRecoveryDelegate(*this);

mPairingDelegate = params.pairingDelegate;

// Configure device attestation validation
mDeviceAttestationVerifier = params.deviceAttestationVerifier;
if (mDeviceAttestationVerifier == nullptr)
{
mDeviceAttestationVerifier = Credentials::GetDeviceAttestationVerifier();
if (mDeviceAttestationVerifier == nullptr)
{
ChipLogError(Controller,
"Missing DeviceAttestationVerifier configuration at DeviceCommissioner init and none set with "
"Credentials::SetDeviceAttestationVerifier()!");
return CHIP_ERROR_INVALID_ARGUMENT;
}

// We fell back on a default from singleton accessor.
ChipLogProgress(Controller,
"*** Missing DeviceAttestationVerifier configuration at DeviceCommissioner init: using global default, "
"consider passing one in CommissionerInitParams.");
}

if (params.defaultCommissioner != nullptr)
{
mDefaultCommissioner = params.defaultCommissioner;
Expand Down Expand Up @@ -1030,10 +1050,9 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device
{
MATTER_TRACE_EVENT_SCOPE("ValidateAttestationInfo", "DeviceCommissioner");
VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE);

DeviceAttestationVerifier * dac_verifier = GetDeviceAttestationVerifier();

dac_verifier->VerifyAttestationInformation(info, &mDeviceAttestationInformationVerificationCallback);
mDeviceAttestationVerifier->VerifyAttestationInformation(info, &mDeviceAttestationInformationVerificationCallback);

// TODO: Validate Firmware Information

Expand All @@ -1045,8 +1064,7 @@ CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan &
{
MATTER_TRACE_EVENT_SCOPE("ValidateCSR", "DeviceCommissioner");
VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);

DeviceAttestationVerifier * dacVerifier = GetDeviceAttestationVerifier();
VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE);

P256PublicKey dacPubkey;
ReturnErrorOnFailure(ExtractPubkeyFromX509Cert(dac, dacPubkey));
Expand All @@ -1056,8 +1074,8 @@ CHIP_ERROR DeviceCommissioner::ValidateCSR(DeviceProxy * proxy, const ByteSpan &
proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge();

// The operational CA should also verify this on its end during NOC generation, if end-to-end attestation is desired.
return dacVerifier->VerifyNodeOperationalCSRInformation(NOCSRElements, attestationChallenge, AttestationSignature, dacPubkey,
csrNonce);
return mDeviceAttestationVerifier->VerifyNodeOperationalCSRInformation(NOCSRElements, attestationChallenge,
AttestationSignature, dacPubkey, csrNonce);
}

CHIP_ERROR DeviceCommissioner::SendOperationalCertificateSigningRequestCommand(DeviceProxy * device, const ByteSpan & csrNonce)
Expand Down
5 changes: 5 additions & 0 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ struct CommissionerInitParams : public ControllerInitParams
{
DevicePairingDelegate * pairingDelegate = nullptr;
CommissioningDelegate * defaultCommissioner = nullptr;
// Device attestation verifier instance for the commissioning.
// If null, the globally set attestation verifier (e.g. from GetDeviceAttestationVerifier()
// singleton) will be used.
Credentials::DeviceAttestationVerifier * deviceAttestationVerifier = nullptr;
};

/**
Expand Down Expand Up @@ -803,6 +807,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
Platform::UniquePtr<app::ClusterStateCache> mAttributeCache;
Platform::UniquePtr<app::ReadClient> mReadClient;
Credentials::AttestationVerificationResult mAttestationResult;
Credentials::DeviceAttestationVerifier * mDeviceAttestationVerifier = nullptr;
};

} // namespace Controller
Expand Down
5 changes: 3 additions & 2 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -284,8 +284,9 @@ CHIP_ERROR DeviceControllerFactory::SetupCommissioner(SetupParams params, Device
PopulateInitParams(commissionerParams, params);

// Set commissioner-specific fields not in ControllerInitParams
commissionerParams.pairingDelegate = params.pairingDelegate;
commissionerParams.defaultCommissioner = params.defaultCommissioner;
commissionerParams.pairingDelegate = params.pairingDelegate;
commissionerParams.defaultCommissioner = params.defaultCommissioner;
commissionerParams.deviceAttestationVerifier = params.deviceAttestationVerifier;

CHIP_ERROR err = commissioner.Init(commissionerParams);
return err;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class DeviceAttestationDelegate

/**
* @brief
* If valid, value to set for the fail-safe timer before the delegate's OnDeviceAttestionFailed is invoked.
* If valid, value to set for the fail-safe timer before the delegate's OnDeviceAttestationFailed is invoked.
*
* @return Optional value for the fail-safe timer in seconds.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1112,7 +1112,7 @@ - (void)deviceAttestation:(CHIPDeviceController *)controller failedForDevice:(vo
dispatch_async(dispatch_get_main_queue(), ^{
UIAlertController * alertController = [UIAlertController
alertControllerWithTitle:@"Device Attestation"
message:@"Device Attestion failed for device under commissioning. Do you wish to continue pairing?"
message:@"Device Attestation failed for device under commissioning. Do you wish to continue pairing?"
preferredStyle:UIAlertControllerStyleAlert];

[alertController addAction:[UIAlertAction actionWithTitle:@"No"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
chip::DeviceProxy * device, chip::Credentials::AttestationVerificationResult attestationResult)
{
dispatch_async(mQueue, ^{
NSLog(@"CHIPDeviceAttestationDelegateBridge::OnDeviceAttestionFailed failed with result: %hu", attestationResult);
NSLog(@"CHIPDeviceAttestationDelegateBridge::OnDeviceAttestationFailed failed with result: %hu", attestationResult);

mResult = attestationResult;

Expand Down

0 comments on commit 1283992

Please sign in to comment.