Skip to content

Conversation

@clue
Copy link
Contributor

@clue clue commented Sep 6, 2015

  • TLS endpoints do not have to match connection endpoints (proxy setup)
  • More SOLID design, better separation of concerns

This change also makes it easy to introduce additional SSL/TLS context options in the future.
Required for SOCKS proxy (clue/reactphp-socks#28) and eventually HTTP proxy (reactphp/http-client#44).

* TLS endpoints do not have to match connection endpoints (proxy setup)
* More SOLID design, better separation of concerns
@WyriHaximus
Copy link
Contributor

How can we make this work well with #17?

@clue
Copy link
Contributor Author

clue commented Sep 6, 2015

How can we make this work well with #17?

Afaict this isn't (directly) affected. I've updated #4 to list some concerns (separation of concerns, API design, usability etc.) with the initial approach in #17, so I'm not sure #17 should block this feature.

This change also makes it easy to introduce additional SSL/TLS context options in the future

Personally, I would suggest building SSL/TLS context options on top of this changeset, also for the following reason:

Required for SOCKS proxy (clue/reactphp-socks#28) and eventually HTTP proxy (reactphp/http-client#44).

The underlying TCP/IP connection endpoints (handled via Connector) don't necessarily represent the TLS endpoints:

Client A connects to proxy B which connects to target C

In this scenario, client A needs to establish an underlying TCP/IP connection to proxy B while its TLS endpoint would be target C. See also the linked tickets for some background.

I'll look into filing an alternative to #17 in the next days :)

@clue
Copy link
Contributor Author

clue commented Sep 7, 2015

I'll look into filing an alternative to #17 in the next days :)

See linked WIP PR #45.

Is there anything else I can do to help this PR progress faster? :-)

@clue clue mentioned this pull request Sep 7, 2015
@clue clue added this to the v0.4.5 milestone Sep 23, 2015
@clue
Copy link
Contributor Author

clue commented Sep 23, 2015

Is there anything else I can do to help this PR progress faster? :-)

Ping @WyriHaximus, @cboden

@WyriHaximus
Copy link
Contributor

@clue alright good makes sense LGTM 👍

cboden added a commit that referenced this pull request Sep 24, 2015
Move SSL/TLS context options to SecureConnector
@cboden cboden merged commit 5e3c4c6 into reactphp-legacy:master Sep 24, 2015
@clue clue modified the milestones: v0.5, v0.4.5 Nov 21, 2015
@clue clue deleted the secure-context branch March 31, 2016 17:46
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.

3 participants