Skip to content

Commit

Permalink
Properly manage operational key lifecycle for fail-safe
Browse files Browse the repository at this point in the history
- Fail-safe did not properly manage the roll-back of operational keys
- Operational key storage being centralized by value in FabricTable
  prevented ability to back keys by hardware/OS and allow the rollback
  of keys on failsafe expiry
- CASE code was using "raw" FabricInfo * which could go stale on UpdateNOC
  or after fail-safe expiry.

This PR:
- Adds an OperationalKeystore interface
- Make the FabricTable use the OperationalKeystore for when
  a commissionable node (with Opcreds cluster) is being commissioned
- Retain legacy controller behavior that allows injection of operational
  keys
- Simplifies the fail-safe handling lifecycle
- Add logging to fail-safe handling
- Add logging to general commissioning cluster
- Make CASE use ScopedNodeId everywhere
- Implement IsForUpdateNOC in fail-safe and opcreds cluster

Fixes project-chip#19072
Issue project-chip#18633
Fixes project-chip#16443
  • Loading branch information
tcarmelveilleux committed Jun 7, 2022
1 parent a81171b commit f29b087
Show file tree
Hide file tree
Showing 28 changed files with 1,153 additions and 374 deletions.
2 changes: 1 addition & 1 deletion src/app/CASEClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ CHIP_ERROR CASEClient::EstablishSession(PeerId peer, const Transport::PeerAddres
VerifyOrReturnError(exchange != nullptr, CHIP_ERROR_INTERNAL);

mCASESession.SetGroupDataProvider(mInitParams.groupDataProvider);
ReturnErrorOnFailure(mCASESession.EstablishSession(*mInitParams.sessionManager, mInitParams.fabricInfo, peer.GetNodeId(),
ReturnErrorOnFailure(mCASESession.EstablishSession(*mInitParams.sessionManager, mInitParams.fabricTable, ScopedNodeId{peer.GetNodeId(), mInitParams.fabricIndex},
exchange, mInitParams.sessionResumptionStorage, delegate,
mInitParams.mrpLocalConfig));

Expand Down
4 changes: 3 additions & 1 deletion src/app/CASEClient.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ struct CASEClientInitParams
SessionManager * sessionManager = nullptr;
SessionResumptionStorage * sessionResumptionStorage = nullptr;
Messaging::ExchangeManager * exchangeMgr = nullptr;
FabricInfo * fabricInfo = nullptr;
FabricTable * fabricTable = nullptr;
FabricIndex fabricIndex = kUndefinedFabricIndex;

Credentials::GroupDataProvider * groupDataProvider = nullptr;

Optional<ReliableMessageProtocolConfig> mrpLocalConfig = Optional<ReliableMessageProtocolConfig>::Missing();
Expand Down
6 changes: 3 additions & 3 deletions src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ bool OperationalDeviceProxy::AttachToExistingSecureSession()
{
VerifyOrReturnError(mState == State::NeedsAddress || mState == State::ResolvingAddress || mState == State::HasAddress, false);

ScopedNodeId peerNodeId(mPeerId.GetNodeId(), mFabricInfo->GetFabricIndex());
ScopedNodeId peerNodeId(mPeerId.GetNodeId(), mFabricIndex);
auto sessionHandle =
mInitParams.sessionManager->FindSecureSessionForNode(peerNodeId, MakeOptional(Transport::SecureSession::Type::kCASE));
if (!sessionHandle.HasValue())
Expand Down Expand Up @@ -213,7 +213,7 @@ CHIP_ERROR OperationalDeviceProxy::EstablishConnection()
{
mCASEClient = mInitParams.clientPool->Allocate(
CASEClientInitParams{ mInitParams.sessionManager, mInitParams.sessionResumptionStorage, mInitParams.exchangeMgr,
mFabricInfo, mInitParams.groupDataProvider, mInitParams.mrpLocalConfig });
mInitParams.fabricTable, mFabricIndex, mInitParams.groupDataProvider, mInitParams.mrpLocalConfig });
ReturnErrorCodeIf(mCASEClient == nullptr, CHIP_ERROR_NO_MEMORY);

CHIP_ERROR err = mCASEClient->EstablishSession(mPeerId, mDeviceAddress, mRemoteMRPConfig, this);
Expand Down Expand Up @@ -352,7 +352,7 @@ void OperationalDeviceProxy::OnSessionHang()

CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
{
return app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricInfo->GetFabricIndex(), GetDeviceId());
return app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex, GetDeviceId());
}

OperationalDeviceProxy::~OperationalDeviceProxy()
Expand Down
20 changes: 12 additions & 8 deletions src/app/OperationalDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,15 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,

mSystemLayer = params.exchangeMgr->GetSessionManager()->SystemLayer();
mPeerId = peerId;
mFabricInfo = params.fabricTable->FindFabricWithCompressedId(peerId.GetCompressedFabricId());

const auto * fabricInfo = params.fabricTable->FindFabricWithCompressedId(peerId.GetCompressedFabricId());
if (fabricInfo == nullptr)
{
mState = State::Uninitialized;
return;
}

mFabricIndex = fabricInfo->GetFabricIndex();
mState = State::NeedsAddress;
mAddressLookupHandle.SetListener(this);
}
Expand Down Expand Up @@ -190,15 +198,11 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
}

/**
* @brief Get the raw Fabric ID assigned to the device.
* @brief Get the fabricIndex
*/
FabricIndex GetFabricIndex() const
{
if (mFabricInfo != nullptr)
{
return mFabricInfo->GetFabricIndex();
}
return kUndefinedFabricIndex;
return mFabricIndex;
}

/**
Expand All @@ -222,7 +226,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
};

DeviceProxyInitParams mInitParams;
FabricInfo * mFabricInfo;
FabricIndex mFabricIndex;
System::Layer * mSystemLayer;

// mCASEClient is only non-null if we are in State::Connecting or just
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ bool emberAfGeneralCommissioningClusterArmFailSafeCallback(app::CommandHandler *
FailSafeContext & failSafeContext = DeviceLayer::DeviceControlServer::DeviceControlSvr().GetFailSafeContext();
Commands::ArmFailSafeResponse::Type response;

ChipLogProgress(FailSafe, "GeneralCommissioning: Received ArmFailSafe (%us)", static_cast<unsigned>(commandData.expiryLengthSeconds));

/*
* If the fail-safe timer is not fully disarmed, don't allow arming a new fail-safe.
* If the fail-safe timer was not currently armed, then the fail-safe timer SHALL be armed.
Expand Down Expand Up @@ -214,7 +216,10 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
MATTER_TRACE_EVENT_SCOPE("CommissioningComplete", "GeneralCommissioning");

DeviceControlServer * server = &DeviceLayer::DeviceControlServer::DeviceControlSvr();
const auto & failSafe = server->GetFailSafeContext();
auto & failSafe = server->GetFailSafeContext();
auto & fabricTable = Server::GetInstance().GetFabricTable();

ChipLogProgress(FailSafe, "GeneralCommissioning: Received CommissioningComplete");

Commands::CommissioningCompleteResponse::Type response;
if (!failSafe.IsFailSafeArmed())
Expand All @@ -231,15 +236,31 @@ bool emberAfGeneralCommissioningClusterCommissioningCompleteCallback(
!failSafe.MatchesFabricIndex(commandObj->GetAccessingFabricIndex()))
{
response.errorCode = CommissioningError::kInvalidAuthentication;
ChipLogError(FailSafe, "GeneralCommissioning: Got commissioning complete in invalid security context");
}
else
{
if (failSafe.NocCommandHasBeenInvoked())
{
CHIP_ERROR err = fabricTable.CommitPendingFabricData();
if (err != CHIP_NO_ERROR)
{
ChipLogError(FailSafe, "GeneralCommissioning: Failed to commit pending fabric data: %" CHIP_ERROR_FORMAT, err.Format());
}
else
{
ChipLogProgress(FailSafe, "GeneralCommissioning: Successfully commited pending fabric data");
}
CheckSuccess(err, Failure);
}

/*
* Pass fabric of commissioner to DeviceControlSvr.
* This allows device to send messages back to commissioner.
* Once bindings are implemented, this may no longer be needed.
*/
CheckSuccess(server->CommissioningComplete(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
failSafe.DisarmFailSafe();
CheckSuccess(server->PostCommissioningCompleteEvent(handle->AsSecureSession()->GetPeerNodeId(), handle->GetFabricIndex()),
Failure);

Breadcrumb::Set(commandPath.mEndpointId, 0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ void Instance::OnResult(Status commissioningError, CharSpan debugText, int32_t i
}
if (commissioningError == Status::kSuccess)
{
DeviceLayer::DeviceControlServer::DeviceControlSvr().ConnectNetworkForOperational(
DeviceLayer::DeviceControlServer::DeviceControlSvr().PostConnectedToOperationalNetworkEvent(
ByteSpan(mLastNetworkID, mLastNetworkIDLen));
mLastConnectErrorValue.SetNull();
}
Expand Down
Loading

0 comments on commit f29b087

Please sign in to comment.