Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix signature verification in mempool #727

Merged
merged 8 commits into from
Oct 7, 2020
Merged

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Oct 2, 2020

Summary of changes
Changes introduced in this pull request:

  • Fixes signature verification in new_from_parts
  • Fixes signature generation and verification in message pool
  • Released new versions
    • Only message crate was a breaking change, and removed the types dependency from the crate (to avoid fil-proofs from being pulled into this dep tree)
    • Added CHANGELOG to message crate, to allow @rllola and @jleni to easily follow changes in releases for their usages.
  • Cleaned up and added convenience functions for verifications to make this harder to be an issue later
    • Didn't change the new_from_parts function to not verify, because it's a good sanity check for a bit, but it probably should be removed later as there is no need for it to verify.

The issue was that message Cbor was being signed, not the message cid bytes. The only borked usage was in the mempool, which is why we didn't run into this yet. I could have sworn I commented to change this, but I guess I didn't follow up.

These interfaces can definitely be cleaned up (especially in the mempool), but I'm just going to keep scope small for this PR

Reference issue to close (if applicable)

Closes #725

Other information and links

msg.signature()
.verify(umsg.as_slice(), msg.from())
.map_err(Error::Other)?;
msg.verify().map_err(Error::Other)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this error type be InvalidFromAddr rather than Other? It seems the verify fn would fail if the from address is not the correct address which seems more of an InvalidFromAddr error type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the only error that can happen with signature validation. You also lose the error message for no benefit.

Side note, I don't like the error type in this crate, would be nice to improve later.

@ec2
Copy link
Member

ec2 commented Oct 6, 2020

I don't get why you moved valid_for_block_inclusion from being a method on the Message trait into the msgpool

@austinabell
Copy link
Contributor Author

I don't get why you moved valid_for_block_inclusion from being a method on the Message trait into the msgpool

The reason is because filecoin proofs is brought into the dependency tree of messages and it makes it unusable by others using published crate for no benefit to us (because proofs dependency only works on certain targets).

Idea is just to keep the message crate lean, and separating proofs logic by a feature was not something I was eager to do now, since there was no need.

@austinabell austinabell merged commit 15eb3f0 into main Oct 7, 2020
@austinabell austinabell deleted the austin/ecrecovercleanup branch October 7, 2020 11:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

verify_secp256k1_sig use an invalid hash for signature
3 participants