-
Notifications
You must be signed in to change notification settings - Fork 121
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
feat(s2n-quic-transport): dc Manager #2218
Conversation
enum State { | ||
#[default] | ||
Init, | ||
PathSecretsReady(Flag), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be a bit simpler to just put the Flag
on the Manager
so you don't have to match on it for interests. You also get to use the state transitions macro which can make it a bit clearer which transitions are valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the only thing that is a little weird with this approach is that the dc provider doesn't have the stateless reset tokens ready when the dc Manager is initialized, so the flag would just be Flag::default() and then it would get overwritten with the actual flag later on when path secrets are ready. Not a big deal though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah default should be fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up adding a new state ServerTokensSent
, that the Server transitions to after it has received tokens from the client. I now have the server's flag only activating once it has received tokens from the Client, so that we don't end up in a situation where the Client didn't send tokens or they got lost, and the Server sent tokens and had them acked, and thus thinking it was done.
session: &impl tls::TlsSession, | ||
publisher: &mut Pub, | ||
) { | ||
ensure!(!self.state.is_complete()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also ensure!(self.path.is_some())
? otherwise, we may be emitting an event that doesn't really make sense for the configured environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if path is none, the state moves straight to Complete
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah good call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ended up getting rid of all the ensures and just using the states
Description of changes:
This change wires up the
dc::Endpoint
anddc::Path
types through adc::Manager
. Thedc:Manager
is responsible for completing the dc handshake by transmitting and receivingDC_STATELESS_RESET_TOKENS
and sending notifications to thedc::Path
.The expected handshake with a suitable
dc::Endpoint
and using MTLS looks like:Call-outs:
I ended up using the
Flag
sync to ensure both the client and the server aggressively sendDC_STATELESS_RESET_TOKENS
, as only the Server side could piggyback on theHANDSHAKE_DONE
functionality. I couldn't find a way to useFlag
without copying thestateless_reset::Tokens
from thedc::Path
, but I'm not sure that allocation matters much.Testing:
Added unit tests and an integration to show the dc states changing and the handshake completing in the expected RTTs
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.