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.

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

Post-landing review comments for PR 10985 #11072

Closed
bzbarsky-apple opened this issue Oct 27, 2021 · 1 comment · Fixed by #11189
Closed

Post-landing review comments for PR 10985 #11072

bzbarsky-apple opened this issue Oct 27, 2021 · 1 comment · Fixed by #11189
Assignees

Comments

@bzbarsky-apple
Copy link
Contributor

bzbarsky-apple commented Oct 27, 2021

Problem

I didn't get to PR 10985 before it landed. Looking at it now, I have the following questions/comments:

  • StaticGroupsProvider::Decrypt should take the PacketHeader as a const reference, not by value. Implement the GroupDataProvider #11075
  • StaticGroupsProvider::Decrypt should almost certainly take the PacketBufferHandle as an lvalue reference, not an rvalue reference, since it's not taking ownership of the buffer, I assume. Implement the GroupDataProvider #11075
  • In SecureUnicastMessageDispatch all the MCSP stuff should go away; we should never be in a situation where !state->GetSessionMessageCounter().GetPeerMessageCounter().IsSynchronized() is true for a unicast message, and if that ever does happen we should just error out. That whole block needs to move into SecureGroupMessageDispatch somewhere.
  • In SecureGroupMessageDispatch if packetHeader.IsSecureSessionControlMsg() we should probably just error out. The only control messages are MCSP messages, which are not group messages. [EDIT: This is not true. SecureGroupMessageDispatch is called for a group session, and MCSP messages do have a group session. What they do not have is a group id to filter possible decryption keys on, but we are not doing anything with group ids here yet.]

Proposed Solution

Fix those.

@jepenven-silabs

@jepenven-silabs jepenven-silabs self-assigned this Oct 27, 2021
@jepenven-silabs
Copy link
Contributor

jepenven-silabs commented Oct 28, 2021

Moving some of those comments to other issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants