From 116d0460fc329b6d96b615782c8d3f57e30aa505 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 11 Nov 2020 11:09:57 +0100 Subject: [PATCH] solo machine GetPubKey returns an error instead of panicing (#7883) * solo machine public key now returns an error instead of panicing * apply @fedekunze review suggestions Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- .../06-solomachine/types/client_state.go | 70 ++++++++++--------- .../06-solomachine/types/consensus_state.go | 20 ++++-- .../06-solomachine/types/header.go | 19 +++-- .../types/misbehaviour_handle.go | 7 +- .../types/misbehaviour_handle_test.go | 10 +++ .../06-solomachine/types/proposal_handle.go | 12 +++- .../types/proposal_handle_test.go | 9 ++- .../06-solomachine/types/update.go | 7 +- .../06-solomachine/types/update_test.go | 10 +++ 9 files changed, 116 insertions(+), 48 deletions(-) diff --git a/x/ibc/light-clients/06-solomachine/types/client_state.go b/x/ibc/light-clients/06-solomachine/types/client_state.go index 5e9fe014ac..05ed359baf 100644 --- a/x/ibc/light-clients/06-solomachine/types/client_state.go +++ b/x/ibc/light-clients/06-solomachine/types/client_state.go @@ -4,6 +4,7 @@ import ( ics23 "github.com/confio/ics23/go" "github.com/cosmos/cosmos-sdk/codec" + cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/types/tx/signing" @@ -95,7 +96,7 @@ func (cs ClientState) VerifyClientState( proof []byte, clientState exported.ClientState, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -111,7 +112,7 @@ func (cs ClientState) VerifyClientState( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -133,7 +134,7 @@ func (cs ClientState) VerifyClientConsensusState( proof []byte, consensusState exported.ConsensusState, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -149,7 +150,7 @@ func (cs ClientState) VerifyClientConsensusState( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -170,7 +171,7 @@ func (cs ClientState) VerifyConnectionState( connectionID string, connectionEnd exported.ConnectionI, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -186,7 +187,7 @@ func (cs ClientState) VerifyConnectionState( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -208,7 +209,7 @@ func (cs ClientState) VerifyChannelState( channelID string, channel exported.ChannelI, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -224,7 +225,7 @@ func (cs ClientState) VerifyChannelState( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -247,7 +248,7 @@ func (cs ClientState) VerifyPacketCommitment( packetSequence uint64, commitmentBytes []byte, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -263,7 +264,7 @@ func (cs ClientState) VerifyPacketCommitment( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -286,7 +287,7 @@ func (cs ClientState) VerifyPacketAcknowledgement( packetSequence uint64, acknowledgement []byte, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -302,7 +303,7 @@ func (cs ClientState) VerifyPacketAcknowledgement( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -325,7 +326,7 @@ func (cs ClientState) VerifyPacketReceiptAbsence( channelID string, packetSequence uint64, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -341,7 +342,7 @@ func (cs ClientState) VerifyPacketReceiptAbsence( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -363,7 +364,7 @@ func (cs ClientState) VerifyNextSequenceRecv( channelID string, nextSequenceRecv uint64, ) error { - sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) + publicKey, sigData, timestamp, sequence, err := produceVerificationArgs(cdc, cs, height, prefix, proof) if err != nil { return err } @@ -379,7 +380,7 @@ func (cs ClientState) VerifyNextSequenceRecv( return err } - if err := VerifySignature(cs.ConsensusState.GetPubKey(), signBz, sigData); err != nil { + if err := VerifySignature(publicKey, signBz, sigData); err != nil { return err } @@ -390,71 +391,76 @@ func (cs ClientState) VerifyNextSequenceRecv( } // produceVerificationArgs perfoms the basic checks on the arguments that are -// shared between the verification functions and returns the unmarshalled -// proof representing the signature and timestamp along with the solo-machine sequence -// encoded in the proofHeight. +// shared between the verification functions and returns the public key of the +// consensus state, the unmarshalled proof representing the signature and timestamp +// along with the solo-machine sequence encoded in the proofHeight. func produceVerificationArgs( cdc codec.BinaryMarshaler, cs ClientState, height exported.Height, prefix exported.Prefix, proof []byte, -) (signing.SignatureData, uint64, uint64, error) { +) (cryptotypes.PubKey, signing.SignatureData, uint64, uint64, error) { if version := height.GetVersionNumber(); version != 0 { - return nil, 0, 0, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version must be 0 for solomachine, got version-number: %d", version) + return nil, nil, 0, 0, sdkerrors.Wrapf(sdkerrors.ErrInvalidHeight, "version must be 0 for solomachine, got version-number: %d", version) } // sequence is encoded in the version height of height struct sequence := height.GetVersionHeight() if cs.IsFrozen() { - return nil, 0, 0, clienttypes.ErrClientFrozen + return nil, nil, 0, 0, clienttypes.ErrClientFrozen } if prefix == nil { - return nil, 0, 0, sdkerrors.Wrap(commitmenttypes.ErrInvalidPrefix, "prefix cannot be empty") + return nil, nil, 0, 0, sdkerrors.Wrap(commitmenttypes.ErrInvalidPrefix, "prefix cannot be empty") } _, ok := prefix.(commitmenttypes.MerklePrefix) if !ok { - return nil, 0, 0, sdkerrors.Wrapf(commitmenttypes.ErrInvalidPrefix, "invalid prefix type %T, expected MerklePrefix", prefix) + return nil, nil, 0, 0, sdkerrors.Wrapf(commitmenttypes.ErrInvalidPrefix, "invalid prefix type %T, expected MerklePrefix", prefix) } if proof == nil { - return nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "proof cannot be empty") + return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "proof cannot be empty") } timestampedSigData := &TimestampedSignatureData{} if err := cdc.UnmarshalBinaryBare(proof, timestampedSigData); err != nil { - return nil, 0, 0, sdkerrors.Wrapf(err, "failed to unmarshal proof into type %T", timestampedSigData) + return nil, nil, 0, 0, sdkerrors.Wrapf(err, "failed to unmarshal proof into type %T", timestampedSigData) } timestamp := timestampedSigData.Timestamp if len(timestampedSigData.SignatureData) == 0 { - return nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "signature data cannot be empty") + return nil, nil, 0, 0, sdkerrors.Wrap(ErrInvalidProof, "signature data cannot be empty") } sigData, err := UnmarshalSignatureData(cdc, timestampedSigData.SignatureData) if err != nil { - return nil, 0, 0, err + return nil, nil, 0, 0, err } if cs.ConsensusState == nil { - return nil, 0, 0, sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state cannot be empty") + return nil, nil, 0, 0, sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state cannot be empty") } latestSequence := cs.GetLatestHeight().GetVersionHeight() if latestSequence != sequence { - return nil, 0, 0, sdkerrors.Wrapf( + return nil, nil, 0, 0, sdkerrors.Wrapf( sdkerrors.ErrInvalidHeight, "client state sequence != proof sequence (%d != %d)", latestSequence, sequence, ) } if cs.ConsensusState.GetTimestamp() > timestamp { - return nil, 0, 0, sdkerrors.Wrapf(ErrInvalidProof, "the consensus state timestamp is greater than the signature timestamp (%d >= %d)", cs.ConsensusState.GetTimestamp(), timestamp) + return nil, nil, 0, 0, sdkerrors.Wrapf(ErrInvalidProof, "the consensus state timestamp is greater than the signature timestamp (%d >= %d)", cs.ConsensusState.GetTimestamp(), timestamp) } - return sigData, timestamp, sequence, nil + publicKey, err := cs.ConsensusState.GetPubKey() + if err != nil { + return nil, nil, 0, 0, err + } + + return publicKey, sigData, timestamp, sequence, nil } // sets the client state to the store diff --git a/x/ibc/light-clients/06-solomachine/types/consensus_state.go b/x/ibc/light-clients/06-solomachine/types/consensus_state.go index 2b637a7ad2..cebd399ffa 100644 --- a/x/ibc/light-clients/06-solomachine/types/consensus_state.go +++ b/x/ibc/light-clients/06-solomachine/types/consensus_state.go @@ -27,13 +27,19 @@ func (cs ConsensusState) GetRoot() exported.Root { } // GetPubKey unmarshals the public key into a cryptotypes.PubKey type. -func (cs ConsensusState) GetPubKey() cryptotypes.PubKey { - publicKey, ok := cs.PublicKey.GetCachedValue().(cryptotypes.PubKey) - if !ok { - panic("ConsensusState PublicKey is not cryptotypes.PubKey") +// An error is returned if the public key is nil or the cached value +// is not a PubKey. +func (cs ConsensusState) GetPubKey() (cryptotypes.PubKey, error) { + if cs.PublicKey == nil { + return nil, sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state PublicKey cannot be nil") } - return publicKey + publicKey, ok := cs.PublicKey.GetCachedValue().(cryptotypes.PubKey) + if !ok { + return nil, sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "consensus state PublicKey is not cryptotypes.PubKey") + } + + return publicKey, nil } // ValidateBasic defines basic validation for the solo machine consensus state. @@ -44,7 +50,9 @@ func (cs ConsensusState) ValidateBasic() error { if cs.Diversifier != "" && strings.TrimSpace(cs.Diversifier) == "" { return sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "diversifier cannot contain only spaces") } - if cs.PublicKey == nil || cs.GetPubKey() == nil || len(cs.GetPubKey().Bytes()) == 0 { + + publicKey, err := cs.GetPubKey() + if err != nil || publicKey == nil || len(publicKey.Bytes()) == 0 { return sdkerrors.Wrap(clienttypes.ErrInvalidConsensus, "public key cannot be empty") } diff --git a/x/ibc/light-clients/06-solomachine/types/header.go b/x/ibc/light-clients/06-solomachine/types/header.go index e833a70202..cb2b55b4f8 100644 --- a/x/ibc/light-clients/06-solomachine/types/header.go +++ b/x/ibc/light-clients/06-solomachine/types/header.go @@ -24,13 +24,19 @@ func (h Header) GetHeight() exported.Height { } // GetPubKey unmarshals the new public key into a cryptotypes.PubKey type. -func (h Header) GetPubKey() cryptotypes.PubKey { - publicKey, ok := h.NewPublicKey.GetCachedValue().(cryptotypes.PubKey) - if !ok { - panic("Header NewPublicKey is not cryptotypes.PubKey") +// An error is returned if the new public key is nil or the cached value +// is not a PubKey. +func (h Header) GetPubKey() (cryptotypes.PubKey, error) { + if h.NewPublicKey == nil { + return nil, sdkerrors.Wrap(ErrInvalidHeader, "header NewPublicKey cannot be nil") } - return publicKey + publicKey, ok := h.NewPublicKey.GetCachedValue().(cryptotypes.PubKey) + if !ok { + return nil, sdkerrors.Wrap(ErrInvalidHeader, "header NewPublicKey is not cryptotypes.PubKey") + } + + return publicKey, nil } // ValidateBasic ensures that the sequence, signature and public key have all @@ -52,7 +58,8 @@ func (h Header) ValidateBasic() error { return sdkerrors.Wrap(clienttypes.ErrInvalidHeader, "signature cannot be empty") } - if h.NewPublicKey == nil || h.GetPubKey() == nil || len(h.GetPubKey().Bytes()) == 0 { + newPublicKey, err := h.GetPubKey() + if err != nil || newPublicKey == nil || len(newPublicKey.Bytes()) == 0 { return sdkerrors.Wrap(clienttypes.ErrInvalidHeader, "new public key cannot be empty") } diff --git a/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle.go b/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle.go index 2fcbe75c7a..721b84f82e 100644 --- a/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle.go +++ b/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle.go @@ -79,7 +79,12 @@ func verifySignatureAndData(cdc codec.BinaryMarshaler, clientState ClientState, return err } - if err := VerifySignature(clientState.ConsensusState.GetPubKey(), data, sigData); err != nil { + publicKey, err := clientState.ConsensusState.GetPubKey() + if err != nil { + return err + } + + if err := VerifySignature(publicKey, data, sigData); err != nil { return err } diff --git a/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle_test.go b/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle_test.go index fd5871adc6..f247ed2332 100644 --- a/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle_test.go +++ b/x/ibc/light-clients/06-solomachine/types/misbehaviour_handle_test.go @@ -241,6 +241,16 @@ func (suite *SoloMachineTestSuite) TestCheckMisbehaviourAndUpdateState() { }, false, }, + { + "consensus state pubkey is nil", + func() { + cs := solomachine.ClientState() + cs.ConsensusState.PublicKey = nil + clientState = cs + misbehaviour = solomachine.CreateMisbehaviour() + }, + false, + }, } for _, tc := range testCases { diff --git a/x/ibc/light-clients/06-solomachine/types/proposal_handle.go b/x/ibc/light-clients/06-solomachine/types/proposal_handle.go index f715ebb4c5..b55e612b3c 100644 --- a/x/ibc/light-clients/06-solomachine/types/proposal_handle.go +++ b/x/ibc/light-clients/06-solomachine/types/proposal_handle.go @@ -33,7 +33,17 @@ func (cs ClientState) CheckProposedHeaderAndUpdateState( ) } - if reflect.DeepEqual(cs.ConsensusState.GetPubKey(), smHeader.GetPubKey()) { + consensusPublicKey, err := cs.ConsensusState.GetPubKey() + if err != nil { + return nil, nil, sdkerrors.Wrap(err, "failed to get consensus public key") + } + + headerPublicKey, err := smHeader.GetPubKey() + if err != nil { + return nil, nil, sdkerrors.Wrap(err, "failed to get header public key") + } + + if reflect.DeepEqual(consensusPublicKey, headerPublicKey) { return nil, nil, sdkerrors.Wrapf( clienttypes.ErrInvalidHeader, "new public key in header equals current public key", ) diff --git a/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go b/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go index c36e252d41..86d5c78dcc 100644 --- a/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go +++ b/x/ibc/light-clients/06-solomachine/types/proposal_handle_test.go @@ -69,7 +69,14 @@ func (suite *SoloMachineTestSuite) TestCheckProposedHeaderAndUpdateState() { suite.Require().True(ok) suite.Require().Equal(cs.(*types.ClientState).ConsensusState, consState) - suite.Require().Equal(smHeader.GetPubKey(), smConsState.GetPubKey()) + + headerPubKey, err := smHeader.GetPubKey() + suite.Require().NoError(err) + + consStatePubKey, err := smConsState.GetPubKey() + suite.Require().NoError(err) + + suite.Require().Equal(headerPubKey, consStatePubKey) suite.Require().Equal(smHeader.NewDiversifier, smConsState.Diversifier) suite.Require().Equal(smHeader.Timestamp, smConsState.Timestamp) suite.Require().Equal(smHeader.GetHeight().GetVersionHeight(), cs.(*types.ClientState).Sequence) diff --git a/x/ibc/light-clients/06-solomachine/types/update.go b/x/ibc/light-clients/06-solomachine/types/update.go index 4deb8833d5..4cf31fd988 100644 --- a/x/ibc/light-clients/06-solomachine/types/update.go +++ b/x/ibc/light-clients/06-solomachine/types/update.go @@ -62,7 +62,12 @@ func checkHeader(cdc codec.BinaryMarshaler, clientState *ClientState, header *He return err } - if err := VerifySignature(clientState.ConsensusState.GetPubKey(), data, sigData); err != nil { + publicKey, err := clientState.ConsensusState.GetPubKey() + if err != nil { + return err + } + + if err := VerifySignature(publicKey, data, sigData); err != nil { return sdkerrors.Wrap(ErrInvalidHeader, err.Error()) } diff --git a/x/ibc/light-clients/06-solomachine/types/update_test.go b/x/ibc/light-clients/06-solomachine/types/update_test.go index a58b00dc48..74a29017b5 100644 --- a/x/ibc/light-clients/06-solomachine/types/update_test.go +++ b/x/ibc/light-clients/06-solomachine/types/update_test.go @@ -143,6 +143,16 @@ func (suite *SoloMachineTestSuite) TestCheckHeaderAndUpdateState() { }, false, }, + { + "consensus state public key is nil", + func() { + cs := solomachine.ClientState() + cs.ConsensusState.PublicKey = nil + clientState = cs + header = solomachine.CreateHeader() + }, + false, + }, } for _, tc := range testCases {