http: allow HTTP to return an HTTPS server

[brendanashworth]

This commit allows the http module to create a HTTPS server
if TLS options are passed to createServer() or the Server()
constructor.

Adoption of joyent/node#8738. /cc @cjihrig
The second commit contains rebase edits, code style edits, etc that has happened in the past few months.

[rvagg]

this seems brittle to me, perhaps we could consider connecting it with the https module such that when the options are modified there we don't run the risk of this falling out of sync?

[brendanashworth]

That would be a better solution than what is already there, but I don't think there is a global list in tls for options that we could hook into, so the only difference that would be made is changing the location for the list.

As sort of a counter proposal, I'd say we don't check for the TLS option names, rather just assume its TLS if there is an object, because before this it would just throw an error anyways.

[mscdex]

+1 to instantiating an HTTPS server just if the first argument is an object instead of also checking for explicit object properties.

[cjihrig]

FWIW, when I originally wrote this patch, that is how I implemented it. However, @hueniverse, who I wrote this for, asked me to explicitly check for the TLS options.

[silverwind]

The recently added dhparam option for PFS is missing in this list.

[rvagg]

hah, point proven, this is a risk then.

I'm also not convinced that just using the existence of the options object is good API either--what if we want to add options that http.Server cares about? Then we're stuck with having to break API.

[mscdex]

If we're planning on adding an options for http.createServer() in the future, then if nothing else we could just check for one of the required https config options (e.g. pfx and key) to determine if an https.Server should be created.

[silverwind]

The minimum would be either pfx or both key and cert. Certainly better than the full list in terms of maintainability.

[brendanashworth]

What about accepting a { tls: true } parameter in the options?

[piscisaureus]

What about accepting a { tls: true } parameter in the options?

That! No spooky action at a distance please...

[silverwind]

What's the motivation for adding this? I'm not sure it's good to mix up http and https like that, and isn't something like this simple enough?

if (secure)
    require("https").createServer(options, listener);
else
    require("http").createServer(listener);

I could see http#createServer taking a noop first argument (to be used later, maybe with http2) for consistency, so above example could be a one-liner:

require(secure ? "https" : "http").createServer(options, listener);

[brendanashworth]

@silverwind the basis of it all started here. It especially got my attention when I read this comment - I would eventually like to merge the http and https modules, though that would be a very future-istic roadmap sort of view and belong in iojs/NG.

Both modules do basically the same thing, and if a wrapper of TLS is all that the other offers (with a relatively tiny amount of developer-side configuration), I see no reason that they shouldn't be implemented in the same core module.

The best example of this is the http.request() vs https.request() dilemna; just because a server (which may not be out of the developer's hand) prefers to use encryption or not for HTTP requests doesn't mean we should limit the developer to use only a specific module.

[silverwind]

Sounds resonable. We should also think on how we integrate http2, which too can be both with and without TLS. I think it's probably better to not introduce an http2 core module, but rely on options.

I could see options.tls and options.version (or maybe proctolVersion) introduced for that purpose.

[brendanashworth]

@silverwind actually, that does make sense, since from the looks of the http2 discussion most everyone would like it to stay in the same module. I'd say http2 would default to on, and you could disable it with something like { http2: false }. I'd like to see the same for TLS, but I'll wait for a little while before I update commit to solicit comments on the proposal.

[silverwind]

Probably safer to use version over http2 for future versions, but it's off-topic here. Making TLS the default is desireable, but I'd at least wait until letsencrypt or similar launches. Also, most people terminate TLS elsewhere to take load off the io.js process.

As for this PR, my vote is for { tls: true } to prepare for a merge of http and https.

[piscisaureus]

In general, in favor of merging of the http and https modules.
It'd be also nice if http.get("https://foo.com/bar") would work transparently.

[brendanashworth]

Changed to accept an options.tls boolean. How does it look? @rvagg @silverwind @mscdex @cjihrig

@piscisaureus I think I may open an issue on NG for that, with a checklist and stuff.

[rvagg]

I'm +0 on this because the API still seems a little clumsy but that's mainly because we're retrofitting rather than defining something new.

[silverwind]

Implementation looks fine. I think the API is a good compromise.

[cjihrig]

I don't hate the tls option. My issue with it is that if you pass in required HTTPS options, but forget tls, some users will expect to get HTTPS functionality, but won't (granted this is user error, but still). Comparing it to @piscisaureus' http.get() example, that would be like requesting a HTTPS URL, but still going over HTTP without setting a flag.

[silverwind]

In addition to tls, we could check for the presence of the required tls options (key + cert or pfx) to avoid that user error.

[cjihrig]

My choice would be to ditch tls and detect the minimum required TLS options.

[silverwind]

My stance is that a explicit option is cleaner than relying on magic options, and we're going to need one for http2 later too.

[Fishrock123]

I think we should do whatever will cause the least future issues in light of the entire http2 thing.

[brendanashworth]

I'd say that to keep in the vein of a future HTTP2 (which I envision as { version: 2 }, or { http2: true}), that regardless of the default setting, there should be an option passed. How would we "detect" HTTP2?

It would be possible to log a warning to the console if options are passed without a tls option, but I'd rather not have that - in the vein of Golang, if it's worth warning, then it isn't worth running (?).

[silverwind]

log a warning to the console if options are passed without a tls option

I think someone who uses http with options will have read the docs about that change, so it should be clear to him/her that it's necessary to pass tls to actually get TLS.

So, LGTM

[silverwind]

Any more opinions here?

To summarize the issue at hand, we have two proposals on how to discern http from https when doing it through a single module:

require("http").createServer({tls: true, /* https options */}, listener);

or without an explicit option (by detecting the required args key + cert or pfx):

require("http").createServer({/* https options */}, listener);

[tellnes]

This breaks the following:

new http.Server({ tls: true }) instanceof http.Server

Edit: But that did trow before this change, so it should not be a problem.

[Fishrock123]

My understanding is that HTTP/2 requires TLS to be enabled.

As such, I'm -1 on this.

Let's just consolidate all of this in to http in the future, and not add easily outdated options and configuration now.

[brendanashworth]

@Fishrock123 I'm no HTTP2 master, but you do seem to be at least partially right according to the wikipedia page. However, just because HTTP2 requires (or may not?) TLS from the client perspective, the server must still be handled differently.

Currently, many setups use some sort of TLS terminator, such as nginx, which I would say be enough reason to allow for non-TLS HTTP2 servers created. The option of built-in TLS isn't going to become outdated - through that same path, the entire HTTPS module will also become outdated.

Plus, it'd be backwards compatibility to support non-HTTPS connections over HTTP1.1, anyways.

[silverwind]

Regarding encryption: https://http2.github.io/faq/#does-http2-require-encryption

I've searched through https://tools.ietf.org/html/draft-ietf-httpbis-http2-17 but couldn't find any information regarding tls/encryption in general. Maybe this is covered elsewhere? Maybe in the upcoming RFC?

[bnoordhuis]

@silverwind The spec allows unencrypted HTTP/2 but I believe all browser vendors have stated they won't support that. Encryption is effectively mandatory.

[tellnes]

The spec does not require you to use TLS. But if you do, then it has some requirements to the configuration section 9.2.

We actually will need tre different http/2 modes:

• http/2 negotiated with alpn over tls
• http/2 in tls mode but external tls terminator
• http/2 plain text mode (using http/1.1 upgrade)

The http/2 protocol does start with a sequence of 24 octets (the string "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"). This string is designed to confuse http/1/1 parser.

With this preface it should be enough to determine between tls mode and plain text mode. Including the new tls option introduced in this pr, we will have enough information to correctly choose how to handle an incoming request.

Edit: This is a +1 from me to add the tls option since it will be compatible with a future implementation of http/2.

[Fishrock123]

With this preface it should be enough to determine between tls mode and plain text mode. Including the new tls option introduced in this pr, we will have enough information to correctly choose how to handle an incoming request.

Noted. +1 then.

I don't really have an option on { tls: true, /* tls options */} vs {/* tls options */}. Perhaps tls: true will become a little redundant?

[tellnes]

I'm for using the tls flag because I don't like magic options.

Also, if some wrapper code (eg. a library) which adds some default options (eg. adding options.ALPNProtocols) should not automatically trigger tls mode.

[brendanashworth]

@Fishrock123 my stance on the "tls might become a little redundant" is that, yes, it would be redundant - but it is better than magical parsing, as you have a way to override a guess.

At the moment, only two people are for an explicit option (not including me), and one is for magic options. Would anyone else like to volunteer their opinion?

[silverwind]

Maybe the inclusion of the tls option can briefly be voted on in the next TC meeting. Ideally, I'd like to see this merged in the next semver-minor window.

[Fishrock123]

Well, do we have anyone who isn't for the tls flag? I suppose it is ok.

[brendanashworth]

@Fishrock123, @cjihrig is for the magic options - he originally wrote the patch, which used magic options.

[Fishrock123]

Ah.

As a 3rd (weird) case, tls: <t/f> could be optional, making it require tls options when true, and ignoring them when false, but having it default to tls: true when tls options are provided.

[brendanashworth]

@Fishrock123 I think that'd be awkward, personally. IMHO, I think the best solution to this would be rather:

var server = require('http').createServer(function() {});

server.listen(443, {tls: true, ca: ...});

That would require a big structural change to the whole thing, though.

[tellnes]

What about { protocol: 'https' } instead. Thats what eg. https://github.com/mikeal/request is using (indirectly via url.parse). If we in the future also wants to do this change for http.request and http.get, that makes more sense.

HTTP/2 also introduces a pseudo header field named :scheme. It is not restricted to http and https, but we should default to one of them based in if we are talking over tls or not.

[piscisaureus]

At the TC meeting it was suggested to pass all the options to the .listen() call. I am sympathetic to that idea - it feels unlogical to structure the API as http.createServer(options).listen(more_options).

[brendanashworth]

Based on the TC feedback, this PR looks like it'll need some work. It's still a WIP on my part, but if someone wants to take it over before I get to it, feel free.

[tellnes]

I don't see how this can be easily implemented good without breaking something. I propose we work towards an api like this.

var server = http.createServer({ /* accept options here for backwards compatibility */ })

// either this
server.listen({ port: 80 })
server.listen({ port: 443, tls: true, http2: true, cert: '...', key: '...' })

// or like this
server.listen([ { port: 80 }, { port: 443, tls: true, http2: true, cert: '...', key: '...' } ])


// and when you want to stop incoming connections
server.close() // closes all

To do this we need to break server instanceof (tls | net).Server. It will still inherit from EventEmitter. http.Server and https.Server could be the same constructor.

If we think this is to much suggar, I think we should let userland deal with this and keep the separation between http and https.

[brendanashworth]

@tellnes I agree it can't be implemented through .listen() without breaking something and doing it correctly, so I think this needs to be brought back to the think tank for a little while. I'll close this PR for now.

[tellnes]

Maybe we can bring this back to the tsc and they can make a decision if we can break http(s)Server instanceof (tls | net).Server?

[brendanashworth]

@tellnes to be honest, I don't think it should break that (it seems like a logical place to use inheritance), and still think we should consider other ways (if there are any?).