-
Notifications
You must be signed in to change notification settings - Fork 53
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
feat: Added message size check before relay for lightpush #2671
Conversation
You can find the image built from this PR at
Built from 759448a |
You can find the image built from this PR at
Built from 759448a |
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.
Congratulations on your first PR! Great to see that you're already getting the hang of Nim language and syntax. Note my comment below - I think it simplifies what needs to be done in this PR quite significantly. However, this did point out an unnecessary architectural complexity introduced by the pushHandler
pattern.
My suggestion:
- simplify this PR by adding Relay validation in the
pushHandler
as described in my comment below - for the RLN-proofs-as-a-service work, first remove the
pushHandler
pattern by adding a reference towakuRelay
into the Lightpush module and moving all push logic to this module itself. Let me know if this next step is unclear.
@@ -26,6 +26,25 @@ type WakuLightPush* = ref object of LPProtocol | |||
pushHandler*: PushMessageHandler | |||
requestRateLimiter*: Option[TokenBucket] | |||
|
|||
proc validateMessage*( |
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.
Ah, the idea is to use the exact same validation function that Relay would use when it publishes the message - not create a new validation function (which would have to be maintained separately and would invariably at some point miss validation edge cases enforced by the relay validator).
In other words, we should use:
nwaku/waku/waku_relay/protocol.nim
Lines 225 to 252 in b46226f
proc validateMessage*( | |
w: WakuRelay, pubsubTopic: string, msg: WakuMessage | |
): Future[Result[void, string]] {.async.} = | |
let messageSizeBytes = msg.encode().buffer.len | |
let msgHash = computeMessageHash(pubsubTopic, msg).to0xHex() | |
if messageSizeBytes > w.maxMessageSize: | |
let message = fmt"Message size exceeded maximum of {w.maxMessageSize} bytes" | |
error "too large Waku message", | |
msg_hash = msgHash, | |
error = message, | |
messageSizeBytes = messageSizeBytes, | |
maxMessageSize = w.maxMessageSize | |
return err(message) | |
for (validator, message) in w.wakuValidators: | |
let validatorRes = await validator(pubsubTopic, msg) | |
if validatorRes != ValidationResult.Accept: | |
if message.len > 0: | |
error "invalid Waku message", msg_hash = msgHash, error = message | |
return err(message) | |
else: | |
## This should never happen | |
error "uncertain invalid Waku message", msg_hash = msgHash, error = message | |
return err("validator failed") | |
return ok() |
Of course, that requires a reference to the instantiated wakuRelay
object that will eventually publish this message. This is currently not available in the lightpush module, but in the pushHandler provided to lightpush by the waku node here:
Lines 917 to 929 in b46226f
pushHandler = proc( | |
peer: PeerId, pubsubTopic: string, message: WakuMessage | |
): Future[WakuLightPushResult[void]] {.async.} = | |
let publishedCount = | |
await node.wakuRelay.publish(pubsubTopic, message.encode().buffer) | |
if publishedCount == 0: | |
## Agreed change expected to the lightpush protocol to better handle such case. https://github.com/waku-org/pm/issues/93 | |
let msgHash = computeMessageHash(pubsubTopic, message).to0xHex() | |
debug "Lightpush request has not been published to any peers", | |
msg_hash = msgHash | |
return ok() |
Architecturally, I think this indirection created by the pushHandler is unnecessary. I'll add a comment on this for future work, but for now I think this PR should be as simple as adding:
node.wakuRelay.validateMessage(
in the push handler before pushing the message (and testing)
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.
@shash256 and @jm-clius : I think the biggest issue with lightpush is not that it does not pre-validates the message but in contrary it will not get noticed by the relay that the message validation done in relay protocol side is failed. It cause that we do not have meaningful answer to the lightpush client that would help error handling.
WDYT?
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.
Well, if we validate the message before attempting to relay it and return an explicit "validation error" to the client on failure, wouldn't it cover - or at least improve - this silent failure? Similar to how we return a "bad request" error to Relay API clients in REST: https://github.com/waku-org/nwaku/blob/master/waku/waku_api/rest/relay/handlers.nim#L163-L164
In other words, we add the capability on the lightpush server to explicitly detect validation errors and return a validation error to the client.
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.
If validation is done in the pushHandler
(see my comment elsewhere), changes to this file is likely unnecessary.
Opened a new PR at #2695 with signed commits enabled. Closing this one. |
Description
Added setup for message validation for lightpush service nodes to catch validation failures before relay. Fixes issue #2595
Changes