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

BindingManager invalidates CASE sessions when it shouldn't #18436

Closed
tcarmelveilleux opened this issue May 13, 2022 · 7 comments
Closed

BindingManager invalidates CASE sessions when it shouldn't #18436

tcarmelveilleux opened this issue May 13, 2022 · 7 comments
Assignees
@tcarmelveilleux
Labels
p1 priority 1 work secure channel stale Stale issue or PR V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

In:

void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, FabricIndex fabricIndex)
{
    mPendingNotificationMap.RemoveAllEntriesForFabric(fabricIndex);
    mInitParams.mCASESessionManager->ReleaseSessionsForFabric(compressedFabricId);
}

This invalidates session from a place where this should not be done. The behavior of fabric removal in NOC cluster should suffice, and propagate the necessary session eviction.

Proposed Solution

  • Remove ReleaseSessionsForFabric from BindingManager
tcarmelveilleux added a commit to tcarmelveilleux/connectedhomeip that referenced this issue May 15, 2022
@tcarmelveilleux
Remove unneeded/ambiguous CompressedFabricId usages
cecc951 
Within the SDK, FabricIndex is the only unambiguous identifier.

Many methods that emanated from
FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId, chip::FabricIndex)
perpetuated use of the compressed fabric ID, because at the bottom, we have
CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId)

- Replace CASESessionManager::ReleaseSessionsForFabric argument from
  CompressedFabricId to FabricIndex.
- Rework FabricTableDelegate to remove need to pass any
  CompressedFabricId.
- Replace all downstream usages of CompressedFabricId with FabricIndex
  and FabricTable reference.
- Make FabricTableDelegate calls symmetrical in arguments
- Make FabricTableDelegate an inner class of FabricTable to remove
  a friend relationship
- Clarify when adding a FabricTableDelegate causes its deletion due
  to ownership changes
- Add session resumption state clearing on fabric removal

Fixes project-chip#18435
Issue project-chip#18436
@tcarmelveilleux tcarmelveilleux mentioned this issue May 15, 2022
Merged
tcarmelveilleux added a commit that referenced this issue May 17, 2022
@tcarmelveilleux @restyled-commits
Remove unneeded/ambiguous CompressedFabricId usages (#18459)
4de9692 
* Remove unneeded/ambiguous CompressedFabricId usages

Within the SDK, FabricIndex is the only unambiguous identifier.

Many methods that emanated from
FabricTableDelegate::OnFabricDeletedFromStorage(chip::CompressedFabricId, chip::FabricIndex)
perpetuated use of the compressed fabric ID, because at the bottom, we have
CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId)

- Replace CASESessionManager::ReleaseSessionsForFabric argument from
  CompressedFabricId to FabricIndex.
- Rework FabricTableDelegate to remove need to pass any
  CompressedFabricId.
- Replace all downstream usages of CompressedFabricId with FabricIndex
  and FabricTable reference.
- Make FabricTableDelegate calls symmetrical in arguments
- Make FabricTableDelegate an inner class of FabricTable to remove
  a friend relationship
- Clarify when adding a FabricTableDelegate causes its deletion due
  to ownership changes
- Add session resumption state clearing on fabric removal

Fixes #18435
Issue #18436

* Restyled by clang-format

* Fix clang CI

* Apply review comments

* Restyled by clang-format

* Address review comments

* Remove missed field

* Please the gods of Tidy

Co-authored-by: Restyled.io <[email protected]>
@mrjerryjohns
Copy link
Contributor

We still need to to fix this for V1.0.

@mrjerryjohns mrjerryjohns added the p1 priority 1 work label Jun 23, 2022
@tcarmelveilleux tcarmelveilleux self-assigned this Jul 11, 2022
@andy31415
Copy link
Contributor

V1 review: @mrjerryjohns and @tehampson to review and provide justification if this needs to be a v1.0 blocker.

@mrjerryjohns
Copy link
Contributor

Given the recent changes that @tehampson made to pivot OperationalSessionSetup to now just be relevant during CASE establishment only, this isn't a P0 anymore.

When a fabric is removed, two things happen:

  1. CleanupSessionsForFabric is called, which will expire all sessions in the SessionManager for that fabric.
  2. CASESession::OnFabricRemoved will get called, which will terminate any pending session establishment and as a result, terminate any open OperationalSessionSetup instances as well. Consequently, the call to ReleaseSessionsForFabric in BindingManager is just redundant.

@andy31415
Copy link
Contributor

Decreased blocking status based on comment from @mrjerryjohns above (Jerry added the p1 flag oringally)

@mrjerryjohns mrjerryjohns removed the p1 priority 1 work label Aug 26, 2022
@mrjerryjohns
Copy link
Contributor

Taking off p1 label.

@franck-apple franck-apple added p1 priority 1 work and removed v1.0 master patch acceptable labels Oct 24, 2022
@stale
Copy link

stale bot commented Apr 25, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Apr 25, 2023
@stale
Copy link

stale bot commented May 4, 2023

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tcarmelveilleux tcarmelveilleux

Labels
p1 priority 1 work secure channel stale Stale issue or PR V1.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@tcarmelveilleux @andy31415 @mrjerryjohns @franck-apple