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.

Sign up for GitHub

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

Jump to bottom

Add Opportunistic TLS implementation #302

Open
wants to merge 1 commit into
base: 1.x
Choose a base branch
from
Open

Add Opportunistic TLS implementation #302

wants to merge 1 commit into from

Conversation

WyriHaximus
Copy link
Member

@WyriHaximus WyriHaximus commented Dec 3, 2022 •
edited
Loading

This PR introduces the functionality required to build opportunistic TLS clients and servers with ReactPHP. It does so by introducing a prefix to tls://, namely opportunistic, to create opportunistic+tls://example.com:5432 for example as the full URL. This will create an OpportunisticTlsConnectionInterface (instead of a ConnectionInterface) that extends the ConnectionInterface and exposes the enableEncryption method to enable TLS encryption at the desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS and enable it when ready.

Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this PR: voryx/PgAsync#52

@WyriHaximus WyriHaximus added this to the v1.13.0 milestone Dec 3, 2022
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch 5 times, most recently from c45aa34 to 6bf2080 Compare December 4, 2022 16:09
@WyriHaximus WyriHaximus mentioned this pull request Dec 4, 2022
Open
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch from 6bf2080 to 454673d Compare December 5, 2022 10:56
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch 2 times, most recently from da6bc3c to 1c1cb5a Compare December 5, 2022 16:02
@WyriHaximus WyriHaximus marked this pull request as ready for review December 5, 2022 16:02
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch 4 times, most recently from 0275788 to 5216b9e Compare December 7, 2022 18:15
@WyriHaximus
Copy link
Member Author

Thanks to a suggestion from @clue I managed to drop the dependency on reactphp/async#65

SimonFrings
SimonFrings reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some remarks and suggestions for the documentation, same suggestions are also important for the doc-blocks inside the added classes.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch from 5216b9e to f86e4f8 Compare December 16, 2022 09:15
@WyriHaximus
Copy link
Member Author

I added some remarks and suggestions for the documentation, same suggestions are also important for the doc-blocks inside the added classes.

@SimonFrings Let me know if you have any more pointers? Will apply them to the docblocks later today

@WyriHaximus
Add Opportunistic TLS implementation
fb5c2e7 
This commit introduces the functionality required to build opportunistic TLS clients and servers with
ReactPHP. It does so by introducing a prefix to `tls://`, namely `opportunistic`, to create
`opportunistic+tls://example.com:5432` for example as the full URL. This will create an
`OpportunisticTlsConnectionInterface` (instead of a `ConnectionInterface`) that extends the
`ConnectionInterface` and exposes the `enableEncryption` method to enable TLS encryption at the
desired moment. Inside this PR is an example of a server and client negotiating when to enable TLS
and enable it when ready.

Opportunistic Security described in RFC7435: https://www.rfc-editor.org/rfc/rfc7435
External PR using the proposed changes in this commit: voryx/PgAsync#52
@WyriHaximus WyriHaximus force-pushed the 1.x-opportunistic-tls-connection branch from f86e4f8 to fb5c2e7 Compare December 20, 2022 09:50
@WyriHaximus
Copy link
Member Author

@SimonFrings Updated the doc blocks

SimonFrings
SimonFrings approved these changes Jan 2, 2023
View reviewed changes
Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for updating, texts and the overall code structure are looking fine 👍

I am not very experienced when it comes to opportunistic TLS, so I can't really review if this is exactly the way to introduce this to the project, but the rest looks good to me.

@WyriHaximus
Copy link
Member Author

Thanks for updating, texts and the overall code structure are looking fine 👍

👍

I am not very experienced when it comes to opportunistic TLS, so I can't really review if this is exactly the way to introduce this to the project, but the rest looks good to me.

The only reason I found out STARTSSL is a flavor of opportunistic TLS is because I started working on voryx/PgAsync#52, did a writedown of the why at https://blog.wyrihaximus.net/2023/01/migrating-from-self-hosted-in-k8s-databases-to-managed-hosted-at-digital-ocean/. That is also why I started with STARTTLS and ended up with opportunistic TLS just providing the barebones to do it and not implement some standard on top of it.

@WyriHaximus
Copy link
Member Author

As discussed, I've extracted this into a package: https://github.com/WyriHaximus/reactphp-opportunistic-tls

@SimonFrings SimonFrings removed this from the v1.13.0 milestone Jun 7, 2023
@mbonneau
Copy link

Hello, is there a roadmap on when this can be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SimonFrings SimonFrings SimonFrings approved these changes

@clue clue Awaiting requested review from clue

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@WyriHaximus @mbonneau @SimonFrings