Skip to content

Commit

Permalink
[FAB-7952] Improve unclear and generic error message
Browse files Browse the repository at this point in the history 
This CR changes the error message reported when a proposal is processed
for a channel that either does not exist or the creator is not a member
of. The message is still left vague ("access denied") to avoid potential
malicious users scanning for channels but echoes back the channel and
and org name supplied by the user to provide some troubleshooting help
for cases where there was a typo in the channel name or the user had an
unexpected MSP configured. The originating error message is logged as-is
on the peer to allow peer admins to know the exact root cause of the
error. This CR also updates the errors throughout the file to use the
github.com/pkg/errors package.

Change-Id: Id3589ceb56fa817a3b95026bb3e3e34629346f80
Signed-off-by: Will Lahti <wtlahti@us.ibm.com>
  • Loading branch information
@wlahti
wlahti committed Feb 23, 2018
1 parent 326d431 commit 2fee96b
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 2fee96b

Please sign in to comment.