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

Closed
axxelG opened this issue Jan 3, 2019 · 11 comments
Closed

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

axxelG opened this issue Jan 3, 2019 · 11 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@axxelG
Copy link

axxelG 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
Copy link

slrz 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
Copy link
Author

axxelG 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
Copy link
Author

axxelG 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
Copy link
Author

axxelG 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
Copy link
Contributor

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.

@FiloSottile FiloSottile added Proposal Proposal-Crypto Proposal related to crypto packages or other security issues labels Jan 29, 2019
@axxelG
Copy link
Author

axxelG 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
@FiloSottile
Copy link
Contributor

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

@x1ddos
Copy link

x1ddos commented Feb 12, 2020

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
Copy link
Contributor

rsc commented Feb 12, 2020

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
Copy link
Contributor

rsc commented Feb 26, 2020

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

@rsc rsc moved this from Active to Likely Decline in Proposals (old) Feb 26, 2020
@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

No change in consensus, so declined.

@rsc rsc closed this as completed Mar 4, 2020
@rsc rsc moved this from Likely Decline to Declined in Proposals (old) Mar 4, 2020
@golang golang locked and limited conversation to collaborators Mar 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
No open projects
Development

No branches or pull requests

6 participants