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

CASE: Send Busy response when we are in the middle of handshake and receives sigma1 #27473

Closed
shubhamdp opened this issue Jun 26, 2023 · 6 comments · Fixed by #28153
Closed
Labels
bug Something isn't working needs triage spec Mismatch between spec and implementation

Comments

@shubhamdp
Copy link
Contributor

Type

Other

Explanation

Once #27226 gets merged, the unsolicited handler for sigma1 will not be unregistered after receiving a sigma1, so we can send the busy response when we receives a sigma1 when we are in the middle of handshake.

Platform

core (please add to version below)

Platform Version(s)

No response

Testing

Manually tested with SDK

(Optional) If manually tested please explain why this is only manually tested

No response

Anything else?

No response

@shubhamdp
Copy link
Contributor Author

shubhamdp commented Jul 6, 2023

While looking into this I had three options... so though of putting up here.

  1. Define a function and do similar stuff what SendStatusReport from PairingSession.h is doing.
  2. SendStatusReport() is not dependent on anything from PairingSession and it just requires exchange context which it takes as an argument, so can we make it static?
  3. Move SendStatusReport() to ExchangeContext besides SendMessage so that we call it using exchange context. And we stop duplicating Sending status response code.

Thoughts?

@bzbarsky-apple
Copy link
Contributor

@shubhamdp I really like option 3. Presumably this will have SendStatusReport taking const Protocols::SecureChannel::StatusReport & or so? Having that be a single utility on exchanges makes a lot of sense.

@bzbarsky-apple
Copy link
Contributor

I guess another option would be to have a SendOnExchange on SecureChannel::StatusReport, but that seems like a bit of a layering issue.....

@shubhamdp
Copy link
Contributor Author

  1. Going with this... and implemented in the PR CASE: Send busy status report if we receive a sigma1 and we are in the middle of handshake #28153
  2. Since SendStatusReport is protected and I did not feel good about making it private, so avoided that path.
  3. I see the cyclic dependency issue so, could not get around it

I'll raise an issue to see if I can optimize the SendStatusReport bits to avoid code duplication.

@bzbarsky-apple
Copy link
Contributor

I see the cyclic dependency issue so, could not get around it

@shubhamdp what was the cyclic dependency?

@shubhamdp
Copy link
Contributor Author

After #28219 got merged, going with option 3, where we add SendStatusReport() API on ExchangeContext.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs triage spec Mismatch between spec and implementation
Projects
2 participants