From 8ecd543519c09bba63368bd799e194b82978e9f8 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Mon, 19 Sep 2016 17:24:34 -0600 Subject: [PATCH] Refactor and improve TLS storage code (related to locking) --- caddy/caddymain/run.go | 2 +- caddytls/client.go | 32 ++++--- caddytls/config_test.go | 12 +-- caddytls/filestorage.go | 109 ++++++++++++++--------- caddytls/storage.go | 64 +++++++------ caddytls/storagetest/memorystorage.go | 12 +-- caddytls/storagetest/storagetest_test.go | 5 +- caddytls/tls.go | 4 +- caddytls/tls_test.go | 13 +-- caddytls/user_test.go | 12 +-- 10 files changed, 154 insertions(+), 111 deletions(-) diff --git a/caddy/caddymain/run.go b/caddy/caddymain/run.go index 1720c58a..ebd6a3b4 100644 --- a/caddy/caddymain/run.go +++ b/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) diff --git a/caddytls/client.go b/caddytls/client.go index da510f08..8345c431 100644 --- a/caddytls/client.go +++ b/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) } }() diff --git a/caddytls/config_test.go b/caddytls/config_test.go index c12c98a0..2ca6547c 100644 --- a/caddytls/config_test.go +++ b/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") } diff --git a/caddytls/filestorage.go b/caddytls/filestorage.go index f0dc03a3..e3a3a53e 100644 --- a/caddytls/filestorage.go +++ b/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 "" diff --git a/caddytls/storage.go b/caddytls/storage.go index 2815ec83..27df4b3a 100644 --- a/caddytls/storage.go +++ b/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() +} diff --git a/caddytls/storagetest/memorystorage.go b/caddytls/storagetest/memorystorage.go index f83e9dd5..d2831590 100644 --- a/caddytls/storagetest/memorystorage.go +++ b/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 } diff --git a/caddytls/storagetest/storagetest_test.go b/caddytls/storagetest/storagetest_test.go index e602e405..7902ee74 100644 --- a/caddytls/storagetest/storagetest_test.go +++ b/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 diff --git a/caddytls/tls.go b/caddytls/tls.go index 607dfe46..50c9814a 100644 --- a/caddytls/tls.go +++ b/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{}) } diff --git a/caddytls/tls_test.go b/caddytls/tls_test.go index fb7d59dc..dccb8ec9 100644 --- a/caddytls/tls_test.go +++ b/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) } }() diff --git a/caddytls/user_test.go b/caddytls/user_test.go index 10393d21..77d04d4a 100644 --- a/caddytls/user_test.go +++ b/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) }