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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve TLS 1.3 support #186

Merged
merged 2 commits into from Jan 3, 2019
Merged

Improve TLS 1.3 support #186

merged 2 commits into from Jan 3, 2019

Conversation

@clue
Copy link
Member

@clue clue commented Jan 2, 2019

TLS 1.3 is now an official standard as of August 2018 (https://tools.ietf.org/html/rfc8446) which is great news! 馃帀 See https://wiki.openssl.org/index.php/TLS1.3 if you want to learn more about why this is great news.

OpenSSL 1.1.1 supports TLS 1.3 (https://www.openssl.org/blog/blog/2017/05/04/tlsv1.3/). For example, this version ships with Ubuntu 18.10 (and newer) by default, meaning that recent installations support TLS 1.3 out of the box :shipit:

At the time of writing this, PHP does not know about TLS 1.3 at all. Interestingly, due to the way how PHP interfaces with OpenSSL, this means that TLS 1.3 is in fact enabled by default for all client and server connections when using a recent OpenSSL version (see also #184 for more details).

This PR improves TLS 1.3 support by working around the issues described in #184. In other words, prior to applying this patch, creating a TLS 1.3 connection could result in 100% CPU usage due to a bug in PHP. After applying this patch, this is worked around by consuming all stale data from the TLS receive buffers and as such support TLS 1.3 as well as older TLS versions. Accordingly, the test suite has been updated to add tests for all available TLS versions. The test suite confirms that existing behavior has not changed.

While PHP does not know about TLS 1.3 at the moment, there is however a pending PR that adds full TLS 1.3 support for a future PHP version (php/php-src#3700). This PR is designed in such a way as to be forwards compatible with when PHP receives official TLS 1.3 support and also when the underlying stream issue has been fixed (php/php-src#3729). Again, the test suite covers these details to avoid any future regressions.

Builds on top of #185
Builds on top of reactphp/stream#139
Fixes #184

Construct underlying stream to always consume complete receive buffer.
This avoids stale data in TLS buffers and also works around possible
buffering issues in legacy PHP versions. The buffer size is limited
due to TCP/IP buffers anyway, so this should not affect usage otherwise.

This builds on top of reactphp/stream#139 to
work around a bug in PHP where reading from a TLS 1.3 stream resource
would hang with 100% CPU usage due to the changed TLS 1.3 handshake.
@clue clue added this to the v1.2.0 milestone Jan 2, 2019
This only simplifies some of unneeded assignments for legacy PHP
versions and should not affect usage otherwise. TLS 1.3 is implicitly
available despite being omitted in this assignment. The required crypto
flag is likely going to be added in PHP 7.2.x in the future via
php/php-src#3700 and should thus be covered by
the main crypto method constant in the future already.

Due to the way how PHP interfaces with OpenSSL, this means that TLS 1.3
is in fact already enabled by default when using a recent OpenSSL
version for all client and server connections even for older PHP
versions.
@clue clue requested review from WyriHaximus and jsor Jan 2, 2019
Copy link
Member

@WyriHaximus WyriHaximus left a comment

LGTM 馃憤

jsor
jsor approved these changes Jan 3, 2019
@jsor jsor merged commit 78a2591 into reactphp:master Jan 3, 2019
1 check passed
@clue clue deleted the tls1.3 branch Jan 4, 2019
@clue clue mentioned this pull request Jan 4, 2019
clue added a commit to clue-labs/socket that referenced this issue May 26, 2019
This PR improves the test suite to avoid a possible race condition for
our TLS tests. It does not change anything about the actual behavior or
the expected output, but it helps making the expected output more
explicit and no longer subject to a possible race condition. This helps
avoiding possible false negatives if TLS 1.3 is supported and PHP
reports the EOF indicator before consuming all application data.

This builds on top of reactphp#185 and
reactphp#186
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants