Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API SecureManager::SendMessage, use SecureSessoinHandle instead NodeId #3728

Merged
merged 1 commit into from
Jan 12, 2021
Merged

API SecureManager::SendMessage, use SecureSessoinHandle instead NodeId #3728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/app/CommandHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ void CommandHandler::OnMessageReceived(Messaging::ExchangeContext * ec, const Pa
err = ProcessCommandMessage(std::move(payload), kCommandHandlerId);
SuccessOrExit(err);

SendCommandResponse(ec->GetPeerNodeId());
SendCommandResponse();

exit:
ChipLogFunctError(err);
}

CHIP_ERROR CommandHandler::SendCommandResponse(NodeId aNodeId)
CHIP_ERROR CommandHandler::SendCommandResponse()
{
CHIP_ERROR err = CHIP_NO_ERROR;

Expand Down
2 changes: 1 addition & 1 deletion src/app/CommandHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ namespace app {
class DLL_EXPORT CommandHandler : public Command
{
public:
CHIP_ERROR SendCommandResponse(NodeId aNodeId);
CHIP_ERROR SendCommandResponse();
void OnMessageReceived(Messaging::ExchangeContext * ec, const PacketHeader & packetHeader, uint32_t protocolId, uint8_t msgType,
System::PacketBufferHandle payload);

Expand Down
5 changes: 3 additions & 2 deletions src/app/CommandSender.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ CHIP_ERROR CommandSender::SendCommandRequest(NodeId aNodeId)
ClearExistingExchangeContext();

// Create a new exchange context.
mpExchangeCtx = mpExchangeMgr->NewContext(aNodeId, this);
// TODO: temprary create a SecureSessionHandle from node id, will be fix in PR 3602
mpExchangeCtx = mpExchangeMgr->NewContext({ aNodeId, Transport::kAnyKeyId }, this);
VerifyOrExit(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
mpExchangeCtx->SetResponseTimeout(CHIP_INVOKE_COMMAND_RSP_TIMEOUT);

Expand Down Expand Up @@ -96,7 +97,7 @@ void CommandSender::OnMessageReceived(Messaging::ExchangeContext * apEc, const P

void CommandSender::OnResponseTimeout(Messaging::ExchangeContext * apEc)
{
ChipLogProgress(DataManagement, "Time out! failed to receive invoke command response from Node: %llu", apEc->GetPeerNodeId());
ChipLogProgress(DataManagement, "Time out! failed to receive invoke command response from Exchange: %d", apEc->GetExchangeId());
Reset();
}

Expand Down
2 changes: 1 addition & 1 deletion src/app/InteractionModelEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ void InteractionModelEngine::OnMessageReceived(Messaging::ExchangeContext * apEc

void InteractionModelEngine::OnResponseTimeout(Messaging::ExchangeContext * ec)
{
ChipLogProgress(DataManagement, "Time out! failed to receive echo response from Node: %llu", ec->GetPeerNodeId());
ChipLogProgress(DataManagement, "Time out! failed to receive echo response from Exchange: %d", ec->GetExchangeId());
}

InteractionModelEngine::HandlerKey::HandlerKey(chip::ClusterId aClusterId, chip::CommandId aCommandId,
Expand Down
11 changes: 5 additions & 6 deletions src/app/server/Server.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ bool isRendezvousBypassed()
class ServerCallback : public SecureSessionMgrDelegate
{
public:
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader,
const Transport::PeerConnectionState * state, System::PacketBufferHandle buffer,
SecureSessionMgr * mgr) override
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader, SecureSessionHandle session,
System::PacketBufferHandle buffer, SecureSessionMgr * mgr) override
{
auto state = mgr->GetPeerConnectionState(session);
const size_t data_len = buffer->DataLength();
char src_addr[PeerAddress::kMaxToStringSize];

Expand Down Expand Up @@ -92,7 +92,7 @@ class ServerCallback : public SecureSessionMgrDelegate
}
}

void OnNewConnection(const Transport::PeerConnectionState * state, SecureSessionMgr * mgr) override
void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) override
{
ChipLogProgress(AppServer, "Received a new connection.");
}
Expand Down Expand Up @@ -181,11 +181,10 @@ void InitServer(AppDelegate * delegate)
SuccessOrExit(err);
#endif

gSessions.SetDelegate(&gCallbacks);
err = gSessions.NewPairing(peer, chip::kTestControllerNodeId, &gTestPairing);
SuccessOrExit(err);

gSessions.SetDelegate(&gCallbacks);

exit:
if (err != CHIP_NO_ERROR)
{
Expand Down
3 changes: 2 additions & 1 deletion src/app/util/chip-message-send.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ EmberStatus chipSendUnicast(NodeId destination, EmberApsFrame * apsFrame, uint16
memcpy(buffer->Start() + frameSize, message, messageLength);
buffer->SetDataLength(dataLength);

CHIP_ERROR err = SessionManager().SendMessage(destination, std::move(buffer));
// TODO: temprary create a handle from node id, will be fix in PR 3602
CHIP_ERROR err = SessionManager().SendMessage({ destination, Transport::kAnyKeyId }, std::move(buffer));
if (err != CHIP_NO_ERROR)
{
// FIXME: Figure out better translations between our error types?
Expand Down
23 changes: 16 additions & 7 deletions src/controller/CHIPDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ CHIP_ERROR Device::SendMessage(System::PacketBufferHandle buffer)
resend = buffer.Retain();
}

err = mSessionManager->SendMessage(mDeviceId, std::move(buffer));
err = mSessionManager->SendMessage(mSecureSession, std::move(buffer));
buffer = nullptr;
ChipLogDetail(Controller, "SendMessage returned %d", err);

Expand All @@ -87,7 +87,7 @@ CHIP_ERROR Device::SendMessage(System::PacketBufferHandle buffer)
err = LoadSecureSessionParameters(ResetTransport::kYes);
SuccessOrExit(err);

err = mSessionManager->SendMessage(mDeviceId, std::move(resend));
err = mSessionManager->SendMessage(mSecureSession, std::move(resend));
ChipLogDetail(Controller, "Re-SendMessage returned %d", err);
SuccessOrExit(err);
}
Expand Down Expand Up @@ -175,9 +175,20 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input)
return error;
}

void Device::OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader,
const Transport::PeerConnectionState * state, System::PacketBufferHandle msgBuf,
SecureSessionMgr * mgr)
void Device::OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr)
{
mState = ConnectionState::SecureConnected;
mSecureSession = session;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect SecureSessionHandle to always be copyable? Or are we likely to switch to move semantics on it at some point? Put another way, should there be an std::move() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'll expect that the handle is POD, and always be copyable. In the future, only messaging layer will use the handle, the handle class won't be exposed to applications or controller API.

}

void Device::OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr)
{
mState = ConnectionState::NotConnected;
mSecureSession = SecureSessionHandle{};
}

