Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Capability Fixes #7918

Merged
merged 8 commits into from
Nov 13, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ Ref: https://keepachangelog.com/en/1.0.0/
* `server/types.AppExporter` requires extra argument: `AppOptions`.
* `server.AddCommands` requires extra argument: `addStartFlags types.ModuleInitFlags`
* `x/crisis.NewAppModule` has a new attribute: `skipGenesisInvariants`. [PR](https://github.com/cosmos/cosmos-sdk/pull/7764)
* [#7918](https://github.com/cosmos/cosmos-sdk/pull/7918) Add x/capability safety checks:
* All outward facing APIs will now check that capability is not nil and name is not empty before performing any state-machine changes
* `SetIndex` has been renamed to `InitializeIndex`

### Features

Expand Down
2 changes: 1 addition & 1 deletion x/capability/genesis.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
// InitGenesis initializes the capability module's state from a provided genesis
// state.
func InitGenesis(ctx sdk.Context, k keeper.Keeper, genState types.GenesisState) {
if err := k.SetIndex(ctx, genState.Index); err != nil {
if err := k.InitializeIndex(ctx, genState.Index); err != nil {
panic(err)
}

Expand Down
46 changes: 39 additions & 7 deletions x/capability/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package keeper

import (
"fmt"
"strings"

"github.com/tendermint/tendermint/libs/log"

Expand Down Expand Up @@ -49,6 +50,8 @@ type (
}
)

// NewKeeper constructs a new CapabilityKeeper instance and initializes maps
// for capability map and scopedModules map.
func NewKeeper(cdc codec.BinaryMarshaler, storeKey, memKey sdk.StoreKey) *Keeper {
return &Keeper{
cdc: cdc,
Expand All @@ -67,6 +70,9 @@ func (k *Keeper) ScopeToModule(moduleName string) ScopedKeeper {
if k.sealed {
panic("cannot scope to module via a sealed capability keeper")
}
if strings.TrimSpace(moduleName) == "" {
panic("cannot scope to an empty module name")
}

if _, ok := k.scopedModules[moduleName]; ok {
panic(fmt.Sprintf("cannot create multiple scoped keepers for the same module name: %s", moduleName))
Expand Down Expand Up @@ -117,10 +123,10 @@ func (k *Keeper) InitializeAndSeal(ctx sdk.Context) {
k.sealed = true
}

// SetIndex sets the index to one (or greater) in InitChain according
// InitializeIndex sets the index to one (or greater) in InitChain according
// to the GenesisState. It must only be called once.
// It will panic if the provided index is 0, or if the index is already set.
func (k Keeper) SetIndex(ctx sdk.Context, index uint64) error {
func (k Keeper) InitializeIndex(ctx sdk.Context, index uint64) error {
if index == 0 {
panic("SetIndex requires index > 0")
}
Expand Down Expand Up @@ -200,6 +206,9 @@ func (k Keeper) InitializeCapability(ctx sdk.Context, index uint64, owners types
// Note, namespacing is completely local, which is safe since records are prefixed
// with the module name and no two ScopedKeeper can have the same module name.
func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capability, error) {
if strings.TrimSpace(name) == "" {
return nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
}
store := ctx.KVStore(sk.storeKey)

if _, ok := sk.GetCapability(ctx, name); ok {
Expand Down Expand Up @@ -247,6 +256,9 @@ func (sk ScopedKeeper) NewCapability(ctx sdk.Context, name string) (*types.Capab
// Note, the capability's forward mapping is indexed by a string which should
// contain its unique memory reference.
func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capability, name string) bool {
if strings.TrimSpace(name) == "" || cap == nil {
return false
}
return sk.GetCapabilityName(ctx, cap) == name
}

Expand All @@ -256,6 +268,12 @@ func (sk ScopedKeeper) AuthenticateCapability(ctx sdk.Context, cap *types.Capabi
// index. If the owner already exists, it will return an error. Otherwise, it will
// also set a forward and reverse index for the capability and capability name.
func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, name string) error {
if cap == nil {
return sdkerrors.Wrap(types.ErrNilCapability, "cannot claim nil capability")
}
if strings.TrimSpace(name) == "" {
return sdkerrors.Wrap(types.ErrInvalidCapabilityName, "capability name cannot be empty")
}
// update capability owner set
if err := sk.addOwner(ctx, cap, name); err != nil {
return err
Expand All @@ -282,6 +300,9 @@ func (sk ScopedKeeper) ClaimCapability(ctx sdk.Context, cap *types.Capability, n
// previously claimed or created. After releasing the capability, if no more
// owners exist, the capability will be globally removed.
func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability) error {
if cap == nil {
return sdkerrors.Wrap(types.ErrNilCapability, "cannot release nil capability")
}
name := sk.GetCapabilityName(ctx, cap)
if len(name) == 0 {
return sdkerrors.Wrap(types.ErrCapabilityNotOwned, sk.module)
Expand Down Expand Up @@ -321,6 +342,9 @@ func (sk ScopedKeeper) ReleaseCapability(ctx sdk.Context, cap *types.Capability)
// by name. The module is not allowed to retrieve capabilities which it does not
// own.
func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capability, bool) {
if strings.TrimSpace(name) == "" {
return nil, false
}
memStore := ctx.KVStore(sk.memKey)

key := types.RevCapabilityKey(sk.module, name)
Expand All @@ -332,15 +356,14 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab
// to still have the capability in the go map since changes to
// go map do not automatically get reverted on tx failure,
// so we delete here to remove unnecessary values in map
delete(sk.capMap, index)
// TODO: Delete index correctly from capMap by storing some reverse lookup
// in-memory map. Issue: https://github.com/cosmos/cosmos-sdk/issues/7805
return nil, false
}

cap := sk.capMap[index]
if cap == nil {
// delete key from store to remove unnecessary mapping
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a reason this was expected to happen before?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as we can tell, no. Maybe some prior bug which has since been fixed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, afaik this should never happen. I think it used to just ignore the inconsistency and move on, now we panic

memStore.Delete(key)
return nil, false
panic("capability found in memstore is missing from map")
}

return cap, true
Expand All @@ -349,14 +372,20 @@ func (sk ScopedKeeper) GetCapability(ctx sdk.Context, name string) (*types.Capab
// GetCapabilityName allows a module to retrieve the name under which it stored a given
// capability given the capability
func (sk ScopedKeeper) GetCapabilityName(ctx sdk.Context, cap *types.Capability) string {
if cap == nil {
return ""
}
memStore := ctx.KVStore(sk.memKey)

return string(memStore.Get(types.FwdCapabilityKey(sk.module, cap)))
}

// Get all the Owners that own the capability associated with the name this ScopedKeeper uses
// GetOwners all the Owners that own the capability associated with the name this ScopedKeeper uses
// to refer to the capability
func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.CapabilityOwners, bool) {
if strings.TrimSpace(name) == "" {
return nil, false
}
cap, ok := sk.GetCapability(ctx, name)
if !ok {
return nil, false
Expand All @@ -382,6 +411,9 @@ func (sk ScopedKeeper) GetOwners(ctx sdk.Context, name string) (*types.Capabilit
// The method returns an error if either the capability or the owners cannot be
// retreived from the memstore.
func (sk ScopedKeeper) LookupModules(ctx sdk.Context, name string) ([]string, *types.Capability, error) {
if strings.TrimSpace(name) == "" {
return nil, nil, sdkerrors.Wrap(types.ErrInvalidCapabilityName, "cannot lookup modules with empty capability name")
}
cap, ok := sk.GetCapability(ctx, name)
if !ok {
return nil, nil, sdkerrors.Wrap(types.ErrCapabilityNotFound, name)
Expand Down
18 changes: 18 additions & 0 deletions x/capability/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ func (suite *KeeperTestSuite) SetupTest() {

func (suite *KeeperTestSuite) TestInitializeAndSeal() {
sk := suite.keeper.ScopeToModule(banktypes.ModuleName)
suite.Require().Panics(func() {
suite.keeper.ScopeToModule(" ")
})

caps := make([]*types.Capability, 5)
// Get Latest Index before creating new ones to sychronize indices correctly
Expand Down Expand Up @@ -105,6 +108,10 @@ func (suite *KeeperTestSuite) TestNewCapability() {
suite.Require().True(ok)
suite.Require().Equal(cap, got)
suite.Require().True(cap == got, "expected memory addresses to be equal")

cap, err = sk.NewCapability(suite.ctx, " ")
suite.Require().Error(err)
suite.Require().Nil(cap)
}

func (suite *KeeperTestSuite) TestOriginalCapabilityKeeper() {
Expand Down Expand Up @@ -151,11 +158,15 @@ func (suite *KeeperTestSuite) TestAuthenticateCapability() {
badCap := types.NewCapability(100)
suite.Require().False(sk1.AuthenticateCapability(suite.ctx, badCap, "transfer"))
suite.Require().False(sk2.AuthenticateCapability(suite.ctx, badCap, "bond"))

suite.Require().False(sk1.AuthenticateCapability(suite.ctx, cap1, " "))
suite.Require().False(sk1.AuthenticateCapability(suite.ctx, nil, "transfer"))
}

func (suite *KeeperTestSuite) TestClaimCapability() {
sk1 := suite.keeper.ScopeToModule(banktypes.ModuleName)
sk2 := suite.keeper.ScopeToModule(stakingtypes.ModuleName)
sk3 := suite.keeper.ScopeToModule("foo")

cap, err := sk1.NewCapability(suite.ctx, "transfer")
suite.Require().NoError(err)
Expand All @@ -171,6 +182,9 @@ func (suite *KeeperTestSuite) TestClaimCapability() {
got, ok = sk2.GetCapability(suite.ctx, "transfer")
suite.Require().True(ok)
suite.Require().Equal(cap, got)

suite.Require().Error(sk3.ClaimCapability(suite.ctx, cap, " "))
suite.Require().Error(sk3.ClaimCapability(suite.ctx, nil, "transfer"))
}

func (suite *KeeperTestSuite) TestGetOwners() {
Expand Down Expand Up @@ -237,6 +251,8 @@ func (suite *KeeperTestSuite) TestGetOwners() {
}
}

_, ok := sk1.GetOwners(suite.ctx, " ")
suite.Require().False(ok, "got owners from empty capability name")
}

func (suite *KeeperTestSuite) TestReleaseCapability() {
Expand Down Expand Up @@ -264,6 +280,8 @@ func (suite *KeeperTestSuite) TestReleaseCapability() {
got, ok = sk1.GetCapability(suite.ctx, "transfer")
suite.Require().False(ok)
suite.Require().Nil(got)

suite.Require().Error(sk1.ReleaseCapability(suite.ctx, nil))
}

func (suite KeeperTestSuite) TestRevertCapability() {
Expand Down
12 changes: 7 additions & 5 deletions x/capability/types/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import (

// x/capability module sentinel errors
var (
ErrCapabilityTaken = sdkerrors.Register(ModuleName, 2, "capability name already taken")
ErrOwnerClaimed = sdkerrors.Register(ModuleName, 3, "given owner already claimed capability")
ErrCapabilityNotOwned = sdkerrors.Register(ModuleName, 4, "capability not owned by module")
ErrCapabilityNotFound = sdkerrors.Register(ModuleName, 5, "capability not found")
ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 6, "owners not found for capability")
ErrInvalidCapabilityName = sdkerrors.Register(ModuleName, 2, "capability name not valid")
ErrNilCapability = sdkerrors.Register(ModuleName, 3, "provided capability is nil")
ErrCapabilityTaken = sdkerrors.Register(ModuleName, 4, "capability name already taken")
ErrOwnerClaimed = sdkerrors.Register(ModuleName, 5, "given owner already claimed capability")
ErrCapabilityNotOwned = sdkerrors.Register(ModuleName, 6, "capability not owned by module")
ErrCapabilityNotFound = sdkerrors.Register(ModuleName, 7, "capability not found")
ErrCapabilityOwnersNotFound = sdkerrors.Register(ModuleName, 8, "owners not found for capability")
)
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func checkMisbehaviourHeader(
chainID, _ = clienttypes.SetVersionNumber(chainID, header.GetHeight().GetVersionNumber())
}

// - ValidatorSet must have 2/3 similarity with trusted FromValidatorSet
// - ValidatorSet must have TrustLevel similarity with trusted FromValidatorSet
// - ValidatorSets on both headers are valid given the last trusted ValidatorSet
if err := tmTrustedValset.VerifyCommitLightTrusting(
chainID, tmCommit, clientState.TrustLevel.ToTendermint(),
Expand Down