Skip to content

Conversation

@eziskind
Copy link
Contributor

This change allows configuring the maximum size of the ClientHello that the TLS inspector will process via the TlsInspector proto configuration. The default value is 16KiB. The initial_read_buffer_size now defaults to this configurable maximum size. No change to current behavior if the field is unset.

Risk Level: low
Testing: unit tests

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #42278 was opened by eziskind.

see: more, trace.

Copy link
Member

@botengyao botengyao left a comment

Choose a reason for hiding this comment

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

Just catches this from my another PR, thanks for adding this support!

/wait

Signed-off-by: Elisha Ziskind <[email protected]>
Signed-off-by: Elisha Ziskind <[email protected]>
@agrawroh
Copy link
Member

I am curious what's the motivation behind making this configurable. The current value is per the RFC specs. In what cases do we wanna change this?

@eziskind
Copy link
Contributor Author

I am curious what's the motivation behind making this configurable. The current value is per the RFC specs. In what cases do we wanna change this?

We want to be able to use a lower default value to limit abuse (eg. from DoS attacks) and only allow higher values (up to the RFC spec of 16K) when valid use-cases arise.

Signed-off-by: Elisha Ziskind <[email protected]>
@agrawroh
Copy link
Member

I am curious what's the motivation behind making this configurable. The current value is per the RFC specs. In what cases do we wanna change this?

We want to be able to use a lower default value to limit abuse (eg. from DoS attacks) and only allow higher values (up to the RFC spec of 16K) when valid use-cases arise.

Make sense. Thanks for sharing.

KBaichoo
KBaichoo previously approved these changes Nov 27, 2025
Copy link
Contributor

@KBaichoo KBaichoo left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

Signed-off-by: Elisha Ziskind <[email protected]>
@agrawroh
Copy link
Member

This still needs an API LGTM.
cc @abeyad @ggreenway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants