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

kghost · 2020-11-09T09:10:04Z

Problem

Currently SecureSessionMgrBase::SendMessage takes a PeerNodeId as key to find proper PeerConnectionState, but is way may causing unexpected behavior. For example.

If there are multiple connection to the same node, there is no way to specify which connection to use,
When switching a connection (SPAKE -> SIGMA, or SIGMA -> SPAKE), the application can not aware the change, and mistakenly send messages to wrong channel.
The ACL permission information is associated with a certification, which is used to initialized the connection, the connection derives access permission from it. So we should use connection as key to verify permission. Using node id is insecure and may cause potential security flaws.

Summary of Changes

SecureSessionMgrBase::SendMessage take a Transport::PeerConnectionState * instead of PeerNodeId
Enable SecureSessionMgrDelegate::OnNewConnection so that upper components can receive a PeerConnectionState object
Add SecureSessionMgrDelegate::OnConnectionExpired callback, so upper components can acknowledge that the connection has been released.

kghost · 2020-11-09T09:31:32Z

Before I resolve all CI failures, I want to collect opinions first.

andy31415 · 2020-11-09T14:08:24Z

I have conmcerns about the approach: I agree with the intent that there may be more communication paths than 1 to a node, which we plan to call 'exchanges'. The PeerConnectionState however was meant to encapsulate things like encryption keys and counters, which applications should not see.

I would rather imagine we would like something like a 'connectionID' exposed that could be something like a pair<nodeid, exchangeid>. This sounds like a minimal information required rather than an entire large structure pointer.

andy31415 · 2020-11-09T14:10:44Z

I have conmcerns about the approach: I agree with the intent that there may be more communication paths than 1 to a node, which we plan to call 'exchanges'. The PeerConnectionState however was meant to encapsulate things like encryption keys and counters, which applications should not see.

I would rather imagine we would like something like a 'connectionID' exposed that could be something like a pair<nodeid, exchangeid>. This sounds like a minimal information required rather than an entire large structure pointer.

Marked as 'changes requested' since I believe the PeerConnectionState to be too large of an object to pass around and ideally API boundaries should be with smaller pieces of data.

pan-apple · 2020-11-09T17:38:47Z

I would also like to understand the scenario where the user of the secure session manager is aware of SPAKE based connection vs Sigma based connection. Currently, SPAKE based secure session is only used by Rendezvous state machine. It's not exposed to the controller or the ZCL layer.

The multiple peer connections between two nodes are supported, but mainly for

Unicast/Multicast keys
Key rolling (where the old key/session is being torn down and new session is being established)

The first could be disambiguated by adding a multicast API to the secure session manager.
The second should be self contained inside the secure session layer.

kghost · 2020-11-09T18:46:02Z

@andy31415

I would rather imagine we would like something like a 'connectionID' exposed that could be something like a pair<nodeid, exchangeid>. This sounds like a minimal information required rather than an entire large structure pointer.

Marked as 'changes requested' since I believe the PeerConnectionState to be too large of an object to pass around and ideally API boundaries should be with smaller pieces of data.

I agree with this part, an ExchangeContext could be enough for application, node id can be retrieved via the context. But here I'm proposing the internal relation ship between ExchangeContext and SecureSession. The Transport::PeerConnectionState * is only exposed to the ExchangeManager, not application.

@pan-apple

I would also like to understand the scenario where the user of the secure session manager is aware of SPAKE based connection vs Sigma based connection. Currently, SPAKE based secure session is only used by Rendezvous state machine. It's not exposed to the controller or the ZCL layer.

Problem 1 and 2 may not be a real problem, but 3 is. Lets focus on the changes.,

How to implement ExchangeContext::GetPeerCertification ? The ACL information is stored inside the certification, and applications need it to grant access permission. Using only node id to look up peer state may not be a good idea, it is kind of tricky and unstable.

pan-apple · 2020-11-09T21:12:27Z

@andy31415

I would rather imagine we would like something like a 'connectionID' exposed that could be something like a pair<nodeid, exchangeid>. This sounds like a minimal information required rather than an entire large structure pointer.

Marked as 'changes requested' since I believe the PeerConnectionState to be too large of an object to pass around and ideally API boundaries should be with smaller pieces of data.

I agree with this part, an ExchangeContext could be enough for application, node id can be retrieved via the context. But here I'm proposing the internal relation ship between ExchangeContext and SecureSession. The Transport::PeerConnectionState * is only exposed to the ExchangeManager, not application.

@pan-apple

I would also like to understand the scenario where the user of the secure session manager is aware of SPAKE based connection vs Sigma based connection. Currently, SPAKE based secure session is only used by Rendezvous state machine. It's not exposed to the controller or the ZCL layer.

Problem 1 and 2 may not be a real problem, but 3 is. Lets focus on the changes.,

How to implement ExchangeContext::GetPeerCertification ? The ACL information is stored inside the certification, and applications need it to grant access permission. Using only node id to look up peer state may not be a good idea, it is kind of tricky and unstable.

What value (besides peer node ID) would be needed to look up the peer certificates? Is it peer's encryption key ID.

kghost · 2020-11-10T02:11:23Z

What value (besides peer node ID) would be needed to look up the peer certificates? Is it peer's encryption key ID.

Yes, using Tuple<NodeId, KeyId> may work. I have encountered another problem when writing message counter synchronization protocol, the application (protocol) needs to access message counter in the PeerConnectionState. If we directly associate ExchangeContext and PeerConnectionState we can reduce these lookups. And actually there is a one-to-many relationship between PeerConnectionState and ExchangeContext.

rwalker-apple · 2020-11-11T22:30:17Z

@pan-apple, still looking at this?

kghost · 2020-11-12T03:09:32Z

I'm looking for suggestions to move forward. Both Tuple<NodeId, KeyId> and direct pointer can work. If we can decide to move this direction, we can compare them deeply.

pan-apple · 2020-11-12T05:26:39Z

I'm looking for suggestions to move forward. Both Tuple<NodeId, KeyId> and direct pointer can work. If we can decide to move this direction, we can compare them deeply.

I don't have a strong opinion on either approach. Exposing the PeerConnectionState seems a bit excessive. How about we start with Tuple<NodeId, KeyId> and see if any other information is needed and add in future?

bzbarsky-apple · 2020-12-11T05:16:10Z

examples/common/chip-app-server/Server.cpp

-
-        VerifyOrExit(state->GetPeerNodeId() != kUndefinedNodeId, ChipLogProgress(AppServer, "Unknown source for received message"));
-
-        state->GetPeerAddress().ToString(src_addr, sizeof(src_addr));
-
-        ChipLogProgress(AppServer, "Packet received from %s: %zu bytes", src_addr, static_cast<size_t>(data_len));
-
-        HandleDataModelMessage(header, std::move(buffer), mgr);


Maybe switch this code to reporting the node id instead of the source address (which it can get from the SecureSessionHandle , presumably, at least for a CASE session)? Either way, keeping the "we got N bytes logging" would be useful.

The member of the SecureSessionHandle is not decided yet. I don't want to expose the internal detail of SecureSessionHandle class.

I'm still considering how do we get information from session handle interface. Maybe:

auto session = mSecureSessionManager->GetSession(handle /* an secure session handle */); bool isPaseSession = session->IsPaseSession(); // or session->IsCaseSession() for case check. auto nodeid = session->GetPeerNodeId(); auto localKey = session->GetLocalKeyId(); auto peerKey = session->GetPeerKeyId();

That's fair, but we really will need a way to get the peer node ID for CASE sessions. And we really should keep the length logging here.

Good, I'll make it so.

bzbarsky-apple · 2020-12-11T05:24:40Z

src/controller/CHIPDevice.cpp

+void Device::OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr)
+{
+    mState         = ConnectionState::SecureConnected;
+    mSecureSession = session;


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?

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.

bzbarsky-apple · 2020-12-11T05:25:02Z

src/controller/CHIPDevice.cpp

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


Should mSecureSession be reset in some way?

bzbarsky-apple · 2020-12-11T05:25:20Z

src/controller/CHIPDevice.h

+     * @brief
+     *   Called when a new pairing is being established
+     *
+     * @param state   connection state


There is no such param

bzbarsky-apple · 2020-12-11T05:25:32Z

src/controller/CHIPDevice.h

+     *
+     *   The receiver should release all resources associated with the connection.
+     *
+     * @param state   connection state


No such param...

bzbarsky-apple · 2020-12-11T05:26:44Z

src/controller/CHIPDevice.h

@@ -191,6 +213,8 @@ class DLL_EXPORT Device

    NodeId GetDeviceId() const { return mDeviceId; }

+    SecureSessionHandle GetSecureSession() const { return mSecureSession; }


What will this return if OnNewConnection has not happened yet? Will it even be initialized memory?

I'll remove GetSecureSession. The session should not be exposed.

bzbarsky-apple · 2020-12-11T05:34:55Z

src/controller/CHIPDeviceController.cpp

@@ -395,6 +429,21 @@ void DeviceController::ReleaseAllDevices()
    }
 }

+uint16_t DeviceController::FindDeviceIndex(SecureSessionHandle session)


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.

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).

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

src/controller/CHIPDevice.h

