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

Be able to specify protocols available for the ALPN server #33

Merged
merged 2 commits into from
Mar 13, 2023

Conversation

dinosaure
Copy link
Contributor

Requested by @kit-ty-kate to be able to specify http/1.1 (x)or h2: it seems that the h2 server has some bugs.

@@ -73,7 +73,7 @@ struct
Alpn.injection;
}

let with_lets_encrypt_certificates ?(port = 443) stackv4v6 ~production config
let with_lets_encrypt_certificates ?(port = 443) ?(alpn_protocols= [ "http/1.1"; "h2" ]) stackv4v6 ~production config

Choose a reason for hiding this comment

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

The order was changed

Suggested change
let with_lets_encrypt_certificates ?(port = 443) ?(alpn_protocols= [ "http/1.1"; "h2" ]) stackv4v6 ~production config
let with_lets_encrypt_certificates ?(port = 443) ?(alpn_protocols= [ "h2""http/1.1" ]) stackv4v6 ~production config

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I would like to keep http/1.1 by default and when the user wants to handle h2, it must write explicitely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So indeed the order is relevant here: a client proposes a list of ALPN in its client hello, and the server selects one ALPN by using a "first match" from the ones configured for the server (in the case above "http/1.1"; "h2" -- previously "h2"; "http/1.1") and the list received by the client.

So, TL;DR: should we prioritize HTTP1.1 or HTTP2? If "it seems that the h2 server has some bugs", maybe it is better to only advertise http1 by default? In any case, we should more clearly document the default setting (including priority) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved the documentation to explain the order of protocols and what is the default value.

@hannesm hannesm merged commit 44812cc into mmaker:master Mar 13, 2023
@hannesm
Copy link
Collaborator

hannesm commented Mar 13, 2023

great, should I cut a release with this change?

@dinosaure
Copy link
Contributor Author

Yes, just to inform @kit-ty-kate, I will delete all of this code from the paf distribution (to avoid copy) when ocaml-letsencrypt will be released.

@dinosaure dinosaure deleted the alpn branch March 13, 2023 13:00
hannesm added a commit to hannesm/opam-repository that referenced this pull request Mar 13, 2023
…tsencrypt-app (0.5.1)

CHANGES:

* letsencrypt-mirage: allow alpn protocols to be specified (mmaker/ocaml-letsencrypt#33 @dinosaure)
dinosaure added a commit to dinosaure/opam-repository that referenced this pull request Mar 21, 2023
CHANGES:

- Upgrade to `mirage-crypto-rng.0.11.0` (@hannesm, @dinosaure, dinosaure/paf-le-chien#85)
- Be able to specify ALPN protocols (@kit-ty-kate, @dinosaure, dinosaure/paf-le-chien#86)
  Also merged into `ocaml-letsencrypt` (see mmaker/ocaml-letsencrypt#33)
- Set the default protocol used for the ALPN negotiation to "http/1.1" (@dinosaure, dinosaure/paf-le-chien#87)
  Also merged into `ocaml-letsencrypt` (see mmaker/ocaml-letsencrypt#33)
- Upgrade `paf` to `h2.0.10.0` (@kit-ty-kate, @dinosaure, dinosaure/paf-le-chien#83)
- Replace `Cstruct.copy` (deprecated) by `Cstruct.to_string` (@dinosaure, dinosaure/paf-le-chien#83)
- Delete `paf-le` package (@dinosaure, @hannesm, dinosaure/paf-le-chien#88)
  Implementations are available via the new package `letsencrypt-mirage`
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.

None yet

3 participants