Implement STARTTLS

[ghost]

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.

[jchook]

Hey thanks a ton for doing this.

[ghost]

@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:
a) Drop disabling TLS completely (however I don't see much sense into shipping a half feature)
b) Don't test it?

How do we want to proceed?

[kelunik]

@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.

[WyriHaximus]

@kelunik is it fixable? Or do we need to hope it succeeded?

[kelunik]

@WyriHaximus We need to fix php-src, but I never bothered to do that. But yes, usually the implementation will be hope it succeeds.

[ghost]

@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.

[kelunik]

@CharlotteDunois I don't think so, sorry. If you have time, it'd be great if you can create one.

[ghost]

@kelunik @WyriHaximus @jsor @clue
Tracking at #200 and https://bugs.php.net/bug.php?id=78051!

[ghost]

This PR is ready for review.

The only remaining tasks are:

• Update the unit test to include testing disabling TLS on fixed PHP versions ([PHP Bug] Disabling Encryption succeeds silently #200)
• Update the documentation block of the SecureConnectorInterface to include fixed versions ([PHP Bug] Disabling Encryption succeeds silently #200)

[ghost]

@clue @WyriHaximus How do we want to proceed here?

[WyriHaximus]

@CharlotteDunois will review it tomorrow or the day after. It's bigger then I thought and fell off my rader 🤐

[WyriHaximus]

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

[ghost]

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.

[kelunik]

@WyriHaximus RealExtConnectionInterface 😜

[WyriHaximus]

@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

[ghost]

@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.

[WyriHaximus]

@CharlotteDunois I'd be up for that, you as well @clue and @jsor ?

[clue]

@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!

[ghost]

@clue

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?

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 StreamEncryption into the new connection implementation class). We would still get code duplication and complexity to some extend, but at least we don't have an unnecessary packet abstraction, which doesn't help in anyway.