From dda9f98578b41603195b5fb764e3a4e40beab772 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 31 Aug 2022 18:29:40 +0200 Subject: [PATCH] chore: ics27 middleware callback routing (#2157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [WIP] add middleware enabled flags and conditional logic * adapting private registerInterchainAccount func to accept portID in favour of owner * updating tests * cleaning up tests * adding changelog * updating tests: adding cbs with unreachable error returns for safety * Update modules/apps/27-interchain-accounts/controller/keeper/keeper.go Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com> --- CHANGELOG.md | 1 + .../controller/ibc_middleware.go | 8 +-- .../controller/ibc_middleware_test.go | 50 ++++++++++++++++++- .../controller/keeper/account.go | 24 +++++---- .../controller/keeper/keeper.go | 18 +++++++ .../controller/keeper/msg_server.go | 8 ++- .../apps/27-interchain-accounts/types/keys.go | 8 +++ .../27-interchain-accounts/types/keys_test.go | 8 +++ 8 files changed, 110 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a761de92589..bea65a2bd0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (apps/27-interchain-accounts) [\#2102](https://github.com/cosmos/ibc-go/pull/2102) ICS27 controller middleware now supports a nil underlying application. This allows chains to make use of interchain accounts with existing auth mechanisms such as x/group and x/gov. * (apps/27-interchain-accounts) [\#2146](https://github.com/cosmos/ibc-go/pull/2146) ICS27 controller now claims the channel capability passed via ibc core, and passes `nil` to the underlying app callback. The channel capability arg in `SendTx` is now ignored and looked up internally. * (apps/27-interchain-accounts) [\#2134](https://github.com/cosmos/ibc-go/pull/2134) Adding upgrade handler to ICS27 `controller` submodule for migration of channel capabilities. This upgrade handler migrates ownership of channel capabilities from the underlying application to the ICS27 `controller` submodule. +* (apps/27-interchain-accounts) [\#2157](https://github.com/cosmos/ibc-go/pull/2157) Adding `IsMiddlewareEnabled` functionality to enforce calls to ICS27 msg server to *not* route to the underlying application. ### Features diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go index 2da54dd3eda..5be4baf5aee 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware.go @@ -63,7 +63,7 @@ func (im IBCMiddleware) OnChanOpenInit( // call underlying app's OnChanOpenInit callback with the passed in version // the version returned is discarded as the ica-auth module does not have permission to edit the version string. // ics27 will always return the version string containing the Metadata struct which is created during the `RegisterInterchainAccount` call. - if im.app != nil { + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) { if _, err := im.app.OnChanOpenInit(ctx, order, connectionHops, portID, channelID, nil, counterparty, version); err != nil { return "", err } @@ -108,7 +108,7 @@ func (im IBCMiddleware) OnChanOpenAck( } // call underlying app's OnChanOpenAck callback with the counterparty app version. - if im.app != nil { + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, portID, channelID) { return im.app.OnChanOpenAck(ctx, portID, channelID, counterpartyChannelID, counterpartyVersion) } @@ -167,7 +167,7 @@ func (im IBCMiddleware) OnAcknowledgementPacket( } // call underlying app's OnAcknowledgementPacket callback. - if im.app != nil { + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) { return im.app.OnAcknowledgementPacket(ctx, packet, acknowledgement, relayer) } @@ -188,7 +188,7 @@ func (im IBCMiddleware) OnTimeoutPacket( return err } - if im.app != nil { + if im.app != nil && im.keeper.IsMiddlewareEnabled(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) { return im.app.OnTimeoutPacket(ctx, packet, relayer) } diff --git a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go index 6a4680a4dfc..44fd201b0ef 100644 --- a/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go +++ b/modules/apps/27-interchain-accounts/controller/ibc_middleware_test.go @@ -124,6 +124,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { var ( channel *channeltypes.Channel isNilApp bool + path *ibctesting.Path ) testCases := []struct { @@ -185,6 +186,18 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { isNilApp = true }, true, }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenInit = func(ctx sdk.Context, order channeltypes.Order, connectionHops []string, + portID, channelID string, chanCap *capabilitytypes.Capability, + counterparty channeltypes.Counterparty, version string, + ) (string, error) { + return "", fmt.Errorf("error should be unreachable") + } + }, true, + }, } for _, tc := range testCases { @@ -194,7 +207,7 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { suite.SetupTest() // reset isNilApp = false - path := NewICAPath(suite.chainA, suite.chainB) + path = NewICAPath(suite.chainA, suite.chainB) suite.coordinator.SetupConnections(path) // mock init interchain account @@ -207,6 +220,8 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenInit() { path.EndpointA.ChannelConfig.PortID = portID path.EndpointA.ChannelID = ibctesting.FirstChannelID + suite.chainA.GetSimApp().ICAControllerKeeper.SetMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + // default values counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID) channel = &channeltypes.Channel{ @@ -336,6 +351,17 @@ func (suite *InterchainAccountsTestSuite) TestOnChanOpenAck() { isNilApp = true }, true, }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnChanOpenAck = func( + ctx sdk.Context, portID, channelID string, counterpartyChannelID string, counterpartyVersion string, + ) error { + return fmt.Errorf("error should be unreachable") + } + }, true, + }, } for _, tc := range testCases { @@ -572,6 +598,17 @@ func (suite *InterchainAccountsTestSuite) TestOnAcknowledgementPacket() { isNilApp = true }, true, }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnAcknowledgementPacket = func( + ctx sdk.Context, packet channeltypes.Packet, acknowledgement []byte, relayer sdk.AccAddress, + ) error { + return fmt.Errorf("error should be unreachable") + } + }, true, + }, } for _, tc := range testCases { @@ -654,6 +691,17 @@ func (suite *InterchainAccountsTestSuite) TestOnTimeoutPacket() { isNilApp = true }, true, }, + { + "middleware disabled", func() { + suite.chainA.GetSimApp().ICAControllerKeeper.DeleteMiddlewareEnabled(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID) + + suite.chainA.GetSimApp().ICAAuthModule.IBCApp.OnTimeoutPacket = func( + ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress, + ) error { + return fmt.Errorf("error should be unreachable") + } + }, true, + }, } for _, tc := range testCases { diff --git a/modules/apps/27-interchain-accounts/controller/keeper/account.go b/modules/apps/27-interchain-accounts/controller/keeper/account.go index d9143bf19ab..27bb4376c55 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/account.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/account.go @@ -17,22 +17,28 @@ import ( // - An error is returned if the port identifier is already in use. Gaining access to interchain accounts whose channels // have closed cannot be done with this function. A regular MsgChannelOpenInit must be used. func (k Keeper) RegisterInterchainAccount(ctx sdk.Context, connectionID, owner, version string) error { - _, err := k.registerInterchainAccount(ctx, connectionID, owner, version) - return err -} - -// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse -// and an error if one occurred. -func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, owner, version string) (string, error) { portID, err := icatypes.NewControllerPortID(owner) if err != nil { - return "", err + return err } + channelID, err := k.registerInterchainAccount(ctx, connectionID, portID, version) + if err != nil { + return err + } + + k.SetMiddlewareEnabled(ctx, portID, channelID) + + return nil +} + +// registerInterchainAccount registers an interchain account, returning the channel id of the MsgChannelOpenInitResponse +// and an error if one occurred. +func (k Keeper) registerInterchainAccount(ctx sdk.Context, connectionID, portID, version string) (string, error) { // if there is an active channel for this portID / connectionID return an error activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionID, portID) if found { - return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s for owner %s", activeChannelID, portID, connectionID, owner) + return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s on connection %s", activeChannelID, portID, connectionID) } switch { diff --git a/modules/apps/27-interchain-accounts/controller/keeper/keeper.go b/modules/apps/27-interchain-accounts/controller/keeper/keeper.go index 9ccd4abc6a4..fbba6c5140c 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/keeper.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/keeper.go @@ -206,3 +206,21 @@ func (k Keeper) SetInterchainAccountAddress(ctx sdk.Context, connectionID, portI store := ctx.KVStore(k.storeKey) store.Set(icatypes.KeyOwnerAccount(portID, connectionID), []byte(address)) } + +// IsMiddlewareEnabled returns true if the underlying application callbacks are enabled for given port and channel identifier pair, otherwise false +func (k Keeper) IsMiddlewareEnabled(ctx sdk.Context, portID, channelID string) bool { + store := ctx.KVStore(k.storeKey) + return store.Has(icatypes.KeyIsMiddlewareEnabled(portID, channelID)) +} + +// SetMiddlewareEnabled stores a flag to indicate that the underlying application callbacks should be enabled for the given port and channel identifier pair +func (k Keeper) SetMiddlewareEnabled(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Set(icatypes.KeyIsMiddlewareEnabled(portID, channelID), []byte{byte(1)}) +} + +// DeleteMiddlewareEnabled deletes the middleware enabled flag stored in state +func (k Keeper) DeleteMiddlewareEnabled(ctx sdk.Context, portID, channelID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(icatypes.KeyIsMiddlewareEnabled(portID, channelID)) +} diff --git a/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go b/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go index aaf50bcb930..72af21ce29e 100644 --- a/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go +++ b/modules/apps/27-interchain-accounts/controller/keeper/msg_server.go @@ -6,6 +6,7 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/controller/types" + icatypes "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" ) var _ types.MsgServer = Keeper{} @@ -14,7 +15,12 @@ var _ types.MsgServer = Keeper{} func (k Keeper) RegisterAccount(goCtx context.Context, msg *types.MsgRegisterAccount) (*types.MsgRegisterAccountResponse, error) { ctx := sdk.UnwrapSDKContext(goCtx) - channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, msg.Owner, msg.Version) + portID, err := icatypes.NewControllerPortID(msg.Owner) + if err != nil { + return nil, err + } + + channelID, err := k.registerInterchainAccount(ctx, msg.ConnectionId, portID, msg.Version) if err != nil { return nil, err } diff --git a/modules/apps/27-interchain-accounts/types/keys.go b/modules/apps/27-interchain-accounts/types/keys.go index 2bf05ffda3f..fa13eed1bae 100644 --- a/modules/apps/27-interchain-accounts/types/keys.go +++ b/modules/apps/27-interchain-accounts/types/keys.go @@ -36,6 +36,9 @@ var ( // PortKeyPrefix defines the key prefix used to store ports PortKeyPrefix = "port" + + // IsMiddlewareEnabledPrefix defines the key prefix used to store a flag for legacy API callback routing via ibc middleware + IsMiddlewareEnabledPrefix = "isMiddlewareEnabled" ) // KeyActiveChannel creates and returns a new key used for active channels store operations @@ -52,3 +55,8 @@ func KeyOwnerAccount(portID, connectionID string) []byte { func KeyPort(portID string) []byte { return []byte(fmt.Sprintf("%s/%s", PortKeyPrefix, portID)) } + +// KeyIsMiddlewareEnabled creates and returns a new key used for signaling legacy API callback routing via ibc middleware +func KeyIsMiddlewareEnabled(portID, channelID string) []byte { + return []byte(fmt.Sprintf("%s/%s/%s", IsMiddlewareEnabledPrefix, portID, channelID)) +} diff --git a/modules/apps/27-interchain-accounts/types/keys_test.go b/modules/apps/27-interchain-accounts/types/keys_test.go index 45800573a12..48255493d74 100644 --- a/modules/apps/27-interchain-accounts/types/keys_test.go +++ b/modules/apps/27-interchain-accounts/types/keys_test.go @@ -1,7 +1,10 @@ package types_test import ( + fmt "fmt" + "github.com/cosmos/ibc-go/v5/modules/apps/27-interchain-accounts/types" + ibctesting "github.com/cosmos/ibc-go/v5/testing" ) func (suite *TypesTestSuite) TestKeyActiveChannel() { @@ -13,3 +16,8 @@ func (suite *TypesTestSuite) TestKeyOwnerAccount() { key := types.KeyOwnerAccount("port-id", "connection-id") suite.Require().Equal("owner/port-id/connection-id", string(key)) } + +func (suite *TypesTestSuite) TestKeyIsMiddlewareEnabled() { + key := types.KeyIsMiddlewareEnabled(ibctesting.MockPort, ibctesting.FirstChannelID) + suite.Require().Equal(fmt.Sprintf("%s/%s/%s", types.IsMiddlewareEnabledPrefix, ibctesting.MockPort, ibctesting.FirstChannelID), string(key)) +}