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

ServerHandler: do not send pings unless the TLS handshake has completed. #3303

Merged
merged 1 commit into from Jan 13, 2018

Conversation

@mkrautz
Copy link
Member

commented Jan 13, 2018

sss123next in #3294 reports that the current ping
timer logic causes problems with slow TLS handshakes, such as when
connecting to servers with large DH parameters.

This commit ensures pings are not sent before the TLS handshake has
completed.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2018

This was tested against a Grumble instance built against a modified crypto/tls that sleeps during the handshake. I verified that isEncrypted() returns false when the handshake has not finished, and returns true when the handshake has completed.

ServerHandler: do not send pings unless the TLS handshake has completed.
sss123next in #3294 reports that the current ping
timer logic causes problems with slow TLS handshakes, such as when
connecting to servers with large DH parameters.

This commit ensures pings are not sent before the TLS handshake has
completed.

This was tested against a Grumble instance built against a modified
crypto/tls package that sleeps during the handshake. I verified that
isEncrypted() returns false when the handshake has not finished, and
returns true when the handshake has completed.

@mkrautz mkrautz force-pushed the mkrautz:serverhandler-timer-fixes branch from 3601e2d to d188861 Jan 13, 2018

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2018

Added my testing methodology to the commit message.

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Jan 13, 2018

@hacst
hacst approved these changes Jan 13, 2018
Copy link
Member

left a comment

LGTM

@mkrautz mkrautz merged commit 2c24ee0 into mumble-voip:master Jan 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.