feat: add HTTPS and HTTP/2 support for adapter-node

[aradalvand]

Closes #1451
Closes #2190

More of a proof of concept right now, just want to know if we're on the same page regarding the approach here, before I add tests and so on. Let me know your thoughts.

There is a type error when trying to pass app.handler to the http*.createServer methods, which I've filed an issue for in the Polka repo here. It does actually work though, Polka's typing just seems to be slightly off.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 083af9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/adapter-node	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ghostdevv]

Is there a reason for not using options.server? I notice the official https example also doesn't use it

We should probably also add tests if possible, and somehow fix this type issue since lint is failing - happy to take a look at the types issue if you like

[aradalvand]

Is there a reason for not using options.server? I notice the official https example also doesn't use it

@ghostdevv That's a good point, I did actually consider that. In Polka's official HTTPS example, I think using options.server would've been functionally identical, but the downside in our case is we'd have to create two Polka instances, each with a different server; whereas this way we only create one, and then just pass its app.handler to the different server instances we create. Let me know what you think.

We should probably also add tests if possible

Sure. I noticed that currently in the adapter-node tests, a 'mock' server is being used, so the code in the index.js file apparently isn't being tested at all. I could use some guidance on how to actually do this.

and somehow fix this type issue since lint is failing

I'm hoping lukeed/polka#194 gets fixed soon but if it doesn't, maybe we can silence these errors for the time being. We can potentially also use the options.server approach as a way of avoiding these type errors, since the typing there seems to be fine.

happy to take a look at the types issue if you like

That would be amazing.

[ghostdevv]

happy to take a look at the types issue if you like

The main issues with the types seems to be that req is typed as http.IncomingMessage for http/https and http2.Http2ServerRequest for http. There is also the case of none of the servers providing a next function. I think the only way to do "fix" this is with a // @ts-expect-error, and hopefully @lukeed can take a look at the issue you filed soon.

Sure. I noticed that currently in the adapter-node tests, a 'mock' server is being used, so the code in the index.js file apparently isn't being tested at all. I could use some guidance on how to actually do this.

Actually I think we can leave tests for now, they seem to be disabled for adapter-node

[aradalvand]

The main issues with the types seems to be that req is typed as http.IncomingMessage for http/https and http2.Http2ServerRequest for http. There is also the case of none of the servers providing a next function.

@ghostdevv Thanks! It does seem to need fixing on the part of Polka then.

I think the only way to do "fix" this is with a // @ts-expect-error, and hopefully @lukeed can take a look at the issue you filed soon.

Yeah I agree. Done.

Actually I think we can leave tests for now, they seem to be disabled for adapter-node

Ah okay, good to know.

[ghostdevv]

LGTM, we should wait for some other maintainers to take a look (might take a little bit as most people are focused on svelte 4 atm) 🙏

[dummdidumm]

To me the change is acceptable given that it doesn't really bloat the package in size or complexity. I know that @Conduitry and @dominikg both had reservations against this change though, so pinging them here

[dummdidumm]

Suggested change

	> We recommend using a reverse proxy instead and only use these options when you have no other possibilities of securing the connection

[aradalvand]

Added a slightly reworded version of this in 083af9a

@dummdidumm Also, one little question: I just noticed that variables like export const cert_path = env('CERT_PATH', false); are actually exported, so according to the style guide they should be camelCase, am I right?

• Internal variables are written with snake_case while external APIs are written with camelCase

Do these count as "external APIs"?

[ghostdevv]

I think they should be camel case here yes, good catch

[benmccann]

personally I think the recommendation should be in the opposite direction. this seems better than a reverse proxy to me as I explained here: #10183 (comment)

perhaps we should just remove this section if we can't agree on a recommendation? or perhaps we can list pros and cons rather than just making a blanket statement?

[aradalvand]

Hi @dummdidumm. Regarding the point @Conduitry and @dominikg made in #2190 about a reverse proxy being preferable, they're generally right, but I did try to respond in that same issue here. I quote myself:

