From cbb40173d04232eaa95923d478cc8c8f13a2e2c9 Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Thu, 5 Mar 2020 14:28:02 +0100 Subject: [PATCH] keystore: finish addressing encodedFSKeystore * Use Go's base32 library * Set repo to version 9 * Resolve linting problems and docs. * Merge EncodedFSKeystore into FSKeystore * Remove name limitations and adjust tests --- keystore/keystore.go | 158 +++++++---------------------------- keystore/keystore_test.go | 120 ++++---------------------- keystore/memkeystore.go | 19 ++--- keystore/memkeystore_test.go | 8 +- repo/fsrepo/fsrepo.go | 4 +- 5 files changed, 61 insertions(+), 248 deletions(-) diff --git a/keystore/keystore.go b/keystore/keystore.go index a40ab5cb060..463f90e007e 100644 --- a/keystore/keystore.go +++ b/keystore/keystore.go @@ -7,13 +7,16 @@ import ( "path/filepath" "strings" + base32 "encoding/base32" + logging "github.com/ipfs/go-log" ci "github.com/libp2p/go-libp2p-core/crypto" - base32 "github.com/whyrusleeping/base32" ) var log = logging.Logger("keystore") +var codec = base32.StdEncoding.WithPadding(base32.NoPadding) + // Keystore provides a key management interface type Keystore interface { // Has returns whether or not a key exist in the Keystore @@ -29,46 +32,20 @@ type Keystore interface { List() ([]string, error) } +// ErrNoSuchKey is an error message returned when no key of a given name was found. var ErrNoSuchKey = fmt.Errorf("no key by the given name was found") + +// ErrKeyExists is an error message returned when a key already exists var ErrKeyExists = fmt.Errorf("key by that name already exists, refusing to overwrite") +const keyFilenamePrefix = "key_" + // FSKeystore is a keystore backed by files in a given directory stored on disk. type FSKeystore struct { dir string } -func validateName(name string) error { - if name == "" { - return fmt.Errorf("key names must be at least one character") - } - - if strings.Contains(name, "/") { - return fmt.Errorf("key names may not contain slashes") - } - - if strings.HasPrefix(name, ".") { - return fmt.Errorf("key names may not begin with a period") - } - - return nil -} - -// NewKeystore is a factory for getting instance of Keystore interface implementation -func NewKeystore(dir string) (Keystore, error) { - return NewEncodedFSKeystore(dir) -} - -// NewEncodedFSKeystore is a factory for getting instance of EncodedFSKeystore -func NewEncodedFSKeystore(dir string) (*EncodedFSKeystore, error) { - keystore, err := NewFSKeystore(dir) - - if err != nil { - return nil, err - } - - return &EncodedFSKeystore{keystore}, nil -} - +// NewFSKeystore returns a new filesystem-backed keystore. func NewFSKeystore(dir string) (*FSKeystore, error) { _, err := os.Stat(dir) if err != nil { @@ -85,28 +62,25 @@ func NewFSKeystore(dir string) (*FSKeystore, error) { // Has returns whether or not a key exist in the Keystore func (ks *FSKeystore) Has(name string) (bool, error) { + name, err := encode(name) + if err != nil { + return false, err + } + kp := filepath.Join(ks.dir, name) - _, err := os.Stat(kp) + _, err = os.Stat(kp) if os.IsNotExist(err) { return false, nil } - - if err != nil { - return false, err - } - - if err := validateName(name); err != nil { - return false, err - } - - return true, nil + return err == nil, err } // Put stores a key in the Keystore, if a key with the same name already exists, returns ErrKeyExists func (ks *FSKeystore) Put(name string, k ci.PrivKey) error { - if err := validateName(name); err != nil { + name, err := encode(name) + if err != nil { return err } @@ -138,7 +112,8 @@ func (ks *FSKeystore) Put(name string, k ci.PrivKey) error { // Get retrieves a key from the Keystore if it exists, and returns ErrNoSuchKey // otherwise. func (ks *FSKeystore) Get(name string) (ci.PrivKey, error) { - if err := validateName(name); err != nil { + name, err := encode(name) + if err != nil { return nil, err } @@ -157,7 +132,8 @@ func (ks *FSKeystore) Get(name string) (ci.PrivKey, error) { // Delete removes a key from the Keystore func (ks *FSKeystore) Delete(name string) error { - if err := validateName(name); err != nil { + name, err := encode(name) + if err != nil { return err } @@ -181,25 +157,23 @@ func (ks *FSKeystore) List() ([]string, error) { list := make([]string, 0, len(dirs)) for _, name := range dirs { - err := validateName(name) + decodedName, err := decode(name) if err == nil { - list = append(list, name) + list = append(list, decodedName) } else { - log.Warnf("Ignoring the invalid keyfile: %s", name) + log.Errorf("Ignoring keyfile with invalid encoded filename: %s", name) } } return list, nil } -const keyFilenamePrefix = "key_" - func encode(name string) (string, error) { if name == "" { return "", fmt.Errorf("key name must be at least one character") } - encodedName := base32.RawStdEncoding.EncodeToString([]byte(name)) + encodedName := codec.EncodeToString([]byte(name)) log.Debugf("Encoded key name: %s to: %s", name, encodedName) return keyFilenamePrefix + strings.ToLower(encodedName), nil @@ -211,86 +185,12 @@ func decode(name string) (string, error) { } nameWithoutPrefix := strings.ToUpper(name[len(keyFilenamePrefix):]) - data, err := base32.RawStdEncoding.DecodeString(nameWithoutPrefix) - + decodedName, err := codec.DecodeString(nameWithoutPrefix) if err != nil { return "", err } - decodedName := string(data[:]) - log.Debugf("Decoded key name: %s to: %s", name, decodedName) - return decodedName, nil -} - -// EncodedFSKeystore is extension of FSKeystore that encodes the key filenames in base32 -type EncodedFSKeystore struct { - *FSKeystore -} - -// Has indicates if key is in keystore -func (ks *EncodedFSKeystore) Has(name string) (bool, error) { - encodedName, err := encode(name) - - if err != nil { - return false, err - } - - return ks.FSKeystore.Has(encodedName) -} - -// Put places key into the keystore -func (ks *EncodedFSKeystore) Put(name string, k ci.PrivKey) error { - encodedName, err := encode(name) - - if err != nil { - return err - } - - return ks.FSKeystore.Put(encodedName, k) -} - -// Get retrieves key by its name from the keystore -func (ks *EncodedFSKeystore) Get(name string) (ci.PrivKey, error) { - encodedName, err := encode(name) - - if err != nil { - return nil, err - } - - return ks.FSKeystore.Get(encodedName) -} - -// Delete removes key from the keystore -func (ks *EncodedFSKeystore) Delete(name string) error { - encodedName, err := encode(name) - - if err != nil { - return err - } - - return ks.FSKeystore.Delete(encodedName) -} - -// List returns list of all keys in keystore -func (ks *EncodedFSKeystore) List() ([]string, error) { - dirs, err := ks.FSKeystore.List() - - if err != nil { - return nil, err - } - - list := make([]string, 0, len(dirs)) - - for _, name := range dirs { - decodedName, err := decode(name) - if err == nil { - list = append(list, decodedName) - } else { - log.Warningf("Ignoring keyfile with invalid encoded filename: %s", name) - } - } - - return list, nil + return string(decodedName), nil } diff --git a/keystore/keystore_test.go b/keystore/keystore_test.go index 685e5d942ef..2a48b43e569 100644 --- a/keystore/keystore_test.go +++ b/keystore/keystore_test.go @@ -132,16 +132,16 @@ func TestKeystoreBasics(t *testing.T) { t.Fatal(err) } - if err := ks.Put("..///foo/", k1); err == nil { - t.Fatal("shouldnt be able to put a poorly named key") + if err := ks.Put("..///foo/", k1); err != nil { + t.Fatal(err) } if err := ks.Put("", k1); err == nil { t.Fatal("shouldnt be able to put a key with no name") } - if err := ks.Put(".foo", k1); err == nil { - t.Fatal("shouldnt be able to put a key with a 'hidden' name") + if err := ks.Put(".foo", k1); err != nil { + t.Fatal(err) } } @@ -166,12 +166,17 @@ func TestInvalidKeyFiles(t *testing.T) { t.Fatal(err) } - err = ioutil.WriteFile(filepath.Join(ks.dir, "valid"), bytes, 0644) + encodedName, err := encode("valid") + if err != nil { + t.Fatal(err) + } + + err = ioutil.WriteFile(filepath.Join(ks.dir, encodedName), bytes, 0644) if err != nil { t.Fatal(err) } - err = ioutil.WriteFile(filepath.Join(ks.dir, ".invalid"), bytes, 0644) + err = ioutil.WriteFile(filepath.Join(ks.dir, "z.invalid"), bytes, 0644) if err != nil { t.Fatal(err) } @@ -197,10 +202,6 @@ func TestInvalidKeyFiles(t *testing.T) { if err != nil { t.Fatal(err) } - - if _, err = ks.Has(".invalid"); err == nil { - t.Fatal("shouldnt be able to put a key with a 'hidden' name") - } } func TestNonExistingKey(t *testing.T) { @@ -231,12 +232,12 @@ func TestMakeKeystoreNoDir(t *testing.T) { } func assertGetKey(ks Keystore, name string, exp ci.PrivKey) error { - out_k, err := ks.Get(name) + outK, err := ks.Get(name) if err != nil { return err } - if !out_k.Equals(exp) { + if !outK.Equals(exp) { return fmt.Errorf("key we got out didnt match expectation") } @@ -255,7 +256,11 @@ func assertDirContents(dir string, exp []string) error { var names []string for _, fi := range finfos { - names = append(names, fi.Name()) + decodedName, err := decode(fi.Name()) + if err != nil { + return err + } + names = append(names, decodedName) } sort.Strings(names) @@ -271,92 +276,3 @@ func assertDirContents(dir string, exp []string) error { } return nil } - -func TestEncodedKeystoreBasics(t *testing.T) { - tdir, err := ioutil.TempDir("", "encoded-keystore-test") - if err != nil { - t.Fatal(err) - } - - ks, err := NewEncodedFSKeystore(tdir) - if err != nil { - t.Fatal(err) - } - - l, err := ks.List() - if err != nil { - t.Fatal(err) - } - - if len(l) != 0 { - t.Fatal("expected no keys") - } - - k1 := privKeyOrFatal(t) - k1Name, err := encode("foo") - if err != nil { - t.Fatal(err) - } - - k2 := privKeyOrFatal(t) - k2Name, err := encode("bar") - if err != nil { - t.Fatal(err) - } - - err = ks.Put("foo", k1) - if err != nil { - t.Fatal(err) - } - - err = ks.Put("bar", k2) - if err != nil { - t.Fatal(err) - } - - l, err = ks.List() - if err != nil { - t.Fatal(err) - } - - sort.Strings(l) - if l[0] != "bar" || l[1] != "foo" { - t.Fatal("wrong entries listed") - } - - if err := assertDirContents(tdir, []string{k1Name, k2Name}); err != nil { - t.Fatal(err) - } - - exist, err := ks.Has("foo") - if !exist { - t.Fatal("should know it has a key named foo") - } - if err != nil { - t.Fatal(err) - } - - if err := ks.Delete("bar"); err != nil { - t.Fatal(err) - } - - if err := assertDirContents(tdir, []string{k1Name}); err != nil { - t.Fatal(err) - } - - if err := assertGetKey(ks, "foo", k1); err != nil { - t.Fatal(err) - } - - if err := ks.Put("..///foo/", k1); err != nil { - t.Fatal(err) - } - - if err := ks.Put("", k1); err == nil { - t.Fatal("shouldnt be able to put a key with no name") - } - - if err := ks.Put(".foo", k1); err != nil { - t.Fatal(err) - } -} diff --git a/keystore/memkeystore.go b/keystore/memkeystore.go index 4067bbce29a..c96985252ac 100644 --- a/keystore/memkeystore.go +++ b/keystore/memkeystore.go @@ -1,6 +1,10 @@ package keystore -import ci "github.com/libp2p/go-libp2p-core/crypto" +import ( + "errors" + + ci "github.com/libp2p/go-libp2p-core/crypto" +) // MemKeystore is an in memory keystore implementation that is not persisted to // any backing storage. @@ -8,6 +12,7 @@ type MemKeystore struct { keys map[string]ci.PrivKey } +// NewMemKeystore creates a MemKeystore. func NewMemKeystore() *MemKeystore { return &MemKeystore{make(map[string]ci.PrivKey)} } @@ -20,8 +25,8 @@ func (mk *MemKeystore) Has(name string) (bool, error) { // Put store a key in the Keystore func (mk *MemKeystore) Put(name string, k ci.PrivKey) error { - if err := validateName(name); err != nil { - return err + if name == "" { + return errors.New("key name must be at least one character") } _, ok := mk.keys[name] @@ -35,10 +40,6 @@ func (mk *MemKeystore) Put(name string, k ci.PrivKey) error { // Get retrieve a key from the Keystore func (mk *MemKeystore) Get(name string) (ci.PrivKey, error) { - if err := validateName(name); err != nil { - return nil, err - } - k, ok := mk.keys[name] if !ok { return nil, ErrNoSuchKey @@ -49,10 +50,6 @@ func (mk *MemKeystore) Get(name string) (ci.PrivKey, error) { // Delete remove a key from the Keystore func (mk *MemKeystore) Delete(name string) error { - if err := validateName(name); err != nil { - return err - } - delete(mk.keys, name) return nil } diff --git a/keystore/memkeystore_test.go b/keystore/memkeystore_test.go index 62533d54b92..a7214893a3f 100644 --- a/keystore/memkeystore_test.go +++ b/keystore/memkeystore_test.go @@ -85,15 +85,15 @@ func TestMemKeyStoreBasics(t *testing.T) { t.Fatal(err) } - if err := ks.Put("..///foo/", k1); err == nil { - t.Fatal("shouldnt be able to put a poorly named key") + if err := ks.Put("..///foo/", k1); err != nil { + t.Fatal(err) } if err := ks.Put("", k1); err == nil { t.Fatal("shouldnt be able to put a key with no name") } - if err := ks.Put(".foo", k1); err == nil { - t.Fatal("shouldnt be able to put a key with a 'hidden' name") + if err := ks.Put(".foo", k1); err != nil { + t.Fatal(err) } } diff --git a/repo/fsrepo/fsrepo.go b/repo/fsrepo/fsrepo.go index 59728dc466d..a14e96ff25c 100644 --- a/repo/fsrepo/fsrepo.go +++ b/repo/fsrepo/fsrepo.go @@ -36,7 +36,7 @@ const LockFile = "repo.lock" var log = logging.Logger("fsrepo") // version number that we are currently expecting to see -var RepoVersion = 8 +var RepoVersion = 9 var migrationInstructions = `See https://github.com/ipfs/fs-repo-migrations/blob/master/run.md Sorry for the inconvenience. In the future, these will run automatically.` @@ -385,7 +385,7 @@ func (r *FSRepo) openConfig() error { func (r *FSRepo) openKeystore() error { ksp := filepath.Join(r.path, "keystore") - ks, err := keystore.NewKeystore(ksp) + ks, err := keystore.NewFSKeystore(ksp) if err != nil { return err }