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

[axxelG]

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

[slrz]

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]

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]

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]

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]

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]

@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?

[FiloSottile]

@x1ddos, does #29540 (comment) look good to you?

[x1ddos]

This issue requires no code changes imho:

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.

The above sounds like a non-issue to me:

m := &autocert.Manager{
    Cache:      autocert.DirCache("secret-dir"),
    Prompt:     autocert.AcceptTOS,
    HostPolicy: autocert.HostWhitelist("example.org", "www.example.org"),
}
s := &http.Server{
    Addr:      "127.0.0.1:8443",
    TLSConfig: m.TLSConfig(),
}
s.ListenAndServeTLS("", "")

Listener has a different purpose:

It enables one-line HTTPS servers

It even says so later:

NewListener is a convenience function for a common configuration. More complex or custom configurations can use the autocert.Manager type instead.

(from https://godoc.org/golang.org/x/crypto/acme/autocert#NewListener)

[rsc]

Talked to @bradfitz about this (we designed this package).

It seems odd to have both Listen and Listener (maybe Listener should have been named Listen from the start but oh well). For >99% of use cases Listener is doing the right thing.

We could imagine adding a field Addr string to Manager, defaulting to ":443", but then what if you want to change the keepalives that listener does?

As it is, listener (the unexported type) is a very small wrapper around Manager itself. It probably makes sense to just copy it out. All you really need access to is m.TLSConf(), which you have.

[rsc]

Based on the discussion above, this seems like a likely decline.

[rsc]

No change in consensus, so declined.