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 Transport config parameter for MaxConnsPerHost #1858

Merged
merged 8 commits into from
Jan 2, 2024

Conversation

ccapurso
Copy link
Contributor

Rendering templates within Vault Agent that contain many secrets can result in aggressively opening a very large number of TCP connections all at once. Opening this many connections essentially results in a DoS against Vault. This PR adds the ability to specify a value for MaxConnsPerHost within a client's Transport configuration. Support for this new field was only added for the Vault client.

Fixes #1840

@ccapurso ccapurso marked this pull request as ready for review December 15, 2023 17:04
@ccapurso ccapurso requested a review from a team as a code owner December 15, 2023 17:04
Copy link
Member

@lornasong lornasong left a comment

Choose a reason for hiding this comment

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

Hey @ccapurso! These changes make sense to me.

I'm not super familiar with this code so did a review by comparing how a similar field(I chose MaxIdleConnsPerHost) is implemented. I noticed a couple additional changes with MaxIdleConnsPerHost:

  • transport.go GoString() - it doesn't look like all transport fields are in the GoString. I'm not sure if it would be worthwhile to add MaxConnsPerHost here?
  • config_test.go TestParse - it looks like adding a test case for MaxConnsPerHost might keep parity?
  • cli.go ParseFlags() - would it make sense to add flag support for this field?

Totally okay ifMaxConnsPerHost doesn't make sense to be in the 3 places above, so just want to get your input.

Another question mostly because I'm not familiar with things, would you mind sharing a little bit about how you tested these changes? Appreciate it, thank you

config/convert_test.go Show resolved Hide resolved
@ccapurso
Copy link
Contributor Author

ccapurso commented Jan 2, 2024

@lornasong, thank you for the review! I have addressed the three spots you mentioned:

I tested using the reproduction steps provided in #1840 and ensured that the connections were limited based on the configured maximum.

Copy link
Member

@lornasong lornasong left a comment

Choose a reason for hiding this comment

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

thanks @ccapurso! the changes make sense to me

@ccapurso ccapurso merged commit 68c06bd into main Jan 2, 2024
54 checks passed
@ccapurso ccapurso deleted the vault-22552-max-conns branch January 2, 2024 18:46
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.

Consul template unlimitely spawn TCP connections which DDoS Consul/Vault
3 participants