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

Uses Herd or Valet certificate when available #46

Merged
merged 5 commits into from
Feb 25, 2024
Merged

Conversation

joedixon
Copy link
Collaborator

@joedixon joedixon commented Feb 20, 2024

This PR improves SSL support for local development.

Those who use herd secure or valet secure for local development will already have access to an SSL certificate which is trusted by their machine meaning they don't need to instruct their browser to trust it as is the case with self-signed certificates.

In most cases, Reverb will start on 0.0.0.0 or localhost, which is also where sites served by Herd or Valet will resolve. This means, assuming a site is running on example.test, Reverb will be accessible from both ws://0.0.0.0:8080 and ws://example.test:8080. As such, we can use the existing certificate for example.test when instantiating the Reverb server.

To make this work, we need to know on which site the user wishes to make Reverb accessible. This can be set using the new REVERB_SERVER_HOSTNAME environment variable or by passing the --hostname option when starting the command.

I made the decision not to default the hostname to the APP_URL even though it seems like this would be the most likely setting in local development. However, in production where Reverb is likely to be running on a different hostname to the app behind a reverse proxy, this doesn't make sense. Additionally, a hostname is not needed when running without TLS locally, which I assume is most likely to be the case.

When a hostname is discoverd by Reverb, it will look for a matching certificate in the certificate directory of both Herd and Valet, using the certificate file paths when instantiating the server.

@mpociot
Copy link

mpociot commented Feb 23, 2024

This looks good to me - the only thing I stumbled upon when reviewing this was that the $hostname variable/argument is passed to the $url arugment of the ServerFactory, which I found a bit misleading at first. Maybe it makes sense to rename the $url variable to $hostname in the server factory as well?

@joedixon
Copy link
Collaborator Author

Maybe it makes sense to rename the $url variable to $hostname in the server factory as well?

Thanks @mpociot, totally agree with you. Updated accordingly.

@joedixon joedixon marked this pull request as ready for review February 23, 2024 13:39
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