From 254b8e205ae4a8eb14d8cf7ebb6dda423916d42f Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 28 Jun 2023 14:30:49 -0600 Subject: [PATCH] caddytls: Cache 'ask' results to reduce load --- modules/caddytls/acmeissuer.go | 34 -------------- modules/caddytls/automation.go | 84 +++++++++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 35 deletions(-) diff --git a/modules/caddytls/acmeissuer.go b/modules/caddytls/acmeissuer.go index ca7998179b3..94e20d5a2bc 100644 --- a/modules/caddytls/acmeissuer.go +++ b/modules/caddytls/acmeissuer.go @@ -19,7 +19,6 @@ import ( "crypto/x509" "errors" "fmt" - "net/url" "os" "strconv" "time" @@ -491,39 +490,6 @@ func (iss *ACMEIssuer) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return nil } -// onDemandAskRequest makes a request to the ask URL -// to see if a certificate can be obtained for name. -// The certificate request should be denied if this -// returns an error. -func onDemandAskRequest(logger *zap.Logger, ask string, name string) error { - askURL, err := url.Parse(ask) - if err != nil { - return fmt.Errorf("parsing ask URL: %v", err) - } - qs := askURL.Query() - qs.Set("domain", name) - askURL.RawQuery = qs.Encode() - - askURLString := askURL.String() - resp, err := onDemandAskClient.Get(askURLString) - if err != nil { - return fmt.Errorf("error checking %v to determine if certificate for hostname '%s' should be allowed: %v", - ask, name, err) - } - resp.Body.Close() - - logger.Debug("response from ask endpoint", - zap.String("domain", name), - zap.String("url", askURLString), - zap.Int("status", resp.StatusCode)) - - if resp.StatusCode < 200 || resp.StatusCode > 299 { - return fmt.Errorf("%s: %w %s - non-2xx status code %d", name, errAskDenied, ask, resp.StatusCode) - } - - return nil -} - func ParseCaddyfilePreferredChainsOptions(d *caddyfile.Dispenser) (*ChainPreference, error) { chainPref := new(ChainPreference) if d.NextArg() { diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go index de882013137..d80a9e5a188 100644 --- a/modules/caddytls/automation.go +++ b/modules/caddytls/automation.go @@ -19,7 +19,9 @@ import ( "errors" "fmt" "net/http" + "net/url" "strings" + "sync" "time" "github.com/caddyserver/caddy/v2" @@ -254,7 +256,8 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error { if tlsApp.Automation == nil || tlsApp.Automation.OnDemand == nil { return nil } - if err := onDemandAskRequest(tlsApp.logger, tlsApp.Automation.OnDemand.Ask, name); err != nil { + + if err := onDemandAskAllowed(tlsApp.logger, tlsApp.Automation.OnDemand.Ask, name); err != nil { // distinguish true errors from denials, because it's important to elevate actual errors if errors.Is(err, errAskDenied) { tlsApp.logger.Debug("certificate issuance denied", @@ -483,6 +486,85 @@ type RateLimit struct { Burst int `json:"burst,omitempty"` } +// onDemandAskAllowed checks whether the given name is allowed to have a certificate +// managed for it; first by checking the cache of recent queries, and if needed, a +// fresh answer is obtained from making a GET request to askURL. +func onDemandAskAllowed(logger *zap.Logger, askURL string, name string) error { + askCacheMu.RLock() + cachedResult, ok := askCache[name] + askCacheMu.RUnlock() + if ok && cachedResult.expires.After(time.Now()) { + logger.Debug("using cached 'ask' result", + zap.String("name", name), + zap.Time("expires", cachedResult.expires), + zap.Error(cachedResult.err)) + return cachedResult.err + } + + err := onDemandAskRequest(logger, askURL, name) + + resultExpires := time.Now().Add(1 * time.Hour) + + askCacheMu.Lock() + if len(askCache) >= 1000 { + // remove an entry from the cache to make room for this one; + // I know this is not normally distributed... not sure if I + // care unless we see measurable performance impact (this + // is very fast and simple) + for key := range askCache { + delete(askCache, key) + break + } + } + askCache[name] = askResult{err, resultExpires} + askCacheMu.Unlock() + + return err +} + +// onDemandAskRequest makes a request to the ask URL +// to see if a certificate can be obtained for name. +// The certificate request should be denied if this +// returns an error. +func onDemandAskRequest(logger *zap.Logger, ask string, name string) error { + askURL, err := url.Parse(ask) + if err != nil { + return fmt.Errorf("parsing ask URL: %v", err) + } + qs := askURL.Query() + qs.Set("domain", name) + askURL.RawQuery = qs.Encode() + + askURLString := askURL.String() + resp, err := onDemandAskClient.Get(askURLString) + if err != nil { + return fmt.Errorf("error checking %v to determine if certificate for hostname '%s' should be allowed: %v", + ask, name, err) + } + resp.Body.Close() + + logger.Debug("response from ask endpoint", + zap.String("domain", name), + zap.String("url", askURLString), + zap.Int("status", resp.StatusCode)) + + if resp.StatusCode < 200 || resp.StatusCode > 299 { + return fmt.Errorf("%s: %w %s - non-2xx status code %d", name, errAskDenied, ask, resp.StatusCode) + } + + return nil +} + +var ( + askCache = make(map[string]askResult) + askCacheMu sync.RWMutex +) + +type askResult struct { + err error + expires time.Time +} + // ConfigSetter is implemented by certmagic.Issuers that // need access to a parent certmagic.Config as part of // their provisioning phase. For example, the ACMEIssuer