Adds lightning message signing/verification/pk_recovery #844

sr-gi · 2021-03-16T15:02:34Z

Adds message signing, verification and public key recovery.

Signatures are pk-recoverable zbase32-encoded, following:

zbase32(SigRec(SHA256(SHA256("Lightning Signed Message:" + msg))))

SigRec has first byte 31 + recovery id, followed by 64 byte sig.

close #843

lightning/Cargo.toml

TheBlueMatt

There are a number of issues in the zbase32 implementation, but I went ahead and fixed those while writing new tests at https://git.bitcoin.ninja/?p=rust-lightning;a=shortlog;h=refs/heads/fix-zbase32 so you can just pull the commits off that branch.

There's a few comments in the message_signing stuff too, but mostly just documentation and API questions.

lightning/src/util/zbase32.rs

lightning/src/util/message_signing.rs

TheBlueMatt · 2021-03-22T16:37:35Z

lightning/src/util/message_signing.rs

+    Ok(zbase32::encode(&sigrec_encode(sig)))
+}
+
+pub fn recover_pk(msg: &[u8], sig: String) ->  Result<PublicKey, Error>{


Does this need to be pub?

It may so it can be bound to foreign langs, doesn't it?

Right, my question is what's the use for it - do users have a direct need for this?

Ohh, there is.

lnd's verifymessage returns a bool + the recovered public key.

Furthermore, in the Eye of Satoshi case, we actually use this to authenticate users. All requests are signed but public keys are only shared when the user is registered, so EC recovery is used elsewhere.

Ah, ok! May be useful to rename this function, then - something that includes the word "verify" in it, indicating that the message is valid if the public key matches. Needs docs either way.

Checking this further I think there's a need for the three of them to be public.

lnd's implementation verifies the message by checking if the recovered public key matches the id of any of the nodes in the node's database.

c-lightning's allows you to provide a public key to verify. If you do, then it verifies the signature against the provided key, otherwise it does the same as lnd's, and checks it agains the known nodes.

How you check if the recovered key is part of a set of known nodes looks implementation dependant to me, so it feels making this public makes the most sense. Furthermore, in order to allow users to check this against something different than the nodes in the network (e.g. to allow ephemeral identities or identities created from key tweaking) this would be required. This is the exact use case for teos.

I kinda like having the verify one, its certainly a much, much safer API, whereas this API its possible to forget to compare the public key against something known-good. Obviously if there's a use for it, then we should keep this, but having the server version feels nice too.

ariard

Concept ACK.

I just made it up. Since there is no spec I don't think there are any standard test vectors, but I can replicate the ones used by c-lightning and lnd so we can make sure those pass at least

If you feel it, maybe just propose a spec against the BOLT/BIPs ? It should be easy to land if you have test vectors and other implementations already have support.

lightning/src/util/message_signing.rs

ariard · 2021-03-24T15:16:38Z

lightning/src/util/zbase32.rs

+
+/// Decodes a zbase32 string to the original bytes, failing if the string was not encoded by a
+/// proper zbase32 encoder.
+pub fn decode(data: &str) -> Result<Vec<u8>, ()> {


This work well pub(super), no need to make them public ?

I made it public so it can be easily bound with methods that import lightning instead of working within the same repo for the bindings, like the Python bindings do. Happy do discuss it though.

Does that mean you need zbase32 for other uses, or just generally you think people may need zbase32 for other things and want it via bindings? I don't think there's any difference between bindings-vs-not when deciding if something should be pub.

It may not be needed for anything but signing, so I guess we could make it private.

lightning/src/util/message_signing.rs

sr-gi · 2021-03-26T11:13:47Z

Addressed formatting issue, nits, docs missing and added additional tests.

If you feel it, maybe just propose a spec against the BOLT/BIPs ? It should be easy to land if you have test vectors and other implementations already have support.

Wouldn't mind proposing adding this to the specs, but not sure where it would fit best. Any suggestions?

TheBlueMatt · 2021-03-26T15:02:33Z

Wouldn't mind proposing adding this to the specs, but not sure where it would fit best. Any suggestions?

Presumably a new BOLT? I dunno.

Note that CI is failing because the zbase32 module doesn't have documentation (ie via //! comments at the top of zbase32.rs).

TheBlueMatt · 2021-03-28T19:59:05Z

I don't think we should have to wait on a new BOLT for this, though one would certainly be nice. I think the only thing still needed is some module-docs on zbase32.

sr-gi · 2021-03-31T09:47:50Z

I've changed zbase32 to pub(crate) based on #844 (comment). If you feel there may be an additional use case that would require this to be public I'm happy to rebase and add docs.

codecov · 2021-03-31T09:58:41Z

Codecov Report

Merging #844 (7bcf5a1) into main (32f6205) will decrease coverage by 0.43%.
The diff coverage is 94.82%.

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   90.66%   90.23%   -0.44%     
==========================================
  Files          51       57       +6     
  Lines       27135    29207    +2072     
==========================================
+ Hits        24603    26355    +1752     
- Misses       2532     2852     +320

Impacted Files	Coverage Δ
lightning/src/util/message_signing.rs	`93.10% <93.10%> (ø)`
lightning/src/util/zbase32.rs	`96.55% <96.55%> (ø)`
lightning/src/util/events.rs	`18.26% <0.00%> (-5.63%)`	⬇️
lightning-persister/src/lib.rs	`94.06% <0.00%> (-1.55%)`	⬇️
lightning/src/routing/router.rs	`95.79% <0.00%> (-1.18%)`	⬇️
lightning/src/util/config.rs	`47.61% <0.00%> (-1.17%)`	⬇️
lightning/src/util/test_utils.rs	`83.23% <0.00%> (-0.42%)`	⬇️
lightning/src/ln/functional_tests.rs	`96.83% <0.00%> (-0.28%)`	⬇️
lightning/src/ln/channelmanager.rs	`83.11% <0.00%> (-0.18%)`	⬇️
lightning/src/ln/onion_route_tests.rs	`96.77% <0.00%> (-0.08%)`	⬇️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32f6205...7bcf5a1. Read the comment docs.

TheBlueMatt · 2021-03-31T13:22:09Z

lightning/src/util/mod.rs


 pub(crate) mod byte_utils;
 pub(crate) mod chacha20;
+pub(crate) mod zbase32;


Oops, the fuzztarget mode needs it, see CI.

Suggested change

pub(crate) mod zbase32;

#[cfg(feature = "fuzztarget")]

pub mod zbase32;

#[cfg(not(feature = "fuzztarget"))]

pub(crate) mod zbase32;

Added it in cf384a3 and changed zbase32::{encode, decode} to pub so they can be used in fuzzing.

lightning/src/lib.rs

TheBlueMatt · 2021-04-01T15:09:00Z

lightning/src/util/mod.rs

+#[cfg(feature = "fuzztarget")]
+pub mod zbase32;
 #[cfg(not(feature = "fuzztarget"))]
+pub(crate) mod zbase32;


The compile error in CI is that poly1305 is no longer #[cfg(not(feature = "fuzztarget"))] (ie not compiled in fuzzing). You need to duplicate the cfg line, above.

valentinewallace

Sorry to nit you to death on grammar, wouldn't be so pressed if it weren't public docs.

Lmk if the testing instructions are unclear or if I'm in the wrong there

lightning/src/util/message_signing.rs

valentinewallace · 2021-04-09T16:51:46Z

lightning/src/util/message_signing.rs

+
+/// Verifies a message was signed by a PrivateKey that derives to a given PublicKey, given a message, a signature,
+/// and the PublicKey.
+pub fn verify(msg: &[u8], sig: &str, pk: PublicKey) -> bool {


I tested this with the sample and lnd's telling me the message is invalid (the pubkey's correct): https://i.imgur.com/lizoFib.png

Haven't tested further.
Instructions to reproduce/test further (I used Polar but feel free to use whatever you want):

clone this branch of the sample: https://github.com/valentinewallace/ldk-sample/tree/for-testing-signing (it uses a branch of rust-lightning that's just your branch + one commit to expose a struct that's needed)

I used Polar: https://github.com/jamaljsr/polar by cloning and running yarn dev

start a new test network in Polar and add an lnd node

start the sample: cargo run polaruser:[email protected]:18443 ./ 9738 regtest

in the sample CLI: connectpeer <lnd node>, where lnd node's found under alice > Connect > P2P External

in the sample CLI: signmessage (the message is hardcoded, see: https://github.com/valentinewallace/ldk-sample/blob/for-testing-signing/src/cli.rs#L378)

copy this output, then open a terminal for the lnd node under alice > Actions > Terminal > Launch

lncli verifymessage --msg abcdefghijklmnopqrstuvwxyz1234567890 --sig <sig you just copied>

Also added verifymessage to the sample: https://github.com/valentinewallace/ldk-sample/blob/for-testing-signing/src/cli.rs#L390 if you wanna test that

I tested this with the sample and lnd's telling me the message is invalid (the pubkey's correct): https://i.imgur.com/lizoFib.png

Wouldn't this mean the two nodes are not properly connected (or the node that originated the signature is not on the verifiers database?). I'm wondering since you point that the recovered public key is correct, so the signature is actually valid.

From lnd docs:

VerifyMessage verifies a signature over a msg. The signature must be zbase32 encoded and signed by an active node in the resident node's channel database. In addition to returning the validity of the signature, VerifyMessage also returns the recovered pubkey from the signature.

@valentinewallace I double checked this against lnd and c-lightning. Both return:

{ "pubkey": our_public_key, "verified": false }

However, in c-lightning, if our public key is passed as optional parameter then the message is verified.

Checking c-lightning's docs, we can find the following:

As a special case, if pubkey is not specified, we will try every known node key (as per listnodes), and verification succeeds if it matches for any one of them.

And, If I check listnodes, the list is empty (even though the node is one of our peers), so I'm guessing that's the root of the issue. The pubkey is correctly recovered in both cases, but the node seems not to be part of neither of the peers known nodes, so it fails validation against known nodes.

Furthermore, querying describegraph in lnd's node does not show our testing node, which I'd say backs the aforementioned claim.

I'm not sure why that's happening though. Any thoughts?

Asked around #c-lightning on IRC, looks like a channel announced is required to show-up in listnodes, and I'm assuming the same may apply to describegraph in lnd:

20:12 <sr_gi> Hey guys, what is needed for a node to show in listnodes? I was doing some testing using Polar and connecting a c-lightning node to a rust-lightning node and even though they were connected the peer did no show up in the list. Is a channel required or something on those lines? 20:14 <@cdecker> An announced channel is required, yes

As discussed offline, this seems like a reasonable explanation for why lnd's giving verified: false :) Though it might be reasonable to test at some point with a public channel using the sample.

sr-gi · 2021-04-15T20:43:52Z

Sorry to nit you to death on grammar, wouldn't be so pressed if it weren't public docs.

No worries, I actually appreciated it.

Addressed the grammar issues you pointed out and a couple more related I found.

TheBlueMatt · 2021-04-16T02:05:56Z

lightning/src/util/message_signing.rs

+// You may not use this file except in accordance with one or both of these
+// licenses.
+
+//! Ligthning message signing and verification lives here. These tools can be used to sign messages using the node's


Lightning, transpose the t and h

The original work is licensed dual MIT+Apache-2 just like us, so the license header should not only mention MIT.

ariard

Code Review ACK 7bcf5a1

ariard · 2021-04-17T00:47:07Z

lightning/src/util/message_signing.rs

+use bitcoin::secp256k1::recovery::{RecoverableSignature, RecoveryId};
+use bitcoin::secp256k1::{Error, Message, PublicKey, Secp256k1, SecretKey};
+
+static LN_MESSAGE_PREFIX: &[u8] = b"Lightning Signed Message:";


Bind message to a lightning signing context to avoid replay of signed data in another context with same key material ? Though w.r.t to this concern our message API signing is safe as this is always committed by the implementation and not an argument caller, not sure if it's worthy to document.

Do we have risks of harmful API misuses we should document ? I think we're good, can't foresee real ones.

Yeah, that's the purpose of the prefix AFAIK. I can write a comment there stating the purpose of the prefix if you feel it's necessary.

Do we have risks of harmful API misuses we should document ? I think we're good, can't foresee real ones.

I cannot see any either.

TheBlueMatt reviewed Mar 16, 2021

View reviewed changes

lightning/Cargo.toml Outdated Show resolved Hide resolved

sr-gi force-pushed the 843-ln-signing branch from d37d536 to 668915c Compare March 22, 2021 10:16

Adds zbase32 encoding

c7f405e

sr-gi force-pushed the 843-ln-signing branch from 668915c to 8013667 Compare March 22, 2021 10:27

TheBlueMatt reviewed Mar 22, 2021

View reviewed changes

ariard reviewed Mar 24, 2021

View reviewed changes

sr-gi force-pushed the 843-ln-signing branch from 40eccf2 to 5706495 Compare March 26, 2021 11:08

sr-gi force-pushed the 843-ln-signing branch from 5706495 to 7ca8761 Compare March 31, 2021 09:39

TheBlueMatt reviewed Mar 31, 2021

View reviewed changes

sr-gi force-pushed the 843-ln-signing branch from 7ca8761 to 27d3655 Compare April 1, 2021 14:40

TheBlueMatt reviewed Apr 1, 2021

View reviewed changes

sr-gi force-pushed the 843-ln-signing branch from 27d3655 to cf384a3 Compare April 1, 2021 15:44

TheBlueMatt approved these changes Apr 1, 2021

View reviewed changes

TheBlueMatt added this to the 0.0.14 milestone Apr 5, 2021

valentinewallace reviewed Apr 9, 2021

View reviewed changes

sr-gi force-pushed the 843-ln-signing branch from cf384a3 to a747046 Compare April 15, 2021 20:41

TheBlueMatt reviewed Apr 16, 2021

View reviewed changes

sr-gi and others added 5 commits April 16, 2021 07:34

Adds lightning message signing/verification/pk_recovery

6f7a2bc

Fix a number of bugs in zbase32 and add a fuzzer which caught them.

7c9302f

Replace spaces with tabs in zbase32.rs

d871645

Correct license header in zbase32 to match original work

f13e38c

The original work is licensed dual MIT+Apache-2 just like us, so the license header should not only mention MIT.

Changes zbase32 crate from pub to pub(crate)

7bcf5a1

sr-gi force-pushed the 843-ln-signing branch from a747046 to 7bcf5a1 Compare April 16, 2021 05:35

ariard reviewed Apr 17, 2021

View reviewed changes

valentinewallace approved these changes Apr 19, 2021

View reviewed changes

TheBlueMatt merged commit e6c9228 into lightningdevkit:main Apr 20, 2021

Adds lightning message signing/verification/pk_recovery #844

Adds lightning message signing/verification/pk_recovery #844

Uh oh!

Conversation

sr-gi commented Mar 16, 2021

Uh oh!

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sr-gi Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sr-gi commented Mar 26, 2021

Uh oh!

TheBlueMatt commented Mar 26, 2021

Uh oh!

TheBlueMatt commented Mar 28, 2021

Uh oh!

sr-gi commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sr-gi Mar 26, 2021 •

edited

Loading

TheBlueMatt Mar 26, 2021 •

edited

Loading

sr-gi commented Mar 31, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

sr-gi Apr 18, 2021 •

edited

Loading

valentinewallace Apr 19, 2021 •

edited

Loading

sr-gi Apr 17, 2021 •

edited

Loading