From 6ab9252bb264c8f581f0dff8222719e4da71383a Mon Sep 17 00:00:00 2001 From: ukane-philemon Date: Wed, 16 Mar 2022 12:20:27 +0100 Subject: [PATCH 1/2] add new helper functions --- webapi/helpers.go | 81 ++++++++++++++++++++++++++++++++++++----- webapi/middleware.go | 85 ++++++++++++-------------------------------- 2 files changed, 94 insertions(+), 72 deletions(-) diff --git a/webapi/helpers.go b/webapi/helpers.go index 7734adc7..e5e2cadd 100644 --- a/webapi/helpers.go +++ b/webapi/helpers.go @@ -15,6 +15,7 @@ import ( "github.com/decred/dcrd/dcrec/secp256k1/v4" "github.com/decred/dcrd/dcrutil/v4" "github.com/decred/dcrd/wire" + "github.com/decred/vspd/rpc" "github.com/gin-gonic/gin" ) @@ -105,16 +106,22 @@ func validPolicyOption(policy string) error { } } -func validateSignature(reqBytes []byte, commitmentAddress string, c *gin.Context) error { - // Ensure a signature is provided. - signature := c.GetHeader("VSP-Client-Signature") - if signature == "" { - return errors.New("no VSP-Client-Signature header") - } +func validateSignature(hash, commitmentAddress, signature, message string, c *gin.Context) error { + firstErr := dcrutil.VerifyMessage(commitmentAddress, signature, message, cfg.NetParams) + if firstErr != nil { + // Don't return an error straight away if sig validation fails - + // first check if we have an alternate sign address for this ticket. + altSigData, err := db.AltSignAddrData(hash) + if err != nil { + return fmt.Errorf("db.AltSignAddrData failed (ticketHash=%s): %v", hash, err) + } - err := dcrutil.VerifyMessage(commitmentAddress, signature, string(reqBytes), cfg.NetParams) - if err != nil { - return err + // If we have no alternate sign address, or if validating with the + // alt sign addr fails, return an error to the client. + if altSigData == nil || dcrutil.VerifyMessage(altSigData.AltSignAddr, signature, message, cfg.NetParams) != nil { + return fmt.Errorf("Bad signature (clientIP=%s, ticketHash=%s)", c.ClientIP(), hash) + } + return firstErr } return nil } @@ -141,3 +148,59 @@ func isValidTicket(tx *wire.MsgTx) error { return nil } + +// validateTicketHash ensures the provided ticket hash is a valid ticket hash. +// A ticket hash should be 64 chars (MaxHashStringSize) and should parse into +// a chainhash.Hash without error. +func validateTicketHash(c *gin.Context, hash string) (bool, error) { + if len(hash) != chainhash.MaxHashStringSize { + return false, fmt.Errorf("Incorrect hash length (clientIP=%s): got %d, expected %d", c.ClientIP(), len(hash), chainhash.MaxHashStringSize) + + } + _, err := chainhash.NewHashFromStr(hash) + if err != nil { + return false, fmt.Errorf("Invalid hash (clientIP=%s): %v", c.ClientIP(), err) + + } + + return true, nil +} + +// getCommitmentAddress gets the commitment address of the provided ticket hash +// from the chain. +func getCommitmentAddress(c *gin.Context, hash string) (string, bool, error) { + var commitmentAddress string + dcrdClient := c.MustGet(dcrdKey).(*rpc.DcrdRPC) + dcrdErr := c.MustGet(dcrdErrorKey) + if dcrdErr != nil { + return commitmentAddress, false, fmt.Errorf("could not get dcrd client: %v", dcrdErr.(error)) + + } + + resp, err := dcrdClient.GetRawTransaction(hash) + if err != nil { + return commitmentAddress, false, fmt.Errorf("dcrd.GetRawTransaction for ticket failed (ticketHash=%s): %v", hash, err) + + } + + msgTx, err := decodeTransaction(resp.Hex) + if err != nil { + return commitmentAddress, false, fmt.Errorf("Failed to decode ticket hex (ticketHash=%s): %v", hash, err) + + } + + err = isValidTicket(msgTx) + if err != nil { + return commitmentAddress, true, fmt.Errorf("Invalid ticket (clientIP=%s, ticketHash=%s): %v", c.ClientIP(), hash, err) + + } + + addr, err := stake.AddrFromSStxPkScrCommitment(msgTx.TxOut[1].PkScript, cfg.NetParams) + if err != nil { + return commitmentAddress, false, fmt.Errorf("AddrFromSStxPkScrCommitment error (ticketHash=%s): %v", hash, err) + + } + + commitmentAddress = addr.String() + return commitmentAddress, false, nil +} diff --git a/webapi/middleware.go b/webapi/middleware.go index 0b66e63c..cc36e1c5 100644 --- a/webapi/middleware.go +++ b/webapi/middleware.go @@ -11,8 +11,6 @@ import ( "net/http" "strings" - "github.com/decred/dcrd/blockchain/stake/v4" - "github.com/decred/dcrd/chaincfg/chainhash" "github.com/decred/vspd/rpc" "github.com/gin-gonic/gin" "github.com/gin-gonic/gin/binding" @@ -287,17 +285,9 @@ func vspAuth() gin.HandlerFunc { hash := request.TicketHash // Before hitting the db or any RPC, ensure this is a valid ticket hash. - // A ticket hash should be 64 chars (MaxHashStringSize) and should parse - // into a chainhash.Hash without error. - if len(hash) != chainhash.MaxHashStringSize { - log.Errorf("%s: Incorrect hash length (clientIP=%s): got %d, expected %d", - funcName, c.ClientIP(), len(hash), chainhash.MaxHashStringSize) - sendErrorWithMsg("invalid ticket hash", errBadRequest, c) - return - } - _, err = chainhash.NewHashFromStr(hash) - if err != nil { - log.Errorf("%s: Invalid hash (clientIP=%s): %v", funcName, c.ClientIP(), err) + validticket, err := validateTicketHash(c, hash) + if !validticket { + log.Errorf("%s: %v", funcName, err) sendErrorWithMsg("invalid ticket hash", errBadRequest, c) return } @@ -313,67 +303,36 @@ func vspAuth() gin.HandlerFunc { // If the ticket was found in the database, we already know its // commitment address. Otherwise we need to get it from the chain. var commitmentAddress string + var isInvalid bool + if ticketFound { commitmentAddress = ticket.CommitmentAddress } else { - dcrdClient := c.MustGet(dcrdKey).(*rpc.DcrdRPC) - dcrdErr := c.MustGet(dcrdErrorKey) - if dcrdErr != nil { - log.Errorf("%s: could not get dcrd client: %v", funcName, dcrdErr.(error)) - sendError(errInternalError, c) - return - } - - resp, err := dcrdClient.GetRawTransaction(hash) - if err != nil { - log.Errorf("%s: dcrd.GetRawTransaction for ticket failed (ticketHash=%s): %v", funcName, hash, err) - sendError(errInternalError, c) - return - } - - msgTx, err := decodeTransaction(resp.Hex) - if err != nil { - log.Errorf("%s: Failed to decode ticket hex (ticketHash=%s): %v", funcName, ticket.Hash, err) - sendError(errInternalError, c) - return - } - - err = isValidTicket(msgTx) - if err != nil { - log.Warnf("%s: Invalid ticket (clientIP=%s, ticketHash=%s): %v", funcName, c.ClientIP(), hash, err) - sendError(errInvalidTicket, c) - return - } - - addr, err := stake.AddrFromSStxPkScrCommitment(msgTx.TxOut[1].PkScript, cfg.NetParams) + commitmentAddress, isInvalid, err = getCommitmentAddress(c, hash) if err != nil { - log.Errorf("%s: AddrFromSStxPkScrCommitment error (ticketHash=%s): %v", funcName, hash, err) - sendError(errInternalError, c) + if isInvalid { + sendError(errInvalidTicket, c) + } else { + sendError(errInternalError, c) + } + log.Errorf("%s: %v", funcName, err) return } + } - commitmentAddress = addr.String() + // Ensure a signature is provided. + signature := c.GetHeader("VSP-Client-Signature") + if signature == "" { + sendErrorWithMsg("no VSP-Client-Signature header", errBadRequest, c) + return } // Validate request signature to ensure ticket ownership. - err = validateSignature(reqBytes, commitmentAddress, c) + err = validateSignature(hash, commitmentAddress, signature, string(reqBytes), c) if err != nil { - // Don't return an error straight away if sig validation fails - - // first check if we have an alternate sign address for this ticket. - altSigData, err := db.AltSignAddrData(hash) - if err != nil { - log.Errorf("%s: db.AltSignAddrData failed (ticketHash=%s): %v", funcName, hash, err) - sendError(errInternalError, c) - return - } - - // If we have no alternate sign address, or if validating with the - // alt sign addr fails, return an error to the client. - if altSigData == nil || validateSignature(reqBytes, altSigData.AltSignAddr, c) != nil { - log.Warnf("%s: Bad signature (clientIP=%s, ticketHash=%s)", funcName, c.ClientIP(), hash) - sendError(errBadSignature, c) - return - } + log.Errorf("%s: %v", funcName, err) + sendError(errBadSignature, c) + return } // Add ticket information to context so downstream handlers don't need From 746e7d95ff5387736e66b318b8f197253726c48f Mon Sep 17 00:00:00 2001 From: ukane-philemon Date: Thu, 17 Mar 2022 12:48:05 +0100 Subject: [PATCH 2/2] review: refactor errors strings and return values --- webapi/helpers.go | 36 ++++++++++++++---------------------- webapi/middleware.go | 26 +++++++++++++++++--------- 2 files changed, 31 insertions(+), 31 deletions(-) diff --git a/webapi/helpers.go b/webapi/helpers.go index e5e2cadd..6cd063ee 100644 --- a/webapi/helpers.go +++ b/webapi/helpers.go @@ -16,7 +16,6 @@ import ( "github.com/decred/dcrd/dcrutil/v4" "github.com/decred/dcrd/wire" "github.com/decred/vspd/rpc" - "github.com/gin-gonic/gin" ) func currentVoteVersion(params *chaincfg.Params) uint32 { @@ -106,22 +105,22 @@ func validPolicyOption(policy string) error { } } -func validateSignature(hash, commitmentAddress, signature, message string, c *gin.Context) error { +func validateSignature(hash, commitmentAddress, signature, message string) error { firstErr := dcrutil.VerifyMessage(commitmentAddress, signature, message, cfg.NetParams) if firstErr != nil { // Don't return an error straight away if sig validation fails - // first check if we have an alternate sign address for this ticket. altSigData, err := db.AltSignAddrData(hash) if err != nil { - return fmt.Errorf("db.AltSignAddrData failed (ticketHash=%s): %v", hash, err) + return fmt.Errorf("db.AltSignAddrData failed: %v", err) } // If we have no alternate sign address, or if validating with the // alt sign addr fails, return an error to the client. if altSigData == nil || dcrutil.VerifyMessage(altSigData.AltSignAddr, signature, message, cfg.NetParams) != nil { - return fmt.Errorf("Bad signature (clientIP=%s, ticketHash=%s)", c.ClientIP(), hash) + return fmt.Errorf("bad signature") } - return firstErr + } return nil } @@ -152,55 +151,48 @@ func isValidTicket(tx *wire.MsgTx) error { // validateTicketHash ensures the provided ticket hash is a valid ticket hash. // A ticket hash should be 64 chars (MaxHashStringSize) and should parse into // a chainhash.Hash without error. -func validateTicketHash(c *gin.Context, hash string) (bool, error) { +func validateTicketHash(hash string) error { if len(hash) != chainhash.MaxHashStringSize { - return false, fmt.Errorf("Incorrect hash length (clientIP=%s): got %d, expected %d", c.ClientIP(), len(hash), chainhash.MaxHashStringSize) + return fmt.Errorf("incorrect hash length: got %d, expected %d", len(hash), chainhash.MaxHashStringSize) } _, err := chainhash.NewHashFromStr(hash) if err != nil { - return false, fmt.Errorf("Invalid hash (clientIP=%s): %v", c.ClientIP(), err) + return fmt.Errorf("invalid hash: %v", err) } - return true, nil + return nil } // getCommitmentAddress gets the commitment address of the provided ticket hash // from the chain. -func getCommitmentAddress(c *gin.Context, hash string) (string, bool, error) { +func getCommitmentAddress(hash string, dcrdClient *rpc.DcrdRPC) (string, error) { var commitmentAddress string - dcrdClient := c.MustGet(dcrdKey).(*rpc.DcrdRPC) - dcrdErr := c.MustGet(dcrdErrorKey) - if dcrdErr != nil { - return commitmentAddress, false, fmt.Errorf("could not get dcrd client: %v", dcrdErr.(error)) - - } - resp, err := dcrdClient.GetRawTransaction(hash) if err != nil { - return commitmentAddress, false, fmt.Errorf("dcrd.GetRawTransaction for ticket failed (ticketHash=%s): %v", hash, err) + return commitmentAddress, fmt.Errorf("dcrd.GetRawTransaction for ticket failed: %v", err) } msgTx, err := decodeTransaction(resp.Hex) if err != nil { - return commitmentAddress, false, fmt.Errorf("Failed to decode ticket hex (ticketHash=%s): %v", hash, err) + return commitmentAddress, fmt.Errorf("Failed to decode ticket hex: %v", err) } err = isValidTicket(msgTx) if err != nil { - return commitmentAddress, true, fmt.Errorf("Invalid ticket (clientIP=%s, ticketHash=%s): %v", c.ClientIP(), hash, err) + return commitmentAddress, fmt.Errorf("Invalid ticket: %w", errInvalidTicket) } addr, err := stake.AddrFromSStxPkScrCommitment(msgTx.TxOut[1].PkScript, cfg.NetParams) if err != nil { - return commitmentAddress, false, fmt.Errorf("AddrFromSStxPkScrCommitment error (ticketHash=%s): %v", hash, err) + return commitmentAddress, fmt.Errorf("AddrFromSStxPkScrCommitment error: %v", err) } commitmentAddress = addr.String() - return commitmentAddress, false, nil + return commitmentAddress, nil } diff --git a/webapi/middleware.go b/webapi/middleware.go index cc36e1c5..9bc235d1 100644 --- a/webapi/middleware.go +++ b/webapi/middleware.go @@ -285,9 +285,9 @@ func vspAuth() gin.HandlerFunc { hash := request.TicketHash // Before hitting the db or any RPC, ensure this is a valid ticket hash. - validticket, err := validateTicketHash(c, hash) - if !validticket { - log.Errorf("%s: %v", funcName, err) + err = validateTicketHash(hash) + if err != nil { + log.Errorf("%s: Bad request (clientIP=%s): %v", funcName, c.ClientIP(), err) sendErrorWithMsg("invalid ticket hash", errBadRequest, c) return } @@ -303,19 +303,26 @@ func vspAuth() gin.HandlerFunc { // If the ticket was found in the database, we already know its // commitment address. Otherwise we need to get it from the chain. var commitmentAddress string - var isInvalid bool + dcrdClient := c.MustGet(dcrdKey).(*rpc.DcrdRPC) + dcrdErr := c.MustGet(dcrdErrorKey) + if dcrdErr != nil { + log.Errorf("%s: could not get dcrd client: %v", funcName, dcrdErr.(error)) + sendError(errInternalError, c) + return + } if ticketFound { commitmentAddress = ticket.CommitmentAddress } else { - commitmentAddress, isInvalid, err = getCommitmentAddress(c, hash) + commitmentAddress, err = getCommitmentAddress(hash, dcrdClient) if err != nil { - if isInvalid { + var apiErr *apiError + if errors.Is(err, apiErr) { sendError(errInvalidTicket, c) } else { sendError(errInternalError, c) } - log.Errorf("%s: %v", funcName, err) + log.Errorf("%s: (clientIP: %s, ticketHash: %s): %v", funcName, c.ClientIP(), hash, err) return } } @@ -323,14 +330,15 @@ func vspAuth() gin.HandlerFunc { // Ensure a signature is provided. signature := c.GetHeader("VSP-Client-Signature") if signature == "" { + log.Warnf("%s: Bad request (clientIP=%s): %v", funcName, c.ClientIP(), err) sendErrorWithMsg("no VSP-Client-Signature header", errBadRequest, c) return } // Validate request signature to ensure ticket ownership. - err = validateSignature(hash, commitmentAddress, signature, string(reqBytes), c) + err = validateSignature(hash, commitmentAddress, signature, string(reqBytes)) if err != nil { - log.Errorf("%s: %v", funcName, err) + log.Errorf("%s: Bad signature (clientIP=%s, ticketHash=%s): %v", funcName, err) sendError(errBadSignature, c) return }