fix: Throw Error on unsuported addresses in `is_signed_by_address()` #819

Shatnerz · 2022-02-05T21:37:44Z

This is a direct response to this comment: #684 (comment)

Currently unsupported addresses simply return false which is highly misleading as it is entirely possible that the keys related to a given address were used to sign the message.

If this is successful I can follow up with

a method to check if a PublicKey is associated with an address

tcharding

Nice PR, thanks!

tcharding · 2022-02-07T11:09:44Z

src/util/misc.rs

@@ -63,6 +67,8 @@ mod message_signing {
                MessageSignatureError::InvalidLength => write!(f, "length not 65 bytes"),
                MessageSignatureError::InvalidEncoding(ref e) => write!(f, "invalid encoding: {}", e),
                MessageSignatureError::InvalidBase64 => write!(f, "invalid base64"),
+                MessageSignatureError::UnableToRecoverPubKey(ref e) => write!(f, "error recovering public key: {}", e),
+                MessageSignatureError::UnsupportedAddressType(ref e) => write!(f, "unsupported address type: {}", e),


The inner type is not an error but an AddressType, perhaps ty or addr would be better than e?

Yes good catch. I hope an explicit address_type is acceptable.

@Kixunil I was distracted and posted my reply to a different comment here. You can ignore. I immediately deleted the comment, but you are too fast.

tcharding · 2022-02-07T11:10:46Z

src/util/misc.rs

+            let msg = secp256k1::Message::from_slice(&msg_hash[..])
+                .map_err(MessageSignatureError::UnableToRecoverPubKey)?;


No need for the explicit map_err here, the ? does the conversion for us because there is only one variant of MessageSignatureError with an inner secp error.

Wouldn't an explicit map be better here? It helps differentiate between InvalidEncoding(secp256k1::Error) and UnableToRecoverPubKey(secp256k1::Error)

There are two actually.

Message::from_slice cannot fail. This should actually be an .expect but ? was shorter to type and it doesn't matter anyway.

I can add the expect and then remove UnableToRecoverPublicKey is so desired.

What the hell, it only takes a second. Changed in 1be1bb72d1a

Excluding the fact that I was too blind to see the second variant I cannot work out why this works. If I purposefully break the call to from_slice the compiler returns InvalidEncoding even though the error conversion is ambiguous. I did not think this sort of ambiguity would get past rustc?

@tcharding There exists a conversion from secp256k1::Error for MessageSignatureError::InvalidEncoding so this can happen implicitly since there is only one path as there was no conversion of the same error into MessageSignatureError::UnableToRecoverPubKey

impl From<secp256k1::Error> for MessageSignatureError { fn from(e: secp256k1::Error) -> MessageSignatureError { MessageSignatureError::InvalidEncoding(e) } }

Thanks for pointing this out and not telling me to read the code more carefully, twice in one review thread :)

tcharding · 2022-02-07T11:14:46Z

src/util/misc.rs

@@ -314,6 +318,10 @@ mod tests {
        use core::str::FromStr;
        use secp256k1;

+        use util::address::AddressType;
+
+        use super::MessageSignatureError;


We could use use super::* at the top of the tests module and then wouldn't need this import statement.

Kixunil

Looks good so far. I have trouble focusing rn so I will need to check again later to be confident I didn't miss anything.

Kixunil · 2022-02-07T12:11:01Z

src/util/misc.rs

-                Some(AddressType::P2tr) => false,
-                None => false,
-            })
+                Some(address_type) => Err(MessageSignatureError::UnsupportedAddressType(address_type)),


Some(ty @ AddressType::P2sh) | Some(ty @ AddressType::P2wpkh) | Some(ty @ AddressType::P2wsh) | Some(ty @ AddressType::P2tr) would be more refactor-proof but I guess it's unlikely a new address will be introduced together with a signature scheme?

If we base the current behaviour off of this comment: #684 (comment)

There is no signmessage standard for non-pkh address types.

Then all other address types would be unsupported and we wouldn't need to explicitly list each. I can go either way. No strong opinions from my end.

Yes, it is very unlikely that any new signature schemes are going to show up which support onlly specific address types.

@Shatnerz I meant a hypothetical scenario when a new address is added and a signature scheme too. Since this is unlikely, let's keep it as is.

Kixunil

ACK 1be1bb72d1abba4d2addaee8c208ab1f8c650fd4

I think it'd be best to address my comments in new PRs. Especially for conversions we could inspect the whole crate for these and improve them together.

@tcharding am I guessing correctly that this would be well-suited for you?

Kixunil · 2022-02-08T10:19:36Z

src/util/misc.rs

-            let msg = secp256k1::Message::from_slice(&msg_hash[..])?;
+        ) -> Result<PublicKey, MessageSignatureError> {
+            let msg = secp256k1::Message::from_slice(&msg_hash[..])
+                .expect("cannot fail");


Actually just let msg = secp256k1::Message::from(msg_hash); should be possible if we activate bitcoin_hashes feature of secp256k1 .

Oh nice, I forgot that we finally added that feature. Yes, let's do this.

Opened #824 for that.

Nice catch. I was looking for the fewest changes possible but this is definitely an improvement.

@apoelstra @Kixunil I pushed the changes in 8a05ca042d4e8

Kixunil · 2022-02-08T10:25:07Z

src/util/misc.rs

@@ -335,9 +338,14 @@ mod tests {
        let p2pkh = ::Address::p2pkh(&pubkey, ::Network::Bitcoin);


Not relevant to this PR but I noticed we don't have Ok(false) test for broken signatures.

@Kixunil I created #825 to test broken signatures

tcharding · 2022-02-08T18:24:17Z

@tcharding am I guessing correctly that this would be well-suited for you?

I don't have anything specific in mind at the moment but I'm starting to get the feeling that our error handling needs a bit of love. After MSRV bump though I'd say because of Rust 1.34 changes to the error trait.

Kixunil · 2022-02-08T20:15:35Z

@tcharding yes it does need love but these cases don't require MSRV bump (unless ThirtyTwoByteHash got introduced in MSRV-breaking secp256k1, which I didn't check and guess is not the case).

tcharding · 2022-02-09T04:54:24Z

Oh cool, thanks, so we can start thinking about this right away.

Kixunil

ACK 8a05ca042d4e8e28a60efecaf7b542ef3232ec52

apoelstra · 2022-02-09T21:02:56Z

Can you rearrange the code so that the expensive pubkey recovery step is not done unless the result will actually be used?

Can you also squash all these commits? It is hard to read PRs that introduce code in one commit and then remove them in the next.

Shatnerz · 2022-02-09T21:13:38Z

Can you rearrange the code so that the expensive pubkey recovery step is not done unless the result will actually be used?

Yeah, no problem

Can you also squash all these commits? It is hard to read PRs that introduce code in one commit and then remove them in the next.

Yeah no problem. I'm use to gitlab squashing commits. If I'm not sure a change will be accepted I leave the commit so its easy to revert. Squashing at the end only takes a moment

apoelstra · 2022-02-09T21:18:07Z

@Shatnerz makes sense re "not sure if it'll be reverted or not". In this case I think the changes are good, and the extra commits make reviewing harder for new reviewers.

apoelstra

ACK 31ec64d

apoelstra · 2022-02-09T21:29:10Z

Looks like core2 has broken somehow in the last 15 minutes :(.

Shatnerz · 2022-02-09T21:38:08Z

Looks like core2 has broken somehow in the last 15 minutes :(.

@apoelstra This is a bit outside my domain so it will likely take me awhile and I can't seem to reproduce locally. Advice on how to reproduce locally would be great.

Is this breakage related to enabled the bitcoin_hashes feature? Should I just revert that change?

apoelstra · 2022-02-09T21:49:53Z

@Shatnerz yeah, let's try dropping that (lol, sorry, and after I said "just squash stuff, we won't revert"..) But I suspect that things have broken unrelatedly to your PR.

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz · 2022-02-09T22:08:24Z

yeah, let's try dropping that (lol, sorry, and after I said "just squash stuff, we won't revert"..) But I suspect that things have broken unrelatedly to your PR.

@apoelstra yes, just tested it out in my fork. 79cee4c should solve the CI issues

apoelstra · 2022-02-09T22:20:33Z

It does seem to be working. Quite concerning that the bitcoin_hashes feature seems to blow up CI like that.

Shatnerz · 2022-02-09T22:44:00Z

It does seem to be working. Quite concerning that the bitcoin_hashes feature seems to blow up CI like that.

A bigger change for another day. At least the CI pipeline is catching these. It'll be nice to clean up implicit side effects like this eventually so its easier to make changes.

Shall we create an issue for this? I'm not sure how to label it, "Enable secp256k1 feature bitcoin_hashes without breaking CI"? You can link to https://github.com/rust-bitcoin/rust-bitcoin/actions/runs/1820479684 as an example of what failures it introduces.

apoelstra

ACK 79cee4c

Kixunil

ACK 79cee4c

Kixunil · 2022-02-21T14:52:03Z

The failure seems to be related to #758 and I'm not sure reexporting everything but a kitchen sink is a good approach. Sadly, probably the best solution requires yet-to-be-released cargo version.

…igned_by_address` e391ce9 test: Add a test for incorrect message signature (Andrew Ahlers) Pull request description: In response to this comment: #819 (comment) This should be straightforward. Let me know if there are any style issues. I tried to keep things similar to the existing test while cutting out any extra cruft to keep things small. ACKs for top commit: apoelstra: ACK e391ce9 Kixunil: ACK e391ce9 dr-orlovsky: ACK e391ce9 Tree-SHA512: 47296a7e0b2f45d5e50f507727ae4360686730a386f37dedfd1360b8cdf4b9dd3ce3bb5d05ea630177379ce4109059b6924fa362396b984ebab0ed1754318627

51fef76 feat: Add Address.is_related_to_pubkey() (Andrew Ahlers) Pull request description: ## Motivation This is addressing the second half of this comment: #684 (comment) > but would accept a PR (or two PRs) that returns Result<bool, UnsupportedAddress> and a method to check if a PublicKey is associated with an address. (The first half was addressed [here](#819)) These changes will help build out and improve message signature verification. We don't necessarily need to add it to this crate but it allows for easy verification with something such as: 1. recovering a pubkey 2. checking if that pubkey relates to the given address ## Possible Improvements - There is likely a better name than `is_related_to_secp256k1_key()` - This could drop the `secp256k1` part of the name and take in a Pubkey enum that also supports Schnorr pubkeys and then this could be used for taproot addresses as well. This felt like a much larger change that will likely get turned down. Verifying taproot is simple enough and if absolutely desired, similar functions can be added for schnorr keys (tweaked and untweaked) ACKs for top commit: Kixunil: ACK 51fef76 for merging after TR apoelstra: ACK 51fef76 Tree-SHA512: c9ab8c0f101fb4c647713e7f500656617025d8741676e8eb8a3132009dde9937d50cf9ac3d8055feb14452324a292397e46639cbaca71cac77af4b06dc42d09d

Shatnerz force-pushed the fix/error-on-unsupported-addresses branch 4 times, most recently from c271b83 to 782182e Compare February 5, 2022 22:27

tcharding reviewed Feb 7, 2022

View reviewed changes

Shatnerz force-pushed the fix/error-on-unsupported-addresses branch 2 times, most recently from 13deba9 to 2f34b8e Compare February 7, 2022 12:06

Kixunil reviewed Feb 7, 2022

View reviewed changes

Shatnerz force-pushed the fix/error-on-unsupported-addresses branch from 2f34b8e to 2bc9115 Compare February 7, 2022 12:35

Kixunil previously approved these changes Feb 8, 2022

View reviewed changes

Kixunil mentioned this pull request Feb 9, 2022

Use infallible conversions from hashes to secp256k1::Message #824

Closed

Shatnerz dismissed Kixunil’s stale review via 8a05ca0 February 9, 2022 19:10

Kixunil previously approved these changes Feb 9, 2022

View reviewed changes

Shatnerz mentioned this pull request Feb 9, 2022

test: Add a test for incorrect message signature in is_signed_by_address #825

Merged

Shatnerz dismissed Kixunil’s stale review via 31ec64d February 9, 2022 21:16

Shatnerz force-pushed the fix/error-on-unsupported-addresses branch from 8a05ca0 to 31ec64d Compare February 9, 2022 21:16

apoelstra previously approved these changes Feb 9, 2022

View reviewed changes

fix: Error on unsuported addresses in is_signed_by_address

79cee4c

Inspired by this comment: rust-bitcoin#684 (comment)

Shatnerz dismissed apoelstra’s stale review via 79cee4c February 9, 2022 22:06

Shatnerz force-pushed the fix/error-on-unsupported-addresses branch from 31ec64d to 79cee4c Compare February 9, 2022 22:06

apoelstra approved these changes Feb 9, 2022

View reviewed changes

Kixunil approved these changes Feb 9, 2022

View reviewed changes

apoelstra mentioned this pull request Feb 14, 2022

"bitcoin_hashes" feature in rust-secp seems to break CI #830

Closed

apoelstra merged commit 0c5b695 into rust-bitcoin:master Feb 14, 2022

Shatnerz mentioned this pull request Feb 20, 2022

feat: Add Address.is_related_to_*_key() #839

Merged

tcharding mentioned this pull request Feb 21, 2022

Add bitcoin_hashes features rust-bitcoin/rust-secp256k1#408

Closed

		let msg = secp256k1::Message::from_slice(&msg_hash[..])
		.map_err(MessageSignatureError::UnableToRecoverPubKey)?;

		@@ -335,9 +338,14 @@ mod tests {
		let p2pkh = ::Address::p2pkh(&pubkey, ::Network::Bitcoin);

fix: Throw Error on unsuported addresses in is_signed_by_address() #819

fix: Throw Error on unsuported addresses in is_signed_by_address() #819

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

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!

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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

fix: Throw Error on unsuported addresses in `is_signed_by_address()` #819

fix: Throw Error on unsuported addresses in `is_signed_by_address()` #819