Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: don't use WakuMessageSize in req/resp protocols #2601
fix: don't use WakuMessageSize in req/resp protocols #2601
Changes from 1 commit
81d5121
f9b8c35
7a466a6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
I think adding state and dynamic calculation here is overkill. The idea of the max stream read size was simply to have some reasonable safety limit that will prevent unrealistically (read: adversarially) large requests (which presumably could be an attack vector). We could have just used a very, very big magic number, but opted to initialise our constants with some value based off the
MaxWakuMsgSize
with safety margins to show that there was some rhyme and reason behind our thinking. Because this max msg size value is now configurable, our generous high limit may indeed now not be generous enough for differently configured nodes - at least in the lightpush case. However, even here the node could likely not configure message size to anything larger than the underlying libp2p limit (of 1MB). If we want to change this to be future proof, I'd rather keep using the underlying libp2p constant (1 MB) for all nodes and keep the simplicity of pure constants or just ignore the max rpc size limit (set to-1
all around). The latter could presumably expose node to more attacks, though.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.
yep, realised this ... will change it to -1.
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.
I am wondering what kind of attacks, because read would anyways be done by the libp2p layer ir-respective of length passed. It is only the length check for the message that is done based on what is passed to it.
We could keep it to 1MB, but then it is going to cause issues for protocols like Store where response can include 100*WakuMessageSize bytes which would definitely be more than 1MB even with default value.
If we really have to deal with attacks, then maybe there should be some sort of length negotiation as part of metadata protocol or some other protocol when nodes connect to each other. This i feel is overkill at this point and can be taken up if we notice any vulnerabilities.
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.
Also, if this is a hard limit set by libp2p...then maybe we shouldn't let users configure anything more than 1MB-WakuHeaders as maxMsgSize.
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.
Mmm. Indeed, not sure if possible impacts. It seems to me if someone were to pack the length prefix (or just send a very large amount of bytes), we'd have some potential to DoS the stream. This is a sanity check for the buffer length and if not reasonable the stream reader exits: https://github.com/vacp2p/nim-libp2p/blob/b30b2656d52ee304bd56f8f8bbf59ab82b658a36/libp2p/stream/lpstream.nim#L238-L239
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.
Which is why for store the max rpc size is
MaxPageSize*MaxWakuMsgSize + safety_margin
. Agree though. This is complicated by configurable sizes.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.
Agree.