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

bugfix: fix attempting to bind to invalid https port #120

Merged
merged 1 commit into from
Sep 9, 2021

Conversation

jimisaacs
Copy link
Collaborator

I'm not sure if there's something I'm doing wrong, but when I don't configure this (I do not want https support at all as we do our own TLS termination), it was still failing if the TLS config was passed in.

@ije
Copy link
Member

ije commented Sep 8, 2021

you are right, i figured out the problem at https://github.com/ije/rex/blob/0634e671c27790e45a9c151d28f95b1404f06df2/rex.go#L37, i non-dev mode the autotls is enabled.

@ije
Copy link
Member

ije commented Sep 8, 2021

thanks, look great to me 🙏

server/server.go Outdated
TLS: rex.TLSConfig{
}
if httpsPort > 0 {
serverConfig.TLS = rex.TLSConfig{
Port: uint16(httpsPort),
AutoTLS: rex.AutoTLSConfig{
AcceptTOS: !isDev,
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: AcceptTOS: httpsPort > 0 && !isDev

Copy link
Member

Choose a reason for hiding this comment

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

can make the code looks better

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 kind of confused about the business logic regarding AutoTSL, AcceptTOS, and TSL. I would expect AutoTSL is optional, is it not?

@jimisaacs
Copy link
Collaborator Author

jimisaacs commented Sep 8, 2021

Ok so now that you pointed to me to that code, I see you are using 0 to denote "default", in which rex then actually defaults to 443. Should this be changed in rex?

My ideal behavior is that esm.sh has the 443 default, and then I can just set it to 0 in esm.sh. So essentially rex does the httpsPort > 0 check and this PR change would simply change the -http-port default from 0 to 443.

@ije
Copy link
Member

ije commented Sep 8, 2021

Should this be changed in rex?

i don't need to change. when https.AutoTLS.AcceptTOS equals to true, even the port is zero the autotls also should work with 443 (default) port.

@jimisaacs
Copy link
Collaborator Author

jimisaacs commented Sep 8, 2021

What is your intent here AcceptTOS: httpsPort > 0 && !isDev?
My intent is to be able to completely disable TLS, is that what that is doing? Sorry, still confused around these options.

@ije
Copy link
Member

ije commented Sep 8, 2021

i mean we should disable the autotls when the https port is zero or in development mode

@jimisaacs
Copy link
Collaborator Author

@ije thanks, makes sense now, took your suggestion and tested in my deployed env. Works. PR updated.

@ije
Copy link
Member

ije commented Sep 9, 2021

amazing, thanks @jimisaacs

@ije ije merged commit 4e41026 into esm-dev:master Sep 9, 2021
@jimisaacs jimisaacs deleted the https-optional branch September 10, 2021 00:34
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