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

x/crypto/acme/autocert: Enable custom port / IP binding #29540

Open
axxelG opened this issue Jan 3, 2019 · 6 comments

Comments

@axxelG
Copy link

commented Jan 3, 2019

It is mandatory to be reachable on port 443 from the public internet for autocert to work but it is completely fine to run a service on a different port internally and use e. g. NAT on a router to fulfill this requirement.

Additionally there are setups where I don't want to listen on all my local IP addresses.

This can be implemented through a new function ListenerCustomAddress(address) that works like Listener() but accepts a custom address.

PR: golang/crypto#69
Gerrit: https://go-review.googlesource.com/c/crypto/+/155744

@gopherbot gopherbot added this to the Unreleased milestone Jan 3, 2019

@slrz

This comment has been minimized.

Copy link

commented Jan 4, 2019

Am I missing something or is it already possible to do what you want by calling tls.Listen/http.(*Server).ListenAndServeTLS directly instead of using the autocert manager's convenience function?

@axxelG

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

In my opinion the whole autocert package is about convenience and providing easy HTTPS implementation.
I'm pretty sure the step from autocerts NewListener() to tls.Listen will scare some people away. Maybe this is the reason why there is already autocerts Listener. This option to easy implement HTTPS will be extended for people that do not own a pool of free public IP addresses and/or multiple servers.

@axxelG

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

Copy of @bradfitz comment in Gerrit:

I'm not a huge fan of this name. Plus Go APIs typically say "Addr" instead of "Address", but even that's kinda weird.
The adjectives should probably go before the noun, too, even though we don't do that everywhere (Dial, DialContext, etc).
Maybe "Indirect443Listener"? To make it super explicit that this is really for port 443 but not directly?
In any case, file a bug and we can bikeshed the name there with others. We don't typically make these sorts of decisions on Gerrit

@axxelG

This comment has been minimized.

Copy link
Author

commented Jan 4, 2019

I'm not really happy with the name, too. Here are my thoughts why I ended up with ListenerCustomAddress and my proposal to solve it.

My first thougt was to make Listener more flexible and move the current Listener functionality to something like Listener443Any but this will break backwards compatibility and is easy to be used wrong so people will face not working Letsencrypt challenges.

My 2nd idea was to call the function NATListener but this name does not include binding the listener to a selected IP on port 443 without NAT and generally I don't like to refer to names of "external" techniques that might be used but maybe there are other situations without NAT where you want to (and can) use different ports.

So I went with the 3rd option ListenerCustomAddress to stay close to the underlying net package. At the end we are setting net.Listener.Addr.

I thought about a 4th option that maybe work well with @bradfitz proposal Indirect443Listener We can create two new functions

  1. Indirect443Listener(port int) net.Listener or NATListener(port int) net.Listener
  2. ListenerBindIP(IP string) net.Listener.

If somebody really wants to bind to a specific address on a port different from 443 he needs to build this on his own outside of autocert. I'm not sure if it is a good idea to sacrifice flexibility for better function naming but the described case will be rarely needed so I'm fine with it.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

NewListener is documented as an extreme helper and is just a few lines, I don't think we need an alternative for rerouted ports. Instead, we can add a new method to Manager which does all the useful work that (*Manager).Listener does.

I'd argue the Listener name and API were a mistake, as it does the same thing as net.Listen and tls.Listen but with arbitrary assumptions on interfaces, so my proposal is that we call the method

(*Manager) Listen(network, address string) (net.Listener, error)

and we deprecate Listener.

@axxelG

This comment has been minimized.

Copy link
Author

commented Feb 2, 2019

@FiloSottile I support your proposal. I think it's more straightforward to use and returning errors is a huge improvement.

The changes should be fairly easy to do but I have two questions regarding the workflow:

  1. Can I mess around with the branch (rebase / force push) without breaking Gerrit integration or something else?

  2. Should I change the PR now and hope it will be approved or wait until a decision is made?

@golang golang deleted a comment from shankaryoga Feb 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.