Refactor and improve TLS storage code (related to locking)

caddy/caddymain/run.go

@@ -180,7 +180,7 @@ func moveStorage() {
 	if err != nil {
 		log.Fatalf("[ERROR] Unable to get new path for certificate storage: %v", err)
 	}
-	newPath := string(fileStorage.(caddytls.FileStorage))
+	newPath := fileStorage.(*caddytls.FileStorage).Path
 	err = os.MkdirAll(string(newPath), 0700)
 	if err != nil {
 		log.Fatalf("[ERROR] Unable to make new certificate storage path: %v\n\nPlease follow instructions at:\nhttps://github.com/mholt/caddy/issues/902#issuecomment-228876011", err)

caddytls/client.go

@@ -175,16 +175,18 @@ func (c *ACMEClient) Obtain(name string) error {
 		return err
 	}
 
-	// We must lock the obtain with the storage engine
-	if lockObtained, err := storage.LockRegister(name); err != nil {
+	waiter, err := storage.TryLock(name)
+	if err != nil {
 		return err
-	} else if !lockObtained {
-		log.Printf("[INFO] Certificate for %v is already being obtained elsewhere", name)
-		return nil
+	}
+	if waiter != nil {
+		log.Printf("[INFO] Certificate for %s is already being obtained elsewhere and stored; waiting", name)
+		waiter.Wait()
+		return nil // we assume the process with the lock succeeded, rather than hammering this execution path again
 	}
 	defer func() {
-		if err := storage.UnlockRegister(name); err != nil {
-			log.Printf("[ERROR] Unable to unlock obtain lock for %v: %v", name, err)
+		if err := storage.Unlock(name); err != nil {
+			log.Printf("[ERROR] Unable to unlock obtain call for %s: %v", name, err)
 		}
 	}()
 
@@ -249,16 +251,18 @@ func (c *ACMEClient) Renew(name string) error {
 		return err
 	}
 
-	// We must lock the renewal with the storage engine
-	if lockObtained, err := storage.LockRegister(name); err != nil {
+	waiter, err := storage.TryLock(name)
+	if err != nil {
 		return err
-	} else if !lockObtained {
-		log.Printf("[INFO] Certificate for %v is already being renewed elsewhere", name)
-		return nil
+	}
+	if waiter != nil {
+		log.Printf("[INFO] Certificate for %s is already being renewed elsewhere and stored; waiting", name)
+		waiter.Wait()
+		return nil // we assume the process with the lock succeeded, rather than hammering this execution path again
 	}
 	defer func() {
-		if err := storage.UnlockRegister(name); err != nil {
-			log.Printf("[ERROR] Unable to unlock renewal lock for %v: %v", name, err)
+		if err := storage.Unlock(name); err != nil {
+			log.Printf("[ERROR] Unable to unlock renew call for %s: %v", name, err)
 		}
 	}()
 

caddytls/config_test.go

@@ -142,8 +142,8 @@ func TestStorageForDefault(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if reflect.TypeOf(s).Name() != "FileStorage" {
-		t.Fatalf("Unexpected storage type: %v", reflect.TypeOf(s).Name())
+	if _, ok := s.(*FileStorage); !ok {
+		t.Fatalf("Unexpected storage type: %#v", s)
 	}
 }
 
@@ -179,8 +179,8 @@ func TestStorageForCustomNil(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if reflect.TypeOf(s).Name() != "FileStorage" {
-		t.Fatalf("Unexpected storage type: %v", reflect.TypeOf(s).Name())
+	if _, ok := s.(*FileStorage); !ok {
+		t.Fatalf("Unexpected storage type: %#v", s)
 	}
 }
 
@@ -202,11 +202,11 @@ func (s fakeStorage) DeleteSite(domain string) error {
 	panic("no impl")
 }
 
-func (s fakeStorage) LockRegister(domain string) (bool, error) {
+func (s fakeStorage) TryLock(domain string) (Waiter, error) {
 	panic("no impl")
 }
 
-func (s fakeStorage) UnlockRegister(domain string) error {
+func (s fakeStorage) Unlock(domain string) error {
 	panic("no impl")
 }
 

caddytls/filestorage.go

@@ -7,67 +7,74 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 
 	"github.com/mholt/caddy"
 )
 
 func init() {
-	RegisterStorageProvider("file", FileStorageCreator)
+	RegisterStorageProvider("file", NewFileStorage)
 }
 
 // storageBasePath is the root path in which all TLS/ACME assets are
 // stored. Do not change this value during the lifetime of the program.
 var storageBasePath = filepath.Join(caddy.AssetsPath(), "acme")
 
-// FileStorageCreator creates a new Storage instance backed by the local
-// disk. The resulting Storage instance is guaranteed to be non-nil if
-// there is no error. This can be used by "middleware" implementations that
-// may want to proxy the disk storage.
-func FileStorageCreator(caURL *url.URL) (Storage, error) {
-	return FileStorage(filepath.Join(storageBasePath, caURL.Host)), nil
+// NewFileStorage is a StorageConstructor function that creates a new
+// Storage instance backed by the local disk. The resulting Storage
+// instance is guaranteed to be non-nil if there is no error.
+func NewFileStorage(caURL *url.URL) (Storage, error) {
+	return &FileStorage{
+		Path:      filepath.Join(storageBasePath, caURL.Host),
+		nameLocks: make(map[string]*sync.WaitGroup),
+	}, nil
 }
 
-// FileStorage is a root directory and facilitates forming file paths derived
-// from it. It is used to get file paths in a consistent, cross- platform way
-// for persisting ACME assets on the file system.
-type FileStorage string
+// FileStorage facilitates forming file paths derived from a root
+// directory. It is used to get file paths in a consistent,
+// cross-platform way or persisting ACME assets on the file system.
+type FileStorage struct {
+	Path        string
+	nameLocks   map[string]*sync.WaitGroup
+	nameLocksMu sync.Mutex
+}
 
 // sites gets the directory that stores site certificate and keys.
-func (s FileStorage) sites() string {
-	return filepath.Join(string(s), "sites")
+func (s *FileStorage) sites() string {
+	return filepath.Join(s.Path, "sites")
 }
 
 // site returns the path to the folder containing assets for domain.
-func (s FileStorage) site(domain string) string {
+func (s *FileStorage) site(domain string) string {
 	domain = strings.ToLower(domain)
 	return filepath.Join(s.sites(), domain)
 }
 
 // siteCertFile returns the path to the certificate file for domain.
-func (s FileStorage) siteCertFile(domain string) string {
+func (s *FileStorage) siteCertFile(domain string) string {
 	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".crt")
 }
 
 // siteKeyFile returns the path to domain's private key file.
-func (s FileStorage) siteKeyFile(domain string) string {
+func (s *FileStorage) siteKeyFile(domain string) string {
 	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".key")
 }
 
 // siteMetaFile returns the path to the domain's asset metadata file.
-func (s FileStorage) siteMetaFile(domain string) string {
+func (s *FileStorage) siteMetaFile(domain string) string {
 	domain = strings.ToLower(domain)
 	return filepath.Join(s.site(domain), domain+".json")
 }
 
 // users gets the directory that stores account folders.
-func (s FileStorage) users() string {
-	return filepath.Join(string(s), "users")
+func (s *FileStorage) users() string {
+	return filepath.Join(s.Path, "users")
 }
 
 // user gets the account folder for the user with email
-func (s FileStorage) user(email string) string {
+func (s *FileStorage) user(email string) string {
 	if email == "" {
 		email = emptyEmail
 	}
@@ -89,7 +96,7 @@ func emailUsername(email string) string {
 
 // userRegFile gets the path to the registration file for the user with the
 // given email address.
-func (s FileStorage) userRegFile(email string) string {
+func (s *FileStorage) userRegFile(email string) string {
 	if email == "" {
 		email = emptyEmail
 	}
@@ -103,7 +110,7 @@ func (s FileStorage) userRegFile(email string) string {
 
 // userKeyFile gets the path to the private key file for the user with the
 // given email address.
-func (s FileStorage) userKeyFile(email string) string {
+func (s *FileStorage) userKeyFile(email string) string {
 	if email == "" {
 		email = emptyEmail
 	}
@@ -117,7 +124,7 @@ func (s FileStorage) userKeyFile(email string) string {
 
 // readFile abstracts a simple ioutil.ReadFile, making sure to return an
 // ErrNotExist instance when the file is not found.
-func (s FileStorage) readFile(file string) ([]byte, error) {
+func (s *FileStorage) readFile(file string) ([]byte, error) {
 	b, err := ioutil.ReadFile(file)
 	if os.IsNotExist(err) {
 		return nil, ErrNotExist(err)
@@ -127,7 +134,7 @@ func (s FileStorage) readFile(file string) ([]byte, error) {
 
 // SiteExists implements Storage.SiteExists by checking for the presence of
 // cert and key files.
-func (s FileStorage) SiteExists(domain string) (bool, error) {
+func (s *FileStorage) SiteExists(domain string) (bool, error) {
 	_, err := os.Stat(s.siteCertFile(domain))
 	if os.IsNotExist(err) {
 		return false, nil
@@ -144,7 +151,7 @@ func (s FileStorage) SiteExists(domain string) (bool, error) {
 
 // LoadSite implements Storage.LoadSite by loading it from disk. If it is not
 // present, an instance of ErrNotExist is returned.
-func (s FileStorage) LoadSite(domain string) (*SiteData, error) {
+func (s *FileStorage) LoadSite(domain string) (*SiteData, error) {
 	var err error
 	siteData := new(SiteData)
 	siteData.Cert, err = s.readFile(s.siteCertFile(domain))
@@ -164,7 +171,7 @@ func (s FileStorage) LoadSite(domain string) (*SiteData, error) {
 
 // StoreSite implements Storage.StoreSite by writing it to disk. The base
 // directories needed for the file are automatically created as needed.
-func (s FileStorage) StoreSite(domain string, data *SiteData) error {
+func (s *FileStorage) StoreSite(domain string, data *SiteData) error {
 	err := os.MkdirAll(s.site(domain), 0700)
 	if err != nil {
 		return fmt.Errorf("making site directory: %v", err)
@@ -186,7 +193,7 @@ func (s FileStorage) StoreSite(domain string, data *SiteData) error {
 
 // DeleteSite implements Storage.DeleteSite by deleting just the cert from
 // disk. If it is not present, an instance of ErrNotExist is returned.
-func (s FileStorage) DeleteSite(domain string) error {
+func (s *FileStorage) DeleteSite(domain string) error {
 	err := os.Remove(s.siteCertFile(domain))
 	if err != nil {
 		if os.IsNotExist(err) {
@@ -197,21 +204,9 @@ func (s FileStorage) DeleteSite(domain string) error {
 	return nil
 }
 
-// LockRegister implements Storage.LockRegister by just returning true because
-// it is not a multi-server storage implementation.
-func (s FileStorage) LockRegister(domain string) (bool, error) {
-	return true, nil
-}
-
-// UnlockRegister implements Storage.UnlockRegister as a no-op because it is
-// not a multi-server storage implementation.
-func (s FileStorage) UnlockRegister(domain string) error {
-	return nil
-}
-
 // LoadUser implements Storage.LoadUser by loading it from disk. If it is not
 // present, an instance of ErrNotExist is returned.
-func (s FileStorage) LoadUser(email string) (*UserData, error) {
+func (s *FileStorage) LoadUser(email string) (*UserData, error) {
 	var err error
 	userData := new(UserData)
 	userData.Reg, err = s.readFile(s.userRegFile(email))
@@ -227,7 +222,7 @@ func (s FileStorage) LoadUser(email string) (*UserData, error) {
 
 // StoreUser implements Storage.StoreUser by writing it to disk. The base
 // directories needed for the file are automatically created as needed.
-func (s FileStorage) StoreUser(email string, data *UserData) error {
+func (s *FileStorage) StoreUser(email string, data *UserData) error {
 	err := os.MkdirAll(s.user(email), 0700)
 	if err != nil {
 		return fmt.Errorf("making user directory: %v", err)
@@ -243,11 +238,41 @@ func (s FileStorage) StoreUser(email string, data *UserData) error {
 	return nil
 }
 
+// TryLock attempts to get a lock for name, otherwise it returns
+// a Waiter value to wait until the other process is finished.
+func (s *FileStorage) TryLock(name string) (Waiter, error) {
+	s.nameLocksMu.Lock()
+	defer s.nameLocksMu.Unlock()
+	wg, ok := s.nameLocks[name]
+	if ok {
+		// lock already obtained, let caller wait on it
+		return wg, nil
+	}
+	// caller gets lock
+	wg = new(sync.WaitGroup)
+	wg.Add(1)
+	s.nameLocks[name] = wg
+	return nil, nil
+}
+
+// Unlock unlocks name.
+func (s *FileStorage) Unlock(name string) error {
+	s.nameLocksMu.Lock()
+	defer s.nameLocksMu.Unlock()
+	wg, ok := s.nameLocks[name]
+	if !ok {
+		return fmt.Errorf("FileStorage: no lock to release for %s", name)
+	}
+	wg.Done()
+	delete(s.nameLocks, name)
+	return nil
+}
+
 // MostRecentUserEmail implements Storage.MostRecentUserEmail by finding the
 // most recently written sub directory in the users' directory. It is named
 // after the email address. This corresponds to the most recent call to
 // StoreUser.
-func (s FileStorage) MostRecentUserEmail() string {
+func (s *FileStorage) MostRecentUserEmail() string {
 	userDirs, err := ioutil.ReadDir(s.users())
 	if err != nil {
 		return ""

caddytls/storage.go

@@ -2,10 +2,10 @@ package caddytls
 
 import "net/url"
 
-// StorageCreator is a function type that is used in the Config to instantiate
-// a new Storage instance. This function can return a nil Storage even without
-// an error.
-type StorageCreator func(caURL *url.URL) (Storage, error)
+// StorageConstructor is a function type that is used in the Config to
+// instantiate a new Storage instance. This function can return a nil
+// Storage even without an error.
+type StorageConstructor func(caURL *url.URL) (Storage, error)
 
 // SiteData contains persisted items pertaining to an individual site.
 type SiteData struct {
@@ -34,6 +34,34 @@ type Storage interface {
 	// successfully (without DeleteSite having been called, of course).
 	SiteExists(domain string) (bool, error)
 
+	// TryLock is called before Caddy attempts to obtain or renew a
+	// certificate for a certain name and store it. From the perspective
+	// of this method and its companion Unlock, the actions of
+	// obtaining/renewing and then storing the certificate are atomic,
+	// and both should occur within a lock. This prevents multiple
+	// processes -- maybe distributed ones -- from stepping on each
+	// other's space in the same shared storage, and from spamming
+	// certificate providers with multiple, redundant requests.
+	//
+	// If a lock could be obtained, (nil, nil) is returned and you may
+	// continue normally. If not (meaning another process is already
+	// working on that name), a Waiter value will be returned upon
+	// which you can Wait() until it is finished, and then return
+	// when it unblocks. If waiting, do not unlock!
+	//
+	// To prevent deadlocks, all implementations (where this concern
+	// is relevant) should put a reasonable expiration on the lock in
+	// case Unlock is unable to be called due to some sort of storage
+	// system failure or crash.
+	TryLock(name string) (Waiter, error)
+
+	// Unlock unlocks the mutex for name. Only callers of TryLock who
+	// successfully obtained the lock (no Waiter value was returned)
+	// should call this method, and it should be called only after
+	// the obtain/renew and store are finished, even if there was
+	// an error (or a timeout).
+	Unlock(name string) error
+
 	// LoadSite obtains the site data from storage for the given domain and
 	// returns it. If data for the domain does not exist, an error value
 	// of type ErrNotExist is returned. For multi-server storage, care
@@ -54,29 +82,6 @@ type Storage interface {
 	// the site does not exist, an error value of type ErrNotExist is returned.
 	DeleteSite(domain string) error
 
-	// LockRegister is called before Caddy attempts to obtain or renew a
-	// certificate. This function is used as a mutex/semaphore for making
-	// sure something else isn't already attempting obtain/renew. It should
-	// return true (without error) if the lock is successfully obtained
-	// meaning nothing else is attempting renewal. It should return false
-	// (without error) if this domain is already locked by something else
-	// attempting renewal. As a general rule, if this isn't multi-server
-	// shared storage, this should always return true. To prevent deadlocks
-	// for multi-server storage, all internal implementations should put a
-	// reasonable expiration on this lock in case UnlockRegister is unable to
-	// be called due to system crash. Errors should only be returned in
-	// exceptional cases. Any error will prevent renewal.
-	LockRegister(domain string) (bool, error)
-
-	// UnlockRegister is called after Caddy has attempted to obtain or renew
-	// a certificate, regardless of whether it was successful. If
-	// LockRegister essentially just returns true because this is not
-	// multi-server storage, this can be a no-op. Otherwise this should
-	// attempt to unlock the lock obtained in this process by LockRegister.
-	// If no lock exists, the implementation should not return an error. An
-	// error is only for exceptional cases.
-	UnlockRegister(domain string) error
-
 	// LoadUser obtains user data from storage for the given email and
 	// returns it. If data for the email does not exist, an error value
 	// of type ErrNotExist is returned. Multi-server implementations
@@ -101,3 +106,8 @@ type Storage interface {
 type ErrNotExist interface {
 	error
 }
+
+// Waiter is a type that can block until a storage lock is released.
+type Waiter interface {
+	Wait()
+}

caddytls/storagetest/memorystorage.go

@@ -97,15 +97,15 @@ func (s *InMemoryStorage) DeleteSite(domain string) error {
 	return nil
 }
 
-// LockRegister implements Storage.LockRegister by just returning true because
-// it is not a multi-server storage implementation.
-func (s *InMemoryStorage) LockRegister(domain string) (bool, error) {
-	return true, nil
+// TryLock implements Storage.TryLock by returning nil values because it
+// is not a multi-server storage implementation.
+func (s *InMemoryStorage) TryLock(domain string) (caddytls.Waiter, error) {
+	return nil, nil
 }
 
-// UnlockRegister implements Storage.UnlockRegister as a no-op because it is
+// Unlock implements Storage.Unlock as a no-op because it is
 // not a multi-server storage implementation.
-func (s *InMemoryStorage) UnlockRegister(domain string) error {
+func (s *InMemoryStorage) Unlock(domain string) error {
 	return nil
 }
 

caddytls/storagetest/storagetest_test.go

@@ -2,11 +2,12 @@ package storagetest
 
 import (
 	"fmt"
-	"github.com/mholt/caddy/caddytls"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
+
+	"github.com/mholt/caddy/caddytls"
 )
 
 // TestFileStorage tests the file storage set with the test harness in this
@@ -14,7 +15,7 @@ import (
 func TestFileStorage(t *testing.T) {
 	emailCounter := 0
 	storageTest := &StorageTest{
-		Storage:  caddytls.FileStorage("./testdata"),
+		Storage:  &caddytls.FileStorage{Path: "./testdata"}, // nameLocks isn't made here, but it's okay because the tests don't call TryLock or Unlock
 		PostTest: func() { os.RemoveAll("./testdata") },
 		AfterUserEmailStore: func(email string) error {
 			// We need to change the dir mod time to show a

caddytls/tls.go

@@ -169,10 +169,10 @@ var (
 	DefaultKeyType = acme.RSA2048
 )
 
-var storageProviders = make(map[string]StorageCreator)
+var storageProviders = make(map[string]StorageConstructor)
 
 // RegisterStorageProvider registers provider by name for storing tls data
-func RegisterStorageProvider(name string, provider StorageCreator) {
+func RegisterStorageProvider(name string, provider StorageConstructor) {
 	storageProviders[name] = provider
 	caddy.RegisterPlugin("tls.storage."+name, caddy.Plugin{})
 }

caddytls/tls_test.go

@@ -2,6 +2,7 @@ package caddytls
 
 import (
 	"os"
+	"sync"
 	"testing"
 
 	"github.com/xenolf/lego/acme"
@@ -79,11 +80,11 @@ func TestQualifiesForManagedTLS(t *testing.T) {
 }
 
 func TestSaveCertResource(t *testing.T) {
-	storage := FileStorage("./le_test_save")
+	storage := &FileStorage{Path: "./le_test_save", nameLocks: make(map[string]*sync.WaitGroup)}
 	defer func() {
-		err := os.RemoveAll(string(storage))
+		err := os.RemoveAll(storage.Path)
 		if err != nil {
-			t.Fatalf("Could not remove temporary storage directory (%s): %v", storage, err)
+			t.Fatalf("Could not remove temporary storage directory (%s): %v", storage.Path, err)
 		}
 	}()
 
@@ -125,11 +126,11 @@ func TestSaveCertResource(t *testing.T) {
 }
 
 func TestExistingCertAndKey(t *testing.T) {
-	storage := FileStorage("./le_test_existing")
+	storage := &FileStorage{Path: "./le_test_existing", nameLocks: make(map[string]*sync.WaitGroup)}
 	defer func() {
-		err := os.RemoveAll(string(storage))
+		err := os.RemoveAll(storage.Path)
 		if err != nil {
-			t.Fatalf("Could not remove temporary storage directory (%s): %v", storage, err)
+			t.Fatalf("Could not remove temporary storage directory (%s): %v", storage.Path, err)
 		}
 	}()
 

caddytls/user_test.go

@@ -7,11 +7,13 @@ import (
 	"crypto/rand"
 	"io"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 
-	"github.com/xenolf/lego/acme"
 	"os"
+
+	"github.com/xenolf/lego/acme"
 )
 
 func TestUser(t *testing.T) {
@@ -120,7 +122,7 @@ func TestGetUserAlreadyExists(t *testing.T) {
 }
 
 func TestGetEmail(t *testing.T) {
-	storageBasePath = string(testStorage) // to contain calls that create a new Storage...
+	storageBasePath = testStorage.Path // to contain calls that create a new Storage...
 
 	// let's not clutter up the output
 	origStdout := os.Stdout
@@ -180,8 +182,8 @@ func TestGetEmail(t *testing.T) {
 	}
 }
 
-var testStorage = FileStorage("./testdata")
+var testStorage = &FileStorage{Path: "./testdata", nameLocks: make(map[string]*sync.WaitGroup)}
 
-func (s FileStorage) clean() error {
-	return os.RemoveAll(string(s))
+func (s *FileStorage) clean() error {
+	return os.RemoveAll(s.Path)
 }