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

Jump to bottom

Need to implement handling of CloseSession status messages #16839

Closed
bzbarsky-apple opened this issue Mar 30, 2022 · 2 comments · Fixed by #19636 or #19645
Closed

Need to implement handling of CloseSession status messages #16839

bzbarsky-apple opened this issue Mar 30, 2022 · 2 comments · Fixed by #19636 or #19645
Labels
commissioning Involves placing devices on the network, initial setup spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Mar 30, 2022 •
edited
Loading

Problem

The spec defines, in "4.10.1.4. CloseSession", a secure channel status code that forces closing of the secure session it's delivered on. This is not implemented, but error recovery during commissioning critically depends on this.

Proposed Solution

What I think we need to do is:

  1. Sort out the actual semantics; see https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/5071 and https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/5072.
  2. For now, assume that we just need to handle CloseSession in unsolicited status messages, which we can do somewhat easily via an unsolicited message handler.
  3. Make sure that as we're closing the session we at least get the MRP ack off first, so the other side doesn't keep retrying. This might already work, possibly; we will need to check. Edit: no MRP involved with CloseSession.
  4. Ensure that when a PASE session is closed via CloseSession any associated fail-safe context is expired (and does its roll-backs as needed).
  5. Ensure that when a PASE session is closed via CloseSession the commissioning window manager is notified so it can go back to listening for PASE session establishment if the commissioning window is still open.

@cecille @tcarmelveilleux @lochan-apple @msandstedt
@bzbarsky-apple bzbarsky-apple added V1.0 spec Mismatch between spec and implementation commissioning Involves placing devices on the network, initial setup labels Mar 30, 2022
@msandstedt
Copy link
Contributor

Make sure that as we're closing the session we at least get the MRP ack off first, so the other side doesn't keep retrying. This might already work, possibly; we will need to check.

Are we talking about the recipient of CloseSession acking? We don't need to:

The CloseSession StatusReport SHALL only be sent encrypted within an exchange associated with a PASE or CASE session. The CloseSession StatusReport SHALL NOT set the R Flag.

@bzbarsky-apple
Copy link
Contributor Author

Ah, perfect. I had missed that part. Then yes, we don't need to worry about acks.

This was referenced Jun 15, 2022
Merged
Merged
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Jun 15, 2022
@bzbarsky-apple
Add handling of CloseSession messages.
865bbda 
To handle a CloseSession status message, we need something with access
to SecureChannel::StatusReport.  That means it can't live in
src/transport or src/messaging, so can't be handled by the session
manager or exchange manager automatically.

So unfortunate as it is, add yet another thing that needs to be
initialized and initialize it once we have an initialized exchange
manager.

Fixes project-chip#16839
@bzbarsky-apple bzbarsky-apple mentioned this issue Jun 15, 2022
Merged
woody-apple pushed a commit that referenced this issue Jun 16, 2022
@bzbarsky-apple
Add handling of CloseSession messages. (#19645)
83a0863 
To handle a CloseSession status message, we need something with access
to SecureChannel::StatusReport.  That means it can't live in
src/transport or src/messaging, so can't be handled by the session
manager or exchange manager automatically.

So unfortunate as it is, add yet another thing that needs to be
initialized and initialize it once we have an initialized exchange
manager.

Fixes #16839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
commissioning Involves placing devices on the network, initial setup spec Mismatch between spec and implementation V1.0
Projects
None yet
Milestone
No milestone
3 participants
@msandstedt @harsha-rajendran @bzbarsky-apple