From 8e75ae249524e4d39b9ff21907b227c88d1f5a21 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 10 Aug 2016 22:13:06 -0600 Subject: [PATCH 1/3] Only consume HTTP challenge for names we are solving for (closes #549) If another ACME client is trying to solve a challenge for a name not being served by Caddy on the same machine where Caddy is running, the HTTP challenge will be consumed by Caddy rather than allowing the owner to use the Caddyfile to proxy the challenge. With this change, we only consume requests for HTTP challenges for hostnames that we recognize. Before doing the challenge, we add the name to a set, and when seeing if we should proxy the challenge, we first check the path of course to see if it is an HTTP challenge; if it is, we then check that set to see if the hostname is in the set. Only if it is, do we consume it. Otherwise, the request is treated like any other, allowing the owner to configure a proxy for such requests to another ACME client. --- caddytls/client.go | 46 ++++++++++++++++++++++++++++++++++++ caddytls/httphandler.go | 3 +++ caddytls/httphandler_test.go | 9 ++++++- 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/caddytls/client.go b/caddytls/client.go index 09d6425d..c754ab1c 100644 --- a/caddytls/client.go +++ b/caddytls/client.go @@ -144,9 +144,11 @@ var newACMEClient = func(config *Config, allowPrompts bool) (*ACMEClient, error) func (c *ACMEClient) Obtain(names []string) error { Attempts: for attempts := 0; attempts < 2; attempts++ { + namesObtaining.Add(names) acmeMu.Lock() certificate, failures := c.ObtainCertificate(names, true, nil) acmeMu.Unlock() + namesObtaining.Remove(names) if len(failures) > 0 { // Error - try to fix it or report it to the user and abort var errMsg string // we'll combine all the failures into a single error message @@ -294,3 +296,47 @@ func (c *ACMEClient) Revoke(name string) error { return nil } + +// namesObtaining is a set of hostnames with thread-safe +// methods. A name should be in this set only while this +// package is in the process of obtaining a certificate +// for the name. ACME challenges that are received for +// names which are not in this set were not initiated by +// this package and probably should not be handled by +// this package. +var namesObtaining = nameCoordinator{names: make(map[string]struct{})} + +type nameCoordinator struct { + names map[string]struct{} + mu sync.RWMutex +} + +// Add adds names to c. It is safe for concurrent use. +func (c *nameCoordinator) Add(names []string) { + c.mu.Lock() + for _, name := range names { + c.names[strings.ToLower(name)] = struct{}{} + } + c.mu.Unlock() +} + +// Remove removes names from c. It is safe for concurrent use. +func (c *nameCoordinator) Remove(names []string) { + c.mu.Lock() + for _, name := range names { + delete(c.names, strings.ToLower(name)) + } + c.mu.Unlock() +} + +// Has returns true if c has name. It is safe for concurrent use. +func (c *nameCoordinator) Has(name string) bool { + hostname, _, err := net.SplitHostPort(name) + if err != nil { + hostname = name + } + c.mu.RLock() + _, ok := c.names[strings.ToLower(hostname)] + c.mu.RUnlock() + return ok +} diff --git a/caddytls/httphandler.go b/caddytls/httphandler.go index 68dd3fd3..8b7aebf7 100644 --- a/caddytls/httphandler.go +++ b/caddytls/httphandler.go @@ -19,6 +19,9 @@ func HTTPChallengeHandler(w http.ResponseWriter, r *http.Request, altPort string if !strings.HasPrefix(r.URL.Path, challengeBasePath) { return false } + if !namesObtaining.Has(r.Host) { + return false + } scheme := "http" if r.TLS != nil { diff --git a/caddytls/httphandler_test.go b/caddytls/httphandler_test.go index fc04e8ee..223c31d7 100644 --- a/caddytls/httphandler_test.go +++ b/caddytls/httphandler_test.go @@ -8,13 +8,17 @@ import ( ) func TestHTTPChallengeHandlerNoOp(t *testing.T) { - // try base paths that aren't handled by this handler + namesObtaining.Add([]string{"localhost"}) + + // try base paths and host names that aren't + // handled by this handler for _, url := range []string{ "http://localhost/", "http://localhost/foo.html", "http://localhost/.git", "http://localhost/.well-known/", "http://localhost/.well-known/acme-challenging", + "http://other/.well-known/acme-challenge/foo", } { req, err := http.NewRequest("GET", url, nil) if err != nil { @@ -46,6 +50,9 @@ func TestHTTPChallengeHandlerSuccess(t *testing.T) { } ts.Listener = ln + // Tell this package that we are handling a challenge for 127.0.0.1 + namesObtaining.Add([]string{"127.0.0.1"}) + // Start our engines and run the test ts.Start() defer ts.Close() From 46bc0d5c4e96c51be6eb21e93a700a6e46bd9c57 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 10 Aug 2016 23:44:43 -0600 Subject: [PATCH 2/3] Whoops, finishing up the last commit properly Need to add the name to namesObtaining each time we use the ACME client. --- caddytls/client.go | 2 ++ caddytls/config.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/caddytls/client.go b/caddytls/client.go index c754ab1c..a051880f 100644 --- a/caddytls/client.go +++ b/caddytls/client.go @@ -236,9 +236,11 @@ func (c *ACMEClient) Renew(name string) error { var newCertMeta acme.CertificateResource var success bool for attempts := 0; attempts < 2; attempts++ { + namesObtaining.Add([]string{name}) acmeMu.Lock() newCertMeta, err = c.RenewCertificate(certMeta, true) acmeMu.Unlock() + namesObtaining.Remove([]string{name}) if err == nil { success = true break diff --git a/caddytls/config.go b/caddytls/config.go index 89ccafd7..f1dcff84 100644 --- a/caddytls/config.go +++ b/caddytls/config.go @@ -214,9 +214,11 @@ func (c *Config) renewCertName(name string, allowPrompts bool) error { var newCertMeta acme.CertificateResource var success bool for attempts := 0; attempts < 2; attempts++ { + namesObtaining.Add([]string{name}) acmeMu.Lock() newCertMeta, err = client.RenewCertificate(certMeta, true) acmeMu.Unlock() + namesObtaining.Remove([]string{name}) if err == nil { success = true break From 68be4a91610a17c95716bd06c3191282dd9ce47c Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 10 Aug 2016 23:46:04 -0600 Subject: [PATCH 3/3] Don't prompt for email when user is not there to provide one Also don't bother showing stdout output in same situation --- caddy.go | 23 +++++++++++++++++++++++ caddyhttp/httpserver/https.go | 9 ++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/caddy.go b/caddy.go index 1b1caf54..ff578d12 100644 --- a/caddy.go +++ b/caddy.go @@ -50,6 +50,14 @@ var ( // was started as part of an upgrade, where a parent // Caddy process started this one. isUpgrade bool + + // started will be set to true when the first + // instance is started; it never gets set to + // false after that. + started bool + + // mu protects the variables 'isUpgrade' and 'started'. + mu sync.Mutex ) // Instance contains the state of servers created as a result of @@ -497,6 +505,10 @@ func startWithListenerFds(cdyfile Input, inst *Instance, restartFds map[string]r } } + mu.Lock() + started = true + mu.Unlock() + return nil } @@ -737,9 +749,20 @@ func Upgrade() error { // where a parent caddy process spawned this one to ugprade // the binary. func IsUpgrade() bool { + mu.Lock() + defer mu.Unlock() return isUpgrade } +// Started returns true if at least one instance has been +// started by this package. It never gets reset to false +// once it is set to true. +func Started() bool { + mu.Lock() + defer mu.Unlock() + return started +} + // CaddyfileInput represents a Caddyfile as input // and is simply a convenient way to implement // the Input interface. diff --git a/caddyhttp/httpserver/https.go b/caddyhttp/httpserver/https.go index 3a6ebb70..825f028d 100644 --- a/caddyhttp/httpserver/https.go +++ b/caddyhttp/httpserver/https.go @@ -10,7 +10,9 @@ import ( ) func activateHTTPS(cctx caddy.Context) error { - if !caddy.Quiet { + operatorPresent := !caddy.Started() + + if !caddy.Quiet && operatorPresent { fmt.Print("Activating privacy features...") } @@ -21,7 +23,7 @@ func activateHTTPS(cctx caddy.Context) error { // place certificates and keys on disk for _, c := range ctx.siteConfigs { - err := c.TLS.ObtainCert(true) + err := c.TLS.ObtainCert(operatorPresent) if err != nil { return err } @@ -44,9 +46,10 @@ func activateHTTPS(cctx caddy.Context) error { return err } - if !caddy.Quiet { + if !caddy.Quiet && operatorPresent { fmt.Println(" done.") } + return nil }