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

Don't set hq as the default ALPN protocol in quinn-proto #32

Closed
djc opened this issue Oct 3, 2018 · 7 comments
Closed

Don't set hq as the default ALPN protocol in quinn-proto #32

djc opened this issue Oct 3, 2018 · 7 comments
Labels

Comments

@djc
Copy link
Collaborator

djc commented Oct 3, 2018

Leave this to higher API levels to decide.

@djc djc added good first issue Good for newcomers Hacktoberfest labels Oct 3, 2018
@stammw
Copy link
Contributor

stammw commented Oct 5, 2018

Hello,

I'd like to try to solve this one !

If understand well, this ALPN parameter could be set in endpoint building code like so:

builder
    .logger(log.clone())
    .config(quinn::Config {
        max_remote_bi_streams: 64,
        alpn_protocols: vec!["hq-11"],
        ..Default::default()
}).listen();

or

let mut builder = quinn::Endpoint::new();
builder.set_alpn_protocols(vec!["hq-11"]);

These two options would let a possibility for the the user not to give any ALPN value. Is that desirable, or is it better to make this parameter mandatory?

@djc
Copy link
Collaborator Author

djc commented Oct 5, 2018

Hi @stammw, that would be great! I think we would want a method on EndpointBuilder that operates similar to set_certificate() and enable_keylog(): something that mutates the ClientConfig contained in the existing Config. @Ralith any thoughts?

@stammw
Copy link
Contributor

stammw commented Oct 5, 2018

Then TLS's ALPN parameter would be left empty when this method has never been called ?

stammw pushed a commit to stammw/quinn that referenced this issue Oct 5, 2018
stammw pushed a commit to stammw/quinn that referenced this issue Oct 5, 2018
@djc
Copy link
Collaborator Author

djc commented Oct 5, 2018

Yeah, I think that would be okay for some use cases, but I'll await the other reviewer.

@Ralith
Copy link
Collaborator

Ralith commented Oct 5, 2018

Yes, to the best of my knowledge ALPN is an optional part of the handshake.

djc pushed a commit that referenced this issue Oct 12, 2018
@djc
Copy link
Collaborator Author

djc commented Oct 12, 2018

This has been fixed in #47. Thanks, @stammw!

@djc djc closed this as completed Oct 12, 2018
@stammw
Copy link
Contributor

stammw commented Oct 12, 2018

Thank you both for all the time spent reviewing and helping !

twilco pushed a commit to twilco/quinn that referenced this issue Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants