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

Allow multiple values for HTTPS_PORT #3622

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

Conversation

zenhack
Copy link
Collaborator

@zenhack zenhack commented May 20, 2022

Fixes #3619


@ocdtrekkie, feel like testing? I have verified it doesn't break my box, but haven't actually tried adding a second port, mainly because I'd have to set up port forwarding and it's late enough that I'm wanting to put this down for the night.

@@ -2611,10 +2620,6 @@ private:
KJ_SYSCALL(setenv("HTTP_GATEWAY", "local", true));

KJ_SYSCALL(setenv("PORT", kj::str(config.ports[0]).cStr(), true));
KJ_IF_MAYBE(httpsPort, config.httpsPort) {
// TODO(cleanup): At this point, all this does is tell Sandcats to refresh certs.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm fairly sure at this point this doesn't do anything; the sandcats logic doesn't look for this variable, and grepping around for it didn't come up with any uses.

@ocdtrekkie ocdtrekkie added enhancement Feature requests install-config Installation/configuration issues labels May 20, 2022
@ocdtrekkie
Copy link
Collaborator

I will definitely test this weekend (partially because if this works I do not want to wait for a release). Running a server backup now, which I always do before updating to experimental code.

@ocdtrekkie
Copy link
Collaborator

Okay, so, we are close, because Sandstorm is definitely listening on the port! However, it looks like that host name recognition behavior may look at ports, because I get this (I have 443 as an HTTPS alternate port for now):

image

@zenhack
Copy link
Collaborator Author

zenhack commented May 20, 2022

Interesting. And this doesn't happen with an extra port added to PORT?

I will have to actually test this myself soonish.

@ocdtrekkie
Copy link
Collaborator

I don't have any HTTP ports exposed to the outside world, so I don't know, to be honest. I'd probably go for setting up a VM to play with it if I want to start opening those.

What I did test (because I was getting the unrecognized hostname on my alternate port) was trying to set up a static web publishing on the alternate port, and that also did not work.

auto promise = altPortServer->listenHttp(*listener);
auto promise = isHttpsPort(port)
? tlsManager.listenHttps(*listener)
: altPortServer->listenHttp(*listener);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the issue be here? If I can parse this, it means the altPortServer is only being reached if it's an HTTP port?

@zenhack
Copy link
Collaborator Author

zenhack commented May 21, 2022 via email

@ocdtrekkie
Copy link
Collaborator

This definitely worked for the main URL, but it appears to not work for anything else.

https://ocdhost.sandcats.io/shared/X8HYTaml-nrKIHORyo-C9AwuCzZ0FUoT_-OEXHUw10f even has the same exact hostname, as the main URL, but it gives the unrecognized hostname error. Web publishing didn't work either.

@zenhack
Copy link
Collaborator Author

zenhack commented May 21, 2022

Ok. I'm going to mark this as a draft for now; at some point I'll set up port forwarding myself and mess with it so we can do fewer round trips.

@zenhack zenhack marked this pull request as draft May 21, 2022 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests install-config Installation/configuration issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow listening on multiple HTTPS_PORTs
2 participants