-
Notifications
You must be signed in to change notification settings - Fork 281
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
pubsub: signing policy -- #294 + editorial changes #299
Conversation
Co-authored-by: Jacek Sieka <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, and thanks for making the necessary changes @raulk
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good; some minor comments, probably worth addressing.
@vyzo all comments addressed. Was your review an approval, subject to the edits you suggested? Or do you want to do another full review? |
authorship is preserved. | ||
|
||
The `seqno` field (optional) is a 64-bit big-endian uint that is a linearly | ||
increasing number that is unique among messages originating from each given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afair rust-libp2p puts a random number here? is linearly increasing a "hard" requirement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really, it just has to be unique within the time cache window.
**Signature verification.** There are two configurations possible: | ||
|
||
* `Strict`: either expect or not expect a signature. | ||
* `Lax` (legacy, insecure, underterministic, to be deprecated): accept a signed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to export some of this language to a separate section which simply says that all fields that are set must be valid for their type - seqno
must be 8 bytes, if the signature is set it must be valid - these would be pre-conditions to even consider the message for propagation before the "modes" are considered - the "lax" vs "strict" distinction then becomes a requirement for a signature or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that would be a welcome improvement. All protobuf types are bytes
and string
s, which express no constraints in and of themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind submitting a PR against master when this lands (i.e. now)?
This introduces editorial changes on top of #294. We now concentrate all our additions in
pubsub.md
.@protolambda, I wanted to target your branch, but it's in a fork.
cc @arnetheduck