From 2f49b0e792cc09ae6aa4362021ee8bded92e7686 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 7 Feb 2023 11:14:10 +0100 Subject: [PATCH 1/3] disallow localhost client creation, adapt tests --- go.mod | 2 +- modules/core/02-client/keeper/client_test.go | 31 +++++++++++++++----- modules/core/02-client/types/params.go | 6 +++- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/go.mod b/go.mod index 24c60c0705c..692c949614c 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,6 @@ require ( github.com/stretchr/testify v1.8.1 github.com/tendermint/tendermint v0.37.0-rc2 github.com/tendermint/tm-db v0.6.7 - golang.org/x/exp v0.0.0-20221019170559-20944726eadf google.golang.org/genproto v0.0.0-20221227171554-f9683d7f8bef google.golang.org/grpc v1.52.3 google.golang.org/protobuf v1.28.1 @@ -147,6 +146,7 @@ require ( go.etcd.io/bbolt v1.3.6 // indirect go.opencensus.io v0.24.0 // indirect golang.org/x/crypto v0.4.0 // indirect + golang.org/x/exp v0.0.0-20221019170559-20944726eadf // indirect golang.org/x/net v0.4.0 // indirect golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783 // indirect golang.org/x/sys v0.3.0 // indirect diff --git a/modules/core/02-client/keeper/client_test.go b/modules/core/02-client/keeper/client_test.go index 94b5fed855a..ce338d25853 100644 --- a/modules/core/02-client/keeper/client_test.go +++ b/modules/core/02-client/keeper/client_test.go @@ -13,22 +13,39 @@ import ( "github.com/cosmos/ibc-go/v7/modules/core/exported" solomachine "github.com/cosmos/ibc-go/v7/modules/light-clients/06-solomachine" ibctm "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint" + localhost "github.com/cosmos/ibc-go/v7/modules/light-clients/09-localhost" ibctesting "github.com/cosmos/ibc-go/v7/testing" ) func (suite *KeeperTestSuite) TestCreateClient() { cases := []struct { - msg string - clientState exported.ClientState - expPass bool + msg string + clientState exported.ClientState + consensusState exported.ConsensusState + expPass bool }{ - {"success", ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), true}, - {"client type not supported", solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), false}, + { + "success: 07-tendermint client type supported", + ibctm.NewClientState(testChainID, ibctm.DefaultTrustLevel, trustingPeriod, ubdPeriod, maxClockDrift, testClientHeight, commitmenttypes.GetSDKSpecs(), ibctesting.UpgradePath), + suite.consensusState, + true, + }, + { + "success: 06-solomachine client type supported", + solomachine.NewClientState(0, &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}), + &solomachine.ConsensusState{PublicKey: suite.solomachine.ConsensusState().PublicKey, Diversifier: suite.solomachine.Diversifier, Timestamp: suite.solomachine.Time}, + true, + }, + { + "failure: 09-localhost client type not supported", + localhost.NewClientState(clienttypes.GetSelfHeight(suite.ctx)), + nil, + false, + }, } for i, tc := range cases { - - clientID, err := suite.keeper.CreateClient(suite.ctx, tc.clientState, suite.consensusState) + clientID, err := suite.keeper.CreateClient(suite.ctx, tc.clientState, tc.consensusState) if tc.expPass { suite.Require().NoError(err, "valid test case %d failed: %s", i, tc.msg) suite.Require().NotNil(clientID, "valid test case %d failed: %s", i, tc.msg) diff --git a/modules/core/02-client/types/params.go b/modules/core/02-client/types/params.go index ab6cfd7c43d..20d391c134d 100644 --- a/modules/core/02-client/types/params.go +++ b/modules/core/02-client/types/params.go @@ -11,7 +11,7 @@ import ( var ( // DefaultAllowedClients are "06-solomachine" and "07-tendermint" - DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint, exported.Localhost} + DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint} // KeyAllowedClients is store's key for AllowedClients Params KeyAllowedClients = []byte("AllowedClients") @@ -48,6 +48,10 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs { // IsAllowedClient checks if the given client type is registered on the allowlist. func (p Params) IsAllowedClient(clientType string) bool { + if clientType == exported.Localhost { + return false + } + for _, allowedClient := range p.AllowedClients { if allowedClient == clientType { return true From 5b2080d5c05d632d73d2c149f391e5a54799468f Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 7 Feb 2023 11:39:29 +0100 Subject: [PATCH 2/3] adapting logic to accomodate usage of allowed clients in InitGenesis --- modules/core/02-client/keeper/client.go | 2 +- modules/core/02-client/types/params.go | 6 +----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index fd260d65d98..6b9c188b74e 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -17,7 +17,7 @@ func (k Keeper) CreateClient( ctx sdk.Context, clientState exported.ClientState, consensusState exported.ConsensusState, ) (string, error) { params := k.GetParams(ctx) - if !params.IsAllowedClient(clientState.ClientType()) { + if !params.IsAllowedClient(clientState.ClientType()) || clientState.ClientType() == exported.Localhost { return "", sdkerrors.Wrapf( types.ErrInvalidClientType, "client state type %s is not registered in the allowlist", clientState.ClientType(), diff --git a/modules/core/02-client/types/params.go b/modules/core/02-client/types/params.go index 20d391c134d..ab6cfd7c43d 100644 --- a/modules/core/02-client/types/params.go +++ b/modules/core/02-client/types/params.go @@ -11,7 +11,7 @@ import ( var ( // DefaultAllowedClients are "06-solomachine" and "07-tendermint" - DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint} + DefaultAllowedClients = []string{exported.Solomachine, exported.Tendermint, exported.Localhost} // KeyAllowedClients is store's key for AllowedClients Params KeyAllowedClients = []byte("AllowedClients") @@ -48,10 +48,6 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs { // IsAllowedClient checks if the given client type is registered on the allowlist. func (p Params) IsAllowedClient(clientType string) bool { - if clientType == exported.Localhost { - return false - } - for _, allowedClient := range p.AllowedClients { if allowedClient == clientType { return true From ab0b8eb75b61a89e2fe923220bd3e52e67c4fd9d Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 7 Feb 2023 15:29:38 +0100 Subject: [PATCH 3/3] move localhost check to separate conditional --- modules/core/02-client/keeper/client.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go index 6b9c188b74e..2b5e65a1cf7 100644 --- a/modules/core/02-client/keeper/client.go +++ b/modules/core/02-client/keeper/client.go @@ -16,8 +16,12 @@ import ( func (k Keeper) CreateClient( ctx sdk.Context, clientState exported.ClientState, consensusState exported.ConsensusState, ) (string, error) { + if clientState.ClientType() == exported.Localhost { + return "", sdkerrors.Wrapf(types.ErrInvalidClientType, "cannot create client of type: %s", clientState.ClientType()) + } + params := k.GetParams(ctx) - if !params.IsAllowedClient(clientState.ClientType()) || clientState.ClientType() == exported.Localhost { + if !params.IsAllowedClient(clientState.ClientType()) { return "", sdkerrors.Wrapf( types.ErrInvalidClientType, "client state type %s is not registered in the allowlist", clientState.ClientType(),