From 98da6c61c14531a47ae92c51e808c2d8b2588b53 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 4 Jun 2020 17:44:25 -0700 Subject: [PATCH] signrpc: parse both KeyDescriptor fields for SignOutputRaw requests This is meant to handle a quirk in which key descriptors obtained through walletrpc.DeriveKey don't result in the derived key being persisted to the wallet's database, unlike with DeriveNextKey. Due to this and some fallback logic in the wallet with regards to empty key locators, if a request only specified the compressed public key, the signature returned would be over a different key, namely the one derived from (family=0, index=0). --- lnrpc/signrpc/signer.pb.go | 4 ++++ lnrpc/signrpc/signer.proto | 4 ++++ lnrpc/signrpc/signer.swagger.json | 2 +- lnrpc/signrpc/signer_server.go | 15 ++++++++------- 4 files changed, 17 insertions(+), 8 deletions(-) diff --git a/lnrpc/signrpc/signer.pb.go b/lnrpc/signrpc/signer.pb.go index f10416008..8e78c8a9e 100644 --- a/lnrpc/signrpc/signer.pb.go +++ b/lnrpc/signrpc/signer.pb.go @@ -180,6 +180,10 @@ type SignDescriptor struct { //A descriptor that precisely describes *which* key to use for signing. This //may provide the raw public key directly, or require the Signer to re-derive //the key according to the populated derivation path. + // + //Note that if the key descriptor was obtained through walletrpc.DeriveKey, + //then the key locator MUST always be provided, since the derived keys are not + //persisted unlike with DeriveNextKey. KeyDesc *KeyDescriptor `protobuf:"bytes,1,opt,name=key_desc,json=keyDesc,proto3" json:"key_desc,omitempty"` // //A scalar value that will be added to the private key corresponding to the diff --git a/lnrpc/signrpc/signer.proto b/lnrpc/signrpc/signer.proto index 844090405..003d1e7e9 100644 --- a/lnrpc/signrpc/signer.proto +++ b/lnrpc/signrpc/signer.proto @@ -98,6 +98,10 @@ message SignDescriptor { A descriptor that precisely describes *which* key to use for signing. This may provide the raw public key directly, or require the Signer to re-derive the key according to the populated derivation path. + + Note that if the key descriptor was obtained through walletrpc.DeriveKey, + then the key locator MUST always be provided, since the derived keys are not + persisted unlike with DeriveNextKey. */ KeyDescriptor key_desc = 1; diff --git a/lnrpc/signrpc/signer.swagger.json b/lnrpc/signrpc/signer.swagger.json index 28439300d..594a9b82d 100644 --- a/lnrpc/signrpc/signer.swagger.json +++ b/lnrpc/signrpc/signer.swagger.json @@ -303,7 +303,7 @@ "properties": { "key_desc": { "$ref": "#/definitions/signrpcKeyDescriptor", - "description": "A descriptor that precisely describes *which* key to use for signing. This\nmay provide the raw public key directly, or require the Signer to re-derive\nthe key according to the populated derivation path." + "description": "A descriptor that precisely describes *which* key to use for signing. This\nmay provide the raw public key directly, or require the Signer to re-derive\nthe key according to the populated derivation path.\n\nNote that if the key descriptor was obtained through walletrpc.DeriveKey,\nthen the key locator MUST always be provided, since the derived keys are not\npersisted unlike with DeriveNextKey." }, "single_tweak": { "type": "string", diff --git a/lnrpc/signrpc/signer_server.go b/lnrpc/signrpc/signer_server.go index fca12ad78..33651d7b3 100644 --- a/lnrpc/signrpc/signer_server.go +++ b/lnrpc/signrpc/signer_server.go @@ -242,17 +242,17 @@ func (s *Server) SignOutputRaw(ctx context.Context, in *SignReq) (*SignResp, err keyDesc := signDesc.KeyDesc // The caller can either specify the key using the raw pubkey, - // or the description of the key. Below we'll feel out the - // oneof field to decide which one we will attempt to parse. + // or the description of the key. We'll still attempt to parse + // both if both were provided however, to ensure the underlying + // SignOutputRaw has as much information as possible. var ( targetPubKey *btcec.PublicKey keyLoc keychain.KeyLocator ) - switch { // If this method doesn't return nil, then we know that user is // attempting to include a raw serialized pub key. - case keyDesc.GetRawKeyBytes() != nil: + if keyDesc.GetRawKeyBytes() != nil { rawKeyBytes := keyDesc.GetRawKeyBytes() switch { @@ -275,10 +275,11 @@ func (s *Server) SignOutputRaw(ctx context.Context, in *SignReq) (*SignResp, err "parse pubkey: %v", err) } } + } - // Similarly, if they specified a key locator, then we'll use - // that instead. - case keyDesc.GetKeyLoc() != nil: + // Similarly, if they specified a key locator, then we'll parse + // that as well. + if keyDesc.GetKeyLoc() != nil { protoLoc := keyDesc.GetKeyLoc() keyLoc = keychain.KeyLocator{ Family: keychain.KeyFamily(