As for the reverse proxy argument, you could very well have a reverse proxy that isn't located on the same network and communicates with your app via an untrusted channel like the internet (e.g. CDN edge servers), meaning you would still want secure (HTTPS) connections between those proxies and your application; but having to introduce another proxy on the same server just to enable HTTPS feels unideal, especially given that Node already has support for this and therefore not much needs to be done on the part of SvelteKit/adapter-node in order to support it out of the box.

So, again, although the idea that you should normally opt for a reverse proxy and not expose your SvelteKit app directly to the outside world is totally correct and understandable, as I explain here, there are still valid use cases where you're not exposing your SvelteKit app to the outside world directly, but to other proxies elsewhere, in those cases, wanting to equip the SvelteKit app with HTTPS is desirable and reasonable.
I think adding a note in the documentation about the reverse proxy thing, as you suggested, will be helpful.

It's almost guaranteed that feature requests for HTTPS and HTTP/2 — HTTP/2 requires HTTPS so the two issues are linked — will come up again in the future for adapter-node; this PR isn't introducing too much complexity as you acknowledged, yet covers users who'll need this functionality once and for all.

[benmccann]

lgtm

I will also note that a reverse proxy leaves some of your internal traffic unencrypted. E.g. one of my past employers was hacked by the Chinese government and they targeted unencryped internal traffic. So from a security perspective it's better to terminal SSL at the app server than reverse proxy.

[dominikg]

You can put the reverse proxy on the same machine. IF this makes it in, it needs to come with significant noticable warnings about key handling and keeping node up2date.

Protecting inexperienced users from enabling it and fudging something is important. I still believe this should live in a community supported adapter-node-ssl (that comes with the aforementioned warnings)

Or is there docimentation/proof that node SSL/TLS is as secure/convenient/performant as nginx or caddy?

[Conduitry]

I agree with @dominikg. I don't think this is something we want to have as a blessed solution.

Someone could make their own adapter, but they wouldn't even have to. You can create an HTTPS server today by using a custom server entrypoint that uses the generated handler function, and then publish that entrypoint for other people to use.

[dummdidumm]

But what is the benefit of that? The additional code is very small, so why make people jump through that additional hoop?

[Conduitry]

We want people to jump through extra hoops because I don't think we want to be blessing TLS in Node. Even if we put a big warning, people are going to use it because they don't want to set up a proper TLS termination proxy, and they and their app will be in a worse position for it.

The small amount of extra code here isn't the only thing that people should be adding if they are doing this. As Dominik mentioned above, they also need to worry about key handling (if you're worried about attackers who have already gained access to the machine, but you have your private key in a file on the disk, that's not very helpful). You need to worry about renewing your certificates. You need to worry about keeping Node up to date because you're using its statically linked version of openssl.

Something else I've just noticed is that, as written, I believe this PR will simply also server your app over HTTP, which is bad. There's no option to redirect from HTTP to HTTPS. There are so many things that there is already the perfect solution for - a TLS termination proxy - and we don't want it to be our responsibility to do them correctly, or to open up apps to various issues if people trusted us to provide them with good defaults.

[aradalvand]

...You need to worry about renewing your certificates. You need to worry about keeping Node up to date because you're using its statically linked version of openssl.
...There's no option to redirect from HTTP to HTTPS.

@Conduitry As I pointed out before, the primary use case for this isn't when your Node web server is user-facing — the warning, plus the lack of things like HTTP-to-HTTPS redirection and so on, do a good job of deterring people from using it that way — the primary use case is when you want encrypted connections between your app and your proxies, currently this can't be done unless you deploy an additional proxy on the same server as your application — which in this case introduces unnecessary indirection, complexity, and an additional point of failure. and overall just one more thing to worry about — so concerns like HTTP-to-HTTPS redirection, certificate renewal, and so on, aren't really relevant to the actual use case this is meant for.

... I believe this PR will simply also server your app over HTTP, which is bad.

You can disable the plain HTTP endpoint via the ONLY_HTTPS environment variable.

we don't want it to be our responsibility to do them correctly

