caddytls: Better handle FileStorage and cleaning up locks on exit

caddy.go

@@ -469,9 +469,10 @@ func (i *Instance) Caddyfile() Input {
 //
 // This function blocks until all the servers are listening.
 func Start(cdyfile Input) (*Instance, error) {
-	// set up the clustering plugin, if there is one (this should be done
-	// exactly once -- but we can't do it during init when they're still
-	// getting plugged in, so do it when starting the first instance)
+	// set up the clustering plugin, if there is one (and there should
+	// always be one) -- this should be done exactly once, but we can't
+	// do it during init while plugins are still registering, so do it
+	// when starting the first instance)
 	if atomic.CompareAndSwapInt32(&clusterPluginSetup, 0, 1) {
 		clusterPluginName := os.Getenv("CADDY_CLUSTERING")
 		if clusterPluginName == "" {
@@ -486,6 +487,7 @@ func Start(cdyfile Input) (*Instance, error) {
 			return nil, fmt.Errorf("constructing cluster plugin %s: %v", clusterPluginName, err)
 		}
 		certmagic.DefaultStorage = storage
+		OnProcessExit = append(OnProcessExit, certmagic.DefaultStorage.UnlockAllObtained)
 	}
 
 	inst := &Instance{serverType: cdyfile.ServerType(), wg: new(sync.WaitGroup), Storage: make(map[interface{}]interface{})}

caddytls/config.go

@@ -98,7 +98,7 @@ func NewConfig(inst *caddy.Instance) *Config {
 	certCache, ok := inst.Storage[CertCacheInstStorageKey].(*certmagic.Cache)
 	inst.StorageMu.RUnlock()
 	if !ok || certCache == nil {
-		certCache = certmagic.NewCache(certmagic.FileStorage{Path: caddy.AssetsPath()})
+		certCache = certmagic.NewCache(certmagic.DefaultStorage)
 		inst.OnShutdown = append(inst.OnShutdown, func() error {
 			certCache.Stop()
 			return nil
@@ -108,7 +108,7 @@ func NewConfig(inst *caddy.Instance) *Config {
 		inst.StorageMu.Unlock()
 	}
 	return &Config{
-		Manager: certmagic.NewWithCache(certCache, certmagic.Config{}), // TODO
+		Manager: certmagic.NewWithCache(certCache, certmagic.Config{}),
 	}
 }
 

caddytls/setup.go

@@ -57,7 +57,7 @@ func setupTLS(c *caddy.Controller) error {
 	// a single certificate cache is used by the whole caddy.Instance; get a pointer to it
 	certCache, ok := c.Get(CertCacheInstStorageKey).(*certmagic.Cache)
 	if !ok || certCache == nil {
-		certCache = certmagic.NewCache(certmagic.FileStorage{Path: caddy.AssetsPath()})
+		certCache = certmagic.NewCache(certmagic.DefaultStorage)
 		c.OnShutdown(func() error {
 			certCache.Stop()
 			return nil
@@ -252,18 +252,6 @@ func setupTLS(c *caddy.Controller) error {
 					return c.Errf("Setting up DNS provider '%s': %v", dnsProvName, err)
 				}
 				config.Manager.DNSProvider = dnsProv
-			// TODO
-			// case "storage":
-			// 	args := c.RemainingArgs()
-			// 	if len(args) != 1 {
-			// 		return c.ArgErr()
-			// 	}
-			// 	storageProvName := args[0]
-			// 	storageProvConstr, ok := storageProviders[storageProvName]
-			// 	if !ok {
-			// 		return c.Errf("Unsupported Storage provider '%s'", args[0])
-			// 	}
-			// 	config.Manager.Storage = storageProvConstr
 			case "alpn":
 				args := c.RemainingArgs()
 				if len(args) == 0 {

vendor/github.com/mholt/certmagic/cache.go

@@ -165,6 +165,5 @@ func (certCache *Cache) reloadManagedCertificate(oldCert Certificate) error {
 	return nil
 }
 
-// defaultCache is a convenient, default certificate cache for
-// use by this process when no other certificate cache is provided.
-var defaultCache = NewCache(DefaultStorage)
+var defaultCache *Cache
+var defaultCacheMu sync.Mutex

vendor/github.com/mholt/certmagic/client.go

@@ -94,7 +94,7 @@ func (cfg *Config) newACMEClient(interactive bool) (*acmeClient, error) {
 		legoCfg := lego.NewConfig(&leUser)
 		legoCfg.CADirURL = caURL
 		legoCfg.KeyType = keyType
-		legoCfg.UserAgent = UserAgent
+		legoCfg.UserAgent = buildUAString()
 		legoCfg.HTTPClient.Timeout = HTTPTimeout
 		client, err = lego.NewClient(legoCfg)
 		if err != nil {
@@ -373,6 +373,14 @@ func (c *acmeClient) Revoke(name string) error {
 	return nil
 }
 
+func buildUAString() string {
+	ua := "CertMagic"
+	if UserAgent != "" {
+		ua += " " + UserAgent
+	}
+	return ua
+}
+
 // Some default values passed down to the underlying lego client.
 var (
 	UserAgent   string

vendor/github.com/mholt/certmagic/config.go

@@ -125,15 +125,28 @@ func NewDefault() *Config {
 // a default certificate cache. All calls to
 // New() will use the same certificate cache.
 func New(cfg Config) *Config {
-	return NewWithCache(defaultCache, cfg)
+	return NewWithCache(nil, cfg)
 }
 
 // NewWithCache makes a valid new config based on cfg
-// and uses the provided certificate cache.
+// and uses the provided certificate cache. If certCache
+// is nil, a new, default one will be created using
+// DefaultStorage; or, if a default cache has already
+// been created, it will be reused.
 func NewWithCache(certCache *Cache, cfg Config) *Config {
-	// avoid nil pointers with sensible defaults
+	// avoid nil pointers with sensible defaults,
+	// careful to initialize a default cache (which
+	// begins its maintenance goroutine) only if
+	// needed - and only once (we don't initialize
+	// it at package init to give importers a chance
+	// to set DefaultStorage if they so desire)
 	if certCache == nil {
+		defaultCacheMu.Lock()
+		if defaultCache == nil {
+			defaultCache = NewCache(DefaultStorage)
+		}
 		certCache = defaultCache
+		defaultCacheMu.Unlock()
 	}
 	if certCache.storage == nil {
 		certCache.storage = DefaultStorage

vendor/github.com/mholt/certmagic/filestorage.go

@@ -17,6 +17,7 @@ package certmagic
 import (
 	"fmt"
 	"io/ioutil"
+	"log"
 	"os"
 	"path"
 	"path/filepath"
@@ -171,18 +172,36 @@ func (fs FileStorage) TryLock(key string) (Waiter, error) {
 	}
 
 	fw = &fileStorageWaiter{
+		key:      key,
 		filename: filepath.Join(lockDir, StorageKeys.safe(key)+".lock"),
 		wg:       new(sync.WaitGroup),
 	}
 
+	var checkedStaleLock bool // sentinel value to avoid infinite goto-ing
+
+createLock:
 	// create the file in a special mode such that an
 	// error is returned if it already exists
 	lf, err := os.OpenFile(fw.filename, os.O_CREATE|os.O_EXCL, 0644)
 	if err != nil {
 		if os.IsExist(err) {
-			// another process has the lock; use it to wait
+			// another process has the lock
+
+			// check to see if the lock is stale, if we haven't already
+			if !checkedStaleLock {
+				checkedStaleLock = true
+				if fs.lockFileStale(fw.filename) {
+					log.Printf("[INFO][%s] Lock for '%s' is stale; removing then retrying: %s",
+						fs, key, fw.filename)
+					os.Remove(fw.filename)
+					goto createLock
+				}
+			}
+
+			// if lock is not stale, wait upon it
 			return fw, nil
 		}
+
 		// otherwise, this was some unexpected error
 		return nil, err
 	}
@@ -225,10 +244,33 @@ func (fs FileStorage) Unlock(key string) error {
 	return nil
 }
 
+// UnlockAllObtained removes all locks obtained by
+// this instance of fs.
+func (fs FileStorage) UnlockAllObtained() {
+	for key, fw := range fileStorageNameLocks {
+		err := fs.Unlock(fw.key)
+		if err != nil {
+			log.Printf("[ERROR][%s] Releasing obtained lock for %s: %v", fs, key, err)
+		}
+	}
+}
+
+func (fs FileStorage) lockFileStale(filename string) bool {
+	info, err := os.Stat(filename)
+	if err != nil {
+		return true // no good way to handle this, really; lock is useless?
+	}
+	return time.Since(info.ModTime()) > staleLockDuration
+}
+
 func (fs FileStorage) lockDir() string {
 	return filepath.Join(fs.Path, "locks")
 }
 
+func (fs FileStorage) String() string {
+	return "FileStorage:" + fs.Path
+}
+
 // fileStorageWaiter waits for a file to disappear; it
 // polls the file system to check for the existence of
 // a file. It also uses a WaitGroup to optimize the
@@ -237,6 +279,7 @@ func (fs FileStorage) lockDir() string {
 // the lock will still block, but must wait for the
 // polling to get their answer.)
 type fileStorageWaiter struct {
+	key      string
 	filename string
 	wg       *sync.WaitGroup
 }
@@ -259,3 +302,7 @@ var fileStorageNameLocksMu sync.Mutex
 
 var _ Storage = FileStorage{}
 var _ Waiter = &fileStorageWaiter{}
+
+// staleLockDuration is the length of time
+// before considering a lock to be stale.
+const staleLockDuration = 2 * time.Hour

vendor/github.com/mholt/certmagic/maintain.go

@@ -38,21 +38,21 @@ func (certCache *Cache) maintainAssets() {
 	for {
 		select {
 		case <-renewalTicker.C:
-			log.Println("[INFO] Scanning for expiring certificates")
+			log.Printf("[INFO][%s] Scanning for expiring certificates", certCache.storage)
 			err := certCache.RenewManagedCertificates(false)
 			if err != nil {
-				log.Printf("[ERROR] Renewing managed certificates: %v", err)
+				log.Printf("[ERROR][%s] Renewing managed certificates: %v", certCache.storage, err)
 			}
-			log.Println("[INFO] Done checking certificates")
+			log.Printf("[INFO][%s] Done scanning certificates", certCache.storage)
 		case <-ocspTicker.C:
-			log.Println("[INFO] Scanning for stale OCSP staples")
+			log.Printf("[INFO][%s] Scanning for stale OCSP staples", certCache.storage)
 			certCache.updateOCSPStaples()
 			certCache.deleteOldStapleFiles()
-			log.Println("[INFO] Done checking OCSP staples")
+			log.Printf("[INFO][%s] Done checking OCSP staples", certCache.storage)
 		case <-certCache.stopChan:
 			renewalTicker.Stop()
 			ocspTicker.Stop()
-			log.Println("[INFO] Stopped certificate maintenance routine")
+			log.Printf("[INFO][%s] Stopped certificate maintenance routine", certCache.storage)
 			return
 		}
 	}

vendor/github.com/mholt/certmagic/storage.go

@@ -30,6 +30,8 @@ import (
 // same Storage value (its implementation and configuration)
 // in order to share certificates and other TLS resources
 // with the cluster.
+//
+// Implementations of Storage must be safe for concurrent use.
 type Storage interface {
 	// Locker provides atomic synchronization
 	// operations, making Storage safe to share.
@@ -87,6 +89,15 @@ type Locker interface {
 	// TryLock or if Unlock was not called at all. Unlock should also
 	// clean up any unused resources allocated during TryLock.
 	Unlock(key string) error
+
+	// UnlockAllObtained removes all locks obtained by this process,
+	// upon which others may be waiting. The importer should call
+	// this on shutdowns (and crashes, ideally) to avoid leaving stale
+	// locks, but Locker implementations must NOT rely on this being
+	// the case and should anticipate and handle stale locks. Errors
+	// should be printed or logged, since there could be multiple,
+	// with no good way to handle them anyway.
+	UnlockAllObtained()
 }
 
 // Waiter is a type that can block until a lock is released.

vendor/manifest

@@ -138,7 +138,7 @@
 			"importpath": "github.com/mholt/certmagic",
 			"repository": "https://github.com/mholt/certmagic",
 			"vcs": "git",
-			"revision": "dc98c40439d15f67021f10f0d9219a39d7cf2990",
+			"revision": "fe722057f2654b33cd528b8fd8b90e53fa495564",
 			"branch": "master",
 			"notests": true
 		},