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

feat(codec): Configure max request message size #1274

Merged
merged 10 commits into from
Feb 17, 2023

Conversation

aoudiamoncef
Copy link
Contributor

Motivation

In gRPC specs, by default there is a limit for the length of messages(4MB). Actually, Tonic Codec doesn't set a limit which could cause security concerns. If an attacker decides to send us heavy messages, It'll be decoded and could fill all available RAM.

See #1097

Solution

Support changing the max message size via a configuration. Set the limit to 4MB by default.

resolves #1097

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

This looks awesome! Really great work, I think I would like to see one integration test as well that shows that we can produce this error and then continue to make normal requests etc. I think once we have that we should be good, I would also try to think about where we could document this that would be helpful.

@aoudiamoncef
Copy link
Contributor Author

aoudiamoncef commented Feb 16, 2023

Hi @aoudiamoncef,

I have an issue with tonic-build client, I added the two parameters to the client config, but I got an error when I tried to add it in the generator side. I haven't any issue in the server side.

image

@LucioFranco
Copy link
Member

@aoudiamoncef commented I think those two lines in the client.rs of tonic-build need to just remove the self.inner = part and it should work?

@aoudiamoncef
Copy link
Contributor Author

@aoudiamoncef commented I think those two lines in the client.rs of tonic-build need to just remove the self.inner = part and it should work?

I tired a couple of things without success. It's strange, as we have similar code above

@LucioFranco
Copy link
Member

@aoudiamoncef I think you need to call the fn on Grpc?

Like this
image

@aoudiamoncef
Copy link
Contributor Author

@aoudiamoncef I think you need to call the fn on Grpc?

Like this image

Ohhhhh yeah, copy and past is evil, my brain is fucked

@LucioFranco
Copy link
Member

Ohhhhh yeah, copy and past is evil, my brain is fucked

All good xD

@aoudiamoncef
Copy link
Contributor Author

aoudiamoncef commented Feb 16, 2023

Are you interested by rust-toolchain.toml which align rust toolchain with the Tonic CI ?

[toolchain]
channel = "1.60"

@aoudiamoncef
Copy link
Contributor Author

aoudiamoncef commented Feb 16, 2023

@aoudiamoncef I think you need to call the fn on Grpc?
Like this image

Ohhhhh yeah, copy and past is evil, my brain is fucked

I was fucked by inner which has a nested inner. In my head, it's was the same integration as the server.rs

@aoudiamoncef aoudiamoncef marked this pull request as ready for review February 16, 2023 16:57
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

All the docs should state the default value beyond that everything looks pretty good. Left a few comments.

tonic/src/client/grpc.rs Outdated Show resolved Hide resolved
tonic/src/client/grpc.rs Show resolved Hide resolved
tonic/src/client/grpc.rs Show resolved Hide resolved
tonic/src/server/grpc.rs Outdated Show resolved Hide resolved
tonic/src/server/grpc.rs Show resolved Hide resolved
@LucioFranco
Copy link
Member

Also you will need to rebase your branch and run cargo test --all and commit the generated code for the sub libraries.

@LucioFranco
Copy link
Member

I wrote some instructions here https://github.com/hyperium/tonic/blob/master/CONTRIBUTING.md#generated-code

@aoudiamoncef
Copy link
Contributor Author

@LucioFranco, I corrected all the typos, tests passes locally and I tested the new feature in my projet with success 💯

@aoudiamoncef
Copy link
Contributor Author

The CI is green https://github.com/hyperium/tonic/actions/runs/4198616103

I just rebased from master to include the changes in the contributing file.

@LucioFranco LucioFranco added this pull request to the merge queue Feb 17, 2023
Merged via the queue into hyperium:master with commit 9f716d8 Feb 17, 2023
@BoynChan
Copy link

hi @LucioFranco, sorry for bother you in a merged PR, here's I want to ask a short but essential question. I want to know does this change have any document or explain about how I can configure when I create an tonic grpc client?

@aoudiamoncef
Copy link
Contributor Author

I'll try to write an example to show the usage of this feature

LucioFranco added a commit that referenced this pull request Mar 29, 2023
This PR adds new defaults for both client and server max
encoding/decoding message size limits. By default, the max message
decoding size is `4MB` and the max message encoding size is
`usize::MAX`.

This is follow up work from #1274

BREAKING: Default max message encoding/decoding limits
LucioFranco added a commit that referenced this pull request Mar 30, 2023
* feat(core): Default encoding/decoding limits

This PR adds new defaults for both client and server max
encoding/decoding message size limits. By default, the max message
decoding size is `4MB` and the max message encoding size is
`usize::MAX`.

This is follow up work from #1274

BREAKING: Default max message encoding/decoding limits

* update generated code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Config max request message size to fend off potential malicious attacks
3 participants