Most of it isn't really "our" (as in SvelteKit's) responsibility, 99% of the work is done by Node anyway, there's not much we have to do correctly, just some pretty basic configuration; and isn't it reasonable to assume that the Node folks have done their homework and implemented this correctly?

[dominikg]

As I pointed out before, the primary use case for this isn't when your Node web server is user-facing

your usecase is not. But it would be (mis-) used by a lot more people than used correctly.

99% of the work is done by Node anyway

No, it does the bare minimum. Securely handling certificates and updating them would be left as an exercise for our users.

This is a rather rare usecase which has industry standard solutions and alternatives. We have to weigh the benefit of this addition for few users against the risk and additional future work cost in form of maintenance and support/help.

[benmccann]

I don't understand the objection about key handling. It's been several years since I setup a reverse proxy, but the way I remember that working you had your keys on disk in the same manner that you would with this solution. Is there some other standard and recommended way to setup a reverse proxy that handles keys in any different manner than they would be with this solution?

The other objection is that you'd have to update your Node.js version in response to any security advisories, but that's already the case today, so I don't see how this PR would change anything in that respect.

[Conduitry]

If the desire is to encrypt the connection between the Node server and the user-facing proxy, then that can also be achieved by them living on the same machine. If an attacker has access to the user running the Node server where the SSL keys live on disk, you're toast. If you concern is an attacker gaining access to other users on that system and still being able to connect to the unencrypted HTTP connection, then you can expose your unencrypted app as a Unix domain socket (#2048) rather than listening on a port.

Of course, the other other option all along is to do this in a custom server entrypoint. If someone really is in the niche use case where they want the underlying app always served over HTTPS, then they presumably will know it, and will be able to trivially set up a custom entrypoint that does exactly that - without adding a bunch of new runtime configuration options to the adapter, and without tempting people to use it as their user-facing app server.

[dummdidumm]

As an alternative to this should we then enhance the docs if the node adapter to go into more detail of what we just talked about?

[aradalvand]

If the desire is to encrypt the connection between the Node server and the user-facing proxy, then that can also be achieved by them living on the same machine.

@Conduitry Not sure what you mean?! I'm obviously talking about cases where the proxy lives elsewhere, and I gave CDN edge servers as an example. It looks like you haven't read this comment.

If an attacker has access to the user running the Node server where the SSL keys live on disk, you're toast

If an attacker has access to your server, you're screwed anyway, the SSL keys are the least of your problems.

[lukeed]

Pretty sure you can bypass the ts-expect-errors by creating server via native modules (as you are) and then passing through options.server to Polka. The type errors are there cuz of Node's complex types.

let server = certPath && certKeyPath
  ? (noHttp2 ? https.createServer({ ... }) : http2.createServer({ ... })
  : http.createServer();

let app = polka({ server });

[aradalvand]

@lukeed This was brought up and I explained why I decided against it here.

[lukeed]

I saw but it's invalid because there's only 1 Polka instance & 1 server needed

[aradalvand]

@lukeed Not that it matters anymore but that's incorrect, you could have 2 servers, a plain HTTP one and an HTTPS one; hence the intention to reuse the Polka instance.

[Conduitry]

Sorry, I'm vetoing this.

Despite whatever warnings we put, people will absolutely use this for use cases other than having an encrypted connection between the application and the user-facing proxy. Many more people will use it incorrectly than will use it correctly. Electrical outlets don't come with a fork just because there's some conceivable good use for the fork. If you're sure you have a good use, then I'm sure you have your own fork.

If you want to serve HTTPS from your app, then this is trivial, as previously mentioned, using a custom server entrypoint.

If you want communication between your various internal services to be encrypted, it sounds like you want a service mesh.

[xamir82]

@Conduitry Then close #2190 and #1451 too.

[dominikg]

It's almost guaranteed that feature requests for HTTPS and HTTP/2 — HTTP/2 requires HTTPS so the two issues are linked — will come up again in the future

In case it does come up, nginx supports http/2 including push for proxies as well:
https://www.nginx.com/blog/nginx-1-13-9-http2-server-push/
https://stackoverflow.com/questions/41637076/http2-with-node-js-behind-nginx-proxy