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: implement HTTP-01 challenge responses #20049

Closed
dgpc opened this issue Apr 20, 2017 · 6 comments
Closed

x/crypto/acme/autocert: implement HTTP-01 challenge responses #20049

dgpc opened this issue Apr 20, 2017 · 6 comments

Comments

@dgpc
Copy link

dgpc commented Apr 20, 2017

Presently only the TLS-SNI-02 & TLS-SNI-01 Challenges are supported:
https://github.com/golang/crypto/blob/12c985a/acme/autocert/autocert.go#L455..L463

Adding support for the HTTP-01 challenge would be helpful for servers that sit behind a TLS-terminating proxy (such as Google Cloud HTTP(S) Load Balancer), and therefore cannot receive TLS-SNI requests directly. My intended use case is to have a server running on GKE behind such a load balancer (Kube Ingress) provision/update certs and then use the Kube API to update TLS secrets for the load balancer to serve.

Would you be open to a change that adds this support? Probably by way of adding an optional *http.ServeMux to the autocert.Manager struct, and updating the function linked above?

/cc @x1ddos @bradfitz

@gopherbot gopherbot added this to the Unreleased milestone Apr 20, 2017
@bradfitz
Copy link
Contributor

If you're doing to do something custom and wire up a weird contraption, the https://godoc.org/golang.org/x/crypto/acme package has HTTP-01 support already. Have you got that working first?

autocert is supposed to be magic for a common config. Maybe your use case will also become a common config, but it's not clear it is already.

FWIW, I also run a bunch of autocert sites on GKE, but it's just using Google's TCP proxy, so I still do the SNI+HTTPS, without HTTP proxies or TLS in front of me. You can implement the autocert cache interface and put the certs on GCS or something.

In any case, what API do you have in mind? I'd say make it work first and then propose the least invasive API change possible.

@dgpc
Copy link
Author

dgpc commented Apr 20, 2017

The API change I had in mind is adding this exported field to autocert.Manager:

// ServeMux optionally specifies a request multiplexer to use for HTTP-01 challenges.
// If specified, HTTP-01 challenges will be preferred over TLS-SNI challenges.
ServeMux *http.ServeMux

Usage would then look something like:

func main() {
    m := autocert.Manager{
        Prompt:     autocert.AcceptTOS,
        HostPolicy: autocert.HostWhitelist("example.org"),
        Cache:      autocert.DirCache("/mnt/autocert/cache"),
        ServeMux:   http.DefaultServeMux, // Used to serve HTTP-01 challenge responses.
    }
    go func() {
        t := time.NewTicker(time.Minute)
        for range t.C {
            if cert, err := m.GetCertificate(&tls.ClientHelloInfo{ServerName: "example.org"}); err != nil {
                // Log error.
            } else {
                // Store cert in place used by TLS-terminating proxy.
            }
        }
    }()
    http.ListenAndServe(":http", nil)
}

This could be done just using the acme package, but I'd like to take advantage of the conveniences autocert provides.

In any case, I'll try and get it working first then report back. Thanks for the feedback!

@bradfitz
Copy link
Contributor

The concrete type http.ServeMux should never appear in API surface. It's a silly little helper type; the fundamental type is http.Handler.

And instead of naming the field ServeMux, you'd write what it's used for. Actually, try writing the docs. What would you say?

type Manager struct {
     // Foo optionally specifies a Handler to ...
     // If nil, ...
     Foo http.Handler

Also, that one minute polling loop is kinda gross. Any polling is gross: it's either too fast or too slow. You want to have no TLS for a minute while you wait to install the cert?

Any API needs to work instantly and not involve loops with sleeps.

@x1ddos
Copy link

x1ddos commented Apr 25, 2017

Wouldn't it be even simpler to just have Manager implement http.Handler? Then one could do:

m := &autocert.Manager{Prompt: autocert.AcceptTOS}
http.Handle("/.well-known/acme-challenge/", m)
http.ListenAndServe(":80", nil)

Manager would still need to know which challenge type is preferred, tls-sni or http-01, but this feels less intrusive towards existing API.

@bradfitz
Copy link
Contributor

That seems like too big of a cut through the abstractions. I'd rather not add a ServeHTTP method and make users know or think about the ACME protocol. That sends the wrong message about how Manager is supposed to be used.

@dgpc
Copy link
Author

dgpc commented Apr 26, 2017

RE: Polling - yes, it's ugly, the client could just poll once every RenewBefore or so, but it would be nicer if Manager exposed a way of watching for updates to certificates. Something like:
func (m *Manager) WatchCertificate(hello *tls.ClientHelloInfo) <-chan *tls.Certificate
But this would be a fairly invasive change to Manager.

RE: ServeMux/Handler. I don't understand how one can add a Handler for a path (e.g./.well-known/ to a Server by passing Manager an http.Handler. I think it would have to be the other way around, having Manager implement http.Handler like Alex suggests, or perhaps adding a function to Manager's interface like:
func (m *Manager) HTTP01ChallengeHandler() http.Handler

Given this would expose details of the ACME protocol to users of autocert.Manager, and that's not desirable, I think I'll just use the acme package directly as suggested by Brad in the first reply. Thanks all!

@dgpc dgpc closed this as completed Apr 26, 2017
@golang golang locked and limited conversation to collaborators Apr 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants