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

Set a default ssl.peer_name context in StreamHandler #2988

Merged
merged 2 commits into from Mar 20, 2022

Conversation

TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Feb 24, 2022

This is required when using the force_ip_resolve option with the stream
handler:

As that option will cause the StreamHandler to manually resolve the hostname
and then replace the hostname with the resolved IP address in the URI, PHP
will use that IP address by default in the SNI of the TLS handshake.

Set an explicit ssl.peer_name within the stream's context based on the hostname
in the URL to fix this.

Setting a proper SNI is independent from TLS certificate validation, thus this
value must not be dependent on the verify option.

A test cannot be added, due to a lack of TLS support with the current testing
infrastructure. TLS support cannot easily be added, because it would require a
separate port and also certificates that would need to be commited to the
repository. However correctness can be verified by setting force_ip_resolve
to v4 and attempting to make a request to https://www.example.com/. It will
fail without this commit and work with.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 13, 2022

What if Guzzle follows a redirect to another host. Won't this break that?

@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 13, 2022

It should not: getDefaultContext already is responsible for setting the request headers (including host and possible authorization).

However I did not test this. If you'd like to hold off merging this, until I tested this: This is fine for me.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 13, 2022

Thanks. Let us know the outcome, please. :)

@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 13, 2022

Will do, but this is not going to happen today. I will try to get around to it next week, but can't promise anything.

If you planned to release today, then please don't wait for this. I don't think this fix is super important, I just noticed this issue by accident while working on #2989.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Mar 13, 2022

No worries, and no, we don't plan to release today. Maybe we will do releases on this repo and the psr7 repo, this time next weekend.

@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 14, 2022

I've added a new TestCase that actually performs networking against third party services, it needs to be explicitly enabled by setting the GUZZLE_TEST_ALLOW_NETWORK environment variable.

With that I could confirm that redirects work as expected.

TimWolla added 2 commits Mar 14, 2022
This is required when using the `force_ip_resolve` option with the stream
handler:

As that option will cause the StreamHandler to manually resolve the hostname
and then replace the hostname with the resolved IP address in the URI, PHP
will use that IP address by default in the SNI of the TLS handshake.

Set an explicit ssl.peer_name within the stream's context based on the hostname
in the URL to fix this.

Setting a proper SNI is independent from TLS certificate validation, thus this
value must not be dependent on the `verify` option.

A test cannot be added, due to a lack of TLS support with the current testing
infrastructure. TLS support cannot easily be added, because it would require a
separate port and also certificates that would need to be commited to the
repository. However correctness can be verified by setting `force_ip_resolve`
to `v4` and attempting to make a request to `https://www.example.com/`. It will
fail without this commit and work with.
@TimWolla TimWolla force-pushed the force-ip-resolve-stream branch from fff54b0 to 35b5a09 Compare Mar 14, 2022
@TimWolla
Copy link
Contributor Author

TimWolla commented Mar 14, 2022

On an unrelated note: Should the 8.0/8.1 CI be set to 'Required' in the repository settings?

Nyholm
Nyholm approved these changes Mar 20, 2022
Copy link
Member

@Nyholm Nyholm left a comment

Thank you

@Nyholm Nyholm merged commit 8e4a4cd into guzzle:master Mar 20, 2022
22 checks passed
@TimWolla TimWolla deleted the force-ip-resolve-stream branch Mar 20, 2022
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.

None yet

3 participants