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

Add buffer_size_limit config option for providers #233

Merged

Conversation

joechrisellis
Copy link
Contributor

The buffer size limit can be used to cap the maximum allowed buffer size
sent in a response. Requests that ask for buffers larger than this value
will be automatically rejected.

Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Thanks! That will be useful for generate_random. Added an alternative that might simplify the change. See what you think.

@@ -53,6 +53,9 @@ const WIRE_PROTOCOL_VERSION_MAJOR: u8 = 1;
/// Default value for the limit on the request body size (in bytes) - equal to 1MB
const DEFAULT_BODY_LEN_LIMIT: usize = 1 << 19;
Copy link
Member

Choose a reason for hiding this comment

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

We should probably fix this one as well 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, was going to do that in a separate patch. Let me do that now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

config.toml Outdated
@@ -64,10 +64,15 @@ provider_type = "MbedCrypto"
# (Required) Name of key info manager that will support this provider.
key_info_manager = "on-disk-manager"

# (Optional) Decide how large (in bytes) buffers inside responses from this provider can be.
# Requests that ask for buffers larger than this threshold will be rejected. Defaults to 1MB.
#buffer_size_limit = 1048576
Copy link
Member

Choose a reason for hiding this comment

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

I think for this option it is fine if we only have one global config option declared in the core_settings and that work for all the service.
That way, you just have to add the option to the GlobalConfig and access it from there directly when you need it 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it might be a good idea to have a separate one for each provider in case the provider itself has specific limitations that we might want to accommodate for. Although thinking about it, I can't really come up with anything concrete off the top of my head. I'll change this.

Copy link
Member

Choose a reason for hiding this comment

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

I guess as all providers are running with the same allocated memory, that's fine to assume that they would all have similar limitations! Also, did not want to increase the configuration complexity for such a small feature 😋

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM!

@joechrisellis joechrisellis force-pushed the add_buffer_size_limit branch 2 times, most recently from 849f375 to 8a48384 Compare August 26, 2020 17:12
The buffer size limit can be used to cap the maximum allowed buffer size
sent in a response. Requests that ask for buffers larger than this value
will be automatically rejected.

Signed-off-by: Joe Ellis <[email protected]>
Copy link
Member

@hug-dev hug-dev left a comment

Choose a reason for hiding this comment

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

Looks good!

@hug-dev hug-dev requested a review from ionut-arm August 26, 2020 17:39
@ionut-arm ionut-arm merged commit 3ea3461 into parallaxsecond:master Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants