Skip to content

Commit

Permalink
Expire associated secure sessions when a fabric is deleted (#9571)
Browse files Browse the repository at this point in the history
* Expire associated secure sessions when a fabric is deleted

* regen zap files

* another zap regen

* address review comments

* address review comments

* updates

* update CASESession error handling

* fix build error
  • Loading branch information
pan-apple authored Sep 13, 2021
1 parent f0723a2 commit 692b86c
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -238,18 +238,58 @@ void emberAfPluginOperationalCredentialsServerInitCallback(void)
writeFabricsIntoFabricsListAttribute();
}

namespace {
class FabricCleanupExchangeDelegate : public Messaging::ExchangeDelegate
{
public:
CHIP_ERROR OnMessageReceived(Messaging::ExchangeContext * ec, const PayloadHeader & payloadHeader,
System::PacketBufferHandle && payload) override
{
return CHIP_NO_ERROR;
}
void OnResponseTimeout(Messaging::ExchangeContext * ec) override {}
void OnExchangeClosing(Messaging::ExchangeContext * ec) override
{
FabricIndex currentFabricIndex = ec->GetSecureSession().GetFabricIndex();
ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(currentFabricIndex);
}
};

FabricCleanupExchangeDelegate gFabricCleanupExchangeDelegate;

} // namespace

bool emberAfOperationalCredentialsClusterRemoveFabricCallback(EndpointId endpoint, app::CommandHandler * commandObj,
FabricIndex fabricIndex)
FabricIndex fabricBeingRemoved)
{
emberAfPrintln(EMBER_AF_PRINT_DEBUG, "OpCreds: RemoveFabric"); // TODO: Generate emberAfFabricClusterPrintln

EmberAfStatus status = EMBER_ZCL_STATUS_SUCCESS;
CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricIndex);
CHIP_ERROR err = Server::GetInstance().GetFabricTable().Delete(fabricBeingRemoved);
VerifyOrExit(err == CHIP_NO_ERROR, status = EMBER_ZCL_STATUS_FAILURE);

exit:
writeFabricsIntoFabricsListAttribute();
emberAfSendImmediateDefaultResponse(status);
if (err == CHIP_NO_ERROR)
{
Messaging::ExchangeContext * ec = commandObj->GetExchangeContext();
FabricIndex currentFabricIndex = ec->GetSecureSession().GetFabricIndex();
if (currentFabricIndex == fabricBeingRemoved)
{
// If the current fabric is being removed, expiring all the secure sessions causes crashes as
// the message sent by emberAfSendImmediateDefaultResponse() is still in the queue. Also, RMP
// retries to send the message and runs into issues.
// We are hijacking the exchange delegate here (as no more messages should be received on this exchange),
// and wait for it to close, before expiring the secure sessions for the fabric.
// TODO: https://github.com/project-chip/connectedhomeip/issues/9642
ec->SetDelegate(&gFabricCleanupExchangeDelegate);
}
else
{
ec->GetExchangeMgr()->GetSessionMgr()->ExpireAllPairingsForFabric(fabricBeingRemoved);
}
}
return true;
}

Expand Down
12 changes: 12 additions & 0 deletions src/app/tests/suites/OperationalCredentialsCluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,15 @@ tests:
constraints:
type: uint8
minValue: 1

# This test is currently disabled as it breaks on Darwin.
# The test removes the current fabric, and Darwin test runner reuses
# the same pairing to run all the tests. Due to that, all subsequent
# tests fail.
- label: "Remove fabric"
disabled: true
command: "RemoveFabric"
arguments:
values:
- name: "FabricIndex"
value: 1
1 change: 1 addition & 0 deletions src/messaging/ExchangeContext.h
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen
}

SessionHandle GetSecureSession() { return mSecureSession.Value(); }
bool HasSecureSession() const { return mSecureSession.HasValue(); }

uint16_t GetExchangeId() const { return mExchangeId; }

Expand Down
2 changes: 1 addition & 1 deletion src/messaging/ReliableMessageMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ CHIP_ERROR ReliableMessageMgr::SendFromRetransTable(RetransTableEntry * entry)
}

const ExchangeMessageDispatch * dispatcher = rc->GetExchangeContext()->GetMessageDispatch();
if (dispatcher == nullptr)
if (dispatcher == nullptr || !rc->GetExchangeContext()->HasSecureSession())
{
// Using same error message for all errors to reduce code size.
ChipLogError(ExchangeManager, "Crit-err %" CHIP_ERROR_FORMAT " when sending CHIP MsgId:%08" PRIX32 ", send tries: %d",
Expand Down
6 changes: 4 additions & 2 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1035,8 +1035,10 @@ void CASESession::SendErrorMsg(SigmaErrorType errorCode)

msg->SetDataLength(msglen);

VerifyOrReturn(mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) != CHIP_NO_ERROR,
ChipLogError(SecureChannel, "Failed to send error message"));
if (mExchangeCtxt->SendMessage(Protocols::SecureChannel::MsgType::CASE_SigmaErr, std::move(msg)) != CHIP_NO_ERROR)
{
ChipLogError(SecureChannel, "Failed to send error message");
}
}

CHIP_ERROR CASESession::ConstructSaltSigmaR2(const ByteSpan & rand, const Crypto::P256PublicKey & pubkey, const ByteSpan & ipk,
Expand Down
24 changes: 24 additions & 0 deletions src/transport/PeerConnections.h
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,30 @@ class PeerConnections
return state;
}

/**
* Get the first peer connection state that matches the given fabric index.
*
* @param fabric The fabric index to match
*
* @return the state found, nullptr if not found
*/
CHECK_RETURN_VALUE
PeerConnectionState * FindPeerConnectionStateByFabric(FabricIndex fabric)
{
for (auto & state : mStates)
{
if (!state.IsInitialized())
{
continue;
}
if (state.GetFabricIndex() == fabric)
{
return &state;
}
}
return nullptr;
}

/// Convenience method to mark a peer connection state as active
void MarkConnectionActive(PeerConnectionState * state)
{
Expand Down
12 changes: 12 additions & 0 deletions src/transport/SecureSessionMgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,18 @@ void SecureSessionMgr::ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric)
}
}

void SecureSessionMgr::ExpireAllPairingsForFabric(FabricIndex fabric)
{
ChipLogDetail(Inet, "Expiring all connections for fabric %d!!", fabric);
PeerConnectionState * state = mPeerConnections.FindPeerConnectionStateByFabric(fabric);
while (state != nullptr)
{
mPeerConnections.MarkConnectionExpired(
state, [this](const Transport::PeerConnectionState & state1) { HandleConnectionExpired(state1); });
state = mPeerConnections.FindPeerConnectionStateByFabric(fabric);
}
}

CHIP_ERROR SecureSessionMgr::NewPairing(const Optional<Transport::PeerAddress> & peerAddr, NodeId peerNodeId,
PairingSession * pairing, SecureSession::SessionRole direction, FabricIndex fabric)
{
Expand Down
1 change: 1 addition & 0 deletions src/transport/SecureSessionMgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ class DLL_EXPORT SecureSessionMgr : public TransportMgrDelegate

void ExpirePairing(SessionHandle session);
void ExpireAllPairings(NodeId peerNodeId, FabricIndex fabric);
void ExpireAllPairingsForFabric(FabricIndex fabric);

/**
* @brief
Expand Down

0 comments on commit 692b86c

Please sign in to comment.