Skip to content

Conversation

@WyriHaximus
Copy link
Contributor

For PHP5.6+ with the new SSL fixes in that version also changed the way some of the constants work some how forcing STREAM_CRYPTO_METHOD_TLS_CLIENT to TLS1.0. The older 5.4 and 5.5 versions of that constant seem to include TLS1.1 and TLS1.2.

I came across this issue when a remote server I was connecting to supported SSLv2, SSLv3, TLS1.1, and TLS1.2 but not TLS1.0. PHP was throwing the following error:

stream_socket_enable_crypto(): SSL operation failed with code 1. OpenSSL Error messages: error:1408F10B:SSL routines:SSL3_GET_RECORD:wrong version number

By explicitly assigning 1.0, 1.1, and 1.2 support to StreamEncryption::method for PHP5.6+ the SSL handshake succeeds and the connections follows it's expected course.

@cboden
Copy link
Contributor

cboden commented Apr 2, 2015

Thoughts on using defined() instead of a version check? I think it would be more reliable.

@rdlowrey
Copy link

rdlowrey commented Apr 2, 2015

Yes, absolutely specify the crypto methods as a bitmask here. In particular you should do this because there was a big discussion on the security mailing list on this topic and the result is you'll get inconsistent behavior between 5.6.0-5.6.6 and 5.6.7+ if you use the tls:// stream wrapper (so just checking the minor version isn't good enough).

Also, I'm going to push for deprecation of all the extraneous encryption wrappers soon (maybe for 7.0 with hopeful removal in 8.0) as they only create confusion. With the ability to specify the protocol flags as a bitmask in 5.6 you always want to do that instead of relying on the stream wrapper to choose your available protocols.

This is always the best option because it removes any ambiguity about which protocols the stream will/won't allow.

@clue
Copy link
Contributor

clue commented Apr 3, 2015

👍 I agree, this should probably check defined() and add all flags to the bitmask.

Also, this should not overwrite the original method definition, so that the default STREAM_CRYPTO_METHOD_TLS_CLIENT is still set either way. This should not have any effect right now, but the documentation suggests that this will also include any future TLS versions.

And while we're at it, I suppose it makes sense to add this as an optional $method argument to the ctor and make your code the default path if nothing is passed explicitly.

@WyriHaximus
Copy link
Contributor Author

@cboden @rdlowrey @clue like cf29c6c?

@clue that makes sense, those who still want SSL3 can do that that way but would you suggest adding $method to the bitmask as I've done with cf29c6c or overwrite it?

@clue
Copy link
Contributor

clue commented Apr 12, 2015

LGTM :shipit:

those who still want SSL3 […]

IMO this PR looks good as is, considering it was only meant to be about a sane default behavior.
Everything else (supporting SSL3) should probably be left up to a dedicated PR anyway 👍

@WyriHaximus
Copy link
Contributor Author

Alright, I'll look into another PR for that later 👍

@cboden cboden merged commit cf29c6c into reactphp-legacy:master Apr 12, 2015
@cboden cboden added this to the v0.4.4 milestone Apr 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants