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.

Sign up for GitHub

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

Jump to bottom

chore: add defensive check to ensure metadata does not change when reopening an active channel #847

Merged
merged 14 commits into from
Feb 8, 2022
Merged

chore: add defensive check to ensure metadata does not change when reopening an active channel #847

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a0386ec
add defensive check to ensure metadata does not change when reopening…
colin-axner Feb 3, 2022
e67068d
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 3, 2022
5521ee3
add previous metadata check to onchanopentry
colin-axner Feb 3, 2022
1d5bf68
readjust handshake checks to use helper function, remove tests becaus…
colin-axner Feb 3, 2022
21adb6c
add tests for IsPreviousMetadataEqual
colin-axner Feb 3, 2022
2f2f4dc
Merge branch 'colin/795-reopen-active-check' of github.com:cosmos/ibc…
colin-axner Feb 3, 2022
3fdc6de
fix bug
colin-axner Feb 3, 2022
3559f2d
use correct version in OpenInit handshake
colin-axner Feb 7, 2022
146c7fa
Apply Aditya's error improvement comments
colin-axner Feb 7, 2022
bebbfa7
Update modules/apps/27-interchain-accounts/types/metadata.go
colin-axner Feb 7, 2022
e7b48ca
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 7, 2022
0f422e2
test: add tests for previous metadata being different
colin-axner Feb 8, 2022
4c2c0f3
Merge branch 'colin/795-reopen-active-check' of github.com:cosmos/ibc…
colin-axner Feb 8, 2022
42557b3
Merge branch 'main' into colin/795-reopen-active-check
colin-axner Feb 8, 2022
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
16 changes: 14 additions & 2 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"fmt"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -49,9 +50,20 @@ func (k Keeper) OnChanOpenInit(
return err
}

activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], portID)
activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], portID)
if found {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
if !found {
panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
}

if channel.State == channeltypes.OPEN {
return sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
}

if !icatypes.IsPreviousMetadataEqual(version, metadata) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something, but the version that is passed here, isn't it the same that is unmarshalled to metadata above? So here:

var metadata icatypes.Metadata
if err := icatypes.ModuleCdc.UnmarshalJSON([]byte(version), &metadata); err != nil {
    return sdkerrors.Wrapf(icatypes.ErrUnknownDataType, "cannot unmarshal ICS-27 interchain accounts metadata")
}

If that's the case, wouldn't this check here just redundant?

Oh, wait... Or shouldn't you be using here channel.Version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice catch, it should be channel.Version 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

return sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
}
}

return nil
Expand Down
18 changes: 18 additions & 0 deletions modules/apps/27-interchain-accounts/controller/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,24 @@ func (suite *KeeperTestSuite) TestOnChanOpenInit() {
},
true,
},
{
"success - previous active channel closed",
Copy link
Member

Choose a reason for hiding this comment

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

Is there an error testcase for when previous metadata is different?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unreachable, but I might as well add one

func() {
suite.chainA.GetSimApp().ICAControllerKeeper.SetActiveChannelID(suite.chainA.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)

counterparty := channeltypes.NewCounterparty(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
channel := channeltypes.Channel{
State: channeltypes.CLOSED,
Ordering: channeltypes.ORDERED,
Counterparty: counterparty,
ConnectionHops: []string{path.EndpointA.ConnectionID},
Version: TestVersion,
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
}

path.EndpointA.SetChannel(channel)
},
true,
},
{
"invalid order - UNORDERED",
func() {
Expand Down
21 changes: 17 additions & 4 deletions modules/apps/27-interchain-accounts/host/keeper/handshake.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package keeper

import (
"fmt"
"strings"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -47,8 +48,20 @@ func (k Keeper) OnChanOpenTry(
return "", err
}

if activeChannelID, found := k.GetOpenActiveChannel(ctx, connectionHops[0], counterparty.PortId); found {
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
activeChannelID, found := k.GetActiveChannelID(ctx, connectionHops[0], counterparty.PortId)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this whole block of code is the same as the one in OnChannelOpenInit, could we extract it into a helper function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is fine to leave as is/be explicit. There's still a lot of duplicate code between OpenInit and OpenTry

if found {
channel, found := k.channelKeeper.GetChannel(ctx, portID, activeChannelID)
if !found {
panic(fmt.Sprintf("active channel mapping set for %s but channel does not exist in channel store", activeChannelID))
}

if channel.State == channeltypes.OPEN {
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s", activeChannelID, portID)
return "", sdkerrors.Wrapf(icatypes.ErrActiveChannelAlreadySet, "existing active channel %s for portID %s is already OPEN", activeChannelID, portID)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated on controller and host

}

if !icatypes.IsPreviousMetadataEqual(channel.Version, metadata) {
return "", sdkerrors.Wrap(icatypes.ErrInvalidVersion, "previous active channel metadata does not match provided version")
}
}

// On the host chain the capability may only be claimed during the OnChanOpenTry
Expand Down Expand Up @@ -83,9 +96,9 @@ func (k Keeper) OnChanOpenConfirm(
}

// It is assumed the controller chain will not allow multiple active channels to be created for the same connectionID/portID
// If the controller chain does allow multiple active channels to be created for the same connectionID/portID,
// If the controller chain does allow multiple active channels to be created for the same connectionID/portID,
// disallowing overwriting the current active channel guarantees the channel can no longer be used as the controller
// and host will disagree on what the currently active channel is
// and host will disagree on what the currently active channel is
k.SetActiveChannelID(ctx, channel.ConnectionHops[0], channel.Counterparty.PortId, channelID)

return nil
Expand Down
13 changes: 12 additions & 1 deletion modules/apps/27-interchain-accounts/host/keeper/handshake_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,17 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
},
true,
},
{
"success - reopening closed active channel",
Copy link
Member

Choose a reason for hiding this comment

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

test case for error when the previous channel has different metadata?

func() {
// create a new channel and set it in state
ch := channeltypes.NewChannel(channeltypes.CLOSED, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, TestVersion)
suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)

// set the active channelID in state
suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, true,
},
{
"invalid order - UNORDERED",
func() {
Expand Down Expand Up @@ -140,7 +151,7 @@ func (suite *KeeperTestSuite) TestOnChanOpenTry() {
func() {
// create a new channel and set it in state
ch := channeltypes.NewChannel(channeltypes.OPEN, channeltypes.ORDERED, channeltypes.NewCounterparty(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID), []string{path.EndpointA.ConnectionID}, ibctesting.DefaultChannelVersion)
suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)
suite.chainB.GetSimApp().GetIBCKeeper().ChannelKeeper.SetChannel(suite.chainB.GetContext(), path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, ch)

// set the active channelID in state
suite.chainB.GetSimApp().ICAHostKeeper.SetActiveChannelID(suite.chainB.GetContext(), ibctesting.FirstConnectionID, path.EndpointA.ChannelConfig.PortID, path.EndpointB.ChannelID)
Expand Down
15 changes: 15 additions & 0 deletions modules/apps/27-interchain-accounts/types/metadata.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,21 @@ func NewMetadata(version, controllerConnectionID, hostConnectionID, accAddress,
}
}

// IsMetadataEqual compares a metadata to a pevious version string set in a channel struct.
// It ensure all fields are equal except the Address string
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
func IsPreviousMetadataEqual(previousVersion string, metadata Metadata) bool {
Copy link
Contributor

@crodriguezvega crodriguezvega Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggestion...

We already have code to unmarhsall the version string here and the same code is found in OnChannelOpenTry and OnOpenChannelAck. And now we also have it inside this function. So we could make an extra constructor function and remove the duplication of code in all these places. So basically to create a function with a signature like this: func NewMetadata(version string) (Metadata, error).

And if you do that then you could also change this function so that it has the Metadata receiver type and change also the name and signature, so something like this: func (Metadata m) IsEqual(metadata Metadata) bool. I leave it up to you if you prefer a pointer receiver type.

The you can invoke these functions like:

previousMetadata, err := icatypes.NewMetadata(previousVersion)
if !err {
    return "", err
}
if !metadata.IsEqual(previousMetadata) {
    ...   
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your suggestion. NewMetadata would need to be named something else to avoid name collision with the traditional usage of NewMetadata. IsEqual is also a good idea, but we will remove all this logic with the removal of active channels, so I'm leaning towards leaving as is. It makes the equality check explicitly to be used for active channel security and nothing else (otherwise external users may start depending on that API which should be removed in the future)

Copy link
Member

Choose a reason for hiding this comment

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

I prefer @crodriguezvega's suggestion too, but given this will be removed in a couple of months hopefully, then I think we should just leave it as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree with @damiannolan

var previousMetadata Metadata
if err := ModuleCdc.UnmarshalJSON([]byte(previousVersion), &previousMetadata); err != nil {
return false
}

return (previousMetadata.Version == metadata.Version &&
previousMetadata.ControllerConnectionId == metadata.ControllerConnectionId &&
previousMetadata.HostConnectionId == metadata.HostConnectionId &&
previousMetadata.Encoding == metadata.Encoding &&
previousMetadata.TxType == metadata.TxType)
}

// ValidateControllerMetadata performs validation of the provided ICS27 controller metadata parameters
func ValidateControllerMetadata(ctx sdk.Context, channelKeeper ChannelKeeper, connectionHops []string, metadata Metadata) error {
if !isSupportedEncoding(metadata.Encoding) {
Expand Down
121 changes: 121 additions & 0 deletions modules/apps/27-interchain-accounts/types/metadata_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,127 @@ import (
ibctesting "github.com/cosmos/ibc-go/v3/testing"
)

// use TestVersion as metadata being compared against
func (suite *TypesTestSuite) TestIsPreviousMetadataEqual() {

var (
metadata types.Metadata
previousVersion string
)

testCases := []struct {
name string
malleate func()
expEqual bool
}{
{
"success",
func() {
versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
true,
},
{
"success with empty account address",
func() {
metadata.Address = ""

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
true,
},
{
"cannot decode previous version",
func() {
previousVersion = "invalid previous version"
},
false,
},
{
"unequal encoding format",
func() {
metadata.Encoding = "invalid-encoding-format"

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
false,
},
{
"unequal transaction type",
func() {
metadata.TxType = "invalid-tx-type"

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
false,
},
{
"unequal controller connection",
func() {
metadata.ControllerConnectionId = "connection-10"

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
false,
},
{
"unequal host connection",
func() {
metadata.HostConnectionId = "connection-10"

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
false,
},
{
"unequal version",
func() {
metadata.Version = "invalid version"

versionBytes, err := types.ModuleCdc.MarshalJSON(&metadata)
suite.Require().NoError(err)
previousVersion = string(versionBytes)
},
false,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest() // reset

path := ibctesting.NewPath(suite.chainA, suite.chainB)
suite.coordinator.SetupConnections(path)

expectedMetadata := types.NewMetadata(types.Version, ibctesting.FirstConnectionID, ibctesting.FirstConnectionID, TestOwnerAddress, types.EncodingProtobuf, types.TxTypeSDKMultiMsg)
metadata = expectedMetadata // default success case

tc.malleate() // malleate mutates test data

equal := types.IsPreviousMetadataEqual(previousVersion, expectedMetadata)

if tc.expEqual {
suite.Require().True(equal)
} else {
suite.Require().False(equal)
}
})
}
}

func (suite *TypesTestSuite) TestValidateControllerMetadata() {

var metadata types.Metadata
Expand Down