Skip to content

Commit

Permalink
Stop using Credentials::GetDefaultDACVerifier on Darwin. (#19697)
Browse files Browse the repository at this point in the history
Credentials::GetDefaultDACVerifier returns a singleton object,
initialized the very first time the function is called.  The Darwin
framework wants to be able to shut down and restart with a new PAA
trust store, which means not relying on this initialized-once
singleton.

The fixes to DeviceControllerSystemState shutdown were needed to make
the tests (which start up a factory, then shut it down without
bringing up any controllers, which is a valid thing to do) pass.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Jul 8, 2022
1 parent b34c9bd commit 4bbcd57
Show file tree
Hide file tree
Showing 8 changed files with 75 additions and 34 deletions.
8 changes: 7 additions & 1 deletion src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,13 @@ void DeviceControllerFactory::Shutdown()

CHIP_ERROR DeviceControllerSystemState::Shutdown()
{
VerifyOrReturnError(mRefCount == 1, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mRefCount <= 1, CHIP_ERROR_INCORRECT_STATE);
if (mHaveShutDown)
{
// Nothing else to do here.
return CHIP_NO_ERROR;
}
mHaveShutDown = true;

ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack");

Expand Down
10 changes: 9 additions & 1 deletion src/controller/CHIPDeviceControllerSystemState.h
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,13 @@ class DeviceControllerSystemState
using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool;

public:
~DeviceControllerSystemState(){};
~DeviceControllerSystemState()
{
// We could get here if a DeviceControllerFactory is shut down
// without ever creating any controllers, so our refcount never goes
// above 1. In that case we need to make sure we call Shutdown().
Shutdown();
};
DeviceControllerSystemState(DeviceControllerSystemStateParams params) :
mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager),
mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr),
Expand Down Expand Up @@ -195,6 +201,8 @@ class DeviceControllerSystemState

std::atomic<uint32_t> mRefCount{ 1 };

bool mHaveShutDown = false;

CHIP_ERROR Shutdown();
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,31 +145,7 @@ CHIP_ERROR GetCertificationDeclarationCertificate(const ByteSpan & skid, Mutable
return CopySpanToMutableSpan(ByteSpan{ sCertChainLookupTable[certChainLookupTableIdx].mCertificate }, outCertificate);
}

class DefaultDACVerifier : public DeviceAttestationVerifier
{
public:
DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {}

void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;

AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer,
ByteSpan & certDeclBuffer) override;

AttestationVerificationResult ValidateCertificateDeclarationPayload(const ByteSpan & certDeclBuffer,
const ByteSpan & firmwareInfo,
const DeviceInfoForAttestation & deviceInfo) override;

CHIP_ERROR VerifyNodeOperationalCSRInformation(const ByteSpan & nocsrElementsBuffer,
const ByteSpan & attestationChallengeBuffer,
const ByteSpan & attestationSignatureBuffer, const P256PublicKey & dacPublicKey,
const ByteSpan & csrNonce) override;

protected:
DefaultDACVerifier() {}

const AttestationTrustStore * mAttestationTrustStore;
};
} // namespace

void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
Callback::Callback<OnAttestationInformationVerification> * onCompletion)
Expand Down Expand Up @@ -443,8 +419,6 @@ CHIP_ERROR DefaultDACVerifier::VerifyNodeOperationalCSRInformation(const ByteSpa
return CHIP_NO_ERROR;
}

} // namespace

