Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

feat: verify signatures #20

Merged
merged 5 commits into from
Jul 8, 2019
Merged

feat: verify signatures #20

merged 5 commits into from
Jul 8, 2019

Conversation

jacobheun
Copy link
Contributor

This adds an option to pubsub, strictSigning which defaults to true. When validate is called, the signature of the message will be verified, unless strictSigning is disabled.

I originally intended to enforce this at the pubsub interface layer so that both Floodsub and Gossipsub could get the behavior out of the box, however, the implementations don't use the same message processing pipeline. Ideally, we should unify this more so that the abstraction layer can do common validation, like signature validation, before moving on to implementation specific message processing.

As it stands, validate is going to need to be called in both implementations in their respective pipelines. I will be creating PRs for both implementations.

@jacobheun jacobheun requested a review from vasco-santos July 8, 2019 08:23
Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @jacobheun !

Just found 2 minor things that I would like you to have a look

}

module.exports = {
messagePublicKey,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to export this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need to, but I could see value in having a utility for unmarshalling keys from messages, especially once key inlining is supported. We could remove it for now, but I don't think it's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

We can keep it then

SignPrefix,
Message.encode(normalizeOutRpcMessage(message))
])
// const bytesToSign = Buffer.concat([
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove these lines?

Copy link
Member

@vasco-santos vasco-santos left a comment

Choose a reason for hiding this comment

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

LGTM

@vasco-santos vasco-santos merged commit 165f01b into master Jul 8, 2019
@vasco-santos vasco-santos deleted the feat/verify-sign branch July 8, 2019 12:45
@vasco-santos vasco-santos mentioned this pull request Jul 10, 2019
2 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants