Skip to content

Commit

Permalink
Merge "[FAB-7952] Improve unclear and generic error message"
Browse files Browse the repository at this point in the history
  • Loading branch information
@C0rWin
C0rWin authored and Gerrit Code Review committed Feb 23, 2018
2 parents 3bb392a + 2fee96b commit bf3ed58
Show file tree
Hide file tree
Showing 6 changed files with 273 additions and 48 deletions.
18 changes: 10 additions & 8 deletions core/common/validation/fullflow_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ import (
"github.com/stretchr/testify/assert"
)

func getProposal() (*peer.Proposal, error) {
func getProposal(channel string) (*peer.Proposal, error) {
cis := &peer.ChaincodeInvocationSpec{
ChaincodeSpec: &peer.ChaincodeSpec{
ChaincodeId: getChaincodeID(),
Type: peer.ChaincodeSpec_GOLANG}}

proposal, _, err := utils.CreateProposalFromCIS(common.HeaderType_ENDORSER_TRANSACTION, util.GetTestChainID(), cis, signerSerialized)
proposal, _, err := utils.CreateProposalFromCIS(common.HeaderType_ENDORSER_TRANSACTION, channel, cis, signerSerialized)
return proposal, err
}

Expand Down Expand Up @@ -120,7 +120,7 @@ func createSignedTxTwoActions(proposal *peer.Proposal, signer msp.SigningIdentit

func TestGoodPath(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -194,7 +194,7 @@ func TestGoodPath(t *testing.T) {

func TestTXWithTwoActionsRejected(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -227,7 +227,7 @@ func TestTXWithTwoActionsRejected(t *testing.T) {

func TestBadProp(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -304,7 +304,7 @@ func corrupt(bytes []byte) {

func TestBadTx(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -361,7 +361,7 @@ func TestBadTx(t *testing.T) {

func Test2EndorsersAgree(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -404,7 +404,7 @@ func Test2EndorsersAgree(t *testing.T) {

func Test2EndorsersDisagree(t *testing.T) {
// get a toy proposal
prop, err := getProposal()
prop, err := getProposal(util.GetTestChainID())
if err != nil {
t.Fatalf("getProposal failed, err %s", err)
return
Expand Down Expand Up @@ -469,6 +469,7 @@ func TestInvocationsBadArgs(t *testing.T) {

var signer msp.SigningIdentity
var signerSerialized []byte
var signerMSPId string

func TestMain(m *testing.M) {
// setup crypto algorithms
Expand All @@ -486,6 +487,7 @@ func TestMain(m *testing.M) {
os.Exit(-1)
return
}
signerMSPId = signer.GetMSPIdentifier()

signerSerialized, err = signer.Serialize()
if err != nil {
Expand Down
69 changes: 40 additions & 29 deletions core/common/validation/msgvalidation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,32 @@ package validation

import (
"bytes"
"errors"
"fmt"

"github.com/golang/protobuf/proto"
"github.com/hyperledger/fabric/common/channelconfig"
"github.com/hyperledger/fabric/common/flogging"
mspmgmt "github.com/hyperledger/fabric/msp/mgmt"
"github.com/hyperledger/fabric/protos/common"
"github.com/hyperledger/fabric/protos/msp"
pb "github.com/hyperledger/fabric/protos/peer"
"github.com/hyperledger/fabric/protos/utils"
"github.com/pkg/errors"
)

var putilsLogger = flogging.MustGetLogger("protoutils")

// validateChaincodeProposalMessage checks the validity of a Proposal message of type CHAINCODE
func validateChaincodeProposalMessage(prop *pb.Proposal, hdr *common.Header) (*pb.ChaincodeHeaderExtension, error) {
if prop == nil || hdr == nil {
return nil, errors.New("Nil arguments")
return nil, errors.New("nil arguments")
}

putilsLogger.Debugf("validateChaincodeProposalMessage starts for proposal %p, header %p", prop, hdr)

// 4) based on the header type (assuming it's CHAINCODE), look at the extensions
chaincodeHdrExt, err := utils.GetChaincodeHeaderExtension(hdr)
if err != nil {
return nil, errors.New("Invalid header extension for type CHAINCODE")
return nil, errors.New("invalid header extension for type CHAINCODE")
}

if chaincodeHdrExt.ChaincodeId == nil {
Expand All @@ -63,7 +64,7 @@ func validateChaincodeProposalMessage(prop *pb.Proposal, hdr *common.Header) (*p
// encode more elaborate visibility mechanisms that shall be encoded in
// this field (and handled appropriately by the peer)
if chaincodeHdrExt.PayloadVisibility != nil {
return nil, errors.New("Invalid payload visibility field")
return nil, errors.New("invalid payload visibility field")
}

return chaincodeHdrExt, nil
Expand All @@ -74,7 +75,7 @@ func validateChaincodeProposalMessage(prop *pb.Proposal, hdr *common.Header) (*p
// have been unmarshalled and validated
func ValidateProposalMessage(signedProp *pb.SignedProposal) (*pb.Proposal, *common.Header, *pb.ChaincodeHeaderExtension, error) {
if signedProp == nil {
return nil, nil, nil, errors.New("Nil arguments")
return nil, nil, nil, errors.New("nil arguments")
}

putilsLogger.Debugf("ValidateProposalMessage starts for signed proposal %p", signedProp)
Expand All @@ -100,7 +101,17 @@ func ValidateProposalMessage(signedProp *pb.SignedProposal) (*pb.Proposal, *comm
// validate the signature
err = checkSignatureFromCreator(shdr.Creator, signedProp.Signature, signedProp.ProposalBytes, chdr.ChannelId)
if err != nil {
return nil, nil, nil, err
// log the exact message on the peer but return a generic error message to
// avoid malicious users scanning for channels
putilsLogger.Warningf("channel [%s]: %s", chdr.ChannelId, err)
sId := &msp.SerializedIdentity{}
err := proto.Unmarshal(shdr.Creator, sId)
if err != nil {
// log the error here as well but still only return the generic error
err = errors.Wrap(err, "could not deserialize a SerializedIdentity")
putilsLogger.Warningf("channel [%s]: %s", chdr.ChannelId, err)
}
return nil, nil, nil, errors.Errorf("access denied: channel [%s] creator org [%s]", chdr.ChannelId, sId.Mspid)
}

// Verify that the transaction ID has been computed properly.
Expand Down Expand Up @@ -135,49 +146,49 @@ func ValidateProposalMessage(signedProp *pb.SignedProposal) (*pb.Proposal, *comm
return prop, hdr, chaincodeHdrExt, err
default:
//NOTE : we proably need a case
return nil, nil, nil, fmt.Errorf("Unsupported proposal type %d", common.HeaderType(chdr.Type))
return nil, nil, nil, errors.Errorf("unsupported proposal type %d", common.HeaderType(chdr.Type))
}
}

// given a creator, a message and a signature,
// this function returns nil if the creator
// is a valid cert and the signature is valid
func checkSignatureFromCreator(creatorBytes []byte, sig []byte, msg []byte, ChainID string) error {
putilsLogger.Debugf("checkSignatureFromCreator starts")
putilsLogger.Debugf("begin")

// check for nil argument
if creatorBytes == nil || sig == nil || msg == nil {
return errors.New("Nil arguments")
return errors.New("nil arguments")
}

mspObj := mspmgmt.GetIdentityDeserializer(ChainID)
if mspObj == nil {
return fmt.Errorf("could not get msp for chain [%s]", ChainID)
return errors.Errorf("could not get msp for channel [%s]", ChainID)
}

// get the identity of the creator
creator, err := mspObj.DeserializeIdentity(creatorBytes)
if err != nil {
return fmt.Errorf("Failed to deserialize creator identity, err %s", err)
return errors.WithMessage(err, "MSP error")
}

putilsLogger.Debugf("checkSignatureFromCreator info: creator is %s", creator.GetIdentifier())
putilsLogger.Debugf("creator is %s", creator.GetIdentifier())

// ensure that creator is a valid certificate
err = creator.Validate()
if err != nil {
return fmt.Errorf("The creator certificate is not valid, err %s", err)
return errors.WithMessage(err, "creator certificate is not valid")
}

putilsLogger.Debugf("checkSignatureFromCreator info: creator is valid")
putilsLogger.Debugf("creator is valid")

// validate the signature
err = creator.Verify(msg, sig)
if err != nil {
return fmt.Errorf("The creator's signature over the proposal is not valid, err %s", err)
return errors.WithMessage(err, "creator's signature over the proposal is not valid")
}

putilsLogger.Debugf("checkSignatureFromCreator exists successfully")
putilsLogger.Debugf("exits successfully")

return nil
}
Expand All @@ -186,17 +197,17 @@ func checkSignatureFromCreator(creatorBytes []byte, sig []byte, msg []byte, Chai
func validateSignatureHeader(sHdr *common.SignatureHeader) error {
// check for nil argument
if sHdr == nil {
return errors.New("Nil SignatureHeader provided")
return errors.New("nil SignatureHeader provided")
}

// ensure that there is a nonce
if sHdr.Nonce == nil || len(sHdr.Nonce) == 0 {
return errors.New("Invalid nonce specified in the header")
return errors.New("invalid nonce specified in the header")
}

// ensure that there is a creator
if sHdr.Creator == nil || len(sHdr.Creator) == 0 {
return errors.New("Invalid creator specified in the header")
return errors.New("invalid creator specified in the header")
}

return nil
Expand All @@ -206,15 +217,15 @@ func validateSignatureHeader(sHdr *common.SignatureHeader) error {
func validateChannelHeader(cHdr *common.ChannelHeader) error {
// check for nil argument
if cHdr == nil {
return errors.New("Nil ChannelHeader provided")
return errors.New("nil ChannelHeader provided")
}

// validate the header type
if common.HeaderType(cHdr.Type) != common.HeaderType_ENDORSER_TRANSACTION &&
common.HeaderType(cHdr.Type) != common.HeaderType_CONFIG_UPDATE &&
common.HeaderType(cHdr.Type) != common.HeaderType_CONFIG &&
common.HeaderType(cHdr.Type) != common.HeaderType_PEER_RESOURCE_UPDATE {
return fmt.Errorf("invalid header type %s", common.HeaderType(cHdr.Type))
return errors.Errorf("invalid header type %s", common.HeaderType(cHdr.Type))
}

putilsLogger.Debugf("validateChannelHeader info: header type %d", common.HeaderType(cHdr.Type))
Expand All @@ -226,7 +237,7 @@ func validateChannelHeader(cHdr *common.ChannelHeader) error {
// TODO: This check will be modified once the Epoch management
// will be in place.
if cHdr.Epoch != 0 {
return fmt.Errorf("Invalid Epoch in ChannelHeader. It must be 0. It was [%d]", cHdr.Epoch)
return errors.Errorf("invalid Epoch in ChannelHeader. Expected 0, got [%d]", cHdr.Epoch)
}

// TODO: Validate version in cHdr.Version
Expand All @@ -237,7 +248,7 @@ func validateChannelHeader(cHdr *common.ChannelHeader) error {
// checks for a valid Header
func validateCommonHeader(hdr *common.Header) (*common.ChannelHeader, *common.SignatureHeader, error) {
if hdr == nil {
return nil, nil, errors.New("Nil header")
return nil, nil, errors.New("nil header")
}

chdr, err := utils.UnmarshalChannelHeader(hdr.ChannelHeader)
Expand Down Expand Up @@ -270,7 +281,7 @@ func validateConfigTransaction(data []byte, hdr *common.Header) error {

// check for nil argument
if data == nil || hdr == nil {
return errors.New("Nil arguments")
return errors.New("nil arguments")
}

// There is no need to do this validation here, the configtx.Validator handles this
Expand All @@ -285,7 +296,7 @@ func validateEndorserTransaction(data []byte, hdr *common.Header) error {

// check for nil argument
if data == nil || hdr == nil {
return errors.New("Nil arguments")
return errors.New("nil arguments")
}

// if the type is ENDORSER_TRANSACTION we unmarshal a Transaction message
Expand All @@ -296,7 +307,7 @@ func validateEndorserTransaction(data []byte, hdr *common.Header) error {

// check for nil argument
if tx == nil {
return errors.New("Nil transaction")
return errors.New("nil transaction")
}

// TODO: validate tx.Version
Expand All @@ -305,15 +316,15 @@ func validateEndorserTransaction(data []byte, hdr *common.Header) error {

// hlf version 1 only supports a single action per transaction
if len(tx.Actions) != 1 {
return fmt.Errorf("Only one action per transaction is supported (tx contains %d)", len(tx.Actions))
return errors.Errorf("only one action per transaction is supported, tx contains %d", len(tx.Actions))
}

putilsLogger.Debugf("validateEndorserTransaction info: there are %d actions", len(tx.Actions))

for _, act := range tx.Actions {
// check for nil argument
if act == nil {
return errors.New("Nil action")
return errors.New("nil action")
}

// if the type is ENDORSER_TRANSACTION we unmarshal a SignatureHeader
Expand Down
Loading

0 comments on commit bf3ed58

Please sign in to comment.