diff --git a/client/swagger/models/model_wallet.go b/client/swagger/models/model_wallet.go index 335f3129d..93614fe8d 100644 --- a/client/swagger/models/model_wallet.go +++ b/client/swagger/models/model_wallet.go @@ -26,7 +26,7 @@ type ModelWallet struct { // id ID int64 `json:"id,omitempty"` - // absolute path to key file + // keystore-relative name (typically the address) KeyPath string `json:"keyPath,omitempty"` // local, yubikey, aws-kms, etc diff --git a/docs/swagger/docs.go b/docs/swagger/docs.go index 25bd94729..f61d0b2a7 100644 --- a/docs/swagger/docs.go +++ b/docs/swagger/docs.go @@ -7625,7 +7625,7 @@ const docTemplate = `{ "type": "integer" }, "keyPath": { - "description": "absolute path to key file", + "description": "keystore-relative name (typically the address)", "type": "string" }, "keyStore": { diff --git a/docs/swagger/swagger.json b/docs/swagger/swagger.json index 8f8f07e6c..4629db093 100644 --- a/docs/swagger/swagger.json +++ b/docs/swagger/swagger.json @@ -7618,7 +7618,7 @@ "type": "integer" }, "keyPath": { - "description": "absolute path to key file", + "description": "keystore-relative name (typically the address)", "type": "string" }, "keyStore": { diff --git a/docs/swagger/swagger.yaml b/docs/swagger/swagger.yaml index c0e6259c9..6ddfcdd98 100644 --- a/docs/swagger/swagger.yaml +++ b/docs/swagger/swagger.yaml @@ -739,7 +739,7 @@ definitions: id: type: integer keyPath: - description: absolute path to key file + description: keystore-relative name (typically the address) type: string keyStore: description: local, yubikey, aws-kms, etc diff --git a/handler/wallet/export_keys.go b/handler/wallet/export_keys.go index 05069364e..6e43ea6f8 100644 --- a/handler/wallet/export_keys.go +++ b/handler/wallet/export_keys.go @@ -148,22 +148,19 @@ func exportOneKey(db *gorm.DB, ks keystore.KeyStore, actor legacyActorRow) (expo return false, fmt.Sprintf("actor %s: db query failed: %v", actor.ID, err) } - // save key to keystore - keyPath, _, err := ks.Put(actor.PrivateKey) + keyName, _, err := ks.Put(actor.PrivateKey) if err != nil { return false, fmt.Sprintf("actor %s: keystore write failed: %v", actor.ID, err) } - // create wallet record w := model.Wallet{ - KeyPath: keyPath, + KeyPath: keyName, KeyStore: "local", Address: addr.String(), ActorID: &actor.ID, } if err := db.Create(&w).Error; err != nil { - // cleanup keystore file on db failure - ks.Delete(keyPath) + ks.Delete(keyName) return false, fmt.Sprintf("actor %s: wallet create failed: %v", actor.ID, err) } diff --git a/handler/wallet/export_keys_test.go b/handler/wallet/export_keys_test.go index 137a16a30..c95566b00 100644 --- a/handler/wallet/export_keys_test.go +++ b/handler/wallet/export_keys_test.go @@ -195,7 +195,7 @@ func TestExportKeysHandler_MissingKeyFile(t *testing.T) { // create wallet record pointing to a nonexistent key file require.NoError(t, db.Create(&model.Wallet{ - KeyPath: "/nonexistent/key", + KeyPath: "nonexistent", KeyStore: "local", Address: testutil.TestWalletAddr, }).Error) @@ -218,12 +218,11 @@ func TestExportKeysHandler_CorruptKeyFile(t *testing.T) { addLegacyColumn(t, db) createLegacyActor(t, db, "f01234", testutil.TestWalletAddr, testutil.TestPrivateKeyHex) - // write garbage to the key file path - corruptPath := dir + "/corrupt" - require.NoError(t, os.WriteFile(corruptPath, []byte("garbage"), 0600)) + // write garbage as a key file inside the keystore dir + require.NoError(t, os.WriteFile(dir+"/corrupt", []byte("garbage"), 0600)) require.NoError(t, db.Create(&model.Wallet{ - KeyPath: corruptPath, + KeyPath: "corrupt", KeyStore: "local", Address: testutil.TestWalletAddr, }).Error) diff --git a/handler/wallet/import_keystore.go b/handler/wallet/import_keystore.go index c7ac525ae..a6ffa6b86 100644 --- a/handler/wallet/import_keystore.go +++ b/handler/wallet/import_keystore.go @@ -49,16 +49,16 @@ func (DefaultHandler) ImportKeystoreHandler( return nil, errors.Wrap(handlererror.ErrInvalidParameter, err.Error()) } - keyPath, _, err := ks.Put(request.PrivateKey) + keyName, _, err := ks.Put(request.PrivateKey) if err != nil { logger.Errorw("failed to save key to keystore", "err", err) return nil, errors.WithStack(err) } - logger.Infow("saved key to keystore", "address", addr.String(), "path", keyPath) + logger.Infow("saved key to keystore", "address", addr.String(), "name", keyName) walletRecord := model.Wallet{ - KeyPath: keyPath, + KeyPath: keyName, KeyStore: "local", Address: addr.String(), Name: request.Name, @@ -70,12 +70,12 @@ func (DefaultHandler) ImportKeystoreHandler( }) if util.IsDuplicateKeyError(err) { - // don't delete the key file — it belongs to the existing wallet record + // don't delete the key file -- it belongs to the existing wallet record return nil, errors.Wrap(handlererror.ErrDuplicateRecord, "wallet already imported") } if err != nil { - ks.Delete(keyPath) // cleanup only for non-duplicate failures + ks.Delete(keyName) // cleanup only for non-duplicate failures return nil, errors.WithStack(err) } diff --git a/model/wallet.go b/model/wallet.go index 556f16a57..b656a99ad 100644 --- a/model/wallet.go +++ b/model/wallet.go @@ -5,7 +5,7 @@ package model type Wallet struct { ID uint `gorm:"primaryKey" json:"id"` - KeyPath string `gorm:"uniqueIndex;not null" json:"keyPath"` // absolute path to key file + KeyPath string `gorm:"uniqueIndex;not null" json:"keyPath"` // keystore-relative name (typically the address) KeyStore string `gorm:"default:'local';not null" json:"keyStore"` // local, yubikey, aws-kms, etc Address string `gorm:"index;not null" json:"address"` // filecoin address (f1.../f3...) Name string `json:"name,omitempty"` // optional label diff --git a/util/keystore/keystore.go b/util/keystore/keystore.go index 3fdcf5e51..e26f9b946 100644 --- a/util/keystore/keystore.go +++ b/util/keystore/keystore.go @@ -4,22 +4,26 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/data-preservation-programs/go-synapse/signer" "github.com/filecoin-project/go-address" ) +// KeyStore stores keys by a short name (typically the filecoin address). +// The name is a relative identifier -- implementations resolve it against +// their own storage root. Callers must pass back the name returned by Put. type KeyStore interface { - Put(privateKey string) (path string, addr address.Address, err error) // saves key, returns path and address - Get(path string) (privateKey string, err error) // loads key from path - List() ([]KeyInfo, error) // lists all keys - Delete(path string) error // removes key - Has(path string) bool // checks if key exists + Put(privateKey string) (name string, addr address.Address, err error) + Get(name string) (privateKey string, err error) + List() ([]KeyInfo, error) + Delete(name string) error + Has(name string) bool } type KeyInfo struct { Address address.Address - Path string + Name string // relative identifier within the keystore } // filesystem keystore implementation @@ -34,6 +38,17 @@ func NewLocalKeyStore(dir string) (*LocalKeyStore, error) { return &LocalKeyStore{dir: dir}, nil } +// reject names that would escape the keystore dir or address another directory +func validateName(name string) error { + if name == "" { + return fmt.Errorf("empty key name") + } + if strings.ContainsRune(name, os.PathSeparator) || name == "." || name == ".." { + return fmt.Errorf("invalid key name %q: must be a basename", name) + } + return nil +} + // lotus wallet export format expected (hex-encoded JSON with Type and PrivateKey) func (ks *LocalKeyStore) Put(privateKey string) (string, address.Address, error) { addr, err := AddressFromExport(privateKey) @@ -41,19 +56,21 @@ func (ks *LocalKeyStore) Put(privateKey string) (string, address.Address, error) return "", address.Undef, fmt.Errorf("failed to derive address from private key: %w", err) } - // file named by address (f1.../f3...) - filename := addr.String() - path := filepath.Join(ks.dir, filename) + name := addr.String() + path := filepath.Join(ks.dir, name) if err := os.WriteFile(path, []byte(privateKey), 0600); err != nil { return "", address.Undef, fmt.Errorf("failed to write key file: %w", err) } - return path, addr, nil + return name, addr, nil } -func (ks *LocalKeyStore) Get(path string) (string, error) { - data, err := os.ReadFile(path) +func (ks *LocalKeyStore) Get(name string) (string, error) { + if err := validateName(name); err != nil { + return "", err + } + data, err := os.ReadFile(filepath.Join(ks.dir, name)) if err != nil { return "", fmt.Errorf("failed to read key file: %w", err) } @@ -72,8 +89,8 @@ func (ks *LocalKeyStore) List() ([]KeyInfo, error) { continue } - path := filepath.Join(ks.dir, entry.Name()) - data, err := os.ReadFile(path) + name := entry.Name() + data, err := os.ReadFile(filepath.Join(ks.dir, name)) if err != nil { continue // skip unreadable } @@ -85,22 +102,28 @@ func (ks *LocalKeyStore) List() ([]KeyInfo, error) { keys = append(keys, KeyInfo{ Address: addr, - Path: path, + Name: name, }) } return keys, nil } -func (ks *LocalKeyStore) Delete(path string) error { - if err := os.Remove(path); err != nil { +func (ks *LocalKeyStore) Delete(name string) error { + if err := validateName(name); err != nil { + return err + } + if err := os.Remove(filepath.Join(ks.dir, name)); err != nil { return fmt.Errorf("failed to delete key file: %w", err) } return nil } -func (ks *LocalKeyStore) Has(path string) bool { - _, err := os.Stat(path) +func (ks *LocalKeyStore) Has(name string) bool { + if err := validateName(name); err != nil { + return false + } + _, err := os.Stat(filepath.Join(ks.dir, name)) return err == nil } diff --git a/util/keystore/keystore_test.go b/util/keystore/keystore_test.go index edb5cbd71..7086bb270 100644 --- a/util/keystore/keystore_test.go +++ b/util/keystore/keystore_test.go @@ -32,23 +32,14 @@ func TestLocalKeyStore_PutAndGet(t *testing.T) { // Use test key privateKey := getTestKey(0) - // Put the key - path, addr, err := ks.Put(privateKey) + name, addr, err := ks.Put(privateKey) require.NoError(t, err) - require.NotEmpty(t, path) require.NotEqual(t, address.Undef, addr) - // Verify file exists - require.FileExists(t, path) + require.Equal(t, addr.String(), name) + require.FileExists(t, filepath.Join(tmpdir, name)) - // Verify path is in the keystore directory - require.Contains(t, path, tmpdir) - - // Verify filename matches address - require.Equal(t, filepath.Join(tmpdir, addr.String()), path) - - // Get the key back - loadedKey, err := ks.Get(path) + loadedKey, err := ks.Get(name) require.NoError(t, err) require.Equal(t, privateKey, loadedKey) } @@ -66,19 +57,15 @@ func TestLocalKeyStore_List(t *testing.T) { // Add a key (we only have one unique test key, so add it once) key1 := getTestKey(0) - path1, addr1, err := ks.Put(key1) + name1, addr1, err := ks.Put(key1) require.NoError(t, err) - // List should return it keys, err = ks.List() require.NoError(t, err) require.Len(t, keys, 1) - // Verify address matches require.Equal(t, addr1, keys[0].Address) - - // Verify path is correct - require.Equal(t, path1, keys[0].Path) + require.Equal(t, name1, keys[0].Name) } func TestLocalKeyStore_Delete(t *testing.T) { @@ -86,22 +73,17 @@ func TestLocalKeyStore_Delete(t *testing.T) { ks, err := NewLocalKeyStore(tmpdir) require.NoError(t, err) - // Add a key privateKey := getTestKey(0) - path, _, err := ks.Put(privateKey) + name, _, err := ks.Put(privateKey) require.NoError(t, err) - // Verify it exists - require.True(t, ks.Has(path)) + require.True(t, ks.Has(name)) - // Delete it - err = ks.Delete(path) + err = ks.Delete(name) require.NoError(t, err) - // Verify it's gone - require.False(t, ks.Has(path)) + require.False(t, ks.Has(name)) - // List should be empty keys, err := ks.List() require.NoError(t, err) require.Empty(t, keys) @@ -112,16 +94,17 @@ func TestLocalKeyStore_Has(t *testing.T) { ks, err := NewLocalKeyStore(tmpdir) require.NoError(t, err) - // Non-existent key - require.False(t, ks.Has(filepath.Join(tmpdir, "nonexistent"))) + require.False(t, ks.Has("nonexistent")) - // Add a key privateKey := getTestKey(0) - path, _, err := ks.Put(privateKey) + name, _, err := ks.Put(privateKey) require.NoError(t, err) - // Should exist - require.True(t, ks.Has(path)) + require.True(t, ks.Has(name)) + + // paths with separators are rejected + require.False(t, ks.Has(filepath.Join(tmpdir, name))) + require.False(t, ks.Has("../escape")) } func TestLocalKeyStore_InvalidKey(t *testing.T) { @@ -166,20 +149,17 @@ func TestLocalKeyStore_PutSameKeyTwice(t *testing.T) { ks, err := NewLocalKeyStore(tmpdir) require.NoError(t, err) - // Add a key privateKey := getTestKey(0) - path1, addr1, err := ks.Put(privateKey) + name1, addr1, err := ks.Put(privateKey) require.NoError(t, err) - // Add the same key again (should overwrite) - path2, addr2, err := ks.Put(privateKey) + // same key again should overwrite + name2, addr2, err := ks.Put(privateKey) require.NoError(t, err) - // Paths and addresses should be the same - require.Equal(t, path1, path2) + require.Equal(t, name1, name2) require.Equal(t, addr1, addr2) - // Should only have one key in the list keys, err := ks.List() require.NoError(t, err) require.Len(t, keys, 1)