/cc @x1ddos

I'd rather not. I'd rather keep autocert simple and only support the 95% use case. There are rapidly diminishing returns disproportionate to the added complexity required to support new use cases. Currently autocert doesn't need to intercept http.Handlers at all, for instance.

Fair point. I only thought it was possible considering the x/crypto/acme already supports this challenge, and wasn't looking at rapidly expanding scope either.

Due to an incident, LetsEncrypt is not issuing certs based on the tls-sni challenge, so perhaps this issue should be revisited. I volunteer to do this work.

There's also #23198 that suggests supporting dns-01, but I think supporting http-01 in package autocert would probably be easier, in that it would only require users to make modifications to their web servers (and not their DNS infrastructure).

@adg, I'm following that incident too. Let's wait to see the details. You can prep a CL in the meantime if you'd like, but I'm already a bit sad with what I suspect it'd need to look like.

Josh from Let's Encrypt posted to HN confirming that the exploit is an "interaction between the protocol and provider services" as opposed to an easily fixable issue like a bug and that they "have not yet reached a conclusion as to whether or not the TLS-SNI challenge will need to remain disabled permanently".

For those of us depending on autocert to renew our certs in prod it sure would be nice to have support for multiple protocols (both for the current incident and in the event of theoretical future vulnerabilities).

@bradfitz maybe I'm missing something, but what are you imagining here?

I'm imagining an "EnableHTTPChallenge bool" field on Manager, and a "func (m *Manager) ChallengeHandler() http.Handler" method.

I think we can leave registration in muxes up-to clients, this is a (slightly) advanced use-case, and its fairly straightforward anyways. Also, the implementation should probably stick the challenges in the Cache, so it'd also work in a multi-frontend setup, same as the SNI challenge.

@adg @bradfitz let me guys know if you need any help.

@x1ddos I've done little but read the spec and think about how it might look, so far. If you're in a more amenable time zone (it's my late evening now) then feel free to take a crack at it. (I don't need to own this; I just wanted to take the initiative to make forward progress.)

Here's a very simple API addition that should work. The autocert.Manager type would implement http.Handler, and that ServeHTTP method would serve LetsEncrypt validation requests, or otherwise pass the request to the underlying handler as specified by a new Handler field in the autocert.Manager struct. API declaration diff follows.

diff --git a/acme/autocert/autocert.go b/acme/autocert/autocert.go
index 94edba9..c2afdb8 100644
--- a/acme/autocert/autocert.go
+++ b/acme/autocert/autocert.go
@@ -144,6 +144,9 @@ type Manager struct {
        // is EC-based keys using the P-256 curve.
        ForceRSA bool
 
+       // Handler optional specifies an HTTP handler to invoke...TODO
+       Handler http.Handler
+
        clientMu sync.Mutex
        client   *acme.Client // initialized by acmeClient method
 
@@ -161,6 +164,9 @@ type Manager struct {
        renewal   map[string]*domainRenewal
 }
 
+func (m *Manager) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+}
+

I should also mention how this new API would be used. The caller, after they set up an autocert.Manager and provide it to the tls.Config, should also set up a straight HTTP listener that serves the manager:

http.ListenAndServe(":80", manager)

optionally setting the Manager's Handler field, should they wish to serve other content by plain HTTP.

Thanks @adg! I like that the autocert.Manager can just be an http.Handler, although I'm not sure @bradfitz does. I think I proposed it once, last year.

So, a complete example would look something like the this:

m := &autocert.Manager{
  Handler: http.DefaultServeMux
  // other fields
}
s := &http.Server{
    Addr:      ":https",
    TLSConfig: &tls.Config{GetCertificate: m.GetCertificate},
}
go http.ListenAndServe(":80", m)
s.ListenAndServeTLS("", "")

@balasanjay's version usage would look like the following. I have to admit, m.HTTPChallengeHandler() is more consistent with already existing m.Listener() feature. See below about the Listener.

m := &autocert.Manager{
  EnableHTTPChallenge: true
  // other fields
}
s := &http.Server{
    Addr:      ":https",
    TLSConfig: &tls.Config{GetCertificate: m.GetCertificate},
}
http.Handle("/.well-known/acme-challenge/", m.HTTPChallengeHandler())
go http.ListenAndServe(":80", nil)
s.ListenAndServeTLS("", "")

I think we actually need no new field like EnableHTTPChallenge. The HTTPChallengeHandler method call itself is probably sufficient.

Maybe we should also do something for autocert.NewListener users, which allows one-line servers.
It currently looks like this:

// NewListener returns a net.Listener that listens on the standard TLS ...
func NewListener(domains ...string) net.Listener

// Usage example.
http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
    fmt.Fprintf(w, "Hello, TLS user! Your config: %+v", r.TLS)
})
http.Serve(autocert.NewListener("example.com"), nil)

Anyway, working on a fix without NewListener for now.

I chatted with @adg about this last night. I don't like adding port 80 challenge support at all, but if we have to, I don't want to see callers need to concern themselves with low-level details like:

   http.Handle("/.well-known/acme-challenge/", m.HTTPChallengeHandler())

So I'd regrettably accept a ServeHTTP method on the Manager instead. It should return a 500 error if it's called when its Manager.Handler field is nil. (We were thinking that manager.Handler != nil implies http-01 challenge is enabled.)

Ok, sounds good.
What if we don't even have "enable/disable http challenge" concept at all. Just let Manager accept both tls-sni and http challenges, and if either of them works, I think it should do it.

@x1ddos, what is your proposal? I'm all for simplicity, and I don't propose a boolean knob, but we do need some extra crap so acme.Manager can get port 80 requests and delegate the non-ACME port 80 requests off to somewhere else.

Yeah, sorry, I meant to have the Manager always fulfill both types of challenges regardless of the value of the new Handler field, and then hope that at least one works.

I need to verify this, but if I'm not mistaken Let's Encrypt will still release the cert if at least one challenge was successfully verified.

It would also make it closer to the original use case of this bug, where a Go server does not terminate TLS.

@x1ddos, that is a super hand-wavy proposal. Please propose something concrete about how it would work. Nobody disagrees with you that magic would be nice.

Ok, I'll try while @adg is asleep.

Having Manager implement http.Handler seems simpler, I like it.

One thing: if the Manager's Handler field is nil, I propose we instead do a redirect to HTTPs, instead of 500-ing. Or if we want to make that explicit, maybe provide an autocert.RedirectToHTTPSHandler() that makes that redirecting GETs to HTTPS a one-liner.

We definitely should not have a "Handler: http.DefaultServeMux" as an example in the docs; handlers should ideally not be registered on HTTP at all to avoid clients inadvertently calling some API over HTTP and it "happening to work" but is actually insecure.
Actually, now that I type this, maybe we should just not include the Handler field at all; if clients really need to serve non-HTTPS traffic, we can treat this as an advanced (dangerous) use-case, and require that they know how to do the equivalent thing explicitly; the mux.Handle("/.well-known/acme-challenge/", m) above.

@balasanjay yeah, I thought the same thing after posting that example. Will send the CL without the Handler field. Running some tests now.

Change https://golang.org/cl/87201 mentions this issue: acme/autocert: support http-01 challenge type

After reading https://community.letsencrypt.org/t/2018-01-11-update-regarding-acme-tls-sni-and-shared-hosting-infrastructure/50188, I think this issue has just become even more important, because:

We have arrived at the conclusion that we cannot generally re-enable TLS-SNI validation. There are simply too many vulnerable shared hosting and infrastructure services that violate the assumptions behind TLS-SNI validation. We will be executing the following plan to mitigate impact and entirely sunset the TLS-SNI-01 and TLS-SNI-02 validation methods.

Mitigations for Existing TLS-SNI Users

Our recommendation for users is to begin a migration to the HTTP-01 or DNS-01 validation methods. We are working to provide a reasonable amount of migration time for as many users as possible, while maintaining our commitment to security.

@bradfitz (commenting on the CL design from a device without Gerrit login)

I disagree that we should optimize the API for folks having a custom unencrypted handler. The 2018 thing to do is to only serve 302 (or 301) on port 80.

I like the version with just ServeHTTP and a fallback to 302. If you think you know when to go unencrypted, you can take the extra step of muxing. (Maybe make the path a constant if you don’t want to make users type it?)

As opposed to having to build your own redirect handler or causing 404/500 on port 80 when you are in fact doing the right thing.

@FiloSottile maybe I can add an autocert.HTTPSRedirectHandler to be able to do something like:

http.ListenAndServe(":http", m.HTTPHandler(autocert.HTTPSRedirectHandler))

@x1ddos That seems quite far out-of-scope for this package, and also glazes over the issue of the exact behaviour it should exhibit: should it be a 301 or 302, should it use the inbound Host header (what if there isn't one?), should the new URL include the request's path, etc.? Just leave it to the user to provide an implementation.

@FiloSottile Yes, but, I often would ideally like control over where we redirect them. For instance I want:

http://example.com -> https://www.example.com

Though I suppose http://example.com -> https://example.com -> https://www.example.com isn't the worst situation.

My preference would be to provide a package const for "/.well-known/acme-challenge/" and a handler for that URL. It would be easy to create a helper function for ACME challenge + redirect. I'm concerned that too much sugar that you can't reduce would just make it harder to integrate into existing applications.

@billinghamj, I agree that adding a symbol like autocert.HTTPSRedirectHandler is far out of scope for the autocert package (godoc pollution), but having http→http2 redirect behavior might be a valid fallback default (if no other fallback http.Handler is provided).

@x1ddos need any assistance? Following the changes and it is looking pretty good!

Thanks @marclave! Join the change reviews if you want.

Re: default handler. It seems there are different opinions. Maybe we should just leave it at 404 for now. After all, "not found" and "not provided" have similar meaning.

Awesome to see this gaining traction, sad that it's because we don't have a choice.

If there's anything I can help with, please let me know 😄!

Hello all,

Any idea on the release plan / ETA for this change? I'm in dire need of getting a cert working for a new domain.

Thanks!

@jack-chung this is already done check golang/crypto@13931e2

just update your crypto/acme dependency

Thanks @jhernandezb !
Is there an example of how to use it? I read this thread and the code review, there has been numerous different designs being proposed and debated. It wasn't clear to me what is the final design. Appreciate your help!

Use the .HTTPHandler method of cert manager, as per the doc.

certMgr := autocert.Manager{ Prompt: ... }
s := &http.Server{
    Handler: certMgr.HTTPHandler(nil),
    Addr: ":80",
}
go s.ListenAndServe()

// normal :443 init, be sure to still pass TLSConfig with {GetCertificate: certMgr.GetCertificate }

Does this mean that it will default to using tls-sni if this new HTTP handler is never called?

func (m *Manager) HTTPHandler(fallback http.Handler) http.Handler {
	m.tokensMu.Lock()
	defer m.tokensMu.Unlock()
	m.tryHTTP01 = true

Because it seemed that I saw this behaviour, only when I manually called that HTTP handler I started getting certs.

If yes, would it make sense to have a method to set default strategy manually?

https://github.com/golang/crypto/blob/master/acme/autocert/autocert.go#L258-L263

Since the http-01 challenge has been implemented and this issue is closed, I'm locking this thread.

Please take this conversation elsewhere, like the golang-nuts mailing list.