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

Easier TLS setup #120

Merged
merged 14 commits into from
Apr 19, 2024
Merged

Easier TLS setup #120

merged 14 commits into from
Apr 19, 2024

Conversation

redecs
Copy link
Contributor

@redecs redecs commented Apr 16, 2024

Work to address the easier TLS setup discussed here: #113.

The goals would be to:

  • run Fuego in TLS mode
  • support custom TLS configuration to have easy option to hook TLS library (for ACME protocol TLS managers that can handle everything automatically) not required not, since you can access Fuego's underling http.Server to set the tls.Config directly.

@redecs redecs marked this pull request as draft April 16, 2024 08:50
serve.go Outdated Show resolved Hide resolved
@redecs
Copy link
Contributor Author

redecs commented Apr 16, 2024

I made this a draft until I get a better idea on how the integration with a TLS library would look like.

I're looked at autocert but I found it lacking some features.
The next place I looked is Caddy, and I think certmagic it's a better choice for the average developer that would want TLS (and HTTP/2) with Fuego, while of course keeping any of the functionality out of the Fuego code. So I want to include an example on how a setup with Fuego and Certmagic would look like.

Right now I think both library would work by hooking into the fuego.Server.TLSConfig and I thinking to add a WithTLSConfig option, but I want to build an examples and figure out some details. I'll try go get this done in the following days (I need to get a VPS for testing).

@dylanhitt
Copy link
Collaborator

I made this a draft until I get a better idea on how the integration with a TLS library would look like.

What you've implemented here is fine for the scope of the issue and you shouldn't worry about the other integration in this Pull Request as I'm sure the next PR will have a lot more discussion 😉

serve.go Outdated Show resolved Hide resolved
@redecs
Copy link
Contributor Author

redecs commented Apr 18, 2024

I've added the custom TLSConfig option and removed the RunTLS function I initially added because to me that simplifies the Fuego API for the developers that are using it. We can easily determine if the server should run in TLS mode based on the configured options (via TLSConfig or the provided TLS files). If you don't agree, I think we can roll this back quite easily.

I would like to also contribute on example of TLS server setup to show people how this can be leveraged better but I still have some bits to figure out here.

@redecs redecs marked this pull request as ready for review April 18, 2024 09:23
@redecs redecs requested a review from dylanhitt April 18, 2024 09:25
serve_test.go Outdated Show resolved Hide resolved
serve_test.go Outdated Show resolved Hide resolved
serve.go Outdated Show resolved Hide resolved
@redecs redecs requested a review from dylanhitt April 19, 2024 10:29
@redecs
Copy link
Contributor Author

redecs commented Apr 19, 2024

Added RunTLS back in and removed the options I've previously added. Proper testing for TLS Server. I think we need to updated the test for Run to use a real http client if we want to really test the Run method, not just s.Mux, but I haven't made that change yet. I did add graceful shutdown for both HTTP and HTTPS servers to make sure there are not left overs between test (I had issues when I was running the full test suite).

* certmagic poc

* add missing flag parse

* add with tls config from acme client

* listen on http too

* manage sync + https redir

* updated acme tls example

* acme tls update with comments

* remove debug log from acme-tls example

* acme-tls RunTLS
@redecs
Copy link
Contributor Author

redecs commented Apr 19, 2024

Also pushed my working example for TLS with ACME Server (Let's Encrypt Staging) using certmagic. Let me know if think it's useful otherwise I can remove it.

@dylanhitt
Copy link
Collaborator

Nice, do you mind submitting a separate PR for the example. It's easier on the repo history and makes things easier to find.

@redecs
Copy link
Contributor Author

redecs commented Apr 19, 2024

@dylanhitt removed the example, will open a new PR with that after we are done with this one, since it requires the code.

@dylanhitt dylanhitt merged commit 79d2c63 into go-fuego:main Apr 19, 2024
3 of 4 checks passed
@redecs redecs deleted the eaasier-tls branch April 19, 2024 15:38
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.

2 participants