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

Add a high-level mirage-compatible module for paf-le #75

Merged
merged 8 commits into from
Jan 10, 2023

Conversation

kit-ty-kate
Copy link
Contributor

No description provided.

@dinosaure
Copy link
Owner

This proposal sounds good to me but I am worried about the dependency graph. Indeed, Paf_le requires multiple dependencies on which LE should not depend (in order to keep the possibility of using the latter with CoHTTP). I would therefore be in favour of making a new sub-package (whose name I couldn't say...).

As soon as I have some time, I could look at the proposal in more detail.

- We renamed the module to `paf-le.mirage` (and follow the same distribution
  pattern of `paf`
- We updated the implementation to use `Lwt.both` and ensure the concurrency
  between the HTTP webserver and our Let's encrypt _asker_. When the second is
  done, we stop the first one and returns the result
- A documentation is added which mentions the required DNS configuration
@dinosaure
Copy link
Owner

I took the opportunity to improve your PR. I will try to figure out about a solution for #76

@dinosaure
Copy link
Owner

So I added a new function with_lets_encrypt_certificates which should fix #76 but I would like a review from @hannesm and I would like to test it. It seems a good first solution for the initial problem.

request) in
Paf.http_service ~error_handler request_handler in

Paf.init ~port:80 (Stack.tcp stackv4v6) >>= fun http ->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this still necessary? I was under the impression that the Alpn protocol would remove the need for a hardcoded HTTP service on port 80.

Copy link
Owner

@dinosaure dinosaure Nov 21, 2022

Choose a reason for hiding this comment

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

The HTTP/80 webserver is mainly here to solve 2 issues:

  • redirect any request from http://hostname/ to https://hostname/
  • still be able to launch the let's encrypt challenge when the certificate expired

EDIT: ALPN just solve the issue of the underlying protocol used to communicate via TLS (and say the we can communicate with http/1.1 or h2. The redirection is still needed on 80 (where ALPN operates on the 443 default TLS port)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess that's where my confusion comes from, eventually @dinosaure as well.

With RFC 8737, let's encrypt allows a ALPN challenge which works as follows:

  • the ACME server (from let's encrypt) connects via http + tls (client) to the domain name of the certificate
  • in this connection, the ALPN tls-alpn-01 is used
  • our server is supposed to use a specific certificate (self-signed with some ACME extension) and private key to show that our let's encrypt client is in control of the web server

Now, with the current setup of TLS and certificate selection, I don't think we can easily get this up.

So, the current approach, as implemented by @dinosaure, looks fine to me. Future work may include to extend the TLS config in respect of certificate selection (though the logic is already pretty complex, unfortunately).

TL;DR: I opened mirleft/ocaml-tls#465 to track the above, and once that is achieved, we can come back to paf.

@kit-ty-kate
Copy link
Contributor Author

kit-ty-kate commented Nov 21, 2022

Apart from me having forgotten the content-length header (i was using cohttp before which did it for me), the new with_lets_encrypt_certificates function seems to work ok (though obviously i haven't left it for long enough for the certificate to be renewed automatically). I'll try to shorten the time tomorrow and see if it works.

lib/lE_mirage.ml Outdated Show resolved Hide resolved
@dinosaure
Copy link
Owner

/cc @hannesm, this is the PR which is needed to improve your about letsencrypt unikernel. This PR is good for you?

@hannesm
Copy link
Contributor

hannesm commented Jan 10, 2023

/cc @hannesm, this is the PR which is needed to improve your about letsencrypt unikernel. This PR is good for you?

yes, this looks fine to me. thanks for working on that :)

lib/dune Outdated Show resolved Hide resolved
paf-le.opam Outdated Show resolved Hide resolved
@dinosaure
Copy link
Owner

Let's merge and cut a release, @kit-ty-kate: I'm not totally sure about the with_lets_encrypt_certificates but I think it should be ok. If you have the time to test it, I will happy to improve it. Thanks both!

@dinosaure dinosaure merged commit 11c4aa3 into dinosaure:master Jan 10, 2023
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Jan 10, 2023
CHANGES:

- Fix memory leak about functor application (@dinosaure, dinosaure/paf-le-chien#78)
- Add a new sub-package `le.mirage` to facilite obtaining a let's encrypt certificate
  and expose few functions to handle Let's encrypt certificates (@kit-ty-kate, @dinosaure, @hannesm, dinosaure/paf-le-chien#75)
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