From abdf13ea306a0b872cf7ff3a02d758e3a519a2ea Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Thu, 8 Sep 2016 18:48:32 -0600 Subject: [PATCH] Improve TLS storage provider errors We renamed caddytls.ErrStorageNotFound to caddytls.ErrNotExist to more closely mirror the os package. We changed it to an interface wrapper so that the custom error message can be preserved. Returning only "data not found" was useless in debugging because we couldn't know the concrete value of the error (like what it was trying to load). Users can do a type assertion to determine if the error value is a "not found" error instead of doing an equality check. --- caddytls/filestorage.go | 83 +++++++++++++++++---------- caddytls/storage.go | 34 ++++++----- caddytls/storagetest/memorystorage.go | 10 ++-- caddytls/storagetest/storagetest.go | 23 +++++--- caddytls/user.go | 2 +- 5 files changed, 90 insertions(+), 62 deletions(-) diff --git a/caddytls/filestorage.go b/caddytls/filestorage.go index 4e9d3f5c..f5487a05 100644 --- a/caddytls/filestorage.go +++ b/caddytls/filestorage.go @@ -1,12 +1,14 @@ package caddytls import ( - "github.com/mholt/caddy" + "fmt" "io/ioutil" "net/url" "os" "path/filepath" "strings" + + "github.com/mholt/caddy" ) func init() { @@ -114,13 +116,14 @@ func (s FileStorage) userKeyFile(email string) string { } // readFile abstracts a simple ioutil.ReadFile, making sure to return an -// ErrStorageNotFound instance when the file is not found. +// ErrNotExist instance when the file is not found. func (s FileStorage) readFile(file string) ([]byte, error) { - byts, err := ioutil.ReadFile(file) + b, err := ioutil.ReadFile(file) if os.IsNotExist(err) { - return nil, ErrStorageNotFound + return nil, ErrNotExist(err) } - return byts, err + return b, err + } // SiteExists implements Storage.SiteExists by checking for the presence of @@ -141,18 +144,23 @@ func (s FileStorage) SiteExists(domain string) (bool, error) { } // LoadSite implements Storage.LoadSite by loading it from disk. If it is not -// present, the ErrStorageNotFound error instance is returned. +// present, an instance of ErrNotExist is returned. func (s FileStorage) LoadSite(domain string) (*SiteData, error) { var err error siteData := new(SiteData) siteData.Cert, err = s.readFile(s.siteCertFile(domain)) - if err == nil { - siteData.Key, err = s.readFile(s.siteKeyFile(domain)) + if err != nil { + return nil, err } - if err == nil { - siteData.Meta, err = s.readFile(s.siteMetaFile(domain)) + siteData.Key, err = s.readFile(s.siteKeyFile(domain)) + if err != nil { + return nil, err } - return siteData, err + siteData.Meta, err = s.readFile(s.siteMetaFile(domain)) + if err != nil { + return nil, err + } + return siteData, nil } // StoreSite implements Storage.StoreSite by writing it to disk. The base @@ -160,27 +168,34 @@ func (s FileStorage) LoadSite(domain string) (*SiteData, error) { func (s FileStorage) StoreSite(domain string, data *SiteData) error { err := os.MkdirAll(s.site(domain), 0700) if err != nil { - return err + return fmt.Errorf("making site directory: %v", err) } err = ioutil.WriteFile(s.siteCertFile(domain), data.Cert, 0600) - if err == nil { - err = ioutil.WriteFile(s.siteKeyFile(domain), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing certificate file: %v", err) } - if err == nil { - err = ioutil.WriteFile(s.siteMetaFile(domain), data.Meta, 0600) + err = ioutil.WriteFile(s.siteKeyFile(domain), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing key file: %v", err) } - return err + err = ioutil.WriteFile(s.siteMetaFile(domain), data.Meta, 0600) + if err != nil { + return fmt.Errorf("writing cert meta file: %v", err) + } + return nil } // DeleteSite implements Storage.DeleteSite by deleting just the cert from -// disk. If it is not present, the ErrStorageNotFound error instance is -// returned. +// disk. If it is not present, an instance of ErrNotExist is returned. func (s FileStorage) DeleteSite(domain string) error { err := os.Remove(s.siteCertFile(domain)) - if os.IsNotExist(err) { - return ErrStorageNotFound + if err != nil { + if os.IsNotExist(err) { + return ErrNotExist(err) + } + return err } - return err + return nil } // LockRegister implements Storage.LockRegister by just returning true because @@ -196,15 +211,19 @@ func (s FileStorage) UnlockRegister(domain string) error { } // LoadUser implements Storage.LoadUser by loading it from disk. If it is not -// present, the ErrStorageNotFound error instance is returned. +// present, an instance of ErrNotExist is returned. func (s FileStorage) LoadUser(email string) (*UserData, error) { var err error userData := new(UserData) userData.Reg, err = s.readFile(s.userRegFile(email)) - if err == nil { - userData.Key, err = s.readFile(s.userKeyFile(email)) + if err != nil { + return nil, err } - return userData, err + userData.Key, err = s.readFile(s.userKeyFile(email)) + if err != nil { + return nil, err + } + return userData, nil } // StoreUser implements Storage.StoreUser by writing it to disk. The base @@ -212,13 +231,17 @@ func (s FileStorage) LoadUser(email string) (*UserData, error) { func (s FileStorage) StoreUser(email string, data *UserData) error { err := os.MkdirAll(s.user(email), 0700) if err != nil { - return err + return fmt.Errorf("making user directory: %v", err) } err = ioutil.WriteFile(s.userRegFile(email), data.Reg, 0600) - if err == nil { - err = ioutil.WriteFile(s.userKeyFile(email), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing user registration file: %v", err) } - return err + err = ioutil.WriteFile(s.userKeyFile(email), data.Key, 0600) + if err != nil { + return fmt.Errorf("writing user key file: %v", err) + } + return nil } // MostRecentUserEmail implements Storage.MostRecentUserEmail by finding the diff --git a/caddytls/storage.go b/caddytls/storage.go index d2c1ff65..548fcb48 100644 --- a/caddytls/storage.go +++ b/caddytls/storage.go @@ -1,13 +1,6 @@ package caddytls -import ( - "errors" - "net/url" -) - -// ErrStorageNotFound is returned by Storage implementations when data is -// expected to be present but is not. -var ErrStorageNotFound = errors.New("data not found") +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 @@ -42,10 +35,10 @@ type Storage interface { SiteExists(domain string) (bool, error) // LoadSite obtains the site data from storage for the given domain and - // returns it. If data for the domain does not exist, the - // ErrStorageNotFound error instance is returned. For multi-server - // storage, care should be taken to make this load atomic to prevent - // race conditions that happen with multiple data loads. + // returns it. If data for the domain does not exist, an error value + // of type ErrNotExist is returned. For multi-server storage, care + // should be taken to make this load atomic to prevent race conditions + // that happen with multiple data loads. LoadSite(domain string) (*SiteData, error) // StoreSite persists the given site data for the given domain in @@ -58,8 +51,7 @@ type Storage interface { // DeleteSite deletes the site for the given domain from storage. // Multi-server implementations should attempt to make this atomic. If - // the site does not exist, the ErrStorageNotFound error instance is - // returned. + // 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 @@ -86,10 +78,10 @@ type Storage interface { 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, the - // ErrStorageNotFound error instance is returned. Multi-server - // implementations should take care to make this operation atomic for - // all loaded data items. + // returns it. If data for the email does not exist, an error value + // of type ErrNotExist is returned. Multi-server implementations + // should take care to make this operation atomic for all loaded + // data items. LoadUser(email string) (*UserData, error) // StoreUser persists the given user data for the given email in @@ -101,4 +93,10 @@ type Storage interface { // in StoreUser. The result is an empty string if there are no // persisted users in storage. MostRecentUserEmail() string + +// ErrNotExist is returned by Storage implementations when +// a resource is not found. It is similar to os.ErrNotExist +// except this is a type, not a variable. +type ErrNotExist interface { + error } diff --git a/caddytls/storagetest/memorystorage.go b/caddytls/storagetest/memorystorage.go index 138342d5..f83e9dd5 100644 --- a/caddytls/storagetest/memorystorage.go +++ b/caddytls/storagetest/memorystorage.go @@ -1,9 +1,11 @@ package storagetest import ( - "github.com/mholt/caddy/caddytls" + "errors" "net/url" "sync" + + "github.com/mholt/caddy/caddytls" ) // memoryMutex is a mutex used to control access to memoryStoragesByCAURL. @@ -65,7 +67,7 @@ func (s *InMemoryStorage) Clear() { func (s *InMemoryStorage) LoadSite(domain string) (*caddytls.SiteData, error) { siteData, ok := s.Sites[domain] if !ok { - return nil, caddytls.ErrStorageNotFound + return nil, caddytls.ErrNotExist(errors.New("not found")) } return siteData, nil } @@ -89,7 +91,7 @@ func (s *InMemoryStorage) StoreSite(domain string, data *caddytls.SiteData) erro // DeleteSite implements caddytls.Storage.DeleteSite in memory. func (s *InMemoryStorage) DeleteSite(domain string) error { if _, ok := s.Sites[domain]; !ok { - return caddytls.ErrStorageNotFound + return caddytls.ErrNotExist(errors.New("not found")) } delete(s.Sites, domain) return nil @@ -111,7 +113,7 @@ func (s *InMemoryStorage) UnlockRegister(domain string) error { func (s *InMemoryStorage) LoadUser(email string) (*caddytls.UserData, error) { userData, ok := s.Users[email] if !ok { - return nil, caddytls.ErrStorageNotFound + return nil, caddytls.ErrNotExist(errors.New("not found")) } return userData, nil } diff --git a/caddytls/storagetest/storagetest.go b/caddytls/storagetest/storagetest.go index 9c11d77b..c759dee8 100644 --- a/caddytls/storagetest/storagetest.go +++ b/caddytls/storagetest/storagetest.go @@ -6,8 +6,9 @@ import ( "bytes" "errors" "fmt" - "github.com/mholt/caddy/caddytls" "testing" + + "github.com/mholt/caddy/caddytls" ) // StorageTest is a test harness that contains tests to execute all exposed @@ -160,13 +161,15 @@ func (s *StorageTest) TestSite() error { defer s.runPostTest() // Should be a not-found error at first - if _, err := s.LoadSite("example.com"); err != caddytls.ErrStorageNotFound { - return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err) + _, err := s.LoadSite("example.com") + if _, ok := err.(caddytls.ErrNotExist); !ok { + return fmt.Errorf("Expected caddytls.ErrNotExist from load, got %T: %v", err, err) } // Delete should also be a not-found error at first - if err := s.DeleteSite("example.com"); err != caddytls.ErrStorageNotFound { - return fmt.Errorf("Expected ErrStorageNotFound from delete, got: %v", err) + err = s.DeleteSite("example.com") + if _, ok := err.(caddytls.ErrNotExist); !ok { + return fmt.Errorf("Expected ErrNotExist from delete, got: %v", err) } // Should store successfully and then load just fine @@ -197,8 +200,9 @@ func (s *StorageTest) TestSite() error { if err := s.DeleteSite("example.com"); err != nil { return err } - if _, err := s.LoadSite("example.com"); err != caddytls.ErrStorageNotFound { - return fmt.Errorf("Expected ErrStorageNotFound after delete, got: %v", err) + _, err = s.LoadSite("example.com") + if _, ok := err.(caddytls.ErrNotExist); !ok { + return fmt.Errorf("Expected caddytls.ErrNotExist after delete, got %T: %v", err, err) } return nil @@ -221,8 +225,9 @@ func (s *StorageTest) TestUser() error { defer s.runPostTest() // Should be a not-found error at first - if _, err := s.LoadUser("foo@example.com"); err != caddytls.ErrStorageNotFound { - return fmt.Errorf("Expected ErrStorageNotFound from load, got: %v", err) + _, err := s.LoadUser("foo@example.com") + if _, ok := err.(caddytls.ErrNotExist); !ok { + return fmt.Errorf("Expected caddytls.ErrNotExist from load, got %T: %v", err, err) } // Should store successfully and then load just fine diff --git a/caddytls/user.go b/caddytls/user.go index 40f484d6..3b6f83ef 100644 --- a/caddytls/user.go +++ b/caddytls/user.go @@ -103,7 +103,7 @@ func getUser(storage Storage, email string) (User, error) { // open user reg userData, err := storage.LoadUser(email) if err != nil { - if err == ErrStorageNotFound { + if _, ok := err.(ErrNotExist); ok { // create a new user return newUser(email) }