void Device::OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader, SecureSessionHandle session,
System::PacketBufferHandle msgBuf, SecureSessionMgr * mgr)
{
if (mState == ConnectionState::SecureConnected)
{
Expand Down Expand Up @@ -255,8 +266,6 @@ CHIP_ERROR Device::LoadSecureSessionParameters(ResetTransport resetNeeded)
&pairingSession);
SuccessOrExit(err);

mState = ConnectionState::SecureConnected;

exit:

if (err != CHIP_NO_ERROR)
Expand Down
32 changes: 29 additions & 3 deletions src/controller/CHIPDevice.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,26 @@ class DLL_EXPORT Device
**/
CHIP_ERROR Deserialize(const SerializedDevice & input);

/**
* @brief
* Called when a new pairing is being established
*
* @param session A handle to the secure session
* @param mgr A pointer to the SecureSessionMgr
*/
void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr);

/**
* @brief
* Called when a connection is closing.
*
* The receiver should release all resources associated with the connection.
*
* @param session A handle to the secure session
* @param mgr A pointer to the SecureSessionMgr
*/
void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr);

/**
* @brief
* This function is called when a message is received from the corresponding CHIP
Expand All @@ -164,12 +184,12 @@ class DLL_EXPORT Device
*
* @param[in] header Reference to common packet header of the received message
* @param[in] payloadHeader Reference to payload header in the message
* @param[in] state Pointer to the peer connection state on which message is received
* @param[in] session A handle to the secure session
* @param[in] msgBuf The message buffer
* @param[in] mgr Pointer to secure session manager which received the message
*/
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader,
const Transport::PeerConnectionState * state, System::PacketBufferHandle msgBuf, SecureSessionMgr * mgr);
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader, SecureSessionHandle session,
System::PacketBufferHandle msgBuf, SecureSessionMgr * mgr);

/**
* @brief
Expand All @@ -180,6 +200,8 @@ class DLL_EXPORT Device

void SetActive(bool active) { mActive = active; }

bool IsSecureConnected() const { return IsActive() && mState == ConnectionState::SecureConnected; }

void Reset()
{
SetActive(false);
Expand All @@ -191,6 +213,8 @@ class DLL_EXPORT Device

NodeId GetDeviceId() const { return mDeviceId; }

bool MatchesSession(SecureSessionHandle session) const { return mSecureSession == session; }

void SetAddress(const Inet::IPAddress & deviceAddr) { mDeviceAddr = deviceAddr; }

SecurePairingSessionSerializable & GetPairing() { return mPairing; }
Expand Down Expand Up @@ -242,6 +266,8 @@ class DLL_EXPORT Device

DeviceTransportMgr * mTransportMgr;

SecureSessionHandle mSecureSession = {};

/* Track all outstanding response callbacks for this device. The callbacks are
registered when a command is sent to the device, to get notified with the results. */
Callback::CallbackDeque mResponses;
Expand Down
61 changes: 54 additions & 7 deletions src/controller/CHIPDeviceController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,24 +334,57 @@ CHIP_ERROR DeviceController::ServiceEventSignal()
return err;
}

void DeviceController::OnNewConnection(const Transport::PeerConnectionState * peerConnection, SecureSessionMgr * mgr) {}
void DeviceController::OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr)
{
CHIP_ERROR err = CHIP_NO_ERROR;
uint16_t index = 0;

VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);

index = FindDeviceIndex(mgr->GetPeerConnectionState(session)->GetPeerNodeId());
VerifyOrExit(index < kNumMaxActiveDevices, err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);

mActiveDevices[index].OnNewConnection(session, mgr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the device object use the secure session corresponding to the keyID that was derived during pairing (which is based on SPAKE2P handshake). How will it switch to using the key derived by SIGMA handshake (which is more secure, and should typically be used for all communication once pairing process is complete).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm not quite understand your question, but IMHO, SecureSessionHandle can represent both SPAKE2 and SIGMA session. And there can be multiple connections to a device, OnNewConnection is triggered for every connection.

If the Device class wants to "persistent" a connection, it can choose one connection it wants.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, let me rephrase the question. It's two parts.

  1. What's the use case where an application decides to use a PASE based connection (established by SPAKE2P), after a CASE based connection (established via SIGMA) is already established?
  2. How would the application know which KeyID corresponds to which connection. (it's ok if this logic is added in a follow on PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created another PR to address the problem (#4171), Let's continue the discussion on that PR.


exit:
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed to process received message: err %d", err);
}
}

void DeviceController::OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr)
{
CHIP_ERROR err = CHIP_NO_ERROR;
uint16_t index = 0;

VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);

index = FindDeviceIndex(session);
VerifyOrExit(index < kNumMaxActiveDevices, err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);

mActiveDevices[index].OnConnectionExpired(session, mgr);

exit:
if (err != CHIP_NO_ERROR)
{
ChipLogError(Controller, "Failed to process received message: err %d", err);
}
}

void DeviceController::OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader,
const Transport::PeerConnectionState * state, System::PacketBufferHandle msgBuf,
SecureSessionMgr * mgr)
SecureSessionHandle session, System::PacketBufferHandle msgBuf, SecureSessionMgr * mgr)
{
CHIP_ERROR err = CHIP_NO_ERROR;
uint16_t index = 0;
NodeId peer;

VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);
VerifyOrExit(header.GetSourceNodeId().HasValue(), err = CHIP_ERROR_INVALID_ARGUMENT);

peer = header.GetSourceNodeId().Value();
index = FindDeviceIndex(peer);
index = FindDeviceIndex(session);
VerifyOrExit(index < kNumMaxActiveDevices, err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);

mActiveDevices[index].OnMessageReceived(header, payloadHeader, state, std::move(msgBuf), mgr);
mActiveDevices[index].OnMessageReceived(header, payloadHeader, session, std::move(msgBuf), mgr);

exit:
if (err != CHIP_NO_ERROR)
Expand Down Expand Up @@ -395,6 +428,20 @@ void DeviceController::ReleaseAllDevices()
}
}

uint16_t DeviceController::FindDeviceIndex(SecureSessionHandle session)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is what the code used to look like with node id, but is there a reason this is not FindDevice returning Device *, with nullptr returned if not found? Then we could use a ranged for loop instead of the indexing we are doing now, and callers wouldn't need to index back into the array; they would just use the pointer they got, if not null.

Copy link
Contributor Author

@kghost kghost Dec 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While audit the usage of FindDeviceIndex, I don't think there is any reason preventing it from returning a Device *.

We can refactor it in another PR. but here I want to keep it consistent with FindDeviceIndex(NodeId id).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, followup is fine. Just file an issue, please.

{
uint16_t i = 0;
while (i < kNumMaxActiveDevices)
{
if (mActiveDevices[i].IsActive() && mActiveDevices[i].IsSecureConnected() && mActiveDevices[i].MatchesSession(session))
{
return i;
}
i++;
}
return i;
}

uint16_t DeviceController::FindDeviceIndex(NodeId id)
{
uint16_t i = 0;
Expand Down
9 changes: 5 additions & 4 deletions src/controller/CHIPDeviceController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,17 +192,18 @@ class DLL_EXPORT DeviceController : public SecureSessionMgrDelegate, public Pers

uint16_t mListenPort;
uint16_t GetInactiveDeviceIndex();
uint16_t FindDeviceIndex(SecureSessionHandle session);
uint16_t FindDeviceIndex(NodeId id);
void ReleaseDevice(uint16_t index);
CHIP_ERROR SetPairedDeviceList(const char * pairedDeviceSerializedSet);

private:
//////////// SecureSessionMgrDelegate Implementation ///////////////
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader,
const Transport::PeerConnectionState * state, System::PacketBufferHandle msgBuf,
SecureSessionMgr * mgr) override;
void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader, SecureSessionHandle session,
System::PacketBufferHandle msgBuf, SecureSessionMgr * mgr) override;

void OnNewConnection(const Transport::PeerConnectionState * state, SecureSessionMgr * mgr) override;
void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) override;
void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) override;

//////////// PersistentStorageResultDelegate Implementation ///////////////
void OnValue(const char * key, const char * value) override;
Expand Down
Loading