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

[Patch v2] Added tls 1.3 support for PHP #3700

Open
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@codarrenvelvindron
Copy link

codarrenvelvindron commented Dec 5, 2018

Compiled/Tested php with openssl 1.1.1/1.1.0 official
This is a clean version of original(in sync with current) with all changes made according to review by @bukka :
Original: #3650

  • Added tls 1.3 support for php
  • Added/Updated test files

Ran make tests - OK
Work done during IETF 103 hackathon

~ codarren at cyberstorm.mu ~

@bukka

This comment has been minimized.

Copy link
Contributor

bukka commented Dec 9, 2018

Look good, just one small NIT. Will try to test it during the week.

P.S. you don't need to create a new PR next time - just squashing (or commit amend for the NIT fix) and then push force should be ok ;)

@codarrenvelvindron codarrenvelvindron force-pushed the codarrenvelvindron:master branch from e8b3722 to 741ff7a Dec 10, 2018

@codarrenvelvindron

This comment has been minimized.

Copy link

codarrenvelvindron commented Dec 10, 2018

@bukka : Done. Thanks for the tip ! :)

@clue clue referenced this pull request Dec 31, 2018

Closed

Improve TLS 1.3 support #184

@@ -171,6 +171,7 @@ typedef enum {
STREAM_CRYPTO_METHOD_TLSv1_0_CLIENT = (1 << 3 | 1),
STREAM_CRYPTO_METHOD_TLSv1_1_CLIENT = (1 << 4 | 1),
STREAM_CRYPTO_METHOD_TLSv1_2_CLIENT = (1 << 5 | 1),
STREAM_CRYPTO_METHOD_TLSv1_3_CLIENT = (1 << 6 | 1),
/* TLS equates to TLS_ANY as of PHP 7.2 */
STREAM_CRYPTO_METHOD_TLS_CLIENT = ((1 << 3) | (1 << 4) | (1 << 5) | 1),
STREAM_CRYPTO_METHOD_TLS_ANY_CLIENT = ((1 << 3) | (1 << 4) | (1 << 5) | 1),

This comment has been minimized.

@kelunik

kelunik Dec 31, 2018

Contributor

These generic constants for any version need to include TLS v1.3, too.

This comment has been minimized.

@bukka

bukka Dec 31, 2018

Contributor

I have requested not to touch those as I don't want to negotiate 1.3 by default. It could break the existing clients so I would like to not do it at least for PHP 7.4. I think we could change that in PHP 8 though.

This comment has been minimized.

@kelunik

kelunik Dec 31, 2018

Contributor

Why not? Significant work has been put into TLS 1.3 to avoid breaking with broken TLS 1.2 implementations.

If you want to avoid TLS 1.3 by default, please update the default wrapper, but these constants specifically are for ANY TLS version, so the patch should reflect that.

@kelunik

This comment has been minimized.

Copy link
Contributor

kelunik commented Dec 31, 2018

Seems like there's a bug in stream_get_contents with TLS 1.3, IMO we should fix that before merging this PR: reactphp/socket#184

clue added a commit to clue-labs/socket that referenced this pull request Jan 2, 2019

Simplify assigning crypto method to include all TLS versions
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 referenced this pull request Jan 2, 2019

Merged

Improve TLS 1.3 support #186

clue added a commit to clue-labs/socket that referenced this pull request Jan 2, 2019

Simplify assigning crypto method to include all TLS versions
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment