Skip to content

Commit

Permalink
Address PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@tehampson
tehampson committed Jun 15, 2022
1 parent ad4d136 commit 5fe85ab
Show file tree
Hide file tree
Showing 3 changed files with 69 additions and 60 deletions.
14 changes: 8 additions & 6 deletions src/protocols/secure_channel/CASESession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ void CASESession::Clear()

if (mFabricsTable)
{
mFabricsTable->RemoveFabricDelegate(&mFabricDelegate);
mFabricsTable->RemoveFabricDelegate(this);
}

mLocalNodeId = kUndefinedNodeId;
Expand Down Expand Up @@ -196,11 +196,13 @@ CASESession::PrepareForSessionEstablishment(SessionManager & sessionManager, Fab
SessionEstablishmentDelegate * delegate, ScopedNodeId previouslyEstablishedPeer,
Optional<ReliableMessageProtocolConfig> mrpConfig)
{
// Below VerifyOrReturnError is not SuccessOrExit since we only want to goto `exit:` after
// Init has been successfully called.
VerifyOrReturnError(fabricTable != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
ReturnErrorOnFailure(Init(sessionManager, policy, delegate, previouslyEstablishedPeer));

CHIP_ERROR err = CHIP_NO_ERROR;
SuccessOrExit(err = fabricTable->AddFabricDelegate(&mFabricDelegate));
SuccessOrExit(err = fabricTable->AddFabricDelegate(this));

mFabricsTable = fabricTable;
mRole = CryptoContext::SessionRole::kResponder;
Expand Down Expand Up @@ -248,7 +250,7 @@ CHIP_ERROR CASESession::EstablishSession(SessionManager & sessionManager, Fabric
// been initialized
SuccessOrExit(err);

SuccessOrExit(err = fabricTable->AddFabricDelegate(&mFabricDelegate));
SuccessOrExit(err = fabricTable->AddFabricDelegate(this));

mFabricsTable = fabricTable;
mFabricIndex = fabricInfo->GetFabricIndex();
Expand Down Expand Up @@ -279,7 +281,7 @@ void CASESession::OnResponseTimeout(ExchangeContext * ec)
VerifyOrReturn(mExchangeCtxt == ec, ChipLogError(SecureChannel, "CASESession::OnResponseTimeout exchange doesn't match"));
ChipLogError(SecureChannel, "CASESession timed out while waiting for a response from the peer. Current state was %u",
to_underlying(mState));
// Discard the exchange so that Clear() doesn't try closing it. The
// Discard the exchange so that Clear() doesn't try aborting it. The
// exchange will handle that.
DiscardExchange();
AbortPendingEstablish(CHIP_ERROR_TIMEOUT);
Expand Down Expand Up @@ -1734,7 +1736,7 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
// pending state. In order to keep this side open we have to tell the exchange context that we will send an
// async message.
//
// Should you need to resume the CASESession you could theortically pass along msg to a callback that gets
// Should you need to resume the CASESession, you could theoretically pass along the msg to a callback that gets
// registered when setting mStopHandshakeAtState.
mExchangeCtxt->WillSendMessage();
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -1837,7 +1839,7 @@ CHIP_ERROR CASESession::OnMessageReceived(ExchangeContext * ec, const PayloadHea
// Call delegate to indicate session establishment failure.
if (err != CHIP_NO_ERROR)
{
// Discard the exchange so that Clear() doesn't try closing it. The
// Discard the exchange so that Clear() doesn't try aborting it. The
// exchange will handle that.
DiscardExchange();
AbortPendingEstablish(err);
Expand Down
89 changes: 39 additions & 50 deletions src/protocols/secure_channel/CASESession.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,22 +58,10 @@ namespace chip {
// when implementing concurrent CASE session.
class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
public Messaging::ExchangeDelegate,
public FabricTable::Delegate,
public PairingSession
{
public:
// Public so it is accessible to unit test
enum class State : uint8_t
{
kInitialized = 0,
kSentSigma1 = 1,
kSentSigma2 = 2,
kSentSigma3 = 3,
kSentSigma1Resume = 4,
kSentSigma2Resume = 5,
kFinished = 6,
kFinishedViaResume = 7,
};

~CASESession() override;

Transport::SecureSession::Type GetSecureSessionType() const override { return Transport::SecureSession::Type::kCASE; }
Expand Down Expand Up @@ -179,51 +167,47 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
//// SessionDelegate ////
void OnSessionReleased() override;

//// FabricTable::Delegate Implementation ////
void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
}
void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
(void) fabricIndex;
}
void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
(void) fabricIndex;
}
void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override
{
(void) fabricTable;
InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
}

FabricIndex GetFabricIndex() const { return mFabricIndex; }

// TODO: remove Clear, we should create a new instance instead reset the old instance.
/** @brief This function zeroes out and resets the memory used by the object.
**/
void Clear();

void InvalidateIfPendingEstablishmentOnFabric(FabricIndex fabricIndex);

#if CONFIG_IM_BUILD_FOR_UNIT_TEST
void SetStopSigmaHandshakeAt(Optional<State> state) { mStopHandshakeAtState = state; }
#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST

private:
class CASESessionFabricDelegate final : public chip::FabricTable::Delegate
friend class CASESessionForTest;
enum class State : uint8_t
{
public:
CASESessionFabricDelegate(CASESession * caseSession) : mCASESession(caseSession) {}

void OnFabricDeletedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
mCASESession->InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
}

void OnFabricRetrievedFromStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
(void) fabricIndex;
}

void OnFabricPersistedToStorage(FabricTable & fabricTable, FabricIndex fabricIndex) override
{
(void) fabricTable;
(void) fabricIndex;
}

void OnFabricNOCUpdated(chip::FabricTable & fabricTable, chip::FabricIndex fabricIndex) override
{
(void) fabricTable;
mCASESession->InvalidateIfPendingEstablishmentOnFabric(fabricIndex);
}

private:
CASESession * mCASESession;
kInitialized = 0,
kSentSigma1 = 1,
kSentSigma2 = 2,
kSentSigma3 = 3,
kSentSigma1Resume = 4,
kSentSigma2Resume = 5,
kFinished = 6,
kFinishedViaResume = 7,
};

/*
Expand Down Expand Up @@ -286,6 +270,12 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
CHIP_ERROR ValidateReceivedMessage(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
const System::PacketBufferHandle & msg);

void InvalidateIfPendingEstablishmentOnFabric(FabricIndex fabricIndex);

#if CONFIG_IM_BUILD_FOR_UNIT_TEST
void SetStopSigmaHandshakeAt(Optional<State> state) { mStopHandshakeAtState = state; }
#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST

Crypto::Hash_SHA256_stream mCommissioningHash;
Crypto::P256PublicKey mRemotePubKey;
#ifdef ENABLE_HSM_CASE_EPHEMERAL_KEY
Expand Down Expand Up @@ -313,7 +303,6 @@ class DLL_EXPORT CASESession : public Messaging::UnsolicitedMessageHandler,
// Sigma1 initiator random, maintained to be reused post-Sigma1, such as when generating Sigma2 S2RK key
uint8_t mInitiatorRandom[kSigmaParamRandomNumberSize];

CASESessionFabricDelegate mFabricDelegate{ this };
State mState;

#if CONFIG_IM_BUILD_FOR_UNIT_TEST
Expand Down
26 changes: 22 additions & 4 deletions src/protocols/secure_channel/tests/TestCASESession.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -843,8 +843,18 @@ static void CASE_SessionResumptionStorage(nlTestSuite * inSuite, void * inContex
}
}

// TODO, move all tests above into this class
#if CONFIG_IM_BUILD_FOR_UNIT_TEST
static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext)
namespace chip {
// TODO rename CASESessionForTest to TestCASESession. Not doing that immediately since that requires
// removing a lot of the `using namesapce` above which is a larger cleanup.
class CASESessionForTest
{
public:
static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext);
};

void CASESessionForTest::CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * inSuite, void * inContext)
{
SessionManager sessionManager;
TestCASESecurePairingDelegate delegateCommissioner;
Expand All @@ -863,6 +873,8 @@ static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * i
ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Protocols::SecureChannel::MsgType::CASE_Sigma1,
&pairingAccessory) == CHIP_NO_ERROR);

// In order for all the test iterations below, we need to stop the CASE sigma handshake in the middle such
// that the CASE session is in the process of being established.
pairingCommissioner.SetStopSigmaHandshakeAt(MakeOptional(CASESession::State::kSentSigma1));

ExchangeContext * contextCommissioner = ctx.NewUnauthenticatedExchangeToBob(&pairingCommissioner);
Expand All @@ -882,26 +894,32 @@ static void CASE_SimulateUpdateNOCInvalidatePendingEstablishment(nlTestSuite * i
nullptr, nullptr, &delegateCommissioner) == CHIP_NO_ERROR);
ctx.DrainAndServiceIO();

// At this point the CASESession is in the process of establishing. Confirm that there are no errors and there are session
// has not been established.
NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 0);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 0);
NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 0);

// Simulating an update to the Fabric NOC for gCommissionerFabrics fabric table.
// Confirm that CASESession on commisioner side has reported an error.
gCommissionerFabrics.SendUpdateFabricNotificationForTest(gCommissionerFabricIndex);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 0);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 1);

// Simulating an update to the Fabric NOC for gDeviceFabrics fabric table.
// Confirm that CASESession on accessory side has reported an error.
gDeviceFabrics.SendUpdateFabricNotificationForTest(gDeviceFabricIndex);
ctx.DrainAndServiceIO();

NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingErrors == 1);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingErrors == 1);

// Sanity check that pairing did not complete.
NL_TEST_ASSERT(inSuite, delegateAccessory.mNumPairingComplete == 0);
NL_TEST_ASSERT(inSuite, delegateCommissioner.mNumPairingComplete == 0);
}
} // namespace chip
#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST

// Test Suite
Expand All @@ -922,7 +940,7 @@ static const nlTest sTests[] =
#if CONFIG_IM_BUILD_FOR_UNIT_TEST
// This is compiled for host tests which is enough test coverage to ensure updating NOC invalidates
// CASESession that are in the process of establishing.
NL_TEST_DEF("InvalidatePendingSessionEstablishment", CASE_SimulateUpdateNOCInvalidatePendingEstablishment),
NL_TEST_DEF("InvalidatePendingSessionEstablishment", chip::CASESessionForTest::CASE_SimulateUpdateNOCInvalidatePendingEstablishment),
#endif // CONFIG_IM_BUILD_FOR_UNIT_TEST

NL_TEST_SENTINEL()
Expand Down

0 comments on commit 5fe85ab

Please sign in to comment.