const AttestationTrustStore * GetTestAttestationTrustStore()
{
return &kTestAttestationTrustStore;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,32 @@
namespace chip {
namespace Credentials {

class DefaultDACVerifier : public DeviceAttestationVerifier
{
public:
DefaultDACVerifier(const AttestationTrustStore * paaRootStore) : mAttestationTrustStore(paaRootStore) {}

void VerifyAttestationInformation(const DeviceAttestationVerifier::AttestationInfo & info,
Callback::Callback<OnAttestationInformationVerification> * onCompletion) override;

AttestationVerificationResult ValidateCertificationDeclarationSignature(const ByteSpan & cmsEnvelopeBuffer,
ByteSpan & certDeclBuffer) override;

AttestationVerificationResult ValidateCertificateDeclarationPayload(const ByteSpan & certDeclBuffer,
const ByteSpan & firmwareInfo,
const DeviceInfoForAttestation & deviceInfo) override;

CHIP_ERROR VerifyNodeOperationalCSRInformation(const ByteSpan & nocsrElementsBuffer,
const ByteSpan & attestationChallengeBuffer,
const ByteSpan & attestationSignatureBuffer,
const Crypto::P256PublicKey & dacPublicKey, const ByteSpan & csrNonce) override;

protected:
DefaultDACVerifier() {}

const AttestationTrustStore * mAttestationTrustStore;
};

/**
* @brief Get implementation of a PAA root store containing a basic set of static PAA roots
* sufficient for *testing* only.
Expand All @@ -35,14 +61,17 @@ namespace Credentials {
const AttestationTrustStore * GetTestAttestationTrustStore();

/**
* @brief Get implementation of a sample DAC verifier to validate device
* @brief Get a singleton implementation of a sample DAC verifier to validate device
* attestation procedure.
*
* @param[in] paaRootStore Pointer to the AttestationTrustStore instance to be used by implementation
* of default DeviceAttestationVerifier. Caller must ensure storage is
* always available while the DeviceAttestationVerifier could be used.
*
* @returns a singleton DeviceAttestationVerifier that satisfies basic device attestation procedure requirements.
* This has process lifetime, so the paaRootStore must also have
* process lifetime. In particular, after the first call it's not
* possible to change which AttestationTrustStore is used by this verifier.
*/
DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore);

Expand Down
2 changes: 2 additions & 0 deletions src/darwin/Framework/CHIP/CHIPDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
#include <controller/CommissioningWindowOpener.h>
#include <credentials/FabricTable.h>
#include <credentials/GroupDataProvider.h>
#include <credentials/attestation_verifier/DefaultDeviceAttestationVerifier.h>
#include <lib/core/CHIPVendorIdentifiers.hpp>
#include <platform/PlatformManager.h>
#include <setup_payload/ManualSetupPayloadGenerator.h>
Expand Down Expand Up @@ -277,6 +278,7 @@ - (BOOL)startup:(CHIPDeviceControllerStartupParamsInternal *)startupParams
commissionerParams.controllerNOC = noc;
}
commissionerParams.controllerVendorId = static_cast<chip::VendorId>([startupParams.vendorId unsignedShortValue]);
commissionerParams.deviceAttestationVerifier = _factory.deviceAttestationVerifier;

auto & factory = chip::Controller::DeviceControllerFactory::GetInstance();

Expand Down
18 changes: 15 additions & 3 deletions src/darwin/Framework/CHIP/MatterControllerFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@

static NSString * const kErrorPersistentStorageInit = @"Init failure while creating a persistent storage delegate";
static NSString * const kErrorAttestationTrustStoreInit = @"Init failure while creating the attestation trust store";
static NSString * const kErrorDACVerifierInit = @"Init failure while creating the device attestation verifier";
static NSString * const kInfoFactoryShutdown = @"Shutting down the Matter controller factory";
static NSString * const kErrorGroupProviderInit = @"Init failure while initializing group data provider";
static NSString * const kErrorControllersInit = @"Init controllers array failure";
Expand All @@ -64,6 +65,7 @@ @interface MatterControllerFactory ()
@property (readonly) Credentials::GroupDataProviderImpl * groupDataProvider;
@property (readonly) NSMutableArray<CHIPDeviceController *> * controllers;
@property (readonly) PersistentStorageOperationalKeystore * keystore;
@property () chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier;

- (BOOL)findMatchingFabric:(FabricTable &)fabricTable
params:(CHIPDeviceControllerStartupParams *)params
Expand Down Expand Up @@ -156,6 +158,11 @@ - (void)cleanupInitObjects

- (void)cleanupStartupObjects
{
if (_deviceAttestationVerifier) {
delete _deviceAttestationVerifier;
_deviceAttestationVerifier = nullptr;
}

if (_attestationTrustStoreBridge) {
delete _attestationTrustStoreBridge;
_attestationTrustStoreBridge = nullptr;
Expand Down Expand Up @@ -209,17 +216,22 @@ - (BOOL)startup:(MatterControllerFactoryParams *)startupParams
}

// Initialize device attestation verifier
const Credentials::AttestationTrustStore * trustStore;
if (startupParams.paaCerts) {
_attestationTrustStoreBridge = new CHIPAttestationTrustStoreBridge(startupParams.paaCerts);
if (_attestationTrustStoreBridge == nullptr) {
CHIP_LOG_ERROR("Error: %@", kErrorAttestationTrustStoreInit);
return;
}
chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::GetDefaultDACVerifier(_attestationTrustStoreBridge));
trustStore = _attestationTrustStoreBridge;
} else {
// TODO: Replace testingRootStore with a AttestationTrustStore that has the necessary official PAA roots available
const chip::Credentials::AttestationTrustStore * testingRootStore = chip::Credentials::GetTestAttestationTrustStore();
chip::Credentials::SetDeviceAttestationVerifier(chip::Credentials::GetDefaultDACVerifier(testingRootStore));
trustStore = Credentials::GetTestAttestationTrustStore();
}
_deviceAttestationVerifier = new Credentials::DefaultDACVerifier(trustStore);
if (_deviceAttestationVerifier == nullptr) {
CHIP_LOG_ERROR("Error: %@", kErrorDACVerifierInit);
return;
}

chip::Controller::FactoryInitParams params;
Expand Down
3 changes: 2 additions & 1 deletion src/darwin/Framework/CHIP/MatterControllerFactory_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class CHIPPersistentStorageDelegateBridge;
namespace chip {
namespace Credentials {
class GroupDataProvider;
class DeviceAttestationVerifier;
} // namespace Credentials
} // namespace chip

Expand All @@ -44,7 +45,7 @@ NS_ASSUME_NONNULL_BEGIN

@property (readonly) CHIPPersistentStorageDelegateBridge * storageDelegateBridge;
@property (readonly) chip::Credentials::GroupDataProvider * groupData;

@property (readonly) chip::Credentials::DeviceAttestationVerifier * deviceAttestationVerifier;
@end

NS_ASSUME_NONNULL_END
Expand Down
9 changes: 9 additions & 0 deletions src/darwin/Framework/CHIPTests/CHIPClustersTests.m
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,19 @@ - (void)testInitStack
__auto_type * storage = [[CHIPTestStorage alloc] init];
__auto_type * factoryParams = [[MatterControllerFactoryParams alloc] initWithStorage:storage];
factoryParams.port = @(kLocalPort);
// For our first startup, use a dummy paaCerts array.
factoryParams.paaCerts = [[NSArray alloc] init];

BOOL ok = [factory startup:factoryParams];
XCTAssertTrue(ok);

// Test that things still work right if we then shut down and
// restart the factory.
[factory shutdown];
factoryParams.paaCerts = nil;
ok = [factory startup:factoryParams];
XCTAssertTrue(ok);

__auto_type * testKeys = [[CHIPTestKeys alloc] init];
XCTAssertNotNil(testKeys);

Expand Down

0 comments on commit 4bbcd57

Please sign in to comment.