Implement STARTTLS #199
Implement STARTTLS #199
Conversation
|
Hey thanks a ton for doing this. |
|
@clue @jsor @WyriHaximus I need some quick feedback. Currently the only blocker for this PR is the failing unit test for disabling TLS and I don't see currently how I could test this. Does anyone have any insight how we have to test this? If we can't test this, I see two ways: How do we want to proceed? |
|
@CharlotteDunois Disabling TLS is kind of broken, it always returns immediately. However, TLS connections should still shutdown correctly, otherwise the other side might see it as truncation attack. |
|
@kelunik is it fixable? Or do we need to hope it succeeded? |
|
@WyriHaximus We need to fix php-src, but I never bothered to do that. But yes, usually the implementation will be hope it succeeds. |
|
@kelunik Is there a PHP bug report already? A quick search on bugs.php.net did not seem to reveal anything. I think the way to go here would be to skip the relevant unit test, with a note to the bug report and open a tracking issue for this on this repository. |
|
@CharlotteDunois I don't think so, sorry. If you have time, it'd be great if you can create one. |
ghost
mentioned this pull request
|
@kelunik @WyriHaximus @jsor @clue |
|
This PR is ready for review. The only remaining tasks are:
|
ghost
marked this pull request as ready for review
|
@clue @WyriHaximus How do we want to proceed here? |
|
@CharlotteDunois will review it tomorrow or the day after. It's bigger then I thought and fell off my rader 🤐 |
| * @param bool $flag | ||
| * @return void | ||
| */ | ||
| public function setTLSEnabledFlag($flag); |
There was a problem hiding this comment.
Not sure how I feel about this interface. Have you considered splitting this into two different interfaces? ExtConnectionInterface feels rather general and we might need a ExtExtConnectionInterface later on if we'd decide to expand the interface again. /cc @clue
There was a problem hiding this comment.
We could make two interfaces yes, and at this point we should also split Connection into two classes - one for TCP and one for UDS.
There was a problem hiding this comment.
@WyriHaximus RealExtConnectionInterface 😜
There was a problem hiding this comment.
@kelunik DontDrinkTheCoolAidRealExtConnectionInterface 😎
@CharlotteDunois Didn't think that far ahead tbh. The thing that mainly bugs me is that I would hate to to see ExtExtExt interfaces down the line in a while
There was a problem hiding this comment.
@WyriHaximus Well I did. We can certainly split this off and give better names. However we do need to split the implementation class too when we go that route. I'm willed to create a followup pr for this.
ghost
mentioned this pull request
There was a problem hiding this comment.
@CharlotteDunois Thanks for working on this high-quality PR and for your patience! 👍
I think you did an excellent job at working within the existing API constraints and providing this feature without introducing a BC break. That being said, the interface feels a bit heavy and adds some non-trivial complexity to the code base.
In particular, while I do understand the motivation, I do not feel comfortable with exposing the underlying socket resource through our interfaces because it could easily be changed in a way to break this package (e.g. think calling stream_set_blocking()).
I'm not sure what the best path to proceed here is, but I wonder if it makes sense to compare this with a different implementation approach (just thinking out loud here): How about providing some interfaces so we can write something like $connection->write(new TlsHandshake()) on a plaintext TCP/IP connection to then start the TLS negotiation internally and provide some APIs to wait for a successful event? I don't want to get this PR sidetracked, perhaps we should continue this discussion in the feature ticket #89 instead?
Either way, thank you for working on this and providing this high-quality changeset! I'm looking forward to discuss this further to get this feature in and can see a number of use cases for this! 
While this does make to some extend sense, we will have new problems here: Either we need to expose the underlying stream resource anyway, so we can have the "tls handshake packet" do the handshake, or the connection implementations needs to know what it has to do with each packet. And then we are at the other problem: While we have a packet, the connection implementation still needs to contain all the necessary logic and code to do what the packet intends to do. That means code duplication and complexity, while gaining little to nothing with packets. If we do not want to expose the stream resource at all (it does make sense in some way and the library shouldn't stand in the way of advanced users), the only alternative would be a native TCP connection class, which knows how to do a TLS handshake (merging |
ghost
closed this
This PR implements STARTTLS and exposes the connection stream in a non-BC way through a getter method. The added "extended" interfaces should be merged into the "regular" interfaces with the next major version.
Maybe some more documentation is needed in case of the usage/requirement of the extended connection interface instead of the regular interface (e.g.
SecureConnector::connect).Closes #89.