bzbarsky-apple · 2020-12-11T07:25:44Z

src/transport/SecureSessionMgr.h

+{
+public:
+    SecureSessionHandle() : mPeerNodeId(kAnyNodeId), mPeerKeyId(0) {}
+    SecureSessionHandle(NodeId peerNodeId, uint16_t peerKeyId) : mPeerNodeId(peerNodeId), mPeerKeyId(peerKeyId) {}


Should this constructor be protected, so only SecureSessionMgr can use it?

In the future, it is. But for now, InteractionModel need to create it using this one.

kghost · 2020-12-16T05:50:29Z

@pan-apple @andy31415 this one is ready for merge, please do a final check.

In the future, I may need to change the member if SecureSesoinHandle class, since the internal of this class is never exposed, it should be a very easy task.

kghost · 2021-01-11T07:05:50Z

This one is ready for merge

mspang · 2021-01-11T18:40:16Z

This one is ready for merge

@kghost conflict

rwalker-apple · 2021-01-11T21:00:36Z

@jelderton @hawk248 @saurabhst @BroderickCarlin ?

github-actions · 2021-01-12T05:11:50Z

Size increase report for "esp32-example-build" from 4bb0e53

File	Section	File	VM
chip-all-clusters-app.elf	.flash.text	192	192

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,1668
.debug_line,0,801
.debug_loc,0,508
.debug_str,0,333
.debug_ranges,0,312
.flash.text,192,192
.xt.prop._ZN4chip24SecureSessionMgrDelegate19OnConnectionExpiredENS_19SecureSessionHandleEPNS_16SecureSessionMgrE,0,76
.xt.prop._ZN4chip9Transport15PeerConnectionsILj16ELNS_4Time6SourceE0EE23FindPeerConnectionStateENS_8OptionalIyEEtPNS0_19PeerConnectionStateE,0,52
.debug_abbrev,0,24
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,2
.strtab,0,-4
.symtab,0,-32
.xt.lit._ZNK4chip8OptionalINS_9Transport11PeerAddressEE5ValueEv,0,-40
.xt.prop._ZN4chip24SecureSessionMgrDelegate19OnConnectionExpiredEPKNS_9Transport19PeerConnectionStateEPNS_16SecureSessionMgrE,0,-76
.xt.prop._ZN4chip9Transport15PeerConnectionsILj16ELNS_4Time6SourceE0EE23FindPeerConnectionStateEyPNS0_19PeerConnectionStateE,0,-176
.shstrtab,0,-260

github-actions · 2021-01-12T05:12:02Z

Size increase report for "nrfconnect-example-build" from 4bb0e53

File	Section	File	VM
chip-lighting.elf	text	260	260
chip-lighting.elf	shell_root_cmds_sections	12	12
chip-lock.elf	text	260	260
chip-lock.elf	shell_root_cmds_sections	-4	-4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-lighting.elf and ./pull_artifact/chip-lighting.elf:

sections,vmsize,filesize
.debug_info,0,1435
.debug_line,0,588
.debug_str,0,333
.debug_ranges,0,272
text,260,260
.debug_frame,0,92
.debug_abbrev,0,57
shell_root_cmds_sections,12,12
.strtab,0,-4
.debug_loc,0,-153

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,1435
.debug_line,0,588
.debug_str,0,333
.debug_ranges,0,272
text,260,260
.debug_frame,0,92
.debug_abbrev,0,57
.strtab,0,-4
shell_root_cmds_sections,-4,-4
.debug_loc,0,-145

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

yufengwangca · 2021-01-21T19:53:25Z

This change seems break the end-to-end messaging testing which is temporarily disabled in CI.

With this change, the app does not specify the target NodeID, but relay on the following callback to update the SecureSessionHandle.

void OnNewConnection(chip::SecureSessionHandle session, chip::SecureSessionMgr * mgr) override { 
    mSecureSession = session;
}

We can only specify one delegate of SecureSessionMgr at a time, but there are multiple connections managed by the SecureSessionMgr, how we make sure the SecureSessionMgr can pass the generated SecureSessionHandle to the corresponding target of its connection?

For example, there are multiple objects are set as the delegate of SecureSessionMgr, including DeviceController, ExchangeManager and application itself. The later SetDelegate() will overwrite the previous one.

Echo Application:
gSessionManager.SetDelegate(&gTestSecureSessionMgrDelegate); // Set the echo app as the delegate of SecureSessionMgr.
err = gExchangeManager.Init(&gSessionManager); // Set the ExchangeManager as the delegate of SecureSessionMgr.

The echo application won't received the generated SecureSessionHandle and it will sendMessage to garbage target.

yufengwangca · 2021-01-21T19:55:55Z

Register #4451 to track this issue.

boring-cyborg bot added controller transport labels Nov 9, 2020

restyled-io bot mentioned this pull request Nov 9, 2020

Restyle API SecureManager::SendMessage, use PeerConnectionState instead NodeId #3729

Closed

kghost force-pushed the tp-ss branch from 5835c17 to da34367 Compare November 9, 2020 09:11

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, mspang, rwalker-apple and saurabhst November 9, 2020 09:11

kghost force-pushed the tp-ss branch 2 times, most recently from e6e55e3 to 04dadc4 Compare November 9, 2020 09:26

bzbarsky-apple requested a review from pan-apple November 9, 2020 17:20

kghost force-pushed the tp-ss branch from 04dadc4 to d747abf Compare November 12, 2020 09:23

restyled-io bot mentioned this pull request Nov 12, 2020

Restyle API SecureManager::SendMessage, use PeerConnectionState instead NodeId #3794

Closed

kghost force-pushed the tp-ss branch 2 times, most recently from b735d46 to e577fe1 Compare November 12, 2020 09:27

bzbarsky-apple reviewed Dec 11, 2020

View reviewed changes

kghost force-pushed the tp-ss branch from b4a92f7 to dcedec0 Compare December 11, 2020 07:21

bzbarsky-apple reviewed Dec 11, 2020

View reviewed changes

kghost force-pushed the tp-ss branch from 5d937f0 to ae97425 Compare December 11, 2020 16:27

bzbarsky-apple approved these changes Dec 11, 2020

View reviewed changes

kghost force-pushed the tp-ss branch 3 times, most recently from 7ced116 to ac4f84e Compare December 14, 2020 12:56

kghost force-pushed the tp-ss branch from ac4f84e to 9deb2e7 Compare December 23, 2020 06:16

rwalker-apple approved these changes Jan 5, 2021

View reviewed changes

mspang approved these changes Jan 6, 2021

View reviewed changes

kghost force-pushed the tp-ss branch from 9deb2e7 to f91e902 Compare January 8, 2021 07:22

restyled-io bot mentioned this pull request Jan 8, 2021

Restyle API SecureManager::SendMessage, use SecureSessoinHandle instead NodeId #4297

Closed

kghost force-pushed the tp-ss branch from f91e902 to 22c7716 Compare January 8, 2021 08:01

saurabhst approved these changes Jan 11, 2021

View reviewed changes

kghost force-pushed the tp-ss branch from 22c7716 to 7486fe4 Compare January 12, 2021 04:15

restyled-io bot mentioned this pull request Jan 12, 2021

Restyle API SecureManager::SendMessage, use SecureSessoinHandle instead NodeId #4326

Closed

kghost force-pushed the tp-ss branch from 7486fe4 to 5f0e4ab Compare January 12, 2021 04:18

API SecureManager::SendMessage, use SecureSessoinHandle instead NodeId

b1ba67a

kghost force-pushed the tp-ss branch from 5f0e4ab to b1ba67a Compare January 12, 2021 04:51

rwalker-apple merged commit c07d818 into project-chip:master Jan 12, 2021

yufengwangca mentioned this pull request Jan 21, 2021

Need a solution to get correct generated SecureSessionHandle for each new connection #4451

Closed

		@@ -191,6 +213,8 @@ class DLL_EXPORT Device

		NodeId GetDeviceId() const { return mDeviceId; }

		SecureSessionHandle GetSecureSession() const { return mSecureSession; }

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

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

Conversation

kghost commented Nov 9, 2020

Problem

Summary of Changes

kghost commented Nov 9, 2020

andy31415 commented Nov 9, 2020

andy31415 commented Nov 9, 2020

pan-apple commented Nov 9, 2020

kghost commented Nov 9, 2020

pan-apple commented Nov 9, 2020

kghost commented Nov 10, 2020 • edited Loading

rwalker-apple commented Nov 11, 2020

kghost commented Nov 12, 2020

pan-apple commented Nov 12, 2020

Choose a reason for hiding this comment

kghost Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kghost Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kghost Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kghost commented Dec 16, 2020

kghost commented Jan 11, 2021

mspang commented Jan 11, 2021

rwalker-apple commented Jan 11, 2021

github-actions bot commented Jan 12, 2021

github-actions bot commented Jan 12, 2021

yufengwangca commented Jan 21, 2021

yufengwangca commented Jan 21, 2021

kghost commented Nov 10, 2020 •

edited

Loading

kghost Dec 11, 2020 •

edited

Loading

kghost Dec 11, 2020 •

edited

Loading

kghost Dec 11, 2020 •

edited

Loading