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

Enable create TLS server without initial context #183

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

felicienfrancois
Copy link
Contributor

Fix #182

@nwgh
Copy link
Collaborator

nwgh commented Apr 21, 2016

So I get that node's https module allows this, but how many things does not having this functionality break? In reality, it makes zero sense to have a server with no SNI contexts for HTTP/2 - since SNI support is a requirement in the RFC. As such, I'm not inclined to allow this (even though it deviates from the https API) without a really good reason.

@felicienfrancois
Copy link
Contributor Author

felicienfrancois commented Apr 22, 2016

This simply allows lazy loading or async loading of SNI contexts,
For example when there are a lot of contexts or when they are not known in advance.

My usecase is a mitm proxy.
But I could give you more usecase if you need it.

Also, I think you're misleading 2 things:

  • The requirement for SNI support (the client must indicates the target Server Name and the server must read this field to answer)
  • SNI Context. which does not exists in RFC. This is a server implementation. In the case of nodejs and http2 they can be hotswitched (added/removed when server is running) so there is no reason for forcing to have at least one at start. Why adding constraint when it is not required?

@nwgh
Copy link
Collaborator

nwgh commented Apr 22, 2016

Thanks, @felicienfrancois for the added context. I got to thinking about this more last night (even before you commented), and what I think we should do is this - remove the requirement for adding the SNI context from http2.Server.create, but in http2.Server.start, do the check to make sure we have at least one SNI context (it doesn't make sense, in my mind, to start a server without some context).

Does that seem reasonable?

@felicienfrancois
Copy link
Contributor Author

@nwgh this is better and it would partly solve my usecase (I still need to start the server after I add the first context. So I will have to store or test if the server have already been started or not).

But I still does not understand why leaving this constraint.
Would it break if we start a server without initial SNI context ?
This is like starting a static server with no files to serve or serving an empty directory.
In a functional point of view it does not makes sense but is it a reason to prevent it ?
There could be yet knowns or unknowns situations where it could be useful or required, for a functional or more probably a technical reason.

The usecases that come to my mind are dynamic contexts, coming from a database for example (one context per client). It would means that the server cannot be started until there is at least one entry in database (i.e. one client) which may be tricky to handle.

Then, I think serving the standard SNI error on serve, is as meaningful as throwing an error on start.

@felicienfrancois
Copy link
Contributor Author

just a friendly bump :)

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