Skip to content

Commit 1ad61c6

Browse files
vijsvijs
committed
Implemented PR #31222 review comments
1 parent 11838c6 commit 1ad61c6

9 files changed

+90
-89
lines changed

‎src/controller/CHIPDeviceController.cpp

+50-74
Original file line numberDiff line numberDiff line change
@@ -392,8 +392,8 @@ DeviceCommissioner::DeviceCommissioner() :
392392
mOnDeviceConnectionRetryCallback(OnDeviceConnectionRetryFn, this),
393393
#endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
394394
mDeviceAttestationInformationVerificationCallback(OnDeviceAttestationInformationVerification, this),
395-
mDACChainRevocationStatusVerificationCallback(OnDACChainRevocationStatusVerification, this),
396-
mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this)
395+
mDACChainRevocationStatusCallback(OnDACChainRevocationStatus, this), mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this),
396+
mSetUpCodePairer(this)
397397
{}
398398

399399
CHIP_ERROR DeviceCommissioner::Init(CommissionerInitParams params)
@@ -1107,6 +1107,17 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
11071107
MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
11081108
DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
11091109

1110+
if (commissioner->attestationInformationVerificationResult == AttestationVerificationResult::kNotImplemented)
1111+
{
1112+
commissioner->attestationInformationVerificationResult = result;
1113+
}
1114+
1115+
VerifyOrReturn(commissioner->dacChainRevocationStatusResult != AttestationVerificationResult::kNotImplemented);
1116+
1117+
result = commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kSuccess
1118+
? commissioner->attestationInformationVerificationResult
1119+
: commissioner->dacChainRevocationStatusResult;
1120+
11101121
if (!commissioner->mDeviceBeingCommissioned)
11111122
{
11121123
ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device");
@@ -1156,64 +1167,25 @@ void DeviceCommissioner::OnDeviceAttestationInformationVerification(
11561167
}
11571168
}
11581169

1159-
void DeviceCommissioner::OnDACChainRevocationStatusVerification(
1160-
void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info, AttestationVerificationResult result)
1170+
void DeviceCommissioner::OnDACChainRevocationStatus(void * context,
1171+
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
1172+
AttestationVerificationResult result)
11611173
{
1162-
MATTER_TRACE_SCOPE("OnDACChainRevocationStatusVerification", "DeviceCommissioner");
1174+
MATTER_TRACE_SCOPE("OnDeviceAttestationInformationVerification", "DeviceCommissioner");
11631175
DeviceCommissioner * commissioner = reinterpret_cast<DeviceCommissioner *>(context);
11641176

1165-
if (!commissioner->mDeviceBeingCommissioned)
1177+
if (commissioner->dacChainRevocationStatusResult == AttestationVerificationResult::kNotImplemented)
11661178
{
1167-
ChipLogError(Controller, "Device attestation verification result received when we're not commissioning a device");
1168-
return;
1179+
commissioner->dacChainRevocationStatusResult = result;
11691180
}
11701181

1171-
auto & params = commissioner->mDefaultCommissioner->GetCommissioningParameters();
1172-
Credentials::DeviceAttestationDelegate * deviceAttestationDelegate = params.GetDeviceAttestationDelegate();
1182+
VerifyOrReturn(commissioner->attestationInformationVerificationResult != AttestationVerificationResult::kNotImplemented);
11731183

1174-
if (result != AttestationVerificationResult::kSuccess)
1175-
{
1176-
CommissioningDelegate::CommissioningReport report;
1177-
report.Set<AttestationErrorInfo>(result);
1178-
if (result == AttestationVerificationResult::kNotImplemented)
1179-
{
1180-
ChipLogError(Controller,
1181-
"Failed in verifying 'DAC Chain Revocation Status' command received from the device due to default "
1182-
"DeviceAttestationVerifier Class not being overridden by a real implementation.");
1183-
commissioner->CommissioningStageComplete(CHIP_ERROR_NOT_IMPLEMENTED, report);
1184-
return;
1185-
}
1186-
1187-
ChipLogError(Controller,
1188-
"Failed in verifying 'DAC Chain Revocation Status' command received from the device: err %hu. Look at "
1189-
"AttestationVerificationResult enum to understand the errors",
1190-
static_cast<uint16_t>(result));
1191-
// Go look at AttestationVerificationResult enum in src/credentials/attestation_verifier/DeviceAttestationVerifier.h to
1192-
// understand the errors.
1193-
1194-
// If a device attestation status delegate is installed, delegate handling of failure to the client and let them decide on
1195-
// whether to proceed further or not.
1196-
if (deviceAttestationDelegate)
1197-
{
1198-
commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
1199-
}
1200-
else
1201-
{
1202-
commissioner->CommissioningStageComplete(CHIP_ERROR_INTERNAL, report);
1203-
}
1204-
}
1205-
else
1206-
{
1207-
if (deviceAttestationDelegate && deviceAttestationDelegate->ShouldWaitAfterDeviceAttestation())
1208-
{
1209-
commissioner->ExtendArmFailSafeForDeviceAttestation(info, result);
1210-
}
1211-
else
1212-
{
1213-
ChipLogProgress(Controller, "Successfully validated 'DAC Chain Revocation Status' command received from the device.");
1214-
commissioner->CommissioningStageComplete(CHIP_NO_ERROR);
1215-
}
1216-
}
1184+
OnDeviceAttestationInformationVerification(context, info,
1185+
commissioner->dacChainRevocationStatusResult !=
1186+
AttestationVerificationResult::kSuccess
1187+
? commissioner->dacChainRevocationStatusResult
1188+
: result);
12171189
}
12181190

12191191
void DeviceCommissioner::OnArmFailSafeExtendedForDeviceAttestation(
@@ -1363,13 +1335,13 @@ CHIP_ERROR DeviceCommissioner::ValidateAttestationInfo(const Credentials::Device
13631335
}
13641336

13651337
CHIP_ERROR
1366-
DeviceCommissioner::ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info)
1338+
DeviceCommissioner::CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info)
13671339
{
1368-
MATTER_TRACE_SCOPE("ValidateDACChainRevocationStatus", "DeviceCommissioner");
1340+
MATTER_TRACE_SCOPE("CheckForRevokedDACChain", "DeviceCommissioner");
13691341
VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
13701342
VerifyOrReturnError(mDeviceAttestationVerifier != nullptr, CHIP_ERROR_INCORRECT_STATE);
13711343

1372-
mDeviceAttestationVerifier->ValidateDACChainRevocationStatus(info, &mDACChainRevocationStatusVerificationCallback);
1344+
mDeviceAttestationVerifier->CheckForRevokedDACChain(info, &mDACChainRevocationStatusCallback);
13731345

13741346
return CHIP_NO_ERROR;
13751347
}
@@ -2971,14 +2943,11 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
29712943
}
29722944
case CommissioningStage::kAttestationVerification: {
29732945
ChipLogProgress(Controller, "Verifying attestation");
2974-
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
2975-
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
2976-
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
2977-
{
2978-
ChipLogError(Controller, "Missing attestation information");
2979-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
2980-
return;
2981-
}
2946+
VerifyOrReturn(IsAttestationInformationMissing(params) == false);
2947+
2948+
// Reset results before verifying
2949+
attestationInformationVerificationResult = AttestationVerificationResult::kNotImplemented;
2950+
dacChainRevocationStatusResult = AttestationVerificationResult::kNotImplemented;
29822951

29832952
DeviceAttestationVerifier::AttestationInfo info(
29842953
params.GetAttestationElements().Value(),
@@ -2996,25 +2965,18 @@ void DeviceCommissioner::PerformCommissioningStep(DeviceProxy * proxy, Commissio
29962965
break;
29972966
case CommissioningStage::kAttestationRevocationCheck: {
29982967
ChipLogProgress(Controller, "Verifying device's DAC chain revocation status");
2999-
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
3000-
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
3001-
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
3002-
{
3003-
ChipLogError(Controller, "Missing attestation certificates");
3004-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
3005-
return;
3006-
}
2968+
VerifyOrReturn(IsAttestationInformationMissing(params) == false);
30072969

30082970
DeviceAttestationVerifier::AttestationInfo info(
30092971
params.GetAttestationElements().Value(),
30102972
proxy->GetSecureSession().Value()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge(),
30112973
params.GetAttestationSignature().Value(), params.GetPAI().Value(), params.GetDAC().Value(),
30122974
params.GetAttestationNonce().Value(), params.GetRemoteVendorId().Value(), params.GetRemoteProductId().Value());
30132975

3014-
if (ValidateDACChainRevocationStatus(info) != CHIP_NO_ERROR)
2976+
if (CheckForRevokedDACChain(info) != CHIP_NO_ERROR)
30152977
{
30162978
ChipLogError(Controller, "Error validating device's DAC chain revocation status");
3017-
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
2979+
CommissioningStageComplete(CHIP_ERROR_FAILED_DEVICE_ATTESTATION);
30182980
return;
30192981
}
30202982
}
@@ -3359,6 +3321,20 @@ void DeviceCommissioner::ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device,
33593321
}
33603322
}
33613323

3324+
bool DeviceCommissioner::IsAttestationInformationMissing(CommissioningParameters & params)
3325+
{
3326+
if (!params.GetAttestationElements().HasValue() || !params.GetAttestationSignature().HasValue() ||
3327+
!params.GetAttestationNonce().HasValue() || !params.GetDAC().HasValue() || !params.GetPAI().HasValue() ||
3328+
!params.GetRemoteVendorId().HasValue() || !params.GetRemoteProductId().HasValue())
3329+
{
3330+
ChipLogError(Controller, "Missing attestation information");
3331+
CommissioningStageComplete(CHIP_ERROR_INVALID_ARGUMENT);
3332+
return true;
3333+
}
3334+
3335+
return false;
3336+
}
3337+
33623338
CHIP_ERROR DeviceController::GetCompressedFabricIdBytes(MutableByteSpan & outBytes) const
33633339
{
33643340
const auto * fabricInfo = GetFabricInfo();

‎src/controller/CHIPDeviceController.h

+11-5
Original file line numberDiff line numberDiff line change
@@ -610,7 +610,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
610610
*
611611
* @param[in] info Structure contatining all the required information for validating the device attestation.
612612
*/
613-
CHIP_ERROR ValidateDACChainRevocationStatus(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
613+
CHIP_ERROR CheckForRevokedDACChain(const Credentials::DeviceAttestationVerifier::AttestationInfo & info);
614614

615615
/**
616616
* @brief
@@ -792,6 +792,11 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
792792

793793
ObjectPool<CommissioneeDeviceProxy, kNumMaxActiveDevices> mCommissioneeDevicePool;
794794

795+
Credentials::AttestationVerificationResult attestationInformationVerificationResult =
796+
Credentials::AttestationVerificationResult::kNotImplemented;
797+
Credentials::AttestationVerificationResult dacChainRevocationStatusResult =
798+
Credentials::AttestationVerificationResult::kNotImplemented;
799+
795800
#if CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY // make this commissioner discoverable
796801
UserDirectedCommissioningServer * mUdcServer = nullptr;
797802
// mUdcTransportMgr is for insecure communication (ex. user directed commissioning)
@@ -892,9 +897,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
892897
static void OnDeviceAttestationInformationVerification(void * context,
893898
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
894899
Credentials::AttestationVerificationResult result);
895-
static void OnDACChainRevocationStatusVerification(void * context,
896-
const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
897-
Credentials::AttestationVerificationResult result);
900+
static void OnDACChainRevocationStatus(void * context, const Credentials::DeviceAttestationVerifier::AttestationInfo & info,
901+
Credentials::AttestationVerificationResult result);
898902

899903
static void OnDeviceNOCChainGeneration(void * context, CHIP_ERROR status, const ByteSpan & noc, const ByteSpan & icac,
900904
const ByteSpan & rcac, Optional<IdentityProtectionKeySpan> ipk,
@@ -1020,6 +1024,8 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
10201024
// extend it).
10211025
void ExtendFailsafeBeforeNetworkEnable(DeviceProxy * device, CommissioningParameters & params, CommissioningStage step);
10221026

1027+
bool IsAttestationInformationMissing(CommissioningParameters & params);
1028+
10231029
chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
10241030
chip::Callback::Callback<OnDeviceConnectionFailure> mOnDeviceConnectionFailureCallback;
10251031
#if CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
@@ -1029,7 +1035,7 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
10291035
chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
10301036
mDeviceAttestationInformationVerificationCallback;
10311037
chip::Callback::Callback<Credentials::DeviceAttestationVerifier::OnAttestationInformationVerification>
1032-
mDACChainRevocationStatusVerificationCallback;
1038+
mDACChainRevocationStatusCallback;
10331039

10341040
chip::Callback::Callback<OnNOCChainGeneration> mDeviceNOCChainCallback;
10351041
SetUpCodePairer mSetUpCodePairer;

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -607,12 +607,12 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa
607607
return CHIP_NO_ERROR;
608608
}
609609

610-
void DefaultDACVerifier::ValidateDACChainRevocationStatus(const AttestationInfo & info,
611-
Callback::Callback<OnAttestationInformationVerification> * onCompletion)
610+
void DefaultDACVerifier::CheckForRevokedDACChain(const AttestationInfo & info,
611+
Callback::Callback<OnAttestationInformationVerification> * onCompletion)
612612
{
613613
AttestationVerificationResult attestationError = AttestationVerificationResult::kSuccess;
614614

615-
// TODO
615+
// TODO(#33124): Implement default version of CheckForRevokedDACChain
616616

617617
onCompletion->mCall(onCompletion->mContext, info, attestationError);
618618
}

‎src/credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -74,8 +74,8 @@ class DefaultDACVerifier : public DeviceAttestationVerifier
7474
const ByteSpan & attestationSignatureBuffer,
7575
const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override;
7676

77-
void ValidateDACChainRevocationStatus(const AttestationInfo & info,
78-
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;
77+
void CheckForRevokedDACChain(const AttestationInfo & info,
78+
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;
7979

8080
CsaCdKeysTrustStore * GetCertificationDeclarationTrustStore() override { return &mCdKeysTrustStore; }
8181

‎src/credentials/attestation_verifier/DeviceAttestationVerifier.cpp

+3-2
Original file line numberDiff line numberDiff line change
@@ -70,11 +70,12 @@ class UnimplementedDACVerifier : public DeviceAttestationVerifier
7070
return CHIP_ERROR_NOT_IMPLEMENTED;
7171
}
7272

73-
void ValidateDACChainRevocationStatus(const AttestationInfo & info,
74-
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override
73+
void CheckForRevokedDACChain(const AttestationInfo & info,
74+
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override
7575
{
7676
(void) info;
7777
(void) onCompletion;
78+
VerifyOrDie(false);
7879
}
7980
};
8081

‎src/credentials/attestation_verifier/DeviceAttestationVerifier.h

+9-2
Original file line numberDiff line numberDiff line change
@@ -386,8 +386,15 @@ class DeviceAttestationVerifier
386386
const Crypto::P256PublicKey & dacPublicKey,
387387
const ByteSpan & csrNonce) = 0;
388388

389-
virtual void ValidateDACChainRevocationStatus(const AttestationInfo & info,
390-
Callback::Callback<OnAttestationInformationVerification> * onCompletion) = 0;
389+
/**
390+
* @brief Verify whether or not the given DAC chain is revoked.
391+
*
392+
* @param[in] info All of the information required to check for revoked DAC chain.
393+
* @param[in] onCompletion Callback handler to provide Attestation Information Verification result to the caller of
394+
* CheckForRevokedDACChain()
395+
*/
396+
virtual void CheckForRevokedDACChain(const AttestationInfo & info,
397+
Callback::Callback<OnAttestationInformationVerification> * onCompletion) = 0;
391398

392399
/**
393400
* @brief Get the trust store used for the attestation verifier.

‎src/lib/core/CHIPError.cpp

+3
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
146146
case CHIP_ERROR_INVALID_LIST_LENGTH.AsInteger():
147147
desc = "Invalid list length";
148148
break;
149+
case CHIP_ERROR_FAILED_DEVICE_ATTESTATION.AsInteger():
150+
desc = "Failed Device Attestation";
151+
break;
149152
case CHIP_END_OF_TLV.AsInteger():
150153
desc = "End of TLV";
151154
break;

‎src/lib/core/CHIPError.h

+8-1
Original file line numberDiff line numberDiff line change
@@ -707,7 +707,14 @@ using CHIP_ERROR = ::chip::ChipError;
707707
*/
708708
#define CHIP_ERROR_INVALID_LIST_LENGTH CHIP_CORE_ERROR(0x1f)
709709

710-
// AVAILABLE: 0x20
710+
/**
711+
* @def CHIP_ERROR_FAILED_DEVICE_ATTESTATION
712+
*
713+
* @brief
714+
* Device Attestation failed.
715+
*
716+
*/
717+
#define CHIP_ERROR_FAILED_DEVICE_ATTESTATION CHIP_CORE_ERROR(0x20)
711718

712719
/**
713720
* @def CHIP_END_OF_TLV

‎src/lib/core/tests/TestCHIPErrorStr.cpp

+1
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ static const CHIP_ERROR kTestElements[] =
7171
CHIP_ERROR_UNINITIALIZED,
7272
CHIP_ERROR_INVALID_STRING_LENGTH,
7373
CHIP_ERROR_INVALID_LIST_LENGTH,
74+
CHIP_ERROR_FAILED_DEVICE_ATTESTATION,
7475
CHIP_END_OF_TLV,
7576
CHIP_ERROR_TLV_UNDERRUN,
7677
CHIP_ERROR_INVALID_TLV_ELEMENT,

0 commit comments

Comments
 (0)