From 1291bc4ada1d2dda7bdacab9f93e0cb3d9b5105c Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Tue, 20 Feb 2024 15:19:10 +0100 Subject: [PATCH 1/3] imp: deny selected client types from VerifyMembership rpc (#5871) * chore: add client status check to verify membership rpc * imp: deny selected client types from VerifyMembership rpc (cherry picked from commit 4f14cfd8ea11763793d5ab4dc36c030b507726b2) # Conflicts: # modules/core/02-client/keeper/grpc_query.go # modules/core/02-client/keeper/grpc_query_test.go --- modules/core/02-client/keeper/grpc_query.go | 72 +++++++ .../core/02-client/keeper/grpc_query_test.go | 177 ++++++++++++++++++ 2 files changed, 249 insertions(+) diff --git a/modules/core/02-client/keeper/grpc_query.go b/modules/core/02-client/keeper/grpc_query.go index 12daed284c7..04a24eea1d1 100644 --- a/modules/core/02-client/keeper/grpc_query.go +++ b/modules/core/02-client/keeper/grpc_query.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "fmt" + "slices" "sort" "strings" @@ -328,3 +329,74 @@ func (k Keeper) UpgradedConsensusState(c context.Context, req *types.QueryUpgrad UpgradedConsensusState: protoAny, }, nil } +<<<<<<< HEAD +======= + +// VerifyMembership implements the Query/VerifyMembership gRPC method +// NOTE: Any state changes made within this handler are discarded by leveraging a cached context. Gas is consumed for underlying state access. +// This gRPC method is intended to be used within the context of the state machine and delegates to light clients to verify proofs. +func (k Keeper) VerifyMembership(c context.Context, req *types.QueryVerifyMembershipRequest) (*types.QueryVerifyMembershipResponse, error) { + if req == nil { + return nil, status.Error(codes.InvalidArgument, "empty request") + } + + if err := host.ClientIdentifierValidator(req.ClientId); err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + clientType, _, err := types.ParseClientIdentifier(req.ClientId) + if err != nil { + return nil, status.Error(codes.InvalidArgument, err.Error()) + } + + denyClients := []string{exported.Localhost, exported.Solomachine} + if slices.Contains(denyClients, clientType) { + return nil, status.Error(codes.InvalidArgument, errorsmod.Wrapf(types.ErrInvalidClientType, "verify membership is disabled for client types %s", denyClients).Error()) + } + + if len(req.Proof) == 0 { + return nil, status.Error(codes.InvalidArgument, "empty proof") + } + + if req.ProofHeight.IsZero() { + return nil, status.Error(codes.InvalidArgument, "proof height must be non-zero") + } + + if req.MerklePath.Empty() { + return nil, status.Error(codes.InvalidArgument, "empty merkle path") + } + + if len(req.Value) == 0 { + return nil, status.Error(codes.InvalidArgument, "empty value") + } + + ctx := sdk.UnwrapSDKContext(c) + // cache the context to ensure clientState.VerifyMembership does not change state + cachedCtx, _ := ctx.CacheContext() + + // make sure we charge the higher level context even on panic + defer func() { + ctx.GasMeter().ConsumeGas(cachedCtx.GasMeter().GasConsumed(), "verify membership query") + }() + + clientState, found := k.GetClientState(cachedCtx, req.ClientId) + if !found { + return nil, status.Error(codes.NotFound, errorsmod.Wrap(types.ErrClientNotFound, req.ClientId).Error()) + } + + if clientStatus := k.GetClientStatus(ctx, clientState, req.ClientId); clientStatus != exported.Active { + return nil, status.Error(codes.FailedPrecondition, errorsmod.Wrapf(types.ErrClientNotActive, "cannot verify membership using client (%s) with status %s", req.ClientId, clientStatus).Error()) + } + + if err := clientState.VerifyMembership(cachedCtx, k.ClientStore(cachedCtx, req.ClientId), k.cdc, req.ProofHeight, req.TimeDelay, req.BlockDelay, req.Proof, req.MerklePath, req.Value); err != nil { + k.Logger(ctx).Debug("proof verification failed", "key", req.MerklePath, "error", err) + return &types.QueryVerifyMembershipResponse{ + Success: false, + }, nil + } + + return &types.QueryVerifyMembershipResponse{ + Success: true, + }, nil +} +>>>>>>> 4f14cfd8 (imp: deny selected client types from VerifyMembership rpc (#5871)) diff --git a/modules/core/02-client/keeper/grpc_query_test.go b/modules/core/02-client/keeper/grpc_query_test.go index 231d000a257..2b71ae9ca3a 100644 --- a/modules/core/02-client/keeper/grpc_query_test.go +++ b/modules/core/02-client/keeper/grpc_query_test.go @@ -659,3 +659,180 @@ func (suite *KeeperTestSuite) TestQueryClientParams() { res, _ := suite.chainA.QueryServer.ClientParams(ctx, &types.QueryClientParamsRequest{}) suite.Require().Equal(&expParams, res.Params) } +<<<<<<< HEAD +======= + +func (suite *KeeperTestSuite) TestQueryVerifyMembershipProof() { + var ( + path *ibctesting.Path + req *types.QueryVerifyMembershipRequest + ) + + testCases := []struct { + name string + malleate func() + expError error + }{ + { + "success", + func() { + channel := path.EndpointB.GetChannel() + bz, err := suite.chainB.Codec.Marshal(&channel) + suite.Require().NoError(err) + + channelProof, proofHeight := path.EndpointB.QueryProof(host.ChannelKey(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) + + merklePath := commitmenttypes.NewMerklePath(host.ChannelPath(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)) + merklePath, err = commitmenttypes.ApplyPrefix(suite.chainB.GetPrefix(), merklePath) + suite.Require().NoError(err) + + req = &types.QueryVerifyMembershipRequest{ + ClientId: path.EndpointA.ClientID, + Proof: channelProof, + ProofHeight: proofHeight, + MerklePath: merklePath, + Value: bz, + } + }, + nil, + }, + { + "req is nil", + func() { + req = nil + }, + errors.New("empty request"), + }, + { + "invalid client ID", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: "//invalid_id", + } + }, + host.ErrInvalidID, + }, + { + "localhost client ID is denied", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: exported.LocalhostClientID, + } + }, + types.ErrInvalidClientType, + }, + { + "solomachine client ID is denied", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: types.FormatClientIdentifier(exported.Solomachine, 1), + } + }, + types.ErrInvalidClientType, + }, + { + "empty proof", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: ibctesting.FirstClientID, + Proof: []byte{}, + } + }, + errors.New("empty proof"), + }, + { + "invalid proof height", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: ibctesting.FirstClientID, + Proof: []byte{0x01}, + ProofHeight: types.ZeroHeight(), + } + }, + errors.New("proof height must be non-zero"), + }, + { + "empty merkle path", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: ibctesting.FirstClientID, + Proof: []byte{0x01}, + ProofHeight: types.NewHeight(1, 100), + } + }, + errors.New("empty merkle path"), + }, + { + "empty value", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: ibctesting.FirstClientID, + Proof: []byte{0x01}, + ProofHeight: types.NewHeight(1, 100), + MerklePath: commitmenttypes.NewMerklePath("/ibc", host.ChannelPath(mock.PortID, ibctesting.FirstChannelID)), + } + }, + errors.New("empty value"), + }, + { + "client not found", + func() { + req = &types.QueryVerifyMembershipRequest{ + ClientId: types.FormatClientIdentifier(exported.Tendermint, 100), // use a sequence which hasn't been created yet + Proof: []byte{0x01}, + ProofHeight: types.NewHeight(1, 100), + MerklePath: commitmenttypes.NewMerklePath("/ibc", host.ChannelPath(mock.PortID, ibctesting.FirstChannelID)), + Value: []byte{0x01}, + } + }, + types.ErrClientNotFound, + }, + { + "client not active", + func() { + params := types.NewParams("") // disable all clients + suite.chainA.GetSimApp().GetIBCKeeper().ClientKeeper.SetParams(suite.chainA.GetContext(), params) + + req = &types.QueryVerifyMembershipRequest{ + ClientId: path.EndpointA.ClientID, + Proof: []byte{0x01}, + ProofHeight: types.NewHeight(1, 100), + MerklePath: commitmenttypes.NewMerklePath("/ibc", host.ChannelPath(mock.PortID, ibctesting.FirstChannelID)), + Value: []byte{0x01}, + } + }, + types.ErrClientNotActive, + }, + } + + for _, tc := range testCases { + tc := tc + suite.Run(tc.name, func() { + suite.SetupTest() // reset + + path = ibctesting.NewPath(suite.chainA, suite.chainB) + path.Setup() + + tc.malleate() + + ctx := suite.chainA.GetContext() + initialGas := ctx.GasMeter().GasConsumed() + res, err := suite.chainA.QueryServer.VerifyMembership(ctx, req) + + expPass := tc.expError == nil + if expPass { + suite.Require().NoError(err) + suite.Require().True(res.Success, "failed to verify membership proof") + + gasConsumed := ctx.GasMeter().GasConsumed() + suite.Require().Greater(gasConsumed, initialGas, "gas consumed should be greater than initial gas") + } else { + suite.Require().ErrorContains(err, tc.expError.Error()) + + gasConsumed := ctx.GasMeter().GasConsumed() + suite.Require().GreaterOrEqual(gasConsumed, initialGas, "gas consumed should be greater than or equal to initial gas") + } + }) + } +} +>>>>>>> 4f14cfd8 (imp: deny selected client types from VerifyMembership rpc (#5871)) From 03f38be8d6648b50968f9b4163cf4af2753891a7 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 21 Feb 2024 21:46:54 +0100 Subject: [PATCH 2/3] fix conflicts --- modules/core/02-client/keeper/grpc_query.go | 3 --- modules/core/02-client/keeper/grpc_query_test.go | 6 +++--- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/modules/core/02-client/keeper/grpc_query.go b/modules/core/02-client/keeper/grpc_query.go index 04a24eea1d1..9ba5241fe57 100644 --- a/modules/core/02-client/keeper/grpc_query.go +++ b/modules/core/02-client/keeper/grpc_query.go @@ -329,8 +329,6 @@ func (k Keeper) UpgradedConsensusState(c context.Context, req *types.QueryUpgrad UpgradedConsensusState: protoAny, }, nil } -<<<<<<< HEAD -======= // VerifyMembership implements the Query/VerifyMembership gRPC method // NOTE: Any state changes made within this handler are discarded by leveraging a cached context. Gas is consumed for underlying state access. @@ -399,4 +397,3 @@ func (k Keeper) VerifyMembership(c context.Context, req *types.QueryVerifyMember Success: true, }, nil } ->>>>>>> 4f14cfd8 (imp: deny selected client types from VerifyMembership rpc (#5871)) diff --git a/modules/core/02-client/keeper/grpc_query_test.go b/modules/core/02-client/keeper/grpc_query_test.go index 2b71ae9ca3a..22bc1cbe861 100644 --- a/modules/core/02-client/keeper/grpc_query_test.go +++ b/modules/core/02-client/keeper/grpc_query_test.go @@ -1,15 +1,18 @@ package keeper_test import ( + "errors" "fmt" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/types/query" "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" "github.com/cosmos/ibc-go/v8/modules/core/exported" ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" ibctesting "github.com/cosmos/ibc-go/v8/testing" + "github.com/cosmos/ibc-go/v8/testing/mock" ) func (suite *KeeperTestSuite) TestQueryClientState() { @@ -659,8 +662,6 @@ func (suite *KeeperTestSuite) TestQueryClientParams() { res, _ := suite.chainA.QueryServer.ClientParams(ctx, &types.QueryClientParamsRequest{}) suite.Require().Equal(&expParams, res.Params) } -<<<<<<< HEAD -======= func (suite *KeeperTestSuite) TestQueryVerifyMembershipProof() { var ( @@ -835,4 +836,3 @@ func (suite *KeeperTestSuite) TestQueryVerifyMembershipProof() { }) } } ->>>>>>> 4f14cfd8 (imp: deny selected client types from VerifyMembership rpc (#5871)) From eb592fb9b2e0d7e13dcb65e2357dc2b84c28c5c0 Mon Sep 17 00:00:00 2001 From: Carlos Rodriguez Date: Wed, 21 Feb 2024 22:44:50 +0100 Subject: [PATCH 3/3] fix test --- modules/core/02-client/keeper/grpc_query_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/02-client/keeper/grpc_query_test.go b/modules/core/02-client/keeper/grpc_query_test.go index d69e0b525e1..9df89c4643b 100644 --- a/modules/core/02-client/keeper/grpc_query_test.go +++ b/modules/core/02-client/keeper/grpc_query_test.go @@ -797,7 +797,7 @@ func (suite *KeeperTestSuite) TestQueryVerifyMembershipProof() { suite.SetupTest() // reset path = ibctesting.NewPath(suite.chainA, suite.chainB) - path.Setup() + suite.coordinator.Setup(path) tc.malleate()